Validation only (do not merge): trunk merge for #358

src/main.js

@@ -36,7 +36,8 @@ const { getClientId: getGithubClientId, requestDeviceCode, pollForToken, fetchVi
 const { openPullRequest, buildPullRequestBody, testMode: githubTestMode } = require('./github-pr.cjs');
 const { buildPullRequestEntries } = require('./pr-files.cjs');
 const { openAndScrape, fetchAttachment } = require('./trac-view');
-const { openExternalUrl, ALLOWED_URL_SCHEMES } = require('./external-url');
+const { openExternalUrl, describeRefusedUrl, ALLOWED_URL_SCHEMES } = require('./external-url');
+const { openLinksExternally } = require('./window-links');
 const { deleteRegisteredSite, revealRegisteredSite, clearRegisteredSiteLog } = require('./site-registry');
 const { removeTree } = require('./remove-tree');
 const { createSetupTracker } = require('./setup-tracker');
@@ -55,6 +56,15 @@ const {
 const { createProgressThrottle, describeSwitchProgress } = require('./switch-progress.cjs');
 const { getStore } = require('./settings-store');
 
+// How an address leaves this app, shared by the renderer's `url:open` and the
+// link handling in window-links.js. A refusal is logged rather than dropped, so
+// a caller that trips the guard shows up in the log instead of doing nothing.
+const externalUrlDeps = {
+	openExternal: (target) => shell.openExternal(target),
+	onRefused: (description) => logEvent('url', `refused to open ${description} — only ${ALLOWED_URL_SCHEMES.join(', ')} are allowed`),
+	onFailed: (url, error) => logEvent('url', `could not open ${describeRefusedUrl(url)}: ${error && error.message}`)
+};
+
 // One name for the send-only progress channel (#173), shared with preload.js
 // through the tests rather than by import — the renderer bundle and the main
 // process do not share a module graph, and a rename that only lands on one side
@@ -405,6 +415,10 @@ function createWindow() {
 		}
 	});
 
+	// Links belong in the contributor's browser, not in a window of this app
+	// (#284). Set before the page loads so the first click is covered too.
+	openLinksExternally(mainWindow.webContents, externalUrlDeps);
+
 	mainWindow.loadFile(path.join(__dirname, 'renderer', 'index.html'));
 }
 function buildPatchHtml(content) {
@@ -2191,12 +2205,8 @@ ipcMain.handle('branches:delete', async (_e, sitePath, targetRef) => withRegiste
 }));
 
 // 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`)
-}));
+// why.
+ipcMain.handle('url:open', async (_e, url) => openExternalUrl(url, externalUrlDeps));
 
 // --- opening a site's code -----------------------------------------------
 //

src/window-links.js

@@ -0,0 +1,55 @@
+// Keeps links in the app window opening in the contributor's browser (#284).
+//
+// Each link cancels its own navigation in its onClick handler. That covers a
+// plain click, but a middle click fires `auxclick`, which a click handler never
+// sees — so Chromium's "open in a new window" default ran and the site loaded
+// inside a bare app window with no address bar. Cmd/Ctrl+click was fine, since
+// that does arrive as a click the handler can cancel.
+//
+// Refusing once for the whole window fixes it for every link, including ones
+// added later. The address goes out through external-url.js, the same gate the
+// renderer's own openExternal calls use.
+
+const { isAllowedExternalUrl, openExternalUrl } = require('./external-url');
+
+/**
+ * Keeps a window on its own page and sends any link it opens to the browser.
+ *
+ * @param {import('electron').WebContents} wc
+ * @param {Object}                         [deps]
+ * @param {Function}                       [deps.openExternal] `shell.openExternal` in the app, a stub in tests.
+ * @param {Function}                       [deps.onRefused]    Called with a description of a refused address.
+ * @param {Function}                       [deps.onFailed]     Called with the address and error when opening fails.
+ */
+function openLinksExternally(wc, { openExternal, onRefused, onFailed } = {}) {
+	// These events are synchronous and ignore what the handler returns, so the
+	// hand-off cannot be awaited. The failure is reported rather than dropped:
+	// openExternal rejects when the OS has no handler for the address, and from
+	// the contributor's chair that is a link that did nothing.
+	const handOff = (url) => {
+		Promise.resolve(openExternalUrl(url, { openExternal, onRefused })).catch((error) => {
+			if (typeof onFailed === 'function') onFailed(url, error);
+		});
+	};
+
+	// Middle click, Cmd/Ctrl+click, target="_blank", window.open. A child window
+	// is never this app's UI, so it is denied whatever the address is.
+	wc.setWindowOpenHandler(({ url }) => {
+		handOff(url);
+		return { action: 'deny' };
+	});
+
+	// A click no handler cancelled, or a script navigation. Only http/https is
+	// taken over: the app's own page is a file: URL and has to stay loadable.
+	const sendToBrowser = (event, url) => {
+		if (!isAllowedExternalUrl(url)) return;
+		event.preventDefault();
+		handOff(url);
+	};
+	// will-navigate is the click. will-redirect is the 3xx or <meta refresh>
+	// that does not fire it, and would otherwise move the window.
+	wc.on('will-navigate', sendToBrowser);
+	wc.on('will-redirect', sendToBrowser);
+}
+
+module.exports = { openLinksExternally };

tests/unit/window-links.test.cjs

@@ -0,0 +1,130 @@
+const test = require('node:test');
+const assert = require('node:assert/strict');
+
+const { openLinksExternally } = require('../../src/window-links.js');
+
+// Stands in for a window's webContents, so the gestures can be replayed without
+// an Electron process - the same approach external-url.test.cjs takes with the
+// shell.
+function fakeWebContents() {
+	const listeners = {};
+	return {
+		windowOpenHandler: null,
+		setWindowOpenHandler(handler) { this.windowOpenHandler = handler; },
+		on(event, listener) { (listeners[event] ||= []).push(listener); },
+		// Replays a gesture; returns whether the navigation was cancelled.
+		emit(event, url) {
+			let prevented = false;
+			const fakeEvent = { preventDefault() { prevented = true; } };
+			for (const listener of listeners[event] || []) listener(fakeEvent, url);
+			return prevented;
+		}
+	};
+}
+
+function recorder() {
+	const opened = [];
+	const refused = [];
+	return {
+		opened,
+		refused,
+		deps: {
+			openExternal: async (url) => { opened.push(url); },
+			onRefused: (description) => { refused.push(description); }
+		}
+	};
+}
+
+// The hand-off is a promise the synchronous handlers cannot await.
+const settled = () => new Promise((resolve) => setImmediate(resolve));
+
+test('a middle click opens the browser instead of a window (#284)', async () => {
+	const wc = fakeWebContents();
+	const rec = recorder();
+	openLinksExternally(wc, rec.deps);
+
+	const result = wc.windowOpenHandler({ url: 'http://127.0.0.1:39372/wp-admin/' });
+	await settled();
+
+	assert.deepEqual(result, { action: 'deny' });
+	assert.deepEqual(rec.opened, ['http://127.0.0.1:39372/wp-admin/']);
+});
+
+test('a click no handler cancelled still opens the browser', async () => {
+	const wc = fakeWebContents();
+	const rec = recorder();
+	openLinksExternally(wc, rec.deps);
+
+	assert.equal(wc.emit('will-navigate', 'https://core.trac.wordpress.org/ticket/284'), true);
+	await settled();
+
+	assert.deepEqual(rec.opened, ['https://core.trac.wordpress.org/ticket/284']);
+});
+
+test('a redirect cannot move the app window either', async () => {
+	// will-redirect covers the 3xx and <meta refresh> that never fire
+	// will-navigate - a login redirect on the site would land here.
+	const wc = fakeWebContents();
+	const rec = recorder();
+	openLinksExternally(wc, rec.deps);
+
+	assert.equal(wc.emit('will-redirect', 'https://wordpress.org/'), true);
+	await settled();
+
+	assert.deepEqual(rec.opened, ['https://wordpress.org/']);
+});
+
+test("the app's own page is left alone", async () => {
+	// Cancelling this would stop the app window reloading, and a file: address is
+	// not one to hand to the browser.
+	const wc = fakeWebContents();
+	const rec = recorder();
+	openLinksExternally(wc, rec.deps);
+
+	assert.equal(wc.emit('will-navigate', 'file:///Applications/toolkit/renderer/index.html'), false);
+	await settled();
+
+	assert.deepEqual(rec.opened, []);
+	assert.deepEqual(rec.refused, []);
+});
+
+test('a scheme the app does not open is refused, and opens no window', async () => {
+	const wc = fakeWebContents();
+	const rec = recorder();
+	openLinksExternally(wc, rec.deps);
+
+	assert.deepEqual(wc.windowOpenHandler({ url: 'file:///etc/passwd' }), { action: 'deny' });
+	await settled();
+
+	assert.deepEqual(rec.opened, []);
+	assert.equal(rec.refused.length, 1);
+});
+
+test('a failed open is reported, not swallowed', async () => {
+	// shell.openExternal rejects when the OS has nothing registered. Nothing else
+	// is in a position to catch it, and an unreported failure is a link that did
+	// nothing.
+	const wc = fakeWebContents();
+	const failures = [];
+	openLinksExternally(wc, {
+		openExternal: async () => { throw new Error('no handler'); },
+		onFailed: (url, error) => { failures.push([url, error.message]); }
+	});
+
+	wc.emit('will-navigate', 'https://example.com/');
+	wc.windowOpenHandler({ url: 'https://example.com/' });
+	await settled();
+
+	assert.deepEqual(failures, [
+		['https://example.com/', 'no handler'],
+		['https://example.com/', 'no handler']
+	]);
+});
+
+test('a failure with no reporter still does not crash the app', async () => {
+	const wc = fakeWebContents();
+	openLinksExternally(wc, { openExternal: async () => { throw new Error('no handler'); } });
+
+	wc.emit('will-navigate', 'https://example.com/');
+	await settled();
+});