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

clawbot · 2026-08-14T06:24:19+02:00

clawbot commented

2026-08-14 06:24:19 +02:00

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.

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.

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; the dApp-side
outcome assertion after each call is what proves the click landed.

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, 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 the windows.onRemoved event too
closing the fallback window without deciding is a rejection
a window that closes before its popup ever connected still rejects

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.

Verification

Ran, on this branch rebased onto next at 0be20d7:

make fmt, then make check — green: 710 tests, 28 suites, prettier clean.
make test-e2e — green, 40/40, in the pinned Playwright container.

Before the rebase, on the same tree: three consecutive clean make test-e2e
runs green at 37/37. A fourth run that overlapped a concurrent make check on
this shared host failed eth_sendTransaction rejected with "the extension
opened no approval window within 30000ms" — a load-sensitive wait on the tx
path, which this change does not touch. Reported rather than hidden.

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. `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. 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; the dApp-side outcome assertion after each call is what proves the click landed. ## 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`, 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 the `windows.onRemoved` event too - closing the fallback window without deciding is a rejection - a window that closes before its popup ever connected still rejects `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. ## Verification Ran, on this branch rebased onto `next` at `0be20d7`: - `make fmt`, then `make check` — green: 710 tests, 28 suites, prettier clean. - `make test-e2e` — green, 40/40, in the pinned Playwright container. Before the rebase, on the same tree: three consecutive clean `make test-e2e` runs green at 37/37. A fourth run that overlapped a concurrent `make check` on this shared host failed `eth_sendTransaction rejected` with "the extension opened no approval window within 30000ms" — a load-sensitive wait on the tx path, which this change does not touch. Reported rather than hidden. `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 )

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 referenced this pull request

2026-08-14 06:24:36 +02:00

fix: approving a site connection races the popup teardown and can be recorded as a rejection #275

check / check (push) Successful in 30s

Details

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin issue-275-site-approval-race:issue-275-site-approval-race

git checkout issue-275-site-approval-race

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#289