fix: one transaction approval at a time, and honest copy for a nonce collision (closes #271)

Populating the transaction in the background 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, so two
eth_sendTransaction calls populated concurrently took the same nonce
from a node that had seen neither of them broadcast, and the second
could never be sent: its approved nonce is spent, and the only way to
give it a fresh one is to populate it again after the user has read the
old one off the screen.

A second transaction approval is now refused while one is unanswered,
with EIP-1193 code -32002. The refusal happens before anything is
populated — no second nonce is allocated, no window opens — and the slot
is released when the requesting page has its answer. Signature approvals
are not gated; a signature consumes no nonce.

A collision that does happen is now reported for what it is. A broadcast
the node refused for the nonce, and an approval carrying a nonce this
worker has already broadcast (caught before the node is asked at all),
both report that the transaction did not reach the network and to send
it again, instead of the standing broadcast wording that warns it may
have sent. "already known" keeps that ambiguous wording deliberately: a
node that says it has the transaction has it.

Nothing about verification is weakened. The approval still carries the
transaction the screen displayed, and the artifact is still compared
against that object field for field.

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