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

fix: an approval claimed when its window closes is orphaned, leaving the dApp promise unsettled forever #262

New Issue
Closed
opened 2026-08-12 11:00:04 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-12 11:00:04 +02:00
Collaborator
Copy Link
Copy Source

Found by the round-5 independent review of #205 and reproduced there. Non-blocking for that PR, which passed.

The approval window is closed while an attempt holds the claim, and the attempt then fails retryably at the verify stage (src/background/index.js:914). The release path returns the approval to the pool — but the window it would be retried from no longer exists, and nothing else will ever settle it. The dApp's eth_sendTransaction promise stays pending indefinitely.

This is the deliberate safe side of the round-3 trade, not a regression: settling on that path is precisely the fund-loss defect round 3 found, where an in-flight broadcast was reported to the dApp as 4001 User rejected the request. while the transaction actually sent. Liveness was given up to buy that safety. It should not be given up permanently.

Acceptable fix, from the review: re-check the window on release, and settle 4001 if it is gone. That distinguishes "retry is still possible" from "nobody can ever retry this", which the current release path conflates.

Also in scope: three settlement paths with no test coverage

The suite added by #205 covers the window-close, address-switch and reject paths against a claimed approval. Three more route through settleApproval() and are exercised only by the reviewer's throwaway probes:

  • AUTISTMASK_APPROVAL_RESPONSE, approve polarity
  • AUTISTMASK_APPROVAL_RESPONSE, reject polarity
  • the runtime.onConnect port disconnect

All three were verified correct by hand — each is refused against a mid-broadcast claimed approval and the dApp still receives its result — but nothing in the repo would catch a regression. Given that every one of the five review rounds on that PR found a defect in exactly this machinery, they should be pinned.

One tightening, not a defect

A TX_RESPONSE carrying an attacker-paying artifact aimed at a personal_sign or connection approval (neither has txParams) broadcasts nothing — but it fails closed via a TypeError rather than an explicit type check. Worth making explicit so a future refactor cannot turn an incidental throw into a path.

Definition of done

  • A retryable failure after the approval window has closed settles the dApp promise rather than orphaning it.
  • A retryable failure while the window is still open still leaves the approval retryable — the round-3 fund-loss path must not reopen.
  • Tests cover both, plus the three uncovered settlement paths above against a claimed approval.
  • The approval type is checked explicitly before a TX_RESPONSE is honoured.
  • TODO.md updated in the same commit.
  • make check passes.
Found by the round-5 independent review of https://git.eeqj.de/sneak/AutistMask/pulls/205 and reproduced there. Non-blocking for that PR, which passed. The approval window is closed while an attempt holds the claim, and the attempt then fails **retryably** at the verify stage (`src/background/index.js:914`). The release path returns the approval to the pool — but the window it would be retried from no longer exists, and nothing else will ever settle it. The dApp's `eth_sendTransaction` promise stays pending indefinitely. This is the deliberate safe side of the round-3 trade, not a regression: settling on that path is precisely the fund-loss defect round 3 found, where an in-flight broadcast was reported to the dApp as `4001 User rejected the request.` while the transaction actually sent. Liveness was given up to buy that safety. It should not be given up permanently. Acceptable fix, from the review: **re-check the window on release, and settle `4001` if it is gone.** That distinguishes "retry is still possible" from "nobody can ever retry this", which the current release path conflates. ## Also in scope: three settlement paths with no test coverage The suite added by https://git.eeqj.de/sneak/AutistMask/pulls/205 covers the window-close, address-switch and reject paths against a claimed approval. Three more route through `settleApproval()` and are exercised only by the reviewer's throwaway probes: - `AUTISTMASK_APPROVAL_RESPONSE`, approve polarity - `AUTISTMASK_APPROVAL_RESPONSE`, reject polarity - the `runtime.onConnect` port disconnect All three were verified correct by hand — each is refused against a mid-broadcast claimed approval and the dApp still receives its result — but nothing in the repo would catch a regression. Given that every one of the five review rounds on that PR found a defect in exactly this machinery, they should be pinned. ## One tightening, not a defect A `TX_RESPONSE` carrying an attacker-paying artifact aimed at a `personal_sign` or connection approval (neither has `txParams`) broadcasts nothing — but it fails closed via a `TypeError` rather than an explicit type check. Worth making explicit so a future refactor cannot turn an incidental throw into a path. ## Definition of done - [ ] A retryable failure after the approval window has closed settles the dApp promise rather than orphaning it. - [ ] A retryable failure while the window is still open still leaves the approval retryable — the round-3 fund-loss path must not reopen. - [ ] Tests cover both, plus the three uncovered settlement paths above against a claimed approval. - [ ] The approval type is checked explicitly before a `TX_RESPONSE` is honoured. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.
clawbot added this to the 1.0.0 milestone 2026-09-21 09:20:00 +02:00
clawbot commented 2026-09-21 09:36:38 +02:00
Author
Collaborator
Copy Link
Copy Source

Completed the remaining parts of the definition of done in
#391.

The liveness fix this issue asks for — settle 4001 on release when the window a
retry would use is gone — already landed on next with
#271, together with tests for the
after-close and still-open cases.

This change makes AUTISTMASK_TX_RESPONSE refuse any approval that is not a
transaction approval, so a reject no longer retires a sign or connection
approval and a signed artifact never runs the broadcast path against one. Tests
pin the three settlement paths named here — the site-connection port's approve,
reject and disconnect — against a transaction approval broadcasting behind them,
plus the new type check against a sign and a connection approval.

Model: opus-4-8

Completed the remaining parts of the definition of done in https://git.eeqj.de/sneak/AutistMask/pulls/391. The liveness fix this issue asks for — settle `4001` on release when the window a retry would use is gone — already landed on `next` with https://git.eeqj.de/sneak/AutistMask/issues/271, together with tests for the after-close and still-open cases. This change makes `AUTISTMASK_TX_RESPONSE` refuse any approval that is not a transaction approval, so a reject no longer retires a sign or connection approval and a signed artifact never runs the broadcast path against one. Tests pin the three settlement paths named here — the site-connection port's approve, reject and disconnect — against a transaction approval broadcasting behind them, plus the new type check against a sign and a connection approval. Model: opus-4-8
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#262