From 6613a1f6bc983492306cacf8f6796cc7353393a4 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 5 Oct 2026 08:26:06 +0200 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, the browser refused the resulting position, and the request failed with no window. It now centres only on a browser window, and when the browser refuses a position it asks again without one. In the Chrome suite a test could raise its prompt while the previous test's window was still closing. After a passed test the runner now gives approval windows five seconds to close and fails the test if one is still open; after a failed test it closes them. Model: opus-5-5 --- README.md | 25 ++++----- TODO.md | 14 +++++ src/background/index.js | 25 +++++++-- tests/backgroundApproval.test.js | 91 +++++++++++++++++++++++++++++++- tests/e2e/run.js | 83 ++++++++++++++++++++++++++++- 5 files changed, 220 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 1d19c85..034d06b 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 @@ -1958,7 +1957,9 @@ 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, or the browser refuses the centred + position, 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 e354a4a..45a39d5 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,20 @@ 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 when the browser refuses the position it asks again without one + and lets the browser place the 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. After a + test that passed, the runner now waits a few seconds for approval windows to + close and fails the test if one is still open; after a test that failed, it + closes them. + - 2026-10-05: The Send screen has a "Max" button ([#198](https://git.eeqj.de/sneak/AutistMask/issues/198)). Emptying an ETH address took guessing an amount and being refused by the confirmation screen's diff --git a/src/background/index.js b/src/background/index.js index ff432b9..76c902d 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, ); @@ -455,10 +461,23 @@ async function openApprovalWindow(id) { win = await windowsCreate(opts); } catch (e) { // The promise namespace reports the failure by rejecting where the - // callback namespace reported it by handing back no window; both land - // on the !win branch below, which settles the approval. + // callback namespace reported it by handing back no window; both + // leave win null. log.errorf("could not open the approval window:", e); } + // The browser also refuses a centred position that is too far off screen, + // as it is over a browser window near the screen edge. Asked again + // without a position, it places the window itself. If that fails too, + // the !win branch below settles the approval. + if (!win && opts.left !== undefined) { + delete opts.left; + delete opts.top; + try { + win = await windowsCreate(opts); + } catch (e) { + log.errorf("could not open the approval window:", e); + } + } const approval = pendingApprovals[id]; if (!approval) { diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index 7cdfb02..40d3e6c 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -267,9 +267,22 @@ function loadBackground(options) { lastError: null, }, windows: { - getLastFocused: (cb) => cb(null), + getLastFocused: (cb) => cb(opts.lastFocused || null), create: (options2, cb) => { - created.push(options2); + // A copy, as the browser takes it at the call: the background + // reuses the object when it asks a second time. + created.push({ ...options2 }); + // A browser that refuses any position it is given, as Chrome + // does for one it judges too far off screen. + if (opts.refusePosition && options2.left !== undefined) { + global.chrome.runtime.lastError = { + message: + "Invalid value for bounds. Bounds must be at least 50% within visible screen space.", + }; + cb(undefined); + global.chrome.runtime.lastError = null; + return; + } // A browser that answers with no window at all. The approval // then has no window it can ever be answered in. cb(opts.noWindow ? undefined : { id: created.length }); @@ -2716,3 +2729,77 @@ 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(); + }); + + test("placed by the browser when it refuses the centred position", async () => { + const bg = loadBackground({ + refusePosition: true, + lastFocused: { + type: "normal", + left: 1500, + top: 900, + width: 400, + height: 300, + }, + }); + + const sign = bg.requestSign(); + await settle(); + + expect(bg.created).toHaveLength(2); + expect(bg.created[0]).toMatchObject({ left: 1520, top: 750 }); + expect(bg.created[1].left).toBeUndefined(); + expect(bg.created[1].top).toBeUndefined(); + + // The request waits on the second window rather than failing: + // closing that window is refusing the prompt. + expect(sign.result()).toBeNull(); + bg.closeWindow(2); + await settle(); + expect(sign.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + }); +}); diff --git a/tests/e2e/run.js b/tests/e2e/run.js index 7f022c8..191c405 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: no test starts with an approval window open (see main()), +// 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,36 @@ 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. After a test that passed, its windows get five + // seconds to close, and one still open then fails the test: it should + // have closed itself, and closing it here unreported would hide that. + // After a test that already failed, they are closed unreported, so + // that a prompt it left unanswered does not refuse the next one from + // its site. + if (!failure) { + const deadline = Date.now() + 5000; + let open; + for (;;) { + open = env.ctx + .pages() + .filter( + (p) => !p.isClosed() && p.url().includes("?approval="), + ); + if (open.length === 0 || Date.now() > deadline) break; + await sleep(50); + } + if (open.length > 0) { + failure = + "an approval window was still open 5000ms after the " + + "test: " + + open.map((p) => p.url()).join(", "); + } + } + 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.