From a926802bc7d56f7bb42a0ef3e343ef8aeb2e7330 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Wed, 5 Aug 2026 18:39:30 +0200 Subject: [PATCH 1/3] Only open http and https addresses externally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `url:open` handler passed whatever it received to `shell.openExternal`, which hands an address to whatever application the OS registered for its scheme. That is a 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 any other registered scheme is reachable the same way. Every caller passes an http/https address today, so nothing misuses it. It matters as the second half of a chain: the renderer displays content the app does not author, and the URL it auto-opens on server start is parsed out of the Playground server's stdout. This guard is the step that keeps any future influence over that string from becoming an action on the contributor's machine. The check lives in its own module so both sides of it can be tested without an Electron process — `shell.openExternal` in the app, a recording stub in the tests. 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. Co-Authored-By: Claude Opus 5 (1M context) --- src/external-url.js | 65 ++++++++++++++++++++++ src/main.js | 13 +++-- test/external-url.test.cjs | 111 +++++++++++++++++++++++++++++++++++++ 3 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 src/external-url.js create mode 100644 test/external-url.test.cjs diff --git a/src/external-url.js b/src/external-url.js new file mode 100644 index 0000000..e77c82b --- /dev/null +++ b/src/external-url.js @@ -0,0 +1,65 @@ +// 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:']; + +// 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. +function isAllowedExternalUrl(url) { + if (typeof url !== 'string' || url.trim() === '') return false; + + let parsed; + try { + parsed = new URL(url); + } catch { + return false; + } + + return ALLOWED_URL_SCHEMES.includes(parsed.protocol); +} + +// Truncated because a refused address is attacker-influenced by hypothesis, and +// the log is a file people paste into issue threads. +function describeRefusedUrl(url) { + if (typeof url !== 'string') return `<${url === null ? 'null' : typeof url}>`; + if (url.length <= 120) return url; + return `${url.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 } = {}) { + if (!isAllowedExternalUrl(url)) { + if (typeof onRefused === 'function') onRefused(describeRefusedUrl(url)); + return false; + } + + await openExternal(url); + return true; +} + +module.exports = { + ALLOWED_URL_SCHEMES, + 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..74bae75 --- /dev/null +++ b/test/external-url.test.cjs @@ -0,0 +1,111 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { isAllowedExternalUrl, 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); + } + + // Passed through byte for byte — the guard inspects the parsed URL but must + // not hand the OS a normalized rewrite of what the caller asked for. + 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, []); +}); + +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'); +}); From ae2c595b9caf3c48cf2591c0842f27ee1691f448 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Wed, 5 Aug 2026 19:24:17 +0200 Subject: [PATCH 2/3] Open the parsed address, not the caller's string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checking one string and opening another is not a check. The URL parser strips tabs and newlines from anywhere in the input, including the middle of the scheme, so 'ht\ntp://example.com/x' validates as http: while `openExternal` would receive an address carrying a newline for the OS parser to resolve by its own rules. Forwarding `parsed.href` closes the gap between the two: the address that was checked is the address that gets opened. For every caller in the app the two forms are identical but for the trailing slash the parser adds to a bare origin. The rules file listed this handler as its known-open calibration example, which stops being true here. Replaced with what the fix teaches — the allow-list and the normalization are two halves, and the second is the one that is easy to miss in the next handler that takes a URL or a path. Co-Authored-By: Claude Opus 5 (1M context) --- .../instructions/code-review.instructions.md | 11 ++++--- src/external-url.js | 32 +++++++++++++++---- test/external-url.test.cjs | 23 ++++++++++--- 3 files changed, 52 insertions(+), 14 deletions(-) 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 index e77c82b..c585805 100644 --- a/src/external-url.js +++ b/src/external-url.js @@ -19,21 +19,38 @@ 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. -function isAllowedExternalUrl(url) { - if (typeof url !== 'string' || url.trim() === '') return false; +// +// 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 false; + return null; } - return ALLOWED_URL_SCHEMES.includes(parsed.protocol); + if (!ALLOWED_URL_SCHEMES.includes(parsed.protocol)) return null; + + return parsed.href; +} + +function isAllowedExternalUrl(url) { + return normalizeExternalUrl(url) !== null; } // Truncated because a refused address is attacker-influenced by hypothesis, and @@ -48,17 +65,20 @@ function describeRefusedUrl(url) { // 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 } = {}) { - if (!isAllowedExternalUrl(url)) { + const target = normalizeExternalUrl(url); + + if (target === null) { if (typeof onRefused === 'function') onRefused(describeRefusedUrl(url)); return false; } - await openExternal(url); + await openExternal(target); return true; } module.exports = { ALLOWED_URL_SCHEMES, + normalizeExternalUrl, isAllowedExternalUrl, describeRefusedUrl, openExternalUrl diff --git a/test/external-url.test.cjs b/test/external-url.test.cjs index 74bae75..0f1b4d1 100644 --- a/test/external-url.test.cjs +++ b/test/external-url.test.cjs @@ -1,7 +1,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { isAllowedExternalUrl, openExternalUrl, ALLOWED_URL_SCHEMES } = require('../src/external-url.js'); +const { isAllowedExternalUrl, normalizeExternalUrl, 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. @@ -32,10 +32,12 @@ test('the addresses the app uses still open', async () => { assert.equal(await openExternalUrl(url, rec.options), true); } - // Passed through byte for byte — the guard inspects the parsed URL but must - // not hand the OS a normalized rewrite of what the caller asked for. + // 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', + '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' @@ -43,6 +45,19 @@ test('the addresses the app uses still open', async () => { 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(); From aadd1ec9bd5a16bdc96a8cc0486e52f772292752 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Thu, 6 Aug 2026 10:12:39 +0200 Subject: [PATCH 3/3] Keep a refused address from forging log lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The refused address goes into the log verbatim, and electron-log passes newlines through unchanged, so an address carrying one could close its line and open another in the app's own timestamp-and-scope format. A log that can be made to describe events that never happened is worse than no log — and being the diagnosable trace for a caller that trips the guard is the whole reason the refusal is logged at all. Control characters are now escaped rather than dropped, so the line still says what the caller actually sent, and truncation runs afterwards, since escaping is what decides the final length. Co-Authored-By: Claude Opus 5 (1M context) --- src/external-url.js | 30 ++++++++++++++++++++++++++---- test/external-url.test.cjs | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 5 deletions(-) diff --git a/src/external-url.js b/src/external-url.js index c585805..5b59a28 100644 --- a/src/external-url.js +++ b/src/external-url.js @@ -53,12 +53,34 @@ function isAllowedExternalUrl(url) { return normalizeExternalUrl(url) !== null; } -// Truncated because a refused address is attacker-influenced by hypothesis, and -// the log is a file people paste into issue threads. +// 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}>`; - if (url.length <= 120) return url; - return `${url.slice(0, 120)}…`; + + 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 diff --git a/test/external-url.test.cjs b/test/external-url.test.cjs index 0f1b4d1..7539635 100644 --- a/test/external-url.test.cjs +++ b/test/external-url.test.cjs @@ -1,7 +1,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { isAllowedExternalUrl, normalizeExternalUrl, openExternalUrl, ALLOWED_URL_SCHEMES } = require('../src/external-url.js'); +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. @@ -124,3 +124,39 @@ test('a refusal reports the address, truncated', async () => { 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')); +});