diff --git a/electron/local-file-sync.test.cjs b/electron/local-file-sync.test.cjs index 97ec33dfc2..139b592332 100644 --- a/electron/local-file-sync.test.cjs +++ b/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')); +}); diff --git a/electron/local-file-sync.ts b/electron/local-file-sync.ts index 3ecdaff41c..2729886c58 100644 --- a/electron/local-file-sync.ts +++ b/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( diff --git a/electron/main-window.ts b/electron/main-window.ts index a65483929e..5bfcb5cedd 100644 --- a/electron/main-window.ts +++ b/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: — 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); diff --git a/electron/navigation-guard.test.cjs b/electron/navigation-guard.test.cjs new file mode 100644 index 0000000000..5dba5ae694 --- /dev/null +++ b/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,

x

', 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); +}); diff --git a/electron/navigation-guard.ts b/electron/navigation-guard.ts new file mode 100644 index 0000000000..cb751c1ca4 --- /dev/null +++ b/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: 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; +};