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

security: decrypt and sign dApp approvals in the popup (closes #157) #171

Merged
clawbot merged 1 commits from fix/issue-157-approval-decrypt-in-popup into next 2026-08-10 15:59:30 +02:00
Conversation 5 Commits 1 Files Changed 5
+712 -123
clawbot commented 2026-08-09 04:52:04 +02:00
Collaborator
Copy Link
Copy Source

Closes #157.

What was wrong

The dApp-initiated transaction and signature approval paths read the user's
password in the popup and then sent it, in plaintext, to the background over
runtime.sendMessage, which did the decryption and the signing. Four standing
TODO(security) markers recorded it.

What crosses the boundary now, and why that is safe

message before after
AUTISTMASK_TX_RESPONSE password rawSignedTx
AUTISTMASK_SIGN_RESPONSE password signature
  • rawSignedTx is the RLP-serialized, already-signed transaction. It is
    exactly the value that goes out over eth_sendRawTransaction a moment later
    and is public from that point on. It is not a secret, and possessing it does
    not let anyone sign anything else.
  • signature is the 65-byte signature the dApp receives as the result of its
    own request. Same argument.
  • Neither the password, the recovery phrase, the xprv, nor the private key
    leaves the popup context. The obvious wrong turn for this issue — swapping
    the password for the decrypted key — is not what happened here; sending the
    key would have been strictly worse than the status quo, because the key is
    reusable and the signed artifact is not.

The popup now looks like src/popup/views/confirmTx.js:305, which already
decrypted locally, so all three signing paths in the extension have the same
shape.

Changes

src/popup/views/approval.js

  • Stashes the txParams / signParams it renders, so it signs precisely what
    it displayed. Both are repopulated by show(), so close/reopen still works.
  • On approve: finds the wallet and address index owning state.activeAddress,
    calls decryptWithPassword, then getSignerForAddress.
  • Tx path: populateTransaction then signTransaction. This is the same
    sequence ethers' own AbstractSigner.sendTransaction runs internally
    (populate, delete pop.from, sign, broadcast), so nonce, gas, fee and chain
    id population are byte-for-byte what the background used to produce.
  • Sign path: signMessage(getBytes(sp.message)) for
    personal_sign/eth_sign, signTypedData(domain, types, message) for
    eth_signTypedData_v4/eth_signTypedData — moved over unchanged.
  • Password and decrypted secret references are nulled in finally blocks
    immediately after use, carrying the same immutability caveat comment
    confirmTx.js already has. Neither is captured in any closure: the
    sendMessage callback closes over payload and the view state only.

src/background/index.js

  • AUTISTMASK_TX_RESPONSE: provider.broadcastTransaction(msg.rawSignedTx),
    then resolve the approval and respond with the hash.
  • AUTISTMASK_SIGN_RESPONSE: resolve the approval with msg.signature.
  • Drops the now-unused decryptWithPassword, getSignerForAddress and
    getBytes imports. The string password no longer appears anywhere in
    src/background/.

src/shared/approvalVerify.js (new)

Moving the secret out of the background must not turn the background into a
blind relay that broadcasts whatever an extension page hands it, so before it
acts it re-derives the signer from the artifact and checks it against the
approval it is holding:

  • verifySignedTx(rawSignedTx, txParams, expectedFrom) — parses with
    Transaction.from, recovers the sender from the signature, and asserts the
    recovered from plus to, value and data match the approval.
  • verifySignature(signParams, signature, expectedFrom) — recovers with
    verifyMessage / verifyTypedData and asserts it is the approved address.

All crypto is delegated to ethers, per the Crypto Policy. Every rejection
message is a full sentence. This is what keeps the change security-neutral on
the background side rather than a transfer of trust.

Behaviour preserved

  • Rejection still returns EIP-1193 4001. The !msg.approved branches and
    the windowsApi.onRemoved handler are untouched.
  • Approvals still survive popup close/reopen. runtime.onConnect's
    disconnect handler still keeps tx/sign approvals pending, and show()
    re-fetches and re-stashes the params on reopen.
  • Signing failures still reach the dApp. When the popup cannot produce an
    artifact it sends the same message with an error field instead, so the
    background resolves the request with a failure exactly as it used to. Without
    this the page would have hung until the window was closed.
  • Wrong password now fails better than the DoD asks. It is caught in the
    popup before any message is sent: a full-sentence inline error
    ("That password is incorrect. Please try again."), the button re-enables, and
    the pending approval is untouched and retryable. Previously the bad password
    reached the background, which resolved and deleted the approval, so the
    request was dead.

Verification

make check — green (75 tests, 5 suites; 3 suites and 62 tests were already
there, this adds tests/approvalVerify.test.js). make build — green, both
dist/chrome/ and dist/firefox/ produced.

tests/approvalVerify.test.js covers the verifiers directly (tampered
recipient, inflated value, substituted call data, wrong signer, unsigned
payload, malformed payload, contract creation with no recipient, absent value,
case-differing call data, non-mutation of the approved typed data) and adds a
round trip that runs the exact sequence the popup runs — getSignerForAddress,
connect, populateTransaction, delete pop.from, signTransaction —
against a stub provider, then hands the artifact to the exact check the
background runs. That test also asserts the wire payload has only
{type, id, approved, rawSignedTx} and contains neither the string password
nor the private key.

Grep evidence (DoD item 1)

No runtime.sendMessage payload anywhere carries a password field:

$ grep -rn -A8 "runtime.sendMessage" src/ --include='*.js' | grep -i password
$ echo $?
1

The word does not occur in the background or content script at all any more:

$ grep -rn "password" src/background/ src/content/
$ echo $?
1

And the markers are gone (DoD item 6):

$ grep -rn "TODO(security)" src/
$ echo $?
1

Logging (DoD item 8)

Argued from the code path, not from the value of DEBUG, since the #145
runtime toggle can raise the level at runtime and isDebug() is evaluated
lazily on every emit():

  • The password no longer exists in the background context at all, so no
    background log statement can reach it regardless of level.
  • In the popup it exists only as a local let in the two click handlers, and
    its only use is as an argument to decryptWithPassword. src/shared/vault.js
    imports no logger and calls no console.
  • src/popup/views/approval.js and src/shared/approvalVerify.js import
    neither log nor debugFetch:
$ grep -rn "log\.\|debugFetch" src/popup/views/approval.js src/shared/vault.js src/shared/approvalVerify.js
$ echo $?
1
  • debugFetch is reachable only from proxyRpc and the price/balance/phishing
    fetchers. Its debug lines log method, url and opts.body. The only body
    it is ever given is the JSON-RPC envelope built in proxyRpc, and the popup
    never routes a password through any fetch. The ethers provider does its own
    fetch and does not go through debugFetch at all; what it would carry is
    the raw signed transaction, which is public.
  • The error strings surfaced on the wrong-password path are fixed literals; the
    entered value is never interpolated into a message.

What I could not verify, and needs a human pass

I want to be explicit rather than claim a green tick I did not earn:

  • Firefox: blocked on #153, as expected. That issue documents the Firefox
    target being non-functional for exactly these approval paths (Chrome callback
    APIs against the promise-only browser namespace). Not touched here; folding
    it in would make this unreviewable.
  • Chrome: not verified interactively. There is no browser in the
    environment this was built in, so I could not load the unpacked extension and
    drive a live dApp through eth_sendTransaction, personal_sign and
    eth_signTypedData_v4. What I did verify is that the Chrome bundle builds,
    that the popup signing sequence produces an artifact the background accepts
    (round-trip test above), and that the message contract on both ends matches.
    DoD items 2, 3, 4 and 5 want an end-to-end run, so please do the interactive
    Chrome pass on review rather than taking the build as proof.

Notes for the reviewer

One deliberate, documented behaviour change beyond the wrong-password
improvement: if a dApp ever supplied an ENS name as to, populateTransaction
would resolve it in the popup and the background's to comparison would then
fail closed with "The signed transaction does not go to the approved
recipient." EIP-1193 requires to to be an address and the approval UI already
renders it as one, so I judged a loud, safe failure better than a silent
unverified resolution. Say the word if you would rather that case be tolerated.

Out of scope

  • #153 (Firefox), per the above.
  • The "Wrong password." fragment in confirmTx.js is not a full sentence and
    so diverges from the language rule in RULES.md. Left alone as unrelated;
    happy to file it as its own issue.
Closes #157. ## What was wrong The dApp-initiated transaction and signature approval paths read the user's password in the popup and then sent it, in plaintext, to the background over `runtime.sendMessage`, which did the decryption and the signing. Four standing `TODO(security)` markers recorded it. ## What crosses the boundary now, and why that is safe | message | before | after | | --- | --- | --- | | `AUTISTMASK_TX_RESPONSE` | `password` | `rawSignedTx` | | `AUTISTMASK_SIGN_RESPONSE` | `password` | `signature` | - `rawSignedTx` is the RLP-serialized, already-signed transaction. It is exactly the value that goes out over `eth_sendRawTransaction` a moment later and is public from that point on. It is not a secret, and possessing it does not let anyone sign anything else. - `signature` is the 65-byte signature the dApp receives as the result of its own request. Same argument. - Neither the password, the recovery phrase, the xprv, nor the private key leaves the popup context. The obvious wrong turn for this issue — swapping the password for the decrypted key — is not what happened here; sending the key would have been strictly worse than the status quo, because the key is reusable and the signed artifact is not. The popup now looks like `src/popup/views/confirmTx.js:305`, which already decrypted locally, so all three signing paths in the extension have the same shape. ## Changes **`src/popup/views/approval.js`** - Stashes the `txParams` / `signParams` it renders, so it signs precisely what it displayed. Both are repopulated by `show()`, so close/reopen still works. - On approve: finds the wallet and address index owning `state.activeAddress`, calls `decryptWithPassword`, then `getSignerForAddress`. - Tx path: `populateTransaction` then `signTransaction`. This is the same sequence `ethers`' own `AbstractSigner.sendTransaction` runs internally (populate, `delete pop.from`, sign, broadcast), so nonce, gas, fee and chain id population are byte-for-byte what the background used to produce. - Sign path: `signMessage(getBytes(sp.message))` for `personal_sign`/`eth_sign`, `signTypedData(domain, types, message)` for `eth_signTypedData_v4`/`eth_signTypedData` — moved over unchanged. - Password and decrypted secret references are nulled in `finally` blocks immediately after use, carrying the same immutability caveat comment `confirmTx.js` already has. Neither is captured in any closure: the `sendMessage` callback closes over `payload` and the view state only. **`src/background/index.js`** - `AUTISTMASK_TX_RESPONSE`: `provider.broadcastTransaction(msg.rawSignedTx)`, then resolve the approval and respond with the hash. - `AUTISTMASK_SIGN_RESPONSE`: resolve the approval with `msg.signature`. - Drops the now-unused `decryptWithPassword`, `getSignerForAddress` and `getBytes` imports. The string `password` no longer appears anywhere in `src/background/`. **`src/shared/approvalVerify.js` (new)** Moving the secret out of the background must not turn the background into a blind relay that broadcasts whatever an extension page hands it, so before it acts it re-derives the signer from the artifact and checks it against the approval it is holding: - `verifySignedTx(rawSignedTx, txParams, expectedFrom)` — parses with `Transaction.from`, recovers the sender from the signature, and asserts the recovered `from` plus `to`, `value` and `data` match the approval. - `verifySignature(signParams, signature, expectedFrom)` — recovers with `verifyMessage` / `verifyTypedData` and asserts it is the approved address. All crypto is delegated to `ethers`, per the Crypto Policy. Every rejection message is a full sentence. This is what keeps the change security-neutral on the background side rather than a transfer of trust. ## Behaviour preserved - **Rejection still returns EIP-1193 4001.** The `!msg.approved` branches and the `windowsApi.onRemoved` handler are untouched. - **Approvals still survive popup close/reopen.** `runtime.onConnect`'s disconnect handler still keeps `tx`/`sign` approvals pending, and `show()` re-fetches and re-stashes the params on reopen. - **Signing failures still reach the dApp.** When the popup cannot produce an artifact it sends the same message with an `error` field instead, so the background resolves the request with a failure exactly as it used to. Without this the page would have hung until the window was closed. - **Wrong password now fails better than the DoD asks.** It is caught in the popup before any message is sent: a full-sentence inline error ("That password is incorrect. Please try again."), the button re-enables, and the pending approval is untouched and retryable. Previously the bad password reached the background, which resolved and deleted the approval, so the request was dead. ## Verification `make check` — green (75 tests, 5 suites; 3 suites and 62 tests were already there, this adds `tests/approvalVerify.test.js`). `make build` — green, both `dist/chrome/` and `dist/firefox/` produced. `tests/approvalVerify.test.js` covers the verifiers directly (tampered recipient, inflated value, substituted call data, wrong signer, unsigned payload, malformed payload, contract creation with no recipient, absent value, case-differing call data, non-mutation of the approved typed data) and adds a round trip that runs the exact sequence the popup runs — `getSignerForAddress`, `connect`, `populateTransaction`, `delete pop.from`, `signTransaction` — against a stub provider, then hands the artifact to the exact check the background runs. That test also asserts the wire payload has only `{type, id, approved, rawSignedTx}` and contains neither the string `password` nor the private key. ### Grep evidence (DoD item 1) No `runtime.sendMessage` payload anywhere carries a password field: ``` $ grep -rn -A8 "runtime.sendMessage" src/ --include='*.js' | grep -i password $ echo $? 1 ``` The word does not occur in the background or content script at all any more: ``` $ grep -rn "password" src/background/ src/content/ $ echo $? 1 ``` And the markers are gone (DoD item 6): ``` $ grep -rn "TODO(security)" src/ $ echo $? 1 ``` ### Logging (DoD item 8) Argued from the code path, not from the value of `DEBUG`, since the #145 runtime toggle can raise the level at runtime and `isDebug()` is evaluated lazily on every `emit()`: - The password no longer exists in the background context at all, so no background log statement can reach it regardless of level. - In the popup it exists only as a local `let` in the two click handlers, and its only use is as an argument to `decryptWithPassword`. `src/shared/vault.js` imports no logger and calls no `console`. - `src/popup/views/approval.js` and `src/shared/approvalVerify.js` import neither `log` nor `debugFetch`: ``` $ grep -rn "log\.\|debugFetch" src/popup/views/approval.js src/shared/vault.js src/shared/approvalVerify.js $ echo $? 1 ``` - `debugFetch` is reachable only from `proxyRpc` and the price/balance/phishing fetchers. Its debug lines log `method`, `url` and `opts.body`. The only body it is ever given is the JSON-RPC envelope built in `proxyRpc`, and the popup never routes a password through any fetch. The `ethers` provider does its own `fetch` and does not go through `debugFetch` at all; what it would carry is the raw signed transaction, which is public. - The error strings surfaced on the wrong-password path are fixed literals; the entered value is never interpolated into a message. ### What I could not verify, and needs a human pass I want to be explicit rather than claim a green tick I did not earn: - **Firefox: blocked on #153**, as expected. That issue documents the Firefox target being non-functional for exactly these approval paths (Chrome callback APIs against the promise-only `browser` namespace). Not touched here; folding it in would make this unreviewable. - **Chrome: not verified interactively.** There is no browser in the environment this was built in, so I could not load the unpacked extension and drive a live dApp through `eth_sendTransaction`, `personal_sign` and `eth_signTypedData_v4`. What I did verify is that the Chrome bundle builds, that the popup signing sequence produces an artifact the background accepts (round-trip test above), and that the message contract on both ends matches. DoD items 2, 3, 4 and 5 want an end-to-end run, so please do the interactive Chrome pass on review rather than taking the build as proof. ## Notes for the reviewer One deliberate, documented behaviour change beyond the wrong-password improvement: if a dApp ever supplied an ENS name as `to`, `populateTransaction` would resolve it in the popup and the background's `to` comparison would then fail closed with "The signed transaction does not go to the approved recipient." EIP-1193 requires `to` to be an address and the approval UI already renders it as one, so I judged a loud, safe failure better than a silent unverified resolution. Say the word if you would rather that case be tolerated. ## Out of scope - #153 (Firefox), per the above. - The `"Wrong password."` fragment in `confirmTx.js` is not a full sentence and so diverges from the language rule in `RULES.md`. Left alone as unrelated; happy to file it as its own issue.
clawbot added the needs-review label 2026-08-09 04:52:04 +02:00
clawbot self-assigned this 2026-08-09 04:52:09 +02:00
clawbot commented 2026-08-09 04:52:27 +02:00
Author
Collaborator
Copy Link
Copy Source

Summary of what was built and how it was verified

Built

Five files, +712/-123.

  • src/popup/views/approval.js — the two dApp approval handlers now decrypt
    with decryptWithPassword and sign with getSignerForAddress in the popup,
    exactly as confirmTx.js already did. They keep the rendered txParams /
    signParams so they sign what was displayed, and null the password and the
    decrypted secret in finally blocks straight after use.
  • src/background/index.js — reduced to broadcast and approval resolution.
    provider.broadcastTransaction(msg.rawSignedTx) for the tx path,
    approval.resolve({ signature }) for the sign path. The unused
    decryptWithPassword, getSignerForAddress and getBytes imports are gone.
  • src/shared/approvalVerify.js (new) — verifySignedTx and
    verifySignature, so the background re-derives the signer from the artifact
    and checks it against the approval it holds before acting on it. Without this
    the change would have been a trust transfer rather than a fix.
  • tests/approvalVerify.test.js (new) — 13 unit tests plus a 3-test round trip.
  • TODO.md — one entry at the top of Completed Steps, deliberately kept to a
    single hunk because PR #169 also touches this file.

All four TODO(security) markers removed, in the same commit as the fix.

What crosses the boundary

rawSignedTx (RLP-serialized signed transaction) and signature. Both are
public artifacts the moment they leave the wallet, and neither can be reused to
sign anything else. The password, recovery phrase, xprv and private key all
stay in the popup context. Sending the decrypted key instead of the password
would have been strictly worse and is explicitly not what this does.

Verified

  • make check green: 5 suites, 75 tests. make build green for both targets.
    The pre-commit hook ran make check on the commit as well.
  • Round-trip test runs the real popup sequence — getSignerForAddress,
    connect, populateTransaction, delete pop.from, signTransaction — and
    feeds the result to the real background check, asserting nonce, chain id,
    gas limit, recipient, value and call data all survive. It also asserts the
    wire payload is exactly {type, id, approved, rawSignedTx} and contains
    neither the string password nor the private key.
  • Tamper cases rejected: swapped recipient, inflated value, substituted call
    data, wrong signer, unsigned payload, undecodable payload.
  • Greps in the PR body show no runtime.sendMessage payload carries a password
    field, that password no longer appears in src/background/ or
    src/content/ at all, and that the TODO(security) markers are gone.
  • Logging argued from the code path rather than the DEBUG flag, since
    isDebug() is evaluated lazily on every emit and the #145 runtime toggle can
    raise the level at any time.
  • Behaviour preserved: rejection still resolves EIP-1193 4001; approvals still
    survive popup close/reopen; a popup-side signing failure still reports back
    so the page gets an error instead of hanging.

Not verified — please cover on review

  • Chrome, interactively. No browser exists in the build environment, so I
    could not load the unpacked extension and drive a live dApp. The bundle
    builds and the message contract is covered by tests, but DoD items 2 through
    5 want a real end-to-end run.
  • Firefox. Blocked on #153 as expected, and deliberately not touched here.
## Summary of what was built and how it was verified ### Built Five files, +712/-123. - `src/popup/views/approval.js` — the two dApp approval handlers now decrypt with `decryptWithPassword` and sign with `getSignerForAddress` in the popup, exactly as `confirmTx.js` already did. They keep the rendered `txParams` / `signParams` so they sign what was displayed, and null the password and the decrypted secret in `finally` blocks straight after use. - `src/background/index.js` — reduced to broadcast and approval resolution. `provider.broadcastTransaction(msg.rawSignedTx)` for the tx path, `approval.resolve({ signature })` for the sign path. The unused `decryptWithPassword`, `getSignerForAddress` and `getBytes` imports are gone. - `src/shared/approvalVerify.js` (new) — `verifySignedTx` and `verifySignature`, so the background re-derives the signer from the artifact and checks it against the approval it holds before acting on it. Without this the change would have been a trust transfer rather than a fix. - `tests/approvalVerify.test.js` (new) — 13 unit tests plus a 3-test round trip. - `TODO.md` — one entry at the top of Completed Steps, deliberately kept to a single hunk because PR #169 also touches this file. All four `TODO(security)` markers removed, in the same commit as the fix. ### What crosses the boundary `rawSignedTx` (RLP-serialized signed transaction) and `signature`. Both are public artifacts the moment they leave the wallet, and neither can be reused to sign anything else. The password, recovery phrase, xprv and private key all stay in the popup context. Sending the decrypted key instead of the password would have been strictly worse and is explicitly not what this does. ### Verified - `make check` green: 5 suites, 75 tests. `make build` green for both targets. The pre-commit hook ran `make check` on the commit as well. - Round-trip test runs the real popup sequence — `getSignerForAddress`, `connect`, `populateTransaction`, `delete pop.from`, `signTransaction` — and feeds the result to the real background check, asserting nonce, chain id, gas limit, recipient, value and call data all survive. It also asserts the wire payload is exactly `{type, id, approved, rawSignedTx}` and contains neither the string `password` nor the private key. - Tamper cases rejected: swapped recipient, inflated value, substituted call data, wrong signer, unsigned payload, undecodable payload. - Greps in the PR body show no `runtime.sendMessage` payload carries a password field, that `password` no longer appears in `src/background/` or `src/content/` at all, and that the `TODO(security)` markers are gone. - Logging argued from the code path rather than the `DEBUG` flag, since `isDebug()` is evaluated lazily on every emit and the #145 runtime toggle can raise the level at any time. - Behaviour preserved: rejection still resolves EIP-1193 4001; approvals still survive popup close/reopen; a popup-side signing failure still reports back so the page gets an error instead of hanging. ### Not verified — please cover on review - **Chrome, interactively.** No browser exists in the build environment, so I could not load the unpacked extension and drive a live dApp. The bundle builds and the message contract is covered by tests, but DoD items 2 through 5 want a real end-to-end run. - **Firefox.** Blocked on #153 as expected, and deliberately not touched here.
clawbot commented 2026-08-09 05:01:35 +02:00
Author
Collaborator
Copy Link
Copy Source

Review: PR #171 — security: decrypt and sign dApp approvals in the popup

Independent adversarial review at head ab3b452. Reviewer did not author this change.

Verdict: PASS

Merge condition in item 6 below — nothing for the author to rework.

Gates

Gate Result
make check green — 5 suites, 75 tests, prettier clean, fmt-check clean
make build green — dist/chrome/ and dist/firefox/ both produced
CI on ab3b452 green — check / check (push), "Successful in 28s"
Mergeable vs main yes — branch contains origin/main (23aeae4); no conflicts
Commit title ends with (closes #157)
Attribution trailers none
RULES.md unmodified
TODO.md updated in the same commit
Scope creep none; #153 untouched
Inclusive terminology clean on all changed files

1. What crosses the boundary — verified independently

Enumerated every runtime.sendMessage call site in src/ and read each payload construction. The complete set of payload shapes on the approval paths is:

  • src/popup/views/approval.js:546 — {type, id, approved, rawSignedTx} or {type, id, approved, error}
  • src/popup/views/approval.js:644 — {type, id, approved, signature} or {type, id, approved, error}
  • :455, :466, :558, :656 — {type, id, approved, remember} (site/reject paths)

No password, private key, recovery phrase or xprv field on any of them. The author's claim holds.

The leak path I went looking for, and why it is closed. payload.error = e.shortMessage || e.message (approval.js:539, :637) crosses the boundary and is resolved back to the requesting dApp. That makes any ethers error raised inside getSignerForAddress() a candidate secret-exfiltration channel to a hostile website, since that call receives the decrypted secret directly. It is closed on two independent grounds:

  • The decrypt failure is handled in its own catch that returns before any payload object exists (approval.js:505-511, :596-602), so a bad secret never reaches the error-forwarding block.
  • ethers 6.16.0 redacts secret arguments in assertArgument errors on every branch getSignerForAddress() can take: crypto/signing-key.js:22 ("privateKey", "[REDACTED]"), wallet/mnemonic.js:21,26,38,42, wallet/hdwallet.js:256,258,272,293 (seed and extended key), wallet/base-wallet.js:35. Verified in the installed tree, not assumed.

Also confirmed: signer and decryptedSecret are not captured by the sendMessage callback — it closes over payload and view state only — and both password and decryptedSecret are nulled in finally blocks. This is stricter than the reference implementation in confirmTx.js, which nulls only decryptedSecret and leaves password a const.

2. Is src/shared/approvalVerify.js sound?

I could not construct an artifact that passes verification but differs from what the user approved in a way that matters.

  • Cross-payload replay of a signature: fails closed. verifyMessage / verifyTypedData recover over the approved payload, so a signature captured over anything else recovers to an unrelated address.
  • Malleability: not exploitable, on two grounds. ethers rejects non-canonical (high-s) signatures via the assertion in the Signature.s getter (crypto/signature.js:46), which every recovery path touches. And even if it did not, a malleable variant recovers to the same address over the same payload — it is the approved artifact.
  • sameAddress fallback (approvalVerify.js:507-516): the catch that falls back to lowercase string comparison is not a bypass. parsed.to from ethers is always a well-formed checksummed address or null, so the fallback can only fire on a malformed txParams.to, and it must then lowercase-equal a valid address — which is the correct answer.
  • Typed data: delete types.EIP712Domain operates on a fresh JSON.parse result, so the held approval is not mutated. Pinned by the test at tests/approvalVerify.test.js:855-863. The popup signer and the verifier perform the identical transform, so the check is symmetric — anything the popup can sign, the verifier can verify.
  • Mismatch genuinely fails closed: every rejection is a throw inside the background's try, which resolves the approval with an error and never reaches broadcastTransaction. The verify call precedes getProvider at src/background/index.js:741.

Non-blocking gap (finding A). verifySignedTx compares from, to, value and data, but not chainId, nonce, gasLimit, maxFeePerGas or maxPriorityFeePerGas (src/shared/approvalVerify.js:534-571). A hostile fee field is the one thing to/value/data does not cover. It is not exploitable as shipped, for two compounding reasons: POPUP_ONLY_TYPES at src/background/index.js:663-674 already rejects these message types from any non-extension sender (so a web page cannot forge them — the content script relays only AUTISTMASK_RPC), and the verifier requires the artifact to recover to the approved address, so only a party already holding the key can produce a passing artifact — and such a party can sign whatever it likes anyway. A chainId mismatch is additionally caught by the node at broadcast. Worth a follow-up issue for completeness of the "background is the authority" claim, not a merge blocker.

3. ENS to failing closed — recommendation: keep it, no change

The author's judgement is correct and I would not overrule it.

A dApp-initiated eth_sendTransaction cannot legitimately carry an ENS name in to: the JSON-RPC / EIP-1193 parameter is DATA, 20 bytes, and no mainstream wallet resolves ENS there. More decisively, the approval UI in this repo already treats to strictly as an address — toAddr.toLowerCase() for the token lookup (approval.js:168), decodeCalldata(details.txParams.data, toAddr) (:180) and approvalAddressHtml(toAddr) (:227). An ENS name would already render as an unresolvable string with no blockie and no token label.

So the old behaviour was the bug, not the new one: sendTransaction would silently resolve the name in the background, meaning the user approved the text "name.eth" and an entirely different, unverified address got paid. Failing closed with a full-sentence error is a fix. No user flow is broken.

(The analogous case — an ENS name in an address-typed field of typed data — fails in the popup rather than at the verifier, because the sign-path signer is unconnected and resolveNames has no provider. That is unchanged from the old background code, which also used an unconnected signer.)

4. Preserved behaviour — each verified in code

  • EIP-1193 4001 on rejection: src/background/index.js:718-722 and :776-780, both untouched by the diff. The windowsApi.onRemoved handler is likewise untouched.
  • Survives popup close/reopen: show() re-fetches via AUTISTMASK_GET_APPROVAL, and showTxApproval / showSignApproval re-stash the params at approval.js:166 and :348. The background's disconnect handler still keeps tx/sign approvals pending.
  • Wrong password fails cleanly and does not resolve the approval: caught at approval.js:505-511 / :596-602, full-sentence inline error, button re-enabled, and no message is sent at all — so pendingApprovals[msg.id] is never reached, let alone deleted. This exceeds the DoD; previously the bad password reached the background, which resolved and deleted the approval, killing the request.

5. TODO(security) markers

grep -rn "TODO(security)" src/ returns nothing, and the flaw is genuinely fixed rather than the markers merely deleted. Both halves satisfied.

6. Unverified interactive Chrome pass — not blocking, but gate the merge

Honest assessment, judged on risk rather than on the author's candour.

The automated coverage is a reasonable substitute for review purposes. The round-trip suite runs the real popup sequence (getSignerForAddress, connect, populateTransaction, delete pop.from, signTransaction) and feeds the result to the real background verifier, and the message contract is symmetric and covered on both ends. The two risks unit tests structurally cannot reach are both already mitigated in-tree:

  • Popup provider access at approval time — confirmTx.js:322-329 already does getProvider(state.rpcUrl) and broadcasts from the popup, so popup RPC access under the extension CSP and host permissions is proven by shipping code.
  • The click handler becoming async — the sendMessage callback form is retained on both paths, so MV3 messaging semantics are unchanged.

Given both, I do not consider this blocking, and there is nothing here the author could rework — they have no browser. But this is a wallet's signing path and DoD items 2-5 explicitly demand an end-to-end run. Recommend merging only after sneak performs the interactive Chrome pass across eth_sendTransaction, personal_sign and eth_signTypedData_v4, exactly as the author requested. Firefox being blocked on #153 is stated plainly in the PR, which is what manager note 5 asked for.

Non-blocking findings

  • A. src/shared/approvalVerify.js:534-571 — verifySignedTx does not compare chainId, nonce, gasLimit or the fee fields. Not exploitable as shipped (see item 2). Suggest a follow-up issue.
  • B. src/popup/views/approval.js:644-651 — on a popup-side signing failure the button is re-enabled via setSignButtonBusy(false), but the background already deleted the approval at src/background/index.js:772, so a retry can never succeed. Pre-existing shape, not a regression, but it sits right next to the wrong-password DoD item and deserves its own issue.
  • C. src/popup/views/approval.js:505-511, :596-602 — every decryptWithPassword failure is reported as "That password is incorrect", including a corrupt vault (sodium.from_base64 throwing) or sodium failing to initialise. Matches confirmTx.js. Cosmetic.
  • D. src/popup/views/approval.js:435-445 — findActiveWallet() matches state.activeAddress exactly, whereas the background's getActiveAddress() (src/background/index.js:54-62) falls back to wallets[0].addresses[0] when it is null. In that state the popup now fails closed where the old code signed with wallet 0. Recorded as an improvement, not a defect — the approval UI renders a blank "From" in that state, so refusing to sign is right.
  • E. src/shared/approvalVerify.js:608 — sameAddress is exported solely for the test file. Minor.
  • F. TODO.md:11-22 — the Status and Next Step blocks are stale, still describing the already-merged feat/issue-144-settings-about as in flight. Pre-existing on main and not introduced here; manager note 1 explicitly asked for a surgical TODO.md edit while #169 is open, so this is correct behaviour under the instructions given.

Things I checked that came back clean

  • Dropping await loadState() from the sign path is safe: that path now uses only getActiveAddress(), which reads storage itself via getState() (src/background/index.js:41-52). The tx path retains loadState() because it reads state.rpcUrl.
  • No dangling identifiers after the import removals — checked by grep for getBytes, decryptWithPassword, getSignerForAddress and msg.password in src/background/, all absent. Done by hand because script/lint is prettier-only per #152 and cannot catch this.
  • Every ethers import in both new and changed modules is used; none left over.
  • populateTransaction + delete pop.from + signTransaction reproduces ethers' own AbstractSigner.sendTransaction sequence, so nonce, gas, fee and chain-id population are unchanged from the background implementation. The dApp-supplied gas key was ignored by copyRequest before this change and still is — no regression.
  • Tests are meaningful, not vacuous: they assert on specific rejection messages, and cover contract creation with no recipient, absent value, case-differing call data, non-mutation of held approval state, and the exact wire payload key set.
# Review: PR #171 — security: decrypt and sign dApp approvals in the popup Independent adversarial review at head `ab3b452`. Reviewer did not author this change. ## Verdict: PASS Merge condition in item 6 below — nothing for the author to rework. --- ## Gates | Gate | Result | | --- | --- | | `make check` | green — 5 suites, 75 tests, prettier clean, `fmt-check` clean | | `make build` | green — `dist/chrome/` and `dist/firefox/` both produced | | CI on `ab3b452` | green — `check / check (push)`, "Successful in 28s" | | Mergeable vs `main` | yes — branch contains `origin/main` (`23aeae4`); no conflicts | | Commit title | ends with ` (closes #157)` | | Attribution trailers | none | | `RULES.md` | unmodified | | `TODO.md` | updated in the same commit | | Scope creep | none; #153 untouched | | Inclusive terminology | clean on all changed files | --- ## 1. What crosses the boundary — verified independently Enumerated every `runtime.sendMessage` call site in `src/` and read each payload construction. The complete set of payload shapes on the approval paths is: - `src/popup/views/approval.js:546` — `{type, id, approved, rawSignedTx}` or `{type, id, approved, error}` - `src/popup/views/approval.js:644` — `{type, id, approved, signature}` or `{type, id, approved, error}` - `:455`, `:466`, `:558`, `:656` — `{type, id, approved, remember}` (site/reject paths) No password, private key, recovery phrase or xprv field on any of them. The author's claim holds. **The leak path I went looking for, and why it is closed.** `payload.error = e.shortMessage || e.message` (`approval.js:539`, `:637`) crosses the boundary *and is resolved back to the requesting dApp*. That makes any ethers error raised inside `getSignerForAddress()` a candidate secret-exfiltration channel to a hostile website, since that call receives the decrypted secret directly. It is closed on two independent grounds: - The decrypt failure is handled in its own `catch` that returns before any `payload` object exists (`approval.js:505-511`, `:596-602`), so a bad secret never reaches the error-forwarding block. - ethers 6.16.0 redacts secret arguments in `assertArgument` errors on every branch `getSignerForAddress()` can take: `crypto/signing-key.js:22` (`"privateKey", "[REDACTED]"`), `wallet/mnemonic.js:21,26,38,42`, `wallet/hdwallet.js:256,258,272,293` (seed and extended key), `wallet/base-wallet.js:35`. Verified in the installed tree, not assumed. Also confirmed: `signer` and `decryptedSecret` are not captured by the `sendMessage` callback — it closes over `payload` and view state only — and both `password` and `decryptedSecret` are nulled in `finally` blocks. This is stricter than the reference implementation in `confirmTx.js`, which nulls only `decryptedSecret` and leaves `password` a `const`. ## 2. Is `src/shared/approvalVerify.js` sound? I could not construct an artifact that passes verification but differs from what the user approved in a way that matters. - **Cross-payload replay of a signature**: fails closed. `verifyMessage` / `verifyTypedData` recover over the *approved* payload, so a signature captured over anything else recovers to an unrelated address. - **Malleability**: not exploitable, on two grounds. ethers rejects non-canonical (high-s) signatures via the assertion in the `Signature.s` getter (`crypto/signature.js:46`), which every recovery path touches. And even if it did not, a malleable variant recovers to the same address over the same payload — it *is* the approved artifact. - **`sameAddress` fallback** (`approvalVerify.js:507-516`): the `catch` that falls back to lowercase string comparison is not a bypass. `parsed.to` from ethers is always a well-formed checksummed address or null, so the fallback can only fire on a malformed `txParams.to`, and it must then lowercase-equal a valid address — which is the correct answer. - **Typed data**: `delete types.EIP712Domain` operates on a fresh `JSON.parse` result, so the held approval is not mutated. Pinned by the test at `tests/approvalVerify.test.js:855-863`. The popup signer and the verifier perform the identical transform, so the check is symmetric — anything the popup can sign, the verifier can verify. - **Mismatch genuinely fails closed**: every rejection is a `throw` inside the background's `try`, which resolves the approval with an error and never reaches `broadcastTransaction`. The verify call precedes `getProvider` at `src/background/index.js:741`. **Non-blocking gap (finding A).** `verifySignedTx` compares `from`, `to`, `value` and `data`, but not `chainId`, `nonce`, `gasLimit`, `maxFeePerGas` or `maxPriorityFeePerGas` (`src/shared/approvalVerify.js:534-571`). A hostile fee field is the one thing `to`/`value`/`data` does not cover. It is not exploitable as shipped, for two compounding reasons: `POPUP_ONLY_TYPES` at `src/background/index.js:663-674` already rejects these message types from any non-extension sender (so a web page cannot forge them — the content script relays only `AUTISTMASK_RPC`), and the verifier requires the artifact to recover to the approved address, so only a party already holding the key can produce a passing artifact — and such a party can sign whatever it likes anyway. A chainId mismatch is additionally caught by the node at broadcast. Worth a follow-up issue for completeness of the "background is the authority" claim, not a merge blocker. ## 3. ENS `to` failing closed — recommendation: keep it, no change The author's judgement is correct and I would not overrule it. A dApp-initiated `eth_sendTransaction` cannot legitimately carry an ENS name in `to`: the JSON-RPC / EIP-1193 parameter is DATA, 20 bytes, and no mainstream wallet resolves ENS there. More decisively, the approval UI in this repo already treats `to` strictly as an address — `toAddr.toLowerCase()` for the token lookup (`approval.js:168`), `decodeCalldata(details.txParams.data, toAddr)` (`:180`) and `approvalAddressHtml(toAddr)` (`:227`). An ENS name would already render as an unresolvable string with no blockie and no token label. So the *old* behaviour was the bug, not the new one: `sendTransaction` would silently resolve the name in the background, meaning the user approved the text "name.eth" and an entirely different, unverified address got paid. Failing closed with a full-sentence error is a fix. No user flow is broken. (The analogous case — an ENS name in an `address`-typed field of typed data — fails in the popup rather than at the verifier, because the sign-path signer is unconnected and `resolveNames` has no provider. That is unchanged from the old background code, which also used an unconnected signer.) ## 4. Preserved behaviour — each verified in code - **EIP-1193 4001 on rejection**: `src/background/index.js:718-722` and `:776-780`, both untouched by the diff. The `windowsApi.onRemoved` handler is likewise untouched. - **Survives popup close/reopen**: `show()` re-fetches via `AUTISTMASK_GET_APPROVAL`, and `showTxApproval` / `showSignApproval` re-stash the params at `approval.js:166` and `:348`. The background's disconnect handler still keeps `tx`/`sign` approvals pending. - **Wrong password fails cleanly and does not resolve the approval**: caught at `approval.js:505-511` / `:596-602`, full-sentence inline error, button re-enabled, and **no message is sent at all** — so `pendingApprovals[msg.id]` is never reached, let alone deleted. This exceeds the DoD; previously the bad password reached the background, which resolved and deleted the approval, killing the request. ## 5. `TODO(security)` markers `grep -rn "TODO(security)" src/` returns nothing, and the flaw is genuinely fixed rather than the markers merely deleted. Both halves satisfied. ## 6. Unverified interactive Chrome pass — not blocking, but gate the merge Honest assessment, judged on risk rather than on the author's candour. The automated coverage is a reasonable substitute for review purposes. The round-trip suite runs the real popup sequence (`getSignerForAddress`, `connect`, `populateTransaction`, `delete pop.from`, `signTransaction`) and feeds the result to the real background verifier, and the message contract is symmetric and covered on both ends. The two risks unit tests structurally cannot reach are both already mitigated in-tree: - *Popup provider access at approval time* — `confirmTx.js:322-329` already does `getProvider(state.rpcUrl)` and broadcasts from the popup, so popup RPC access under the extension CSP and host permissions is proven by shipping code. - *The click handler becoming `async`* — the `sendMessage` callback form is retained on both paths, so MV3 messaging semantics are unchanged. Given both, I do not consider this blocking, and there is nothing here the author could rework — they have no browser. But this is a wallet's signing path and DoD items 2-5 explicitly demand an end-to-end run. **Recommend merging only after `sneak` performs the interactive Chrome pass** across `eth_sendTransaction`, `personal_sign` and `eth_signTypedData_v4`, exactly as the author requested. Firefox being blocked on #153 is stated plainly in the PR, which is what manager note 5 asked for. --- ## Non-blocking findings - **A.** `src/shared/approvalVerify.js:534-571` — `verifySignedTx` does not compare `chainId`, `nonce`, `gasLimit` or the fee fields. Not exploitable as shipped (see item 2). Suggest a follow-up issue. - **B.** `src/popup/views/approval.js:644-651` — on a popup-side *signing* failure the button is re-enabled via `setSignButtonBusy(false)`, but the background already deleted the approval at `src/background/index.js:772`, so a retry can never succeed. Pre-existing shape, not a regression, but it sits right next to the wrong-password DoD item and deserves its own issue. - **C.** `src/popup/views/approval.js:505-511`, `:596-602` — every `decryptWithPassword` failure is reported as "That password is incorrect", including a corrupt vault (`sodium.from_base64` throwing) or sodium failing to initialise. Matches `confirmTx.js`. Cosmetic. - **D.** `src/popup/views/approval.js:435-445` — `findActiveWallet()` matches `state.activeAddress` exactly, whereas the background's `getActiveAddress()` (`src/background/index.js:54-62`) falls back to `wallets[0].addresses[0]` when it is null. In that state the popup now fails closed where the old code signed with wallet 0. Recorded as an improvement, not a defect — the approval UI renders a blank "From" in that state, so refusing to sign is right. - **E.** `src/shared/approvalVerify.js:608` — `sameAddress` is exported solely for the test file. Minor. - **F.** `TODO.md:11-22` — the Status and Next Step blocks are stale, still describing the already-merged `feat/issue-144-settings-about` as in flight. Pre-existing on `main` and not introduced here; manager note 1 explicitly asked for a surgical `TODO.md` edit while #169 is open, so this is correct behaviour under the instructions given. ## Things I checked that came back clean - Dropping `await loadState()` from the sign path is safe: that path now uses only `getActiveAddress()`, which reads storage itself via `getState()` (`src/background/index.js:41-52`). The tx path retains `loadState()` because it reads `state.rpcUrl`. - No dangling identifiers after the import removals — checked by grep for `getBytes`, `decryptWithPassword`, `getSignerForAddress` and `msg.password` in `src/background/`, all absent. Done by hand because `script/lint` is prettier-only per #152 and cannot catch this. - Every ethers import in both new and changed modules is used; none left over. - `populateTransaction` + `delete pop.from` + `signTransaction` reproduces ethers' own `AbstractSigner.sendTransaction` sequence, so nonce, gas, fee and chain-id population are unchanged from the background implementation. The dApp-supplied `gas` key was ignored by `copyRequest` before this change and still is — no regression. - Tests are meaningful, not vacuous: they assert on specific rejection messages, and cover contract creation with no recipient, absent value, case-differing call data, non-mutation of held approval state, and the exact wire payload key set.
clawbot commented 2026-08-09 05:02:26 +02:00
Author
Collaborator
Copy Link
Copy Source

Manager note (the review verdict is in its own comment above).

Independent adversarial review passed. The reviewer did not author this change
and went after the one way this PR could have been a fake fix — swapping the
password on the wire for something worse. It enumerated every
runtime.sendMessage payload construction and confirmed only
{type, id, approved} plus rawSignedTx | signature | error crosses.

The interesting part is the leak channel it found and then closed: payload.error
(approval.js:539,637) is forwarded all the way back to the requesting dApp and
sits downstream of the call that handles the decrypted secret. It is safe twice
over — the decrypt failure returns from its own catch before any payload
exists, and ethers 6.16.0 redacts privateKey/mnemonic/seed/extendedKey
in assertArgument errors on every branch getSignerForAddress() can take.
The reviewer verified that redaction in the installed dependency tree rather
than assuming it. That is the check I most wanted made.

It also tried and failed to construct an artifact that passes approvalVerify
but differs from what the user approved, and specifically ruled out signature
malleability and cross-payload replay.

On the ENS behaviour change: no overrule — keeping the fail-closed is
correct, and it turns out to be a security fix rather than a regression. The
old path had sendTransaction silently resolving the name in the background,
so the user approved the literal string name.eth on screen while a different,
unverified address actually got paid. The approval UI never rendered an ENS
name correctly to begin with (toAddr.toLowerCase(), decodeCalldata,
approvalAddressHtml all treat to strictly as an address). So this PR closes
a second, unrelated hole incidentally. Worth knowing when reading the diff.

Wrong-password handling now exceeds the DoD: it is caught in the popup and no
message is sent at all, so the pending approval is never touched.

Marking merge-ready and assigning to @sneak.

Merge condition — please read before merging

DoD items 2-5 (live dApp eth_sendTransaction, personal_sign,
eth_signTypedData_v4 round trips) are unproven. There is no browser in
the agent environment, the author said so plainly rather than claiming the
build as proof, and the reviewer judged the risk rather than the candour —
there is nothing the author could rework to close it.

Both things unit tests structurally cannot reach are already mitigated in-tree:
popup-side RPC access is proven by shipping code at confirmTx.js:322-329, and
the callback form of sendMessage is retained so MV3 semantics are unchanged.

Even so — this is a wallet's signing path. Please do an interactive Chrome
pass across those three flows before merging. This is the first PR where the
gap bites, and I have raised the general problem as #173 (assigned to you) with
options; this PR is the concrete instance to decide on.

Also note #169 (#149) is still open and also touches TODO.md. Merging it
first keeps this one's rebase trivial.

Non-blocking findings and dispositions:

  • A — approvalVerify.js does not compare chainId, nonce, gasLimit
    or fee fields. Not exploitable (POPUP_ONLY_TYPES at
    background/index.js:663-674 restricts the sender, and only a key-holder can
    produce a passing artifact), but a hostile fee field is the one gap that
    to/value/data do not cover. Filed as #174 against 1.0.0.
  • B — after a popup-side signing failure the button re-enables, but the
    background already deleted the approval (background/index.js:772), so a
    retry cannot succeed. Pre-existing shape, not a regression. Folding into #174
    as a second item since it is the same file and the same review.
  • C — a corrupt vault reports "password is incorrect", which is misleading.
    Rolling into #172, which is already the password-message consistency issue.
  • E — sameAddress exported only for tests. Cosmetic, not acting.
  • F — stale TODO.md Status/Next Step. Pre-existing on main; manager
    note 1 explicitly asked for a surgical edit here while #169 is open. I will
    refresh it in the next unit once #169 has landed.
Manager note (the review verdict is in its own comment above). Independent adversarial review passed. The reviewer did not author this change and went after the one way this PR could have been a fake fix — swapping the password on the wire for something worse. It enumerated every `runtime.sendMessage` payload construction and confirmed only `{type, id, approved}` plus `rawSignedTx` | `signature` | `error` crosses. The interesting part is the leak channel it found and then closed: `payload.error` (`approval.js:539,637`) is forwarded all the way back to the requesting dApp and sits downstream of the call that handles the decrypted secret. It is safe twice over — the decrypt failure returns from its own `catch` before any `payload` exists, and ethers 6.16.0 redacts `privateKey`/`mnemonic`/`seed`/`extendedKey` in `assertArgument` errors on every branch `getSignerForAddress()` can take. The reviewer verified that redaction in the installed dependency tree rather than assuming it. That is the check I most wanted made. It also tried and failed to construct an artifact that passes `approvalVerify` but differs from what the user approved, and specifically ruled out signature malleability and cross-payload replay. **On the ENS behaviour change: no overrule — keeping the fail-closed is correct, and it turns out to be a security fix rather than a regression.** The old path had `sendTransaction` silently resolving the name in the background, so the user approved the literal string `name.eth` on screen while a different, unverified address actually got paid. The approval UI never rendered an ENS name correctly to begin with (`toAddr.toLowerCase()`, `decodeCalldata`, `approvalAddressHtml` all treat `to` strictly as an address). So this PR closes a second, unrelated hole incidentally. Worth knowing when reading the diff. Wrong-password handling now exceeds the DoD: it is caught in the popup and **no message is sent at all**, so the pending approval is never touched. Marking `merge-ready` and assigning to @sneak. ## Merge condition — please read before merging DoD items 2-5 (live dApp `eth_sendTransaction`, `personal_sign`, `eth_signTypedData_v4` round trips) are **unproven**. There is no browser in the agent environment, the author said so plainly rather than claiming the build as proof, and the reviewer judged the risk rather than the candour — there is nothing the author could rework to close it. Both things unit tests structurally cannot reach are already mitigated in-tree: popup-side RPC access is proven by shipping code at `confirmTx.js:322-329`, and the callback form of `sendMessage` is retained so MV3 semantics are unchanged. Even so — this is a wallet's signing path. **Please do an interactive Chrome pass across those three flows before merging.** This is the first PR where the gap bites, and I have raised the general problem as #173 (assigned to you) with options; this PR is the concrete instance to decide on. Also note #169 (#149) is still open and also touches `TODO.md`. Merging it first keeps this one's rebase trivial. Non-blocking findings and dispositions: - **A** — `approvalVerify.js` does not compare `chainId`, `nonce`, `gasLimit` or fee fields. Not exploitable (`POPUP_ONLY_TYPES` at `background/index.js:663-674` restricts the sender, and only a key-holder can produce a passing artifact), but a hostile fee field is the one gap that `to`/`value`/`data` do not cover. Filed as #174 against 1.0.0. - **B** — after a popup-side signing failure the button re-enables, but the background already deleted the approval (`background/index.js:772`), so a retry cannot succeed. Pre-existing shape, not a regression. Folding into #174 as a second item since it is the same file and the same review. - **C** — a corrupt vault reports "password is incorrect", which is misleading. Rolling into #172, which is already the password-message consistency issue. - **E** — `sameAddress` exported only for tests. Cosmetic, not acting. - **F** — stale `TODO.md` Status/Next Step. Pre-existing on `main`; manager note 1 explicitly asked for a surgical edit here while #169 is open. I will refresh it in the next unit once #169 has landed.
clawbot added merge-ready and removed needs-review labels 2026-08-09 05:02:31 +02:00
clawbot removed their assignment 2026-08-09 05:02:32 +02:00
sneak was assigned by clawbot 2026-08-09 05:02:32 +02:00
clawbot added this to the 1.0.0 milestone 2026-08-09 05:02:37 +02:00
clawbot commented 2026-08-09 16:11:34 +02:00
Author
Collaborator
Copy Link
Copy Source

Manager note on the outstanding caveat on this PR.

This PR is merge-ready and assigned to you carrying an explicit "Chrome not
verified interactively" caveat, on the grounds that no agent could drive a
browser. That premise was wrong, and I have now measured it rather than
inferred it - see
#173 (comment).

A containerized Chrome loads the unpacked MV3 build, runs the popup, and is
fully scriptable. It already caught a live crash on main (#150) that
make check passes straight through.

So the caveat on this PR is dischargeable by machine rather than by your time.
The approval signing path can be driven end to end - a local test page speaking
EIP-1193 to window.ethereum, through the real content script, the real
background worker and the real approval popup, with the RPC stubbed - covering
the eth_sendTransaction, personal_sign and eth_signTypedData_v4 round
trips that DoD items 2-5 of #157 call for.

The harness is #181 and is in implementation now. Once it lands I will add the
approval-flow coverage and report the result here.

This does not block merging this PR. Its unit coverage is unchanged and the
change is sound; the harness converts an open caveat into a verified assertion
after the fact. If you would rather hold it until the coverage exists, say so
and I will pull the merge-ready label. Otherwise it stays as-is and I will
follow up here with the harness result either way.

The one thing the harness will not discharge is a real dApp with real funds.
That stays with you before the 1.0.0 tag.

Manager note on the outstanding caveat on this PR. This PR is `merge-ready` and assigned to you carrying an explicit "Chrome not verified interactively" caveat, on the grounds that no agent could drive a browser. **That premise was wrong**, and I have now measured it rather than inferred it - see https://git.eeqj.de/sneak/AutistMask/issues/173#issuecomment-49609. A containerized Chrome loads the unpacked MV3 build, runs the popup, and is fully scriptable. It already caught a live crash on `main` (#150) that `make check` passes straight through. So the caveat on this PR is dischargeable by machine rather than by your time. The approval signing path can be driven end to end - a local test page speaking EIP-1193 to `window.ethereum`, through the real content script, the real background worker and the real approval popup, with the RPC stubbed - covering the `eth_sendTransaction`, `personal_sign` and `eth_signTypedData_v4` round trips that DoD items 2-5 of #157 call for. The harness is #181 and is in implementation now. Once it lands I will add the approval-flow coverage and report the result here. **This does not block merging this PR.** Its unit coverage is unchanged and the change is sound; the harness converts an open caveat into a verified assertion after the fact. If you would rather hold it until the coverage exists, say so and I will pull the `merge-ready` label. Otherwise it stays as-is and I will follow up here with the harness result either way. The one thing the harness will not discharge is a real dApp with real funds. That stays with you before the 1.0.0 tag.
clawbot marked the pull request as work in progress 2026-08-10 14:40:01 +02:00
clawbot added needs-rebase and removed merge-ready labels 2026-08-10 14:41:39 +02:00
sneak was unassigned by clawbot 2026-08-10 14:41:56 +02:00
clawbot self-assigned this 2026-08-10 14:41:56 +02:00
clawbot marked the pull request as ready for review 2026-08-10 15:47:05 +02:00
clawbot changed target branch from main to next 2026-08-10 15:47:05 +02:00
clawbot added 1 commit 2026-08-10 15:47:05 +02:00
security: decrypt and sign dApp approvals in the popup (closes #157)
check / check (push) Successful in 28s
Details
ab3b452924 
The dApp transaction and signature approval paths sent the user's plaintext
password to the background over runtime.sendMessage and decrypted there. Both
now decrypt in the popup, where the password is typed, and put only the signed
artifact on the wire: the raw signed transaction, or the signature. Neither the
password, the recovery phrase, the xprv nor the private key crosses the
messaging boundary any more. This matches what the popup-side eth_sendTransaction
path in confirmTx.js already did.

The popup runs the same sequence ethers' own sendTransaction() runs internally
(populateTransaction, then signTransaction), so nonce, gas, fee and chain id
population are unchanged. The background keeps broadcast and approval
resolution, and when the popup cannot produce an artifact it reports the error
over the same message so the requesting page still gets a failure rather than
hanging.

Moving the secret out of the background must not turn the background into a
blind relay, so it re-derives the signer from the artifact and checks it
against the approval it is holding before acting: shared/approvalVerify.js
asserts that a raw transaction is the approved transaction signed by the
approved address, and that a signature covers the approved payload and recovers
to the approved address.

A wrong password is now caught in the popup before anything is sent, so it
fails with an inline full-sentence error and leaves the pending approval alive
to retry; previously it reached the background and destroyed the approval.
Rejection still resolves with EIP-1193 code 4001, and approvals still survive
popup close and reopen.

Removes the four standing TODO(security) markers, now that the flaw is gone.
clawbot force-pushed fix/issue-157-approval-decrypt-in-popup from ab3b452924 to f37d52e04a 2026-08-10 15:58:42 +02:00 Compare
clawbot merged commit ad9162d057 into next 2026-08-10 15:59:30 +02:00
clawbot deleted branch fix/issue-157-approval-decrypt-in-popup 2026-08-10 15:59:31 +02:00
clawbot commented 2026-08-12 13:25:20 +02:00
Author
Collaborator
Copy Link
Copy Source

Closing out the "Chrome not verified interactively" caveat recorded here.

#273 has landed and drives all four EIP-1193 round trips through the real content script, background worker and approval popup, approved and rejected: eth_requestAccounts, personal_sign, eth_signTypedData_v4, eth_sendTransaction. Every signature is recovered and compared to the approved address, and the broadcast transaction is parsed from the bytes captured at eth_sendRawTransaction.

Directly relevant to this PR's change: the password is now asserted absent from every message crossing the extension boundary, verified by observing the messages rather than by reading the code. That gives this fix a permanent floor instead of a one-time review.

Not covered, and still a human pass before the 1.0.0 tag: a real dApp with real funds against mainnet.

Closing out the "Chrome not verified interactively" caveat recorded here. https://git.eeqj.de/sneak/AutistMask/pulls/273 has landed and drives all four EIP-1193 round trips through the real content script, background worker and approval popup, approved and rejected: `eth_requestAccounts`, `personal_sign`, `eth_signTypedData_v4`, `eth_sendTransaction`. Every signature is recovered and compared to the approved address, and the broadcast transaction is parsed from the bytes captured at `eth_sendRawTransaction`. Directly relevant to this PR's change: the password is now asserted absent from every message crossing the extension boundary, verified by observing the messages rather than by reading the code. That gives this fix a permanent floor instead of a one-time review. Not covered, and still a human pass before the 1.0.0 tag: a real dApp with real funds against mainnet.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels needs-rebase
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#171