diff --git a/README.md b/README.md index 03edbec..76fd568 100644 --- a/README.md +++ b/README.md @@ -1200,7 +1200,11 @@ on ConfirmTx, DeleteWallet, ApproveTx and ApproveSign. opening the window, so the screen shows a complete transaction and the signed artifact can be compared with it field for field. A request that cannot be populated — unreachable node, reverting gas estimate — opens no window and is - failed back to the site. + failed back to the site. Only one transaction approval exists at a time: + populating fixes the nonce, so a second `eth_sendTransaction` arriving while + one is unanswered is refused with EIP-1193 code `-32002` rather than being + populated at the same nonce. It opens no window and takes no nonce, and the + site can send it again once the pending one is answered. - **Elements**: - "Transaction Request" heading - Phishing warning banner (shown when the hostname is on the phishing diff --git a/TODO.md b/TODO.md index a0a78ff..b448a45 100644 --- a/TODO.md +++ b/TODO.md @@ -70,6 +70,31 @@ undefined identifiers, which is how stubbed RPC and the unobservable toolbar popup — were checked against the current `src/content/inpage.js` and `tests/e2e/` and left as they are ([#285](https://git.eeqj.de/sneak/AutistMask/issues/285)). +- 2026-08-17: One transaction approval at a time. Populating in the background + before the window opens is what makes the displayed object the verified + object, and it also fixes the nonce: two `eth_sendTransaction` calls populated + concurrently took the same nonce from a node that had seen neither broadcast, + and the second could then never be sent, because the only way to give it a + fresh nonce is to populate it again after the user has read the old one off + the screen. A second request is now refused with EIP-1193 `-32002` while one + is unanswered — the slot is taken immediately before population, after the + authorization checks, so no second nonce is allocated, no second window opens, + and a page the wallet refuses anyway cannot hold the slot against the + connected site. The slot is freed at `settleApproval()`, the single point an + approval is retired, so every path that ends an approval ends the hold with + it; an approval whose window is gone and whose attempt has failed is settled + there rather than left waiting on a window that no longer exists, and an + approval window that could not be opened at all is answered with `-32603` + instead of holding the page's promise open. Signature approvals are not gated, + consuming no nonce. A collision that does happen is also reported accurately + now: a broadcast the node refused for the nonce, and an approval carrying a + nonce this worker has already broadcast for that address on that chain (caught + before the node is asked at all), both say the transaction did not reach the + network and to send it again, instead of warning that it may have sent. The + record is keyed by chain as well as address, because nonce spaces are per + chain and low nonces overlap across them. `already known` deliberately keeps + the ambiguous wording, because a node that says it has the transaction has it + ([#271](https://git.eeqj.de/sneak/AutistMask/issues/271)). - 2026-08-14: The parts of the [#150](https://git.eeqj.de/sneak/AutistMask/issues/150) and [#151](https://git.eeqj.de/sneak/AutistMask/issues/151) definition of done the diff --git a/src/background/index.js b/src/background/index.js index f89e386..cbb6f40 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -24,6 +24,7 @@ const { TX_STAGE_VERIFY, TX_STAGE_BROADCAST, TX_STAGE_INFLIGHT, + TX_STAGE_NONCE, } = require("../shared/approvalVerify"); const { prepareApprovalTx } = require("../shared/approvalTx"); const { @@ -57,6 +58,114 @@ const connectedSites = {}; // Pending approval requests: { id: { origin, hostname, resolve } } const pendingApprovals = {}; +// One transaction approval at a time, wallet-wide. +// +// The transaction a site asks for is populated before its approval window +// opens, so that the object the user is shown is the object the signed +// artifact is verified against. Populating fixes the nonce. Two requests +// populated concurrently therefore take the SAME nonce — the node reports the +// same pending count to both, neither having been broadcast — and whichever is +// broadcast second is refused by the network for a nonce it can never be +// re-signed at, because re-signing it would mean signing something other than +// what was displayed. +// +// So the second request is refused while the first is unanswered. It is +// refused before anything is populated, so no second nonce is allocated at +// all, and while the page is still waiting with nothing on screen. The +// alternatives were considered and rejected in +// https://git.eeqj.de/sneak/AutistMask/issues/271: populating again at Confirm +// puts a nonce on screen that is not the nonce that gets signed, and +// allocating around in-flight approvals makes the wallet's own bookkeeping the +// authority on a nonce the network has not accepted, which an abandoned +// approval then leaves a hole in. +// +// Sign approvals are not gated: a signature consumes no nonce. +// +// The slot is null when free, and otherwise the handle of the request holding +// it. Once that request has raised its approval the handle carries the +// approval's id, so that retiring the approval frees the slot: every exit from +// pendingApprovals goes through settleApproval(), which makes that one hook +// complete. The holder's own finally is the backstop for the interval before +// the approval exists. +let txApprovalSlot = null; + +// EIP-1474 "resource unavailable": the standard code for a request that is +// refused because another one is already pending. +const TX_APPROVAL_PENDING_CODE = -32002; + +// True at every moment this can be sent: the slot is taken immediately before +// the transaction is populated, so the other request is either being prepared +// or on screen. It does not claim the other one is displayed yet, because for +// the length of one network round trip it is not. +const TX_APPROVAL_PENDING_MESSAGE = + "AutistMask handles one transaction at a time, and another one is" + + " already in progress, so this one was not sent. Please finish that" + + " transaction, then send this one again."; + +// Take the slot, or refuse. Nothing awaits between the test and the set, so +// two requests that reach this in the same tick cannot both pass it — the +// position of the call in the handler is irrelevant to that, which is why it +// sits after the authorization checks. A page the wallet is going to refuse +// anyway must not be able to take the slot away from the connected site. +function reserveTxApprovalSlot() { + if (txApprovalSlot) return null; + txApprovalSlot = { approvalId: null }; + return txApprovalSlot; +} + +// Free the slot, if this handle is still the one holding it. +function releaseTxApprovalSlot(handle) { + if (handle && txApprovalSlot !== handle) return; + txApprovalSlot = null; +} + +// Free the slot held on behalf of a retired approval. Called from +// settleApproval() for every approval, and a no-op for the ones the slot was +// not taken for. +function releaseTxApprovalSlotFor(approvalId) { + if (txApprovalSlot && txApprovalSlot.approvalId === approvalId) { + txApprovalSlot = null; + } +} + +// Nonces this worker has already handed to the node, per chain and address. +// This is the wallet's own knowledge that a nonce is spent, and it is checked +// before a broadcast rather than after: a node's pending count can lag a +// transaction it has itself just accepted, and a request populated inside that +// window would otherwise be signed and sent at a nonce this wallet has already +// used. +// +// The chain is part of the key because nonce spaces are per chain and the +// wallet switches networks. Without it a nonce spent on one chain would refuse +// that nonce on every other chain — and low nonces overlap across chains as a +// matter of course, so the refusal would be both routine and false. +// +// The record dies with the worker, which is correct rather than merely +// convenient: after a restart the node's count is the only answer available, +// and a transaction of this wallet's that the node has forgotten is one the +// user does want to be able to send again. +const broadcastNonces = {}; + +function broadcastNoncesFor(chainId, address) { + const key = + String(chainId).toLowerCase() + + ":" + + String(address || "").toLowerCase(); + if (!broadcastNonces[key]) broadcastNonces[key] = new Set(); + return broadcastNonces[key]; +} + +// An approved transaction's nonce as a decimal string, or null if it cannot be +// read as a number. Verification refuses an unreadable nonce before this is +// ever reached; null here only keeps the record from holding junk. +function approvedNonce(approvedTx) { + try { + return BigInt(approvedTx.nonce).toString(); + } catch { + return null; + } +} + async function getState() { const result = await storageApi.get("autistmask"); return ( @@ -148,11 +257,41 @@ function settleApproval(id, result, options) { const holdsClaim = !!(options && options.holdsClaim); if (approval.attemptInFlight && !holdsClaim) return false; delete pendingApprovals[id]; + // The transaction-approval slot is held for exactly as long as the + // approval it was taken for is alive, and this is the one place an + // approval stops being alive. + releaseTxApprovalSlotFor(id); approval.resolve(result); resetPopupUrl(); return true; } +// What a pending approval resolves to when it is given up on rather than +// answered: the window was closed, or could not be opened at all. A tx or sign +// approval answers the requesting page in EIP-1193 shape; a site-connection +// approval answers the connection handler in its own. +function abandonedResult(approval, code, message) { + if (approval.type === "tx" || approval.type === "sign") { + return { error: { code, message } }; + } + return { approved: false, remember: false }; +} + +// A window the user closed without answering is a refusal by the user, which +// is 4001 and the wording every other rejection path already uses. +const APPROVAL_REJECTED_CODE = 4001; +const APPROVAL_REJECTED_MESSAGE = "User rejected the request."; + +// The window could not be opened, so the user was never asked. This is the +// wallet failing, not the user refusing, so it does not claim to be a +// rejection: -32603 is the JSON-RPC code for the wallet's own internal +// failure, and the page is told plainly that nothing was shown. +const APPROVAL_WINDOW_FAILED_CODE = -32603; + +const APPROVAL_WINDOW_FAILED_MESSAGE = + "AutistMask could not open its approval window, so this request was not" + + " shown to you and nothing was sent."; + // Take exclusive hold of a pending approval for one attempt, or refuse. // // An approval that failed retryably has to stay in pendingApprovals, so its @@ -172,8 +311,26 @@ function claimApproval(approval) { // Release an approval whose attempt failed in a way the user can retry. // Nothing was broadcast, so the next attempt may claim it. +// +// Unless the window it would be retried in is already gone. The user closed it +// while the attempt was running and settleApproval() declined then, correctly, +// because the attempt still owned the approval; the attempt has now failed, so +// nothing owns it and nothing can reach it. Left standing it would hold the +// requesting page's promise open forever and, with it, the transaction +// approval slot. It is settled here as the rejection the closed window +// already meant. function releaseApproval(approval) { approval.attemptInFlight = false; + if (approval.windowClosed) { + settleApproval( + approval.id, + abandonedResult( + approval, + APPROVAL_REJECTED_CODE, + APPROVAL_REJECTED_MESSAGE, + ), + ); + } } // Open approval in a separate popup window. @@ -200,9 +357,35 @@ function openApprovalWindow(id) { ); } windowsApi.create(opts, (win) => { - if (win) { - pendingApprovals[id].windowId = win.id; + const approval = pendingApprovals[id]; + if (!approval) { + // Settled while the window was opening — an address switch, + // say. Nothing is waiting on it, and a window showing an + // approval that no longer exists is not left on screen. + if (win) { + windowsApi.remove(win.id, () => { + if (runtime.lastError) { + // window already closed + } + }); + } + return; } + if (!win) { + // No window means no way to ever answer this approval, and an + // approval nothing can answer holds the requesting page's + // promise open forever. Settle it now instead. + settleApproval( + id, + abandonedResult( + approval, + APPROVAL_WINDOW_FAILED_CODE, + APPROVAL_WINDOW_FAILED_MESSAGE, + ), + ); + return; + } + approval.windowId = win.id; }); }); } @@ -212,7 +395,7 @@ function openApprovalWindow(id) { function requestApproval(origin, hostname) { return new Promise((resolve) => { const id = crypto.randomUUID(); - pendingApprovals[id] = { origin, hostname, resolve }; + pendingApprovals[id] = { id, origin, hostname, resolve }; if (actionApi && typeof actionApi.openPopup === "function") { actionApi.setPopup({ @@ -243,10 +426,13 @@ function requestApproval(origin, hostname) { // it is pinned here rather than read again at signing time — an address switch // between approval and signing must refuse, not sign from an account this // screen never named. -function requestTxApproval(origin, hostname, approvedTx, approvedFrom) { +// `slot` is the transaction-approval slot its caller holds. Handing the +// approval's id to it is what makes retiring the approval free the slot. +function requestTxApproval(origin, hostname, approvedTx, approvedFrom, slot) { return new Promise((resolve) => { const id = crypto.randomUUID(); pendingApprovals[id] = { + id, origin, hostname, approvedTx, @@ -254,6 +440,7 @@ function requestTxApproval(origin, hostname, approvedTx, approvedFrom) { resolve, type: "tx", }; + if (slot) slot.approvalId = id; openApprovalWindow(id); }); @@ -267,6 +454,7 @@ function requestSignApproval(origin, hostname, signParams, approvedFrom) { return new Promise((resolve) => { const id = crypto.randomUUID(); pendingApprovals[id] = { + id, origin, hostname, signParams, @@ -585,31 +773,68 @@ async function handleRpc(method, params, origin) { } if (method === "eth_sendTransaction") { - const s = await getState(); - const activeAddress = await getActiveAddress(); - if (!activeAddress) - return { error: { message: "No accounts available" } }; + return await handleSendTransaction(params, origin); + } - const hostname = extractHostname(origin); - const allowed = s.allowedSites[activeAddress] || []; - if ( - !allowed.includes(hostname) && - !connectedSites[origin + ":" + activeAddress] - ) { - return { error: { code: 4100, message: "Unauthorized" } }; + // Proxy safe read-only methods to the RPC node + if (PROXY_METHODS.includes(method)) { + try { + const result = await proxyRpc(method, params); + return { result }; + } catch (e) { + return { error: { message: e.message } }; } + } - const txParams = params?.[0] || {}; - if (namesAnotherAddress(txParams.from, activeAddress)) { - return { - error: { - code: 4100, - message: - "This site asked to send from an address that is not the active one.", - }, - }; - } + return { error: { message: "Unsupported method: " + method } }; +} +// The body of eth_sendTransaction, from the connection check through to the +// user's decision. It takes the single transaction-approval slot once it knows +// it is going to populate a transaction, and holds it until the requesting +// page has its answer. +async function handleSendTransaction(params, origin) { + const s = await getState(); + const activeAddress = await getActiveAddress(); + if (!activeAddress) return { error: { message: "No accounts available" } }; + + const hostname = extractHostname(origin); + const allowed = s.allowedSites[activeAddress] || []; + if ( + !allowed.includes(hostname) && + !connectedSites[origin + ":" + activeAddress] + ) { + return { error: { code: 4100, message: "Unauthorized" } }; + } + + const txParams = params?.[0] || {}; + if (namesAnotherAddress(txParams.from, activeAddress)) { + return { + error: { + code: 4100, + message: + "This site asked to send from an address that is not the active one.", + }, + }; + } + + // Everything above refuses without populating anything, so the slot is + // taken here rather than at the top of the handler: a page the wallet was + // never going to serve must not be able to hold the slot and make the + // connected site's own transaction fail as "already in progress". The + // reservation is atomic because nothing awaits between its test and its + // set, not because of where it sits. + const slot = reserveTxApprovalSlot(); + if (!slot) { + return { + error: { + code: TX_APPROVAL_PENDING_CODE, + message: TX_APPROVAL_PENDING_MESSAGE, + }, + }; + } + + try { // Populate here, before any window opens, so that the transaction the // user is shown is a complete one and is the same object the signed // artifact is checked against. A failure raises no approval at all and @@ -644,22 +869,17 @@ async function handleRpc(method, params, origin) { hostname, approvedTx, activeAddress, + slot, ); if (decision.error) return { error: decision.error }; return { result: decision.txHash }; + } finally { + // Retiring the approval has normally freed the slot already, through + // settleApproval(); this covers the paths that return before an + // approval exists at all, and frees nothing if another request has + // since taken the slot. + releaseTxApprovalSlot(slot); } - - // Proxy safe read-only methods to the RPC node - if (PROXY_METHODS.includes(method)) { - try { - const result = await proxyRpc(method, params); - return { result }; - } catch (e) { - return { error: { message: e.message } }; - } - } - - return { error: { message: "Unsupported method: " + method } }; } // Broadcast chainChanged to all tabs when the network is switched. @@ -694,15 +914,11 @@ async function broadcastAccountsChanged() { // 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)) { - const rejection = - approval.type === "tx" || approval.type === "sign" - ? { - error: { - code: 4001, - message: "User rejected the request.", - }, - } - : { approved: false, remember: false }; + const rejection = abandonedResult( + approval, + APPROVAL_REJECTED_CODE, + APPROVAL_REJECTED_MESSAGE, + ); if (!settleApproval(id, rejection)) continue; if (approval.windowId) { windowsApi.remove(approval.windowId, () => { @@ -831,21 +1047,19 @@ startBackgroundJobs(); // 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. +// outcome to the page — and the window is recorded as gone, so that an attempt +// which then fails retryably settles instead of waiting in a window that no +// longer exists. if (windowsApi && windowsApi.onRemoved) { windowsApi.onRemoved.addListener((windowId) => { for (const [id, approval] of Object.entries(pendingApprovals)) { 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); + const rejection = abandonedResult( + approval, + APPROVAL_REJECTED_CODE, + APPROVAL_REJECTED_MESSAGE, + ); + if (!settleApproval(id, rejection)) approval.windowClosed = true; } }); } @@ -959,7 +1173,7 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { sendResponse({ error: outcome.error, retryable: outcome.retryable, - stage: TX_STAGE_SIGN, + stage: outcome.stage, }); return false; } @@ -975,8 +1189,15 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { } (async () => { + // The chain this attempt is on, read once. Verification below + // refuses an artifact signed for any other chain, and the nonce + // record is both consulted and written under this one, so a + // network switch part-way through cannot make the check and the + // record disagree about which chain the nonce was spent on. + let chainId; try { await loadState(); + chainId = currentNetwork().chainId; const activeAddress = await getActiveAddress(); // An address switch between approval and signing refuses. The // approval named one account; signing from whichever account @@ -999,7 +1220,7 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { msg.rawSignedTx, approval.approvedTx, approval.approvedFrom, - currentNetwork().chainId, + chainId, ); } catch (e) { // A signed transaction that is not the approved one is not @@ -1019,7 +1240,31 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { sendResponse({ error: outcome.error, retryable: outcome.retryable, - stage: TX_STAGE_VERIFY, + stage: outcome.stage, + }); + return; + } + + // A nonce this worker has already broadcast for this address on + // this chain. The node is not asked: it has answered once already, + // and the wallet holding the receipt of that answer is what makes + // this failure one the user can be told did not reach the network. + // A nonce spent on another chain is not spent here — the chains + // count separately, and refusing across them would block ordinary + // use with a message that is not true. + const nonce = approvedNonce(approval.approvedTx); + const spent = broadcastNoncesFor(chainId, approval.approvedFrom); + if (nonce !== null && spent.has(nonce)) { + const outcome = describeTxFailure(TX_STAGE_NONCE, null); + settleApproval( + msg.id, + { error: { message: outcome.error } }, + { holdsClaim: true }, + ); + sendResponse({ + error: outcome.error, + retryable: outcome.retryable, + stage: outcome.stage, }); return; } @@ -1027,6 +1272,7 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { try { const provider = getProvider(state.rpcUrl); const tx = await provider.broadcastTransaction(msg.rawSignedTx); + if (nonce !== null) spent.add(nonce); settleApproval( msg.id, { txHash: tx.hash }, @@ -1039,6 +1285,11 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { // tell a transaction that never left from one already in the // mempool. The page has been given its outcome for this // request; a second attempt would report a second one. + // + // Unless the node blamed the nonce, which is the one answer + // that says plainly it did not take the transaction: + // describeTxFailure() reclassifies that, and the stage it + // returns is the one reported. const outcome = describeTxFailure(TX_STAGE_BROADCAST, e); settleApproval( msg.id, @@ -1048,7 +1299,7 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { sendResponse({ error: outcome.error, retryable: outcome.retryable, - stage: TX_STAGE_BROADCAST, + stage: outcome.stage, }); } })(); diff --git a/src/shared/approvalVerify.js b/src/shared/approvalVerify.js index d2fea3e..18d2878 100644 --- a/src/shared/approvalVerify.js +++ b/src/shared/approvalVerify.js @@ -602,6 +602,12 @@ const TX_STAGE_BROADCAST = "broadcast"; // may yet succeed, so the one thing the popup must not say is "start again // from the site". const TX_STAGE_INFLIGHT = "inflight"; +// A transaction refused for a nonce that is already spoken for, either by the +// node's own answer or by this wallet's record of what it has broadcast. It is +// the one broadcast-stage failure that is not ambiguous: the transaction was +// not taken, so the user is told it did not reach the network and to send it +// again, rather than being warned that it might already be out there. +const TX_STAGE_NONCE = "nonce"; function errorText(err) { if (typeof err === "string" && err !== "") return err; @@ -611,6 +617,59 @@ function errorText(err) { return "The transaction could not be sent."; } +// Every string a failure might carry its reason in. ethers reports the node's +// own words in `shortMessage`, but a JSON-RPC error it could not classify is +// nested under `error` or `info.error` with the node's message intact, and the +// classification below has to see that too. +function failureTexts(err) { + if (typeof err === "string") return [err]; + if (!err || typeof err !== "object") return []; + const texts = []; + for (const text of [err.shortMessage, err.message, err.reason]) { + if (text) texts.push(String(text)); + } + const nested = err.error || (err.info && err.info.error); + if (nested && nested.message) texts.push(String(nested.message)); + return texts; +} + +// What the Ethereum clients say when a transaction's nonce is already spoken +// for: either it is below the account's next nonce, or another transaction is +// sitting in the pool at that nonce and this one did not outbid it. Either way +// the node answered, and its answer was that it did not take this transaction. +// +// "already known" is deliberately absent. A node that says it knows the +// transaction has it, so that transaction did reach the network and the +// ambiguous broadcast wording is the correct one for it. +const NONCE_COLLISION_PATTERNS = [ + /nonce too low/i, + /nonce has already been used/i, + /invalid nonce/i, + /oldnonce/i, + /replacement transaction underpriced/i, + /replacement fee too low/i, +]; + +// ethers' own classification of the same two conditions. +const NONCE_COLLISION_CODES = ["NONCE_EXPIRED", "REPLACEMENT_UNDERPRICED"]; + +// Whether a failed send is a nonce collision. +function isNonceCollision(err) { + if (!err) return false; + if (err.code && NONCE_COLLISION_CODES.includes(err.code)) return true; + return failureTexts(err).some((text) => + NONCE_COLLISION_PATTERNS.some((pattern) => pattern.test(text)), + ); +} + +// What both the requesting page and the popup are told about a nonce +// collision. The node's own words ("nonce too low") are a fragment and are +// replaced rather than passed through: they are not a sentence, and they say +// less than the wallet knows. +const NONCE_COLLISION_MESSAGE = + "The transaction was not sent, because its nonce had already been used" + + " by another transaction."; + // 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 @@ -627,12 +686,31 @@ function errorText(err) { // that never left from one that is already in the mempool. The approval is // spent and the requesting page has been given its outcome; a second // attempt against it would report a second outcome for one request. +// - nonce: terminal too, and the one case where the wallet does know the +// transaction never left. The approval carries a nonce that is spent, so +// the artifact signed against it can never be accepted and the user is told +// to send it again from the site. +// +// The stage comes back out because a broadcast failure the node blamed on the +// nonce is reclassified here; the caller reports the stage this returns rather +// than the one it passed in. function describeTxFailure(stage, err) { + if ( + stage === TX_STAGE_NONCE || + (stage === TX_STAGE_BROADCAST && isNonceCollision(err)) + ) { + return { + error: NONCE_COLLISION_MESSAGE, + retryable: false, + spendApproval: true, + stage: TX_STAGE_NONCE, + }; + } const error = errorText(err); const retryable = stage === TX_STAGE_SIGN || (stage === TX_STAGE_VERIFY && failureIsRetryable(err)); - return { error, retryable, spendApproval: !retryable }; + return { error, retryable, spendApproval: !retryable, stage }; } // What the popup shows and does after the background reports a failed signing @@ -642,14 +720,20 @@ function describeTxFailure(stage, err) { // // 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. +// wrong instruction. A nonce collision is the exception to that exception — +// the transaction demonstrably did not go out, and saying it might have would +// send the user hunting for a transaction that does not exist. 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) { + if (stage === TX_STAGE_NONCE) { + message += + " The transaction did not reach the network." + + " Please send it again from the site."; + } else if (stage === TX_STAGE_BROADCAST) { message += " The transaction may still have reached the network." + " Check the account before sending it again."; @@ -675,9 +759,11 @@ module.exports = { assertWithinCeilings, sameAddress, failureIsRetryable, + isNonceCollision, describeTxFailure, describeSigningFailure, ApprovalMismatchError, + NONCE_COLLISION_MESSAGE, ALLOWED_TX_TYPES, SERIALIZED_FIELDS, FORBIDDEN_FIELDS, @@ -686,6 +772,7 @@ module.exports = { TX_STAGE_VERIFY, TX_STAGE_BROADCAST, TX_STAGE_INFLIGHT, + TX_STAGE_NONCE, MAX_GAS_LIMIT, MAX_FEE_PER_GAS, }; diff --git a/tests/approvalVerify.test.js b/tests/approvalVerify.test.js index 99fc106..1763b3d 100644 --- a/tests/approvalVerify.test.js +++ b/tests/approvalVerify.test.js @@ -14,8 +14,10 @@ const { assertWithinCeilings, sameAddress, failureIsRetryable, + isNonceCollision, describeTxFailure, describeSigningFailure, + NONCE_COLLISION_MESSAGE, ALLOWED_TX_TYPES, SERIALIZED_FIELDS, FORBIDDEN_FIELDS, @@ -23,6 +25,7 @@ const { TX_STAGE_SIGN, TX_STAGE_VERIFY, TX_STAGE_BROADCAST, + TX_STAGE_NONCE, MAX_GAS_LIMIT, MAX_FEE_PER_GAS, } = require("../src/shared/approvalVerify"); @@ -1191,7 +1194,6 @@ describe("signing failure and retry", () => { "already known", "timeout of 30000ms exceeded", "could not coalesce error", - "replacement transaction underpriced", ]) { const outcome = describeTxFailure( TX_STAGE_BROADCAST, @@ -1199,10 +1201,76 @@ describe("signing failure and retry", () => { ); expect(outcome.retryable).toBe(false); expect(outcome.spendApproval).toBe(true); + expect(outcome.stage).toBe(TX_STAGE_BROADCAST); expect(outcome.error).toBe(message); } }); + // The one broadcast failure that is not ambiguous. The node answered, and + // its answer was that the nonce was already spoken for, so this + // transaction is not in a mempool anywhere. + test("a nonce the node refused is classified however it was worded", () => { + for (const err of [ + new Error("nonce too low"), + new Error("replacement transaction underpriced"), + Object.assign(new Error("could not coalesce error"), { + code: "NONCE_EXPIRED", + }), + Object.assign(new Error("could not coalesce error"), { + code: "REPLACEMENT_UNDERPRICED", + }), + // The shape ethers hands up when it could not classify the node's + // error itself: the node's own words are nested underneath. + Object.assign(new Error("could not coalesce error"), { + info: { error: { code: -32000, message: "OldNonce" } }, + }), + ]) { + const outcome = describeTxFailure(TX_STAGE_BROADCAST, err); + expect( + describeSigningFailure( + outcome, + "The transaction could not be sent.", + ).message, + ).toMatch(/did not reach the network/); + expect(outcome.retryable).toBe(false); + expect(outcome.spendApproval).toBe(true); + expect(outcome.error).toBe(NONCE_COLLISION_MESSAGE); + expect(outcome.stage).toBe(TX_STAGE_NONCE); + expect(isNonceCollision(err)).toBe(true); + } + }); + + // A node that says it knows the transaction has it, so it did reach the + // network and the ambiguous wording is the correct one. + test("already known is not a nonce collision", () => { + const err = new Error("already known"); + const outcome = describeTxFailure(TX_STAGE_BROADCAST, err); + expect( + describeSigningFailure( + outcome, + "The transaction could not be sent.", + ).message, + ).toMatch(/may still have reached the network/); + expect(outcome.stage).toBe(TX_STAGE_BROADCAST); + expect(isNonceCollision(err)).toBe(false); + }); + + test("a nonce collision says the transaction did not reach the network", () => { + const outcome = describeTxFailure( + TX_STAGE_BROADCAST, + new Error("nonce too low"), + ); + const copy = describeSigningFailure( + outcome, + "The transaction could not be sent.", + ); + expect(copy.retryable).toBe(false); + expect(copy.message).toMatch(/did not reach the network/); + expect(copy.message).not.toMatch(/may still have reached the network/); + expect(copy.message).toMatch(/Please send it again from the site\.$/); + expect(copy.message).toMatch(/^[A-Z].*\.$/); + }); + test("a failed broadcast does not tell the user to send it again", () => { const outcome = describeSigningFailure( { diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index d326b81..e2b0ec3 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -30,6 +30,8 @@ const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; const ORIGIN = "https://dapp.example"; const HOSTNAME = "dapp.example"; +// A page the wallet has never been connected to, whose requests are refused. +const UNCONNECTED_ORIGIN = "https://stranger.example"; const EXT_URL = "chrome-extension://autistmask/"; // What the dApp asks for: no nonce, no gas, no fees. This is the shape that @@ -51,10 +53,16 @@ const MESSAGE = "0x48656c6c6f204175746973744d61736b"; // The transaction the background populates and the approval screen displays. // The nonce is a parameter because the duplicate case turns on two artifacts // differing in a field the dApp fixed nothing for. -function populated(nonce) { +// The two chains the tests switch between, as both forms the code uses: the +// hex chain id the wallet's network record carries, and the number the node +// and the signed artifact carry. +const MAINNET = { hex: "0x1", num: 1 }; +const SEPOLIA = { hex: "0xaa36a7", num: 11155111 }; + +function populated(nonce, chainId) { return { type: 2, - chainId: 1, + chainId: chainId || MAINNET.num, nonce, gasLimit: 100000n, maxFeePerGas: 2000000000n, @@ -65,17 +73,17 @@ function populated(nonce) { }; } -function signedAtNonce(nonce, withWallet) { - return (withWallet || signer).signTransaction(populated(nonce)); +function signedAtNonce(nonce, withWallet, chainId) { + return (withWallet || signer).signTransaction(populated(nonce, chainId)); } // The node the background populates against. Its answers are the numbers the // approval screen shows, so they are also the numbers every artifact below is // signed at. -function fakeProvider(broadcastTransaction, overrides) { +function fakeProvider(broadcastTransaction, overrides, chainId) { return { broadcastTransaction, - getNetwork: async () => Network.from(1), + getNetwork: async () => Network.from(chainId || MAINNET.num), getTransactionCount: async () => NONCE, estimateGas: async () => 100000n, getFeeData: async () => ({ @@ -111,14 +119,20 @@ function loadBackground(options) { const broadcastTransaction = jest.fn(); const loadState = jest.fn(opts.loadState || (async () => {})); + // The network the wallet is on, which the tests switch under a pending + // approval. The node the transaction is populated against is on the same + // one, as it would be: switching networks switches the RPC endpoint too. + let chain = MAINNET; + jest.doMock("../src/shared/state", () => ({ state: { rpcUrl: "https://rpc.invalid", wallets: [] }, loadState, saveState: jest.fn(async () => {}), - currentNetwork: () => ({ chainId: "0x1" }), + currentNetwork: () => ({ chainId: chain.hex }), })); jest.doMock("../src/shared/balances", () => ({ - getProvider: () => fakeProvider(broadcastTransaction, opts.provider), + getProvider: () => + fakeProvider(broadcastTransaction, opts.provider, chain.num), refreshBalances: jest.fn(async () => {}), })); jest.doMock("../src/shared/phishingDomains", () => ({ @@ -170,7 +184,9 @@ function loadBackground(options) { getLastFocused: (cb) => cb(null), create: (options2, cb) => { created.push(options2); - cb({ id: created.length }); + // A browser that answers with no window at all. The approval + // then has no window it can ever be answered in. + cb(opts.noWindow ? undefined : { id: created.length }); }, remove: (id, cb) => { removed.push(id); @@ -204,8 +220,12 @@ function loadBackground(options) { // 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(txParams) { + function requestTx(txParams, origin) { let rpcResult = null; + // The window this request opens, if it opens one. A request refused + // before an approval is raised opens none, and the window belonging to + // some other request must not be handed back as this one's. + const windowIndex = created.length; const sendResponse = jest.fn((r) => { rpcResult = r; }); @@ -215,11 +235,16 @@ function loadBackground(options) { method: "eth_sendTransaction", params: [txParams || TX_PARAMS], }, - { origin: ORIGIN }, + { origin: origin || ORIGIN }, sendResponse, ); return { - id: () => new URL(created[0].url).searchParams.get("approval"), + id: () => + created.length > windowIndex + ? new URL(created[windowIndex].url).searchParams.get( + "approval", + ) + : null, result: () => rpcResult, }; } @@ -269,6 +294,10 @@ function loadBackground(options) { setActiveAddress: (address) => { persisted.activeAddress = address; }, + // The user switching network in the toolbar popup. + setNetwork: (network) => { + chain = network; + }, fromPopup: { url: EXT_URL + "src/popup/index.html" }, }; } @@ -444,6 +473,318 @@ describe("one approval, one broadcast", () => { }); }); +// Populating the transaction before the approval window opens is what makes +// the displayed object the verified object. It also fixes the nonce before the +// user has answered anything: two requests populated concurrently take the +// same nonce from a node that has seen neither of them broadcast, and the +// second can then never be sent, because the only way to give it a fresh nonce +// is to populate it again after the user has read the old one off the screen. +// So the second request is refused while the first is unanswered. +describe("one transaction approval at a time", () => { + test("a second eth_sendTransaction while one is pending is refused before it takes a nonce", async () => { + const getTransactionCount = jest.fn(async () => NONCE); + const bg = loadBackground({ provider: { getTransactionCount } }); + + const first = bg.requestTx(); + await settle(); + expect(first.id()).toBeTruthy(); + expect(getTransactionCount).toHaveBeenCalledTimes(1); + + const second = bg.requestTx(); + await settle(); + + expect(second.result()).toEqual({ + error: { + code: -32002, + message: expect.stringMatching( + /one transaction at a time.+already in progress/, + ), + }, + }); + // Where the refusal happened matters as much as that it happened: no + // second window, and the node was never asked for a second nonce. + expect(bg.created).toHaveLength(1); + expect(getTransactionCount).toHaveBeenCalledTimes(1); + + // The refusal leaves the pending approval untouched, and it still + // sends. + bg.broadcastTransaction.mockResolvedValue({ hash: "0xfeed" }); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id: first.id(), + approved: true, + rawSignedTx: await signedAtNonce(NONCE), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(first.result()).toEqual({ result: "0xfeed" }); + }); + + // The slot is only defensible if the wallet was going to raise an approval + // anyway. Taken any earlier, a request the wallet refuses outright still + // holds it, and any page at all — connected or not — can deny the user's + // own transactions for as long as it keeps asking. + test("a request the wallet refuses does not take the slot from the connected site", async () => { + const bg = loadBackground(); + + // Both delivered before either reaches its first suspension point, + // which is the interleaving the slot exists for. + const stranger = bg.requestTx(TX_PARAMS, UNCONNECTED_ORIGIN); + const connected = bg.requestTx(); + await settle(); + + expect(stranger.result()).toEqual({ + error: { code: 4100, message: "Unauthorized" }, + }); + // The connected site's transaction was raised, not refused as one the + // user already has in progress. + expect(connected.result()).toBeNull(); + expect(connected.id()).toBeTruthy(); + expect(bg.created).toHaveLength(1); + }); + + // The user closes an approval window that looks hung while the attempt + // behind it is still running, and that attempt then fails in a way that + // would normally leave the approval standing for a retry. There is no + // window left to retry in, so leaving it standing answers the requesting + // page never — and holds the slot for the life of the worker with it. + test("an approval whose window closed under a failed attempt is answered, and frees the next request", async () => { + const stalled = deferred(); + const bg = loadBackground({ + loadState: async () => { + await stalled.promise; + throw new Error("The wallet data could not be read."); + }, + }); + + const first = bg.requestTx(); + await settle(); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id: first.id(), + approved: true, + rawSignedTx: await signedAtNonce(NONCE), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + // The attempt owns the approval, so closing the window does not settle + // it: the attempt may yet broadcast, and it is the one that reports. + bg.closeWindow(1); + await settle(); + expect(first.result()).toBeNull(); + + stalled.resolve(); + await settle(); + expect(first.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + + const second = bg.requestTx(); + await settle(); + expect(second.result()).toBeNull(); + expect(second.id()).toBeTruthy(); + expect(bg.created).toHaveLength(2); + }); + + // An approval with no window is one nothing can ever answer. + test("a request whose approval window cannot be opened is answered rather than left waiting", async () => { + const bg = loadBackground({ noWindow: true }); + + const first = bg.requestTx(); + await settle(); + expect(first.result()).toEqual({ + error: { + code: -32603, + message: expect.stringMatching( + /could not open its approval window/, + ), + }, + }); + + // And it did not take the slot with it. + const second = bg.requestTx(); + await settle(); + expect(second.result()).toEqual({ + error: { + code: -32603, + message: expect.stringMatching( + /could not open its approval window/, + ), + }, + }); + }); + + test("an answered approval frees the next request", async () => { + const bg = loadBackground(); + const first = bg.requestTx(); + await settle(); + + // The user closes the approval window, which rejects it. + bg.closeWindow(1); + await settle(); + expect(first.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + + const second = bg.requestTx(); + await settle(); + expect(second.id()).toBeTruthy(); + expect(bg.created).toHaveLength(2); + }); + + test("a signature request is not held up by a pending transaction", async () => { + const bg = loadBackground(); + bg.requestTx(); + await settle(); + + // A signature consumes no nonce, so it has nothing to collide with. + const signing = bg.requestSign(); + await settle(); + expect(signing.id()).toBeTruthy(); + expect(signing.result()).toBeNull(); + expect(bg.created).toHaveLength(2); + }); +}); + +// A nonce collision found before the transaction reaches the network is the +// one send failure the wallet can speak about with certainty. The user is told +// it did not go out and to send it again, rather than being warned it might +// already be on the chain — which would send them looking for a transaction +// that does not exist, and stop them retrying the one that never went. +describe("a nonce collision is reported as a transaction that did not go out", () => { + test("a broadcast the node refused for the nonce is not reported as possibly sent", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + + bg.broadcastTransaction.mockRejectedValue( + Object.assign(new Error("nonce too low"), { + code: "NONCE_EXPIRED", + }), + ); + const answer = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id: pending.id(), + approved: true, + rawSignedTx: await signedAtNonce(NONCE), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + expect(answer.sendResponse).toHaveBeenCalledWith({ + error: expect.stringMatching(/nonce had already been used/), + retryable: false, + stage: "nonce", + }); + expect(pending.result()).toEqual({ + error: { + message: expect.stringMatching( + /transaction was not sent, because its nonce/, + ), + }, + }); + }); + + test("a nonce this wallet already broadcast is refused without asking the node again", async () => { + const bg = loadBackground(); + const first = bg.requestTx(); + await settle(); + + bg.broadcastTransaction.mockResolvedValue({ hash: "0xfeed" }); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id: first.id(), + approved: true, + rawSignedTx: await signedAtNonce(NONCE), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(first.result()).toEqual({ result: "0xfeed" }); + + // The stubbed node still reports NONCE as the next nonce — a pending + // count that lags a broadcast the node has already taken — so this + // second approval is populated at a nonce this worker has spent. + const second = bg.requestTx(); + await settle(); + const answer = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id: second.id(), + approved: true, + rawSignedTx: await signedAtNonce(NONCE), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + expect(answer.sendResponse).toHaveBeenCalledWith({ + error: expect.stringMatching(/nonce had already been used/), + retryable: false, + stage: "nonce", + }); + expect(second.result()).toEqual({ + error: { + message: expect.stringMatching(/nonce had already been used/), + }, + }); + }); + + // Nonce spaces are per chain, and the wallet switches networks. A nonce + // this wallet spent on one chain says nothing about the same nonce on + // another — and low nonces overlap across chains as a matter of course, so + // a record that ignored the chain would refuse ordinary transactions, + // permanently and with a message that is not true of them. + test("a nonce spent on one chain is not refused on another", async () => { + const bg = loadBackground(); + + const first = bg.requestTx(); + await settle(); + bg.broadcastTransaction.mockResolvedValue({ hash: "0xfeed" }); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id: first.id(), + approved: true, + rawSignedTx: await signedAtNonce(NONCE), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(first.result()).toEqual({ result: "0xfeed" }); + + // The user switches network. On this chain the address has sent + // nothing, so the node populates the next transaction at the same + // nonce — correctly. + bg.setNetwork(SEPOLIA); + const second = bg.requestTx(); + await settle(); + bg.broadcastTransaction.mockResolvedValue({ hash: "0xbeef" }); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id: second.id(), + approved: true, + rawSignedTx: await signedAtNonce(NONCE, undefined, SEPOLIA.num), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(2); + expect(second.result()).toEqual({ result: "0xbeef" }); + }); +}); + // The approval carries the transaction the user was shown and the address it // was raised for, and the artifact is checked against both. Every case here is // one the old comparison — against the dApp's request, for the address that is