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

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

New Issue
Closed
opened 2026-08-12 13:05:50 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-12 13:05:50 +02:00
Collaborator
Copy Link
Copy Source

btn-approve in src/popup/views/approval.js calls runtime.sendMessage() and then window.close(). The close disconnects the approval port, and port.onDisconnect settles a pending site approval as a rejection. Whether the user's approval or their window closing arrives first is a race.

Measured by the e2e work on #183: driven in a tab, the teardown wins every time — the user clicks Approve and the dApp is told they rejected. The harness has to defer that one window.close() and close the page itself after the outcome, with the reason documented at the call site.

Transaction and signature approvals are NOT affected: their disconnect handler leaves the approval pending rather than rejecting it. It is the site-connection path specifically.

Reachability — establish this before choosing a fix

The measurement is from a tab, because the browser-action popup cannot be driven (headless Chromium opens it but Playwright never exposes it as a page). In production the site prompt goes through chrome.action.openPopup(), and the teardown timing there may differ. So the observed "every time" is a tab result, not necessarily a production result.

That uncertainty is a reason to fix it, not to defer it: the ordering is unspecified either way, and the failure mode is that a user who approved a connection is reported as having refused it.

Implementation requirements

  • Make the outcome independent of teardown ordering rather than tuning the timing. The approve message and the disconnect must not be able to settle the same approval differently depending on which lands first.
  • Consider the same shape the transaction path already uses — a disconnect that leaves a claimed or in-flight approval pending — and whether the site path should simply adopt it.
  • Do not have the popup delay its own close to win the race; that is what the harness does as a test accommodation and it is not a fix.
  • Say in the PR body what you established about the production chrome.action.openPopup() path.

Definition of done

  • Approving a site connection reports approval regardless of when the popup closes.
  • Rejecting, and closing the window without deciding, both still report a rejection.
  • A test drives approve-then-immediate-close and asserts the approval survives.
  • The e2e harness's deferred-window.close() accommodation can be removed, or the PR body says why it must stay.
  • TODO.md updated in the same commit.
  • make check passes.
`btn-approve` in `src/popup/views/approval.js` calls `runtime.sendMessage()` and then `window.close()`. The close disconnects the approval port, and `port.onDisconnect` settles a pending **site** approval as a rejection. Whether the user's approval or their window closing arrives first is a race. Measured by the e2e work on https://git.eeqj.de/sneak/AutistMask/issues/183: driven in a tab, **the teardown wins every time** — the user clicks Approve and the dApp is told they rejected. The harness has to defer that one `window.close()` and close the page itself after the outcome, with the reason documented at the call site. Transaction and signature approvals are NOT affected: their disconnect handler leaves the approval pending rather than rejecting it. It is the site-connection path specifically. ## Reachability — establish this before choosing a fix The measurement is from a tab, because the browser-action popup cannot be driven (headless Chromium opens it but Playwright never exposes it as a page). In production the site prompt goes through `chrome.action.openPopup()`, and the teardown timing there may differ. So the observed "every time" is a tab result, not necessarily a production result. That uncertainty is a reason to fix it, not to defer it: the ordering is unspecified either way, and the failure mode is that a user who approved a connection is reported as having refused it. ## Implementation requirements - Make the outcome independent of teardown ordering rather than tuning the timing. The approve message and the disconnect must not be able to settle the same approval differently depending on which lands first. - Consider the same shape the transaction path already uses — a disconnect that leaves a claimed or in-flight approval pending — and whether the site path should simply adopt it. - Do not have the popup delay its own close to win the race; that is what the harness does as a test accommodation and it is not a fix. - Say in the PR body what you established about the production `chrome.action.openPopup()` path. ## Definition of done - [ ] Approving a site connection reports approval regardless of when the popup closes. - [ ] Rejecting, and closing the window without deciding, both still report a rejection. - [ ] A test drives approve-then-immediate-close and asserts the approval survives. - [ ] The e2e harness's deferred-`window.close()` accommodation can be removed, or the PR body says why it must stay. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.
clawbot added this to the 1.0.0 milestone 2026-08-12 13:05:59 +02:00
clawbot commented 2026-08-14 06:04:54 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan.

The decision and the disconnect race because they travel on two independent
channels: runtime.sendMessage() is a one-off port of its own, the teardown
rides the approval port, and nothing orders them relative to each other. No
amount of claiming or flag-setting in the background fixes that, because the
claim would have to arrive over the same racing channel.

So: move the site-connection decision onto the approval port the popup already
opens in show(). port.postMessage() then window.close() puts the decision
and the disconnect on ONE channel, which is FIFO — the decision is delivered
first by construction, whatever the teardown timing is. The background settles
on that message; onDisconnect then finds nothing pending and rejects nothing.
Close-without-deciding still disconnects with the approval pending, so it still
rejects, and so does Reject. AUTISTMASK_APPROVAL_RESPONSE goes away with its
sender check carried over to the port (the id comes from the port name, so the
popup no longer names an approval id at all).

This is the tx/sign shape — the deciding party holds the channel and the
disconnect defers to it — without giving the site path a
windows.onRemoved backstop it cannot have: under
chrome.action.openPopup() the toolbar popup is not a window, so the port
disconnect is the ONLY close signal, which is why leaving it pending outright
is not available here.

The popup does not delay its close and no timing is tuned.

Proof, both demonstrated red against the unfixed code first:

  1. tests/backgroundApproval.test.js — raise a site approval, post the
    decision on the port, disconnect immediately, assert the dApp gets the
    address. Driven in both shapes, including the production one where
    action.openPopup() succeeds and no window exists. Plus: disconnect with no
    decision rejects, Reject rejects, a decision from a non-extension sender is
    ignored.
  2. tests/e2e/run.js — remove the deferred-window.close() accommodation, so
    the shipped popup really does approve-then-immediate-close in a real
    Chromium and the existing "approved returns the selected address" assertion
    becomes the real-browser proof. If it will not come out green the
    accommodation stays and the PR body says why.

The PR body will state what I established about chrome.action.openPopup()
versus the tab measurement, and will carry the red output.

Plan. The decision and the disconnect race because they travel on two independent channels: `runtime.sendMessage()` is a one-off port of its own, the teardown rides the approval port, and nothing orders them relative to each other. No amount of claiming or flag-setting in the background fixes that, because the claim would have to arrive over the same racing channel. So: move the site-connection decision onto the approval port the popup already opens in `show()`. `port.postMessage()` then `window.close()` puts the decision and the disconnect on ONE channel, which is FIFO — the decision is delivered first by construction, whatever the teardown timing is. The background settles on that message; `onDisconnect` then finds nothing pending and rejects nothing. Close-without-deciding still disconnects with the approval pending, so it still rejects, and so does Reject. `AUTISTMASK_APPROVAL_RESPONSE` goes away with its sender check carried over to the port (the id comes from the port name, so the popup no longer names an approval id at all). This is the tx/sign shape — the deciding party holds the channel and the disconnect defers to it — without giving the site path a `windows.onRemoved` backstop it cannot have: under `chrome.action.openPopup()` the toolbar popup is not a window, so the port disconnect is the ONLY close signal, which is why leaving it pending outright is not available here. The popup does not delay its close and no timing is tuned. Proof, both demonstrated red against the unfixed code first: 1. `tests/backgroundApproval.test.js` — raise a site approval, post the decision on the port, disconnect immediately, assert the dApp gets the address. Driven in both shapes, including the production one where `action.openPopup()` succeeds and no window exists. Plus: disconnect with no decision rejects, Reject rejects, a decision from a non-extension sender is ignored. 2. `tests/e2e/run.js` — remove the deferred-`window.close()` accommodation, so the shipped popup really does approve-then-immediate-close in a real Chromium and the existing "approved returns the selected address" assertion becomes the real-browser proof. If it will not come out green the accommodation stays and the PR body says why. The PR body will state what I established about `chrome.action.openPopup()` versus the tab measurement, and will carry the red output.
clawbot commented 2026-08-14 06:24:36 +02:00
Author
Collaborator
Copy Link
Copy Source

Built as planned, in
#289 (base next).

Definition of done:

  • Approval reported regardless of when the popup closes — the decision rides
    the approval port, which is the same channel the close disconnects, so it is
    delivered ahead of the disconnect. windows.onRemoved also stopped deciding
    a site approval whose port is connected: in the fallback-window shape that
    event raced the decision on a channel of its own, the same defect one level
    over.
  • Rejecting rejects; closing without deciding rejects; and a window that closes
    before its popup ever connected still rejects, so nothing hangs.
  • Approve-then-immediate-close is driven both in
    tests/backgroundApproval.test.js (decision and close with nothing awaited
    between them, in the toolbar-popup shape production uses and in the
    fallback-window shape) and, now, in a real Chromium.
  • The harness accommodation is REMOVED.
  • TODO.md updated in the same commit.

Demonstrated red first. Real browser, accommodation removed, unfixed code:
eth_requestAccounts came back to the page as
{"settled":"rejected","code":4001} after Allow was clicked, and the seven
later tests fell over behind it because the origin never became authorized —
29/37. Unit: the two approve-then-close cases failed with the 4001 rejection
and the four rejection cases passed, which is the defect exactly.

Verified on the branch rebased onto next at 0be20d7: make fmt, then
make check green (710 tests, prettier clean), and make test-e2e green 40/40
in the pinned container. make test-e2e-firefox was not run. Full evidence and
what I established about the production chrome.action.openPopup() path are in
the PR body.

Built as planned, in [#289](https://git.eeqj.de/sneak/AutistMask/pulls/289) (base `next`). Definition of done: - Approval reported regardless of when the popup closes — the decision rides the approval port, which is the same channel the close disconnects, so it is delivered ahead of the disconnect. `windows.onRemoved` also stopped deciding a site approval whose port is connected: in the fallback-window shape that event raced the decision on a channel of its own, the same defect one level over. - Rejecting rejects; closing without deciding rejects; and a window that closes before its popup ever connected still rejects, so nothing hangs. - Approve-then-immediate-close is driven both in `tests/backgroundApproval.test.js` (decision and close with nothing awaited between them, in the toolbar-popup shape production uses and in the fallback-window shape) and, now, in a real Chromium. - The harness accommodation is REMOVED. - `TODO.md` updated in the same commit. Demonstrated red first. Real browser, accommodation removed, unfixed code: `eth_requestAccounts` came back to the page as `{"settled":"rejected","code":4001}` after Allow was clicked, and the seven later tests fell over behind it because the origin never became authorized — 29/37. Unit: the two approve-then-close cases failed with the 4001 rejection and the four rejection cases passed, which is the defect exactly. Verified on the branch rebased onto `next` at `0be20d7`: `make fmt`, then `make check` green (710 tests, prettier clean), and `make test-e2e` green 40/40 in the pinned container. `make test-e2e-firefox` was not run. Full evidence and what I established about the production `chrome.action.openPopup()` path are in the PR body.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#275