fix(desktop): deny window-open side-effect opens (GHSA-9f4c-93c8-jc8g)

setWindowOpenHandler opened details.url as a side effect before denying. Per GHSA-9f4c-93c8-jc8g (CVE-2026-70608, High 7.2), a sandboxed iframe with no allow-popups and no user gesture can reach this handler via the OpenURL path -- and the desktop renders untrusted artifact HTML in <iframe sandbox="allow-scripts">. A malicious artifact could therefore force the OS browser to an attacker URL with zero interaction. Electron ships no fixed 40.x release (fix is 41.10.3+/42.0.1), so we close it at the seam, version-independently. - electron/window-open-policy.ts: pure decideWindowOpen (always deny) + createWindowOpenHandler(onDenied) that denies and never opens a URL; the hook is logging-only. - main.ts: wireCommonWindowHandlers uses it (covers primary + all secondary/quick windows); the deny is logged, no side-effect open. - Trusted external links are unaffected: they already route through the audited hermes:openExternal IPC channel (openExternalUrl, http/https/ mailto allowlist). Converted the one remaining bare window.open on the Electron path (env-var docs menu) to openExternalLink; other window.open sites are bridge-absent web fallbacks. - tests-js/window-open-policy.test.ts: 4 tests pinning always-deny, the logging-only hook, and that a throwing hook never degrades to allow.
2026-08-08 23:24:54 -07:00
parent 1e69c12b64
commit 77ca6a6d12
4 changed files with 136 additions and 6 deletions
@@ -412,6 +412,7 @@ import { fetchMarketplaceThemes, searchMarketplaceThemes } from './vscode-market
 import { createWakeIndicatorWindowController } from './wake-indicator-window'
 import { enumerateWindowsFrontToBack, enumerationFailed, readWindowBelow } from './window-below'
 import { registrySshScopeForWindowRoute, WindowConnectionRouteRegistry } from './window-connection-route'
+import { createWindowOpenHandler } from './window-open-policy'
 import { installWindowRendererLifecycle } from './window-renderer-lifecycle'
 import { createWindowRevealController } from './window-reveal'
 import {
@@ -13242,11 +13243,17 @@ function wireCommonWindowHandlers(win, { zoom = true }: { zoom?: boolean } = {})
  }

  installContextMenuBridge(win)
-  win.webContents.setWindowOpenHandler(details => {
-    openExternalUrl(details.url)
-
-    return { action: 'deny' }
-  })
+  // Deny every window-open request and NEVER open a URL as a side effect here.
+  // Trusted external links go through the audited `hermes:openExternal` IPC
+  // channel; the only content that reaches this handler is what we did not
+  // initiate — including untrusted HTML in sandboxed `allow-scripts` iframes.
+  // Opening `details.url` here is the GHSA-9f4c-93c8-jc8g (CVE-2026-70608)
+  // vector: a sandboxed iframe with no `allow-popups` and no user gesture can
+  // force the OS browser to an attacker URL. No fixed 40.x Electron exists, so
+  // we close it at the seam. See electron/window-open-policy.ts.
+  win.webContents.setWindowOpenHandler(
+    createWindowOpenHandler(url => rememberLog(`[window-open] denied: ${url}`))
+  )
  win.webContents.on('will-navigate', (event, url) => {
    if ((DEV_SERVER && url.startsWith(DEV_SERVER)) || (!DEV_SERVER && url.startsWith('file:'))) {
      return
@@ -0,0 +1,55 @@
+/**
+ * Window-open policy for every BrowserWindow's webContents.
+ *
+ * In the Electron desktop app, every external URL we open on purpose is routed
+ * through the audited `hermes:openExternal` IPC channel (see `openExternalUrl`
+ * in main.ts, which enforces an http/https/mailto scheme allowlist and guards
+ * file: through the IPC path resolver). The `window.open` / `target=_blank`
+ * path that reaches `setWindowOpenHandler` is therefore, inside Electron, only
+ * ever driven by content we did NOT initiate — most dangerously untrusted HTML
+ * rendered in sandboxed `allow-scripts` iframes (artifact previews).
+ *
+ * GHSA-9f4c-93c8-jc8g (CVE-2026-70608, High 7.2) lets such a sandboxed iframe
+ * reach this handler with NO user interaction and WITHOUT `allow-popups`. If
+ * the handler opens `details.url` as a side effect, a malicious artifact can
+ * force the user's real browser to an attacker-chosen URL. There is no fixed
+ * 40.x Electron release (the fix is 41.10.3+/42.0.1), so we defend at the seam
+ * regardless of Electron version: deny every window-open request and NEVER open
+ * a URL as a side effect here. Trusted opens keep working because they go
+ * through the IPC channel, not this handler.
+ */
+
+export interface WindowOpenRequestLike {
+  url: string
+}
+
+export interface WindowOpenDecision {
+  action: 'deny'
+}
+
+/**
+ * The security decision for a window-open request. Always deny — see the module
+ * comment. Kept as a named function so the contract has one tested home and a
+ * future edit that tries to reintroduce conditional opening has to defeat the
+ * test rather than silently succeed.
+ */
+export function decideWindowOpen(_request: WindowOpenRequestLike): WindowOpenDecision {
+  return { action: 'deny' }
+}
+
+/**
+ * Build a `setWindowOpenHandler` callback. It denies unconditionally and never
+ * opens anything. `onDenied` is an optional observability hook (logging only);
+ * it MUST NOT open a URL — doing so would re-open the CVE this handler closes.
+ */
+export function createWindowOpenHandler(
+  onDenied?: (url: string) => void
+): (details: WindowOpenRequestLike) => WindowOpenDecision {
+  return details => {
+    if (onDenied) {
+      onDenied(details.url)
+    }
+
+    return decideWindowOpen(details)
+  }
+}
@@ -10,6 +10,7 @@ import {
 import { Button } from '@/components/ui/button'
 import { Codicon } from '@/components/ui/codicon'
 import { useI18n } from '@/i18n'
+import { openExternalLink } from '@/lib/external-link'
 import { triggerHaptic } from '@/lib/haptics'
 import { ExternalLink, Eye, EyeOff, KeyRound, Trash2 } from '@/lib/icons'
 import { cn } from '@/lib/utils'
@@ -61,7 +62,7 @@ function useEnvVarItems({
        onSelect: event => {
          event.preventDefault()
          triggerHaptic('selection')
-          window.open(docsUrl!, '_blank', 'noopener,noreferrer')
+          openExternalLink(docsUrl!)
        }
      })
    }
@@ -0,0 +1,67 @@
+/**
+ * Security regression for GHSA-9f4c-93c8-jc8g (CVE-2026-70608, High 7.2):
+ * "Sandboxed iframe can bypass the allow-popups restriction via the OpenURL
+ * navigation path."
+ *
+ * A sandboxed iframe without `allow-popups` can reach a window's
+ * `setWindowOpenHandler` with no user gesture. The desktop app renders
+ * untrusted artifact HTML in `<iframe sandbox="allow-scripts">`, so if the
+ * handler opens `details.url` as a side effect, a malicious artifact can force
+ * the OS browser to an attacker URL. Electron has no fixed 40.x release, so the
+ * defense lives in our handler: it must ALWAYS deny and must NEVER open a URL.
+ *
+ * These import the real policy module (pure, dependency-free) so the contract is
+ * tested against the code main.ts actually wires in — a future edit that
+ * reintroduces conditional opening has to defeat these tests.
+ */
+
+import assert from 'node:assert/strict'
+
+import { describe, test } from 'vitest'
+
+import {
+  createWindowOpenHandler,
+  decideWindowOpen
+} from '../apps/desktop/electron/window-open-policy'
+
+describe('window-open policy (GHSA-9f4c-93c8-jc8g)', () => {
+  test('decideWindowOpen always denies, regardless of URL', () => {
+    for (const url of [
+      'https://example.com',
+      'http://attacker.test/steal',
+      'file:///etc/passwd',
+      'javascript:alert(1)',
+      'custom-proto://payload',
+      ''
+    ]) {
+      assert.deepEqual(decideWindowOpen({ url }), { action: 'deny' })
+    }
+  })
+
+  test('handler denies and never returns an allow action', () => {
+    const handler = createWindowOpenHandler()
+    const result = handler({ url: 'https://attacker.test/popup' })
+    assert.equal(result.action, 'deny')
+  })
+
+  test('handler surfaces the denied URL to the observability hook only', () => {
+    const denied: string[] = []
+    const handler = createWindowOpenHandler(url => denied.push(url))
+
+    const result = handler({ url: 'https://attacker.test/x' })
+
+    assert.equal(result.action, 'deny')
+    assert.deepEqual(denied, ['https://attacker.test/x'])
+  })
+
+  test('a throwing observability hook does not turn a deny into an allow', () => {
+    const handler = createWindowOpenHandler(() => {
+      throw new Error('logging blew up')
+    })
+    // The hook is logging-only; even if it throws, the security decision must
+    // not silently become "allow". We assert the throw propagates rather than
+    // being swallowed into an open — the caller (Electron) treats a throw as
+    // deny, and crucially no URL was opened as a side effect.
+    assert.throws(() => handler({ url: 'https://attacker.test/x' }))
+  })
+})