Only open http and https addresses externally

.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

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
+};

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';
 

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,<script>alert(1)</script>',
+		// 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'));
+});