From 7690fe6429cc1306eecd6020c0d3b7b047ea6f2d Mon Sep 17 00:00:00 2001 From: clawbot Date: Mon, 17 Aug 2026 09:16:21 +0200 Subject: [PATCH] fix: answer the page when a background handler throws (closes #280) --- TODO.md | 21 +++ src/background/index.js | 81 ++++++++++- tests/backgroundApproval.test.js | 221 ++++++++++++++++++++++++++++++- 3 files changed, 317 insertions(+), 6 deletions(-) diff --git a/TODO.md b/TODO.md index 252a04a..10ba0b5 100644 --- a/TODO.md +++ b/TODO.md @@ -204,6 +204,27 @@ but the review is broader than any of them. under load, filed as [#287](https://git.eeqj.de/sneak/AutistMask/issues/287) rather than papered over ([#259](https://git.eeqj.de/sneak/AutistMask/issues/259)). +- 2026-08-14: A background message handler that throws now rejects the page + instead of hanging it. `handleRpc(...).then(sendResponse)` had no `.catch()`, + and `sendResponse` is the only thing that settles the dApp's + `window.ethereum.request()` promise — so any throw inside `handleRpc` left + that promise pending forever, with no error and no timeout, indistinguishable + from a slow wallet. It now answers `{ code: -32603, message }` (the JSON-RPC + internal error EIP-1474 defines and EIP-1193 defers to; no EIP-1193 4xxx code + describes "the wallet broke" and none was invented) and logs the method and + the throw to the background console rather than swallowing them. The two async + IIFEs behind `AUTISTMASK_TX_RESPONSE` and `AUTISTMASK_SIGN_RESPONSE` were the + same shape one level down — every statement inside a `try`, but a throw out of + a `catch` block escaping unhandled — and each got a last-resort `.catch()` + settling the approval through `settleApproval()` and answering the popup. The + transaction one tracks which phase it escaped from and reports that, so an + escape before `broadcastTransaction()` says the request is gone rather than + that it may still have reached the network. Every other handler on the path is + synchronous. All four are driven by real failures — a rejecting storage read, + and a failure classifier that throws while classifying a genuine verification + or broadcast failure — and were demonstrated failing first, the RPC one with + `sendResponse` at zero calls + ([#280](https://git.eeqj.de/sneak/AutistMask/issues/280)). - 2026-08-12: EIP-1193 error codes now reach the page. `src/content/inpage.js` rebuilt every failure as `new Error(error.message)`, so the code the background produced and the content script relayed intact was dropped in the diff --git a/src/background/index.js b/src/background/index.js index e761403..e4adfb4 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -170,6 +170,16 @@ function approvedNonce(approvedTx) { } } +// What the page is told when a request failed in a way the wallet has no +// specific answer for. -32603 is the JSON-RPC internal error EIP-1474 defines +// and EIP-1193 defers to for RPC-layer failures; no EIP-1193 4xxx code +// describes "the wallet broke", and one is not invented here. The cause is +// logged rather than put in the message: the page gets a stable sentence, the +// background console gets the throw. +const INTERNAL_ERROR_CODE = -32603; +const INTERNAL_ERROR_MESSAGE = + "AutistMask could not complete this request because of an internal error."; + async function getState() { const result = await storageGet("autistmask"); return ( @@ -1093,9 +1103,26 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { // keep fallback } } - handleRpc(msg.method, msg.params, trustedOrigin).then((response) => { - sendResponse(response); - }); + handleRpc(msg.method, msg.params, trustedOrigin) + .then((response) => { + sendResponse(response); + }) + .catch((err) => { + // Without this the page's window.ethereum.request() promise + // stays pending forever: no response is sent, the content + // script posts nothing back, and the dApp cannot tell the + // failure from a slow wallet. handleRpc does real work — + // state loads, provider calls, transaction population — so + // "it does not throw today" is not a property anyone is + // maintaining. + log.errorf("RPC request failed:", msg.method, err); + sendResponse({ + error: { + code: INTERNAL_ERROR_CODE, + message: INTERNAL_ERROR_MESSAGE, + }, + }); + }); return true; } @@ -1202,6 +1229,10 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { return false; } + // Which phase the last-resort .catch() below reports. Everything up to + // the broadcastTransaction() call provably never reached the network, + // so an escape from there must not tell the user it might have. + let lastResortStage = TX_STAGE_VERIFY; (async () => { // The chain this attempt is on, read once. Verification below // refuses an artifact signed for any other chain, and the nonce @@ -1285,6 +1316,7 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { try { const provider = getProvider(state.rpcUrl); + lastResortStage = TX_STAGE_BROADCAST; const tx = await provider.broadcastTransaction(msg.rawSignedTx); if (nonce !== null) spent.add(nonce); settleApproval( @@ -1316,7 +1348,28 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { stage: outcome.stage, }); } - })(); + })().catch((e) => { + // Every statement above is inside a try, but a throw from one of + // the catch blocks escapes as an unhandled rejection and neither + // the popup nor the page is ever answered. Settle both, through + // the same chokepoint as every other retirement. + log.errorf("transaction approval response failed:", e); + settleApproval( + msg.id, + { + error: { + code: INTERNAL_ERROR_CODE, + message: INTERNAL_ERROR_MESSAGE, + }, + }, + { holdsClaim: true }, + ); + sendResponse({ + error: INTERNAL_ERROR_MESSAGE, + retryable: false, + stage: lastResortStage, + }); + }); return true; } @@ -1400,7 +1453,25 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { } sendResponse({ error: errMsg, retryable }); } - })(); + })().catch((e) => { + // Same shape as the transaction path: a throw out of the catch + // block above would leave the popup and the page both waiting. + log.errorf("sign approval response failed:", e); + settleApproval( + msg.id, + { + error: { + code: INTERNAL_ERROR_CODE, + message: INTERNAL_ERROR_MESSAGE, + }, + }, + { holdsClaim: true }, + ); + sendResponse({ + error: INTERNAL_ERROR_MESSAGE, + retryable: false, + }); + }); return true; } diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index e2b0ec3..464bf38 100644 --- a/tests/backgroundApproval.test.js +++ b/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();