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();