diff --git a/README.md b/README.md index 2515733..0ca5b1a 100644 --- a/README.md +++ b/README.md @@ -619,13 +619,14 @@ 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. The Chrome suite is -**measurably flaky under load** — two of six runs of unmutated code on a busy -machine lost the approval popup out from under the dApp signing wait, always in -the `#183` section, tracked as -[#287](https://git.eeqj.de/sneak/AutistMask/issues/287). So a red `e2e-chrome` -has to be read before it is believed, and that flake is the blocker to ever -making this a required check. Do not answer it with a retry wrapper: a suite +That is not only a statement about configuration. Reports of the Chrome suite +**failing under load** are still open, among them +[#290](https://git.eeqj.de/sneak/AutistMask/issues/290), runs on a busy machine +failing with `the extension opened no approval window within 30000ms`, and +[#446](https://git.eeqj.de/sneak/AutistMask/issues/446), the Settings round trip +closing the popup before its network switch is saved. 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 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 diff --git a/TODO.md b/TODO.md index fa4acd0..b4233f8 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: The extension no longer opens a window for a site-connection + prompt already answered + ([#287](https://git.eeqj.de/sneak/AutistMask/issues/287)). When the prompt was + decided before the toolbar popup raised for it had loaded, that popup was torn + down, `chrome.action.openPopup()` rejected, and the background opened its + fallback window for the answered approval and then removed it. In the Chrome + end-to-end suite the next test could take that window for its own prompt and + lose it under its wait. `openApprovalWindow()` now opens nothing for an + approval that is no longer pending. The blocklist test's Reject, whose window + closes itself, is clicked as the other site Reject is, with the click + witnessed. - 2026-10-05: A `holders_count` that is not a whole number in plain digits is unknown, not read in part ([#251](https://git.eeqj.de/sneak/AutistMask/issues/251)). `parseInt` read diff --git a/src/background/index.js b/src/background/index.js index 4c66c85..ff432b9 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -413,7 +413,7 @@ function releaseApproval(approval) { } } -// Open approval in a separate popup window. +// Open approval in a separate popup window, unless it is no longer pending. // This is the primary mechanism for tx/sign approvals (triggered programmatically, // not from a user gesture) and the fallback for site-connection approvals. // Never rejects. Its callers raise it from inside a Promise executor and drop @@ -446,6 +446,10 @@ async function openApprovalWindow(id) { ); } + // Already answered: a site-connection prompt decided before the toolbar + // popup raised for it had loaded, whose openPopup() rejects only now. + if (!pendingApprovals[id]) return; + let win = null; try { win = await windowsCreate(opts); diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index 3997c10..7cdfb02 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -2235,6 +2235,27 @@ describe("a site connection decided as the popup closes", () => { }); }); + // The prompt is decided before the toolbar popup raised for it has + // loaded; that popup is torn down and openPopup() rejects only after. + test("a toolbar prompt already decided opens no window when openPopup() rejects", async () => { + const bg = loadBackground({ actionPopup: true }); + const opening = deferred(); + bg.openPopup.mockImplementation(() => opening.promise); + const pending = bg.requestSite(); + await settle(); + + const port = bg.connectApproval(pending.id()); + port.decide(true, false); + port.disconnect(); + await settle(); + expect(pending.result()).toEqual({ result: [signer.address] }); + + opening.reject(new Error("the toolbar popup closed before it loaded")); + await settle(); + + expect(bg.created).toHaveLength(0); + }); + // The port carries a decision now, so it carries the sender check the // one-off message used to carry. A content script that guessed an // approval id must not be able to connect the site it is running on. diff --git a/tests/e2e/run.js b/tests/e2e/run.js index 4990bc6..fa49791 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -2738,7 +2738,7 @@ async function closeApprovalPages(ctx) { // #btn-reject on the site prompt — NOT self-proving. A page that went away // without the click landing disconnects the approval port, the background // settles that as 4001, and 4001 is exactly what assertUserRejection -// accepts. That call site arms the click trace below and asserts it. +// accepts. Both call sites arm the click trace below and assert it. // // A button that is missing or unclickable raises a different error, which is // rethrown. @@ -3124,7 +3124,9 @@ test("a connect request from a blocklisted site is flagged (#219)", async (env) // Not remembered: a remembered decision for this origin would // outlive the test. await popup.uncheck("#approve-remember"); - await popup.click("#btn-reject"); + await armClickTrace(env, popup, "#btn-reject"); + await clickAndClose(popup, "#btn-reject"); + await assertClickLanded(env, "#btn-reject"); await assertUserRejection( phishingDapp,