Skip to content

fix(desktop): gate notification content on Runtime Host privacy policy - #5002

Open
Adarsh-Me wants to merge 1 commit into
apache:mainfrom
Adarsh-Me:fix/notifications-privacy-authority
Open

fix(desktop): gate notification content on Runtime Host privacy policy#5002
Adarsh-Me wants to merge 1 commit into
apache:mainfrom
Adarsh-Me:fix/notifications-privacy-authority

Conversation

@Adarsh-Me

Copy link
Copy Markdown
Contributor

Summary

Fixes #4981. The run-ended notification gated content-bearing banners (session name + reply preview, shown outside the app in Notification Center / lock screen) on settings.privacy.incognitoActive — but the local settings copy never receives privacy updates: clientOwnedSettingsPatch excludes the section and projectClientOwnedSettings keeps the host's copy. After a user enabled incognito, the stale local copy kept reading false and the banner kept exposing content.

registerNotificationsIpc now takes an optional privacyAuthority. When provided, resolveNotificationIncognito() reads incognito from the authority and the local copy is never consulted for this decision; when the authority rejects, the notification is suppressed rather than risked (fail-closed); callers without the dep keep the existing local-copy behavior. The resolver lives in notifications-policy.ts beside the rest of the gating logic so it stays unit-testable under plain node --test. Boot wires an adapter that queries every ready host's runtime.policy and suppresses if any holds incognito — the same client.queryRuntimePolicy().policy.privacy path runtime-host-search-ipc-main.ts already uses. No polling, no subscription: the authority is read lazily at notification time, matching the existing "read the toggle lazily" design.

Verification

  • New unit test in notifications-policy.test.ts covers all four branches: authority true overrides a stale local false; authority false overrides a stale local true; absent dep preserves the local-copy gate in both directions; a rejecting authority suppresses. Full file: 4/4 pass, run against the compiled output (node --test dist/main/__tests__/notifications-policy.test.js).
  • biome check clean on all four changed files; desktop build:main compiles the changed files with no new errors (pre-existing missing-workspace errors for unbuilt UI packages only).
  • Not reproduced end-to-end with a live incognito session (same caveat the issue carries); the wiring matches the verified search-ipc precedent for the same authority read.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope:

Checklist

  • Tests cover the change and fail without it — without the authority, a stale false local copy reaches the gate and the banner is raised; the authority-true case asserts what the resolver returns in place of that read. The gate predicate itself was already covered and is unchanged.
  • Lint, format, typecheck and the affected suites pass locally — see Verification; full Desktop suite runs in CI.

Does this PR entail a change in behavior?

  • No — a banner that would previously have leaked title + preview under incognito is now suppressed; all other notification behavior is unchanged.

apache#4981)

The run-ended notification gated content-bearing banners on
settings.privacy.incognitoActive, but the local settings copy never
receives privacy updates: clientOwnedSettingsPatch excludes the
section and projection keeps the host's copy. After incognito was
enabled, the stale local copy kept reading false and the banner kept
exposing the session title + reply preview outside the app.

registerNotificationsIpc now takes an optional privacyAuthority. When
provided, resolveNotificationIncognito() reads incognito from the
authority and the local copy is never consulted; when the authority
rejects, the notification is suppressed rather than risked
(fail-closed). The authority lives in notifications-policy.ts beside the
rest of the gating logic so it stays unit-testable under plain node
--test; boot passes an adapter that queries every ready host's
runtime.policy and suppresses if any holds incognito — the same
client.queryRuntimePolicy().policy.privacy path search-ipc already
uses. Callers without the dep keep the existing local-copy behavior.
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 7, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for moving notification privacy to the Host authority. At acdcc9d90, the boot adapter still conflates an unavailable authority with an explicit non-incognito verdict, and includes session guests that cannot query runtime policy. These produce opposite failures in the same decision boundary: leaked content during reconnect and unrelated notification suppression. Please associate the notification with the relevant authorized Host and preserve an unknown verdict until it can be safely resolved; no mirrored privacy cache is needed. The actual adapter/resolver/gate probe reproduced both decisions; lifecycle and credential reachability were checked in source, not a live OS-banner E2E.

中文

Host 隐私权威方向正确,但实际 adapter 混淆了不可用权威与明确关闭隐身,并查询没有权限的 session guest,分别导致泄漏和误抑制。应绑定通知所属的可查询 Host,并保留未知状态,不新增隐私镜像。真实 adapter/gate 探针复现判断,生命周期和权限由源码核对,不冒充实际系统通知 E2E。

AI-assisted review using OpenAI Codex/Astra; coordinating-agent checks and probe limits are stated above. This is not an independent human review.

const entries = runtimeHostManager?.entries() ?? [];
const ready = entries.filter(
(entry): entry is Extract<typeof entry, { readiness: 'ready' }> =>
entry.readiness === 'ready',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Do not turn a reconnecting privacy authority into a non-incognito verdict

Reasonable disconnect race: an incognito Host delivers a completed/error event, the renderer queues notifications:runEnded, and that Host enters reconnecting while the handler awaits local settings. This ready-only filter removes it before any policy query. With zero ready entries (or only an ordinary local Host remaining), verdicts.some returns false, so an unfocused window can display the queued private title/body. The query-error catch cannot help because the Host was never queried.

The exact adapter/gate probe produces raised=true for zero-ready and local-ready + incognito-reconnecting cases. Keep a missing/unready relevant authority fail-closed; if needed pass the source scope through the existing notification IPC so the main process can resolve the right Host. Cover this actual boot decision, rather than only an injected resolver that throws.

中文

合理断连路径:隐身完成事件已触发通知 IPC,main 等待 settings 时 Host 变为 reconnecting,被 ready filter 排除,空 verdict 或仅普通本地 Host 都返回 false,后台窗口因此可展示私密标题/正文。未查询就不会触发 catch。探针确认 raised=true。相关权威缺失或未 ready 应保守抑制,必要时沿现有 IPC 传来源;测试应经过实际 boot adapter。

);
const verdicts = await Promise.all(
ready.map(async (entry) =>
(await entry.candidate.client.queryRuntimePolicy()).policy.privacy

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Exclude unrelated guest policy failures from local notification decisions

Normal supported path: mount a shared Session and then complete a turn in a local non-incognito Session. mountGuest registers the ready guest in the same manager entries, but SESSION_GUEST_OPERATION_GRANTS does not include runtime.policy.query. This Promise.all therefore rejects on every notification while that guest is mounted; the resolver catches it as incognito and suppresses all local notifications too.

The actual adapter probe with a local false verdict and a ready guest rejecting policy returns raised=false. Scope the query to the notification's legitimate authority; do not grant guests whole-Host policy access or simply ignore failures of the source Host. A local + mounted-guest regression should preserve the local notification.

中文

正常挂载共享 Session 后,guest 同样进入 ready entries,但其权限不包含 runtime.policy.query,因此 Promise.all 拒绝,所有普通本地通知也被当隐身取消。探针复现 raised=false。应按通知来源隔离权威,不给 guest 扩整机权限,也不能忽略来源 Host 的失败。

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by OpenAI Codex, operated by me2seeks, at acdcc9d90e4534b4401b75c24e52e93a337a71a9. This is not an independent human review.

One correctness finding: the production adapter treats an unavailable privacy verdict as permission by omitting every non-ready Host. See inline reproduction and the originating notification path.

  1. Optimal for the actual problem: no; moving to Host authority is correct, but the actual adapter still fails open during reconnect.
  2. Production code that can be deleted: the optional local-settings fallback has no second shipped composition caller; making the authority mandatory can remove that parallel decision path.
  3. Low-quality tests that can be deleted or replaced: none identified. Add coverage of the real readiness-filtering adapter; resolver-only mocks do not exercise this failure.
  4. Deeper refactor: no broad refactor; preserve the Host as privacy owner and suppress content when its verdict cannot be established.
  5. Ready to merge: no, the privacy failure remains reachable.
  6. Residual risks / verification: extracted and executed the exact production adapter; no live Electron notification E2E was run. This changes privacy and user-visible behavior; material protected-area changes require independent human review under CONTRIBUTING.md.

const ready = entries.filter(
(entry): entry is Extract<typeof entry, { readiness: 'ready' }> =>
entry.readiness === 'ready',
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Do not drop a reconnecting Host from the privacy decision

A terminal event can already have been forwarded to the renderer when its Host disconnects; the renderer then invokes notifications:runEnded with the title/preview while the manager publishes that Host as reconnecting (runtime-host-desktop-manager.ts:1365-1370). This filter silently excludes its privacy authority. Executing this exact adapter with only a reconnecting Host returns false; a reconnecting private Host plus another ready non-private Host also returns false. Promise.all([]) does not reject, so resolveNotificationIncognito never reaches its fail-closed catch and the native banner can expose the content. The IPC payload has no originating Host identity, so another ready Host cannot authorize this message. Preserve a suppressing verdict when the originating privacy authority cannot be queried (and cover the real adapter readiness transition).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Content-bearing notifications are still gated on the stale local privacy settings copy instead of Runtime Host policy

3 participants