diff --git a/README.md b/README.md index 6b1c28b..1e7d917 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; if that was another approval window, the browser picks the position. - **Elements**: - "Transaction Request" heading - Phishing warning banner (shown when the hostname is on the phishing diff --git a/TODO.md b/TODO.md index e34755e..87f9a1f 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 only on a browser + window and otherwise lets the browser place it. 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: A token scale of zero decimals is tested ([#325](https://git.eeqj.de/sneak/AutistMask/issues/325)). `resolveTokenDecimals()` already used a scale of 0 from the bundled list or diff --git a/src/background/index.js b/src/background/index.js index ff432b9..202bbd9 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -437,7 +437,13 @@ async function openApprovalWindow(id) { width: popupWidth, height: popupHeight, }; - if (currentWin) { + // Centred on a browser window only. The last focused window can be + // 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. Over a popup, + // the browser picks the position. + if (currentWin && currentWin.type === "normal") { opts.left = Math.round( currentWin.left + (currentWin.width - popupWidth) / 2, ); diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index 7cdfb02..af88c59 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -267,7 +267,7 @@ function loadBackground(options) { lastError: null, }, windows: { - getLastFocused: (cb) => cb(null), + getLastFocused: (cb) => cb(opts.lastFocused || null), create: (options2, cb) => { created.push(options2); // A browser that answers with no window at all. The approval @@ -2716,3 +2716,47 @@ 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 the user was last in", async () => { + const bg = loadBackground({ + lastFocused: { + type: "normal", + left: 0, + top: 0, + width: 1280, + height: 720, + }, + }); + + bg.requestSign(); + await settle(); + + expect(bg.created).toHaveLength(1); + expect(bg.created[0]).toMatchObject({ left: 460, top: 60 }); + }); + + test("not centred on an approval window the user was last in", async () => { + const bg = loadBackground({ + lastFocused: { + type: "popup", + left: 440, + top: 0, + width: 1280, + height: 720, + }, + }); + + bg.requestSign(); + await settle(); + + // Centred, it would be at left 900, the position the browser refused. + expect(bg.created).toHaveLength(1); + expect(bg.created[0].left).toBeUndefined(); + expect(bg.created[0].top).toBeUndefined(); + }); +}); 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.