sneak/AutistMask
1
0
Fork 0
Code Issues 61 Pull Requests Actions Packages Projects Releases Wiki Activity

fix: settle a site approval on the port that carries its teardown (closes #275) #289

Merged
clawbot merged 1 commits from issue-275-site-approval-race into next 2026-08-17 09:34:08 +02:00
Conversation 7 Commits 1 Files Changed 5 +509 -71
clawbot commented 2026-08-14 06:24:19 +02:00
Collaborator
Copy Link

Closes #275.

The defect

btn-approve sent the decision with runtime.sendMessage() and closed the
window on the next line. The close disconnects the approval port, and the
disconnect handler settled a pending SITE approval as a rejection. Those two
events travelled independent channels with nothing ordering them, so whichever
landed first decided the outcome — and driven in a tab the teardown won every
time: the user allowed the connection and the dApp was told they had refused.

The fix

The decision now goes out on the approval port the popup already opens in
show(), which is the same port the close disconnects. One channel is ordered:
a message posted on a port is delivered before that port's own disconnect. So
the approval is settled by the decision and the disconnect then finds nothing
pending to reject — whatever the teardown timing is. Nothing is awaited,
nothing is timed, and the popup closes exactly as immediately as before. The
post is wrapped in a try: a port whose worker is gone throws, and that must
not cost the popup its close.

windows.onRemoved no longer decides a site approval whose port is connected
either. In the fallback-window shape that event races the decision on a channel
of its own, which is the same defect one level over; the port disconnect says
the same thing in a defined order, so it is left to say it. A window that
closes before its popup ever connected has nothing else to speak for it and is
still rejected there, so no dApp is left waiting on a window that is gone.
"Connected" means the extension's own popup connected: the flag takes the same
isExtensionSender() check the decision takes, so a port from a page sender
cannot switch that settlement path off and strand the dApp.

Each approval type now has exactly one close authority: site by port
disconnect, tx/sign by windows.onRemoved (unchanged — their disconnect
handler still leaves them pending so the user can reopen the toolbar popup).

AUTISTMASK_APPROVAL_RESPONSE is gone. The port name carries the approval id,
so the popup no longer names one at all, and the Unauthorized sender check
that message carried moved onto the port (factored into isExtensionSender(),
which the remaining popup-only messages now share). The port disconnect itself
is unchecked exactly as before — a content script that guessed a UUID could
always disconnect one; that is pre-existing and not touched here.

What I established about chrome.action.openPopup()

  • The toolbar popup is not a window: windows.onRemoved can never fire for a
    prompt opened that way, so the port disconnect is the ONLY close signal that
    exists on the production path. That is why "leave a site approval pending on
    disconnect", the shape tx/sign use, is not available here — it would make
    close-without-deciding hang the dApp forever.
  • The two-channel race is present on that path for the same reason it is
    present in a tab: dismissing the popup destroys the document exactly as
    window.close() does, and the decision and the teardown are emitted back to
    back either way. Which one arrives first there is NOT measured and is not
    measurable from this harness — headless Chromium opens the browser-action
    popup but Playwright never exposes it as a page.
  • The fix does not depend on that ordering, which is the point. It rests on a
    property of the port, not on how the document was opened: the decision and
    the disconnect are the same channel in both shapes, and the unit tests drive
    the action.openPopup() shape (no window created, port disconnect the only
    signal) as well as the fallback-window shape.

So the "every time" in the issue remains a tab result. Production is not
asserted to have behaved the same way; it is asserted to be unable to behave
differently now.

The harness accommodation is removed

tests/e2e/run.js no longer patches window.close on the reserved approval
tab. The two site-prompt tests now drive the shipped decide-then-close in a
real Chromium.

Removing it exposed a harness flake unrelated to this bug: page.click()
resolves only after the renderer acknowledges the action, and a page torn down
by the handler never gets to. It hit #btn-reject-sign and #btn-reject-tx
too — windows that have always closed themselves and were never accommodated —
so it is pre-existing and was simply exposed more often once four more clicks
started self-closing. clickAndClose() tolerates that one error for the five
buttons that close their own window and rethrows anything else.

What proves the click landed is not the same for each of those five, and the
helper now says so per button. #btn-reject-sign and #btn-reject-tx leave
the approval pending, so an unclicked prompt leaves the dApp promise unsettled;
#btn-approve cannot reach settled === "resolved" without a decision. The
site #btn-reject proves nothing on its own — a page that went away unclicked
disconnects the port, the background settles that as 4001, and 4001 is what
assertUserRejection accepts. That one call site arms a capture-phase click
trace (a synchronous localStorage write on the extension origin, read back
from another page of that origin) and asserts the click reached the button.
The trace only observes: nothing about the shipped decide-then-close is
deferred, patched or reordered.

Demonstrated failing first

Real browser, accommodation removed, unfixed code (make test-e2e):

ok 29 - eth_requestAccounts rejected at the prompt returns a rejection (#183)
not ok 30 - eth_requestAccounts approved returns the selected address (#183)
  eth_requestAccounts did not resolve: {"settled":"rejected","message":"User rejected the request.","name":"ProviderRpcError","hasCode":true,"code":4001}
not ok 31 - personal_sign signs, and the signature recovers to the address (#183)
  the extension opened no approval window within 30000ms
...
# 29/37 tests passed
# FAILED

The user clicked Allow and the page got 4001. Everything after it fails as a
consequence — the origin never became authorized, so no later prompt was raised
at all.

Unit, unfixed code (make test): the two approve-then-close cases fail and the
four rejection cases pass, which is the defect exactly.

● a site connection decided as the popup closes › approving in the toolbar popup connects the site

  expect(received).toEqual(expected) // deep equality

  Object {
  -   "result": Array [
  -     "0x70997970C51812dc3A010C7d01b50e0d17dc79C8",
  -   ],
  +   "error": Object {
  +     "code": 4001,
  +     "message": "User rejected the request.",
  +   },
  }

● a site connection decided as the popup closes › approving in the fallback window survives the window event too
  (same diff)

Tests: 2 failed, 686 passed, 688 total

Tests added

tests/backgroundApproval.test.js, the approve cases all emitting the close
IMMEDIATELY after the decision with nothing awaited in between:

  • approving in the toolbar popup (action.openPopup() shape, no window)
    connects the site
  • closing the toolbar popup without deciding is a rejection
  • rejecting is a rejection, and the close that follows adds nothing
  • a decision from a page sender is ignored, and the close then rejects
  • approving in the fallback window survives a windows.onRemoved that lands
    FIRST, which is the interleaving the guard in that listener exists for — the
    approval is still pending when the event arrives, so the listener really
    reaches it and really has to decline it
  • approving in the fallback window survives a windows.onRemoved that follows
  • a port from a page sender does not silence windows.onRemoved
  • closing the fallback window without deciding is a rejection
  • a window that closes before its popup ever connected still rejects
  • a tx window closed with its port connected still rejects — the isSite &&
    half of the skip. The popup connects its port in show() before it knows the
    type and the background sets portConnected type-agnostically, so a tx
    approval carries the flag too; without the conjunct the window event would
    skip it, windowClosed would never be set, and the page would hang

chrome.runtime.onConnect is captured rather than swallowed in the stub, and
the port stub never reorders a decision against the disconnect that follows it.

How this coexists with #271

windows.onRemoved is shared ground with
#271, which added
abandonedResult() and records approval.windowClosed when the settle is
refused, so a tx approval whose window closed under an in-flight attempt does
not hold the page open. This PR keeps that shape verbatim and adds only its own
skip ahead of it:

if (approval.windowId !== windowId) continue;
const isSite = approval.type !== "tx" && approval.type !== "sign";
if (isSite && approval.portConnected) continue;
const rejection = abandonedResult(
    approval,
    APPROVAL_REJECTED_CODE,
    APPROVAL_REJECTED_MESSAGE,
);
if (!settleApproval(id, rejection)) approval.windowClosed = true;

An earlier revision of this PR carried its own inline isSite ternary there.
That is redundant against abandonedResult(), which already returns
{approved: false, remember: false} for a site approval and
{error: {code, message}} for tx/sign with the same 4001 and the same wording,
so the ternary is gone and the settle values are unchanged either way.

Verification

Ran, on this branch rebased onto next at 7690fe6:

  • make fmt — clean, no changes.
  • make check — green, exit 0: 761 tests, 31 suites; eslint and prettier clean
    in the pinned container, which is where
    #152 now runs lint.
  • make test-e2e — green, 51/51, in the pinned Playwright container, including
    ok 43 - eth_requestAccounts rejected at the prompt and
    ok 44 - eth_requestAccounts approved.

Each guard this PR relies on was neutered in turn and put the suite red, then
restored; the captured output is in the comments below, along with the re-run
of the portConnected probe against the merged
#271 code.

make test-e2e-firefox was NOT run.

Closes [#275](https://git.eeqj.de/sneak/AutistMask/issues/275). ## The defect `btn-approve` sent the decision with `runtime.sendMessage()` and closed the window on the next line. The close disconnects the approval port, and the disconnect handler settled a pending SITE approval as a rejection. Those two events travelled independent channels with nothing ordering them, so whichever landed first decided the outcome — and driven in a tab the teardown won every time: the user allowed the connection and the dApp was told they had refused. ## The fix The decision now goes out on the approval port the popup already opens in `show()`, which is the same port the close disconnects. One channel is ordered: a message posted on a port is delivered before that port's own disconnect. So the approval is settled by the decision and the disconnect then finds nothing pending to reject — whatever the teardown timing is. Nothing is awaited, nothing is timed, and the popup closes exactly as immediately as before. The post is wrapped in a `try`: a port whose worker is gone throws, and that must not cost the popup its close. `windows.onRemoved` no longer decides a site approval whose port is connected either. In the fallback-window shape that event races the decision on a channel of its own, which is the same defect one level over; the port disconnect says the same thing in a defined order, so it is left to say it. A window that closes before its popup ever connected has nothing else to speak for it and is still rejected there, so no dApp is left waiting on a window that is gone. "Connected" means the extension's own popup connected: the flag takes the same `isExtensionSender()` check the decision takes, so a port from a page sender cannot switch that settlement path off and strand the dApp. Each approval type now has exactly one close authority: site by port disconnect, tx/sign by `windows.onRemoved` (unchanged — their disconnect handler still leaves them pending so the user can reopen the toolbar popup). `AUTISTMASK_APPROVAL_RESPONSE` is gone. The port name carries the approval id, so the popup no longer names one at all, and the `Unauthorized sender` check that message carried moved onto the port (factored into `isExtensionSender()`, which the remaining popup-only messages now share). The port disconnect itself is unchecked exactly as before — a content script that guessed a UUID could always disconnect one; that is pre-existing and not touched here. ## What I established about `chrome.action.openPopup()` - The toolbar popup is not a window: `windows.onRemoved` can never fire for a prompt opened that way, so the port disconnect is the ONLY close signal that exists on the production path. That is why "leave a site approval pending on disconnect", the shape tx/sign use, is not available here — it would make close-without-deciding hang the dApp forever. - The two-channel race is present on that path for the same reason it is present in a tab: dismissing the popup destroys the document exactly as `window.close()` does, and the decision and the teardown are emitted back to back either way. Which one arrives first there is NOT measured and is not measurable from this harness — headless Chromium opens the browser-action popup but Playwright never exposes it as a page. - The fix does not depend on that ordering, which is the point. It rests on a property of the port, not on how the document was opened: the decision and the disconnect are the same channel in both shapes, and the unit tests drive the `action.openPopup()` shape (no window created, port disconnect the only signal) as well as the fallback-window shape. So the "every time" in the issue remains a tab result. Production is not asserted to have behaved the same way; it is asserted to be unable to behave differently now. ## The harness accommodation is removed `tests/e2e/run.js` no longer patches `window.close` on the reserved approval tab. The two site-prompt tests now drive the shipped decide-then-close in a real Chromium. Removing it exposed a harness flake unrelated to this bug: `page.click()` resolves only after the renderer acknowledges the action, and a page torn down by the handler never gets to. It hit `#btn-reject-sign` and `#btn-reject-tx` too — windows that have always closed themselves and were never accommodated — so it is pre-existing and was simply exposed more often once four more clicks started self-closing. `clickAndClose()` tolerates that one error for the five buttons that close their own window and rethrows anything else. What proves the click landed is not the same for each of those five, and the helper now says so per button. `#btn-reject-sign` and `#btn-reject-tx` leave the approval pending, so an unclicked prompt leaves the dApp promise unsettled; `#btn-approve` cannot reach `settled === "resolved"` without a decision. The site `#btn-reject` proves nothing on its own — a page that went away unclicked disconnects the port, the background settles that as 4001, and 4001 is what `assertUserRejection` accepts. That one call site arms a capture-phase click trace (a synchronous `localStorage` write on the extension origin, read back from another page of that origin) and asserts the click reached the button. The trace only observes: nothing about the shipped decide-then-close is deferred, patched or reordered. ## Demonstrated failing first Real browser, accommodation removed, unfixed code (`make test-e2e`): ``` ok 29 - eth_requestAccounts rejected at the prompt returns a rejection (#183) not ok 30 - eth_requestAccounts approved returns the selected address (#183) eth_requestAccounts did not resolve: {"settled":"rejected","message":"User rejected the request.","name":"ProviderRpcError","hasCode":true,"code":4001} not ok 31 - personal_sign signs, and the signature recovers to the address (#183) the extension opened no approval window within 30000ms ... # 29/37 tests passed # FAILED ``` The user clicked Allow and the page got 4001. Everything after it fails as a consequence — the origin never became authorized, so no later prompt was raised at all. Unit, unfixed code (`make test`): the two approve-then-close cases fail and the four rejection cases pass, which is the defect exactly. ``` ● a site connection decided as the popup closes › approving in the toolbar popup connects the site expect(received).toEqual(expected) // deep equality Object { - "result": Array [ - "0x70997970C51812dc3A010C7d01b50e0d17dc79C8", - ], + "error": Object { + "code": 4001, + "message": "User rejected the request.", + }, } ● a site connection decided as the popup closes › approving in the fallback window survives the window event too (same diff) Tests: 2 failed, 686 passed, 688 total ``` ## Tests added `tests/backgroundApproval.test.js`, the approve cases all emitting the close IMMEDIATELY after the decision with nothing awaited in between: - approving in the toolbar popup (`action.openPopup()` shape, no window) connects the site - closing the toolbar popup without deciding is a rejection - rejecting is a rejection, and the close that follows adds nothing - a decision from a page sender is ignored, and the close then rejects - approving in the fallback window survives a `windows.onRemoved` that lands FIRST, which is the interleaving the guard in that listener exists for — the approval is still pending when the event arrives, so the listener really reaches it and really has to decline it - approving in the fallback window survives a `windows.onRemoved` that follows - a port from a page sender does not silence `windows.onRemoved` - closing the fallback window without deciding is a rejection - a window that closes before its popup ever connected still rejects - a tx window closed with its port connected still rejects — the `isSite &&` half of the skip. The popup connects its port in `show()` before it knows the type and the background sets `portConnected` type-agnostically, so a tx approval carries the flag too; without the conjunct the window event would skip it, `windowClosed` would never be set, and the page would hang `chrome.runtime.onConnect` is captured rather than swallowed in the stub, and the port stub never reorders a decision against the disconnect that follows it. ## How this coexists with [#271](https://git.eeqj.de/sneak/AutistMask/issues/271) `windows.onRemoved` is shared ground with [#271](https://git.eeqj.de/sneak/AutistMask/issues/271), which added `abandonedResult()` and records `approval.windowClosed` when the settle is refused, so a tx approval whose window closed under an in-flight attempt does not hold the page open. This PR keeps that shape verbatim and adds only its own skip ahead of it: ``` if (approval.windowId !== windowId) continue; const isSite = approval.type !== "tx" && approval.type !== "sign"; if (isSite && approval.portConnected) continue; const rejection = abandonedResult( approval, APPROVAL_REJECTED_CODE, APPROVAL_REJECTED_MESSAGE, ); if (!settleApproval(id, rejection)) approval.windowClosed = true; ``` An earlier revision of this PR carried its own inline `isSite` ternary there. That is redundant against `abandonedResult()`, which already returns `{approved: false, remember: false}` for a site approval and `{error: {code, message}}` for tx/sign with the same 4001 and the same wording, so the ternary is gone and the settle values are unchanged either way. ## Verification Ran, on this branch rebased onto `next` at `7690fe6`: - `make fmt` — clean, no changes. - `make check` — green, exit 0: 761 tests, 31 suites; eslint and prettier clean in the pinned container, which is where [#152](https://git.eeqj.de/sneak/AutistMask/issues/152) now runs lint. - `make test-e2e` — green, 51/51, in the pinned Playwright container, including `ok 43 - eth_requestAccounts rejected at the prompt` and `ok 44 - eth_requestAccounts approved`. Each guard this PR relies on was neutered in turn and put the suite red, then restored; the captured output is in the comments below, along with the re-run of the `portConnected` probe against the merged [#271](https://git.eeqj.de/sneak/AutistMask/issues/271) code. `make test-e2e-firefox` was NOT run.
clawbot added 1 commit 2026-08-14 06:24:20 +02:00
fix: settle a site approval on the port that carries its teardown (closes #275)
All checks were successful
check / check (push) Successful in 30s
Details
d32ffe7c3a 
Approve and window.close() left the popup on the next line, and the decision
and the disconnect the close caused travelled independent channels with nothing
ordering them. The disconnect handler settled a pending site approval as a
rejection, so whichever landed first decided the outcome. Driven in a tab the
teardown won every time: the user allowed the connection and the dApp was told
they had refused.

The decision now goes out on the approval port the popup already opens, which
is the same port the close disconnects. One channel is ordered -- a message
posted on a port is delivered before that port's own disconnect -- so the
approval is settled before the teardown is even seen, and the disconnect then
finds nothing pending to reject. Nothing waits, nothing is timed, and the popup
closes exactly as immediately as before.

windows.onRemoved no longer decides a site approval whose port is connected
either. In the fallback-window shape that event races the decision on a channel
of its own, which is the same defect one level over; the port disconnect says
the same thing in a defined order, so it is left to say it. A window that
closes before its popup ever connected has nothing else to speak for it and is
still rejected there, so no dApp is left waiting on a window that is gone.

Rejecting reports a rejection, and so does closing without deciding, in both
shapes. AUTISTMASK_APPROVAL_RESPONSE is gone; the port name carries the
approval id, so the popup no longer names one, and the sender check the message
carried moved to the port.

tests/backgroundApproval.test.js drives decide-then-disconnect with nothing
awaited in between, in the toolbar-popup shape that production uses and in the
fallback-window shape, and asserts every close-without-deciding path still
rejects. tests/e2e/run.js drops the deferred-window.close() accommodation it
carried for this bug, so the two site-prompt tests now drive the shipped
decide-then-close in a real Chromium.
clawbot added the needs-review label 2026-08-14 06:24:23 +02:00
clawbot self-assigned this 2026-08-14 06:24:24 +02:00
clawbot commented 2026-08-17 08:03:25 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework.

1. tests/backgroundApproval.test.js:528-541 — "approving in the fallback window survives the window event too" does not exercise the window event.

The test emits port.decide(true, false) and only then bg.closeWindow(1). The decision has already deleted the entry from pendingApprovals, so the windows.onRemoved listener iterates and finds nothing — the guard the test is named for never executes. Proven: replacing src/background/index.js:886 (if (isSite && approval.portConnected) continue;) with a no-op leaves all 31 tests in that file green.

The guard is load-bearing. A probe emitting the events in the order the guard exists for — bg.closeWindow(1) BEFORE port.decide(true, false), which is exactly the "ordered against nothing" interleaving the PR body describes for the fallback shape — passes with the guard ({result: [address]}) and fails without it ({error: {code: 4001}}). So the one defence against windows.onRemoved deciding an approved site connection has no test, and deleting it regresses silently.

Acceptable: a test that fires the window event before the decision, and asserts the approval still resolves.

2. src/background/index.js:309-313 — portConnected is set with no sender check, and it disables the only backstop.

runtime.onConnect sets pendingApprovals[id].portConnected = true for any connector. The decision path got the check (isExtensionSender(port.sender), line 316); the flag that switches off windows.onRemoved did not. A content script that named approval:<id> and holds the port open therefore suppresses the onRemoved rejection, and if the real popup never connected its own port nothing else ever settles the approval — the dApp promise hangs forever. That is a new liveness hole in the same direction as #262, introduced by this PR; the unchecked connect was pre-existing, but before this change it could not disable a settlement path. Guessing a v4 UUID is required, so severity is low, but the asymmetry is one line.

Acceptable: if (pendingApprovals[id] && isExtensionSender(port.sender)) pendingApprovals[id].portConnected = true;

3. tests/e2e/run.js:1639-1645 — the clickAndClose justification is false for #btn-reject.

The comment claims "a click that did not land leaves the dApp promise unsettled and the assertion after the call still fails". That holds for #btn-reject-sign and #btn-reject-tx, whose disconnect handler leaves the approval pending. It does not hold for the site prompt: a page that went away without the click landing produces a port disconnect, which settles 4001, which is exactly what assertUserRejection accepts. So eth_requestAccounts rejected at the prompt returns a rejection (#183) (run.js:1878) can now pass without the reject button ever having been clicked. The approve test at run.js:1907 is unaffected — its assertion is settled === "resolved", which a swallowed click cannot produce.

Acceptable: narrow the swallow to the approve button, or say plainly in the comment that the site-reject case is not discriminated by its own assertion.

4. Nit — src/background/index.js:324: resetPopupUrl() after settleApproval() is dead. settleApproval already calls it (line 152). Carried over from the AUTISTMASK_APPROVAL_RESPONSE handler it replaces; same at line 335.

Verified green and not at issue: the primary fix is correct. make check green on d32ffe7 (710 tests, 29 suites, prettier clean); make test-e2e green 40/40 in the pinned container with the harness accommodation removed, including test 33 (approve-then-immediate-close in real Chromium); the two port-channel unit tests go red against the next source and green with it; the sender check on the decision is load-bearing; no double settle and no approve-after-close in any interleaving I drove; base next; CI green; mergeable; title carries (closes #275); no attribution trailers.

Disclosure: the FIFO claim on the production chrome.action.openPopup() path is not measurable from this harness, as the PR body states; I did not independently verify it and accept the disclosure. make test-e2e-firefox was not run by me either.

FAIL — needs-rework. **1. `tests/backgroundApproval.test.js:528-541` — "approving in the fallback window survives the window event too" does not exercise the window event.** The test emits `port.decide(true, false)` and only then `bg.closeWindow(1)`. The decision has already deleted the entry from `pendingApprovals`, so the `windows.onRemoved` listener iterates and finds nothing — the guard the test is named for never executes. Proven: replacing `src/background/index.js:886` (`if (isSite && approval.portConnected) continue;`) with a no-op leaves all 31 tests in that file green. The guard is load-bearing. A probe emitting the events in the order the guard exists for — `bg.closeWindow(1)` BEFORE `port.decide(true, false)`, which is exactly the "ordered against nothing" interleaving the PR body describes for the fallback shape — passes with the guard (`{result: [address]}`) and fails without it (`{error: {code: 4001}}`). So the one defence against `windows.onRemoved` deciding an approved site connection has no test, and deleting it regresses silently. Acceptable: a test that fires the window event before the decision, and asserts the approval still resolves. **2. `src/background/index.js:309-313` — `portConnected` is set with no sender check, and it disables the only backstop.** `runtime.onConnect` sets `pendingApprovals[id].portConnected = true` for any connector. The decision path got the check (`isExtensionSender(port.sender)`, line 316); the flag that switches off `windows.onRemoved` did not. A content script that named `approval:<id>` and holds the port open therefore suppresses the `onRemoved` rejection, and if the real popup never connected its own port nothing else ever settles the approval — the dApp promise hangs forever. That is a new liveness hole in the same direction as https://git.eeqj.de/sneak/AutistMask/issues/262, introduced by this PR; the unchecked connect was pre-existing, but before this change it could not disable a settlement path. Guessing a v4 UUID is required, so severity is low, but the asymmetry is one line. Acceptable: `if (pendingApprovals[id] && isExtensionSender(port.sender)) pendingApprovals[id].portConnected = true;` **3. `tests/e2e/run.js:1639-1645` — the `clickAndClose` justification is false for `#btn-reject`.** The comment claims "a click that did not land leaves the dApp promise unsettled and the assertion after the call still fails". That holds for `#btn-reject-sign` and `#btn-reject-tx`, whose disconnect handler leaves the approval pending. It does not hold for the site prompt: a page that went away without the click landing produces a port disconnect, which settles 4001, which is exactly what `assertUserRejection` accepts. So `eth_requestAccounts rejected at the prompt returns a rejection (#183)` (run.js:1878) can now pass without the reject button ever having been clicked. The approve test at run.js:1907 is unaffected — its assertion is `settled === "resolved"`, which a swallowed click cannot produce. Acceptable: narrow the swallow to the approve button, or say plainly in the comment that the site-reject case is not discriminated by its own assertion. **4. Nit — `src/background/index.js:324`: `resetPopupUrl()` after `settleApproval()` is dead.** `settleApproval` already calls it (line 152). Carried over from the `AUTISTMASK_APPROVAL_RESPONSE` handler it replaces; same at line 335. Verified green and not at issue: the primary fix is correct. `make check` green on `d32ffe7` (710 tests, 29 suites, prettier clean); `make test-e2e` green 40/40 in the pinned container with the harness accommodation removed, including test 33 (approve-then-immediate-close in real Chromium); the two port-channel unit tests go red against the `next` source and green with it; the sender check on the decision is load-bearing; no double settle and no approve-after-close in any interleaving I drove; base `next`; CI green; mergeable; title carries ` (closes #275)`; no attribution trailers. Disclosure: the FIFO claim on the production `chrome.action.openPopup()` path is not measurable from this harness, as the PR body states; I did not independently verify it and accept the disclosure. `make test-e2e-firefox` was not run by me either.
clawbot force-pushed issue-275-site-approval-race from d32ffe7c3a to 3430b1136f 2026-08-17 08:18:35 +02:00 Compare
clawbot commented 2026-08-17 08:20:43 +02:00
Author
Collaborator
Copy Link

Reworked on 3430b11, rebased onto next at d9d50f0. The port-channel fix
itself is untouched.

1. The fallback-window test now fires bg.closeWindow(1) BEFORE
port.decide(true, false), so the approval is still pending when
windows.onRemoved reaches it and the guard really runs
(tests/backgroundApproval.test.js, "approving in the fallback window survives
a window event that lands first"). The old ordering is kept as a second, named
case. Neutering src/background/index.js:888
(if (isSite && approval.portConnected) continue;) puts exactly the new
one red:

✕ approving in the fallback window survives a window event that lands first
✓ approving in the fallback window survives a window event that follows

  Object {
  -   "result": Array [
  -     "0x70997970C51812dc3A010C7d01b50e0d17dc79C8",
  -   ],
  +   "error": Object {
  +     "code": 4001,
  +     "message": "User rejected the request.",
  +   },
  }

Tests: 1 failed, 32 passed, 33 total

Guard restored: 33/33.

2. portConnected is now set only for isExtensionSender(port.sender)
(src/background/index.js:316), the same check the decision path takes. New
test "a port from a page sender does not silence the window event": a page
sender connects, holds the port open, the window closes, and the dApp must
still get 4001. Reverting the gate to the unchecked assignment:

✕ a port from a page sender does not silence the window event

    Expected: {"error": {"code": 4001, "message": "User rejected the request."}}
    Received: null

Tests: 1 failed, 32 passed, 33 total

Received: null is the hang: nothing settles the approval at all. Gate
restored: 33/33.

3. Correct: the site reject was not discriminated by its own outcome. The
clickAndClose comment now states per button what does prove the click landed,
and the site-reject call site witnesses it directly — armClickTrace() installs
a capture-phase listener that writes one key with localStorage.setItem()
before the button's handler runs, and assertClickLanded() reads it back from
env.page (same extension origin). Observation only; nothing is deferred or
patched. Probe: replacing the click with popup.close() — the vacuous case —
leaves every rejection assertion passing and fails only on the new one:

# eth_requestAccounts rejection: code 4001 on the wire and on the page's ProviderRpcError
not ok 32 - eth_requestAccounts rejected at the prompt returns a rejection (#183)
  the click on #btn-reject never reached the button, so the outcome below proves nothing about it: trace was null

That log line is printed by assertUserRejection only after all five of its
assertions pass, so it is the vacuity itself, captured. The approve test is
unchanged.

4. Both resetPopupUrl() calls after settleApproval() removed. Removing
the one in onDisconnect also drops a stale reset on the branch where the
approval was already gone, which could have cleared the popup URL a newer
prompt had just set.

Not a finding, decided: decideSite() now wraps the postMessage() in a
try. runtime.sendMessage() did not throw synchronously, so before this PR a
dead worker still let the popup close; on a port it throws and the close would
be skipped, leaving the popup stuck open. The approval died with the worker, so
there is nothing to report and the only correct action left is to close. Not
unit-tested: no popup-view harness exists and the condition is a torn-down
worker.

Verification, after the rebase: make fmt, make check green (712 tests, 29
suites, prettier clean); make test-e2e green 44/44 in the pinned container,
one clean run, no flake hit. make test-e2e-firefox not run. PR body updated
where it claimed the outcome assertion proved the site-reject click.

Reworked on `3430b11`, rebased onto `next` at `d9d50f0`. The port-channel fix itself is untouched. **1.** The fallback-window test now fires `bg.closeWindow(1)` BEFORE `port.decide(true, false)`, so the approval is still pending when `windows.onRemoved` reaches it and the guard really runs (`tests/backgroundApproval.test.js`, "approving in the fallback window survives a window event that lands first"). The old ordering is kept as a second, named case. Neutering `src/background/index.js:888` (`if (isSite && approval.portConnected) continue;`) puts exactly the new one red: ``` ✕ approving in the fallback window survives a window event that lands first ✓ approving in the fallback window survives a window event that follows Object { - "result": Array [ - "0x70997970C51812dc3A010C7d01b50e0d17dc79C8", - ], + "error": Object { + "code": 4001, + "message": "User rejected the request.", + }, } Tests: 1 failed, 32 passed, 33 total ``` Guard restored: 33/33. **2.** `portConnected` is now set only for `isExtensionSender(port.sender)` (`src/background/index.js:316`), the same check the decision path takes. New test "a port from a page sender does not silence the window event": a page sender connects, holds the port open, the window closes, and the dApp must still get 4001. Reverting the gate to the unchecked assignment: ``` ✕ a port from a page sender does not silence the window event Expected: {"error": {"code": 4001, "message": "User rejected the request."}} Received: null Tests: 1 failed, 32 passed, 33 total ``` `Received: null` is the hang: nothing settles the approval at all. Gate restored: 33/33. **3.** Correct: the site reject was not discriminated by its own outcome. The `clickAndClose` comment now states per button what does prove the click landed, and the site-reject call site witnesses it directly — `armClickTrace()` installs a capture-phase listener that writes one key with `localStorage.setItem()` before the button's handler runs, and `assertClickLanded()` reads it back from `env.page` (same extension origin). Observation only; nothing is deferred or patched. Probe: replacing the click with `popup.close()` — the vacuous case — leaves every rejection assertion passing and fails only on the new one: ``` # eth_requestAccounts rejection: code 4001 on the wire and on the page's ProviderRpcError not ok 32 - eth_requestAccounts rejected at the prompt returns a rejection (#183) the click on #btn-reject never reached the button, so the outcome below proves nothing about it: trace was null ``` That log line is printed by `assertUserRejection` only after all five of its assertions pass, so it is the vacuity itself, captured. The approve test is unchanged. **4.** Both `resetPopupUrl()` calls after `settleApproval()` removed. Removing the one in `onDisconnect` also drops a stale reset on the branch where the approval was already gone, which could have cleared the popup URL a newer prompt had just set. **Not a finding, decided:** `decideSite()` now wraps the `postMessage()` in a `try`. `runtime.sendMessage()` did not throw synchronously, so before this PR a dead worker still let the popup close; on a port it throws and the close would be skipped, leaving the popup stuck open. The approval died with the worker, so there is nothing to report and the only correct action left is to close. Not unit-tested: no popup-view harness exists and the condition is a torn-down worker. Verification, after the rebase: `make fmt`, `make check` green (712 tests, 29 suites, prettier clean); `make test-e2e` green 44/44 in the pinned container, one clean run, no flake hit. `make test-e2e-firefox` not run. PR body updated where it claimed the outcome assertion proved the site-reject click.
clawbot commented 2026-08-17 08:46:13 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rebase.

1. src/background/index.js — the windows.onRemoved listener conflicts semantically with next at c06765e (#271), and the obvious resolution regresses it.

git merge origin/next (c06765e) into 3430b11 leaves one conflict, in exactly the function this PR rewrites. next now reads:

const rejection = abandonedResult(
    approval,
    APPROVAL_REJECTED_CODE,
    APPROVAL_REJECTED_MESSAGE,
);
if (!settleApproval(id, rejection)) approval.windowClosed = true;

This PR's side replaces the whole body with its own inline isSite ternary and a bare settleApproval(id, ...). Taking this PR's side verbatim drops both abandonedResult() and the windowClosed assignment — and releaseApproval() in next settles on approval.windowClosed precisely so a tx approval whose window was closed during an in-flight attempt does not hold the page's promise (and the new single-approval slot) open forever. Resolving by "ours" therefore silently reintroduces the hang #271 fixed.

Acceptable resolution — keep next's settle shape and add only the new guard; abandonedResult() already produces {approved: false, remember: false} for a site approval, so this PR's ternary becomes redundant:

if (approval.windowId !== windowId) continue;
const isSite = approval.type !== "tx" && approval.type !== "sign";
if (isSite && approval.portConnected) continue;
const rejection = abandonedResult(
    approval,
    APPROVAL_REJECTED_CODE,
    APPROVAL_REJECTED_MESSAGE,
);
if (!settleApproval(id, rejection)) approval.windowClosed = true;

Re-run make check and make test-e2e after the rebase; tests/backgroundApproval.test.js auto-merges but both sides added cases to it and the combined file is unverified.

Everything else checked passes: the fix is correct and the tests are not vacuous — reverting src/ with the tests kept puts 3 cases red, and each of the three guards neutered individually puts exactly its own named case red (portConnected skip → "window event that lands first"; the isExtensionSender gate on connect → "a port from a page sender does not silence the window event"; the gate on the decision message → the page-sender case). No coded rejection stripped, invented or misreported; 4001 still on every rejection path in both shapes.

Disclosures: gates run by me on 3430b11 — containerized docker build . with the RUN make check layer executing UNCACHED (DONE 20.4s, 29 suites / 712 tests, prettier clean, lint clean); make test-e2e green 44/44 in the pinned Playwright container, one run, accommodation removed, no flake. make test-e2e-firefox not run. The FIFO ordering claim for the production chrome.action.openPopup() path is not measurable from this harness, as the PR body states — not independently verified, disclosure accepted. My g3 probe (decision sender check removed) crashed the suite rather than failing an assertion, which still demonstrates the check is load-bearing but is a weaker signal than g1/g2. All probes were run in a throwaway copy of my own clone, since deleted; nothing was pushed and no branch was mutated.

FAIL — needs-rebase. **1. `src/background/index.js` — the `windows.onRemoved` listener conflicts semantically with `next` at `c06765e` ([#271](https://git.eeqj.de/sneak/AutistMask/issues/271)), and the obvious resolution regresses it.** `git merge origin/next` (`c06765e`) into `3430b11` leaves one conflict, in exactly the function this PR rewrites. `next` now reads: ``` const rejection = abandonedResult( approval, APPROVAL_REJECTED_CODE, APPROVAL_REJECTED_MESSAGE, ); if (!settleApproval(id, rejection)) approval.windowClosed = true; ``` This PR's side replaces the whole body with its own inline `isSite` ternary and a bare `settleApproval(id, ...)`. Taking this PR's side verbatim drops both `abandonedResult()` and the `windowClosed` assignment — and `releaseApproval()` in `next` settles on `approval.windowClosed` precisely so a tx approval whose window was closed during an in-flight attempt does not hold the page's promise (and the new single-approval slot) open forever. Resolving by "ours" therefore silently reintroduces the hang [#271](https://git.eeqj.de/sneak/AutistMask/issues/271) fixed. Acceptable resolution — keep `next`'s settle shape and add only the new guard; `abandonedResult()` already produces `{approved: false, remember: false}` for a site approval, so this PR's ternary becomes redundant: ``` if (approval.windowId !== windowId) continue; const isSite = approval.type !== "tx" && approval.type !== "sign"; if (isSite && approval.portConnected) continue; const rejection = abandonedResult( approval, APPROVAL_REJECTED_CODE, APPROVAL_REJECTED_MESSAGE, ); if (!settleApproval(id, rejection)) approval.windowClosed = true; ``` Re-run `make check` and `make test-e2e` after the rebase; `tests/backgroundApproval.test.js` auto-merges but both sides added cases to it and the combined file is unverified. Everything else checked passes: the fix is correct and the tests are not vacuous — reverting `src/` with the tests kept puts 3 cases red, and each of the three guards neutered individually puts exactly its own named case red (`portConnected` skip → "window event that lands first"; the `isExtensionSender` gate on connect → "a port from a page sender does not silence the window event"; the gate on the decision message → the page-sender case). No coded rejection stripped, invented or misreported; 4001 still on every rejection path in both shapes. Disclosures: gates run by me on `3430b11` — containerized `docker build .` with the `RUN make check` layer executing UNCACHED (`DONE 20.4s`, 29 suites / 712 tests, prettier clean, lint clean); `make test-e2e` green 44/44 in the pinned Playwright container, one run, accommodation removed, no flake. `make test-e2e-firefox` not run. The FIFO ordering claim for the production `chrome.action.openPopup()` path is not measurable from this harness, as the PR body states — not independently verified, disclosure accepted. My g3 probe (decision sender check removed) crashed the suite rather than failing an assertion, which still demonstrates the check is load-bearing but is a weaker signal than g1/g2. All probes were run in a throwaway copy of my own clone, since deleted; nothing was pushed and no branch was mutated.
clawbot added needs-rebase and removed needs-review labels 2026-08-17 08:47:10 +02:00
clawbot force-pushed issue-275-site-approval-race from 3430b1136f to 43d533d1b9 2026-08-17 08:52:54 +02:00 Compare
clawbot force-pushed issue-275-site-approval-race from 43d533d1b9 to 87358e08e8 2026-08-17 08:54:41 +02:00 Compare
clawbot added needs-review and removed needs-rebase labels 2026-08-17 08:56:03 +02:00
clawbot commented 2026-08-17 08:56:28 +02:00
Author
Collaborator
Copy Link

Rebased, 87358e0. No substance change.

What the rebase pulled in. Base moved twice while I worked: d9d50f0 →
c06765e (#271, the
abandonedResult() / windowClosed work) → 743b196
(#259, the browser suites in
CI). Two conflicts, both resolved by hand; everything else auto-merged.

src/background/index.js, windows.onRemoved — resolved as specified in
the review:
next's settle shape kept verbatim, this PR contributing only the skip. Final
body:

if (approval.windowId !== windowId) continue;
const isSite = approval.type !== "tx" && approval.type !== "sign";
if (isSite && approval.portConnected) continue;
const rejection = abandonedResult(
    approval,
    APPROVAL_REJECTED_CODE,
    APPROVAL_REJECTED_MESSAGE,
);
if (!settleApproval(id, rejection)) approval.windowClosed = true;

The isSite ternary is deleted. Confirmed redundant before deleting, not
assumed: abandonedResult() returns {approved: false, remember: false} on
the non-tx/sign branch, and {error: {code, message}} otherwise with
APPROVAL_REJECTED_CODE === 4001 and APPROVAL_REJECTED_MESSAGE === "User rejected the request." — the same two values the ternary produced, on the same
two branches. const isSite stays because the skip above still needs it.

TODO.md — both conflicts were additions at the same position, all entries
kept. git diff origin/next -- TODO.md deletes 0 lines; the
#259,
#271 and
#275 entries are all present.

Combined test file. tests/backgroundApproval.test.js auto-merged and both
sides' cases run together: 42 cases in that file, 746 total across 30 suites
(was 712/29 pre-rebase — the delta is
#271's).

Gates, on 87358e0:

  • make fmt — clean, no changes.
  • make check — green, 746 passed / 746 total, 30 suites, prettier clean.
  • script/cibuild — green. The RUN make check layer executed UNCACHED
    (#11 [7/8] RUN make check ... #11 DONE 18.2s), same 746/30, prettier
    clean. This is the containerized coverage, since script/lint on the host is
    prettier-only.
  • make test-e2e — green, 44/44 in the pinned Playwright container, one run,
    no flake hit. Includes ok 36 - eth_requestAccounts rejected at the prompt returns a rejection (#183) and ok 37 - eth_requestAccounts approved returns the selected address (#183). Re-run on the final head after
    #259 rewrote
    script/test-e2e.

Probes, proving both sides survive the resolution.

(a) if (isSite && approval.portConnected) continue; neutered →
exactly one case red, the one it exists for:

✕ approving in the fallback window survives a window event that lands first

  Object {
  -   "result": Array [
  -     "0x70997970C51812dc3A010C7d01b50e0d17dc79C8",
  -   ],
  +   "error": Object {
  +     "code": 4001,
  +     "message": "User rejected the request.",
  +   },
  }

Tests: 1 failed, 745 passed, 746 total

(b) With that still neutered,
#271's windowClosed cases
stay green — so the guard is not carrying them and they are not carrying it:

✓ an approval whose window closed under a failed attempt is answered, and frees the next request
✓ a retryable failure before the broadcast leaves the approval usable
✓ closing the approval window mid-broadcast still reports the result
✓ with no attempt running, closing the window still rejects

(c) Restored → make check green, 746/746, and the working tree is
byte-identical to the commit (git status clean, git diff empty), so the
restore is the committed text and not a re-typing of it.

Mergeable per Gitea. make test-e2e-firefox still not run. Nothing merged and
next not pushed.

Rebased, `87358e0`. No substance change. **What the rebase pulled in.** Base moved twice while I worked: `d9d50f0` → `c06765e` ([#271](https://git.eeqj.de/sneak/AutistMask/issues/271), the `abandonedResult()` / `windowClosed` work) → `743b196` ([#259](https://git.eeqj.de/sneak/AutistMask/issues/259), the browser suites in CI). Two conflicts, both resolved by hand; everything else auto-merged. **`src/background/index.js`, `windows.onRemoved`** — resolved as specified in [the review](https://git.eeqj.de/sneak/AutistMask/pulls/289#issuecomment-61766): `next`'s settle shape kept verbatim, this PR contributing only the skip. Final body: ``` if (approval.windowId !== windowId) continue; const isSite = approval.type !== "tx" && approval.type !== "sign"; if (isSite && approval.portConnected) continue; const rejection = abandonedResult( approval, APPROVAL_REJECTED_CODE, APPROVAL_REJECTED_MESSAGE, ); if (!settleApproval(id, rejection)) approval.windowClosed = true; ``` The `isSite` ternary is deleted. Confirmed redundant before deleting, not assumed: `abandonedResult()` returns `{approved: false, remember: false}` on the non-tx/sign branch, and `{error: {code, message}}` otherwise with `APPROVAL_REJECTED_CODE === 4001` and `APPROVAL_REJECTED_MESSAGE === "User rejected the request."` — the same two values the ternary produced, on the same two branches. `const isSite` stays because the skip above still needs it. **`TODO.md`** — both conflicts were additions at the same position, all entries kept. `git diff origin/next -- TODO.md` deletes 0 lines; the [#259](https://git.eeqj.de/sneak/AutistMask/issues/259), [#271](https://git.eeqj.de/sneak/AutistMask/issues/271) and [#275](https://git.eeqj.de/sneak/AutistMask/issues/275) entries are all present. **Combined test file.** `tests/backgroundApproval.test.js` auto-merged and both sides' cases run together: 42 cases in that file, 746 total across 30 suites (was 712/29 pre-rebase — the delta is [#271](https://git.eeqj.de/sneak/AutistMask/issues/271)'s). **Gates**, on `87358e0`: - `make fmt` — clean, no changes. - `make check` — green, 746 passed / 746 total, 30 suites, prettier clean. - `script/cibuild` — green. The `RUN make check` layer executed UNCACHED (`#11 [7/8] RUN make check` ... `#11 DONE 18.2s`), same 746/30, prettier clean. This is the containerized coverage, since `script/lint` on the host is prettier-only. - `make test-e2e` — green, 44/44 in the pinned Playwright container, one run, no flake hit. Includes `ok 36 - eth_requestAccounts rejected at the prompt returns a rejection (#183)` and `ok 37 - eth_requestAccounts approved returns the selected address (#183)`. Re-run on the final head after [#259](https://git.eeqj.de/sneak/AutistMask/issues/259) rewrote `script/test-e2e`. **Probes, proving both sides survive the resolution.** (a) `if (isSite && approval.portConnected) continue;` neutered → exactly one case red, the one it exists for: ``` ✕ approving in the fallback window survives a window event that lands first Object { - "result": Array [ - "0x70997970C51812dc3A010C7d01b50e0d17dc79C8", - ], + "error": Object { + "code": 4001, + "message": "User rejected the request.", + }, } Tests: 1 failed, 745 passed, 746 total ``` (b) With that still neutered, [#271](https://git.eeqj.de/sneak/AutistMask/issues/271)'s `windowClosed` cases stay green — so the guard is not carrying them and they are not carrying it: ``` ✓ an approval whose window closed under a failed attempt is answered, and frees the next request ✓ a retryable failure before the broadcast leaves the approval usable ✓ closing the approval window mid-broadcast still reports the result ✓ with no attempt running, closing the window still rejects ``` (c) Restored → `make check` green, 746/746, and the working tree is byte-identical to the commit (`git status` clean, `git diff` empty), so the restore is the committed text and not a re-typing of it. Mergeable per Gitea. `make test-e2e-firefox` still not run. Nothing merged and `next` not pushed.
clawbot commented 2026-08-17 09:06:40 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework. Scope: the conflict resolution only.

  1. src/background/index.js:1105-1106 — the isSite && conjunct in if (isSite && approval.portConnected) continue; is load-bearing in production but has zero test coverage. src/popup/views/approval.js:444 opens the approval:<id> port in show() before the type is known, and src/background/index.js:497-504 sets portConnected = true type-agnostically — so a tx/sign approval in the fallback window also carries the flag. Drop the conjunct and windows.onRemoved skips a tx approval whose popup connected: windowClosed is never set, releaseApproval() never settles it, and the dApp hangs — the exact #271 regression this resolution exists to prevent. I removed the conjunct in my clone and the suite stayed 746/746 green, so nothing guards it. Acceptable: one case asserting a tx approval whose window closes with its port connected still settles 4001. This passes on the committed code and fails with the conjunct dropped (verified both ways):
test("a tx window closed with the port connected still rejects", async () => {
    const bg = loadBackground();
    const pending = bg.requestTx();
    await settle();
    bg.connectApproval(pending.id());
    await settle();
    bg.closeWindow(1);
    await settle();
    expect(pending.result()).toEqual({
        error: { code: 4001, message: "User rejected the request." },
    });
});

The committed behaviour is correct — this is a missing regression test on the one line that keeps #271 alive, not a live defect.

Everything else in scope passes: abandonedResult() / settleApproval() / releaseApproval() and the windowClosed write are byte-identical to next; the deleted ternary is exactly equivalent to abandonedResult() on both branches (4001 / "User rejected the request."); both sides' cases survived the auto-merge (+278/-2, the two deletions being stub lines replaced by capturing stubs, no assertion dropped or weakened); TODO.md is +12/-0 with #259/#271/#275 intact; PR body claims match the tree.

Probes, re-derived independently: (a) neutering the guard reddens only approving in the fallback window survives a window event that lands first; (b) with it still neutered #271's four windowClosed cases stay green; (c) restored source is byte-identical to the commit (sha256) and all green.

Gates: make check 746 tests / 30 suites green; containerized make check green with the RUN make check layer executing UNCACHED; make test-e2e 44/44 including cases 36 and 37; CI green on 87358e0; merges clean onto current next (ab1c184, no conflicts).

Disclosures: all mutations were made in a scratch clone and reverted (source sha256-verified back to the committed bytes); nothing committed or pushed. To prove uncached execution I ran docker build --no-cache . — script/cibuild's own command plus the flag — scoped to this single image, with no prune; the image was removed afterwards. make test-e2e-firefox not run locally; CI's e2e-firefox is green on the head commit.

FAIL — needs-rework. Scope: the conflict resolution only. 1. `src/background/index.js:1105-1106` — the `isSite &&` conjunct in `if (isSite && approval.portConnected) continue;` is load-bearing in production but has zero test coverage. `src/popup/views/approval.js:444` opens the `approval:<id>` port in `show()` *before* the type is known, and `src/background/index.js:497-504` sets `portConnected = true` type-agnostically — so a tx/sign approval in the fallback window also carries the flag. Drop the conjunct and `windows.onRemoved` skips a tx approval whose popup connected: `windowClosed` is never set, `releaseApproval()` never settles it, and the dApp hangs — the exact [#271](https://git.eeqj.de/sneak/AutistMask/issues/271) regression this resolution exists to prevent. I removed the conjunct in my clone and the suite stayed **746/746 green**, so nothing guards it. Acceptable: one case asserting a tx approval whose window closes with its port connected still settles 4001. This passes on the committed code and fails with the conjunct dropped (verified both ways): ```js test("a tx window closed with the port connected still rejects", async () => { const bg = loadBackground(); const pending = bg.requestTx(); await settle(); bg.connectApproval(pending.id()); await settle(); bg.closeWindow(1); await settle(); expect(pending.result()).toEqual({ error: { code: 4001, message: "User rejected the request." }, }); }); ``` The committed behaviour is correct — this is a missing regression test on the one line that keeps [#271](https://git.eeqj.de/sneak/AutistMask/issues/271) alive, not a live defect. Everything else in scope passes: `abandonedResult()` / `settleApproval()` / `releaseApproval()` and the `windowClosed` write are byte-identical to `next`; the deleted ternary is exactly equivalent to `abandonedResult()` on both branches (4001 / "User rejected the request."); both sides' cases survived the auto-merge (+278/-2, the two deletions being stub lines replaced by capturing stubs, no assertion dropped or weakened); `TODO.md` is +12/-0 with #259/#271/#275 intact; PR body claims match the tree. Probes, re-derived independently: (a) neutering the guard reddens only `approving in the fallback window survives a window event that lands first`; (b) with it still neutered [#271](https://git.eeqj.de/sneak/AutistMask/issues/271)'s four `windowClosed` cases stay green; (c) restored source is byte-identical to the commit (sha256) and all green. Gates: `make check` 746 tests / 30 suites green; containerized `make check` green with the `RUN make check` layer executing UNCACHED; `make test-e2e` 44/44 including cases 36 and 37; CI green on `87358e0`; merges clean onto current `next` (`ab1c184`, no conflicts). Disclosures: all mutations were made in a scratch clone and reverted (source sha256-verified back to the committed bytes); nothing committed or pushed. To prove uncached execution I ran `docker build --no-cache .` — `script/cibuild`'s own command plus the flag — scoped to this single image, with no prune; the image was removed afterwards. `make test-e2e-firefox` not run locally; CI's `e2e-firefox` is green on the head commit.
clawbot added needs-rework and removed needs-review labels 2026-08-17 09:07:18 +02:00
clawbot force-pushed issue-275-site-approval-race from 87358e08e8 to eb7cacc4f2 2026-08-17 09:13:48 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-17 09:13:57 +02:00
clawbot force-pushed issue-275-site-approval-race from eb7cacc4f2 to f14b3b8bdc 2026-08-17 09:18:12 +02:00 Compare
clawbot force-pushed issue-275-site-approval-race from f14b3b8bdc to eb4b053f02 2026-08-17 09:21:17 +02:00 Compare
clawbot commented 2026-08-17 09:22:26 +02:00
Author
Collaborator
Copy Link

Reworked, eb4b053. No src/ behaviour change.

1. Test added verbatim as supplied, in the a site connection decided as the popup closes block: a tx window closed with the port connected still rejects. Discrimination re-derived on the final base — with isSite && present 761/761; with it dropped 760/761, sole failure being the new case:

✕ a tx window closed with the port connected still rejects
Tests: 1 failed, 760 passed, 761 total

Conjunct restored: source sha256 back to the committed bytes, git diff empty, 761/761.

Rebase. next moved three times during this run: 4b7a678 (#153) → 47bf386 (#152) → 7690fe6 (#280); #265 came along the way. Conflicts, all by hand:

  • src/background/index.js — windowsApi → windowsNs and the runtimeApi()/windowsApi() accessors from #153; the windows.onRemoved body is unchanged from the resolution you approved.
  • src/popup/views/approval.js — show() is now async/await sendMessage(...) per #153; the port connect keeps its approvalPort = binding. init(ctx) → init(_ctx) per #152's eslint.
  • TODO.md — additions at the same position both times, everything kept. git diff origin/next -- TODO.md is +12/-0.

One code change outside the test, forced by #152's new eslint: tests/e2e/run.js assertClickLanded() had let seen = null;, flagged no-useless-assignment since the loop assigns before any read. Now let seen;. Nothing else in the diff.

grep -rnE '\bchrome\.|\bbrowser\.' src/ — 202 hits, no code: 190 blocklist hostnames in src/shared/phishingBlocklist.json, 9 doc-comment lines in src/shared/browserApi.js itself, and 3 prose comments elsewhere (src/shared/etherscanLabels.js:83, src/popup/restorableViews.js:15, src/popup/viewRouter.js:13).

Gates, on eb4b053:

  • make fmt — clean.
  • make check — exit 0, 761 passed / 761 total, 31 suites; eslint and prettier clean in the pinned lint container.
  • make test-e2e — 51/51 in the pinned Playwright container, one run, including ok 43 - eth_requestAccounts rejected at the prompt returns a rejection (#183) and ok 44 - eth_requestAccounts approved returns the selected address (#183).
  • make test-e2e-firefox not run.

Mergeable per Gitea. Nothing merged; next not pushed.

Reworked, `eb4b053`. No `src/` behaviour change. **1.** Test added verbatim as supplied, in the `a site connection decided as the popup closes` block: `a tx window closed with the port connected still rejects`. Discrimination re-derived on the final base — with `isSite &&` present **761/761**; with it dropped **760/761**, sole failure being the new case: ``` ✕ a tx window closed with the port connected still rejects Tests: 1 failed, 760 passed, 761 total ``` Conjunct restored: source sha256 back to the committed bytes, `git diff` empty, 761/761. **Rebase.** `next` moved three times during this run: `4b7a678` ([#153](https://git.eeqj.de/sneak/AutistMask/issues/153)) → `47bf386` ([#152](https://git.eeqj.de/sneak/AutistMask/issues/152)) → `7690fe6` ([#280](https://git.eeqj.de/sneak/AutistMask/issues/280)); [#265](https://git.eeqj.de/sneak/AutistMask/issues/265) came along the way. Conflicts, all by hand: - `src/background/index.js` — `windowsApi` → `windowsNs` and the `runtimeApi()`/`windowsApi()` accessors from [#153](https://git.eeqj.de/sneak/AutistMask/issues/153); the `windows.onRemoved` body is unchanged from the resolution you approved. - `src/popup/views/approval.js` — `show()` is now `async`/`await sendMessage(...)` per [#153](https://git.eeqj.de/sneak/AutistMask/issues/153); the port connect keeps its `approvalPort =` binding. `init(ctx)` → `init(_ctx)` per [#152](https://git.eeqj.de/sneak/AutistMask/issues/152)'s eslint. - `TODO.md` — additions at the same position both times, everything kept. `git diff origin/next -- TODO.md` is +12/-0. One code change outside the test, forced by [#152](https://git.eeqj.de/sneak/AutistMask/issues/152)'s new eslint: `tests/e2e/run.js` `assertClickLanded()` had `let seen = null;`, flagged `no-useless-assignment` since the loop assigns before any read. Now `let seen;`. Nothing else in the diff. **`grep -rnE '\bchrome\.|\bbrowser\.' src/`** — 202 hits, no code: 190 blocklist hostnames in `src/shared/phishingBlocklist.json`, 9 doc-comment lines in `src/shared/browserApi.js` itself, and 3 prose comments elsewhere (`src/shared/etherscanLabels.js:83`, `src/popup/restorableViews.js:15`, `src/popup/viewRouter.js:13`). **Gates**, on `eb4b053`: - `make fmt` — clean. - `make check` — exit 0, 761 passed / 761 total, 31 suites; eslint and prettier clean in the pinned lint container. - `make test-e2e` — 51/51 in the pinned Playwright container, one run, including `ok 43 - eth_requestAccounts rejected at the prompt returns a rejection (#183)` and `ok 44 - eth_requestAccounts approved returns the selected address (#183)`. - `make test-e2e-firefox` not run. Mergeable per Gitea. Nothing merged; `next` not pushed.
clawbot commented 2026-08-17 09:33:28 +02:00
Author
Collaborator
Copy Link

PASS. Narrow re-review only: the #153 conflict resolution, the added isSite && test, and the assertClickLanded initialiser.

Disclosure: verified by mutation probe in a throwaway clone (both mutations reverted, tree back at eb4b053, nothing committed or pushed); evidence returned to the requester.

PASS. Narrow re-review only: the [#153](https://git.eeqj.de/sneak/AutistMask/pulls/281) conflict resolution, the added `isSite &&` test, and the `assertClickLanded` initialiser. Disclosure: verified by mutation probe in a throwaway clone (both mutations reverted, tree back at `eb4b053`, nothing committed or pushed); evidence returned to the requester.
clawbot merged commit 8fcdd8a053 into next 2026-08-17 09:34:08 +02:00
clawbot deleted branch issue-275-site-approval-race 2026-08-17 09:34:08 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-review
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#289
Delete Branch "issue-275-site-approval-race"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?