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

clawbot · 2026-08-12T11:49:06+02:00

clawbot commented

2026-08-12 11:49:06 +02:00

Closes #216. Option (a): populate in the background before the approval screen renders, and verify the artifact against that populated-and-displayed object.

The defect

verifySignedTx compared the artifact with the dApp's request. For every field the dApp omitted — normally nonce, gasLimit and every fee field, because populateTransaction() filled them in the popup — there was no approved value, so the comparison was skipped and the number the user read on screen was held only to the absolute ceilings. A bare 21,000-gas transfer at MAX_FEE_PER_GAS hands the validator 2.1 ETH. The ceilings were not the defect; the thing verified was not the thing approved.

Where population lives, and what happens when it fails

New src/shared/approvalTx.js. prepareApprovalTx(provider, from, txParams) runs VoidSigner(from, provider).populateTransaction() — the same sequence the popup ran, so nonce, gas, fee and chain id are populated identically — and serializes the result to exactly the fields its type serializes (SERIALIZED_FIELDS), as hex quantities, plus from. Extension messaging is JSON, which has no bigint; a field that did not survive the trip would be a field displayed and never compared, so the round trip is asserted.

The background calls it in eth_sendTransaction before requestTxApproval(). A pending approval therefore only ever exists fully populated.

On failure there is no approval and no window. The error goes back to the requesting page — where the user's click came from — for an unreachable node, a reverting estimate, a populated type outside ALLOWED_TX_TYPES, a fee or gas limit past the ceilings, or a 20-second timeout (POPULATE_TIMEOUT_MS; ethers' own request timeout is minutes long, which is not a wait anyone sits through with nothing on screen).

The cost is real and deliberate: nothing is displayed during the RPC round trip, so the wallet window appears a beat after the click, and an estimate failure is reported by the site rather than by the wallet. The alternative — open the window first, populate behind a spinner — requires a pending approval that exists before it can be displayed or signed, and a half-initialised approval record is precisely the state shape the settle interlock from #205 exists to keep out of that record. The failure also lands earlier than it used to, not later: the same estimate previously failed after the user had typed their password.

How `from` is pinned to approval time

requestTxApproval() / requestSignApproval() store approvedFrom, the address active when the approval was raised. That — not getActiveAddress() — is the expectedFrom passed to verifySignedTx() / verifySignature(). On top of that:

the signing handler refuses when the current active address is no longer approvedFrom, as an ApprovalMismatchError, so the approval is spent rather than retried;
the same guard runs after population and before the window opens, because population is a round trip during which the user's hands are free;
a request whose from is not the active address is refused up front (4100), on the transaction path and on both message-signing paths;
the popup looks up the wallet for the approved address rather than the active one, and from stays on the object it signs, so ethers' own transaction from address mismatch catches it popup-side too.

The message-signing path had the identical defect (expectedFrom read from current state, signParams.from never checked) and is fixed the same way; a permit signed by an account the approval did not name spends that account's tokens.

Verification

verifySignedTx(rawSignedTx, approvedTx, expectedFrom, selectedChainId) now compares field by field over SERIALIZED_FIELDS[parsed.type] through one comparator table, plus an exact type comparison. A quantity the approval does not fix is a refusal, not a skipped comparison — there is no "the approval did not say" branch left. A new exhaustiveness test pins the comparator table against SERIALIZED_FIELDS in both directions.

The ceilings remain, documented as a backstop: equality with the screen cannot bound what the RPC node talks the wallet into putting on the screen, so assertWithinCeilings() runs at population as well as at verification.

The #205 interlock is untouched

exactly one delete pendingApprovals[...] and one approval.resolve(...), both inside settleApproval(); every new refusal path settles through it or returns before an approval exists;
claimApproval() still sets attemptInFlight synchronously before the first await; releaseApproval() unchanged;
broadcastAccountsChanged() still skips windowsApi.remove() when the settle is refused;
ALLOWED_TX_TYPES = [0, 1, 2], the authorizationList refusal, FORBIDDEN_FIELDS, assertNoForbiddenFields, assertNothingUnchecked, assertCanonicalBytes, normalizeValue delegating to normalizeQuantity, TX_STAGE_INFLIGHT and the Transaction.prototype tripwire all unchanged and still tested;
the duplicate-response tests assert on the claim's own message, which a verification refusal does not produce, so they still discriminate the interlock rather than passing by accident now that a second artifact would also fail to verify.

One comment corrected rather than left misleading: a retry no longer re-populates at a fresh nonce, so the reason a failed broadcast stays terminal is now stated as "the node may have taken it and the page already has its outcome", not "the retry would send a second transaction". The behaviour is unchanged.

UI

The approval screen shows what it now vouches for: network, gas limit, fee per gas, maximum total fee (ETH + USD) and nonce. The popup signs the object it was given — no populateTransaction(), no provider.

Verification run

make check green on the rebased branch: 26 suites, 613 tests, test-verify-build 18 cases, prettier --check clean. make build emits both bundles with DEBUG off. New tests cover an address switch in both directions (popup signs as the new address; wallet moves under a correctly signed artifact), a switch during population, a fee and a nonce differing from the displayed value at both the module and the background-wiring level, and the population module against a stub provider (ceilings, refused types, dropped page fields, timeout, JSON round trip).

One pre-existing quirk left alone and noted here rather than fixed drive-by: a dApp's gas field is dropped by ethers' copyRequest and the wallet estimates instead, exactly as before this change.

Closes https://git.eeqj.de/sneak/AutistMask/issues/216. Option (a): populate in the background before the approval screen renders, and verify the artifact against that populated-and-displayed object. ## The defect `verifySignedTx` compared the artifact with the dApp's request. For every field the dApp omitted — normally `nonce`, `gasLimit` and every fee field, because `populateTransaction()` filled them in the popup — there was no approved value, so the comparison was skipped and the number the user read on screen was held only to the absolute ceilings. A bare 21,000-gas transfer at `MAX_FEE_PER_GAS` hands the validator 2.1 ETH. The ceilings were not the defect; the thing verified was not the thing approved. ## Where population lives, and what happens when it fails New `src/shared/approvalTx.js`. `prepareApprovalTx(provider, from, txParams)` runs `VoidSigner(from, provider).populateTransaction()` — the same sequence the popup ran, so nonce, gas, fee and chain id are populated identically — and serializes the result to exactly the fields its type serializes (`SERIALIZED_FIELDS`), as hex quantities, plus `from`. Extension messaging is JSON, which has no bigint; a field that did not survive the trip would be a field displayed and never compared, so the round trip is asserted. The background calls it in `eth_sendTransaction` **before** `requestTxApproval()`. A pending approval therefore only ever exists fully populated. **On failure there is no approval and no window.** The error goes back to the requesting page — where the user's click came from — for an unreachable node, a reverting estimate, a populated type outside `ALLOWED_TX_TYPES`, a fee or gas limit past the ceilings, or a 20-second timeout (`POPULATE_TIMEOUT_MS`; ethers' own request timeout is minutes long, which is not a wait anyone sits through with nothing on screen). The cost is real and deliberate: nothing is displayed during the RPC round trip, so the wallet window appears a beat after the click, and an estimate failure is reported by the site rather than by the wallet. The alternative — open the window first, populate behind a spinner — requires a pending approval that exists before it can be displayed or signed, and a half-initialised approval record is precisely the state shape the settle interlock from https://git.eeqj.de/sneak/AutistMask/pulls/205 exists to keep out of that record. The failure also lands *earlier* than it used to, not later: the same estimate previously failed after the user had typed their password. ## How `from` is pinned to approval time `requestTxApproval()` / `requestSignApproval()` store `approvedFrom`, the address active when the approval was raised. That — not `getActiveAddress()` — is the `expectedFrom` passed to `verifySignedTx()` / `verifySignature()`. On top of that: - the signing handler refuses when the current active address is no longer `approvedFrom`, as an `ApprovalMismatchError`, so the approval is spent rather than retried; - the same guard runs after population and before the window opens, because population is a round trip during which the user's hands are free; - a request whose `from` is not the active address is refused up front (`4100`), on the transaction path and on both message-signing paths; - the popup looks up the wallet for the approved address rather than the active one, and `from` stays on the object it signs, so ethers' own `transaction from address mismatch` catches it popup-side too. The message-signing path had the identical defect (`expectedFrom` read from current state, `signParams.from` never checked) and is fixed the same way; a permit signed by an account the approval did not name spends that account's tokens. ## Verification `verifySignedTx(rawSignedTx, approvedTx, expectedFrom, selectedChainId)` now compares field by field over `SERIALIZED_FIELDS[parsed.type]` through one comparator table, plus an exact `type` comparison. A quantity the approval does not fix is a refusal, not a skipped comparison — there is no "the approval did not say" branch left. A new exhaustiveness test pins the comparator table against `SERIALIZED_FIELDS` in both directions. The ceilings remain, documented as a **backstop**: equality with the screen cannot bound what the RPC node talks the wallet into putting *on* the screen, so `assertWithinCeilings()` runs at population as well as at verification. ## The https://git.eeqj.de/sneak/AutistMask/pulls/205 interlock is untouched - exactly one `delete pendingApprovals[...]` and one `approval.resolve(...)`, both inside `settleApproval()`; every new refusal path settles through it or returns before an approval exists; - `claimApproval()` still sets `attemptInFlight` synchronously before the first `await`; `releaseApproval()` unchanged; - `broadcastAccountsChanged()` still skips `windowsApi.remove()` when the settle is refused; - `ALLOWED_TX_TYPES = [0, 1, 2]`, the `authorizationList` refusal, `FORBIDDEN_FIELDS`, `assertNoForbiddenFields`, `assertNothingUnchecked`, `assertCanonicalBytes`, `normalizeValue` delegating to `normalizeQuantity`, `TX_STAGE_INFLIGHT` and the `Transaction.prototype` tripwire all unchanged and still tested; - the duplicate-response tests assert on the claim's own message, which a verification refusal does not produce, so they still discriminate the interlock rather than passing by accident now that a second artifact would also fail to verify. One comment corrected rather than left misleading: a retry no longer re-populates at a fresh nonce, so the reason a failed broadcast stays terminal is now stated as "the node may have taken it and the page already has its outcome", not "the retry would send a second transaction". The behaviour is unchanged. ## UI The approval screen shows what it now vouches for: network, gas limit, fee per gas, maximum total fee (ETH + USD) and nonce. The popup signs the object it was given — no `populateTransaction()`, no provider. ## Verification run `make check` green on the rebased branch: 26 suites, 613 tests, `test-verify-build` 18 cases, `prettier --check` clean. `make build` emits both bundles with `DEBUG` off. New tests cover an address switch in both directions (popup signs as the new address; wallet moves under a correctly signed artifact), a switch during population, a fee and a nonce differing from the displayed value at both the module and the background-wiring level, and the population module against a stub provider (ceilings, refused types, dropped page fields, timeout, JSON round trip). One pre-existing quirk left alone and noted here rather than fixed drive-by: a dApp's `gas` field is dropped by ethers' `copyRequest` and the wallet estimates instead, exactly as before this change.

clawbot added 1 commit 2026-08-12 11:49:07 +02:00

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

check / check (push) Successful in 36s

Details

91b36d7d5d

The signed artifact was compared with the dApp's request object, so every
field the dApp left out — normally the nonce, the gas limit and every fee
field, because populateTransaction() filled them in the popup — was checked
by nothing but the absolute ceilings. A bare transfer at the fee ceiling
hands the validator 2.1 ETH. The ceilings were never the defect: the thing
being verified was not the thing the user approved.

The transaction is now populated in the background, before the approval
window opens, and that populated object is what is displayed, what the popup
signs, and what the artifact is verified against. Every consequential field
is compared exactly.

- src/shared/approvalTx.js populates the request through a VoidSigner over
  the configured RPC and serializes the result to the fields its type
  serializes, as hex quantities that survive the JSON messaging boundary.
  Fields the wallet does not act on are dropped before ethers sees the
  page's object.
- Population failure raises no approval and opens no window: the error goes
  back to the requesting page, bounded by a 20-second timeout. A
  half-initialised approval record would be exactly the state the settle
  interlock exists to keep out of that record, and the same estimate
  previously failed after the user had typed their password.
- verifySignedTx compares the artifact field by field over
  SERIALIZED_FIELDS[type], plus the type itself. A quantity the approval
  does not fix is a refusal rather than a skipped comparison. The ceilings
  stay as a documented backstop and now also apply at population, where they
  bound what an RPC node can talk the wallet into displaying.
- The approval pins the address it was raised for. Verification uses that
  address, not getActiveAddress(), and an address switch between approval and
  signing refuses rather than signing from an account the screen never named
  — including a switch during population, and on the message-signing path. A
  request naming an address that is not the active one is refused outright.
- The approval screen shows the network, gas limit, fee per gas, maximum fee
  and nonce it now vouches for, and the popup signs the object it was given
  with no provider and no population of its own.

The settle chokepoint is untouched: one delete of pendingApprovals and one
approval.resolve(), both inside settleApproval(), the claim taken
synchronously before the first await, and a refused settle still leaving the
approval window standing.

clawbot added the needs-review label 2026-08-12 11:49:12 +02:00

clawbot self-assigned this 2026-08-12 11:49:13 +02:00

clawbot referenced this pull request

2026-08-12 11:49:24 +02:00

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

clawbot commented

2026-08-12 12:04:11 +02:00

FAIL — `needs-rebase`

The change itself passes adversarial review. One blocking mechanical defect.

1. `TODO.md` — conflicts with current `origin/next`

Where: TODO.md, first bullet under # Completed Steps.
What: the branch is based on 5af89a1; origin/next is now 18b47cd ("test: close the empty-batch hole in the e2e unstubbed-request guard (closes #187)"), which added its own bullet at the top of the same section. git rebase origin/next on 91b36d7 stops with CONFLICT (content): Merge conflict in TODO.md. Nothing else conflicts — the source and test files rebase cleanly.
Why it matters: the branch is not fast-forwardable onto the current base, so it cannot land as-is.
Acceptable: rebase onto current origin/next, keeping BOTH bullets — the #216 bullet first, the #187 bullet immediately after it — and re-run make fmt so the wrapped prose stays prettier-clean. No landed entry may be dropped.

Verified and passing (adversarial, by mutation and probe, not by reading)

Per-field pinning: patched assertFieldMatches to skip one key at a time via an env switch and ran the full suite for each of the ten comparators. Every single one is individually pinned — chainId 2 fail, nonce 6, gasLimit 3, gasPrice 1, maxFeePerGas 6, maxPriorityFeePerGas 2, to 6, value 2, data 1, accessList 2. No field can be dropped with the suite green. APPROVED_FIELDS is pinned bidirectionally against SERIALIZED_FIELDS, and the Transaction.prototype getter tripwire still binds.
A verification refusal CANNOT shadow a broken interlock. Neutered claimApproval() (src/background/index.js:167); three tests die, and the decisive one is "the same artifact sent twice broadcasts once", where the two artifacts are byte-identical and both verify. It fails on broadcastTransaction called 2 times, not on any verification message. The claim is the only thing standing between one approval and two broadcasts, and the suite proves it.
from pinning: disabling the signing-handler switch guard turns the suite red (2 tests). Both directions of an address switch are covered, plus a switch during population, plus the message-signing path, plus the up-front 4100 refusals.
Serialization across the messaging boundary: probed serializeApprovedTx -> JSON round trip -> the popup's exact signTransaction({...approvedTx}) -> verifySignedTx, for nonce 0, gas limit at MAX_GAS_LIMIT, fee at MAX_FEE_PER_GAS, maxPriorityFeePerGas 0, absent vs zero value, absent vs "0x" data, contract creation, type 0, type 1 with a populated access list, type 2 with []. All fourteen round-trip and verify. Tampering the fee, the nonce or the access list on the signed side is refused with the right message.
No orphan on the new failure path: population touches no window and no pendingApprovals entry, so every failure between the request and requestTxApproval() returns a single {error} to the page. The timeout races populateTransaction() and clears its timer in finally; a provider that resolves late has nothing to resolve into.
The #205 chokepoint is byte-for-byte as claimed; popup does no populateTransaction, no getProvider, no re-estimation; ceilings kept and documented as a backstop and applied at population; script/check green (26 suites, 613 tests, test-verify-build 18 cases, prettier --check clean, exit 0, all executed, no cached markers); make test-e2e green (27/27, exit 0); single commit, author and committer clawbot, title ends (closes #216), base next, no attribution trailers, no competitor named.

Noted, not counted against this PR

The nonce is now fixed at request time, not at Confirm time. The popup used to call populateTransaction() when the user clicked Confirm, so a second transaction picked up a fresh nonce. Now both are populated before either window opens, so two concurrent eth_sendTransaction calls — or one approval window left open while another transaction goes out — are populated at the same nonce, and the second broadcast fails terminally with the "may still have reached the network" wording. This is inherent to option (a) and fails closed, but it is a behaviour change the PR body does not mention. Worth a follow-up issue rather than a rework.
return { error: { message: e.message } } at src/background/index.js:625 and :658 carries no EIP-1193 code. That matches the existing shape at :308, :507, :556, :591, :662 in the same file, so it is left alone.
Flipping expectedFrom back to getActiveAddress() while leaving the handler's switch guard in place does NOT turn the suite red — the guard has already established the two are equal at that point, so no test can distinguish them. Disclosed because it was asked for specifically; it is layering, not a hole. The suite goes red as soon as both are removed.
Known and excluded per instructions: #207 (gas dropped by copyRequest), #262 (orphaned-approval liveness), #220 (tracker CI status).

## FAIL — `needs-rebase` The change itself passes adversarial review. One blocking mechanical defect. ### 1. `TODO.md` — conflicts with current `origin/next` - **Where**: `TODO.md`, first bullet under `# Completed Steps`. - **What**: the branch is based on `5af89a1`; `origin/next` is now `18b47cd` ("test: close the empty-batch hole in the e2e unstubbed-request guard (closes #187)"), which added its own bullet at the top of the same section. `git rebase origin/next` on `91b36d7` stops with `CONFLICT (content): Merge conflict in TODO.md`. Nothing else conflicts — the source and test files rebase cleanly. - **Why it matters**: the branch is not fast-forwardable onto the current base, so it cannot land as-is. - **Acceptable**: rebase onto current `origin/next`, keeping BOTH bullets — the `#216` bullet first, the `#187` bullet immediately after it — and re-run `make fmt` so the wrapped prose stays prettier-clean. No landed entry may be dropped. ### Verified and passing (adversarial, by mutation and probe, not by reading) - **Per-field pinning**: patched `assertFieldMatches` to skip one key at a time via an env switch and ran the full suite for each of the ten comparators. Every single one is individually pinned — `chainId` 2 fail, `nonce` 6, `gasLimit` 3, `gasPrice` 1, `maxFeePerGas` 6, `maxPriorityFeePerGas` 2, `to` 6, `value` 2, `data` 1, `accessList` 2. No field can be dropped with the suite green. `APPROVED_FIELDS` is pinned bidirectionally against `SERIALIZED_FIELDS`, and the `Transaction.prototype` getter tripwire still binds. - **A verification refusal CANNOT shadow a broken interlock.** Neutered `claimApproval()` (`src/background/index.js:167`); three tests die, and the decisive one is "the same artifact sent twice broadcasts once", where the two artifacts are byte-identical and both verify. It fails on `broadcastTransaction` called 2 times, not on any verification message. The claim is the only thing standing between one approval and two broadcasts, and the suite proves it. - **`from` pinning**: disabling the signing-handler switch guard turns the suite red (2 tests). Both directions of an address switch are covered, plus a switch during population, plus the message-signing path, plus the up-front `4100` refusals. - **Serialization across the messaging boundary**: probed `serializeApprovedTx` -> `JSON` round trip -> the popup's exact `signTransaction({...approvedTx})` -> `verifySignedTx`, for nonce 0, gas limit at `MAX_GAS_LIMIT`, fee at `MAX_FEE_PER_GAS`, `maxPriorityFeePerGas` 0, absent vs zero `value`, absent vs `"0x"` `data`, contract creation, type 0, type 1 with a populated access list, type 2 with `[]`. All fourteen round-trip and verify. Tampering the fee, the nonce or the access list on the signed side is refused with the right message. - **No orphan on the new failure path**: population touches no window and no `pendingApprovals` entry, so every failure between the request and `requestTxApproval()` returns a single `{error}` to the page. The timeout races `populateTransaction()` and clears its timer in `finally`; a provider that resolves late has nothing to resolve into. - The `#205` chokepoint is byte-for-byte as claimed; popup does no `populateTransaction`, no `getProvider`, no re-estimation; ceilings kept and documented as a backstop and applied at population; `script/check` green (26 suites, 613 tests, `test-verify-build` 18 cases, `prettier --check` clean, exit 0, all executed, no cached markers); `make test-e2e` green (27/27, exit 0); single commit, author and committer `clawbot`, title ends ` (closes #216)`, base `next`, no attribution trailers, no competitor named. ### Noted, not counted against this PR - **The nonce is now fixed at request time, not at Confirm time.** The popup used to call `populateTransaction()` when the user clicked Confirm, so a second transaction picked up a fresh nonce. Now both are populated before either window opens, so two concurrent `eth_sendTransaction` calls — or one approval window left open while another transaction goes out — are populated at the same nonce, and the second broadcast fails terminally with the "may still have reached the network" wording. This is inherent to option (a) and fails closed, but it is a behaviour change the PR body does not mention. Worth a follow-up issue rather than a rework. - `return { error: { message: e.message } }` at `src/background/index.js:625` and `:658` carries no EIP-1193 `code`. That matches the existing shape at `:308`, `:507`, `:556`, `:591`, `:662` in the same file, so it is left alone. - Flipping `expectedFrom` back to `getActiveAddress()` while leaving the handler's switch guard in place does NOT turn the suite red — the guard has already established the two are equal at that point, so no test can distinguish them. Disclosed because it was asked for specifically; it is layering, not a hole. The suite goes red as soon as both are removed. - Known and excluded per instructions: https://git.eeqj.de/sneak/AutistMask/issues/207 (`gas` dropped by `copyRequest`), https://git.eeqj.de/sneak/AutistMask/issues/262 (orphaned-approval liveness), https://git.eeqj.de/sneak/AutistMask/issues/220 (tracker CI status).

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

clawbot referenced this pull request

2026-08-12 12:04:57 +02:00

fix: two concurrent dApp transactions are populated with the same nonce, and the second fails terminally #271

clawbot force-pushed harden/issue-216-verify-displayed-values from 91b36d7d5d to 993afbc5da

2026-08-12 12:11:48 +02:00

Compare

clawbot merged commit 918e581ef3 into next

2026-08-12 12:15:26 +02:00

clawbot deleted branch harden/issue-216-verify-displayed-values

2026-08-12 12:15:26 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#269