From 3cce94aa8b88aa5aad343c8600b4c35b192a902b Mon Sep 17 00:00:00 2001 From: Amit Raj Date: Tue, 18 Aug 2026 12:00:35 +0530 Subject: [PATCH 1/3] Open links in the browser for every click gesture The site links each cancel their own navigation in an onClick handler. 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 and no way back. Refuse navigation and window creation once for the app window instead, and hand the address to the system browser. Every link is covered, including ones added later, and the address goes out through the same external-url.js gate the renderer already uses. Fixes #284 --- src/main.js | 24 +++++-- src/window-links.js | 55 ++++++++++++++++ test/window-links.test.cjs | 130 +++++++++++++++++++++++++++++++++++++ 3 files changed, 202 insertions(+), 7 deletions(-) create mode 100644 src/window-links.js create mode 100644 test/window-links.test.cjs diff --git a/src/main.js b/src/main.js index 9d1a272..b92020e 100644 --- a/src/main.js +++ b/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 { createSetupTracker } = require('./setup-tracker'); const { planInitialRead, planTailRead } = require('./log-tail'); @@ -54,6 +55,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 @@ -404,6 +414,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) { @@ -2166,12 +2180,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 ----------------------------------------------- // diff --git a/src/window-links.js b/src/window-links.js new file mode 100644 index 0000000..b163e23 --- /dev/null +++ b/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 + // that does not fire it, and would otherwise move the window. + wc.on('will-navigate', sendToBrowser); + wc.on('will-redirect', sendToBrowser); +} + +module.exports = { openLinksExternally }; diff --git a/test/window-links.test.cjs b/test/window-links.test.cjs new file mode 100644 index 0000000..5f1bf0d --- /dev/null +++ b/test/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 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(); +}); From 970dbaf3132657c0bc8cf959599507b53414fb38 Mon Sep 17 00:00:00 2001 From: Amit Raj Date: Wed, 19 Aug 2026 11:08:11 +0530 Subject: [PATCH 2/3] Hold the app window to its own page, and log an open that fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the review pass on this branch. The window guard only intercepted http and https, so anything else was left to navigate the main window. That window is the one with the preload bridge attached, and it renders content the app does not author — captured email bodies among it — where a link can point at a local path or at one relative to the app's own file: origin. It now refuses every navigation except a reload, which is the one case that has to keep working; a plain deny breaks reloading the window. The onFailed reporter also only covered the link path. openExternalUrl ignored it and let a rejecting openExternal propagate out of the url:open handler, where none of the renderer's 23 call sites catches anything — so an ordinary click on an address the OS cannot open was still a link that did nothing with nothing in the log. Reporting moved into openExternalUrl, which is where both paths meet. Tests for both fail on the previous code. --- src/external-url.js | 13 ++++++++-- src/main.js | 2 +- src/window-links.js | 51 ++++++++++++++++++++++---------------- test/external-url.test.cjs | 26 +++++++++++++++++++ test/window-links.test.cjs | 33 ++++++++++++++++++++---- 5 files changed, 95 insertions(+), 30 deletions(-) diff --git a/src/external-url.js b/src/external-url.js index 22a6dc1..9eb0994 100644 --- a/src/external-url.js +++ b/src/external-url.js @@ -65,7 +65,7 @@ function describeRefusedUrl(url) { // 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 } = {}) { +async function openExternalUrl(url, { openExternal, onRefused, onFailed } = {}) { const target = normalizeExternalUrl(url); if (target === null) { @@ -73,7 +73,16 @@ async function openExternalUrl(url, { openExternal, onRefused } = {}) { return false; } - await openExternal(target); + // `openExternal` rejects when the OS has no application registered for the + // address. Every caller in the app fires this and forgets it, so a rejection + // left to propagate is a link that did nothing with nothing in the log. + try { + await openExternal(target); + } catch (error) { + if (typeof onFailed === 'function') onFailed(target, error); + return false; + } + return true; } diff --git a/src/main.js b/src/main.js index b92020e..d534bc7 100644 --- a/src/main.js +++ b/src/main.js @@ -61,7 +61,7 @@ const { getStore } = require('./settings-store'); 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}`) + onFailed: (url, error) => logEvent('url', `could not open ${describeRefusedUrl(url)}: ${String(error && error.message ? error.message : error)}`) }; // One name for the send-only progress channel (#173), shared with preload.js diff --git a/src/window-links.js b/src/window-links.js index b163e23..6271cd0 100644 --- a/src/window-links.js +++ b/src/window-links.js @@ -1,4 +1,5 @@ -// Keeps links in the app window opening in the contributor's browser (#284). +// Keeps the app window on its own page, and links 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 @@ -7,29 +8,32 @@ // 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. +// added later. The window refuses to go anywhere and refuses to open children; +// an http/https address goes out through external-url.js instead, the same gate +// the renderer's own openExternal calls use. +// +// The refusal is a default, not a list of cases: this window renders content the +// app does not author — captured email bodies among it — and a link in there can +// point anywhere, including at a path relative to the app's own file: origin. +// Only a reload is let through. `pinToTrac` in trac-view.js is the same idea for +// the window that shows Trac. -const { isAllowedExternalUrl, openExternalUrl } = require('./external-url'); +const { openExternalUrl } = require('./external-url'); /** - * Keeps a window on its own page and sends any link it opens to the browser. + * Holds a window on its current page and sends links out 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. + * @param {Object} [deps] Passed through to openExternalUrl: `openExternal`, + * `onRefused`, `onFailed`. */ -function openLinksExternally(wc, { openExternal, onRefused, onFailed } = {}) { +function openLinksExternally(wc, deps = {}) { // 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. + // hand-off cannot be awaited. openExternalUrl reports its own refusals and + // failures, so this catch is only there for a reporter that itself throws, + // which must not surface as an unhandled rejection. const handOff = (url) => { - Promise.resolve(openExternalUrl(url, { openExternal, onRefused })).catch((error) => { - if (typeof onFailed === 'function') onFailed(url, error); - }); + Promise.resolve(openExternalUrl(url, deps)).catch(() => {}); }; // Middle click, Cmd/Ctrl+click, target="_blank", window.open. A child window @@ -39,17 +43,20 @@ function openLinksExternally(wc, { openExternal, onRefused, onFailed } = {}) { 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; + // A click no handler cancelled, or a script navigation. Everything is + // refused except a reload, which asks to navigate to the page already + // loaded — denying that one would stop the window reloading. The address is + // then offered to the browser, where external-url.js refuses anything + // outside http/https and logs it. + const stayPut = (event, url) => { + if (url === wc.getURL()) return; event.preventDefault(); handOff(url); }; // will-navigate is the click. will-redirect is the 3xx or // that does not fire it, and would otherwise move the window. - wc.on('will-navigate', sendToBrowser); - wc.on('will-redirect', sendToBrowser); + wc.on('will-navigate', stayPut); + wc.on('will-redirect', stayPut); } module.exports = { openLinksExternally }; diff --git a/test/external-url.test.cjs b/test/external-url.test.cjs index 7539635..093edc0 100644 --- a/test/external-url.test.cjs +++ b/test/external-url.test.cjs @@ -18,6 +18,32 @@ function recorder() { }; } +// openExternal rejects when the OS has no application registered for the +// address. Every caller in the app fires and forgets, so if this is not reported +// here it is not reported anywhere: the link did nothing and the log says +// nothing about it. +test('an address the OS cannot open is reported, not left to the caller', async () => { + const failures = []; + + const result = await openExternalUrl('https://example.com/', { + openExternal: async () => { throw new Error('no application registered'); }, + onFailed: (url, error) => { failures.push([url, error.message]); } + }); + + assert.equal(result, false); + assert.deepEqual(failures, [['https://example.com/', 'no application registered']]); +}); + +test('a failure with no reporter does not reject on the caller', async () => { + // The link handlers in window-links.js are synchronous event listeners and + // cannot await this, so it must not come back as a rejection. + const result = await openExternalUrl('https://example.com/', { + openExternal: async () => { throw new Error('no application registered'); } + }); + + assert.equal(result, false); +}); + // 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 () => { diff --git a/test/window-links.test.cjs b/test/window-links.test.cjs index 5f1bf0d..705170b 100644 --- a/test/window-links.test.cjs +++ b/test/window-links.test.cjs @@ -6,10 +6,11 @@ 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() { +function fakeWebContents(currentUrl = 'file:///Applications/toolkit/renderer/index.html') { const listeners = {}; return { windowOpenHandler: null, + getURL() { return currentUrl; }, setWindowOpenHandler(handler) { this.windowOpenHandler = handler; }, on(event, listener) { (listeners[event] ||= []).push(listener); }, // Replays a gesture; returns whether the navigation was cancelled. @@ -74,10 +75,10 @@ test('a redirect cannot move the app window either', async () => { 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(); +test('a reload is let through', async () => { + // A reload asks to navigate to the page already loaded. Cancelling it would + // stop the app window reloading. + const wc = fakeWebContents('file:///Applications/toolkit/renderer/index.html'); const rec = recorder(); openLinksExternally(wc, rec.deps); @@ -88,6 +89,28 @@ test("the app's own page is left alone", async () => { assert.deepEqual(rec.refused, []); }); +test('the window cannot be navigated off its own page', async () => { + // The window renders content the app does not author - a captured email body + // among it - and a link in there can point at a local path or at one relative + // to the app's own file: origin. Neither is a page this window shows, and + // neither is an address to hand to the OS. + const wc = fakeWebContents('file:///Applications/toolkit/renderer/index.html'); + const rec = recorder(); + openLinksExternally(wc, rec.deps); + + for (const url of [ + 'file:///Applications/toolkit/renderer/other.html', + 'file:///etc/passwd' + ]) { + assert.equal(wc.emit('will-navigate', url), true, `${url} should be refused`); + } + await settled(); + + // Refused by the gate rather than opened, and each refusal is logged. + assert.deepEqual(rec.opened, []); + assert.equal(rec.refused.length, 2); +}); + test('a scheme the app does not open is refused, and opens no window', async () => { const wc = fakeWebContents(); const rec = recorder(); From c141c6927b0ea1f1b8708bffa198da108a60ac8d Mon Sep 17 00:00:00 2001 From: Amit Raj Date: Wed, 19 Aug 2026 11:18:23 +0530 Subject: [PATCH 3/3] Revert the window-hardening and openExternal logging changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both were out of scope for #284. The navigation hole they closed — the app window following a file: or relative link out of its own page — is on trunk already and is not something this fix introduced, and the missing failure log on the url:open path is a gap in external-url.js rather than in the link handling. Neither belongs in a PR that fixes one reported bug. Both are worth doing on their own; noted in the PR description so they are not lost. --- src/external-url.js | 13 ++-------- src/main.js | 2 +- src/window-links.js | 51 ++++++++++++++++---------------------- test/external-url.test.cjs | 26 ------------------- test/window-links.test.cjs | 33 ++++-------------------- 5 files changed, 30 insertions(+), 95 deletions(-) diff --git a/src/external-url.js b/src/external-url.js index 9eb0994..22a6dc1 100644 --- a/src/external-url.js +++ b/src/external-url.js @@ -65,7 +65,7 @@ function describeRefusedUrl(url) { // 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, onFailed } = {}) { +async function openExternalUrl(url, { openExternal, onRefused } = {}) { const target = normalizeExternalUrl(url); if (target === null) { @@ -73,16 +73,7 @@ async function openExternalUrl(url, { openExternal, onRefused, onFailed } = {}) return false; } - // `openExternal` rejects when the OS has no application registered for the - // address. Every caller in the app fires this and forgets it, so a rejection - // left to propagate is a link that did nothing with nothing in the log. - try { - await openExternal(target); - } catch (error) { - if (typeof onFailed === 'function') onFailed(target, error); - return false; - } - + await openExternal(target); return true; } diff --git a/src/main.js b/src/main.js index d534bc7..b92020e 100644 --- a/src/main.js +++ b/src/main.js @@ -61,7 +61,7 @@ const { getStore } = require('./settings-store'); 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)}: ${String(error && error.message ? error.message : error)}`) + 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 diff --git a/src/window-links.js b/src/window-links.js index 6271cd0..b163e23 100644 --- a/src/window-links.js +++ b/src/window-links.js @@ -1,5 +1,4 @@ -// Keeps the app window on its own page, and links opening in the contributor's -// browser (#284). +// 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 @@ -8,32 +7,29 @@ // 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 window refuses to go anywhere and refuses to open children; -// an http/https address goes out through external-url.js instead, the same gate -// the renderer's own openExternal calls use. -// -// The refusal is a default, not a list of cases: this window renders content the -// app does not author — captured email bodies among it — and a link in there can -// point anywhere, including at a path relative to the app's own file: origin. -// Only a reload is let through. `pinToTrac` in trac-view.js is the same idea for -// the window that shows Trac. +// added later. The address goes out through external-url.js, the same gate the +// renderer's own openExternal calls use. -const { openExternalUrl } = require('./external-url'); +const { isAllowedExternalUrl, openExternalUrl } = require('./external-url'); /** - * Holds a window on its current page and sends links out to the browser. + * Keeps a window on its own page and sends any link it opens to the browser. * * @param {import('electron').WebContents} wc - * @param {Object} [deps] Passed through to openExternalUrl: `openExternal`, - * `onRefused`, `onFailed`. + * @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, deps = {}) { +function openLinksExternally(wc, { openExternal, onRefused, onFailed } = {}) { // These events are synchronous and ignore what the handler returns, so the - // hand-off cannot be awaited. openExternalUrl reports its own refusals and - // failures, so this catch is only there for a reporter that itself throws, - // which must not surface as an unhandled rejection. + // 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, deps)).catch(() => {}); + 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 @@ -43,20 +39,17 @@ function openLinksExternally(wc, deps = {}) { return { action: 'deny' }; }); - // A click no handler cancelled, or a script navigation. Everything is - // refused except a reload, which asks to navigate to the page already - // loaded — denying that one would stop the window reloading. The address is - // then offered to the browser, where external-url.js refuses anything - // outside http/https and logs it. - const stayPut = (event, url) => { - if (url === wc.getURL()) return; + // 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 // that does not fire it, and would otherwise move the window. - wc.on('will-navigate', stayPut); - wc.on('will-redirect', stayPut); + wc.on('will-navigate', sendToBrowser); + wc.on('will-redirect', sendToBrowser); } module.exports = { openLinksExternally }; diff --git a/test/external-url.test.cjs b/test/external-url.test.cjs index 093edc0..7539635 100644 --- a/test/external-url.test.cjs +++ b/test/external-url.test.cjs @@ -18,32 +18,6 @@ function recorder() { }; } -// openExternal rejects when the OS has no application registered for the -// address. Every caller in the app fires and forgets, so if this is not reported -// here it is not reported anywhere: the link did nothing and the log says -// nothing about it. -test('an address the OS cannot open is reported, not left to the caller', async () => { - const failures = []; - - const result = await openExternalUrl('https://example.com/', { - openExternal: async () => { throw new Error('no application registered'); }, - onFailed: (url, error) => { failures.push([url, error.message]); } - }); - - assert.equal(result, false); - assert.deepEqual(failures, [['https://example.com/', 'no application registered']]); -}); - -test('a failure with no reporter does not reject on the caller', async () => { - // The link handlers in window-links.js are synchronous event listeners and - // cannot await this, so it must not come back as a rejection. - const result = await openExternalUrl('https://example.com/', { - openExternal: async () => { throw new Error('no application registered'); } - }); - - assert.equal(result, false); -}); - // 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 () => { diff --git a/test/window-links.test.cjs b/test/window-links.test.cjs index 705170b..5f1bf0d 100644 --- a/test/window-links.test.cjs +++ b/test/window-links.test.cjs @@ -6,11 +6,10 @@ 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(currentUrl = 'file:///Applications/toolkit/renderer/index.html') { +function fakeWebContents() { const listeners = {}; return { windowOpenHandler: null, - getURL() { return currentUrl; }, setWindowOpenHandler(handler) { this.windowOpenHandler = handler; }, on(event, listener) { (listeners[event] ||= []).push(listener); }, // Replays a gesture; returns whether the navigation was cancelled. @@ -75,10 +74,10 @@ test('a redirect cannot move the app window either', async () => { assert.deepEqual(rec.opened, ['https://wordpress.org/']); }); -test('a reload is let through', async () => { - // A reload asks to navigate to the page already loaded. Cancelling it would - // stop the app window reloading. - const wc = fakeWebContents('file:///Applications/toolkit/renderer/index.html'); +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); @@ -89,28 +88,6 @@ test('a reload is let through', async () => { assert.deepEqual(rec.refused, []); }); -test('the window cannot be navigated off its own page', async () => { - // The window renders content the app does not author - a captured email body - // among it - and a link in there can point at a local path or at one relative - // to the app's own file: origin. Neither is a page this window shows, and - // neither is an address to hand to the OS. - const wc = fakeWebContents('file:///Applications/toolkit/renderer/index.html'); - const rec = recorder(); - openLinksExternally(wc, rec.deps); - - for (const url of [ - 'file:///Applications/toolkit/renderer/other.html', - 'file:///etc/passwd' - ]) { - assert.equal(wc.emit('will-navigate', url), true, `${url} should be refused`); - } - await settled(); - - // Refused by the gate rather than opened, and each refusal is logged. - assert.deepEqual(rec.opened, []); - assert.equal(rec.refused.length, 2); -}); - test('a scheme the app does not open is refused, and opens no window', async () => { const wc = fakeWebContents(); const rec = recorder();