fix: answer the page when a background handler throws (closes #280)

tests/backgroundApproval.test.js

@@ -20,6 +20,11 @@
 
 const { Network, Wallet } = require("ethers");
 
+// The real formatter the approval screen renders failures through. Bound here,
+// before any jest.doMock() of the module, so the copy assertions below check
+// what the user is actually shown.
+const { describeSigningFailure } = require("../src/shared/approvalVerify");
+
 const SIGNER_KEY =
     "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d";
 const OTHER_KEY =
@@ -147,6 +152,14 @@ function loadBackground(options) {
         ensureRecurringAlarms: jest.fn(async () => {}),
         registerAlarmHandlers: jest.fn(),
     }));
+    // The real verification module, except where a test replaces one export
+    // with a throw to drive the handler's own error handling into failing.
+    if (opts.approvalVerify) {
+        jest.doMock("../src/shared/approvalVerify", () => ({
+            ...jest.requireActual("../src/shared/approvalVerify"),
+            ...opts.approvalVerify,
+        }));
+    }
 
     const persisted = {
         wallets: [
@@ -166,7 +179,10 @@ function loadBackground(options) {
     global.chrome = {
         storage: {
             local: {
-                get: jest.fn(async () => ({ autistmask: persisted })),
+                get: jest.fn(
+                    opts.storageGet ||
+                        (async () => ({ autistmask: persisted })),
+                ),
                 set: jest.fn(async () => {}),
             },
         },
@@ -309,6 +325,15 @@ async function settle() {
     for (let i = 0; i < 50; i++) await Promise.resolve();
 }
 
+// settle() only drains microtasks. A handler whose last-resort .catch() has to
+// run after a macrotask boundary needs those turns too, so the assertion that
+// the page WAS answered is what reports a regression rather than a timeout.
+async function settleIncludingRejections() {
+    await settle();
+    await new Promise((resolve) => setImmediate(resolve));
+    await new Promise((resolve) => setImmediate(resolve));
+}
+
 afterEach(() => {
     delete global.chrome;
     jest.resetModules();
@@ -1375,6 +1400,200 @@ describe("a claimed approval outlives every other retirement path", () => {
     });
 });
 
+// A handler that throws must still answer. `sendResponse` is the only thing
+// that settles the page's window.ethereum.request() promise, so a throw that
+// escapes a handler leaves that promise pending forever — no error, no
+// timeout, indistinguishable from a slow wallet. Each case below drives a real
+// throw out of a handler rather than asserting the catch block exists.
+describe("a handler that throws still settles the page", () => {
+    const INTERNAL_ERROR = {
+        code: -32603,
+        message:
+            "AutistMask could not complete this request because of an internal error.",
+    };
+
+    let errorLog;
+    beforeEach(() => {
+        errorLog = jest.spyOn(console, "error").mockImplementation(() => {});
+    });
+    afterEach(() => {
+        errorLog.mockRestore();
+    });
+
+    // getState() awaits extension storage unguarded, and every read path in
+    // handleRpc goes through it. A storage read that rejects is the whole
+    // failure — no hook in the handler itself.
+    test("a rejected handleRpc rejects the page instead of hanging it", async () => {
+        const bg = loadBackground({
+            storageGet: async () => {
+                throw new Error("storage unavailable");
+            },
+        });
+
+        const answer = bg.send(
+            { type: "AUTISTMASK_RPC", method: "eth_accounts", params: [] },
+            { origin: ORIGIN },
+        );
+        await settleIncludingRejections();
+
+        // The channel is held open for the async answer, and the answer
+        // arrives.
+        expect(answer.kept).toBe(true);
+        expect(answer.sendResponse).toHaveBeenCalledWith({
+            error: INTERNAL_ERROR,
+        });
+        // Not swallowed: the throw is on the background console, which is how
+        // this class gets caught in future.
+        expect(errorLog).toHaveBeenCalledWith(
+            "[AutistMask]",
+            "RPC request failed:",
+            "eth_accounts",
+            expect.objectContaining({ message: "storage unavailable" }),
+        );
+    });
+
+    // The transaction response handler wraps every statement in a try, so what
+    // escapes it is a throw from inside one of its catch blocks. Here the
+    // failure classifier itself throws while classifying a real verification
+    // failure — the approval is left claimed, so nothing else can settle it.
+    // The escape happens before broadcastTransaction() is reached, so the
+    // reported stage must be the one that says the transaction is gone.
+    test("a throw while verifying a transaction settles both the page and the popup", async () => {
+        const bg = loadBackground({
+            approvalVerify: {
+                describeTxFailure: () => {
+                    throw new Error("classifier broke");
+                },
+            },
+        });
+        const pending = bg.requestTx();
+        await settle();
+        const id = pending.id();
+
+        // A real verification failure: the artifact is signed at a nonce the
+        // approval never displayed.
+        const answer = bg.send(
+            {
+                type: "AUTISTMASK_TX_RESPONSE",
+                id,
+                approved: true,
+                rawSignedTx: await signedAtNonce(NONCE + 1),
+            },
+            { url: bg.fromPopup.url },
+        );
+        await settleIncludingRejections();
+
+        expect(bg.broadcastTransaction).not.toHaveBeenCalled();
+        expect(pending.result()).toEqual({ error: INTERNAL_ERROR });
+        expect(answer.sendResponse).toHaveBeenCalledWith({
+            error: INTERNAL_ERROR.message,
+            retryable: false,
+            // Nothing was broadcast, so the popup must say the request is gone
+            // rather than that it may still have reached the network.
+            stage: "verify",
+        });
+        expect(errorLog).toHaveBeenCalledWith(
+            "[AutistMask]",
+            "transaction approval response failed:",
+            expect.objectContaining({ message: "classifier broke" }),
+        );
+        // The copy the user actually reads, from the popup's own formatter.
+        expect(
+            describeSigningFailure(answer.sendResponse.mock.calls[0][0], "")
+                .message,
+        ).toBe(
+            INTERNAL_ERROR.message +
+                " This request can no longer be signed." +
+                " Please start it again from the site.",
+        );
+    });
+
+    // The other side of the same local: once broadcastTransaction() has been
+    // entered the wallet genuinely cannot tell whether the node took the
+    // transaction, and the copy that warns about a second send is correct.
+    test("a throw while handling a failed broadcast reports the broadcast stage", async () => {
+        const bg = loadBackground({
+            approvalVerify: {
+                describeTxFailure: () => {
+                    throw new Error("classifier broke");
+                },
+            },
+        });
+        bg.broadcastTransaction.mockRejectedValue(new Error("node refused"));
+        const pending = bg.requestTx();
+        await settle();
+        const id = pending.id();
+
+        // The approved artifact, so verification passes and the failure
+        // happens at the broadcast.
+        const answer = bg.send(
+            {
+                type: "AUTISTMASK_TX_RESPONSE",
+                id,
+                approved: true,
+                rawSignedTx: await signedAtNonce(NONCE),
+            },
+            { url: bg.fromPopup.url },
+        );
+        await settleIncludingRejections();
+
+        expect(bg.broadcastTransaction).toHaveBeenCalled();
+        expect(pending.result()).toEqual({ error: INTERNAL_ERROR });
+        expect(answer.sendResponse).toHaveBeenCalledWith({
+            error: INTERNAL_ERROR.message,
+            retryable: false,
+            stage: "broadcast",
+        });
+        expect(
+            describeSigningFailure(answer.sendResponse.mock.calls[0][0], "")
+                .message,
+        ).toBe(
+            INTERNAL_ERROR.message +
+                " The transaction may still have reached the network." +
+                " Check the account before sending it again.",
+        );
+    });
+
+    test("a throw while handling a failed signature settles both the page and the popup", async () => {
+        const bg = loadBackground({
+            approvalVerify: {
+                failureIsRetryable: () => {
+                    throw new Error("classifier broke");
+                },
+            },
+        });
+        const pending = bg.requestSign();
+        await settle();
+
+        // A real verification failure: the active address moved after the
+        // approval was raised.
+        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 settleIncludingRejections();
+
+        expect(pending.result()).toEqual({ error: INTERNAL_ERROR });
+        expect(answer.sendResponse).toHaveBeenCalledWith({
+            error: INTERNAL_ERROR.message,
+            retryable: false,
+        });
+        expect(errorLog).toHaveBeenCalledWith(
+            "[AutistMask]",
+            "sign approval response failed:",
+            expect.objectContaining({ message: "classifier broke" }),
+        );
+    });
+});
+
 describe("popup-only messages", () => {
     test("a page sender cannot answer an approval", async () => {
         const bg = loadBackground();