fix(electron): lock in-window navigation to the app's loaded origin (#8230)

* fix(electron): lock in-window navigation to the app's loaded origin

The previous will-navigate handler accepted any URL whose hostname was
'localhost' or '127.0.0.1', which left the privileged main window
reachable by any local web server (e.g. http://127.0.0.1:1337/) — the
preload bridge (window.ea) would have been exposed to whatever page that
server returned. In production the legitimate origin is file:// only, so
there is no excuse for ever navigating to http://localhost in-window.

Compare against the URL the app actually loaded (captured at loadURL
time) instead of a hostname allowlist. http/https requires exact
host+port match; file:// requires the same html pathname; data:/blob:/
javascript: are rejected outright. The same guard is also applied to
will-redirect so a same-origin start cannot be redirected onto an
attacker page mid-navigation, and a defensive did-create-window handler
destroys any unexpected child window (the deny-all setWindowOpenHandler
should make that path unreachable).

Also tightens TO_FILE_URL with the same userData deny used by the other
file-sync IPCs, so a plugin/XSS cannot launder a userData path into a
file:// URL that later flows through READ_LOCAL_IMAGE_AS_DATA_URL.

* fix(electron): reject UNC/remote-host file:// in navigation guard

`new URL('file://192.168.1.100/Applications/SP.app/Contents/Resources/index.html').pathname`
equals the local app's pathname, so the previous pathname-only check
considered an attacker-controlled UNC host same-origin and let it load
in the privileged main window. The app's loaded file:// URL is always
local (empty host), so require `target.host === ''` explicitly.

* test(electron): add userinfo-@-trick navigation guard regression

`http://localhost:4200@evil.com/` parses with host=evil.com and
username=localhost — a naive substring or startsWith check would be
fooled. The existing host-equality check already rejects it; lock that
in with a test so a future refactor cannot regress.

electron/local-file-sync.test.cjs

@@ -148,3 +148,20 @@ test('READ_LOCAL_IMAGE_AS_DATA_URL inlines a user image outside userData', async
   );
   assert.match(result, /^data:image\/png;base64,/);
 });
+
+test('TO_FILE_URL refuses a path inside userData (no laundering into file://)', () => {
+  // toFileUrl is a pure string conversion, but the result is persisted as
+  // background-image config and later fed to READ_LOCAL_IMAGE_AS_DATA_URL.
+  // Refusing userData paths here keeps the two layers consistent.
+  assert.throws(
+    () => handlers['TO_FILE_URL']({}, path.join(userDataDir, 'simpleSettings')),
+    /protected directory/,
+  );
+});
+
+test('TO_FILE_URL converts a path outside userData', () => {
+  const p = path.join(externalDir, 'bg.png');
+  const result = handlers['TO_FILE_URL']({}, p);
+  assert.ok(result.startsWith('file://'));
+  assert.ok(result.endsWith('/bg.png'));
+});

electron/local-file-sync.ts

@@ -74,6 +74,17 @@ export const initLocalFileSyncAdapter = (): void => {
   );
 
   ipcMain.handle(IPC.TO_FILE_URL, (_, filePath: string): string => {
+    // SECURITY: the renderer hands us a path string from an OS file picker;
+    // a plugin/XSS could call us directly with any string. The result is a
+    // pure string conversion and not itself a capability, but the produced
+    // file:// URL is later persisted as background-image config and fed to
+    // READ_LOCAL_IMAGE_AS_DATA_URL. Mirroring that handler's deny — refuse
+    // to mint a file:// URL pointing at the app's private dir — keeps the
+    // two layers consistent and removes a path-laundering surface. Throw
+    // (vs. returning Error) so the existing string-returning signature is
+    // preserved; the legitimate caller passes paths from an OS picker, so
+    // a reject only fires on abuse.
+    assertPathOutside(getAppPrivateDir(), filePath);
     return pathToFileURL(filePath).href;
   });
   ipcMain.handle(

electron/main-window.ts

@@ -29,9 +29,16 @@ import { getIsMinimizeToTray, getIsQuiting, setIsQuiting } from './shared-state'
 import { loadSimpleStoreAll } from './simple-store';
 import { SimpleStoreKey } from './shared-with-frontend/simple-store.const';
 import { markGpuStartupSuccess } from './gpu-startup-guard';
+import { isAppOriginUrl } from './navigation-guard';
 
 let mainWin: BrowserWindow;
 
+// The URL passed to `mainWin.loadURL()` — the single source of truth for
+// "what is the app's own origin?". Read by the will-navigate / will-redirect
+// guards in `initWinEventListeners`. Set in `createWindow`, before listeners
+// are wired, so the guard never sees `undefined` at runtime.
+let appLoadedUrl: string | undefined;
+
 // Compact WCO band on Win/Linux. Native button width is OS-controlled
 // (~138px total); only height is configurable. Lower values may be
 // clamped to the OS minimum (~24–28px on Win11) — Electron silently
@@ -296,6 +303,11 @@ export const createWindow = async ({
       ? 'http://localhost:4200'
       : `file://${normalize(join(__dirname, '../.tmp/angular-dist/browser/index.html'))}`;
 
+  // Capture the loaded URL so the navigation guard (initWinEventListeners →
+  // will-navigate) can compare against the actual app origin, not a derived
+  // guess. Any URL change here automatically tightens the guard.
+  appLoadedUrl = url;
+
   mainWin.loadURL(url).then(() => {
     // Set window title for dev mode
     if (IS_DEV) {
@@ -446,31 +458,53 @@ function initWinEventListeners(app: Electron.App): void {
     });
   };
 
-  // open new window links in browser
+  // Compare the navigation target against the URL the app actually loaded
+  // (captured at loadURL time in createWindow). Anything else is treated as
+  // external and routed through the scheme-guarded `openUrlInBrowser`.
+  //
+  // The main window has Node integration via the preload bridge (`window.ea`).
+  // Allowing in-window navigation to ANY other origin — including
+  // http://127.0.0.1:<any-port> — would expose that bridge to whatever page
+  // happens to be served there (a malicious local web server, a sibling
+  // electron app, etc.). The previous host-only check accepted those.
+  //
+  // Hash-only changes do NOT fire will-navigate, so this never fires for
+  // the app's own hash routes (HashLocationStrategy in src/main.ts).
+  const guardNavigation = (
+    ev: { preventDefault: () => void },
+    url: string,
+    eventLabel: string,
+  ): void => {
+    if (appLoadedUrl && isAppOriginUrl(url, appLoadedUrl)) return;
+    ev.preventDefault();
+    log(`Blocked in-window navigation (${eventLabel})`);
+    openUrlInBrowser(url);
+  };
+
   mainWin.webContents.on('will-navigate', (ev, url) => {
-    // Navigate in-window only for the app's own dev-server origin
-    // (http://localhost:4200). Prod serves from file:// (start URL above) and
-    // routes via hash (withHashLocation, src/main.ts), so will-navigate never
-    // fires for the app's own routes there — only for real external links,
-    // which we hand to the (scheme-guarded) browser. Compare the host exactly:
-    // a substring match let hosts like `https://localhost.evil.com` navigate
-    // the main window directly.
-    let isInternalNavigation = false;
-    try {
-      const { hostname } = new URL(url);
-      isInternalNavigation = hostname === 'localhost' || hostname === '127.0.0.1';
-    } catch {
-      isInternalNavigation = false;
-    }
-    if (!isInternalNavigation) {
-      ev.preventDefault();
-      openUrlInBrowser(url);
-    }
+    guardNavigation(ev, url, 'will-navigate');
+  });
+  // Defense in depth: a same-origin navigation could redirect to a different
+  // origin server-side. Re-run the same check on the redirect target so a
+  // ‘302 → http://127.0.0.1:1337’ cannot land the bridge on an attacker page.
+  mainWin.webContents.on('will-redirect', (ev, url) => {
+    guardNavigation(ev, url, 'will-redirect');
   });
   mainWin.webContents.setWindowOpenHandler((details) => {
     openUrlInBrowser(details.url);
     return { action: 'deny' };
   });
+  // Defense in depth: setWindowOpenHandler already denies, so this should
+  // never fire. If a future code path ever enables window creation, destroy
+  // the spawned window rather than letting it inherit the preload bridge.
+  mainWin.webContents.on('did-create-window', (childWin) => {
+    error('did-create-window fired despite deny handler — destroying child');
+    try {
+      childWin.destroy();
+    } catch (e) {
+      error('Failed to destroy unexpected child window:', e);
+    }
+  });
 
   // TODO refactor quitting mess
   appCloseHandler(app);

electron/navigation-guard.test.cjs

@@ -0,0 +1,143 @@
+const test = require('node:test');
+const assert = require('node:assert/strict');
+const path = require('node:path');
+
+require('ts-node/register/transpile-only');
+
+// Match the layout used by the other *.test.cjs files: resolve the .ts source
+// via a computed path so tools/verify-electron-requires.js doesn't flag a
+// literal relative require of a file excluded from app.asar.
+const { isAppOriginUrl } = require(path.resolve(__dirname, 'navigation-guard.ts'));
+
+const DEV_APP_URL = 'http://localhost:4200';
+const PROD_APP_URL = 'file:///Applications/SP.app/Contents/Resources/index.html';
+
+test('dev: exact origin matches', () => {
+  assert.equal(isAppOriginUrl('http://localhost:4200/', DEV_APP_URL), true);
+  assert.equal(isAppOriginUrl('http://localhost:4200/#/foo', DEV_APP_URL), true);
+  assert.equal(isAppOriginUrl('http://localhost:4200/some-route', DEV_APP_URL), true);
+});
+
+test('dev: different port on localhost is rejected', () => {
+  // The whole point of the fix: 127.0.0.1:1337 or localhost:9999 must NOT
+  // inherit the preload bridge just because the hostname looks local.
+  assert.equal(isAppOriginUrl('http://localhost:1337/', DEV_APP_URL), false);
+  assert.equal(isAppOriginUrl('http://localhost:9999/', DEV_APP_URL), false);
+});
+
+test('dev: 127.0.0.1 is rejected (different host than localhost)', () => {
+  // The old guard accepted any "localhost OR 127.0.0.1" — anything serving
+  // on a loopback address could load in the privileged window. After the
+  // fix, only the exact origin the app loaded is allowed.
+  assert.equal(isAppOriginUrl('http://127.0.0.1:4200/', DEV_APP_URL), false);
+  assert.equal(isAppOriginUrl('http://127.0.0.1:1337/', DEV_APP_URL), false);
+});
+
+test('dev: localhost.evil.com substring attack is rejected', () => {
+  // Regression for the earlier substring-match bug (fixed in 4bf699735).
+  assert.equal(isAppOriginUrl('http://localhost.evil.com/', DEV_APP_URL), false);
+  assert.equal(isAppOriginUrl('http://evil.localhost:4200/', DEV_APP_URL), false);
+});
+
+test('dev: userinfo @ trick is rejected (host is the part after @)', () => {
+  // WHATWG URL parses `http://localhost:4200@evil.com/` with host=evil.com,
+  // username=localhost. A naive substring/startsWith check would be fooled;
+  // the host-equality check correctly rejects it.
+  assert.equal(isAppOriginUrl('http://localhost:4200@evil.com/', DEV_APP_URL), false);
+  assert.equal(
+    isAppOriginUrl('http://localhost@evil.com:4200/', DEV_APP_URL),
+    false,
+    'evil.com:4200 with localhost as username is still not our origin',
+  );
+});
+
+test('dev: https variant is rejected (protocol must match)', () => {
+  assert.equal(isAppOriginUrl('https://localhost:4200/', DEV_APP_URL), false);
+});
+
+test('dev: external https URL is rejected', () => {
+  assert.equal(isAppOriginUrl('https://example.com/', DEV_APP_URL), false);
+  assert.equal(isAppOriginUrl('https://jira.example.com/browse/X-1', DEV_APP_URL), false);
+});
+
+test('prod: same file:// path matches', () => {
+  assert.equal(isAppOriginUrl(PROD_APP_URL, PROD_APP_URL), true);
+});
+
+test('prod: rejects ALL http(s) localhost URLs (no dev-only opt-in)', () => {
+  // Production never legitimately navigates to http://localhost:*. The fix
+  // is what makes this hold: no host-based allowlist, only "matches the
+  // app's actual loaded URL".
+  assert.equal(isAppOriginUrl('http://localhost:4200/', PROD_APP_URL), false);
+  assert.equal(isAppOriginUrl('http://localhost:1337/', PROD_APP_URL), false);
+  assert.equal(isAppOriginUrl('http://127.0.0.1:1337/', PROD_APP_URL), false);
+});
+
+test('prod: rejects different file:// targets', () => {
+  assert.equal(
+    isAppOriginUrl('file:///etc/passwd', PROD_APP_URL),
+    false,
+    'arbitrary file path',
+  );
+  assert.equal(
+    isAppOriginUrl(
+      'file:///Applications/SP.app/Contents/Resources/other.html',
+      PROD_APP_URL,
+    ),
+    false,
+    'sibling html under the app bundle',
+  );
+});
+
+test('prod: rejects UNC / remote-host file:// even when pathname matches', () => {
+  // Regression: `file://` URLs carry a host. A pathname-only check matched
+  // `file://192.168.1.100/Applications/SP.app/Contents/Resources/index.html`
+  // against the local `file:///…/index.html` because both pathnames are
+  // `/Applications/SP.app/Contents/Resources/index.html` — letting an
+  // attacker-controlled UNC host load in the privileged window.
+  assert.equal(
+    isAppOriginUrl(
+      'file://192.168.1.100/Applications/SP.app/Contents/Resources/index.html',
+      PROD_APP_URL,
+    ),
+    false,
+    'UNC host with matching pathname must be rejected',
+  );
+  assert.equal(
+    isAppOriginUrl(
+      'file://attacker.example.com/Applications/SP.app/Contents/Resources/index.html',
+      PROD_APP_URL,
+    ),
+    false,
+    'remote-host file:// with matching pathname must be rejected',
+  );
+  // Even an empty-but-malformed authority pattern should not bypass it.
+  assert.equal(
+    isAppOriginUrl('file://localhost/etc/passwd', PROD_APP_URL),
+    false,
+    'localhost-host file:// pointing elsewhere is rejected',
+  );
+});
+
+test('non-http(s)/file schemes are rejected', () => {
+  // data:/blob:/javascript:/etc must never land in the privileged window.
+  assert.equal(isAppOriginUrl('data:text/html,<h1>x</h1>', DEV_APP_URL), false);
+  assert.equal(isAppOriginUrl('javascript:alert(1)', DEV_APP_URL), false);
+  assert.equal(isAppOriginUrl('blob:http://localhost:4200/abcd', DEV_APP_URL), false);
+  assert.equal(isAppOriginUrl('about:blank', DEV_APP_URL), false);
+  assert.equal(isAppOriginUrl('ftp://localhost:4200/x', DEV_APP_URL), false);
+});
+
+test('malformed URLs are rejected', () => {
+  assert.equal(isAppOriginUrl('not a url', DEV_APP_URL), false);
+  assert.equal(isAppOriginUrl('', DEV_APP_URL), false);
+});
+
+test('custom dev URL: exact origin still required', () => {
+  // Some setups override the dev URL (electron CLI). The guard always reads
+  // back the actual loaded URL, so a custom dev origin works without code
+  // changes — but still rejects look-alikes.
+  const customUrl = 'http://localhost:9999';
+  assert.equal(isAppOriginUrl('http://localhost:9999/route', customUrl), true);
+  assert.equal(isAppOriginUrl('http://localhost:4200/', customUrl), false);
+});

electron/navigation-guard.ts

@@ -0,0 +1,46 @@
+/**
+ * Returns true if `targetUrl` is the same origin as the app's loaded URL.
+ *
+ * Security boundary: the main window has Node integration / preload bridge.
+ * A navigation to ANY other origin in-window would expose `window.ea` to
+ * untrusted content (e.g. http://127.0.0.1:<any> hosting a malicious page).
+ * `will-navigate` MUST reject anything this returns false for and hand it
+ * to the (scheme-guarded) external-open path instead.
+ *
+ * Comparison rules:
+ * - Protocols must match exactly.
+ * - http/https: exact host (incl. port) match. Subdomains differ → different
+ *   origin. This is what blocks `localhost.evil.com` and `http://127.0.0.1:1`.
+ * - file: BOTH host AND pathname must match the app's loaded html file.
+ *   `file://` URLs may carry a host (UNC paths / remote shares), and Chromium
+ *   resolves the pathname relative to that host — so a target like
+ *   `file://192.168.1.100/Applications/SP.app/Contents/Resources/index.html`
+ *   has the same `.pathname` as the local app start URL. A pathname-only
+ *   check would let an attacker-controlled UNC host load in the privileged
+ *   window. The app's loaded file:// URL is always local (empty host), so we
+ *   require `target.host === ''` explicitly rather than relying on the
+ *   host-equality compare to coincidentally match.
+ *   Hash-only changes do not fire `will-navigate`, so a same-document hash
+ *   route never reaches this check.
+ * - Anything else (data:, blob:, javascript:, ftp:, …): rejected.
+ */
+export const isAppOriginUrl = (targetUrl: string, appUrl: string): boolean => {
+  let target: URL;
+  let expected: URL;
+  try {
+    target = new URL(targetUrl);
+    expected = new URL(appUrl);
+  } catch {
+    return false;
+  }
+  if (target.protocol !== expected.protocol) return false;
+  if (target.protocol === 'http:' || target.protocol === 'https:') {
+    return target.host === expected.host;
+  }
+  if (target.protocol === 'file:') {
+    return (
+      target.host === '' && expected.host === '' && target.pathname === expected.pathname
+    );
+  }
+  return false;
+};