diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md index b3cd809..bc2cd23 100644 --- a/.github/instructions/code-review.instructions.md +++ b/.github/instructions/code-review.instructions.md @@ -141,10 +141,13 @@ network, running a WordPress with `admin`/`admin`. **Validate what crosses IPC.** Every `ipcMain.handle` argument comes from the renderer and is untrusted input. Paths get used for file operations, URLs get opened. -> Known open case, useful as a calibration example: the `url:open` handler in `src/main.js` passes -> its argument straight to `shell.openExternal()` with no scheme check, so `file://` and -> `javascript:` get through. If a PR touches this handler and the review does not mention it, the -> review is not working. +> Worked example, kept because it shows the shape rather than a single bug: `url:open` in +> `src/main.js` used to pass its argument straight to `shell.openExternal()`, so `file://` and +> `javascript:` got through to the OS handler. It now goes through `src/external-url.js`, which +> refuses anything outside an http/https allow-list and — the part that is easy to miss — hands +> the OS the *parsed* address rather than the caller's string, because the URL parser strips +> control characters and a validator that checks one string while the caller opens another has +> not checked anything. Look for both halves in any new handler that takes a URL or a path. **Servers stay on loopback.** `src/bind-loopback.js` patches `net.Server.prototype.listen` so Playground's servers bind to `127.0.0.1` instead of every interface. Any new listener that is diff --git a/src/external-url.js b/src/external-url.js new file mode 100644 index 0000000..5b59a28 --- /dev/null +++ b/src/external-url.js @@ -0,0 +1,107 @@ +// The gate in front of `shell.openExternal`. +// +// `openExternal` hands an address to whatever application the OS has registered +// for its scheme, which is a much wider action than "show this page in the +// browser". A `file:` address opens an arbitrary local path in its associated +// application — on Windows that can mean running it rather than viewing it — +// and every other scheme registered on the machine is reachable the same way. +// +// The renderer is the only caller and every call site passes an http/https +// address, so nothing today misuses it. But the renderer also displays content +// the app does not author: child-process output from an install or a build, and +// the site being developed. The server URL the app auto-opens is itself parsed +// out of the Playground server's stdout. This module is the step that keeps any +// future influence over that string from turning into an action on the +// contributor's machine. +// +// Widen ALLOWED_URL_SCHEMES only for a scheme the app actually needs, and only +// after asking what the OS does with it. + +const ALLOWED_URL_SCHEMES = ['http:', 'https:']; + +// Returns the address to open, or null if it is not one this app opens. +// +// Scheme is read off the parsed URL rather than the raw string, so casing and +// leading whitespace ('FILE:', ' file:') are normalized before the comparison +// instead of being a way around it. An address Node can't parse is refused +// rather than passed on to the OS to interpret. +// +// What comes back is the parser's own `href`, not the caller's string. Checking +// one string and opening a different one is the gap this module exists to +// close: the URL parser strips tabs and newlines from anywhere in the input, +// including the middle of the scheme, so 'ht\ntp://example.com' validates as +// http while the OS would receive an address its own parser resolves by its own +// rules. Returning the normalized form means the address that was checked is +// the address that gets opened. For every caller in this app the two are +// identical but for a trailing slash. +function normalizeExternalUrl(url) { + if (typeof url !== 'string' || url.trim() === '') return null; + + let parsed; + try { + parsed = new URL(url); + } catch { + return null; + } + + if (!ALLOWED_URL_SCHEMES.includes(parsed.protocol)) return null; + + return parsed.href; +} + +function isAllowedExternalUrl(url) { + return normalizeExternalUrl(url) !== null; +} + +// Line breaks, and everything else that would let a refused address end a log +// line and start another one. +const CONTROL_CHARACTERS = /[\x00-\x1f\x7f-\x9f\u2028\u2029]/g; + +// A refused address is attacker-influenced by hypothesis, and it is about to be +// written into the file contributors attach to bug reports. Two things follow. +// +// It has to stay on one line: a newline in the address would otherwise let it +// write a second entry in the app's own timestamp-and-scope format, and a log +// that can be made to describe events that never happened is worse than no log. +// The control characters are escaped rather than dropped so the line still says +// what the caller actually sent. +// +// And it has to be bounded, so a very long address cannot flood the file. +// Truncation comes after escaping, since escaping is what decides the final +// length. +function describeRefusedUrl(url) { + if (typeof url !== 'string') return `<${url === null ? 'null' : typeof url}>`; + + const oneLine = url.replace(CONTROL_CHARACTERS, (c) => { + const code = c.codePointAt(0); + return code <= 0xff + ? `\\x${code.toString(16).padStart(2, '0')}` + : `\\u${code.toString(16).padStart(4, '0')}`; + }); + + if (oneLine.length <= 120) return oneLine; + return `${oneLine.slice(0, 120)}…`; +} + +// The `url:open` handler's body, kept out of main.js so both sides of the guard +// can be tested without an Electron process: `openExternal` is the real +// `shell.openExternal` in the app and a recording stub in the tests. +async function openExternalUrl(url, { openExternal, onRefused } = {}) { + const target = normalizeExternalUrl(url); + + if (target === null) { + if (typeof onRefused === 'function') onRefused(describeRefusedUrl(url)); + return false; + } + + await openExternal(target); + return true; +} + +module.exports = { + ALLOWED_URL_SCHEMES, + normalizeExternalUrl, + isAllowedExternalUrl, + describeRefusedUrl, + openExternalUrl +}; diff --git a/src/main.js b/src/main.js index 880e180..50d3fa6 100644 --- a/src/main.js +++ b/src/main.js @@ -29,6 +29,7 @@ const { buildMenuTemplate } = require('./menu'); const { killChildTree } = require('./kill-tree'); const { normalizeEol } = require('./git-update.cjs'); const { ensureAutocrlf, readTrunkInfo, collectDirtyFiles, discardChanges, updateToLatestTrunk } = require('./trunk-update'); +const { openExternalUrl, ALLOWED_URL_SCHEMES } = require('./external-url'); const WORDPRESS_GIT_URL = 'https://github.com/WordPress/wordpress-develop.git'; @@ -663,11 +664,13 @@ ipcMain.handle('sites:set-label', async (_e, sitePath, label) => { return true; }); -ipcMain.handle('url:open', async (_e, url) => { - if (!url) return false; - await shell.openExternal(url); - return true; -}); +// Only the schemes the app actually uses reach the OS — see external-url.js for +// why. A refusal is logged rather than dropped so a future caller that trips the +// guard shows up in the log file instead of just doing nothing. +ipcMain.handle('url:open', async (_e, url) => openExternalUrl(url, { + openExternal: (target) => shell.openExternal(target), + onRefused: (description) => logEvent('url', `refused to open ${description} — only ${ALLOWED_URL_SCHEMES.join(', ')} are allowed`) +})); const ENGINE_RETRY_NOTICE = '\n⚠ This site requires a newer Node.js than this app bundles.\n Retrying with engine checks relaxed…\n\n'; diff --git a/test/external-url.test.cjs b/test/external-url.test.cjs new file mode 100644 index 0000000..7539635 --- /dev/null +++ b/test/external-url.test.cjs @@ -0,0 +1,162 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { isAllowedExternalUrl, normalizeExternalUrl, describeRefusedUrl, openExternalUrl, ALLOWED_URL_SCHEMES } = require('../src/external-url.js'); + +// Stands in for shell.openExternal, so "did this reach the OS?" is an assertion +// rather than something the test has to take on trust. +function recorder() { + const opened = []; + const refused = []; + return { + opened, + refused, + options: { + openExternal: async (url) => { opened.push(url); }, + onRefused: (description) => { refused.push(description); } + } + }; +} + +// The addresses the app actually passes today: Trac, the feedback form, the +// site and its admin on an ephemeral loopback port. +test('the addresses the app uses still open', async () => { + const rec = recorder(); + + for (const url of [ + 'https://core.trac.wordpress.org', + 'http://127.0.0.1:39372/', + 'http://127.0.0.1:8881/wp-admin/', + 'https://docs.google.com/forms/d/e/1FAIpQLS/viewform' + ]) { + assert.equal(await openExternalUrl(url, rec.options), true); + } + + // What reaches the OS is the parsed `href`, so that the address that was + // checked is the address that gets opened. For real callers that is the same + // string they passed, give or take the trailing slash the parser adds to a + // bare origin. + assert.deepEqual(rec.opened, [ + 'https://core.trac.wordpress.org/', + 'http://127.0.0.1:39372/', + 'http://127.0.0.1:8881/wp-admin/', + 'https://docs.google.com/forms/d/e/1FAIpQLS/viewform' + ]); + assert.deepEqual(rec.refused, []); +}); + +// The reason the OS gets the parsed form. The URL parser strips tabs and +// newlines from anywhere in the input, including the middle of the scheme, so +// this string validates as http — and if the raw text were forwarded, the OS +// would be resolving an address nothing had checked. +test('control characters cannot split what is checked from what is opened', async () => { + const rec = recorder(); + + assert.equal(await openExternalUrl('ht\ntp://example.com/x', rec.options), true); + assert.deepEqual(rec.opened, ['http://example.com/x']); + + assert.equal(normalizeExternalUrl('http://example.com/\tfoo'), 'http://example.com/foo'); +}); + +test('a file: address never reaches the OS', async () => { + const rec = recorder(); + + // The Windows one is the case that matters most: the OS association for a + // .exe is "run it", not "show it". + for (const url of ['file:///etc/passwd', 'file:///C:/Windows/System32/cmd.exe']) { + assert.equal(await openExternalUrl(url, rec.options), false); + } + + assert.deepEqual(rec.opened, []); + assert.equal(rec.refused.length, 2); +}); + +test('other schemes are refused too', async () => { + const rec = recorder(); + + for (const url of [ + 'javascript:alert(1)', + 'data:text/html,', + // Anything a third-party installer registered on the machine. + 'ms-msdt:/id PCWDiagnostic', + 'vscode://file/etc/hosts', + 'mailto:someone@example.com' + ]) { + assert.equal(await openExternalUrl(url, rec.options), false); + } + + assert.deepEqual(rec.opened, []); + assert.equal(rec.refused.length, 5); +}); + +test('junk input is refused rather than thrown', async () => { + const rec = recorder(); + + for (const url of ['', ' ', null, undefined, 42, {}, ['https://example.com'], 'not a url']) { + assert.equal(await openExternalUrl(url, rec.options), false); + } + + assert.deepEqual(rec.opened, []); +}); + +test('the scheme is read off the parsed URL, not the raw string', () => { + // Casing and leading whitespace are normalized by the URL parser before the + // comparison, so they are neither a false refusal nor a way past the guard. + assert.equal(isAllowedExternalUrl('HTTPS://example.com'), true); + assert.equal(isAllowedExternalUrl(' https://example.com'), true); + assert.equal(isAllowedExternalUrl('FILE:///etc/passwd'), false); + assert.equal(isAllowedExternalUrl(' file:///etc/passwd'), false); +}); + +test('the allow-list is only http and https', () => { + // A guard against widening it by accident: adding a scheme should be a + // deliberate change with a reason, and this test is where that shows up. + assert.deepEqual(ALLOWED_URL_SCHEMES, ['http:', 'https:']); +}); + +test('a refusal reports the address, truncated', async () => { + const rec = recorder(); + const long = `file:///${'a'.repeat(500)}`; + + await openExternalUrl(long, rec.options); + + assert.equal(rec.refused.length, 1); + assert.ok(rec.refused[0].length < 200, 'the log line should not carry a 500-character address'); + assert.ok(rec.refused[0].startsWith('file:///aaa'), 'enough of the address to diagnose the caller'); +}); + +// The address is about to be written into the file contributors attach to bug +// reports, and electron-log passes newlines through unchanged. Left as-is, a +// refused address could close the log line and open another one in the app's own +// timestamp-and-scope format — a log that describes events that never happened. +test('a refused address cannot forge a second log line', async () => { + const rec = recorder(); + const forged = 'file:///tmp/x\n[2026-08-06 10:00:00.000] [info] (app) update completed successfully'; + + await openExternalUrl(forged, rec.options); + + assert.equal(rec.refused.length, 1); + assert.ok(!rec.refused[0].includes('\n'), 'the description must stay on one line'); + // Escaped, not dropped: the line still says what the caller actually sent. + assert.ok(rec.refused[0].includes('file:///tmp/x\\x0a[2026-08-06')); +}); + +test('every control character is escaped, not just newlines', () => { + // Carriage return alone ends a line in some viewers, and U+2028/U+2029 do it + // in others, so the whole class is escaped rather than the obvious member. + assert.equal(describeRefusedUrl('file:///a\rb'), 'file:///a\\x0db'); + assert.equal(describeRefusedUrl('file:///a\tb'), 'file:///a\\x09b'); + assert.equal(describeRefusedUrl('file:///a\u2028b'), 'file:///a\\u2028b'); + assert.equal(describeRefusedUrl('file:///a\u0000b'), 'file:///a\\x00b'); + // Ordinary addresses are untouched. + assert.equal(describeRefusedUrl('file:///etc/passwd'), 'file:///etc/passwd'); +}); + +test('truncation is applied to the escaped form', () => { + // Escaping expands the string, so truncating first would let an address of + // control characters land in the log several times over the cap. + const description = describeRefusedUrl(`file:${'\n'.repeat(500)}`); + + assert.ok(description.length <= 121, `escaped description was ${description.length} characters`); + assert.ok(!description.includes('\n')); +});