From d377b1f9feac0ef3b4f0dce4f29aefb262947c4f Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 5 Oct 2026 03:36:47 +0000 Subject: [PATCH] fix: open an approval window while another one has focus (closes #290) The background centred each approval window on the last focused window, which could be an earlier approval window still open. Headless Chrome reports one as 1280x720, so the position came out where the browser refused to create the window ("Bounds must be at least 50% within visible screen space"), and the request failed with -32603 and no window. It now centres on the last focused browser window. Under load a test in the Chrome suite raised its prompt while the previous test's window was still closing, and either hit that refusal or took the closing window for its own. The runner now closes every approval window between tests. Model: opus-5-5 --- README.md | 24 +++++----- TODO.md | 11 +++++ src/background/index.js | 7 ++- src/shared/browserApi.js | 7 +-- tests/backgroundApproval.test.js | 32 ++++++++++++- tests/backgroundStateIsolation.test.js | 2 +- tests/chainSwitchGate.test.js | 2 +- tests/coldWorkerChainId.test.js | 2 +- tests/coldWorkerChainSwitch.test.js | 2 +- tests/coldWorkerSendTransaction.test.js | 2 +- tests/e2e/run.js | 60 ++++++++++++++++++++++++- tests/stateUnusableRpc.test.js | 2 +- 12 files changed, 129 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 6b1c28b..2c018e0 100644 --- a/README.md +++ b/README.md @@ -381,11 +381,13 @@ handler on a reserved-TLD origin, gets `window.ethereum` from the shipped the runner and compared against the active address, the transaction assertions run against the raw signed transaction captured at `eth_sendRawTransaction` rather than against anything the extension reported, rejecting each prompt is -required to return a rejection to the page rather than hang or resolve, and the -password is required to be absent from every message the approval window sends -to the background — with the message that would carry it required to be present, -so that check cannot pass by observing nothing. That last one is the standing -floor under [#157](https://git.eeqj.de/sneak/AutistMask/issues/157). +required to return a rejection to the page rather than hang or resolve, a prompt +raised while another approval window has focus is required to open a window of +its own, and the password is required to be absent from every message the +approval window sends to the background — with the message that would carry it +required to be present, so that check cannot pass by observing nothing. That +last one is the standing floor under +[#157](https://git.eeqj.de/sneak/AutistMask/issues/157). Two limits of that coverage, neither of them papered over. The RPC is stubbed throughout, so this is **not** a real dApp against a real network with real @@ -619,12 +621,9 @@ The jobs **report, they do not gate.** A failure is a red mark against the commit that a reviewer has to account for, not a hard block: whether a check blocks a merge is Gitea branch protection, which this repo does not configure. -That is not only a statement about configuration. A report of the Chrome suite -**failing under load** is still open: -[#290](https://git.eeqj.de/sneak/AutistMask/issues/290), runs on a busy machine -failing with `the extension opened no approval window within 30000ms`. So a red -`e2e-chrome` has to be read before it is believed, and those failures are the -blocker to ever making this a required check. Do not answer them with a retry +That is not only a statement about configuration. No report of the Chrome suite +**failing under load** is open now, but it has failed that way before, so a red +`e2e-chrome` is read before it is believed. Do not answer one with a retry wrapper: a suite that reruns until it is green stops being evidence. Nothing in either job can pass vacuously. There is no `continue-on-error` and no @@ -1940,7 +1939,8 @@ view would leave a wallet one click from deletion. `eth_sendTransaction` arriving while one is unanswered is refused with EIP-1193 code `-32002` rather than being populated at the same nonce. It opens no window and takes no nonce, and the site can send it again once the pending - one is answered. + one is answered. The window is centred on the browser window the user was last + in, never on another approval window still open. - **Elements**: - "Transaction Request" heading - Phishing warning banner (shown when the hostname is on the phishing diff --git a/TODO.md b/TODO.md index 333ef45..143b06e 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,17 @@ but the review is broader than any of them. # Completed Steps +- 2026-10-05: A prompt raised while another approval window has focus opens a + window of its own ([#290](https://git.eeqj.de/sneak/AutistMask/issues/290)). + The background centred each approval window on the last focused window, which + could be an earlier approval window still open; headless Chrome reports one as + 1280x720, so the new window came out where the browser refused to create it, + and the request failed with no window at all. It now centres on the last + focused browser window. In the Chrome end-to-end suite a test could raise its + prompt while the previous test's window was still closing, and then either hit + that refusal or take the closing window for its own; the runner now closes + every approval window between tests. + - 2026-10-05: The e2e suite waits for a save to land before it closes the popup ([#446](https://git.eeqj.de/sneak/AutistMask/issues/446)). The Settings round trip switched the theme and the network and closed the popup at once, and a diff --git a/src/background/index.js b/src/background/index.js index ff432b9..8ff943d 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -423,9 +423,14 @@ async function openApprovalWindow(id) { const popupWidth = 360; const popupHeight = 600; + // Centred on the browser window the user was last in, never on a popup. + // A popup here is another approval window still open, and centring on + // one can give a position the browser refuses ("Bounds must be at least + // 50% within visible screen space"): headless Chrome reports this 360x600 + // popup as 1280x720. The request then failed with no window at all. let currentWin = null; try { - currentWin = await windowsGetLastFocused(); + currentWin = await windowsGetLastFocused({ windowTypes: ["normal"] }); } catch { // Nothing focused to centre on. The window still opens, at whatever // position the browser picks. diff --git a/src/shared/browserApi.js b/src/shared/browserApi.js index c512286..8a5c64c 100644 --- a/src/shared/browserApi.js +++ b/src/shared/browserApi.js @@ -244,10 +244,11 @@ function windowsCreate(createData) { } /** - * @returns {Promise} the last focused window. + * @param {Object} queryOptions which windows count, e.g. `windowTypes`. + * @returns {Promise} the last focused of those windows. */ -function windowsGetLastFocused() { - return invoke(windowsApi(), "getLastFocused"); +function windowsGetLastFocused(queryOptions) { + return invoke(windowsApi(), "getLastFocused", queryOptions); } /** diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index 7cdfb02..eef0270 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -267,7 +267,14 @@ function loadBackground(options) { lastError: null, }, windows: { - getLastFocused: (cb) => cb(null), + // The last focused of `opts.windows` (listed oldest focus first) + // whose type the caller asked for, as the browser filters them. + getLastFocused: (queryOptions, cb) => { + const asked = (opts.windows || []).filter((w) => + queryOptions.windowTypes.includes(w.type), + ); + cb(asked[asked.length - 1] || null); + }, create: (options2, cb) => { created.push(options2); // A browser that answers with no window at all. The approval @@ -2716,3 +2723,26 @@ describe("removing a site in Settings disconnects it", () => { expect(await siteAccounts(bg)).toEqual({ result: [signer.address] }); }); }); + +// An approval window still open is often the last focused window, and headless +// Chrome reports one as 1280x720. Centred on that, the next approval window +// lands where the browser refuses to create it, and its request failed with no +// window at all (https://git.eeqj.de/sneak/AutistMask/issues/290). +describe("where an approval window opens", () => { + test("centred on the browser window, not on an approval window focused since", async () => { + const bg = loadBackground({ + windows: [ + { type: "normal", left: 0, top: 0, width: 1280, height: 720 }, + { type: "popup", left: 440, top: 0, width: 1280, height: 720 }, + ], + }); + + bg.requestSign(); + await settle(); + + // Centred on the browser window; on the approval window it would be at + // left 900, the position the browser refused. + expect(bg.created).toHaveLength(1); + expect(bg.created[0]).toMatchObject({ left: 460, top: 60 }); + }); +}); diff --git a/tests/backgroundStateIsolation.test.js b/tests/backgroundStateIsolation.test.js index ef34d04..21ee707 100644 --- a/tests/backgroundStateIsolation.test.js +++ b/tests/backgroundStateIsolation.test.js @@ -179,7 +179,7 @@ function loadWorker(networkId, opts) { lastError: null, }, windows: { - getLastFocused: (cb) => cb(null), + getLastFocused: (queryOptions, cb) => cb(null), create: (createOpts, cb) => { createdUrls.push(createOpts.url); cb({ id: createdUrls.length }); diff --git a/tests/chainSwitchGate.test.js b/tests/chainSwitchGate.test.js index 1c94b53..b655957 100644 --- a/tests/chainSwitchGate.test.js +++ b/tests/chainSwitchGate.test.js @@ -109,7 +109,7 @@ function loadBackground() { lastError: null, }, windows: { - getLastFocused: (cb) => cb(null), + getLastFocused: (queryOptions, cb) => cb(null), create: (options, cb) => cb({ id: 1 }), remove: (id, cb) => { if (cb) cb(); diff --git a/tests/coldWorkerChainId.test.js b/tests/coldWorkerChainId.test.js index 77331fb..812e3e6 100644 --- a/tests/coldWorkerChainId.test.js +++ b/tests/coldWorkerChainId.test.js @@ -113,7 +113,7 @@ function loadColdWorker(networkId, opts) { lastError: null, }, windows: { - getLastFocused: (cb) => cb(null), + getLastFocused: (queryOptions, cb) => cb(null), create: (options, cb) => cb({ id: 1 }), remove: (id, cb) => { if (cb) cb(); diff --git a/tests/coldWorkerChainSwitch.test.js b/tests/coldWorkerChainSwitch.test.js index 56b5481..2da4a7e 100644 --- a/tests/coldWorkerChainSwitch.test.js +++ b/tests/coldWorkerChainSwitch.test.js @@ -105,7 +105,7 @@ function loadColdWorker(networkId) { lastError: null, }, windows: { - getLastFocused: (cb) => cb(null), + getLastFocused: (queryOptions, cb) => cb(null), create: (options, cb) => cb({ id: 1 }), remove: (id, cb) => { if (cb) cb(); diff --git a/tests/coldWorkerSendTransaction.test.js b/tests/coldWorkerSendTransaction.test.js index 1344807..ecee656 100644 --- a/tests/coldWorkerSendTransaction.test.js +++ b/tests/coldWorkerSendTransaction.test.js @@ -148,7 +148,7 @@ function loadColdWorker(networkId) { lastError: null, }, windows: { - getLastFocused: (cb) => cb(null), + getLastFocused: (queryOptions, cb) => cb(null), create: (opts, cb) => { createdUrls.push(opts.url); cb({ id: createdUrls.length }); diff --git a/tests/e2e/run.js b/tests/e2e/run.js index 7f022c8..ff20a49 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -2676,7 +2676,9 @@ function dappMessages(page, type) { // The approval window the background opened. Approvals are raised from an // RPC call rather than from a user gesture, so the extension opens a real -// popup window for them; it is an ordinary page in this context. +// popup window for them; it is an ordinary page in this context. It is the +// first one found: the runner closes every approval window between tests, so +// within a test it can only be this test's. async function waitForApprovalWindow(ctx, timeout = 30000) { const deadline = Date.now() + timeout; for (;;) { @@ -3633,6 +3635,55 @@ test("eth_sendTransaction rejected broadcasts nothing (#183)", async (env) => { ); }); +// The extension used to centre every approval window on the last focused +// window. Centred on another approval window, the new one landed where the +// browser refused to create it, and the request failed with no window at all +// (https://git.eeqj.de/sneak/AutistMask/issues/290). +test("a prompt raised while another approval window has focus opens its own (#290)", async (env) => { + await startRequest(env.dapp, "focus-sign", "personal_sign", [ + SIGN_HEX, + env.expectedAddress, + ]); + const signWindow = await waitForApprovalWindow(env.ctx); + await visible(signWindow, "#view-approve-sign"); + await signWindow.bringToFront(); + const focused = await env.page.evaluate( + () => + new Promise((resolve) => { + chrome.windows.getLastFocused((w) => resolve(w.type)); + }), + ); + assert( + focused === "popup", + "the sign window does not have focus, so this proves nothing: the " + + "last focused window is a " + + focused, + ); + + const opened = env.ctx.waitForEvent("page"); + await startRequest(env.dapp, "focus-tx", "eth_sendTransaction", [ + { + from: env.expectedAddress, + to: STUB_COUNTERPARTY, + value: toQuantity(TX_VALUE_WEI), + data: TX_DATA, + }, + ]); + const txWindow = await opened.catch(async () => { + const outcome = await settleRequest(env.dapp, "focus-tx", 1000); + throw new Error( + "the extension opened no window for the transaction: " + + JSON.stringify(outcome), + ); + }); + await visible(txWindow, "#view-approve-tx"); + + // Closing a window is refusing its prompt, so both answer 4001. + await closeApprovalPages(env.ctx); + await assertUserRejection(env.dapp, "focus-tx", "the transaction prompt"); + await assertUserRejection(env.dapp, "focus-sign", "the sign prompt"); +}); + // The closing pass over both boundaries at once. Every message the section // put on either channel is re-read here and required to be free of the // password — and required to be there at all, method by method, so the @@ -4002,6 +4053,13 @@ async function main() { failure = e.message; } + // No test starts with an approval window open. One that closes itself, + // after a signature or on Reject, can still be closing when the next + // test raises its prompt, and waitForApprovalWindow() would take it + // for the new one; one a failed test left unanswered would refuse the + // next prompt from its site. + await closeApprovalPages(env.ctx); + // Any uncaught page error, console.error or unstubbed request // fails the test that provoked it, whether or not its assertions // passed. This is the mechanism that caught #150. diff --git a/tests/stateUnusableRpc.test.js b/tests/stateUnusableRpc.test.js index 66fd70a..6a9c6de 100644 --- a/tests/stateUnusableRpc.test.js +++ b/tests/stateUnusableRpc.test.js @@ -109,7 +109,7 @@ function loadColdWorker(stored) { lastError: null, }, windows: { - getLastFocused: (cb) => cb(null), + getLastFocused: (queryOptions, cb) => cb(null), create: (options, cb) => cb({ id: 1 }), remove: (id, cb) => { if (cb) cb();