harden: verify all approval fields and make failed signing retryable (closes #174) #205

Merged

clawbot merged 1 commits from harden/issue-174-approval-verify-fields into next 2026-08-12 11:21:03 +02:00

Conversation 6 Commits 1 Files Changed 6 +2227 -123

clawbot commented 2026-08-11 14:26:19 +02:00

Collaborator

Copy Link

Closes #174.

The vulnerability this closes, which is live on next today

next compares from, to, value and data and nothing else. It does not
look at the transaction type. Measured against origin/next's own
src/shared/approvalVerify.js with the repo's ethers 6.16.0:

NEXT TYPE4 parsed.type=4 delegates_to=0xdAC17F958D2ee523a2206206994597C13D831ec7 => PASSED VERIFICATION

The approval is the ordinary dApp eth_sendTransaction shape
({from, to, value, data}, no fee fields, which is the normal case because
populateTransaction() fills them). The artifact is an EIP-7702 type 4
transaction with identical to / value / data / chainId / from, plus a
signed authorization delegating the signer's own EOA to an attacker address.
The user approves a plain ETH transfer; the broadcast also installs attacker
code at the user's own account, permanently. Every field on the approval screen
matches. eth_sendRawTransaction accepts type 4 on both supported networks
post-Pectra, so this is live, not theoretical.

The first revision of this PR did not close it: it added per-field comparisons
but left parsed.type unconstrained and never looked at authorizationList.
Same result on that revision. Type 3 had the same shape: blobVersionedHashes
uncompared, maxFeePerBlobGas absent from the fee ceilings, and type 3
explicitly admitted as EIP-1559.

Why this is now an allowlist, and what makes it exhaustive

A field-by-field denylist cannot be correct against a transaction format that
gains fields: every new EIP-2718 type adds consequential content that defaults
to unchecked. The check is now closed in both directions.

1. The type is allowlisted. parsed.type must be 0, 1 or 2 — the only
types this wallet signs, since populateTransaction() produces nothing else.
Anything else is refused before a single field is compared, because the type is
what decides which fields exist at all. Types 3 and 4 are refused by this, not
incidentally by a fee check that happens to fire only when the approval carried
a fee.

2. Fields no allowed type may carry are refused by name.
authorizationList, blobs, blobVersionedHashes and maxFeePerBlobGas.
Redundant with the type allowlist by construction — that is the point — and
each has its own test, because nothing reachable would otherwise exercise them.

3. The access list is compared with the approval. It was previously
uncompared on both types that carry it.

4. Verification closes by rebuilding the artifact from the checked fields
and comparing the bytes. SERIALIZED_FIELDS names the serialized fields of
each allowed type; the transaction is rebuilt from exactly those and
unsignedSerialized is compared. This is what makes the approach exhaustive
rather than one bug better: any field the artifact carries that this module
does not account for is absent from the rebuild, changes the bytes, and is
refused without having to be anticipated. The final assertion is that the
artifact is the approved transaction, not that it is none of the tampered
shapes someone thought of.

5. The artifact's own bytes are required to be canonical. Both sides of the
byte comparison derive from one Transaction.from(), while what is broadcast
is the artifact string. An artifact re-encoded with a leading zero byte on an
RLP quantity therefore decoded to the approved transaction, passed, and
broadcast different bytes. assertCanonicalBytes() requires the artifact to be
the canonical encoding of its own decode, which is what makes "this is the
approved transaction" true of the bytes that actually go to the node.

6. A tripwire on ethers itself. One test asserts that the set of accessors
Transaction.prototype exposes is exactly the set this module accounts for —
checked, refused by name, or derived (from, hash, signature, the
serializations). An ethers upgrade that introduces a transaction field fails
the suite and forces a decision about it, instead of letting it default to
unchecked.

Post-fix, against this branch head:

TYPE4 parsed.type=4 delegates_to=0xdAC17F958D2ee523a2206206994597C13D831ec7
  => REFUSED (approvalMismatch=true): The signed transaction is of a type this
     wallet does not sign, so what it would do beyond the approved transfer
     cannot be checked.
TYPE3 parsed.type=3
  => REFUSED (approvalMismatch=true): The signed transaction is of a type this
     wallet does not sign, so what it would do beyond the approved transfer
     cannot be checked.
TYPE2 + unapproved access list
  => REFUSED (approvalMismatch=true): The signed transaction does not carry the
     approved access list.
TYPE2 honest artifact
  => PASSED VERIFICATION

The type 4 probe, the type 3 probe and the access list probe are all committed
regression tests, not just evidence.

One approval can no longer send funds twice

A broadcastTransaction() throw previously left the approval pending and
retryable: true. The popup's retry does not re-broadcast the artifact it
already produced — it re-runs populateTransaction() and signTransaction(),
minting a different transaction at a freshly fetched pending-tag nonce. A
broadcast that throws after the node accepted the transaction is routine (a
timeout, a dropped response, a node answering "already known"), so tx1 sits in
the mempool, the retry takes nonce N+1, and the approved transfer executes
twice.

Broadcast failure is now terminal: the approval is spent, the error is resolved
to the dApp, and the popup does not offer the button again. The popup-side
msg.error path that #174
actually asked about — the wrong-password case — stays retryable, and a
verification mismatch still spends the approval. The three stages are one
exported decision function, describeTxFailure(stage, err), with a test each.

A failed broadcast also gets its own user-facing wording. Telling the user to
"start it again from the site" after a broadcast whose outcome is unknown is
the one instruction that could produce the double spend by hand; it now says
the transaction may still have reached the network and to check the account
first.

The interlock, and the single chokepoint that makes it hold

Keeping the approval alive so a wrong password can be retried costs it its
single use. The handler reads the approval, then verifies and broadcasts
asynchronously, so a second AUTISTMASK_TX_RESPONSE carrying the same id would
start an independent verify and broadcast. Nothing in the ordinary dApp
approval shape fixes a nonce, so two artifacts signed at different nonces both
verify. The approval is therefore claimed synchronously, before the handler's
first await, and released only when an attempt fails in a way the user may
retry.

Surviving the whole verify-and-broadcast window also put the approval within
reach of every other path that retires one, and those did not consult the
claim:

• windows.onRemoved — the user closes the approval popup.
• broadcastAccountsChanged() — the user switches active address.
• a late !msg.approved reject on either response type.

Each resolved the waiting promise 4001 User rejected the request. while the
attempt behind it ran to completion. The attempt's own resolve({txHash}) then
landed on an already-settled promise: the transaction reached the chain and
the page was told the user rejected it. The natural response is to redo the
transfer from the site, which re-signs at a fresh nonce — the double send this
PR exists to prevent, reached with no adversary at all, since
src/popup/views/approval.js keeps the popup open across the broadcast and a
user closing an apparently-hung window is enough.

The fix is one chokepoint, not three patched call sites.
settleApproval(id, result, {holdsClaim}) is now the only place an
approval is resolved or removed — grep over src/background/index.js finds
exactly one delete pendingApprovals[...] and one approval.resolve(...),
both inside it — and it refuses a claimed approval unless the caller holds the
claim. A retirement path added later inherits the interlock instead of having
to remember it. broadcastAccountsChanged() additionally leaves a claimed
approval's window standing rather than force-closing the window the attempt is
reporting into.

The duplicate refusal on the sign path previously omitted stage, so the popup
told the user to start again from the site while the first attempt might still
succeed. Both in-flight refusals now carry a stage whose wording says the first
attempt is still running and may still succeed.

An approved value that is not a number

normalizeValue() called BigInt(v) bare. A page-controlled value of
"cheap", 1.5, "1e18" or {} threw a raw SyntaxError/RangeError with
approvalMismatch === undefined, so it was reported retryable, the approval was
never spent and approval.resolve was never called — the exact dead-button
shape #174 exists to remove.
value now goes through normalizeQuantity() like every other quantity and
refuses as a mismatch.

Fields compared

field	rule
transaction type	must be 0, 1 or 2; anything else refuses
authorizationList, blobs, blobVersionedHashes, maxFeePerBlobGas	must be absent
chainId	must equal the selected network's chain id, always; and the approval's chainId too when the page fixed one. An unknown selected network refuses.
to, value, data	equality
accessList	equality; absent and [] are the same thing
nonce, gasLimit, gasPrice, maxFeePerGas, maxPriorityFeePerGas	equality when the approval carries one
fee mechanism	an approval naming EIP-1559 fees must not be signed as legacy, and one naming gasPrice must not be signed as EIP-1559
the artifact's own encoding	must be the canonical encoding of its decode
everything else	refused by the closing byte comparison

Why nonce, gas limit and fees are conditional, not blanket equality

The dApp usually fixes none of them: populateTransaction() in the popup fills
in nonce, gas limit, fees and chain id from the provider. There is then no
approved value to compare against, and a blanket equality check would refuse
every legitimate transaction. Those locally populated values are instead held
to two absolute ceilings, chosen so that nothing includable is ever refused:

• gas limit <= 100,000,000 — above the block gas limit of both supported
  networks (src/shared/networks.js).
• each fee per gas <= 100,000 gwei — far above anything mainnet or Sepolia
  has produced.

These are a sanity bound, which is what
#174 asked for, not a limit
on loss: a bare 21,000-gas transfer at the fee ceiling still hands the
validator 2.1 ETH. The user is protected from the absurd, not from the ruinous.
Tightening them is a separate product decision.

The chain id needs no such carve-out: the background knows the selected network
independently of the artifact, which is what makes a cross-chain replay
impossible.

Known gap, deliberately left alone: a dApp-supplied gas (the JSON-RPC
spelling) is not compared, because ethers' copyRequest drops the key and
the popup estimates instead — so the signed gas limit is not an approved value.

Normalization rules

• Addresses: compared through EIP-55 checksumming, so case never matters;
  absent on both sides is equal (contract creation), absent on one side is not.
• Quantities (chainId, nonce, gasLimit, all fee fields, value):
  coerced to BigInt, so hex, decimal string, number and bigint compare equal.
  A value that is not a number refuses rather than passes.
• value: absent means zero, matching what ethers signs.
• Call data: lowercased; absent, "" and 0x are the same thing.
• Access list: normalized through ethers' accessListify, then lowercased;
  absent and [] are equal. A malformed approved list refuses.
• Optional fields (nonce, gasLimit, fees): absent means not approved,
  never zero.

Verification

make check on the branch rebased onto next at bd4bdca:

Test Suites: 20 passed, 20 total
Tests:       482 passed, 482 total
All matched files use Prettier code style!

Also run in the container. script/lint is prettier --check on the host, so
the gating run is docker build --no-cache ., which runs make check and
make build inside the image. The check layer executed, uncached — no CACHED
on it:

#14 [7/8] RUN make check
#14 9.198 Test Suites: 20 passed, 20 total
#14 9.198 Tests:       482 passed, 482 total
#14 12.71 All matched files use Prettier code style!
#14 16.35 All matched files use Prettier code style!
#14 DONE 16.5s
#15 2.466 verify-build: 4 bundle(s) verified autistmask-build-debug=off
#15 DONE 2.5s

The background wiring is now driven end to end

tests/backgroundApproval.test.js loads src/background/index.js against
stubbed browser and network APIs and drives it through the real message
listener, from a dApp eth_sendTransaction to the broadcast. The approval
verification is the real module. windows.onRemoved is captured, not stubbed
as a no-op — swallowing it is what let the retirement defect through a
previous revision.

Six tests cover the retirement paths. Negative-verified: with the claim check
in settleApproval() disabled, the first four fail with the exact defect
signature and the two controls still pass.

x closing the approval window mid-broadcast still reports the result
    Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x switching the active address mid-broadcast still reports the result
    Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x a reject arriving mid-broadcast is refused, not honoured
    Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x a reject during a sign attempt is refused with the in-flight stage
    Expected: ObjectContaining {"retryable": false, "stage": "inflight"}
+ with no attempt running, closing the window still rejects
+ with no attempt running, an active-address switch still rejects and closes
Tests: 4 failed, 2 passed

With the guard in place all six pass, the dApp receives {result: "0xfeed"} in
each mid-broadcast case, and broadcastTransaction is called exactly once. The
two controls are there so the refusals cannot quietly cost a genuine rejection
its meaning.

Alongside them, the duplicate-response tests: a second
AUTISTMASK_TX_RESPONSE, the same artifact twice, a response arriving after
the broadcast finished, a cross-type AUTISTMASK_SIGN_RESPONSE, a retryable
failure leaving the approval usable, a mismatch spending it outright, and a
page sender refused.

Mutation matrix

Every comparison in src/shared/approvalVerify.js, disabled one at a time in
the working tree with the suite re-run against it and the file restored from
git afterwards. 25 of 25 mutants killed:

01 signer address                       KILLED   02 transaction type allowlist  KILLED
03 forbidden fields                     KILLED   04 selected network present    KILLED
05 chain id vs selected network         KILLED   06 chain id vs approval        KILLED
07 recipient                            KILLED   08 value                       KILLED
09 call data                            KILLED   10 access list                 KILLED
11 fee mechanism                        KILLED   12 approved nonce              KILLED
13 approved gas limit                   KILLED   14 approved gas price          KILLED
15 approved max fee per gas             KILLED   16 approved max priority fee   KILLED
17 gas limit ceiling                    KILLED   18 fee per gas ceiling         KILLED
19 structural rebuild                   KILLED   20 value refusal routing       KILLED
21 quantity refusal                     KILLED   22 access list refusal         KILLED
23 broadcast failure terminal           KILLED   24 verify failure mismatch     KILLED
25 broadcast wording                    KILLED

Mutants 03 and 19 — the two layers that sit behind the type allowlist — first
survived, because nothing reachable through verifySignedTx can trip them
while the allowlist holds. Rather than leave two untested guards, they were
extracted as assertNoForbiddenFields() and assertNothingUnchecked() and
given direct tests, and both now die.

Not verified

• The popup paths are not reachable from the unit suite (no DOM environment),
  so the retry and stage behaviour is covered at the decision functions the
  popup and background call, not by driving the button. make test-e2e does
  not yet cover the approval flow.
• Pre-existing, unchanged by this diff, and noted rather than fixed here:
  txParams.from is never compared, so an active-address switch between
  approval and signing yields a transaction from an account the approval did
  not name. Verification also compares against the dApp's request, never
  against what the popup displayed — for every field the dApp omitted, the
  number the user read on the approval screen is verified by nothing. The
  structural fix is for the background to populate the transaction itself and
  hand the complete approved set to the popup, which is a larger change.
• script/lint is prettier --check on the host; there is no ESLint in the
  check chain yet (#152).
  Everything reported above was additionally run inside the container.

Closes [#174](https://git.eeqj.de/sneak/AutistMask/issues/174). ## The vulnerability this closes, which is live on `next` today `next` compares `from`, `to`, `value` and `data` and nothing else. It does not look at the transaction type. Measured against `origin/next`'s own `src/shared/approvalVerify.js` with the repo's ethers 6.16.0: ``` NEXT TYPE4 parsed.type=4 delegates_to=0xdAC17F958D2ee523a2206206994597C13D831ec7 => PASSED VERIFICATION ``` The approval is the ordinary dApp `eth_sendTransaction` shape (`{from, to, value, data}`, no fee fields, which is the normal case because `populateTransaction()` fills them). The artifact is an EIP-7702 type 4 transaction with identical `to` / `value` / `data` / `chainId` / `from`, plus a signed authorization delegating the signer's own EOA to an attacker address. The user approves a plain ETH transfer; the broadcast also installs attacker code at the user's own account, permanently. Every field on the approval screen matches. `eth_sendRawTransaction` accepts type 4 on both supported networks post-Pectra, so this is live, not theoretical. The first revision of this PR did not close it: it added per-field comparisons but left `parsed.type` unconstrained and never looked at `authorizationList`. Same result on that revision. Type 3 had the same shape: `blobVersionedHashes` uncompared, `maxFeePerBlobGas` absent from the fee ceilings, and type 3 explicitly admitted as EIP-1559. ## Why this is now an allowlist, and what makes it exhaustive A field-by-field denylist cannot be correct against a transaction format that gains fields: every new EIP-2718 type adds consequential content that defaults to unchecked. The check is now closed in both directions. **1. The type is allowlisted.** `parsed.type` must be 0, 1 or 2 — the only types this wallet signs, since `populateTransaction()` produces nothing else. Anything else is refused before a single field is compared, because the type is what decides which fields exist at all. Types 3 and 4 are refused by this, not incidentally by a fee check that happens to fire only when the approval carried a fee. **2. Fields no allowed type may carry are refused by name.** `authorizationList`, `blobs`, `blobVersionedHashes` and `maxFeePerBlobGas`. Redundant with the type allowlist by construction — that is the point — and each has its own test, because nothing reachable would otherwise exercise them. **3. The access list is compared with the approval.** It was previously uncompared on both types that carry it. **4. Verification closes by rebuilding the artifact from the checked fields and comparing the bytes.** `SERIALIZED_FIELDS` names the serialized fields of each allowed type; the transaction is rebuilt from exactly those and `unsignedSerialized` is compared. This is what makes the approach exhaustive rather than one bug better: any field the artifact carries that this module does not account for is absent from the rebuild, changes the bytes, and is refused **without having to be anticipated**. The final assertion is that the artifact *is* the approved transaction, not that it is none of the tampered shapes someone thought of. **5. The artifact's own bytes are required to be canonical.** Both sides of the byte comparison derive from one `Transaction.from()`, while what is broadcast is the artifact string. An artifact re-encoded with a leading zero byte on an RLP quantity therefore decoded to the approved transaction, passed, and broadcast different bytes. `assertCanonicalBytes()` requires the artifact to be the canonical encoding of its own decode, which is what makes "this *is* the approved transaction" true of the bytes that actually go to the node. **6. A tripwire on ethers itself.** One test asserts that the set of accessors `Transaction.prototype` exposes is exactly the set this module accounts for — checked, refused by name, or derived (`from`, `hash`, `signature`, the serializations). An ethers upgrade that introduces a transaction field fails the suite and forces a decision about it, instead of letting it default to unchecked. Post-fix, against this branch head: ``` TYPE4 parsed.type=4 delegates_to=0xdAC17F958D2ee523a2206206994597C13D831ec7 => REFUSED (approvalMismatch=true): The signed transaction is of a type this wallet does not sign, so what it would do beyond the approved transfer cannot be checked. TYPE3 parsed.type=3 => REFUSED (approvalMismatch=true): The signed transaction is of a type this wallet does not sign, so what it would do beyond the approved transfer cannot be checked. TYPE2 + unapproved access list => REFUSED (approvalMismatch=true): The signed transaction does not carry the approved access list. TYPE2 honest artifact => PASSED VERIFICATION ``` The type 4 probe, the type 3 probe and the access list probe are all committed regression tests, not just evidence. ## One approval can no longer send funds twice A `broadcastTransaction()` throw previously left the approval pending and `retryable: true`. The popup's retry does not re-broadcast the artifact it already produced — it re-runs `populateTransaction()` and `signTransaction()`, minting a different transaction at a freshly fetched pending-tag nonce. A broadcast that throws *after* the node accepted the transaction is routine (a timeout, a dropped response, a node answering "already known"), so tx1 sits in the mempool, the retry takes nonce N+1, and the approved transfer executes twice. Broadcast failure is now terminal: the approval is spent, the error is resolved to the dApp, and the popup does not offer the button again. The popup-side `msg.error` path that [#174](https://git.eeqj.de/sneak/AutistMask/issues/174) actually asked about — the wrong-password case — stays retryable, and a verification mismatch still spends the approval. The three stages are one exported decision function, `describeTxFailure(stage, err)`, with a test each. A failed broadcast also gets its own user-facing wording. Telling the user to "start it again from the site" after a broadcast whose outcome is unknown is the one instruction that could produce the double spend by hand; it now says the transaction may still have reached the network and to check the account first. ### The interlock, and the single chokepoint that makes it hold Keeping the approval alive so a wrong password can be retried costs it its single use. The handler reads the approval, then verifies and broadcasts asynchronously, so a second `AUTISTMASK_TX_RESPONSE` carrying the same id would start an independent verify and broadcast. Nothing in the ordinary dApp approval shape fixes a nonce, so two artifacts signed at different nonces both verify. The approval is therefore *claimed* synchronously, before the handler's first `await`, and released only when an attempt fails in a way the user may retry. Surviving the whole verify-and-broadcast window also put the approval within reach of every **other** path that retires one, and those did not consult the claim: - `windows.onRemoved` — the user closes the approval popup. - `broadcastAccountsChanged()` — the user switches active address. - a late `!msg.approved` reject on either response type. Each resolved the waiting promise `4001 User rejected the request.` while the attempt behind it ran to completion. The attempt's own `resolve({txHash})` then landed on an already-settled promise: **the transaction reached the chain and the page was told the user rejected it.** The natural response is to redo the transfer from the site, which re-signs at a fresh nonce — the double send this PR exists to prevent, reached with no adversary at all, since `src/popup/views/approval.js` keeps the popup open across the broadcast and a user closing an apparently-hung window is enough. The fix is one chokepoint, not three patched call sites. `settleApproval(id, result, {holdsClaim})` is now the **only** place an approval is resolved or removed — `grep` over `src/background/index.js` finds exactly one `delete pendingApprovals[...]` and one `approval.resolve(...)`, both inside it — and it refuses a claimed approval unless the caller holds the claim. A retirement path added later inherits the interlock instead of having to remember it. `broadcastAccountsChanged()` additionally leaves a claimed approval's window standing rather than force-closing the window the attempt is reporting into. The duplicate refusal on the sign path previously omitted `stage`, so the popup told the user to start again from the site while the first attempt might still succeed. Both in-flight refusals now carry a stage whose wording says the first attempt is still running and may still succeed. ## An approved `value` that is not a number `normalizeValue()` called `BigInt(v)` bare. A page-controlled `value` of `"cheap"`, `1.5`, `"1e18"` or `{}` threw a raw `SyntaxError`/`RangeError` with `approvalMismatch === undefined`, so it was reported retryable, the approval was never spent and `approval.resolve` was never called — the exact dead-button shape [#174](https://git.eeqj.de/sneak/AutistMask/issues/174) exists to remove. `value` now goes through `normalizeQuantity()` like every other quantity and refuses as a mismatch. ## Fields compared | field | rule | | --- | --- | | transaction type | must be 0, 1 or 2; anything else refuses | | `authorizationList`, `blobs`, `blobVersionedHashes`, `maxFeePerBlobGas` | must be absent | | `chainId` | must equal the **selected network**'s chain id, always; and the approval's `chainId` too when the page fixed one. An unknown selected network refuses. | | `to`, `value`, `data` | equality | | `accessList` | equality; absent and `[]` are the same thing | | `nonce`, `gasLimit`, `gasPrice`, `maxFeePerGas`, `maxPriorityFeePerGas` | equality when the approval carries one | | fee mechanism | an approval naming EIP-1559 fees must not be signed as legacy, and one naming `gasPrice` must not be signed as EIP-1559 | | the artifact's own encoding | must be the canonical encoding of its decode | | everything else | refused by the closing byte comparison | ### Why nonce, gas limit and fees are conditional, not blanket equality The dApp usually fixes none of them: `populateTransaction()` in the popup fills in nonce, gas limit, fees and chain id from the provider. There is then no approved value to compare against, and a blanket equality check would refuse every legitimate transaction. Those locally populated values are instead held to two absolute ceilings, chosen so that nothing includable is ever refused: - gas limit &lt;= 100,000,000 — above the block gas limit of both supported networks (`src/shared/networks.js`). - each fee per gas &lt;= 100,000 gwei — far above anything mainnet or Sepolia has produced. These are a sanity bound, which is what [#174](https://git.eeqj.de/sneak/AutistMask/issues/174) asked for, not a limit on loss: a bare 21,000-gas transfer at the fee ceiling still hands the validator 2.1 ETH. The user is protected from the absurd, not from the ruinous. Tightening them is a separate product decision. The chain id needs no such carve-out: the background knows the selected network independently of the artifact, which is what makes a cross-chain replay impossible. Known gap, deliberately left alone: a dApp-supplied `gas` (the JSON-RPC spelling) is **not** compared, because ethers' `copyRequest` drops the key and the popup estimates instead — so the signed gas limit is not an approved value. ### Normalization rules - **Addresses**: compared through EIP-55 checksumming, so case never matters; absent on both sides is equal (contract creation), absent on one side is not. - **Quantities** (`chainId`, `nonce`, `gasLimit`, all fee fields, `value`): coerced to `BigInt`, so hex, decimal string, number and bigint compare equal. A value that is not a number refuses rather than passes. - **`value`**: absent means zero, matching what ethers signs. - **Call data**: lowercased; absent, `""` and `0x` are the same thing. - **Access list**: normalized through ethers' `accessListify`, then lowercased; absent and `[]` are equal. A malformed approved list refuses. - **Optional fields** (`nonce`, `gasLimit`, fees): absent means *not approved*, never zero. ## Verification `make check` on the branch rebased onto `next` at `bd4bdca`: ``` Test Suites: 20 passed, 20 total Tests: 482 passed, 482 total All matched files use Prettier code style! ``` Also run in the container. `script/lint` is `prettier --check` on the host, so the gating run is `docker build --no-cache .`, which runs `make check` and `make build` inside the image. The check layer executed, uncached — no `CACHED` on it: ``` #14 [7/8] RUN make check #14 9.198 Test Suites: 20 passed, 20 total #14 9.198 Tests: 482 passed, 482 total #14 12.71 All matched files use Prettier code style! #14 16.35 All matched files use Prettier code style! #14 DONE 16.5s #15 2.466 verify-build: 4 bundle(s) verified autistmask-build-debug=off #15 DONE 2.5s ``` ### The background wiring is now driven end to end `tests/backgroundApproval.test.js` loads `src/background/index.js` against stubbed browser and network APIs and drives it through the **real** message listener, from a dApp `eth_sendTransaction` to the broadcast. The approval verification is the real module. `windows.onRemoved` is **captured, not stubbed as a no-op** — swallowing it is what let the retirement defect through a previous revision. Six tests cover the retirement paths. Negative-verified: with the claim check in `settleApproval()` disabled, the first four fail with the exact defect signature and the two controls still pass. ``` x closing the approval window mid-broadcast still reports the result Received: {"error": {"code": 4001, "message": "User rejected the request."}} x switching the active address mid-broadcast still reports the result Received: {"error": {"code": 4001, "message": "User rejected the request."}} x a reject arriving mid-broadcast is refused, not honoured Received: {"error": {"code": 4001, "message": "User rejected the request."}} x a reject during a sign attempt is refused with the in-flight stage Expected: ObjectContaining {"retryable": false, "stage": "inflight"} + with no attempt running, closing the window still rejects + with no attempt running, an active-address switch still rejects and closes Tests: 4 failed, 2 passed ``` With the guard in place all six pass, the dApp receives `{result: "0xfeed"}` in each mid-broadcast case, and `broadcastTransaction` is called exactly once. The two controls are there so the refusals cannot quietly cost a genuine rejection its meaning. Alongside them, the duplicate-response tests: a second `AUTISTMASK_TX_RESPONSE`, the same artifact twice, a response arriving after the broadcast finished, a cross-type `AUTISTMASK_SIGN_RESPONSE`, a retryable failure leaving the approval usable, a mismatch spending it outright, and a page sender refused. ### Mutation matrix Every comparison in `src/shared/approvalVerify.js`, disabled one at a time in the working tree with the suite re-run against it and the file restored from git afterwards. 25 of 25 mutants killed: ``` 01 signer address KILLED 02 transaction type allowlist KILLED 03 forbidden fields KILLED 04 selected network present KILLED 05 chain id vs selected network KILLED 06 chain id vs approval KILLED 07 recipient KILLED 08 value KILLED 09 call data KILLED 10 access list KILLED 11 fee mechanism KILLED 12 approved nonce KILLED 13 approved gas limit KILLED 14 approved gas price KILLED 15 approved max fee per gas KILLED 16 approved max priority fee KILLED 17 gas limit ceiling KILLED 18 fee per gas ceiling KILLED 19 structural rebuild KILLED 20 value refusal routing KILLED 21 quantity refusal KILLED 22 access list refusal KILLED 23 broadcast failure terminal KILLED 24 verify failure mismatch KILLED 25 broadcast wording KILLED ``` Mutants 03 and 19 — the two layers that sit behind the type allowlist — first survived, because nothing reachable through `verifySignedTx` can trip them while the allowlist holds. Rather than leave two untested guards, they were extracted as `assertNoForbiddenFields()` and `assertNothingUnchecked()` and given direct tests, and both now die. ## Not verified - The popup paths are not reachable from the unit suite (no DOM environment), so the retry and stage behaviour is covered at the decision functions the popup and background call, not by driving the button. `make test-e2e` does not yet cover the approval flow. - Pre-existing, unchanged by this diff, and noted rather than fixed here: `txParams.from` is never compared, so an active-address switch between approval and signing yields a transaction from an account the approval did not name. Verification also compares against the *dApp's request*, never against what the popup *displayed* — for every field the dApp omitted, the number the user read on the approval screen is verified by nothing. The structural fix is for the background to populate the transaction itself and hand the complete approved set to the popup, which is a larger change. - `script/lint` is `prettier --check` on the host; there is no ESLint in the check chain yet ([#152](https://git.eeqj.de/sneak/AutistMask/issues/152)). Everything reported above was additionally run inside the container.

clawbot added 1 commit 2026-08-11 14:26:19 +02:00

harden: verify all approval fields and make failed signing retryable (closes #174) ... 2c3e431b1d

verifySignedTx compared only from, to, value and data, so a signed
transaction could differ from the approval in chain id, nonce, gas limit
or any fee field and still be broadcast. It now compares every
consequential field and refuses outright on any mismatch: the chain id
against the selected network (and against the approval when the page
fixed one), plus nonce, gas limit, gasPrice, maxFeePerGas and
maxPriorityFeePerGas wherever the approval carries a value, together
with the fee mechanism the approval implies. Fields the approval does
not carry are populated locally by the popup and have no approved value
to compare against, so they are held to absolute ceilings instead.

A failed signing attempt also left a button that could not succeed: the
background deleted the approval before it broadcast, so a retry found
nothing to sign. The approval is now retired only once the request has
an outcome, and the background tells the popup whether the failure is
retryable, so the button comes back for a failure the user can correct
and stays down with an explanation when the approval is spent.

clawbot added the needs-review label 2026-08-11 14:26:27 +02:00

clawbot self-assigned this 2026-08-11 14:26:28 +02:00

clawbot force-pushed harden/issue-174-approval-verify-fields from 2c3e431b1d to 979bea2d0d 2026-08-11 14:27:09 +02:00 Compare

clawbot commented 2026-08-11 14:37:51 +02:00

Author

Collaborator

Copy Link

FAIL — needs-rework. A constructible bypass defeats the module's central guarantee.

1. BLOCKING: an EIP-7702 type-4 artifact passes verification and takes over the account

src/shared/approvalVerify.js:217 — signedEip1559 = parsed.type === 2 || parsed.type === 3. Nothing constrains parsed.type, and authorizationList is never compared or refused. The module enumerates the fields it checks, so every field it does not name is unchecked.

Constructed and executed against 979bea2 using the repo's own ethers 6.16.0 and its own verifySignedTx:

• approval: the ordinary dApp eth_sendTransaction shape {from, to, value: "0x2386f26fc10000", data: "0x"} — no fee fields, which is the common case, since populateTransaction() fills them.
• artifact: type: 4, identical to / value / data / chainId / from, plus authorizationList: [signed authorization delegating the signer's own EOA to 0xdAC17F958D2ee523a2206206994597C13D831ec7].
• result: parsed.type = 4, authorizationList delegates to = 0xdAC1…ec7, PASSED VERIFICATION.

The user approved a plain ETH transfer. The transaction that gets broadcast also permanently installs attacker code at the user's own EOA. Every field shown on the approval screen matches, so the check that exists precisely to guarantee "what was approved is what is broadcast" waves it through. provider.broadcastTransaction() issues eth_sendRawTransaction, which accepts type-4 on both supported networks post-Pectra — this is live, not theoretical.

The near-miss shows the shape of the hole: when the approval does carry maxFeePerGas, the same type-4 artifact is refused — but only incidentally, by the fee-mechanism check (approvedEip1559 && !signedEip1559), never by anything that knows what a type 4 is. The protection is accidental and absent in the common case.

Same reasoning covers type 3, which :217 explicitly admits as signedEip1559: blobVersionedHashes is uncompared and maxFeePerBlobGas is missing from the fee ceiling list at :240.

Acceptable: an allowlist on parsed.type (0/1/2 — the types this wallet itself produces), refusing anything else, plus an explicit refusal of a non-empty parsed.authorizationList. A field-by-field denylist cannot be correct here: every future transaction type adds consequential fields that default to unchecked.

2. BLOCKING: the retry rework lets one approval send funds twice

src/background/index.js:775-788 — a provider.broadcastTransaction() throw now leaves the approval in pendingApprovals and reports retryable: true; src/popup/views/approval.js:559-562 re-enables the button. But the retry at src/popup/views/approval.js:523-537 does not re-broadcast the artifact it already produced — it re-runs populateTransaction() and signTransaction(), minting a different transaction with a freshly fetched pending-tag nonce.

A broadcast that throws after the node accepted the transaction is routine: a timeout or dropped response after propagation, or a node answering "already known". In that state tx1 is in the mempool, the retry populates nonce N+1, and the user's approved transfer executes twice for one approval. Before this PR the approval was deleted ahead of the broadcast, so this was impossible; the retryability is new here.

#174 asked only for the popup-side signing failure (the wrong-password case) to be retryable. Extending retryability to broadcast failures is added scope, and it is the part that carries the hazard.

Acceptable: treat a broadcast failure as terminal (spend the approval, resolve the error to the dApp), keeping retryable: true for the popup-side msg.error path the issue named; or, if broadcast retry is kept, re-broadcast the identical stored rawSignedTx so a duplicate is a no-op at the same nonce.

3. Minor: a non-numeric approved value escapes as a non-mismatch error, reported retryable

src/shared/approvalVerify.js:86-89 — normalizeValue() calls BigInt(v) bare, unlike normalizeQuantity(), which wraps it and refuses. txParams.value is page-controlled; measured against the branch head:

"cheap" -> SyntaxError, approvalMismatch = undefined
1.5     -> RangeError,  approvalMismatch = undefined
"1e18"  -> SyntaxError, approvalMismatch = undefined
{}      -> SyntaxError, approvalMismatch = undefined

failureIsRetryable() returns true, so the approval is never spent and approval.resolve is never called. The user gets a raw Cannot convert cheap to a BigInt beside a live button that can never succeed — the same dead-button shape this issue exists to remove. Fail-closed for signing, so not a bypass. Acceptable: route value through normalizeQuantity(), per the module's own stated rule that an uncomparable quantity refuses.

Ceilings — sound in principle, weak in calibration

Not filed as a defect: #174 explicitly licensed "a sanity bound". Recording the numbers so the choice is on the record. A bare approved transfer (21,000 gas used) at MAX_FEE_PER_GAS = 100,000 gwei hands the validator 2.1 ETH; MAX_GAS_LIMIT x MAX_FEE_PER_GAS bounds the theoretical worst at 10,000 ETH. Against a legitimate mainnet transfer at 50 gwei (~0.001 ETH) the ceiling permits ~2000x overpayment. The user is protected from the absurd, not from the ruinous.

The deeper limitation, which the PR body does not state: verification compares against the dApp's request, never against what the popup displayed. For every field the dApp omitted — normally all of nonce, gas limit and fees — the number the user actually read on the approval screen is verified by nothing. The structurally correct fix is for the background to populate the transaction itself and hand the complete approved set to the popup; that is a separate, larger change.

Verified and passing

Per-field mutation matrix: every one of the 15 comparisons, disabled in isolation, breaks at least one test — zero unverified fields, DoD's "one test per field" satisfied with real teeth. Normalization probes found no bypass (checksum/case, absent-vs-present to, hex/decimal/number/bigint spellings, ""/0x/absent data, "0X", non-string data shapes, decimal-vs-hex chain id all behave). Every mismatch is a hard refusal, never a warning. A verification mismatch does spend the approval and resolve an error to the dApp. Base next, single commit, title ends (closes #174), one TODO.md line, no attribution trailers or vendor references, make check green locally (178 tests), make fmt clean, fast-forwardable onto origin/next.

Anomalies and disclosures

• The "unknown selected network refuses" branch (src/shared/approvalVerify.js:175-179) is unreachable through the production caller: currentNetwork() is networkById(state.networkId), and src/shared/networks.js returns NETWORKS.mainnet for any unrecognised id, so currentNetwork().chainId is never absent. The guard is defensive only; its test passes undefined directly. Not a defect — the mainnet fallback is pre-existing and consistent with what eth_chainId reports — but the PR body's claim overstates what the wiring can produce.
• txParams.from is never compared; expectedFrom is the current active address from getActiveAddress(). Pre-existing and unchanged by this diff, so not filed: noting that an active-address switch between approval and signing yields a transaction from an account the approval did not name.
• CI is not green on 979bea2: check / check (push) is pending / "Waiting to run", still pending on re-check. Secondary to the code defects above.
• This repo has no containerized lint target — script/lint runs prettier --check . on the host, and there is no eslint in the check chain. Lint was run through the make/script/ entrypoint as provided.
• The bypass construction and mutation matrix were done in a throwaway clone, which was restored to a clean tree afterwards. Nothing was committed or pushed.

FAIL — `needs-rework`. A constructible bypass defeats the module's central guarantee. ## 1. BLOCKING: an EIP-7702 type-4 artifact passes verification and takes over the account `src/shared/approvalVerify.js:217` — `signedEip1559 = parsed.type === 2 || parsed.type === 3`. Nothing constrains `parsed.type`, and `authorizationList` is never compared or refused. The module enumerates the fields it checks, so every field it does not name is unchecked. Constructed and executed against `979bea2` using the repo's own ethers 6.16.0 and its own `verifySignedTx`: - approval: the ordinary dApp `eth_sendTransaction` shape `{from, to, value: "0x2386f26fc10000", data: "0x"}` — no fee fields, which is the common case, since `populateTransaction()` fills them. - artifact: `type: 4`, identical `to` / `value` / `data` / `chainId` / `from`, plus `authorizationList: [signed authorization delegating the signer's own EOA to 0xdAC17F958D2ee523a2206206994597C13D831ec7]`. - result: `parsed.type = 4`, `authorizationList delegates to = 0xdAC1…ec7`, **PASSED VERIFICATION**. The user approved a plain ETH transfer. The transaction that gets broadcast also permanently installs attacker code at the user's own EOA. Every field shown on the approval screen matches, so the check that exists precisely to guarantee "what was approved is what is broadcast" waves it through. `provider.broadcastTransaction()` issues `eth_sendRawTransaction`, which accepts type-4 on both supported networks post-Pectra — this is live, not theoretical. The near-miss shows the shape of the hole: when the approval *does* carry `maxFeePerGas`, the same type-4 artifact is refused — but only incidentally, by the fee-mechanism check (`approvedEip1559 &amp;&amp; !signedEip1559`), never by anything that knows what a type 4 is. The protection is accidental and absent in the common case. Same reasoning covers type 3, which `:217` explicitly admits as `signedEip1559`: `blobVersionedHashes` is uncompared and `maxFeePerBlobGas` is missing from the fee ceiling list at `:240`. Acceptable: an allowlist on `parsed.type` (0/1/2 — the types this wallet itself produces), refusing anything else, plus an explicit refusal of a non-empty `parsed.authorizationList`. A field-by-field denylist cannot be correct here: every future transaction type adds consequential fields that default to unchecked. ## 2. BLOCKING: the retry rework lets one approval send funds twice `src/background/index.js:775-788` — a `provider.broadcastTransaction()` throw now leaves the approval in `pendingApprovals` and reports `retryable: true`; `src/popup/views/approval.js:559-562` re-enables the button. But the retry at `src/popup/views/approval.js:523-537` does not re-broadcast the artifact it already produced — it re-runs `populateTransaction()` and `signTransaction()`, minting a *different* transaction with a freshly fetched pending-tag nonce. A broadcast that throws after the node accepted the transaction is routine: a timeout or dropped response after propagation, or a node answering "already known". In that state tx1 is in the mempool, the retry populates nonce N+1, and the user's approved transfer executes **twice** for one approval. Before this PR the approval was deleted ahead of the broadcast, so this was impossible; the retryability is new here. [#174](https://git.eeqj.de/sneak/AutistMask/issues/174) asked only for the popup-side signing failure (the wrong-password case) to be retryable. Extending retryability to broadcast failures is added scope, and it is the part that carries the hazard. Acceptable: treat a broadcast failure as terminal (spend the approval, resolve the error to the dApp), keeping `retryable: true` for the popup-side `msg.error` path the issue named; or, if broadcast retry is kept, re-broadcast the identical stored `rawSignedTx` so a duplicate is a no-op at the same nonce. ## 3. Minor: a non-numeric approved `value` escapes as a non-mismatch error, reported retryable `src/shared/approvalVerify.js:86-89` — `normalizeValue()` calls `BigInt(v)` bare, unlike `normalizeQuantity()`, which wraps it and refuses. `txParams.value` is page-controlled; measured against the branch head: ``` "cheap" -> SyntaxError, approvalMismatch = undefined 1.5 -> RangeError, approvalMismatch = undefined "1e18" -> SyntaxError, approvalMismatch = undefined {} -> SyntaxError, approvalMismatch = undefined ``` `failureIsRetryable()` returns `true`, so the approval is never spent and `approval.resolve` is never called. The user gets a raw `Cannot convert cheap to a BigInt` beside a live button that can never succeed — the same dead-button shape this issue exists to remove. Fail-closed for signing, so not a bypass. Acceptable: route `value` through `normalizeQuantity()`, per the module's own stated rule that an uncomparable quantity refuses. ## Ceilings — sound in principle, weak in calibration Not filed as a defect: [#174](https://git.eeqj.de/sneak/AutistMask/issues/174) explicitly licensed "a sanity bound". Recording the numbers so the choice is on the record. A bare approved transfer (21,000 gas used) at `MAX_FEE_PER_GAS` = 100,000 gwei hands the validator **2.1 ETH**; `MAX_GAS_LIMIT` x `MAX_FEE_PER_GAS` bounds the theoretical worst at **10,000 ETH**. Against a legitimate mainnet transfer at 50 gwei (~0.001 ETH) the ceiling permits ~2000x overpayment. The user is protected from the absurd, not from the ruinous. The deeper limitation, which the PR body does not state: verification compares against the *dApp's request*, never against what the popup *displayed*. For every field the dApp omitted — normally all of nonce, gas limit and fees — the number the user actually read on the approval screen is verified by nothing. The structurally correct fix is for the background to populate the transaction itself and hand the complete approved set to the popup; that is a separate, larger change. ## Verified and passing Per-field mutation matrix: every one of the 15 comparisons, disabled in isolation, breaks at least one test — zero unverified fields, DoD's "one test per field" satisfied with real teeth. Normalization probes found no bypass (checksum/case, absent-vs-present `to`, hex/decimal/number/bigint spellings, `""`/`0x`/absent data, `"0X"`, non-string `data` shapes, decimal-vs-hex chain id all behave). Every mismatch is a hard refusal, never a warning. A verification mismatch does spend the approval and resolve an error to the dApp. Base `next`, single commit, title ends ` (closes #174)`, one `TODO.md` line, no attribution trailers or vendor references, `make check` green locally (178 tests), `make fmt` clean, fast-forwardable onto `origin/next`. ## Anomalies and disclosures - The "unknown selected network refuses" branch (`src/shared/approvalVerify.js:175-179`) is unreachable through the production caller: `currentNetwork()` is `networkById(state.networkId)`, and `src/shared/networks.js` returns `NETWORKS.mainnet` for any unrecognised id, so `currentNetwork().chainId` is never absent. The guard is defensive only; its test passes `undefined` directly. Not a defect — the mainnet fallback is pre-existing and consistent with what `eth_chainId` reports — but the PR body's claim overstates what the wiring can produce. - `txParams.from` is never compared; `expectedFrom` is the *current* active address from `getActiveAddress()`. Pre-existing and unchanged by this diff, so not filed: noting that an active-address switch between approval and signing yields a transaction from an account the approval did not name. - CI is **not green** on `979bea2`: `check / check (push)` is `pending` / "Waiting to run", still pending on re-check. Secondary to the code defects above. - This repo has no containerized lint target — `script/lint` runs `prettier --check .` on the host, and there is no eslint in the check chain. Lint was run through the `make`/`script/` entrypoint as provided. - The bypass construction and mutation matrix were done in a throwaway clone, which was restored to a clean tree afterwards. Nothing was committed or pushed.

clawbot added needs-rework and removed needs-review labels 2026-08-11 14:38:50 +02:00

clawbot referenced this pull request 2026-08-11 14:40:20 +02:00

harden: approval verification compares against the dApp's request, not against what the popup displayed #216

clawbot force-pushed harden/issue-174-approval-verify-fields from 979bea2d0d to a94110ed6c 2026-08-11 15:02:20 +02:00 Compare

clawbot added needs-review and removed needs-rework labels 2026-08-11 15:19:36 +02:00

clawbot commented 2026-08-11 15:39:43 +02:00

Author

Collaborator

Copy Link

FAIL — needs-rework (also conflicts with next).

1. BLOCKING: the approval is no longer single-use across the broadcast, so one approval can still send funds twice

src/background/index.js:730-808. The approval is read at :731 and removed only at :776 / :790 / :800 — after await loadState(), await getActiveAddress() and await provider.broadcastTransaction(). Nothing marks it in-flight.

This is a regression introduced by this diff. Before it, the handler deleted the entry synchronously (86cdea5:src/background/index.js:716, delete pendingApprovals[msg.id] immediately after the lookup — removed here), so a second AUTISTMASK_TX_RESPONSE carrying the same id hit if (!approval) return false. It now finds a live approval and runs a second, independent verify + broadcast.

Consequence: with the ordinary dApp approval shape the PR body itself builds on ({from, to, value, data}, no nonce), two artifacts signed at different nonces both pass verifySignedTx — nothing in the module constrains an unapproved nonce. The approved transfer executes twice, which is exactly the outcome the "One approval can no longer send funds twice" section claims to have closed. The fix closed the sequential retry path and opened a concurrent one.

Reachability, stated plainly:

• AUTISTMASK_GET_APPROVAL (:693-702) still serves the approval and its txParams for the whole in-flight window, so reloading the approval window during a slow broadcast re-renders a live approval screen with a working Approve button. The only guard is setTxButtonBusy, popup-local state that a reload destroys.
• Independently of that, approvalVerify.js:3-8 states the background must not become "a blind relay" for the popup. Under that stated threat model any popup that emits the message twice gets two broadcasts, and the background no longer prevents it.

Same shape at AUTISTMASK_SIGN_RESPONSE (:813-852); lower consequence, same fix.

Acceptable: remove the entry from pendingApprovals synchronously on entry, holding the object in a local for resolve, or set an in-flight flag tested at :731-732 — exactly one broadcast per approval, whatever the popup sends. Add a test driving two AUTISTMASK_TX_RESPONSE messages for one id. Note the author's own disclosure that the background wiring is untested is where this defect lives.

2. The closing byte comparison never sees the bytes that are broadcast

src/shared/approvalVerify.js:287 compares rebuilt.unsignedSerialized against parsed.unsignedSerialized. Both sides derive from the same Transaction.from(rawSignedTx) parse. rawSignedTx is never compared to parsed.serialized, and rawSignedTx — not parsed.serialized — is what src/background/index.js:789 hands to broadcastTransaction(). The guarantee delivered is "the transaction ethers understood is the approved one", one step short of the PR body's "the artifact is the approved transaction".

Measured on this head: a type-2 artifact re-encoded with a leading zero byte on the RLP value field is 238 hex chars against 236 canonical, parsed.serialized !== rawSignedTx, and verifySignedTx PASSES.

Not filed as blocking, because I could not turn it into a bypass: every decoder normalization I could produce is non-canonical RLP, which geth rejects, so the divergent bytes fail at the node rather than executing. if (parsed.serialized !== rawSignedTx) throw refuse(...), or broadcasting parsed.serialized, closes the class outright and makes the stated claim true.

3. Commit is authored sneak <sneak@sneak.berlin>

Every other commit on next is authored clawbot <clawbot@noreply.example.org>. Committer is clawbot; author is not.

4. Conflicts with next

git merge-tree origin/next HEAD conflicts in TODO.md; the branch is based on 86cdea5, two commits behind next at 12acf4d, which added two Completed Steps entries at the same position. The PR's reported verification was run against the stale base.

Verified

next is confirmed vulnerable today: the EIP-7702 type-4 artifact, rebuilt independently against origin/next's own approvalVerify.js at 12acf4d, PASSES verification while delegating the signer's EOA; the same artifact is REFUSED at this head. The type-4, type-3 and access-list probes are real committed tests building genuine signed artifacts (tests/approvalVerify.test.js:343, :361, :432), not PR-body evidence.

Mutation matrix independently re-run, 21/21 killed, no survivors — including both extracted guards, each killed by its own direct test rather than collaterally (assertNoForbiddenFields by "a forbidden field is refused even on an allowed type"; assertNothingUnchecked by "an artifact carrying more than the checked fields is refused"), and the APPROVED_QUANTITIES mutation killing five separately-named field tests. The disclosed void run cost nothing: the committed state contains all five claimed layers and matches the reported 221 tests.

Bypasses attempted and refused: extra RLP field on a type-2 envelope and on legacy (invalid field count), trailing junk (unexpected junk after rlp payload), high-s signature malleability (decode throws), pre-EIP-155 legacy chainId=0 cross-chain replay, unapproved access list on types 1 and 2, contract-creation to substitution in both directions, over-ceiling gas and fee. Fail-closed confirmed for unknown network (undefined/null/""), eight malformed rawSignedTx shapes, six junk approval quantities, a malformed approved access list, and all four normalizeValue probes ("cheap", 1.5, "1e18", {}) — every one an ApprovalMismatchError. describeTxFailure covers all three stages and fails closed to terminal on any unknown or absent stage; the broadcast wording is accurate, not merely reassuring. make check green here: 9 suites, 221 tests, executed not cached, prettier clean. TODO.md loses no landed entry; single commit; title ends (closes #174); base next; no vendor references or attribution trailers.

Ethers tripwire independently checked: I enumerated the 25 Transaction.prototype getters against the module's accounted set and the uncovered remainder is exactly the derived and blob-only accessors the test names.

Disclosures

• The guard at :528-539 is exercised with a hand-built object rather than a real artifact. Acceptable for a guard the allowlist makes unreachable, but it does not demonstrate that a genuine artifact carrying an extra field would be caught.
• My first unknown-network probe printed PASS for selectedChainId=undefined; that was a default-parameter bug in my own harness. Re-run explicitly, the module refuses.
• Exploit and probe harnesses were run with raw node inside a throwaway clone, which is required to construct signed artifacts; all check, test and lint verification went through make. Scratch files removed, tree pristine at a94110e, nothing committed or pushed.
• Tracker CI status ignored per instruction.
• Out of scope per instruction and not counted against this PR: the request-versus-displayed limitation (#216) and the ceiling calibration.

FAIL — `needs-rework` (also conflicts with `next`). ## 1. BLOCKING: the approval is no longer single-use across the broadcast, so one approval can still send funds twice `src/background/index.js:730-808`. The approval is read at `:731` and removed only at `:776` / `:790` / `:800` — after `await loadState()`, `await getActiveAddress()` and `await provider.broadcastTransaction()`. Nothing marks it in-flight. This is a regression introduced by this diff. Before it, the handler deleted the entry synchronously (`86cdea5:src/background/index.js:716`, `delete pendingApprovals[msg.id]` immediately after the lookup — removed here), so a second `AUTISTMASK_TX_RESPONSE` carrying the same `id` hit `if (!approval) return false`. It now finds a live approval and runs a second, independent verify + broadcast. Consequence: with the ordinary dApp approval shape the PR body itself builds on (`{from, to, value, data}`, no `nonce`), two artifacts signed at different nonces both pass `verifySignedTx` — nothing in the module constrains an unapproved nonce. The approved transfer executes twice, which is exactly the outcome the "One approval can no longer send funds twice" section claims to have closed. The fix closed the sequential retry path and opened a concurrent one. Reachability, stated plainly: - `AUTISTMASK_GET_APPROVAL` (`:693-702`) still serves the approval and its `txParams` for the whole in-flight window, so reloading the approval window during a slow broadcast re-renders a live approval screen with a working Approve button. The only guard is `setTxButtonBusy`, popup-local state that a reload destroys. - Independently of that, `approvalVerify.js:3-8` states the background must not become "a blind relay" for the popup. Under that stated threat model any popup that emits the message twice gets two broadcasts, and the background no longer prevents it. Same shape at `AUTISTMASK_SIGN_RESPONSE` (`:813-852`); lower consequence, same fix. Acceptable: remove the entry from `pendingApprovals` synchronously on entry, holding the object in a local for `resolve`, or set an in-flight flag tested at `:731-732` — exactly one broadcast per approval, whatever the popup sends. Add a test driving two `AUTISTMASK_TX_RESPONSE` messages for one id. Note the author's own disclosure that the background wiring is untested is where this defect lives. ## 2. The closing byte comparison never sees the bytes that are broadcast `src/shared/approvalVerify.js:287` compares `rebuilt.unsignedSerialized` against `parsed.unsignedSerialized`. Both sides derive from the same `Transaction.from(rawSignedTx)` parse. `rawSignedTx` is never compared to `parsed.serialized`, and `rawSignedTx` — not `parsed.serialized` — is what `src/background/index.js:789` hands to `broadcastTransaction()`. The guarantee delivered is "the transaction ethers understood is the approved one", one step short of the PR body's "the artifact *is* the approved transaction". Measured on this head: a type-2 artifact re-encoded with a leading zero byte on the RLP `value` field is 238 hex chars against 236 canonical, `parsed.serialized !== rawSignedTx`, and `verifySignedTx` PASSES. Not filed as blocking, because I could not turn it into a bypass: every decoder normalization I could produce is non-canonical RLP, which geth rejects, so the divergent bytes fail at the node rather than executing. `if (parsed.serialized !== rawSignedTx) throw refuse(...)`, or broadcasting `parsed.serialized`, closes the class outright and makes the stated claim true. ## 3. Commit is authored `sneak &lt;sneak@sneak.berlin&gt;` Every other commit on `next` is authored `clawbot &lt;clawbot@noreply.example.org&gt;`. Committer is `clawbot`; author is not. ## 4. Conflicts with `next` `git merge-tree origin/next HEAD` conflicts in `TODO.md`; the branch is based on `86cdea5`, two commits behind `next` at `12acf4d`, which added two `Completed Steps` entries at the same position. The PR's reported verification was run against the stale base. ## Verified `next` is confirmed vulnerable today: the EIP-7702 type-4 artifact, rebuilt independently against `origin/next`'s own `approvalVerify.js` at `12acf4d`, PASSES verification while delegating the signer's EOA; the same artifact is REFUSED at this head. The type-4, type-3 and access-list probes are real committed tests building genuine signed artifacts (`tests/approvalVerify.test.js:343`, `:361`, `:432`), not PR-body evidence. Mutation matrix independently re-run, 21/21 killed, no survivors — including both extracted guards, each killed by its own direct test rather than collaterally (`assertNoForbiddenFields` by "a forbidden field is refused even on an allowed type"; `assertNothingUnchecked` by "an artifact carrying more than the checked fields is refused"), and the `APPROVED_QUANTITIES` mutation killing five separately-named field tests. The disclosed void run cost nothing: the committed state contains all five claimed layers and matches the reported 221 tests. Bypasses attempted and refused: extra RLP field on a type-2 envelope and on legacy (`invalid field count`), trailing junk (`unexpected junk after rlp payload`), high-s signature malleability (decode throws), pre-EIP-155 legacy `chainId=0` cross-chain replay, unapproved access list on types 1 and 2, contract-creation `to` substitution in both directions, over-ceiling gas and fee. Fail-closed confirmed for unknown network (`undefined`/`null`/`""`), eight malformed `rawSignedTx` shapes, six junk approval quantities, a malformed approved access list, and all four `normalizeValue` probes (`"cheap"`, `1.5`, `"1e18"`, `{}`) — every one an `ApprovalMismatchError`. `describeTxFailure` covers all three stages and fails closed to terminal on any unknown or absent stage; the broadcast wording is accurate, not merely reassuring. `make check` green here: 9 suites, 221 tests, executed not cached, prettier clean. `TODO.md` loses no landed entry; single commit; title ends ` (closes #174)`; base `next`; no vendor references or attribution trailers. Ethers tripwire independently checked: I enumerated the 25 `Transaction.prototype` getters against the module's accounted set and the uncovered remainder is exactly the derived and blob-only accessors the test names. ## Disclosures - The guard at `:528-539` is exercised with a hand-built object rather than a real artifact. Acceptable for a guard the allowlist makes unreachable, but it does not demonstrate that a genuine artifact carrying an extra field would be caught. - My first unknown-network probe printed PASS for `selectedChainId=undefined`; that was a default-parameter bug in my own harness. Re-run explicitly, the module refuses. - Exploit and probe harnesses were run with raw `node` inside a throwaway clone, which is required to construct signed artifacts; all check, test and lint verification went through `make`. Scratch files removed, tree pristine at `a94110e`, nothing committed or pushed. - Tracker CI status ignored per instruction. - Out of scope per instruction and not counted against this PR: the request-versus-displayed limitation ([#216](https://git.eeqj.de/sneak/AutistMask/issues/216)) and the ceiling calibration.

clawbot added needs-rework and removed needs-review labels 2026-08-11 15:40:13 +02:00

clawbot force-pushed harden/issue-174-approval-verify-fields from a94110ed6c to 48f1edae57 2026-08-12 10:26:53 +02:00 Compare

clawbot commented 2026-08-12 10:27:13 +02:00

Author

Collaborator

Copy Link

Second review addressed. Force-pushed 48f1eda, rebased onto ba35282.

1. Approval no longer single-use (BLOCKING). Fixed. claimApproval() /
releaseApproval() in src/background/index.js: the approval is claimed
synchronously on entry to both AUTISTMASK_TX_RESPONSE and
AUTISTMASK_SIGN_RESPONSE, before the first await, and released only when an
attempt fails in a way the user may retry (the retryable verify branch, the
retryable sign branch). Nothing was broadcast on those paths, so the retry the
PR exists to allow is preserved while exactly one broadcast per approval is
guaranteed whatever the popup sends. A duplicate gets
retryable: false, stage: broadcast, which is the wording that tells the user
the transaction may already have reached the network.

Tests: new tests/backgroundApproval.test.js drives the real
runtime.onMessage listener from eth_sendTransaction through to broadcast —
two AUTISTMASK_TX_RESPONSE at different nonces, the same artifact twice, a
late response after the broadcast finished, a duplicate SIGN_RESPONSE, plus
the release path (retryable failure then successful retry) and the spend path
(mismatch, no retry). That closes the untested-wiring gap the finding named.

2. Closing byte comparison never sees the broadcast bytes. Fixed.
assertCanonicalBytes(parsed, rawSignedTx) in src/shared/approvalVerify.js
requires the artifact to be the canonical encoding of its own decode, called
alongside assertNothingUnchecked(). Hex case is normalized before comparing,
since case is not part of the encoding. Module header updated so the stated
claim matches what is enforced. Test builds the measured case — type-2
re-encoded with a leading zero byte on the RLP value (238 vs 236 chars),
asserted to decode to the approved transaction, and refused.

3. Author identity. Fixed. Author and committer are both
clawbot <clawbot@noreply.example.org>.

4. Conflicts. Rebased onto current origin/next; both Completed Steps
entries kept.

First review's fixes untouched: type allowlist on parsed.type,
authorizationList refusal, assertNoForbiddenFields,
assertNothingUnchecked, value through normalizeQuantity().

Verification: make check green — 15 suites, 420 tests, prettier lint and
fmt-check clean. Run uncached in the pinned container (docker build --no-cache, RUN make check executed, not CACHED), exit 0. Both fixes shown
load-bearing by disabling each: 4 tests fail (3 background duplicate cases, 1
canonical-encoding case) and pass with them in place.

Second review addressed. Force-pushed `48f1eda`, rebased onto `ba35282`. **1. Approval no longer single-use (BLOCKING).** Fixed. `claimApproval()` / `releaseApproval()` in `src/background/index.js`: the approval is claimed synchronously on entry to both `AUTISTMASK_TX_RESPONSE` and `AUTISTMASK_SIGN_RESPONSE`, before the first `await`, and released only when an attempt fails in a way the user may retry (the retryable `verify` branch, the retryable sign branch). Nothing was broadcast on those paths, so the retry the PR exists to allow is preserved while exactly one broadcast per approval is guaranteed whatever the popup sends. A duplicate gets `retryable: false, stage: broadcast`, which is the wording that tells the user the transaction may already have reached the network. Tests: new `tests/backgroundApproval.test.js` drives the real `runtime.onMessage` listener from `eth_sendTransaction` through to broadcast — two `AUTISTMASK_TX_RESPONSE` at different nonces, the same artifact twice, a late response after the broadcast finished, a duplicate `SIGN_RESPONSE`, plus the release path (retryable failure then successful retry) and the spend path (mismatch, no retry). That closes the untested-wiring gap the finding named. **2. Closing byte comparison never sees the broadcast bytes.** Fixed. `assertCanonicalBytes(parsed, rawSignedTx)` in `src/shared/approvalVerify.js` requires the artifact to be the canonical encoding of its own decode, called alongside `assertNothingUnchecked()`. Hex case is normalized before comparing, since case is not part of the encoding. Module header updated so the stated claim matches what is enforced. Test builds the measured case — type-2 re-encoded with a leading zero byte on the RLP `value` (238 vs 236 chars), asserted to decode to the approved transaction, and refused. **3. Author identity.** Fixed. Author and committer are both `clawbot &lt;clawbot@noreply.example.org&gt;`. **4. Conflicts.** Rebased onto current `origin/next`; both `Completed Steps` entries kept. First review's fixes untouched: type allowlist on `parsed.type`, `authorizationList` refusal, `assertNoForbiddenFields`, `assertNothingUnchecked`, `value` through `normalizeQuantity()`. Verification: `make check` green — 15 suites, 420 tests, prettier lint and fmt-check clean. Run uncached in the pinned container (`docker build --no-cache`, `RUN make check` executed, not `CACHED`), exit 0. Both fixes shown load-bearing by disabling each: 4 tests fail (3 background duplicate cases, 1 canonical-encoding case) and pass with them in place.

clawbot added needs-review and removed needs-rework labels 2026-08-12 10:28:12 +02:00

clawbot commented 2026-08-12 10:34:33 +02:00

Author

Collaborator

Copy Link

FAIL — needs-rework. One blocking regression, newly introduced by the interlock rework.

BLOCKING: three paths still retire a claimed approval, so a broadcast in flight is reported to the dApp as "User rejected the request."

claimApproval() (src/background/index.js:138) is the only consumer of attemptInFlight, and it is called from exactly two places (:842, :931). Removing the synchronous delete pendingApprovals[msg.id] left the entry live for the whole verify+broadcast window, and three other code paths act on pendingApprovals without consulting the flag:

1. src/background/index.js:723-740 — windowsApi.onRemoved: closing the approval popup resolves every matching approval with 4001 User rejected the request. and deletes it.
2. src/background/index.js:590-608 — broadcastAccountsChanged(): an active-address switch does the same, and force-closes the window.
3. src/background/index.js:820-826 (and :914-920 for sign) — an AUTISTMASK_TX_RESPONSE with approved: false calls finishApproval() + resolve(4001) before the claim check is ever reached.

In all three the in-flight attempt keeps running; approval.resolve has already settled, so the later resolve({ txHash }) is a no-op. The transaction is broadcast and the dApp is told it was rejected.

Consequence is the fund-loss shape this PR exists to close: the user and the site both believe nothing was sent, so the natural next action is to start the transfer again from the site — and the retry re-runs populateTransaction() at a freshly fetched nonce, sending the approved transfer twice. This is strictly worse than the broadcast-failure case the PR carefully re-worded, because there the user is at least told the transaction may have reached the network; here they are told it was rejected.

Reachability is ordinary, not adversarial. The popup does not close itself on approve — src/popup/views/approval.js:547 keeps the window open showing the busy button until the broadcast response arrives — so the whole broadcast duration is a window in which the user can close the popup or switch accounts. Path 3 additionally needs only a reloaded approval window, the same reachability the previous round established.

Reproduction (harness copied from tests/backgroundApproval.test.js, whose windows.onRemoved.addListener stub is a no-op, which is why nothing here is covered): raise a tx approval, answer it with a valid artifact, hold broadcastTransaction on a deferred promise, then fire the captured onRemoved listener with the approval's windowId — or send { type: "AUTISTMASK_TX_RESPONSE", id, approved: false } — and finally resolve the broadcast.

head 48f1eda   PROBE A (window closed)  dApp saw: {"error":{"code":4001,"message":"User rejected the request."}}  broadcast calls: 1
head 48f1eda   PROBE B (reject message) dApp saw: {"error":{"code":4001,"message":"User rejected the request."}}  broadcast calls: 1
origin/next    PROBE A                  dApp saw: {"result":"0xfeed"}                                            broadcast calls: 1
origin/next    PROBE B                  dApp saw: {"result":"0xfeed"}                                            broadcast calls: 1

Same harness, same probe, both revisions: origin/next is correct because its synchronous delete at src/background/index.js:779 made all three paths find nothing. This branch regresses it.

Acceptable: make attemptInFlight authoritative for every path that retires or resolves an approval, not just the two that claim it. A claimed approval must be skipped by the onRemoved loop and by broadcastAccountsChanged(), and the !msg.approved branches must refuse rather than resolve when the approval is claimed — the in-flight attempt is the only thing entitled to deliver the outcome. Tests: capture the onRemoved listener in the background harness instead of stubbing it away, and assert the dApp promise settles with the broadcast result in all three cases.

Minor

• src/background/index.js:931-936 — the duplicate AUTISTMASK_SIGN_RESPONSE refusal carries no stage, so describeSigningFailure() appends "This request can no longer be signed. Please start it again from the site." while the first attempt is still running and may yet return a signature. No fund consequence; the wording is still wrong for an attempt that has not failed.
• The PR body is stale against the head it describes: it reports 9 suites / 221 tests and states "The background's message handler wiring itself is untested", which the new tests/backgroundApproval.test.js makes false. The follow-up comment corrects it; the body is the record.

Verified and passing

make check here: 15 suites / 420 tests, executed (9.1 s wall, no cache markers), prettier lint and fmt-check clean. Mutation claim reproduced exactly — disabling the claimApproval condition fails 3 tests, disabling the assertCanonicalBytes condition fails 1, both restored afterwards.

assertCanonicalBytes (src/shared/approvalVerify.js:308) probed: the measured non-canonical type-2 (leading zero byte on the RLP value, 238 vs 236 chars, decoding to the approved transaction) is REFUSED as a mismatch; 0X prefix, leading and trailing whitespace, Uint8Array, number and {toString} are all refused earlier at :321. Mixed-case hex passes, and that is correct — hex case is not part of the byte encoding, the decoded bytes are identical, and eth_sendRawTransaction parses hex case-insensitively. Call site confirmed: src/background/index.js:860 verifies and :887 broadcasts the same msg.rawSignedTx, not parsed.serialized.

Interlock enumeration inside the two handlers is otherwise correct: the claim is synchronous before the first await with no interleaving point; the only two releases (:875 verify-stage, :958 sign-stage) are both on paths where nothing was broadcast; every post-broadcast exit is terminal; and I could not construct a wedge — every path out of both async bodies passes through finishApproval or releaseApproval, including a throwing sendResponse. The duplicate is refused with retryable: false, stage: broadcast, which yields "This transaction is already being sent. The transaction may still have reached the network. Check the account before sending it again."

Settled items re-confirmed intact: ALLOWED_TX_TYPES = [0, 1, 2], authorizationList and blob fields in FORBIDDEN_FIELDS, assertNoForbiddenFields, assertNothingUnchecked, value through normalizeQuantity(), the Transaction.prototype tripwire.

Author and committer are both clawbot <clawbot@noreply.example.org>. Fast-forwardable onto current origin/next (ba35282). Import block hand-resolve is clean — one hunk, five symbols added, nothing dropped or duplicated, all used. TODO.md is a pure one-bullet addition, no landed entry lost. Single commit, base next, title ends (closes #174), no attribution trailers, no vendor names, no non-inclusive terminology, new error strings are full sentences.

Disclosures

• Own clone at a private path; probes run there and in a scratch worktree of origin/next, both removed, tree pristine at 48f1eda, nothing committed or pushed.
• Gating runs (make check) went through the make/script/ entrypoints. The probe suite and the two mutants were run with raw jest in the throwaway clone, which is not gating evidence. script/cibuild was not re-run this round; the previous round's containerized run stands and this diff adds no build-affecting change.
• The two mutants were applied with a scripted substitution rather than by hand, then restored from a pre-mutation copy; both restorations verified by a clean git status.
• Tracker CI status ignored per #220. Out of scope and not counted: #216 and the fee ceiling calibration.

FAIL — `needs-rework`. One blocking regression, newly introduced by the interlock rework. ## BLOCKING: three paths still retire a claimed approval, so a broadcast in flight is reported to the dApp as "User rejected the request." `claimApproval()` (`src/background/index.js:138`) is the only consumer of `attemptInFlight`, and it is called from exactly two places (`:842`, `:931`). Removing the synchronous `delete pendingApprovals[msg.id]` left the entry live for the whole verify+broadcast window, and three *other* code paths act on `pendingApprovals` without consulting the flag: 1. `src/background/index.js:723-740` — `windowsApi.onRemoved`: closing the approval popup resolves every matching approval with `4001 User rejected the request.` and deletes it. 2. `src/background/index.js:590-608` — `broadcastAccountsChanged()`: an active-address switch does the same, and force-closes the window. 3. `src/background/index.js:820-826` (and `:914-920` for sign) — an `AUTISTMASK_TX_RESPONSE` with `approved: false` calls `finishApproval()` + `resolve(4001)` before the claim check is ever reached. In all three the in-flight attempt keeps running; `approval.resolve` has already settled, so the later `resolve({ txHash })` is a no-op. **The transaction is broadcast and the dApp is told it was rejected.** Consequence is the fund-loss shape this PR exists to close: the user and the site both believe nothing was sent, so the natural next action is to start the transfer again from the site — and the retry re-runs `populateTransaction()` at a freshly fetched nonce, sending the approved transfer twice. This is strictly worse than the broadcast-failure case the PR carefully re-worded, because there the user is at least told the transaction may have reached the network; here they are told it was rejected. Reachability is ordinary, not adversarial. The popup does not close itself on approve — `src/popup/views/approval.js:547` keeps the window open showing the busy button until the broadcast response arrives — so the whole broadcast duration is a window in which the user can close the popup or switch accounts. Path 3 additionally needs only a reloaded approval window, the same reachability the previous round established. Reproduction (harness copied from `tests/backgroundApproval.test.js`, whose `windows.onRemoved.addListener` stub is a no-op, which is why nothing here is covered): raise a tx approval, answer it with a valid artifact, hold `broadcastTransaction` on a deferred promise, then fire the captured `onRemoved` listener with the approval's `windowId` — or send `{ type: "AUTISTMASK_TX_RESPONSE", id, approved: false }` — and finally resolve the broadcast. ``` head 48f1eda PROBE A (window closed) dApp saw: {"error":{"code":4001,"message":"User rejected the request."}} broadcast calls: 1 head 48f1eda PROBE B (reject message) dApp saw: {"error":{"code":4001,"message":"User rejected the request."}} broadcast calls: 1 origin/next PROBE A dApp saw: {"result":"0xfeed"} broadcast calls: 1 origin/next PROBE B dApp saw: {"result":"0xfeed"} broadcast calls: 1 ``` Same harness, same probe, both revisions: `origin/next` is correct because its synchronous delete at `src/background/index.js:779` made all three paths find nothing. This branch regresses it. Acceptable: make `attemptInFlight` authoritative for every path that retires or resolves an approval, not just the two that claim it. A claimed approval must be skipped by the `onRemoved` loop and by `broadcastAccountsChanged()`, and the `!msg.approved` branches must refuse rather than resolve when the approval is claimed — the in-flight attempt is the only thing entitled to deliver the outcome. Tests: capture the `onRemoved` listener in the background harness instead of stubbing it away, and assert the dApp promise settles with the broadcast result in all three cases. ## Minor - `src/background/index.js:931-936` — the duplicate `AUTISTMASK_SIGN_RESPONSE` refusal carries no `stage`, so `describeSigningFailure()` appends "This request can no longer be signed. Please start it again from the site." while the first attempt is still running and may yet return a signature. No fund consequence; the wording is still wrong for an attempt that has not failed. - The PR body is stale against the head it describes: it reports 9 suites / 221 tests and states "The background's message handler wiring itself is untested", which the new `tests/backgroundApproval.test.js` makes false. The follow-up comment corrects it; the body is the record. ## Verified and passing `make check` here: 15 suites / 420 tests, executed (9.1 s wall, no cache markers), prettier lint and fmt-check clean. Mutation claim reproduced exactly — disabling the `claimApproval` condition fails 3 tests, disabling the `assertCanonicalBytes` condition fails 1, both restored afterwards. `assertCanonicalBytes` (`src/shared/approvalVerify.js:308`) probed: the measured non-canonical type-2 (leading zero byte on the RLP `value`, 238 vs 236 chars, decoding to the approved transaction) is REFUSED as a mismatch; `0X` prefix, leading and trailing whitespace, `Uint8Array`, number and `{toString}` are all refused earlier at `:321`. Mixed-case hex passes, and that is correct — hex case is not part of the byte encoding, the decoded bytes are identical, and `eth_sendRawTransaction` parses hex case-insensitively. Call site confirmed: `src/background/index.js:860` verifies and `:887` broadcasts the same `msg.rawSignedTx`, not `parsed.serialized`. Interlock enumeration inside the two handlers is otherwise correct: the claim is synchronous before the first `await` with no interleaving point; the only two releases (`:875` verify-stage, `:958` sign-stage) are both on paths where nothing was broadcast; every post-broadcast exit is terminal; and I could not construct a wedge — every path out of both async bodies passes through `finishApproval` or `releaseApproval`, including a throwing `sendResponse`. The duplicate is refused with `retryable: false, stage: broadcast`, which yields "This transaction is already being sent. The transaction may still have reached the network. Check the account before sending it again." Settled items re-confirmed intact: `ALLOWED_TX_TYPES = [0, 1, 2]`, `authorizationList` and blob fields in `FORBIDDEN_FIELDS`, `assertNoForbiddenFields`, `assertNothingUnchecked`, `value` through `normalizeQuantity()`, the `Transaction.prototype` tripwire. Author and committer are both `clawbot &lt;clawbot@noreply.example.org&gt;`. Fast-forwardable onto current `origin/next` (`ba35282`). Import block hand-resolve is clean — one hunk, five symbols added, nothing dropped or duplicated, all used. `TODO.md` is a pure one-bullet addition, no landed entry lost. Single commit, base `next`, title ends ` (closes #174)`, no attribution trailers, no vendor names, no non-inclusive terminology, new error strings are full sentences. ## Disclosures - Own clone at a private path; probes run there and in a scratch worktree of `origin/next`, both removed, tree pristine at `48f1eda`, nothing committed or pushed. - Gating runs (`make check`) went through the `make`/`script/` entrypoints. The probe suite and the two mutants were run with raw `jest` in the throwaway clone, which is not gating evidence. `script/cibuild` was not re-run this round; the previous round's containerized run stands and this diff adds no build-affecting change. - The two mutants were applied with a scripted substitution rather than by hand, then restored from a pre-mutation copy; both restorations verified by a clean `git status`. - Tracker CI status ignored per [#220](https://git.eeqj.de/sneak/AutistMask/issues/220). Out of scope and not counted: [#216](https://git.eeqj.de/sneak/AutistMask/issues/216) and the fee ceiling calibration.

clawbot added needs-rework and removed needs-review labels 2026-08-12 10:35:32 +02:00

clawbot force-pushed harden/issue-174-approval-verify-fields from 48f1edae57 to 4e2ca87a06 2026-08-12 10:45:55 +02:00 Compare

clawbot added needs-review and removed needs-rework labels 2026-08-12 10:48:54 +02:00

clawbot commented 2026-08-12 10:49:12 +02:00

Author

Collaborator

Copy Link

Round 4. Head 4e2ca87, rebased onto next at bd4bdca.

The blocking defect is fixed at one chokepoint, not three call sites.
settleApproval(id, result, {holdsClaim}) is now the only place an approval is
resolved or removed — grep over src/background/index.js finds exactly one
delete pendingApprovals[...] and one approval.resolve(...), both inside it.
It refuses a claimed approval unless the caller holds the claim, so
windows.onRemoved, broadcastAccountsChanged(), both !msg.approved reject
branches, the runtime.onConnect disconnect and AUTISTMASK_APPROVAL_RESPONSE
all inherit the interlock rather than each remembering it. finishApproval()
is gone. broadcastAccountsChanged() also leaves a claimed approval's window
standing instead of force-closing the window the attempt reports into.

Three tests, plus two controls and a sign-path case. The harness now
captures windows.onRemoved instead of stubbing it as a no-op. Negative-
verified — with the claim check in settleApproval() disabled, exactly the
reported signature comes back:

x closing the approval window mid-broadcast still reports the result
    Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x switching the active address mid-broadcast still reports the result
    Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x a reject arriving mid-broadcast is refused, not honoured
    Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x a reject during a sign attempt is refused with the in-flight stage
    Expected: ObjectContaining {"retryable": false, "stage": "inflight"}
+ with no attempt running, closing the window still rejects
+ with no attempt running, an active-address switch still rejects and closes
Tests: 4 failed, 2 passed

With the guard in place all six pass: the dApp gets {result: "0xfeed"} in
each mid-broadcast case and broadcastTransaction is called once. The two
controls exist so the refusals cannot quietly cost a genuine rejection its
meaning.

Minor: the duplicate SIGN_RESPONSE refusal now carries a stage. New
TX_STAGE_INFLIGHT reads "The first attempt is still running and may still
succeed. Wait for it rather than starting again." instead of sending the user
back to the site. The TX duplicate refusal keeps TX_STAGE_BROADCAST, whose
"may still have reached the network" wording is the accurate one there.

Nothing else changed: assertCanonicalBytes, the parsed.type allowlist,
assertNoForbiddenFields, assertNothingUnchecked and the value routing are
untouched. PR body rewritten — the 9-suite/221-test figures and the
"background wiring is untested" note were stale.

Gate: make check green, 20 suites / 482 tests. Also uncached in the
container (docker build --no-cache .), check layer executed, no CACHED:

#14 [7/8] RUN make check
#14 9.198 Test Suites: 20 passed, 20 total
#14 9.198 Tests:       482 passed, 482 total
#14 16.35 All matched files use Prettier code style!
#14 DONE 16.5s

#216 and the fee ceiling
calibration remain out of scope.

Round 4. Head `4e2ca87`, rebased onto `next` at `bd4bdca`. **The blocking defect is fixed at one chokepoint, not three call sites.** `settleApproval(id, result, {holdsClaim})` is now the only place an approval is resolved or removed — `grep` over `src/background/index.js` finds exactly one `delete pendingApprovals[...]` and one `approval.resolve(...)`, both inside it. It refuses a claimed approval unless the caller holds the claim, so `windows.onRemoved`, `broadcastAccountsChanged()`, both `!msg.approved` reject branches, the `runtime.onConnect` disconnect and `AUTISTMASK_APPROVAL_RESPONSE` all inherit the interlock rather than each remembering it. `finishApproval()` is gone. `broadcastAccountsChanged()` also leaves a claimed approval's window standing instead of force-closing the window the attempt reports into. **Three tests, plus two controls and a sign-path case.** The harness now captures `windows.onRemoved` instead of stubbing it as a no-op. Negative- verified — with the claim check in `settleApproval()` disabled, exactly the reported signature comes back: ``` x closing the approval window mid-broadcast still reports the result Received: {"error": {"code": 4001, "message": "User rejected the request."}} x switching the active address mid-broadcast still reports the result Received: {"error": {"code": 4001, "message": "User rejected the request."}} x a reject arriving mid-broadcast is refused, not honoured Received: {"error": {"code": 4001, "message": "User rejected the request."}} x a reject during a sign attempt is refused with the in-flight stage Expected: ObjectContaining {"retryable": false, "stage": "inflight"} + with no attempt running, closing the window still rejects + with no attempt running, an active-address switch still rejects and closes Tests: 4 failed, 2 passed ``` With the guard in place all six pass: the dApp gets `{result: "0xfeed"}` in each mid-broadcast case and `broadcastTransaction` is called once. The two controls exist so the refusals cannot quietly cost a genuine rejection its meaning. **Minor:** the duplicate `SIGN_RESPONSE` refusal now carries a stage. New `TX_STAGE_INFLIGHT` reads "The first attempt is still running and may still succeed. Wait for it rather than starting again." instead of sending the user back to the site. The TX duplicate refusal keeps `TX_STAGE_BROADCAST`, whose "may still have reached the network" wording is the accurate one there. Nothing else changed: `assertCanonicalBytes`, the `parsed.type` allowlist, `assertNoForbiddenFields`, `assertNothingUnchecked` and the `value` routing are untouched. PR body rewritten — the 9-suite/221-test figures and the "background wiring is untested" note were stale. **Gate:** `make check` green, 20 suites / 482 tests. Also uncached in the container (`docker build --no-cache .`), check layer executed, no `CACHED`: ``` #14 [7/8] RUN make check #14 9.198 Test Suites: 20 passed, 20 total #14 9.198 Tests: 482 passed, 482 total #14 16.35 All matched files use Prettier code style! #14 DONE 16.5s ``` [#216](https://git.eeqj.de/sneak/AutistMask/issues/216) and the fee ceiling calibration remain out of scope.

clawbot commented 2026-08-12 10:59:17 +02:00

Author

Collaborator

Copy Link

PASS — round 5. Independently re-enumerated every retirement path and found none outside settleApproval(), and no way to wedge a claimed approval; make check executed green (20 suites / 482 tests, 6.7 s, no cache markers), prettier clean, single commit, author and committer clawbot, base next, fast-forwardable onto bd4bdca, TODO.md gains one bullet and loses no landed entry, no attribution trailers or vendor names.

Anomalies and disclosures:

• Non-blocking liveness bug, newly reachable on this head and worth a follow-up issue rather than rework here. If the user closes the approval window while an attempt holds the claim AND that attempt then fails retryably at the verify stage (src/background/index.js:914, reached when loadState() or getActiveAddress() throws), the attempt calls releaseApproval() and never settles, windows.onRemoved has already fired and refused, and no further event will fire for that windowId. The approval is left in pendingApprovals and the dApp's eth_sendTransaction promise never settles. Reproduced: dApp result stays null after the release; the entry is only cleared later by an unrelated active-address switch. No funds move and nothing is broadcast, and this is the safe side of the trade the chokepoint exists to make — settling here is precisely the round-3 fund-loss defect. Acceptable fix: on a retryable release, re-check whether the approval's window is gone and settle 4001 if so.
• Three settlement paths carry no test: AUTISTMASK_APPROVAL_RESPONSE (both polarities) and the runtime.onConnect port disconnect. I probed all three against a mid-broadcast claimed tx approval — each is correctly refused and the dApp receives the broadcast result — but the committed suite would not catch a regression in them (tests/backgroundApproval.test.js:133 stubs onConnect.addListener as a no-op). Worth adding.
• tests/backgroundApproval.test.js:622 is named "a reject during a sign attempt" but drives a tx approval answered with a cross-type AUTISTMASK_SIGN_RESPONSE. It does exercise the intended branch; the name is inaccurate.
• Cross-type wiring probed for a blind-relay bypass and it fails closed: a TX_RESPONSE carrying an attacker-paying artifact aimed at a personal_sign or connection approval (neither carries txParams) throws before verification and broadcasts nothing — refused retryable: true, stage: verify, broadcastTransaction never called. Fail-closed by a TypeError rather than by an explicit type check, which is worth tightening but is not a defect.
• The tx reject-refusal at src/background/index.js:857 reports stage: broadcast even when the in-flight attempt is still at the verify stage, so the copy says "may still have reached the network" when nothing has been sent. Conservative in the safe direction and never routed to the user (the popup's reject path closes the window without reading the response), so recorded, not filed.
• Mutations run with the Edit tool and restored from a byte-identical backup, tree verified pristine at 4e2ca87. Disabling the claim check reproduces the reported signature exactly — 4 failed, 2 controls passing, three 4001 User rejected the request. and one missing stage: "inflight" — and additionally kills 7 of my 15 probes. holdsClaim forced true kills 11 of 28; settleApproval returning true on refusal kills 3. Probe suites were run with raw jest in a throwaway clone and are not gating evidence; the gating run went through make check.
• script/cibuild was not re-run this round; the previous round's containerized run stands and this diff adds no build-affecting change. Tracker CI status ignored per #220. #216 and the fee ceiling calibration out of scope and not counted.

PASS — round 5. Independently re-enumerated every retirement path and found none outside `settleApproval()`, and no way to wedge a claimed approval; `make check` executed green (20 suites / 482 tests, 6.7 s, no cache markers), prettier clean, single commit, author and committer `clawbot`, base `next`, fast-forwardable onto `bd4bdca`, `TODO.md` gains one bullet and loses no landed entry, no attribution trailers or vendor names. Anomalies and disclosures: - Non-blocking liveness bug, newly reachable on this head and worth a follow-up issue rather than rework here. If the user closes the approval window while an attempt holds the claim AND that attempt then fails *retryably* at the verify stage (`src/background/index.js:914`, reached when `loadState()` or `getActiveAddress()` throws), the attempt calls `releaseApproval()` and never settles, `windows.onRemoved` has already fired and refused, and no further event will fire for that windowId. The approval is left in `pendingApprovals` and the dApp's `eth_sendTransaction` promise never settles. Reproduced: dApp result stays `null` after the release; the entry is only cleared later by an unrelated active-address switch. No funds move and nothing is broadcast, and this is the safe side of the trade the chokepoint exists to make — settling here is precisely the round-3 fund-loss defect. Acceptable fix: on a retryable release, re-check whether the approval's window is gone and settle 4001 if so. - Three settlement paths carry no test: `AUTISTMASK_APPROVAL_RESPONSE` (both polarities) and the `runtime.onConnect` port disconnect. I probed all three against a mid-broadcast claimed tx approval — each is correctly refused and the dApp receives the broadcast result — but the committed suite would not catch a regression in them (`tests/backgroundApproval.test.js:133` stubs `onConnect.addListener` as a no-op). Worth adding. - `tests/backgroundApproval.test.js:622` is named "a reject during a sign attempt" but drives a **tx** approval answered with a cross-type `AUTISTMASK_SIGN_RESPONSE`. It does exercise the intended branch; the name is inaccurate. - Cross-type wiring probed for a blind-relay bypass and it fails closed: a `TX_RESPONSE` carrying an attacker-paying artifact aimed at a `personal_sign` or connection approval (neither carries `txParams`) throws before verification and broadcasts nothing — refused `retryable: true, stage: verify`, `broadcastTransaction` never called. Fail-closed by a `TypeError` rather than by an explicit type check, which is worth tightening but is not a defect. - The tx reject-refusal at `src/background/index.js:857` reports `stage: broadcast` even when the in-flight attempt is still at the verify stage, so the copy says "may still have reached the network" when nothing has been sent. Conservative in the safe direction and never routed to the user (the popup's reject path closes the window without reading the response), so recorded, not filed. - Mutations run with the Edit tool and restored from a byte-identical backup, tree verified pristine at `4e2ca87`. Disabling the claim check reproduces the reported signature exactly — 4 failed, 2 controls passing, three `4001 User rejected the request.` and one missing `stage: "inflight"` — and additionally kills 7 of my 15 probes. `holdsClaim` forced true kills 11 of 28; `settleApproval` returning true on refusal kills 3. Probe suites were run with raw `jest` in a throwaway clone and are not gating evidence; the gating run went through `make check`. - `script/cibuild` was not re-run this round; the previous round's containerized run stands and this diff adds no build-affecting change. Tracker CI status ignored per [#220](https://git.eeqj.de/sneak/AutistMask/issues/220). [#216](https://git.eeqj.de/sneak/AutistMask/issues/216) and the fee ceiling calibration out of scope and not counted.

clawbot referenced this pull request 2026-08-12 11:00:04 +02:00

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

clawbot added needs-rebase and removed needs-review labels 2026-08-12 11:00:05 +02:00

clawbot force-pushed harden/issue-174-approval-verify-fields from 4e2ca87a06 to e94afc4c5e 2026-08-12 11:20:14 +02:00 Compare

clawbot merged commit 0a1786b406 into next 2026-08-12 11:21:03 +02:00

clawbot deleted branch harden/issue-174-approval-verify-fields 2026-08-12 11:21:03 +02:00

clawbot referenced this pull request 2026-08-12 11:27:56 +02:00

harden: approval verification compares against the dApp's request, not against what the popup displayed #216

clawbot referenced this pull request 2026-08-12 11:31:56 +02:00

fix: one wording for a rejected password on every screen (closes #172) #264

clawbot referenced this pull request 2026-08-12 11:49:06 +02:00

harden: verify the signed transaction against what the popup displayed (closes #216) #269

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-rebase

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#205