diff --git a/src/main.js b/src/main.js index 639eed8..b1701ae 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 { 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 ----------------------------------------------- // 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/tests/unit/window-links.test.cjs b/tests/unit/window-links.test.cjs new file mode 100644 index 0000000..1a30f3c --- /dev/null +++ b/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 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(); +});