harden: verify the signed transaction against what the popup displayed (closes #216)

Verification compared the signed artifact against the dApp's request object.
For every field the dApp omitted -- normally nonce, gas limit and all the fee
fields, since the popup filled them in -- the number the user actually read on
screen was verified by nothing, and only absolute ceilings stood behind it.

The transaction is now populated in the background before the approval window
opens, and that populated object is both what the popup displays and what the
signed artifact is verified against. Every consequential field becomes an
equality comparison; the ceilings remain as a backstop. Population failing
means no approval and no window, and the error goes to the requesting page --
earlier than before, where the same estimate failed after the password had been
typed.

The account is pinned too: `from` is compared against the address named at
approval time rather than whichever address is active at signing, so switching
accounts mid-flow refuses instead of signing from an account the approval did
not name. The message-signing path had the same defect and gets the same fix.

Nonce selection moves earlier as a consequence; the concurrent-approval case
that follows from it is tracked at #271.

tests/backgroundApproval.test.js

@@ -8,16 +8,24 @@
 // already saw — which means the entry being present is not by itself proof
 // that no attempt is running. A second response carrying the same id (a
 // reloaded approval window re-rendering a live Approve button, a popup that
-// emits the message twice) must not start a second verify and broadcast: with
-// the ordinary dApp approval shape the page fixes no nonce, so two artifacts
-// signed at different nonces both verify, and the approved transfer would go
-// out twice.
+// emits the message twice) must not start a second verify and broadcast: the
+// same approved transaction signed twice verifies twice, and the transfer
+// would go out twice.
+//
+// It also covers what the approval is verified against. The approval now
+// carries the transaction the background populated and the screen displayed,
+// and the address that was active when it was raised — so a fee, a nonce or an
+// address that moved between approval and signing is refused rather than
+// signed.
 
-const { Wallet } = require("ethers");
+const { Network, Wallet } = require("ethers");
 
 const SIGNER_KEY =
     "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d";
+const OTHER_KEY =
+    "0x5de4111afa1a4b94908f83103eb1f1706367c2e68ca870fc3fb9a804cdab365a";
 const signer = new Wallet(SIGNER_KEY);
+const other = new Wallet(OTHER_KEY);
 const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a";
 
 const ORIGIN = "https://dapp.example";
@@ -33,9 +41,16 @@ const TX_PARAMS = {
     data: "0x",
 };
 
-// The fields the popup's populateTransaction() would fill in. The nonce is a
-// parameter because the duplicate case turns on the two artifacts differing
-// in exactly the field nothing constrains.
+// The nonce the stubbed node reports, and so the nonce the background
+// populates the approval with.
+const NONCE = 7;
+
+// "Hello AutistMask" as the hex string a dApp passes to personal_sign.
+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) {
     return {
         type: 2,
@@ -50,8 +65,26 @@ function populated(nonce) {
     };
 }
 
-function signedAtNonce(nonce) {
-    return signer.signTransaction(populated(nonce));
+function signedAtNonce(nonce, withWallet) {
+    return (withWallet || signer).signTransaction(populated(nonce));
+}
+
+// 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) {
+    return {
+        broadcastTransaction,
+        getNetwork: async () => Network.from(1),
+        getTransactionCount: async () => NONCE,
+        estimateGas: async () => 100000n,
+        getFeeData: async () => ({
+            gasPrice: 2000000000n,
+            maxFeePerGas: 2000000000n,
+            maxPriorityFeePerGas: 1000000000n,
+        }),
+        ...(overrides || {}),
+    };
 }
 
 // A promise whose settlement the test controls, so a broadcast can be held in
@@ -85,7 +118,7 @@ function loadBackground(options) {
         currentNetwork: () => ({ chainId: "0x1" }),
     }));
     jest.doMock("../src/shared/balances", () => ({
-        getProvider: () => ({ broadcastTransaction }),
+        getProvider: () => fakeProvider(broadcastTransaction, opts.provider),
         refreshBalances: jest.fn(async () => {}),
     }));
     jest.doMock("../src/shared/phishingDomains", () => ({
@@ -171,7 +204,7 @@ 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() {
+    function requestTx(txParams) {
         let rpcResult = null;
         const sendResponse = jest.fn((r) => {
             rpcResult = r;
@@ -180,7 +213,7 @@ function loadBackground(options) {
             {
                 type: "AUTISTMASK_RPC",
                 method: "eth_sendTransaction",
-                params: [TX_PARAMS],
+                params: [txParams || TX_PARAMS],
             },
             { origin: ORIGIN },
             sendResponse,
@@ -191,6 +224,30 @@ function loadBackground(options) {
         };
     }
 
+    // The same for a message-signing approval, which pins the signing address
+    // at approval time in exactly the same way.
+    function requestSign(from) {
+        let rpcResult = null;
+        messageListener(
+            {
+                type: "AUTISTMASK_RPC",
+                method: "personal_sign",
+                params: [MESSAGE, from || signer.address],
+            },
+            { origin: ORIGIN },
+            (r) => {
+                rpcResult = r;
+            },
+        );
+        return {
+            id: () =>
+                new URL(created[created.length - 1].url).searchParams.get(
+                    "approval",
+                ),
+            result: () => rpcResult,
+        };
+    }
+
     // The user closes the approval popup. `created` is index-aligned with the
     // ids the window stub hands back, so window 1 is the first popup opened.
     function closeWindow(windowId) {
@@ -200,18 +257,27 @@ function loadBackground(options) {
     return {
         send,
         requestTx,
+        requestSign,
         closeWindow,
         broadcastTransaction,
         loadState,
         created,
         removed,
+        // The user switching account in the toolbar popup, as the background
+        // sees it: the persisted active address changes underneath a pending
+        // approval.
+        setActiveAddress: (address) => {
+            persisted.activeAddress = address;
+        },
         fromPopup: { url: EXT_URL + "src/popup/index.html" },
     };
 }
 
-// Let the handler's promise chain run to the next suspension point.
+// Let the handler's promise chain run to the next suspension point. Raising a
+// transaction approval now populates it against the node first, which is
+// several awaits deep before the window is opened.
 async function settle() {
-    for (let i = 0; i < 10; i++) await Promise.resolve();
+    for (let i = 0; i < 50; i++) await Promise.resolve();
 }
 
 afterEach(() => {
@@ -244,9 +310,11 @@ describe("one approval, one broadcast", () => {
         await settle();
         expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
 
-        // A reloaded approval window signs the same approval again. Nothing
-        // in the approval fixes a nonce, so this artifact verifies just as
-        // well as the first one.
+        // A reloaded approval window signs the same approval again, at another
+        // nonce. The claim is taken before anything is verified, so what this
+        // asserts is the interlock and not the nonce comparison: the refusal
+        // below is the claim's own message, which a verification failure does
+        // not produce.
         const second = bg.send(
             {
                 type: "AUTISTMASK_TX_RESPONSE",
@@ -376,6 +444,319 @@ describe("one approval, one broadcast", () => {
     });
 });
 
+// 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
+// active now — would have broadcast.
+describe("what the approval is verified against", () => {
+    // The approval screen showed the populated fee. An artifact at ten times
+    // that fee, still far below the ceilings, is what the ceilings alone could
+    // not catch.
+    test("a fee differing from the displayed one is refused, not sent", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestTx();
+        await settle();
+        const id = pending.id();
+
+        const raw = await signer.signTransaction({
+            ...populated(NONCE),
+            maxFeePerGas: 20000000000n,
+        });
+        const answer = bg.send(
+            {
+                type: "AUTISTMASK_TX_RESPONSE",
+                id,
+                approved: true,
+                rawSignedTx: raw,
+            },
+            { url: bg.fromPopup.url },
+        );
+        await settle();
+
+        expect(bg.broadcastTransaction).not.toHaveBeenCalled();
+        expect(answer.sendResponse).toHaveBeenCalledWith(
+            expect.objectContaining({
+                error: expect.stringMatching(/approved maximum fee per gas/),
+                retryable: false,
+                stage: "verify",
+            }),
+        );
+        expect(pending.result()).toEqual({
+            error: {
+                message: expect.stringMatching(/approved maximum fee per gas/),
+            },
+        });
+    });
+
+    test("a nonce differing from the displayed one is refused, not sent", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestTx();
+        await settle();
+        const id = pending.id();
+
+        const answer = bg.send(
+            {
+                type: "AUTISTMASK_TX_RESPONSE",
+                id,
+                approved: true,
+                rawSignedTx: await signedAtNonce(NONCE + 1),
+            },
+            { url: bg.fromPopup.url },
+        );
+        await settle();
+
+        expect(bg.broadcastTransaction).not.toHaveBeenCalled();
+        expect(answer.sendResponse).toHaveBeenCalledWith(
+            expect.objectContaining({
+                error: expect.stringMatching(/approved nonce/),
+                retryable: false,
+                stage: "verify",
+            }),
+        );
+    });
+
+    // The address switch. The approval named one account; the wallet is on
+    // another by the time the artifact arrives. Both halves are covered: the
+    // popup signing as the account that is active now, and the popup correctly
+    // signing as the approved account while the wallet has moved on.
+    test("an artifact signed by the address that is active now is refused", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestTx();
+        await settle();
+        const id = pending.id();
+
+        bg.setActiveAddress(other.address);
+        const answer = bg.send(
+            {
+                type: "AUTISTMASK_TX_RESPONSE",
+                id,
+                approved: true,
+                rawSignedTx: await signedAtNonce(NONCE, other),
+            },
+            { url: bg.fromPopup.url },
+        );
+        await settle();
+
+        expect(bg.broadcastTransaction).not.toHaveBeenCalled();
+        expect(answer.sendResponse).toHaveBeenCalledWith(
+            expect.objectContaining({ retryable: false, stage: "verify" }),
+        );
+        expect(pending.result()).toEqual({
+            error: { message: expect.stringMatching(/active address changed/) },
+        });
+    });
+
+    test("an address switch refuses even the correctly signed artifact", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestTx();
+        await settle();
+        const id = pending.id();
+
+        bg.setActiveAddress(other.address);
+        const answer = bg.send(
+            {
+                type: "AUTISTMASK_TX_RESPONSE",
+                id,
+                approved: true,
+                rawSignedTx: await signedAtNonce(NONCE),
+            },
+            { url: bg.fromPopup.url },
+        );
+        await settle();
+
+        expect(bg.broadcastTransaction).not.toHaveBeenCalled();
+        expect(answer.sendResponse).toHaveBeenCalledWith(
+            expect.objectContaining({
+                error: expect.stringMatching(/active address changed/),
+                retryable: false,
+                stage: "verify",
+            }),
+        );
+        // A refusal, so the approval is spent: the same artifact offered again
+        // finds nothing to answer.
+        const retry = bg.send(
+            {
+                type: "AUTISTMASK_TX_RESPONSE",
+                id,
+                approved: true,
+                rawSignedTx: await signedAtNonce(NONCE),
+            },
+            { url: bg.fromPopup.url },
+        );
+        await settle();
+        expect(retry.sendResponse).not.toHaveBeenCalled();
+        expect(bg.broadcastTransaction).not.toHaveBeenCalled();
+    });
+
+    test("a switch back to the approved address still sends", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestTx();
+        await settle();
+        const id = pending.id();
+
+        bg.setActiveAddress(other.address);
+        bg.setActiveAddress(signer.address);
+        bg.broadcastTransaction.mockResolvedValue({ hash: "0xfeed" });
+        bg.send(
+            {
+                type: "AUTISTMASK_TX_RESPONSE",
+                id,
+                approved: true,
+                rawSignedTx: await signedAtNonce(NONCE),
+            },
+            { url: bg.fromPopup.url },
+        );
+        await settle();
+
+        expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
+        expect(pending.result()).toEqual({ result: "0xfeed" });
+    });
+
+    // The popup is handed the populated transaction and the address it is for,
+    // and nothing else it would have to fetch or decide.
+    test("the popup is given the transaction it is to sign", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestTx();
+        await settle();
+
+        const details = bg.send(
+            { type: "AUTISTMASK_GET_APPROVAL", id: pending.id() },
+            { url: bg.fromPopup.url },
+        );
+        const shown = details.sendResponse.mock.calls[0][0];
+        expect(shown.type).toBe("tx");
+        expect(shown.approvedFrom).toBe(signer.address);
+        expect(shown.approvedTx).toEqual({
+            type: 2,
+            from: signer.address,
+            chainId: "0x1",
+            nonce: "0x7",
+            gasLimit: "0x186a0",
+            maxFeePerGas: "0x77359400",
+            maxPriorityFeePerGas: "0x3b9aca00",
+            to: RECIPIENT,
+            value: TX_PARAMS.value,
+            data: "0x",
+            accessList: [],
+        });
+    });
+
+    // A request naming an account the wallet is not on is refused outright
+    // rather than signed as whichever account is active.
+    test("a request from another address raises no approval at all", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestTx({ ...TX_PARAMS, from: other.address });
+        await settle();
+
+        expect(pending.result()).toEqual({
+            error: {
+                code: 4100,
+                message: expect.stringMatching(/not the active one/),
+            },
+        });
+        expect(bg.created).toEqual([]);
+    });
+
+    // Message signing pins the address the same way, and refuses the same way.
+    // A signature is not a transaction, but a permit signed by an account the
+    // approval did not name spends that account's tokens all the same.
+    test("a sign approval refuses a signature after an address switch", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestSign();
+        await settle();
+
+        bg.setActiveAddress(other.address);
+        const answer = bg.send(
+            {
+                type: "AUTISTMASK_SIGN_RESPONSE",
+                id: pending.id(),
+                approved: true,
+                signature: await signer.signMessage(
+                    Buffer.from(MESSAGE.slice(2), "hex"),
+                ),
+            },
+            { url: bg.fromPopup.url },
+        );
+        await settle();
+
+        expect(answer.sendResponse).toHaveBeenCalledWith(
+            expect.objectContaining({
+                error: expect.stringMatching(/active address changed/),
+                retryable: false,
+            }),
+        );
+        expect(pending.result()).toEqual({
+            error: { message: expect.stringMatching(/active address changed/) },
+        });
+    });
+
+    test("a sign request from another address raises no approval at all", async () => {
+        const bg = loadBackground();
+        const pending = bg.requestSign(other.address);
+        await settle();
+
+        expect(pending.result()).toEqual({
+            error: {
+                code: 4100,
+                message: expect.stringMatching(/not the active one/),
+            },
+        });
+        expect(bg.created).toEqual([]);
+    });
+
+    // Population is a network round trip with the user's hands free. An
+    // approval raised for the address that was active when it started could
+    // never be signed once the wallet has moved off it, so it is never raised.
+    test("an address switch during population raises no approval", async () => {
+        let bg;
+        bg = loadBackground({
+            provider: {
+                // The user switches account in the toolbar popup while the
+                // node is being asked for a gas estimate.
+                estimateGas: async () => {
+                    bg.setActiveAddress(other.address);
+                    return 100000n;
+                },
+            },
+        });
+        const pending = bg.requestTx();
+        await settle();
+
+        expect(pending.result()).toEqual({
+            error: {
+                message: expect.stringMatching(
+                    /active address changed while this transaction was being prepared/,
+                ),
+            },
+        });
+        expect(bg.created).toEqual([]);
+    });
+
+    // Population happens before the window exists, so its failure is a failure
+    // of the request: no approval, no window, and the error goes back to the
+    // page the click came from.
+    test("a transaction that cannot be prepared opens no window", async () => {
+        const bg = loadBackground({
+            provider: {
+                estimateGas: async () => {
+                    throw new Error("execution reverted");
+                },
+            },
+        });
+        const pending = bg.requestTx();
+        await settle();
+
+        expect(pending.result()).toEqual({
+            error: {
+                message: expect.stringMatching(
+                    /could not be prepared.*execution reverted/,
+                ),
+            },
+        });
+        expect(bg.created).toEqual([]);
+    });
+});
+
 // The interlock must not cost the retry the approval exists to allow.
 describe("the interlock releases a failed attempt", () => {
     test("a retryable failure before the broadcast leaves the approval usable", async () => {