fix(electron): grant the dashboard the notifications permission so its OS toasts work

[helenastafford]

Summary

Fixes #8308.

The dashboard already fires page-context new Notification() toasts — useNativeNotification.ts for new unacked notifications, useWebSocket.ts for approval requests — and they work in a plain browser, where Chromium prompts and grants. In the Electron app the permission handlers denied notifications wholesale, pinning Notification.permission to 'denied', so the exact same code no-oped silently. Desktop users got fewer OS notifications than browser-tab users.

Change

electron/permission-handler.js: generalize the existing fullscreen grant into a TRUSTED_PAGE_PERMISSIONS set (fullscreen, notifications). Both members share one shape — requested by the dashboard's own code, no OS resource Electron must broker, silent renderer failure when denied. The grant is:

• trust-gated — the untrusted embedded browser view stays denied (an OS toast is trusted chrome; a browsed page must not post one wearing the app's identity), and the separate persist:kirocrew-browser partition keeps its own deny-all handlers;
• origin-gated — foreign origins stay denied;
• TCC-free — answered before the media rule, so a notification is never gated on or prompts for the microphone.

Zero renderer changes: with permission granted, Electron renders the existing constructor calls as real OS notifications.

Tests

New notifications describe block in electron/test/permission-handler.test.js mirroring the fullscreen-grant pattern: dashboard grant (request + check), null-wc check on origin (the verdict Notification.permission reflects), untrusted-view denial, foreign-origin denial, no-TCC-path guard, request/check agreement. The untrusted-view sweep in permission-untrusted.test.js passes unchanged — the identity gate outranks the new grant, which is exactly what it pins.

node --test test/permission-handler.test.js test/permission-untrusted.test.js: 50/50 pass. Full electron suite matches the clean-main host baseline (failures identical on pristine HEAD, all in unrelated auto-update/petOverlays files).

Not in scope

#4667's broader feature asks (per-category config, click-to-navigate) — this PR only unblocks the notification code that already ships.

Pattern harvest

Rule candidate: audit checklist
Pattern: a renderer calls a permission-gated Web API (new Notification()) that the Electron permission handlers deny by default, so the feature silently no-ops only in the packaged app. When adding or reviewing page-context Web API usage in website/src/, cross-check electron/permission-handler.js grants (TRUSTED_PAGE_PERMISSIONS / media rule) so the app-origin allowlist stays in sync with the APIs the dashboard actually uses.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 8fc9dfdd84b3d19f758be2819a6520a57c310fef — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root-cause fix at the right layer: the existing renderer code is unblocked by the one handler that denied it, gated identically to the fullscreen precedent.

The set-widening is earned (two real consumers, the old single-constant rationale updated in place), the untrusted-view/foreign-origin/subframe denials keep every trust boundary the handler already enforced, and the deliberate fullscreen-vs-notifications frame asymmetry is documented and pinned by tests. No design-level concerns.

[DESIGN-REVIEWED] 8fc9dfd

[github-actions]

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of 8fc9dfdd84b3d19f758be2819a6520a57c310fef — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: CONCERNS

Polished, self-explaining share flow — but a failed card export dies silently, and on the intent path it strands a blank tab.

Watch

• No failure state on export. In ShareMessageModal.tsx, handleDownload/handleCopy return silently on a null blob and an exportBlob() rejection is unhandled (finally only clears busy); in openIntent the rejection also skips tab.location.href = url, so the pre-opened window.open('', '_blank') tab stays blank forever. User sees a spinner stop (or a dead blank tab) with no message — html-to-image is known-flaky on Safari, so this isn't purely theoretical. Low-to-moderate frequency × moderate confusion × every retry. Fix: catch export failure, set a feedback === 'export_failed' status line, and close or navigate the pre-opened tab in that branch.

Suggestions

• Give share-x/share-linkedin the same Loader2 pending treatment share-download/share-copy get while busy === 'intent' — today all four just dim with no motion during a multi-second rasterize.

[UX-REVIEWED] 8fc9dfd

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 8fc9dfdd84b3d19f758be2819a6520a57c310fef — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

First-Principles-Verdict: CONCERNS

The notifications grant is derived (issue #8308) and cause-level; only the one-member MAIN_FRAME_ONLY_PERMISSIONS set is generalized surface, and the description omits the frame gate it ships.

What this change ships

Intent: let desktop-app users get the OS notification toasts the dashboard already fires in a browser — a FIX (#8308).

Note: the diff artifact was cut against a stale base — its non-electron hunks (SSE relay, credential-redaction notice, share cards, ratchet audit, ~25 files) match commits already in the base checkout's main log (#8199, #8136, #8040), so they are base drift, not this PR. Inventory covers the PR-authored change: website/electron/permission-handler.js + its test.

1. Desktop app now shows OS toasts for new unacked notifications — justified (fixes Electron app blocks the dashboard's own native notifications (works in Chrome, silent no-op in the desktop app) #8308; consumer useNativeNotification.ts:37)
2. Desktop app now shows OS toasts for approval requests while hidden — justified (consumer useWebSocket.ts:1190)
3. Notification.permission reads 'granted', so shipped renderer code runs unchanged — justified (check mirrors request)
4. Embedded-browser pages and foreign origins still get no toasts — justified (external-content boundary)
5. Iframes inside the dashboard cannot post OS toasts; missing frame info fails closed — undeclared, justified (dashboard does embed iframes, mcpAppSrcdoc.ts:160)
6. A notification never triggers or waits on a microphone prompt — justified
7. Fullscreen unchanged; its constant regrouped into a two-member trusted set — justified (2 members, identical gating)
8. New MAIN_FRAME_ONLY_PERMISSIONS set — one member, one consumer, generalized

Watch

The description's gate list ("trust-gated… origin-gated… TCC-free") predates the tip commit: the shipped grant is also frame-gated, and fullscreen deliberately is not (permission-handler.js:88-105). That asymmetry is the security-relevant part a human reads the description for.

No unfixed siblings: grepped geolocation|clipboard.read|pointerLock|MIDI under website/src/ — no top-level dashboard consumer of any other denied permission (hits are MCP-iframe allow-attribute plumbing and tests).

Subtractions

Drop the MAIN_FRAME_ONLY_PERMISSIONS set (permission-handler.js:105) — one entry ever constructed, one consumer (frameOk); inline the singular form: permission !== "notifications" || details?.isMainFrame === true. The module's own comment names this discipline (fullscreen stayed a scalar until a second member existed).

[FIRST-PRINCIPLES-REVIEWED] 8fc9dfd

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 8fc9dfdd84b3d19f758be2819a6520a57c310fef — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 8fc9dfd

Verdict parsed from the review's SHA-scoped output markers for commit 8fc9dfdd84b3d19f758be2819a6520a57c310fef.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 8fc9dfdd84b3d19f758be2819a6520a57c310fef: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 8fc9dfdd84b3d19f758be2819a6520a57c310fef and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 8fc9dfd

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 8fc9dfdd84b3d19f758be2819a6520a57c310fef: <one-sentence reason>

[helenastafford]

Finding: GPT 5.6 (BLOCKING) — website/electron/permission-handler.js:250 — subframes inherit the dashboard's notification grant; a scripted localhost iframe sharing the dashboard webContents passes originOk and can emit OS notifications as Kiro Crew.

Disposition: FIXED in 8fc9dfd. The notifications grant now requires details.isMainFrame === true in both the request and check handlers (fail-closed when the field is absent), via a MAIN_FRAME_ONLY_PERMISSIONS set. fullscreen deliberately stays frame-agnostic to preserve the pre-existing inline-video fullscreen behavior. Added 3 tests: subframe denial (both handlers), fail-closed on missing isMainFrame, and fullscreen frame-agnosticism; 47/47 permission-handler tests pass locally.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• This PR is OVERLAPPING with PR #7821. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8312: KEEP. Complementary halves of notification delivery at different layers, with no file or patch conflict. 8312 should not wait on 7821: 7821 preserves the permission gate, so 8312 is a prerequisite for 7821's own delivery path inside the desktop app. Files: website/src/hooks/useNativeNotification.ts, website/electron/permission-handler.js.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[bolichen97]

Scope is exactly two files — permission-handler.js and its test — with zero renderer changes, so this unblocks the new Notification() calls that already ship rather than adding a notification feature. The generalization of FULLSCREEN_PERMISSION into TRUSTED_PAGE_PERMISSIONS is a faithful refactor: both branches keep the same !isUntrusted(wc) && originOk(wc, origin) verdict, and both remain answered before the media rule so a toast is never gated on or prompted for the microphone (pinned by the test that throws from getMicAccessStatus/askForMicAccess/onMicBlocked).

The security-relevant part is handled correctly. Widening a permission set needs the gate to still hold on every axis, and each one has a test: untrusted embedded-browser view denied, foreign origin denied, and — the non-obvious one — subframes denied via MAIN_FRAME_ONLY_PERMISSIONS, which matters because a subframe shares the parent's webContents and isAppOrigin accepts any localhost origin, so without it an embedded localhost preview would inherit the grant and could post OS toasts under the app's identity. frameOk is fail-closed (details?.isMainFrame === true), and the test asserts denial for {} and undefined, not just {isMainFrame:false}. Scoping the frame gate to notifications only, with an explicit test that fullscreen stays frame-agnostic, is right — an inline

The request/check pair is the actual bug surface and is covered as such: Notification.permission is the check handler's verdict, and the renderer never constructs a toast unless it reads 'granted', so a request-side-only grant would have left the hook dead. The suite pins the check directly (including the null-wc frameless case) and asserts the two handlers agree under both trust states. Removing notifications from the three deny-sweep loops is required by the new behavior, and each of those loops retains other members so none becomes vacuous. All checks green.