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

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(
             {

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