diff --git a/README.md b/README.md index cdd0b36..2a52fc9 100644 --- a/README.md +++ b/README.md @@ -1869,13 +1869,16 @@ view would leave a wallet one click from deletion. programmatically rather than by a user gesture. The background populates the transaction (nonce, gas limit, fees, chain id) against the RPC node _before_ 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. 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. + artifact can be compared with it field for field. A nonce the site supplies is + ignored: the nonce is always the account's next nonce from the node, so a site + cannot replace one of the user's pending transactions or leave this one stuck + behind a gap. A request that cannot be populated — unreachable node, reverting + gas estimate — opens no window and is 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 3810e27..4ef8c30 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,15 @@ but the review is broader than any of them. # Completed Steps +- 2026-10-04: A nonce the site supplies with `eth_sendTransaction` is ignored + ([#404](https://git.eeqj.de/sneak/AutistMask/issues/404)). It was passed on to + the transaction, so a site could replace one of the user's pending + transactions (same nonce, higher fee) or leave the new one stuck behind a gap, + and the approval screen showed it as a bare number. `nonce` is no longer one + of the fields taken from the request in `src/shared/approvalTx.js`, so the + transaction always gets the account's next nonce from the node, and that is + the nonce the approval screen shows and the popup signs. + - 2026-10-04: Remembered site permissions are held by full origin ([#402](https://git.eeqj.de/sneak/AutistMask/issues/402)). `allowedSites` and `deniedSites` stored the hostname alone, so a grant to `https://dapp.example` diff --git a/src/shared/approvalTx.js b/src/shared/approvalTx.js index 7dd7e7b..9f7a514 100644 --- a/src/shared/approvalTx.js +++ b/src/shared/approvalTx.js @@ -51,11 +51,15 @@ const POPULATE_TIMEOUT_MS = 20000; // passed to ethers: the object is page-controlled, and a future ethers that // learns to carry a new transaction field must not start picking one up out of // it without this module knowing. +// +// The nonce is not taken from the page; it is always the account's next nonce +// from the network. A page that chose it could replace one of the user's +// pending transactions (the same nonce at a higher fee) or leave this one stuck +// behind a gap (a nonce above the next one). const REQUEST_FIELDS = [ "to", "value", "data", - "nonce", "gasLimit", "gasPrice", "maxFeePerGas", diff --git a/tests/approvalNonce.test.js b/tests/approvalNonce.test.js new file mode 100644 index 0000000..f23221d --- /dev/null +++ b/tests/approvalNonce.test.js @@ -0,0 +1,167 @@ +// A nonce the page supplies is not used +// (https://git.eeqj.de/sneak/AutistMask/issues/404). With it a page could +// replace one of the user's pending transactions (the same nonce at a higher +// fee) or leave the new one stuck behind a gap, so the transaction is always +// given the account's next nonce from the network. +// +// Driven through the preparation the background runs on a page's +// eth_sendTransaction (src/shared/approvalTx.js) and the real approval screen, +// password and Confirm included, against a minimal DOM stub in the shape +// tests/approvalOrigin.test.js uses. The vault is mocked so that no password +// has to be hashed. + +jest.mock("../src/shared/vault", () => ({ + decryptWithPassword: jest.fn(), +})); + +globalThis.chrome = { + storage: { local: { get: async () => ({}), set: async () => {} } }, +}; + +const { Network, Transaction } = require("ethers"); +const { state } = require("../src/shared/state"); +const { decryptWithPassword } = require("../src/shared/vault"); +const { prepareApprovalTx } = require("../src/shared/approvalTx"); +const approval = require("../src/popup/views/approval"); + +// A well-known test phrase, and its first address. +const PHRASE = "test test test test test test test test test test test junk"; +const FROM = "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266"; +const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; + +// The account's next nonce, as the network reports it. +const NETWORK_NONCE = 7; + +const network = { + getNetwork: async () => Network.from(1), + getTransactionCount: async () => NETWORK_NONCE, + estimateGas: async () => 21000n, + getFeeData: async () => ({ + gasPrice: 2000000000n, + maxFeePerGas: 2000000000n, + maxPriorityFeePerGas: 1000000000n, + }), +}; + +function makeElement(id) { + const classes = new Set(); + const el = { + id, + textContent: "", + value: "", + innerHTML: "", + disabled: false, + style: {}, + dataset: {}, + listeners: {}, + classList: { + add: (...names) => names.forEach((n) => classes.add(n)), + remove: (...names) => names.forEach((n) => classes.delete(n)), + contains: (n) => classes.has(n), + toggle: (n, force) => { + const on = force === undefined ? !classes.has(n) : force; + if (on) classes.add(n); + else classes.delete(n); + return on; + }, + }, + addEventListener: (name, fn) => { + el.listeners[name] = el.listeners[name] || []; + el.listeners[name].push(fn); + }, + querySelectorAll: () => [], + appendChild: () => {}, + }; + // Views reach for .parentElement to hide whole sections. + Object.defineProperty(el, "parentElement", { + get: () => node(id + "-parent"), + }); + return el; +} + +function makeDocument() { + const els = new Map(); + return { + getElementById(id) { + // The debug banner is created on demand by helpers.js; absent + // is the state a non-debug, non-testnet popup is in. + if (id === "debug-banner") return null; + if (!els.has(id)) els.set(id, makeElement(id)); + return els.get(id); + }, + createElement: () => makeElement("created"), + body: { prepend: () => {} }, + }; +} + +function node(id) { + return globalThis.document.getElementById(id); +} + +function click(id) { + return Promise.all((node(id).listeners.click || []).map((fn) => fn())); +} + +// Open the transaction approval screen the way the popup does: the background +// hands over the populated transaction and show() draws it. Returns every +// message the screen sends to the background. The background's answer to the +// signed transaction does not matter here; a retryable refusal keeps the +// screen where it is. +async function openTxApproval(approvedTx) { + const sent = []; + globalThis.document = makeDocument(); + globalThis.window = { location: { search: "" }, close: () => {} }; + globalThis.chrome.runtime = { + connect: () => ({ postMessage: () => {} }), + sendMessage: (msg, reply) => { + sent.push(msg); + if (!reply) return; + if (msg.type !== "AUTISTMASK_GET_APPROVAL") { + return reply({ error: "Not sent.", retryable: true }); + } + reply({ + type: "tx", + origin: "https://dapp.example", + isPhishingDomain: false, + approvedFrom: FROM, + approvedTx, + }); + }, + }; + state.activeAddress = FROM; + state.wallets = [ + { + type: "hd", + name: "Wallet 1", + xpub: "xpub-wallet-1", + encryptedSecret: "encrypted-secret-1", + nextIndex: 1, + addresses: [{ address: FROM, balance: "0", tokenBalances: [] }], + }, + ]; + approval.init({}); + await approval.show(1); + return sent; +} + +test("a page's nonce is replaced by the network's, on screen and in the signed transaction", async () => { + // What the background does with the page's request before it opens the + // approval window. + const approvedTx = await prepareApprovalTx(network, FROM, { + from: FROM, + to: RECIPIENT, + value: "0x0", + data: "0x", + nonce: "0x2", + }); + + const sent = await openTxApproval(approvedTx); + expect(node("approve-tx-nonce").textContent).toBe("7"); + + decryptWithPassword.mockResolvedValue(PHRASE); + node("approve-tx-password").value = "any password"; + await click("btn-approve-tx"); + + const response = sent.find((msg) => msg.type === "AUTISTMASK_TX_RESPONSE"); + expect(Transaction.from(response.rawSignedTx).nonce).toBe(NETWORK_NONCE); +}); diff --git a/tests/approvalTx.test.js b/tests/approvalTx.test.js index f943abd..2dcc9b4 100644 --- a/tests/approvalTx.test.js +++ b/tests/approvalTx.test.js @@ -121,7 +121,7 @@ describe("prepareApprovalTx", () => { ); }); - test("keeps a nonce, gas limit and fee the request did fix", async () => { + test("keeps a gas limit and fee the request did fix, but not its nonce", async () => { const approved = await prepareApprovalTx( providerWith(), signer.address, @@ -133,7 +133,7 @@ describe("prepareApprovalTx", () => { maxPriorityFeePerGas: "0x3b9aca00", }, ); - expect(approved.nonce).toBe("0x2"); + expect(approved.nonce).toBe("0x7"); expect(approved.gasLimit).toBe("0x30d40"); expect(approved.maxFeePerGas).toBe("0x12a05f200"); });