diff --git a/TODO.md b/TODO.md index eb6dd07..fe5672a 100644 --- a/TODO.md +++ b/TODO.md @@ -44,6 +44,15 @@ undefined identifiers, which is how # Completed Steps +- 2026-08-12: Approval verification became an allowlist — transaction type + restricted to 0/1/2 so an EIP-7702 delegation can no longer ride along on an + approved transfer, every consequential field compared, the artifact + re-serialized from the checked fields alone and its exact bytes required to be + the canonical encoding of what was broadcast. One approval now yields at most + one broadcast, and every path that retires a pending approval — popup close, + active-address change, a late reject — goes through a single chokepoint that + refuses to settle an attempt already claimed for signing and broadcast + ([#174](https://git.eeqj.de/sneak/AutistMask/issues/174)). - 2026-08-12: An xprv wallet already in storage that was imported from a non-master key is detected from the depth of its stored `xpub`, explained in the wallet list, and blocked from signing, sending and private-key export diff --git a/src/background/index.js b/src/background/index.js index 9d22148..a92c707 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -13,7 +13,16 @@ const { } = require("../shared/state"); const { refreshBalances, getProvider } = require("../shared/balances"); const { debugFetch, log } = require("../shared/log"); -const { verifySignedTx, verifySignature } = require("../shared/approvalVerify"); +const { + verifySignedTx, + verifySignature, + failureIsRetryable, + describeTxFailure, + TX_STAGE_SIGN, + TX_STAGE_VERIFY, + TX_STAGE_BROADCAST, + TX_STAGE_INFLIGHT, +} = require("../shared/approvalVerify"); const { isPhishingDomain, refreshPhishingListOnSchedule, @@ -107,6 +116,55 @@ function resetPopupUrl() { } } +// Settle a pending approval: hand `result` to the promise the requesting page +// is waiting on and retire the approval. This is the ONLY place an approval is +// resolved or removed — the popup closing, an active-address switch, a reject +// from the popup and the attempt that signs and broadcasts all come through +// here — because a settlement that bypasses the claim below is a fund-loss bug +// and enumerating the call sites has repeatedly missed one. +// +// A claimed approval belongs to the attempt holding the claim, and only that +// attempt may settle it. Anything else settling first would leave the attempt +// running to completion against an already-settled promise: the transaction +// reaches the chain while the page is told "User rejected the request", and the +// user's natural response is to send it again at a fresh nonce. +// +// Returns false when the approval is gone or claimed by someone else, so the +// caller can refuse instead of assuming it settled. +function settleApproval(id, result, options) { + const approval = pendingApprovals[id]; + if (!approval) return false; + const holdsClaim = !!(options && options.holdsClaim); + if (approval.attemptInFlight && !holdsClaim) return false; + delete pendingApprovals[id]; + approval.resolve(result); + resetPopupUrl(); + return true; +} + +// Take exclusive hold of a pending approval for one attempt, or refuse. +// +// An approval that failed retryably has to stay in pendingApprovals, so its +// presence cannot be the interlock against a second attempt; this flag is. It +// is set synchronously, before the handler's first await, so a second response +// carrying the same id — a reloaded approval window re-rendering a live +// Approve button, a popup that emits the message twice — finds the attempt +// already running instead of starting an independent verify and broadcast. +// Without it one approval can put two transactions on the chain: with the +// ordinary dApp approval shape the page fixes no nonce, so two artifacts +// signed at different nonces both verify. +function claimApproval(approval) { + if (approval.attemptInFlight) return false; + approval.attemptInFlight = true; + return true; +} + +// Release an approval whose attempt failed in a way the user can retry. +// Nothing was broadcast, so the next attempt may claim it. +function releaseApproval(approval) { + approval.attemptInFlight = false; +} + // Open approval in a separate popup window. // This is the primary mechanism for tx/sign approvals (triggered programmatically, // not from a user gesture) and the fallback for site-connection approvals. @@ -215,8 +273,7 @@ runtime.onConnect.addListener((port) => { // Keep pending — user can reopen the toolbar popup return; } - approval.resolve({ approved: false, remember: false }); - delete pendingApprovals[id]; + settleApproval(id, { approved: false, remember: false }); } resetPopupUrl(); }); @@ -547,15 +604,21 @@ async function broadcastAccountsChanged() { for (const key of Object.keys(connectedSites)) { delete connectedSites[key]; } - // Reject and close any pending approval popups so they don't hang + // Reject and close any pending approval popups so they don't hang. An + // approval an attempt has already claimed is left alone entirely: it is + // being signed and broadcast right now, and neither rejecting it to the + // page nor closing the window it is reporting into is survivable. for (const [id, approval] of Object.entries(pendingApprovals)) { - if (approval.type === "tx" || approval.type === "sign") { - approval.resolve({ - error: { code: 4001, message: "User rejected the request." }, - }); - } else { - approval.resolve({ approved: false, remember: false }); - } + const rejection = + approval.type === "tx" || approval.type === "sign" + ? { + error: { + code: 4001, + message: "User rejected the request.", + }, + } + : { approved: false, remember: false }; + if (!settleApproval(id, rejection)) continue; if (approval.windowId) { windowsApi.remove(approval.windowId, () => { if (runtime.lastError) { @@ -563,7 +626,6 @@ async function broadcastAccountsChanged() { } }); } - delete pendingApprovals[id]; } resetPopupUrl(); const s = await getState(); @@ -679,23 +741,26 @@ if (runtime.onStartup) { } startBackgroundJobs(); -// When approval window is closed without a response, treat as rejection +// When approval window is closed without a response, treat as rejection. +// "Without a response" is the operative part: the popup stays open across the +// verify and broadcast it is waiting on, so a user closing an apparently-hung +// window is an ordinary event with an attempt already in flight behind it. +// settleApproval() refuses those, which leaves the attempt to report its real +// outcome to the page. if (windowsApi && windowsApi.onRemoved) { windowsApi.onRemoved.addListener((windowId) => { for (const [id, approval] of Object.entries(pendingApprovals)) { - if (approval.windowId === windowId) { - if (approval.type === "tx" || approval.type === "sign") { - approval.resolve({ - error: { - code: 4001, - message: "User rejected the request.", - }, - }); - } else { - approval.resolve({ approved: false, remember: false }); - } - delete pendingApprovals[id]; - } + if (approval.windowId !== windowId) continue; + const rejection = + approval.type === "tx" || approval.type === "sign" + ? { + error: { + code: 4001, + message: "User rejected the request.", + }, + } + : { approved: false, remember: false }; + settleApproval(id, rejection); } }); } @@ -761,14 +826,10 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { } if (msg.type === "AUTISTMASK_APPROVAL_RESPONSE") { - const approval = pendingApprovals[msg.id]; - if (approval) { - approval.resolve({ - approved: msg.approved, - remember: msg.remember, - }); - delete pendingApprovals[msg.id]; - } + settleApproval(msg.id, { + approved: msg.approved, + remember: msg.remember, + }); resetPopupUrl(); return false; } @@ -776,21 +837,50 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { if (msg.type === "AUTISTMASK_TX_RESPONSE") { const approval = pendingApprovals[msg.id]; if (!approval) return false; - delete pendingApprovals[msg.id]; - resetPopupUrl(); + // A reject arriving while an attempt holds the approval is refused, + // not honoured: the attempt is on its way to broadcasting the + // transaction, and resolving 4001 here would tell the page the request + // was rejected while it goes out. if (!msg.approved) { - approval.resolve({ - error: { code: 4001, message: "User rejected the request." }, - }); + if ( + !settleApproval(msg.id, { + error: { + code: 4001, + message: "User rejected the request.", + }, + }) + ) { + sendResponse({ + error: "This transaction is already being sent.", + retryable: false, + stage: TX_STAGE_BROADCAST, + }); + return false; + } return true; } - // The popup signs; it reports back here when it could not. Fail the - // request the same way this handler used to when it did the signing. + // The popup signs; it reports back here when it could not. Keep the + // approval so the user can correct the problem and try again with the + // transaction they already saw. if (msg.error) { - approval.resolve({ error: { message: msg.error } }); - sendResponse({ error: msg.error }); + const outcome = describeTxFailure(TX_STAGE_SIGN, msg.error); + sendResponse({ + error: outcome.error, + retryable: outcome.retryable, + stage: TX_STAGE_SIGN, + }); + return false; + } + + // Exactly one broadcast per approval, whatever the popup sends. + if (!claimApproval(approval)) { + sendResponse({ + error: "This transaction is already being sent.", + retryable: false, + stage: TX_STAGE_BROADCAST, + }); return false; } @@ -800,22 +890,63 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { const activeAddress = await getActiveAddress(); // The popup holds the secret, but the background stays the // authority on what is broadcast: the raw transaction must be - // the approved one, signed by the approved address. + // the approved one, signed by the approved address, on the + // network that is selected. verifySignedTx( msg.rawSignedTx, approval.txParams, activeAddress, + currentNetwork().chainId, ); + } catch (e) { + // A signed transaction that is not the approved one is not + // retried against that approval; it is refused outright. + // Anything else that failed before the check ran is the + // user's to retry. + const outcome = describeTxFailure(TX_STAGE_VERIFY, e); + if (outcome.spendApproval) { + settleApproval( + msg.id, + { error: { message: outcome.error } }, + { holdsClaim: true }, + ); + } else { + releaseApproval(approval); + } + sendResponse({ + error: outcome.error, + retryable: outcome.retryable, + stage: TX_STAGE_VERIFY, + }); + return; + } + + try { const provider = getProvider(state.rpcUrl); const tx = await provider.broadcastTransaction(msg.rawSignedTx); - approval.resolve({ txHash: tx.hash }); + settleApproval( + msg.id, + { txHash: tx.hash }, + { holdsClaim: true }, + ); sendResponse({ txHash: tx.hash }); } catch (e) { - const errMsg = e.shortMessage || e.message; - approval.resolve({ - error: { message: errMsg }, + // Terminal, never retried: the node may have accepted the + // transaction and still failed to answer, and the popup's + // retry re-signs at a freshly fetched nonce rather than + // re-broadcasting these bytes. Retrying would send the + // approved transfer a second time. + const outcome = describeTxFailure(TX_STAGE_BROADCAST, e); + settleApproval( + msg.id, + { error: { message: outcome.error } }, + { holdsClaim: true }, + ); + sendResponse({ + error: outcome.error, + retryable: outcome.retryable, + stage: TX_STAGE_BROADCAST, }); - sendResponse({ error: errMsg }); } })(); return true; @@ -824,21 +955,43 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { if (msg.type === "AUTISTMASK_SIGN_RESPONSE") { const approval = pendingApprovals[msg.id]; if (!approval) return false; - delete pendingApprovals[msg.id]; - resetPopupUrl(); + // Same as the transaction path: a reject cannot retire an approval an + // attempt already holds. if (!msg.approved) { - approval.resolve({ - error: { code: 4001, message: "User rejected the request." }, - }); + if ( + !settleApproval(msg.id, { + error: { + code: 4001, + message: "User rejected the request.", + }, + }) + ) { + sendResponse({ + error: "This request is already being signed.", + retryable: false, + stage: TX_STAGE_INFLIGHT, + }); + return false; + } return true; } - // The popup signs; it reports back here when it could not. Fail the - // request the same way this handler used to when it did the signing. + // The popup signs; it reports back here when it could not. Keep the + // approval so the user can correct the problem and try again with the + // message they already saw. if (msg.error) { - approval.resolve({ error: { message: msg.error } }); - sendResponse({ error: msg.error }); + sendResponse({ error: msg.error, retryable: true }); + return false; + } + + // Exactly one signature handed back per approval. + if (!claimApproval(approval)) { + sendResponse({ + error: "This request is already being signed.", + retryable: false, + stage: TX_STAGE_INFLIGHT, + }); return false; } @@ -851,14 +1004,21 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { // address. const signature = msg.signature; verifySignature(approval.signParams, signature, activeAddress); - approval.resolve({ signature }); + settleApproval(msg.id, { signature }, { holdsClaim: true }); sendResponse({ signature }); } catch (e) { const errMsg = e.shortMessage || e.message; - approval.resolve({ - error: { message: errMsg }, - }); - sendResponse({ error: errMsg }); + const retryable = failureIsRetryable(e); + if (!retryable) { + settleApproval( + msg.id, + { error: { message: errMsg } }, + { holdsClaim: true }, + ); + } else { + releaseApproval(approval); + } + sendResponse({ error: errMsg, retryable }); } })(); return true; diff --git a/src/popup/views/approval.js b/src/popup/views/approval.js index 25ba926..14993fc 100644 --- a/src/popup/views/approval.js +++ b/src/popup/views/approval.js @@ -23,6 +23,7 @@ const { decryptWithPassword } = require("../../shared/vault"); const { getSignerForAddress } = require("../../shared/wallet"); const { walletDefect } = require("../../shared/walletDefects"); const { getProvider } = require("../../shared/balances"); +const { describeSigningFailure } = require("../../shared/approvalVerify"); const txStatus = require("./txStatus"); const uniswap = require("../../shared/uniswap"); const runtime = @@ -571,10 +572,20 @@ function init(ctx) { runtime.sendMessage(payload, (response) => { if (response && response.txHash) { txStatus.showWait(pendingTxDetails, response.txHash); + return; + } + // A retryable failure leaves the approval pending in the + // background, so stay on this screen with a live button rather + // than sending the user to a dead end. + const outcome = describeSigningFailure( + response, + "The transaction could not be sent.", + ); + if (outcome.retryable) { + showError("approve-tx-error", outcome.message); + setTxButtonBusy(false); } else { - const msg = - (response && response.error) || "Transaction failed."; - txStatus.showError(pendingTxDetails, null, msg); + txStatus.showError(pendingTxDetails, null, outcome.message); } }); }); @@ -677,11 +688,18 @@ function init(ctx) { runtime.sendMessage(payload, (response) => { if (response && response.signature) { window.close(); - } else { - const msg = (response && response.error) || "Signing failed."; - showError("approve-sign-error", msg); - setSignButtonBusy(false); + return; } + // The button comes back only when the approval is still pending in + // the background; otherwise it stays disabled and the message says + // why, because a control that cannot succeed must not look like it + // can. + const outcome = describeSigningFailure( + response, + "The message could not be signed.", + ); + showError("approve-sign-error", outcome.message); + if (outcome.retryable) setSignButtonBusy(false); }); }); diff --git a/src/shared/approvalVerify.js b/src/shared/approvalVerify.js index 4004425..e46c17b 100644 --- a/src/shared/approvalVerify.js +++ b/src/shared/approvalVerify.js @@ -7,17 +7,144 @@ // the signer from the artifact and checks it against the approval it is // holding before acting on it. All recovery is delegated to ethers. // +// The check is an allowlist, in both directions, because a denylist cannot be +// correct against a transaction format that keeps gaining fields: +// +// - only transaction types 0, 1 and 2 are accepted. Every later EIP-2718 type +// adds a field with consequences of its own — EIP-7702's authorizationList +// rewrites the code at the signer's own account, EIP-4844's blob +// commitments carry a separate fee — and a check that enumerates the fields +// it refuses admits every one of them by default. +// - after the per-field comparisons, the artifact is rebuilt from those +// checked fields and nothing else, and the two are compared byte for byte. +// Anything the artifact carries that this module does not name is absent +// from the rebuild and changes the bytes, so the final assertion is that +// the artifact *is* the approved transaction, not merely that it is not one +// of the tampered shapes that were thought of. +// - every comparison runs against the decode, but the string handed to +// broadcastTransaction() is the artifact. So the artifact is also required +// to be the canonical re-encoding of its own decode, which is what makes +// the checked transaction and the broadcast bytes the same object rather +// than two things that merely decode alike. +// +// Every consequential field is compared, and a mismatch is a refusal to act, +// never a warning: what the user approved is what gets broadcast, or nothing +// does. +// +// Fields the approval does not carry are not treated as zero. The popup +// populates nonce, gas limit, fee and chain id through populateTransaction() +// when the requesting page did not fix them, so there is no approved value to +// compare against; treating absent as zero would refuse every legitimate +// transaction. Those fields are instead held to the absolute ceilings below, +// and the chain id is always checked against the selected network rather than +// against the approval alone, which is what makes a cross-chain replay +// impossible. +// // Every failure message is a full sentence, because these strings are shown to // the user and returned to the dApp. const { Transaction, + accessListify, getAddress, getBytes, verifyMessage, verifyTypedData, } = require("ethers"); +// The only transaction types this wallet signs: legacy, EIP-2930 and +// EIP-1559. populateTransaction() produces nothing else, so nothing else can +// be an artifact of an approval this wallet raised. +const ALLOWED_TX_TYPES = [0, 1, 2]; + +// The serialized fields of each allowed type, which is also the complete set +// of fields the checks below compare or bound. The artifact is rebuilt from +// exactly these at the end of verification and compared byte for byte, so a +// field outside this table cannot ride along unexamined. +const SERIALIZED_FIELDS = { + 0: ["chainId", "nonce", "gasPrice", "gasLimit", "to", "value", "data"], + 1: [ + "chainId", + "nonce", + "gasPrice", + "gasLimit", + "to", + "value", + "data", + "accessList", + ], + 2: [ + "chainId", + "nonce", + "maxPriorityFeePerGas", + "maxFeePerGas", + "gasLimit", + "to", + "value", + "data", + "accessList", + ], +}; + +// Fields no allowed type may carry. The type allowlist already excludes every +// type that defines them, and the structural check at the end of verification +// would catch them anyway; they are named here so that an artifact carrying +// one is refused with a message that says what it was. +const FORBIDDEN_FIELDS = [ + { + key: "authorizationList", + message: + "The signed transaction would hand the signing account over to another contract, which was not approved.", + }, + { + key: "blobVersionedHashes", + message: + "The signed transaction carries blob commitments, which were not approved.", + }, + { + key: "blobs", + message: + "The signed transaction carries blobs, which were not approved.", + }, + { + key: "maxFeePerBlobGas", + message: + "The signed transaction carries a blob gas fee, which was not approved.", + }, +]; + +// Above the block gas limit of every supported network (see networks.js), so +// no transaction that could ever be included is refused by it. +const MAX_GAS_LIMIT = 100000000n; + +// 100,000 gwei per gas: orders of magnitude above the highest fee either +// supported network has produced, and low enough to catch a fee that would +// hand the validator the balance. +const MAX_FEE_PER_GAS = 100000000000000n; + +// A refusal to act on an artifact: it is not the thing that was approved, so +// the approval it was offered against is spent and must not be retried. Every +// throw in this module is one of these; the background distinguishes them from +// transient failures (a busy node, a failed broadcast), which leave the +// approval standing so the user can try again. +class ApprovalMismatchError extends Error { + constructor(message) { + super(message); + this.name = "ApprovalMismatchError"; + this.approvalMismatch = true; + } +} + +function refuse(message) { + return new ApprovalMismatchError(message); +} + +// Whether a signing failure leaves the approval usable. Anything that is not a +// mismatch is the user's to correct and retry. +function failureIsRetryable(err) { + return !(err && err.approvalMismatch === true); +} + // Case-insensitive address comparison that tolerates absent values on either // side. Two absent addresses compare equal (contract creation has no `to`). function sameAddress(a, b) { @@ -31,11 +158,64 @@ function sameAddress(a, b) { } } +// Whether the approval fixed a value for a field at all. +function present(v) { + return v !== null && v !== undefined && v !== ""; +} + +// Whether a field carries anything at all. An empty array is nothing: ethers +// reports an absent access list on a type 2 transaction as `[]`. +function carriesValue(v) { + if (!present(v)) return false; + if (Array.isArray(v)) return v.length > 0; + return true; +} + +// Normalize a quantity that must be present, refusing anything that is not a +// number: an approval carrying junk in a fee field cannot be compared, and an +// uncomparable field is a refusal rather than a pass. +function normalizeQuantity(v, label) { + try { + return BigInt(v); + } catch { + throw refuse( + "The approved " + + label + + " is not a number, so it cannot be" + + " compared with the signed transaction.", + ); + } +} + // Normalize a transaction value (hex string, decimal string, number or -// bigint) to a bigint. An absent value is zero, matching ethers. +// bigint) to a bigint. An absent value is zero, matching ethers. The value is +// page-controlled, so it goes through the same refusal as every other +// quantity rather than throwing a raw BigInt conversion error. function normalizeValue(v) { - if (v === null || v === undefined || v === "") return 0n; - return BigInt(v); + if (!present(v)) return 0n; + return normalizeQuantity(v, "value"); +} + +// Normalize an access list to a comparable string. An absent or empty list is +// the empty string, so absent and `[]` are the same thing. +function normalizeAccessList(v) { + if (!carriesValue(v)) return ""; + let list; + try { + list = accessListify(v); + } catch { + throw refuse( + "The approved access list is not a valid access list, so it cannot be compared with the signed transaction.", + ); + } + return list + .map( + (entry) => + String(entry.address).toLowerCase() + + ":" + + entry.storageKeys.map((k) => String(k).toLowerCase()).join(","), + ) + .join(";"); } // Normalize call data to a lowercase hex string. Absent data is "0x". @@ -44,44 +224,216 @@ function normalizeData(v) { return String(v).toLowerCase(); } +// Quantity fields the requesting page may fix in the approval. Each is +// compared exactly when the approval carries it, and left to the ceilings +// above when it does not. +const APPROVED_QUANTITIES = [ + { + key: "nonce", + label: "nonce", + message: "The signed transaction does not carry the approved nonce.", + }, + { + key: "gasLimit", + label: "gas limit", + message: + "The signed transaction does not carry the approved gas limit.", + }, + { + key: "gasPrice", + label: "gas price", + message: + "The signed transaction does not carry the approved gas price.", + }, + { + key: "maxFeePerGas", + label: "maximum fee per gas", + message: + "The signed transaction does not carry the approved maximum fee per gas.", + }, + { + key: "maxPriorityFeePerGas", + label: "maximum priority fee per gas", + message: + "The signed transaction does not carry the approved maximum priority fee per gas.", + }, +]; + +// Refuse a field only a transaction type this wallet does not sign can carry. +// The type allowlist keeps these unreachable in production, which is exactly +// what they are for; it also means nothing else exercises them, so this is +// exported and tested on its own rather than left to be believed. +function assertNoForbiddenFields(parsed) { + for (const field of FORBIDDEN_FIELDS) { + if (carriesValue(parsed[field.key])) throw refuse(field.message); + } +} + +// Closing structural check. Rebuild the transaction from the fields the +// comparisons cover, and nothing else, then compare the unsigned bytes. Every +// field carried by the artifact but absent from the rebuild changes the +// serialization, so this refuses anything this module does not account for — +// including a field a future ethers learns to parse onto an allowed type — +// instead of waving it through by not naming it. Also exported for its own +// test: nothing reachable today can make the bytes differ. +function assertNothingUnchecked(parsed) { + let rebuilt; + try { + const fields = { type: parsed.type }; + for (const key of SERIALIZED_FIELDS[parsed.type]) { + fields[key] = parsed[key]; + } + rebuilt = Transaction.from(fields); + } catch { + throw refuse( + "The signed transaction could not be rebuilt from the fields that were checked, so it cannot be shown to be the approved transaction.", + ); + } + if (rebuilt.unsignedSerialized !== parsed.unsignedSerialized) { + throw refuse( + "The signed transaction carries data beyond the fields that were checked against the approval.", + ); + } +} + +// The other half of the closing check, and the one that makes it bind on the +// bytes that actually leave: every comparison above runs against the decode, +// so on its own the rebuild proves only that the transaction ethers understood +// is the approved one. What the background hands to broadcastTransaction() is +// the artifact string itself. Requiring the artifact to be exactly the +// canonical re-encoding of its own decode closes the gap between the two — +// no encoding the decoder normalizes away (a leading zero byte on an RLP +// quantity, say) can differ from what was checked. Hex case is not part of the +// encoding, so only that is normalized before comparing. +function assertCanonicalBytes(parsed, rawSignedTx) { + if (parsed.serialized !== String(rawSignedTx).toLowerCase()) { + throw refuse( + "The signed transaction is not encoded canonically, so the bytes that would be broadcast are not the bytes that were checked.", + ); + } +} + // Assert that a raw signed transaction is the transaction the user approved, -// signed by the address the approval was raised for. Returns the parsed -// ethers Transaction on success, throws otherwise. -function verifySignedTx(rawSignedTx, txParams, expectedFrom) { +// signed by the address the approval was raised for, on the network that is +// selected. Returns the parsed ethers Transaction on success, throws +// otherwise. +function verifySignedTx(rawSignedTx, txParams, expectedFrom, selectedChainId) { if (typeof rawSignedTx !== "string" || !rawSignedTx.startsWith("0x")) { - throw new Error("The signed transaction is missing or malformed."); + throw refuse("The signed transaction is missing or malformed."); } let parsed; try { parsed = Transaction.from(rawSignedTx); } catch { - throw new Error("The signed transaction could not be decoded."); + throw refuse("The signed transaction could not be decoded."); } if (!parsed.from) { - throw new Error("The signed transaction carries no valid signature."); + throw refuse("The signed transaction carries no valid signature."); } if (!sameAddress(parsed.from, expectedFrom)) { - throw new Error( + throw refuse( "The signed transaction was signed by a different address than the one that was approved.", ); } + + // Before any field is looked at: the type decides which fields exist at + // all, so an unrecognised type is refused outright rather than compared + // field by field against an approval that cannot describe it. + if (!ALLOWED_TX_TYPES.includes(parsed.type)) { + throw refuse( + "The signed transaction is of a type this wallet does not sign, so what it would do beyond the approved transfer cannot be checked.", + ); + } + assertNoForbiddenFields(parsed); + + // The selected network, not the artifact, is the authority on which chain + // this may be broadcast to; without it nothing can be verified. + if (!present(selectedChainId)) { + throw refuse( + "The selected network is unknown, so the signed transaction cannot be checked against it.", + ); + } + if (parsed.chainId !== normalizeQuantity(selectedChainId, "network")) { + throw refuse( + "The signed transaction is for a different network than the one that is selected.", + ); + } + if ( + present(txParams.chainId) && + parsed.chainId !== normalizeQuantity(txParams.chainId, "network") + ) { + throw refuse( + "The signed transaction is for a different network than the one that was approved.", + ); + } + if (!sameAddress(parsed.to, txParams.to)) { - throw new Error( + throw refuse( "The signed transaction does not go to the approved recipient.", ); } if (normalizeValue(parsed.value) !== normalizeValue(txParams.value)) { - throw new Error( + throw refuse( "The signed transaction does not carry the approved value.", ); } if (normalizeData(parsed.data) !== normalizeData(txParams.data)) { - throw new Error( + throw refuse( "The signed transaction does not carry the approved call data.", ); } + if ( + normalizeAccessList(parsed.accessList) !== + normalizeAccessList(txParams.accessList) + ) { + throw refuse( + "The signed transaction does not carry the approved access list.", + ); + } + + // An approval that fixed EIP-1559 fees must not be signed as a legacy + // transaction, and vice versa: the fee the user agreed to is only + // meaningful under the mechanism it was quoted in. + const approvedEip1559 = + present(txParams.maxFeePerGas) || + present(txParams.maxPriorityFeePerGas); + const approvedLegacy = present(txParams.gasPrice); + const signedEip1559 = parsed.type === 2; + if ( + (approvedEip1559 && !signedEip1559) || + (approvedLegacy && signedEip1559) + ) { + throw refuse( + "The signed transaction does not use the approved fee mechanism.", + ); + } + + for (const field of APPROVED_QUANTITIES) { + if (!present(txParams[field.key])) continue; + const approved = normalizeQuantity(txParams[field.key], field.label); + if (normalizeQuantity(parsed[field.key], field.label) !== approved) { + throw refuse(field.message); + } + } + + if (parsed.gasLimit > MAX_GAS_LIMIT) { + throw refuse( + "The signed transaction sets a gas limit no network this wallet supports can accept.", + ); + } + for (const key of ["gasPrice", "maxFeePerGas", "maxPriorityFeePerGas"]) { + const fee = parsed[key]; + if (fee !== null && fee !== undefined && fee > MAX_FEE_PER_GAS) { + throw refuse( + "The signed transaction sets a fee per gas far above any plausible value.", + ); + } + } + + assertNothingUnchecked(parsed); + assertCanonicalBytes(parsed, rawSignedTx); return parsed; } @@ -91,7 +443,7 @@ function verifySignedTx(rawSignedTx, txParams, expectedFrom) { // address on success, throws otherwise. function verifySignature(signParams, signature, expectedFrom) { if (typeof signature !== "string" || !signature.startsWith("0x")) { - throw new Error("The signature is missing or malformed."); + throw refuse("The signature is missing or malformed."); } let recovered; @@ -109,11 +461,11 @@ function verifySignature(signParams, signature, expectedFrom) { recovered = verifyTypedData(domain, types, message, signature); } } catch { - throw new Error("The signature could not be verified."); + throw refuse("The signature could not be verified."); } if (!sameAddress(recovered, expectedFrom)) { - throw new Error( + throw refuse( "The signature was produced by a different address than the one that was approved.", ); } @@ -121,4 +473,98 @@ function verifySignature(signParams, signature, expectedFrom) { return recovered; } -module.exports = { verifySignedTx, verifySignature, sameAddress }; +// The stage a transaction approval failed at. Which stage it is decides +// whether the approval survives the failure. +const TX_STAGE_SIGN = "sign"; +const TX_STAGE_VERIFY = "verify"; +const TX_STAGE_BROADCAST = "broadcast"; +// Not a failure of this request at all: a second response arrived for an +// approval an attempt already holds. The first attempt is still running and +// may yet succeed, so the one thing the popup must not say is "start again +// from the site". +const TX_STAGE_INFLIGHT = "inflight"; + +function errorText(err) { + if (typeof err === "string" && err !== "") return err; + if (err && (err.shortMessage || err.message)) { + return err.shortMessage || err.message; + } + return "The transaction could not be sent."; +} + +// What the background does with a pending transaction approval after a failed +// attempt: what it tells the popup, and whether the approval is spent +// (resolved to the requesting page as an error and deleted) or left standing +// so the user can try the transaction they already saw again. +// +// - sign: the popup could not produce an artifact, almost always a wrong +// password. Nothing left the extension, so the approval stands. +// - verify: a mismatch is a refusal and spends the approval — an artifact +// that is not the approved transaction must never be retried against that +// approval. Anything else failed before the check ran and is retryable. +// - broadcast: always terminal. A broadcast that throws after the node +// accepted the transaction is routine (a timeout, a dropped response, a +// node answering "already known"), and the popup's retry does not +// re-broadcast these bytes — it re-runs populateTransaction() and signs +// again at a freshly fetched pending-tag nonce. Retrying would therefore +// put a second transaction on the chain for one approval. +function describeTxFailure(stage, err) { + const error = errorText(err); + const retryable = + stage === TX_STAGE_SIGN || + (stage === TX_STAGE_VERIFY && failureIsRetryable(err)); + return { error, retryable, spendApproval: !retryable }; +} + +// What the popup shows and does after the background reports a failed signing +// attempt. A retryable failure leaves the approval pending in the background, +// so the button goes back to being usable; a refusal spent the approval, and +// the popup says so rather than offering a button that cannot succeed. +// +// A failed broadcast gets its own wording: the transaction may already be on +// the network, so telling the user to start again from the site is exactly the +// wrong instruction. +function describeSigningFailure(response, fallbackMessage) { + let message = (response && response.error) || fallbackMessage; + if (!/[.!?]$/.test(message)) message += "."; + const retryable = !!(response && response.retryable); + const stage = response && response.stage; + if (!retryable) { + if (stage === TX_STAGE_BROADCAST) { + message += + " The transaction may still have reached the network." + + " Check the account before sending it again."; + } else if (stage === TX_STAGE_INFLIGHT) { + message += + " The first attempt is still running and may still succeed." + + " Wait for it rather than starting again."; + } else { + message += + " This request can no longer be signed. Please start it" + + " again from the site."; + } + } + return { message, retryable }; +} + +module.exports = { + verifySignedTx, + verifySignature, + assertNoForbiddenFields, + assertNothingUnchecked, + assertCanonicalBytes, + sameAddress, + failureIsRetryable, + describeTxFailure, + describeSigningFailure, + ApprovalMismatchError, + ALLOWED_TX_TYPES, + SERIALIZED_FIELDS, + FORBIDDEN_FIELDS, + TX_STAGE_SIGN, + TX_STAGE_VERIFY, + TX_STAGE_BROADCAST, + TX_STAGE_INFLIGHT, + MAX_GAS_LIMIT, + MAX_FEE_PER_GAS, +}; diff --git a/tests/approvalVerify.test.js b/tests/approvalVerify.test.js index 1146416..1979c47 100644 --- a/tests/approvalVerify.test.js +++ b/tests/approvalVerify.test.js @@ -1,8 +1,28 @@ -const { Network, Transaction, Wallet } = require("ethers"); +const { + Network, + Transaction, + Wallet, + decodeRlp, + encodeRlp, +} = require("ethers"); const { verifySignedTx, verifySignature, + assertNoForbiddenFields, + assertNothingUnchecked, + assertCanonicalBytes, sameAddress, + failureIsRetryable, + describeTxFailure, + describeSigningFailure, + ALLOWED_TX_TYPES, + SERIALIZED_FIELDS, + FORBIDDEN_FIELDS, + TX_STAGE_SIGN, + TX_STAGE_VERIFY, + TX_STAGE_BROADCAST, + MAX_GAS_LIMIT, + MAX_FEE_PER_GAS, } = require("../src/shared/approvalVerify"); const { getSignerForAddress } = require("../src/shared/wallet"); @@ -18,6 +38,10 @@ const other = new Wallet(OTHER_KEY); const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; const OTHER_RECIPIENT = "0xdAC17F958D2ee523a2206206994597C13D831ec7"; +// The chain id of the selected network, as networks.js carries it. +const SELECTED = "0x1"; +const SEPOLIA = "0xaa36a7"; + // Approved parameters as a dApp would supply them over eth_sendTransaction. const TX_PARAMS = { from: signer.address, @@ -27,25 +51,38 @@ const TX_PARAMS = { gas: "0x5208", }; +// The values populateTransaction() fills in when the dApp fixed none of them. +const POPULATED = { + chainId: 1, + nonce: 7, + gasLimit: 100000n, + maxFeePerGas: 2000000000n, + maxPriorityFeePerGas: 1000000000n, + type: 2, +}; + // Build a signable transaction from approved params. The popup does the same // thing through populateTransaction(); here the fields are fixed so the test -// needs no provider. -function txFor(params) { +// needs no provider. `overrides` stands in for what a tampered or misbuilt +// popup would put on the wire. +function txFor(params, overrides) { return { - chainId: 1, - nonce: 7, - gasLimit: 100000n, - maxFeePerGas: 2000000000n, - maxPriorityFeePerGas: 1000000000n, - type: 2, + ...POPULATED, to: params.to, value: params.value === undefined ? 0n : BigInt(params.value), data: params.data || "0x", + ...(overrides || {}), }; } -async function signedFor(params, withWallet) { - return (withWallet || signer).signTransaction(txFor(params)); +async function signedFor(params, withWallet, overrides) { + return (withWallet || signer).signTransaction(txFor(params, overrides)); +} + +// Sign the approved transaction with one field changed from what was +// populated, which is the shape of every tamper case below. +async function signedWith(overrides) { + return signedFor(TX_PARAMS, signer, overrides); } describe("sameAddress", () => { @@ -71,7 +108,7 @@ describe("sameAddress", () => { describe("verifySignedTx", () => { test("accepts the approved transaction signed by the approved address", async () => { const raw = await signedFor(TX_PARAMS); - const parsed = verifySignedTx(raw, TX_PARAMS, signer.address); + const parsed = verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED); expect(parsed.from).toBe(signer.address); expect(parsed.hash).toBe(Transaction.from(raw).hash); }); @@ -79,14 +116,16 @@ describe("verifySignedTx", () => { test("accepts a contract creation with no recipient", async () => { const params = { to: undefined, value: "0x0", data: "0x600160005500" }; const raw = await signedFor(params); - expect(() => verifySignedTx(raw, params, signer.address)).not.toThrow(); + expect(() => + verifySignedTx(raw, params, signer.address, SELECTED), + ).not.toThrow(); }); test("accepts an absent value as zero", async () => { const approved = { to: RECIPIENT, data: "0x" }; const raw = await signedFor(approved); expect(() => - verifySignedTx(raw, approved, signer.address), + verifySignedTx(raw, approved, signer.address, SELECTED), ).not.toThrow(); }); @@ -94,7 +133,7 @@ describe("verifySignedTx", () => { const approved = { to: RECIPIENT, value: "0x0", data: "0xDEADBEEF" }; const raw = await signedFor(approved); expect(() => - verifySignedTx(raw, approved, signer.address), + verifySignedTx(raw, approved, signer.address, SELECTED), ).not.toThrow(); }); @@ -103,9 +142,9 @@ describe("verifySignedTx", () => { ...TX_PARAMS, to: OTHER_RECIPIENT, }); - expect(() => verifySignedTx(raw, TX_PARAMS, signer.address)).toThrow( - /approved recipient/, - ); + expect(() => + verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED), + ).toThrow(/approved recipient/); }); test("rejects an inflated value", async () => { @@ -113,48 +152,48 @@ describe("verifySignedTx", () => { ...TX_PARAMS, value: "0x4563918244f40000", }); - expect(() => verifySignedTx(raw, TX_PARAMS, signer.address)).toThrow( - /approved value/, - ); + expect(() => + verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED), + ).toThrow(/approved value/); }); test("rejects substituted call data", async () => { const raw = await signedFor({ ...TX_PARAMS, data: "0xc0ffee" }); - expect(() => verifySignedTx(raw, TX_PARAMS, signer.address)).toThrow( - /approved call data/, - ); + expect(() => + verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED), + ).toThrow(/approved call data/); }); test("rejects a transaction signed by a different address", async () => { const raw = await signedFor(TX_PARAMS, other); - expect(() => verifySignedTx(raw, TX_PARAMS, signer.address)).toThrow( - /different address/, - ); + expect(() => + verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED), + ).toThrow(/different address/); }); test("rejects an unsigned transaction", () => { const unsigned = Transaction.from(txFor(TX_PARAMS)).unsignedSerialized; expect(() => - verifySignedTx(unsigned, TX_PARAMS, signer.address), + verifySignedTx(unsigned, TX_PARAMS, signer.address, SELECTED), ).toThrow(/no valid signature/); }); test("rejects a missing or malformed payload", () => { expect(() => - verifySignedTx(undefined, TX_PARAMS, signer.address), + verifySignedTx(undefined, TX_PARAMS, signer.address, SELECTED), ).toThrow(/missing or malformed/); - expect(() => verifySignedTx("nope", TX_PARAMS, signer.address)).toThrow( - /missing or malformed/, - ); expect(() => - verifySignedTx("0xc0ffee", TX_PARAMS, signer.address), + verifySignedTx("nope", TX_PARAMS, signer.address, SELECTED), + ).toThrow(/missing or malformed/); + expect(() => + verifySignedTx("0xc0ffee", TX_PARAMS, signer.address, SELECTED), ).toThrow(/could not be decoded/); }); test("every rejection message is a full sentence", async () => { const raw = await signedFor({ ...TX_PARAMS, to: OTHER_RECIPIENT }); try { - verifySignedTx(raw, TX_PARAMS, signer.address); + verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED); throw new Error("expected a rejection"); } catch (e) { expect(e.message).toMatch(/^[A-Z].*\.$/); @@ -162,6 +201,606 @@ describe("verifySignedTx", () => { }); }); +// One case per consequential field: the field alone differs from what was +// approved, and that alone must refuse the signature. +describe("verifySignedTx field comparison", () => { + test("rejects a chain id that is not the selected network", async () => { + const raw = await signedWith({ chainId: 11155111 }); + expect(() => + verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED), + ).toThrow(/different network than the one that is selected/); + }); + + test("rejects a chain id that is not the approved one", async () => { + // Selected network and signed chain id agree; the dApp asked for a + // different chain, so the artifact is not what was approved. + const approved = { ...TX_PARAMS, chainId: SEPOLIA }; + const raw = await signedWith({}); + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).toThrow(/different network than the one that was approved/); + }); + + test("refuses when the selected network is unknown", async () => { + const raw = await signedWith({}); + expect(() => + verifySignedTx(raw, TX_PARAMS, signer.address, undefined), + ).toThrow(/selected network is unknown/); + }); + + test("rejects a substituted nonce", async () => { + const approved = { ...TX_PARAMS, nonce: 7 }; + const raw = await signedWith({ nonce: 8 }); + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).toThrow(/approved nonce/); + }); + + test("rejects a substituted gas limit", async () => { + const approved = { ...TX_PARAMS, gasLimit: "0x186a0" }; + const raw = await signedWith({ gasLimit: 250000n }); + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).toThrow(/approved gas limit/); + }); + + test("rejects a substituted maximum fee per gas", async () => { + const approved = { ...TX_PARAMS, maxFeePerGas: "0x77359400" }; + const raw = await signedWith({ maxFeePerGas: 900000000000n }); + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).toThrow(/approved maximum fee per gas/); + }); + + test("rejects a substituted maximum priority fee per gas", async () => { + const approved = { ...TX_PARAMS, maxPriorityFeePerGas: "0x3b9aca00" }; + const raw = await signedWith({ maxPriorityFeePerGas: 1500000000n }); + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).toThrow(/approved maximum priority fee per gas/); + }); + + test("rejects a substituted legacy gas price", async () => { + const approved = { ...TX_PARAMS, gasPrice: "0x77359400" }; + const legacy = { + type: 0, + gasPrice: 9000000000n, + maxFeePerGas: null, + maxPriorityFeePerGas: null, + }; + const raw = await signedWith(legacy); + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).toThrow(/approved gas price/); + }); + + test("rejects an approved legacy fee signed as an EIP-1559 fee", async () => { + const approved = { ...TX_PARAMS, gasPrice: "0x77359400" }; + const raw = await signedWith({}); + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).toThrow(/approved fee mechanism/); + }); + + test("rejects an approved EIP-1559 fee signed as a legacy fee", async () => { + const approved = { ...TX_PARAMS, maxFeePerGas: "0x77359400" }; + const raw = await signedWith({ + type: 0, + gasPrice: 2000000000n, + maxFeePerGas: null, + maxPriorityFeePerGas: null, + }); + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).toThrow(/approved fee mechanism/); + }); + + test("rejects a gas limit above anything a supported network accepts", async () => { + const raw = await signedWith({ gasLimit: MAX_GAS_LIMIT + 1n }); + expect(() => + verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED), + ).toThrow(/gas limit no network this wallet supports/); + }); + + test("rejects an absurd fee per gas the approval never fixed", async () => { + const raw = await signedWith({ + maxFeePerGas: MAX_FEE_PER_GAS + 1n, + maxPriorityFeePerGas: MAX_FEE_PER_GAS + 1n, + }); + expect(() => + verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED), + ).toThrow(/fee per gas far above any plausible value/); + }); + + test("every field mismatch is a refusal, not a warning", async () => { + const raw = await signedWith({ nonce: 8 }); + try { + verifySignedTx( + raw, + { ...TX_PARAMS, nonce: 7 }, + signer.address, + SELECTED, + ); + throw new Error("expected a rejection"); + } catch (e) { + expect(e.approvalMismatch).toBe(true); + expect(e.message).toMatch(/^[A-Z].*\.$/); + } + }); +}); + +// The transaction type decides which fields exist, so an artifact of a type +// this wallet does not sign carries consequences the approval cannot describe +// and none of the field comparisons can see. The approval used here is the +// ordinary dApp shape with no fee fields — the common case, since +// populateTransaction() fills them — which is exactly the case the +// fee-mechanism check cannot catch by accident. +describe("verifySignedTx transaction type", () => { + const BARE_APPROVAL = { + from: signer.address, + to: RECIPIENT, + value: "0x2386f26fc10000", + data: "0x", + }; + + // An EIP-7702 artifact that pays the approved amount to the approved + // recipient and, in the same transaction, installs the attacker's code at + // the signer's own account for good. Every field the approval screen shows + // matches; only the type and the authorization list do not. + test("refuses a type 4 artifact that delegates the signer's own account", async () => { + const authorization = await signer.authorize({ + address: OTHER_RECIPIENT, + chainId: 1, + nonce: 8, + }); + const raw = await signedFor(BARE_APPROVAL, signer, { + type: 4, + authorizationList: [authorization], + }); + const parsed = Transaction.from(raw); + expect(parsed.type).toBe(4); + expect(parsed.authorizationList[0].address).toBe(OTHER_RECIPIENT); + expect(() => + verifySignedTx(raw, BARE_APPROVAL, signer.address, SELECTED), + ).toThrow(/type this wallet does not sign/); + }); + + test("refuses a type 3 blob artifact", async () => { + const raw = await signedFor(BARE_APPROVAL, signer, { + type: 3, + maxFeePerBlobGas: 1000000000n, + blobVersionedHashes: ["0x01" + "ab".repeat(31)], + }); + expect(Transaction.from(raw).type).toBe(3); + expect(() => + verifySignedTx(raw, BARE_APPROVAL, signer.address, SELECTED), + ).toThrow(/type this wallet does not sign/); + }); + + test("refuses every type outside the allowlist, not just the known ones", async () => { + for (const type of [3, 4]) { + expect(ALLOWED_TX_TYPES).not.toContain(type); + } + expect(ALLOWED_TX_TYPES).toEqual([0, 1, 2]); + }); + + test("a type refusal is a refusal, not a warning", async () => { + const authorization = await signer.authorize({ + address: OTHER_RECIPIENT, + chainId: 1, + nonce: 8, + }); + const raw = await signedFor(BARE_APPROVAL, signer, { + type: 4, + authorizationList: [authorization], + }); + try { + verifySignedTx(raw, BARE_APPROVAL, signer.address, SELECTED); + throw new Error("expected a rejection"); + } catch (e) { + expect(e.approvalMismatch).toBe(true); + expect(e.message).toMatch(/^[A-Z].*\.$/); + } + }); + + test("accepts a legacy type 0 transaction", async () => { + const approved = { ...BARE_APPROVAL, gasPrice: "0x77359400" }; + const raw = await signedFor(approved, signer, { + type: 0, + gasPrice: 2000000000n, + maxFeePerGas: null, + maxPriorityFeePerGas: null, + }); + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).not.toThrow(); + }); + + test("accepts a type 1 transaction whose access list is the approved one", async () => { + const accessList = [{ address: OTHER_RECIPIENT, storageKeys: [] }]; + const approved = { + ...BARE_APPROVAL, + gasPrice: "0x77359400", + accessList, + }; + const raw = await signedFor(approved, signer, { + type: 1, + gasPrice: 2000000000n, + maxFeePerGas: null, + maxPriorityFeePerGas: null, + accessList, + }); + expect(Transaction.from(raw).type).toBe(1); + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).not.toThrow(); + }); + + test("refuses an access list the approval never carried", async () => { + const raw = await signedFor(BARE_APPROVAL, signer, { + accessList: [{ address: OTHER_RECIPIENT, storageKeys: [] }], + }); + expect(() => + verifySignedTx(raw, BARE_APPROVAL, signer.address, SELECTED), + ).toThrow(/approved access list/); + }); + + test("treats an absent access list and an empty one as the same thing", async () => { + const approved = { ...BARE_APPROVAL, accessList: [] }; + const raw = await signedFor(BARE_APPROVAL, signer, {}); + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).not.toThrow(); + }); +}); + +// The allowlist is only exhaustive while it accounts for every field an +// artifact can carry. These tests are what makes that claim checkable rather +// than asserted. +describe("verifySignedTx exhaustiveness", () => { + // Every accessor ethers exposes on a parsed transaction, and where this + // module deals with it. If an ethers upgrade adds a transaction field, + // this fails and forces a decision about it instead of letting it default + // to unchecked. + test("every field ethers can parse is accounted for", () => { + const derived = [ + // Recovered from the signature or computed from the payload, not + // independent content: covered by the signer check and by the + // fields below. + "from", + "fromPublicKey", + "hash", + "serialized", + "signature", + "type", + "typeName", + "unsignedHash", + "unsignedSerialized", + // Blob sidecar machinery, meaningful only alongside `blobs`, + // which is refused outright. + "kzg", + "blobWrapperVersion", + ]; + const accounted = new Set([ + ...derived, + ...FORBIDDEN_FIELDS.map((f) => f.key), + ...Object.values(SERIALIZED_FIELDS).flat(), + ]); + const exposed = Object.getOwnPropertyNames(Transaction.prototype) + .filter((name) => { + const d = Object.getOwnPropertyDescriptor( + Transaction.prototype, + name, + ); + return d && typeof d.get === "function"; + }) + .sort(); + expect(exposed.filter((name) => !accounted.has(name))).toEqual([]); + }); + + // The two layers behind the type allowlist. Nothing reachable through + // verifySignedTx can trip either of them while the allowlist holds — that + // is what they are for — so they are exercised directly rather than taken + // on trust. + test("a forbidden field is refused even on an allowed type", async () => { + const authorization = await signer.authorize({ + address: OTHER_RECIPIENT, + chainId: 1, + nonce: 8, + }); + const carriers = { + authorizationList: [authorization], + blobVersionedHashes: ["0x01" + "ab".repeat(31)], + blobs: ["0x00"], + maxFeePerBlobGas: 1n, + }; + for (const key of Object.keys(carriers)) { + expect(FORBIDDEN_FIELDS.map((f) => f.key)).toContain(key); + let thrown; + try { + assertNoForbiddenFields({ type: 2, [key]: carriers[key] }); + throw new Error("expected a rejection"); + } catch (e) { + thrown = e; + } + expect(thrown.approvalMismatch).toBe(true); + expect(thrown.message).toMatch(/^[A-Z].*\.$/); + } + expect(() => assertNoForbiddenFields({ type: 2 })).not.toThrow(); + }); + + // Stands in for a future ethers that parses a field this module does not + // know about onto an allowed type: every field the module checks is + // identical, and the bytes are not. + test("an artifact carrying more than the checked fields is refused", async () => { + const parsed = Transaction.from(await signedWith({})); + const smuggled = { type: parsed.type }; + for (const key of SERIALIZED_FIELDS[parsed.type]) { + smuggled[key] = parsed[key]; + } + smuggled.unsignedSerialized = parsed.unsignedSerialized + "ff"; + expect(() => assertNothingUnchecked(smuggled)).toThrow( + /beyond the fields that were checked/, + ); + expect(() => assertNothingUnchecked(parsed)).not.toThrow(); + }); + + // The closing check rebuilds the artifact from the fields the module + // compared and compares the bytes, so an artifact carrying anything else + // is refused without the module having to name it. Assert the rebuild is + // faithful for every accepted shape, since a rebuild that dropped a + // legitimate field would refuse honest transactions. + test("an accepted artifact of each allowed type rebuilds byte for byte", async () => { + const shapes = [ + { + approved: { ...TX_PARAMS, gasPrice: "0x77359400" }, + overrides: { + type: 0, + gasPrice: 2000000000n, + maxFeePerGas: null, + maxPriorityFeePerGas: null, + }, + }, + { + approved: { + ...TX_PARAMS, + gasPrice: "0x77359400", + accessList: [ + { + address: RECIPIENT, + storageKeys: ["0x" + "11".repeat(32)], + }, + ], + }, + overrides: { + type: 1, + gasPrice: 2000000000n, + maxFeePerGas: null, + maxPriorityFeePerGas: null, + accessList: [ + { + address: RECIPIENT, + storageKeys: ["0x" + "11".repeat(32)], + }, + ], + }, + }, + { approved: TX_PARAMS, overrides: {} }, + ]; + for (const shape of shapes) { + const raw = await signedFor( + shape.approved, + signer, + shape.overrides, + ); + const parsed = verifySignedTx( + raw, + shape.approved, + signer.address, + SELECTED, + ); + const fields = { type: parsed.type }; + for (const key of SERIALIZED_FIELDS[parsed.type]) { + fields[key] = parsed[key]; + } + expect(Transaction.from(fields).unsignedSerialized).toBe( + parsed.unsignedSerialized, + ); + } + }); +}); + +// Every comparison above runs against the decode, but the string that is +// handed to broadcastTransaction() is the artifact. An encoding the decoder +// normalizes away therefore checks as one transaction and broadcasts as +// different bytes, so the artifact must be the canonical encoding of itself. +describe("verifySignedTx canonical encoding", () => { + // Re-encode a signed type-2 artifact with a leading zero byte on the RLP + // value field. It decodes to exactly the approved transaction — same + // value, same signer, same everything the field comparisons look at — and + // it is not the same string. + async function nonCanonical() { + const raw = await signedWith({}); + const items = decodeRlp("0x" + raw.slice(4)); + // type 2 payload order: chainId, nonce, maxPriorityFeePerGas, + // maxFeePerGas, gasLimit, to, value, data, accessList, then the + // signature. + const padded = items.slice(); + padded[6] = "0x00" + items[6].slice(2); + return "0x02" + encodeRlp(padded).slice(2); + } + + test("the mutation decodes to the approved transaction and is not it", async () => { + const raw = await signedWith({}); + const mutated = await nonCanonical(); + const parsed = Transaction.from(mutated); + expect(mutated).not.toBe(raw); + expect(mutated.length).toBeGreaterThan(raw.length); + expect(parsed.value).toBe(BigInt(TX_PARAMS.value)); + expect(parsed.from).toBe(signer.address); + expect(parsed.serialized).not.toBe(mutated); + }); + + test("refuses an artifact that is not its own canonical encoding", async () => { + const mutated = await nonCanonical(); + expect(() => + verifySignedTx(mutated, TX_PARAMS, signer.address, SELECTED), + ).toThrow(/not encoded canonically/); + }); + + test("assertCanonicalBytes accepts what ethers itself produced", async () => { + const raw = await signedWith({}); + expect(() => + assertCanonicalBytes(Transaction.from(raw), raw), + ).not.toThrow(); + }); + + test("hex case is not part of the encoding", async () => { + const raw = await signedWith({}); + const upper = "0x" + raw.slice(2).toUpperCase(); + expect(() => + verifySignedTx(upper, TX_PARAMS, signer.address, SELECTED), + ).not.toThrow(); + }); +}); + +// The approval and the artifact spell the same values differently. None of +// these differences is tampering, so none may refuse the signature. +describe("verifySignedTx normalization", () => { + test("accepts a decimal chain id against a hex selected network", async () => { + const raw = await signedWith({}); + expect(() => + verifySignedTx(raw, TX_PARAMS, signer.address, 1), + ).not.toThrow(); + expect(() => + verifySignedTx(raw, TX_PARAMS, signer.address, "1"), + ).not.toThrow(); + }); + + test("accepts an approved chain id written in hex", async () => { + const raw = await signedWith({}); + const approved = { ...TX_PARAMS, chainId: "0x1" }; + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).not.toThrow(); + }); + + test("accepts a hex nonce against a numeric one", async () => { + const raw = await signedWith({ nonce: 7 }); + expect(() => + verifySignedTx( + raw, + { ...TX_PARAMS, nonce: "0x7" }, + signer.address, + SELECTED, + ), + ).not.toThrow(); + }); + + test("accepts a decimal gas limit against a hex one", async () => { + const raw = await signedWith({ gasLimit: 100000n }); + expect(() => + verifySignedTx( + raw, + { ...TX_PARAMS, gasLimit: "100000" }, + signer.address, + SELECTED, + ), + ).not.toThrow(); + }); + + test("accepts fee fields spelled as hex, decimal, number and bigint", async () => { + const raw = await signedWith({}); + for (const maxFee of [ + "0x77359400", + "2000000000", + 2000000000, + 2000000000n, + ]) { + expect(() => + verifySignedTx( + raw, + { ...TX_PARAMS, maxFeePerGas: maxFee }, + signer.address, + SELECTED, + ), + ).not.toThrow(); + } + }); + + test("accepts an approval that fixes no nonce, gas or fee at all", async () => { + const raw = await signedWith({}); + expect(() => + verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED), + ).not.toThrow(); + }); + + test("accepts an approval whose recipient case differs", async () => { + const raw = await signedWith({}); + const approved = { ...TX_PARAMS, to: RECIPIENT.toLowerCase() }; + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).not.toThrow(); + }); + + test("accepts absent call data against 0x", async () => { + const approved = { to: RECIPIENT, value: "0x0" }; + const raw = await signedFor({ ...approved, data: "0x" }); + expect(() => + verifySignedTx(raw, approved, signer.address, SELECTED), + ).not.toThrow(); + }); + + test("refuses an approved quantity that is not a number", async () => { + const raw = await signedWith({}); + expect(() => + verifySignedTx( + raw, + { ...TX_PARAMS, maxFeePerGas: "cheap" }, + signer.address, + SELECTED, + ), + ).toThrow(/is not a number/); + }); + + // The value is page-controlled. A refusal is correct; a raw BigInt + // conversion error is not, because it is not a mismatch, so it would be + // reported retryable and leave the approval unspent behind a live button + // that can never succeed. + test("refuses an approved value that is not a number, as a mismatch", async () => { + const raw = await signedWith({}); + for (const value of ["cheap", 1.5, "1e18", {}]) { + let thrown; + try { + verifySignedTx( + raw, + { ...TX_PARAMS, value }, + signer.address, + SELECTED, + ); + throw new Error("expected a rejection"); + } catch (e) { + thrown = e; + } + expect(thrown.approvalMismatch).toBe(true); + expect(thrown.message).toMatch(/approved value is not a number/); + expect(failureIsRetryable(thrown)).toBe(false); + } + }); + + test("refuses an approved access list that is not an access list", async () => { + const raw = await signedWith({}); + expect(() => + verifySignedTx( + raw, + { ...TX_PARAMS, accessList: ["nope"] }, + signer.address, + SELECTED, + ), + ).toThrow(/not a valid access list/); + }); +}); + const TYPED_DATA = JSON.stringify({ domain: { name: "AutistMask Test", @@ -281,6 +920,147 @@ describe("verifySignature", () => { }); }); +// What happens after a signing attempt fails: the background keeps the +// approval for anything the user can correct, and the popup only offers the +// button again when it did. +describe("signing failure and retry", () => { + test("a failure that is not a mismatch leaves the approval retryable", () => { + expect(failureIsRetryable(new Error("The node is unreachable."))).toBe( + true, + ); + expect(failureIsRetryable(undefined)).toBe(true); + }); + + test("a mismatch spends the approval", async () => { + const raw = await signedFor({ ...TX_PARAMS, to: OTHER_RECIPIENT }); + try { + verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED); + throw new Error("expected a rejection"); + } catch (e) { + expect(failureIsRetryable(e)).toBe(false); + } + }); + + test("a retryable failure keeps the button usable and says only what failed", () => { + const outcome = describeSigningFailure( + { error: "The node rejected the transaction.", retryable: true }, + "The transaction could not be sent.", + ); + expect(outcome.retryable).toBe(true); + expect(outcome.message).toBe("The node rejected the transaction."); + }); + + test("a refusal tells the user to start again from the site", () => { + const outcome = describeSigningFailure( + { + error: "The signed transaction does not go to the approved recipient.", + retryable: false, + }, + "The transaction could not be sent.", + ); + expect(outcome.retryable).toBe(false); + expect(outcome.message).toMatch(/start it again from the site\.$/); + }); + + test("a refusal for an attempt already running does not say to start again", () => { + const outcome = describeSigningFailure( + { + error: "This request is already being signed.", + retryable: false, + stage: "inflight", + }, + "The message could not be signed.", + ); + expect(outcome.retryable).toBe(false); + expect(outcome.message).not.toMatch(/start it again from the site/); + expect(outcome.message).toMatch(/first attempt is still running/); + }); + + test("a response the background never sent is treated as a spent approval", () => { + const outcome = describeSigningFailure( + undefined, + "The transaction could not be sent.", + ); + expect(outcome.retryable).toBe(false); + expect(outcome.message).toMatch(/^The transaction could not be sent\./); + }); + + test("every failure message is a full sentence", () => { + const outcome = describeSigningFailure( + { error: "The node is on fire", retryable: true }, + "The transaction could not be sent.", + ); + expect(outcome.message).toMatch(/^[A-Z].*\.$/); + }); + + test("a popup that could not sign leaves the approval standing", () => { + const outcome = describeTxFailure( + TX_STAGE_SIGN, + "That password is incorrect. Please try again.", + ); + expect(outcome.retryable).toBe(true); + expect(outcome.spendApproval).toBe(false); + expect(outcome.error).toMatch(/password is incorrect/); + }); + + test("a mismatch found at verification spends the approval", async () => { + const raw = await signedFor({ ...TX_PARAMS, to: OTHER_RECIPIENT }); + let outcome; + try { + verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED); + } catch (e) { + outcome = describeTxFailure(TX_STAGE_VERIFY, e); + } + expect(outcome.retryable).toBe(false); + expect(outcome.spendApproval).toBe(true); + }); + + test("a failure before the check ran is still retryable", () => { + const outcome = describeTxFailure( + TX_STAGE_VERIFY, + new Error("The wallet state could not be read."), + ); + expect(outcome.retryable).toBe(true); + expect(outcome.spendApproval).toBe(false); + }); + + // A broadcast that throws after the node took the transaction is routine: + // a timeout, a dropped response, a node answering "already known". The + // popup's retry does not re-broadcast the same bytes — it re-populates and + // re-signs at a freshly fetched nonce — so a retryable broadcast failure + // would put the approved transfer on the chain twice. + test("a failed broadcast is terminal, whatever the node said", () => { + for (const message of [ + "already known", + "timeout of 30000ms exceeded", + "could not coalesce error", + "replacement transaction underpriced", + ]) { + const outcome = describeTxFailure( + TX_STAGE_BROADCAST, + new Error(message), + ); + expect(outcome.retryable).toBe(false); + expect(outcome.spendApproval).toBe(true); + expect(outcome.error).toBe(message); + } + }); + + test("a failed broadcast does not tell the user to send it again", () => { + const outcome = describeSigningFailure( + { + error: "The node did not answer.", + retryable: false, + stage: TX_STAGE_BROADCAST, + }, + "The transaction could not be sent.", + ); + expect(outcome.retryable).toBe(false); + expect(outcome.message).toMatch(/may still have reached the network/); + expect(outcome.message).not.toMatch(/start it again from the site/); + }); +}); + // End-to-end over the messaging boundary, without a browser: run the exact // sequence the approval popup runs, then hand the artifact to the exact check // the background runs before it broadcasts or resolves. Only what the popup @@ -314,7 +1094,12 @@ describe("popup signing sequence to background verification", () => { test("a populated, signed transaction is accepted and broadcastable", async () => { const rawSignedTx = await popupSignsTx(TX_PARAMS); - const parsed = verifySignedTx(rawSignedTx, TX_PARAMS, signer.address); + const parsed = verifySignedTx( + rawSignedTx, + TX_PARAMS, + signer.address, + SELECTED, + ); expect(parsed.nonce).toBe(7); expect(parsed.chainId).toBe(1n); expect(parsed.gasLimit).toBe(21000n); @@ -349,7 +1134,14 @@ describe("popup signing sequence to background verification", () => { to: OTHER_RECIPIENT, }); expect(() => - verifySignedTx(rawSignedTx, TX_PARAMS, signer.address), + verifySignedTx(rawSignedTx, TX_PARAMS, signer.address, SELECTED), ).toThrow(/approved recipient/); }); + + test("the background rejects a transaction populated on another network", async () => { + const rawSignedTx = await popupSignsTx(TX_PARAMS); + expect(() => + verifySignedTx(rawSignedTx, TX_PARAMS, signer.address, SEPOLIA), + ).toThrow(/different network than the one that is selected/); + }); }); diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js new file mode 100644 index 0000000..ed313dd --- /dev/null +++ b/tests/backgroundApproval.test.js @@ -0,0 +1,679 @@ +// The background's approval message wiring, driven end to end: a dApp +// eth_sendTransaction raises a pending approval, and the popup answers it with +// AUTISTMASK_TX_RESPONSE / AUTISTMASK_SIGN_RESPONSE. +// +// What this exists for is the duplicate response. The handler verifies and +// broadcasts asynchronously, and the approval deliberately survives a +// retryable failure so the user can try again with the transaction they +// already saw — which means the entry being present is not by itself proof +// that no attempt is running. A second response carrying the same id (a +// reloaded approval window re-rendering a live Approve button, a popup that +// emits the message twice) must not start a second verify and broadcast: with +// the ordinary dApp approval shape the page fixes no nonce, so two artifacts +// signed at different nonces both verify, and the approved transfer would go +// out twice. + +const { Wallet } = require("ethers"); + +const SIGNER_KEY = + "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d"; +const signer = new Wallet(SIGNER_KEY); +const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; + +const ORIGIN = "https://dapp.example"; +const HOSTNAME = "dapp.example"; +const EXT_URL = "chrome-extension://autistmask/"; + +// What the dApp asks for: no nonce, no gas, no fees. This is the shape that +// makes a duplicate broadcast possible at all. +const TX_PARAMS = { + from: signer.address, + to: RECIPIENT, + value: "0x2386f26fc10000", + data: "0x", +}; + +// The fields the popup's populateTransaction() would fill in. The nonce is a +// parameter because the duplicate case turns on the two artifacts differing +// in exactly the field nothing constrains. +function populated(nonce) { + return { + type: 2, + chainId: 1, + nonce, + gasLimit: 100000n, + maxFeePerGas: 2000000000n, + maxPriorityFeePerGas: 1000000000n, + to: TX_PARAMS.to, + value: BigInt(TX_PARAMS.value), + data: TX_PARAMS.data, + }; +} + +function signedAtNonce(nonce) { + return signer.signTransaction(populated(nonce)); +} + +// A promise whose settlement the test controls, so a broadcast can be held in +// flight while the second response arrives. +function deferred() { + let resolve; + let reject; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +// Load the background worker against stubbed browser and network APIs and +// return the handles the tests drive it through. Everything that would touch +// the network or the browser's own schedulers is mocked; the approval +// verification is the real module, because that is what the handler under +// test is wired to. +function loadBackground(options) { + const opts = options || {}; + jest.resetModules(); + + const broadcastTransaction = jest.fn(); + const loadState = jest.fn(opts.loadState || (async () => {})); + + jest.doMock("../src/shared/state", () => ({ + state: { rpcUrl: "https://rpc.invalid", wallets: [] }, + loadState, + saveState: jest.fn(async () => {}), + currentNetwork: () => ({ chainId: "0x1" }), + })); + jest.doMock("../src/shared/balances", () => ({ + getProvider: () => ({ broadcastTransaction }), + refreshBalances: jest.fn(async () => {}), + })); + jest.doMock("../src/shared/phishingDomains", () => ({ + isPhishingDomain: () => false, + refreshPhishingListOnSchedule: jest.fn(async () => {}), + initPhishingList: jest.fn(async () => {}), + })); + jest.doMock("../src/shared/alarms", () => ({ + BALANCE_REFRESH_ALARM: "balance", + PHISHING_REFRESH_ALARM: "phishing", + BALANCE_REFRESH_PERIOD_MINUTES: 1, + ensureRecurringAlarms: jest.fn(async () => {}), + registerAlarmHandlers: jest.fn(), + })); + + const persisted = { + wallets: [ + { name: "Wallet 1", type: "hd", addresses: [signer.address] }, + ], + rpcUrl: "https://rpc.invalid", + activeAddress: signer.address, + allowedSites: { [signer.address]: [HOSTNAME] }, + deniedSites: {}, + }; + + let messageListener = null; + let windowRemovedListener = null; + const created = []; + const removed = []; + + global.chrome = { + storage: { + local: { + get: jest.fn(async () => ({ autistmask: persisted })), + set: jest.fn(async () => {}), + }, + }, + runtime: { + getURL: (path) => EXT_URL + path, + onMessage: { + addListener: (fn) => { + messageListener = fn; + }, + }, + onConnect: { addListener: () => {} }, + lastError: null, + }, + windows: { + getLastFocused: (cb) => cb(null), + create: (options2, cb) => { + created.push(options2); + cb({ id: created.length }); + }, + remove: (id, cb) => { + removed.push(id); + if (cb) cb(); + }, + // Captured, not swallowed: closing the approval window is the + // event that used to retire an approval out from under a live + // broadcast, and a no-op stub here hides exactly that. + onRemoved: { + addListener: (fn) => { + windowRemovedListener = fn; + }, + }, + }, + tabs: { + query: (q, cb) => cb([]), + sendMessage: () => {}, + }, + action: { setPopup: () => {} }, + }; + + require("../src/background/index"); + + // Send a message the way the browser would, and hand back whatever the + // handler passed to sendResponse. + function send(msg, sender) { + const sendResponse = jest.fn(); + const kept = messageListener(msg, sender || {}, sendResponse); + return { sendResponse, kept }; + } + + // Raise a pending transaction approval the way a dApp does, and dig the + // approval id back out of the popup URL the background opened. + function requestTx() { + let rpcResult = null; + const sendResponse = jest.fn((r) => { + rpcResult = r; + }); + messageListener( + { + type: "AUTISTMASK_RPC", + method: "eth_sendTransaction", + params: [TX_PARAMS], + }, + { origin: ORIGIN }, + sendResponse, + ); + return { + id: () => new URL(created[0].url).searchParams.get("approval"), + result: () => rpcResult, + }; + } + + // The user closes the approval popup. `created` is index-aligned with the + // ids the window stub hands back, so window 1 is the first popup opened. + function closeWindow(windowId) { + windowRemovedListener(windowId); + } + + return { + send, + requestTx, + closeWindow, + broadcastTransaction, + loadState, + created, + removed, + fromPopup: { url: EXT_URL + "src/popup/index.html" }, + }; +} + +// Let the handler's promise chain run to the next suspension point. +async function settle() { + for (let i = 0; i < 10; i++) await Promise.resolve(); +} + +afterEach(() => { + delete global.chrome; + jest.resetModules(); +}); + +describe("one approval, one broadcast", () => { + test("a second AUTISTMASK_TX_RESPONSE for the same id does not broadcast again", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + const id = pending.id(); + expect(id).toBeTruthy(); + + const inFlight = deferred(); + bg.broadcastTransaction.mockReturnValue(inFlight.promise); + + // The popup answers. Verification passes and the broadcast is held + // open, which is the whole window the second message arrives in. + const first = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(7), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + + // A reloaded approval window signs the same approval again. Nothing + // in the approval fixes a nonce, so this artifact verifies just as + // well as the first one. + const second = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(8), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + expect(second.sendResponse).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringMatching(/already being sent/), + retryable: false, + }), + ); + + inFlight.resolve({ hash: "0xfeed" }); + await settle(); + expect(first.sendResponse).toHaveBeenCalledWith({ txHash: "0xfeed" }); + expect(pending.result()).toEqual({ result: "0xfeed" }); + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + }); + + test("the same artifact sent twice broadcasts once", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + const id = pending.id(); + + const inFlight = deferred(); + bg.broadcastTransaction.mockReturnValue(inFlight.promise); + const raw = await signedAtNonce(7); + const msg = { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: raw, + }; + + bg.send(msg, { url: bg.fromPopup.url }); + bg.send(msg, { url: bg.fromPopup.url }); + await settle(); + inFlight.resolve({ hash: "0xfeed" }); + await settle(); + + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + }); + + test("a response arriving after the broadcast finished finds nothing to send", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + const id = pending.id(); + + bg.broadcastTransaction.mockResolvedValue({ hash: "0xfeed" }); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(7), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + const late = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(8), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + expect(late.sendResponse).not.toHaveBeenCalled(); + }); + + test("a second AUTISTMASK_SIGN_RESPONSE for the same id is refused", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + const id = pending.id(); + + // Hold the transaction approval in flight, then answer it a second + // time as if it were a sign approval: the sign handler must apply the + // same interlock rather than running its own verification. + const inFlight = deferred(); + bg.broadcastTransaction.mockReturnValue(inFlight.promise); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(7), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + const second = bg.send( + { + type: "AUTISTMASK_SIGN_RESPONSE", + id, + approved: true, + signature: "0x00", + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + expect(second.sendResponse).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringMatching(/already being signed/), + retryable: false, + }), + ); + inFlight.resolve({ hash: "0xfeed" }); + await settle(); + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + }); +}); + +// The interlock must not cost the retry the approval exists to allow. +describe("the interlock releases a failed attempt", () => { + test("a retryable failure before the broadcast leaves the approval usable", async () => { + let failNext = true; + const bg = loadBackground({ + loadState: async () => { + if (failNext) { + failNext = false; + throw new Error("storage unavailable"); + } + }, + }); + const pending = bg.requestTx(); + await settle(); + const id = pending.id(); + + const first = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(7), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(bg.broadcastTransaction).not.toHaveBeenCalled(); + expect(first.sendResponse).toHaveBeenCalledWith( + expect.objectContaining({ retryable: true }), + ); + + bg.broadcastTransaction.mockResolvedValue({ hash: "0xfeed" }); + const retry = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(7), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + expect(retry.sendResponse).toHaveBeenCalledWith({ txHash: "0xfeed" }); + expect(pending.result()).toEqual({ result: "0xfeed" }); + }); + + test("a mismatched artifact spends the approval outright", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + const id = pending.id(); + + // Signed for a different recipient than the one that was approved. + const wrong = await signer.signTransaction({ + ...populated(7), + to: "0xdAC17F958D2ee523a2206206994597C13D831ec7", + }); + const first = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: wrong, + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(first.sendResponse).toHaveBeenCalledWith( + expect.objectContaining({ retryable: false, stage: "verify" }), + ); + + const retry = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(7), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + expect(bg.broadcastTransaction).not.toHaveBeenCalled(); + expect(retry.sendResponse).not.toHaveBeenCalled(); + }); +}); + +// The claim is what makes one approval one broadcast, so it has to hold +// against everything else that retires an approval, not just against a second +// AUTISTMASK_TX_RESPONSE. Each of these paths used to resolve the waiting +// promise 4001 while the attempt behind it ran to completion: the transaction +// reached the chain and the page was told the user rejected it, which invites +// the user to send it a second time at a fresh nonce. +describe("a claimed approval outlives every other retirement path", () => { + // The approval popup stays open across the broadcast it is waiting on, so + // a user closing an apparently-hung window needs no adversary at all. + test("closing the approval window mid-broadcast still reports the result", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + const id = pending.id(); + + const inFlight = deferred(); + bg.broadcastTransaction.mockReturnValue(inFlight.promise); + const first = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(7), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + + // The user closes the window while the broadcast is still open. + bg.closeWindow(1); + await settle(); + expect(pending.result()).toBeNull(); + + inFlight.resolve({ hash: "0xfeed" }); + await settle(); + + expect(pending.result()).toEqual({ result: "0xfeed" }); + expect(first.sendResponse).toHaveBeenCalledWith({ txHash: "0xfeed" }); + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + }); + + test("switching the active address mid-broadcast still reports the result", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + const id = pending.id(); + + const inFlight = deferred(); + bg.broadcastTransaction.mockReturnValue(inFlight.promise); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(7), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + + // The user switches account in the toolbar popup, which rejects and + // force-closes every pending approval. + bg.send( + { type: "AUTISTMASK_ACTIVE_CHANGED" }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(pending.result()).toBeNull(); + // The window an in-flight attempt reports into is left standing too. + expect(bg.removed).toEqual([]); + + inFlight.resolve({ hash: "0xfeed" }); + await settle(); + + expect(pending.result()).toEqual({ result: "0xfeed" }); + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + }); + + test("a reject arriving mid-broadcast is refused, not honoured", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + const id = pending.id(); + + const inFlight = deferred(); + bg.broadcastTransaction.mockReturnValue(inFlight.promise); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(7), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + const reject = bg.send( + { type: "AUTISTMASK_TX_RESPONSE", id, approved: false }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(pending.result()).toBeNull(); + expect(reject.sendResponse).toHaveBeenCalledWith( + expect.objectContaining({ + retryable: false, + stage: "broadcast", + }), + ); + + inFlight.resolve({ hash: "0xfeed" }); + await settle(); + + expect(pending.result()).toEqual({ result: "0xfeed" }); + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + }); + + // The refusals above must not cost the rejection its ordinary meaning. + test("with no attempt running, closing the window still rejects", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + + bg.closeWindow(1); + await settle(); + + expect(pending.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + expect(bg.broadcastTransaction).not.toHaveBeenCalled(); + }); + + test("with no attempt running, an active-address switch still rejects and closes", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + + bg.send( + { type: "AUTISTMASK_ACTIVE_CHANGED" }, + { url: bg.fromPopup.url }, + ); + await settle(); + + expect(pending.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + expect(bg.removed).toEqual([1]); + }); + + // A sign approval held by a running verification is the same shape, and + // the refusal must not tell the user to start again from the site while + // the first attempt may still hand back a signature. + test("a reject during a sign attempt is refused with the in-flight stage", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + const id = pending.id(); + + const inFlight = deferred(); + bg.broadcastTransaction.mockReturnValue(inFlight.promise); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(7), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + const reject = bg.send( + { type: "AUTISTMASK_SIGN_RESPONSE", id, approved: false }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(reject.sendResponse).toHaveBeenCalledWith( + expect.objectContaining({ retryable: false, stage: "inflight" }), + ); + + inFlight.resolve({ hash: "0xfeed" }); + await settle(); + expect(pending.result()).toEqual({ result: "0xfeed" }); + }); +}); + +describe("popup-only messages", () => { + test("a page sender cannot answer an approval", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + const id = pending.id(); + + const spoof = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(7), + }, + { url: ORIGIN + "/index.html" }, + ); + await settle(); + + expect(bg.broadcastTransaction).not.toHaveBeenCalled(); + expect(spoof.sendResponse).toHaveBeenCalledWith({ + error: "Unauthorized sender", + }); + }); +});