fix(security): deny window.open + block foreign navigation on all BrowserWindows (#377)

[Optic00]

Closes #377.

What

Adds navigation/window-open guards to every BrowserWindow. Previously none of the three windows denied window.open or blocked in-app navigation, so a target="_blank" link, a window.open, or a foreign top-level navigation could spawn a chrome-less Electron window or navigate the app frame away from the bundled renderer.

A single hardenWindow(win, { allowExternalLinks }) helper is applied at all three creation sites:

• mainWindow and notificationWindow (allowExternalLinks: true) — deny every popup, but route a genuine http(s)/mailto target (e.g. the target="_blank" "Report an issue" link in Setup.tsx, or a link inside a note) through shell.openExternal so it opens in the user's browser instead of silently dying.
• PDF render window (allowExternalLinks: false) — pure deny/block; this background render of renderer-supplied HTML must never spawn a browser tab on its own.

setWindowOpenHandler returns deny (so did-create-window can't fire); will-navigate cancels any navigation away from the current committed document (reads the target off event.url, the non-deprecated Electron 42 accessor).

Why it's safe

• The renderer already routes all real external links through the open-external IPC (shell.openExternal), so nothing legitimately relies on window.open.
• The SPA uses hash routing, so will-navigate never fires on a real route change — anything reaching it is a foreign/injected navigation.
• loadFile (main/notification) and the PDF loadURL(data:) are main-process programmatic loads, which don't emit will-navigate.

Testing

• New T1 spec security-window-guards.t1.spec.ts: asserts window.open is denied (no popup BrowserWindow created). Verified fail-before (flipping the handler to allow makes it fail).
• The companion will-navigate guard is intentionally not asserted by a test: modern Chromium already blocks the only hermetic navigation vectors (data: / about:blank / missing file:), so a fail-before-provable test isn't achievable without a networked http target. It's covered by review + reasoning instead of a test that would pass regardless. (Rationale is in the spec's docblock.)
• Full unit suite green (111 passed); cross-family (Codex) review returned no blockers.

---

Summary by cubic

Hardened all Electron BrowserWindows: deny window.open popups and block foreign navigations. In interactive windows, only HTTP(S) links open in the default browser. Addresses #377.

• Bug Fixes
  • Added hardenWindow(win, { allowExternalLinks }); applied to mainWindow/notificationWindow (true) and the PDF render window (false).
  • setWindowOpenHandler always returns deny; when allowed, routes only HTTP(S) targets via shell.openExternal (no mailto: or other schemes).
  • will-navigate cancels navigation away from the current document; allowed HTTP(S) URLs open via shell.openExternal.
  • New e2e spec e2e/specs/security-window-guards.t1.spec.ts asserts popup denial.

^{Written for commit a1687fb. Summary will update on new commits.}

[cubic-dev-ai]

All reported issues were addressed across 2 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[ruzin]

Maintainer review — security logic is correct; both cubic P2s resolved (mailto dropped to http(s)-only; the event.url-undefined one is a verified false positive on electron@42). Legitimate external links (openExternal IPC), OAuth, and the PDF data: URL window are all unaffected.

⚠️ Careful rebase required. This is CONFLICTING with the #402 notifications refactor, which moved toast-window creation out of createNotificationWindow into the class method (const win = new BrowserWindow(...), ~main.js:293). A naive 'take main's side' resolution will silently leave the notification window unhardened — 1 of the 3 windows, with no test to catch it. On rebase, re-apply hardenWindow(win, { allowExternalLinks: true }) at that relocated site. The mainWindow + PDF hunks merge cleanly on their own.

[ruzin]

Maintainer review — security logic is correct; both cubic P2s resolved (mailto dropped to http(s)-only; the event.url-undefined one is a verified false positive on electron@42). Legitimate external links (openExternal IPC), OAuth, and the PDF data: URL window are all unaffected.

⚠️ Careful rebase required. This is CONFLICTING with the #402 notifications refactor, which moved toast-window creation out of createNotificationWindow into the class method (const win = new BrowserWindow(...), ~main.js:293). A naive 'take main's side' resolution will silently leave the notification window unhardened — 1 of 3 windows, with no test to catch it. On rebase, re-apply hardenWindow(win, { allowExternalLinks: true }) at that relocated site; the mainWindow + PDF hunks merge cleanly on their own.

[Optic00]

Rebased onto main — and this is exactly the trap you flagged, so I handled it deliberately: the #402 refactor moved toast-window creation into the Notification.show() class method (const win = new BrowserWindow(...)), and the old createNotificationWindow that this PR hardened is gone. I re-applied hardenWindow(win, { allowExternalLinks: true }) at the relocated site (right after notificationWindow = win), so all three windows (main, PDF, toast) are covered — not "take main's side." Verified no dangling createNotificationWindow references remain.

Now MERGEABLE. The mainWindow + PDF hunks merged cleanly as you predicted.

One open point on your "no test to catch it": the existing T1 (security-window-guards.t1) asserts the main window's setWindowOpenHandler at the renderer level, but there's no test that specifically pins the toast window getting hardened (the naive-rebase failure mode). Happy to add a main-process assertion for that if you'd like it as a guardrail — say the word.

[ruzin]

Heads up — after #416 merged (it touches the same notification-window region), this went back to CONFLICTING. Needs one more rebase onto current main. Same caveat as before: re-apply hardenWindow(win, { allowExternalLinks: true }) at the relocated notification-window site so all three windows stay guarded. Logic itself is approved — just the rebase.

[Optic00]

Already done — pushed the rebase onto current main about twenty minutes after your note, so this is MERGEABLE again.

Your caveat is covered: hardenWindow(win, { allowExternalLinks: true }) sits at the relocated notification-window site, and I checked all three new BrowserWindow call sites are guarded (toast, main window, and the PDF-render window with no external links). The conflict itself was additive — main's soft-delete block and hardenWindow landed in the same region, so both are kept.

Verified locally before pushing: typecheck clean, unit green, full T1 suite green including this PR's own security-window-guards.t1. CI is green so far with T2 still queued.