diff --git a/TODO.md b/TODO.md index b9b333e..55adffe 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,20 @@ but the review is broader than any of them. # Completed Steps +- 2026-09-21: A transaction response is honoured only for a transaction + approval, and the three remaining approval-settlement paths are pinned + ([#262](https://git.eeqj.de/sneak/AutistMask/issues/262)). The liveness fix + the issue asks for — settle `4001` on release when the window it would be + retried in is gone — already landed with + [#271](https://git.eeqj.de/sneak/AutistMask/issues/271); this closes the rest. + `AUTISTMASK_TX_RESPONSE` now refuses any approval that is not a transaction + approval, so a reject no longer retires a sign or connection approval and a + signed artifact never runs the broadcast path against one, which before only + failed closed by throwing deeper in. Tests pin the site-connection port's + approve, reject and disconnect paths against a transaction approval + broadcasting behind them: each is declined and the dApp still receives its + broadcast result. + - 2026-08-30: An address no longer wraps, or is shortened to fit, in any of the common views ([#380](https://git.eeqj.de/sneak/AutistMask/issues/380)). The wallet list was the reported case: the address shared one row with the diff --git a/src/background/index.js b/src/background/index.js index e893024..6668a4c 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -1344,6 +1344,15 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { const approval = pendingApprovals[msg.id]; if (!approval) return false; + // This message signs and broadcasts a transaction, so it is honoured + // only for a transaction approval. A sign or connection approval + // carries no approvedTx, and reaching the broadcast path with one used + // to fail closed by throwing deeper in; refusing here keeps a future + // refactor from turning that incidental throw into a live path, and + // keeps a reject on this message from retiring an approval of another + // kind. + if (approval.type !== "tx") return false; + // A reject arriving while an attempt holds the approval is refused, // not honoured: the attempt is on its way to broadcasting the // transaction, and resolving 4001 here would tell the page the request diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index 09f8b7e..e94f44e 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -1541,6 +1541,149 @@ describe("a claimed approval outlives every other retirement path", () => { }); }); +// The approval popup connects a port named for its approval whatever the +// approval's kind, so a decision or a disconnect on that port can reach a +// transaction approval. Both must be declined: the port decides only +// site-connection approvals, and settling a transaction approval it does not +// own — while an attempt is broadcasting behind it — is the round-3 fund-loss +// bug, where the page is told the request was rejected as the transaction goes +// out. These three paths route through settleApproval() and, before this +// suite, were exercised only against site approvals. +describe("the site-connection port never retires a transaction approval", () => { + async function txMidBroadcast() { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + const id = pending.id(); + + const inFlight = deferred(); + bg.broadcastTransaction.mockReturnValue(inFlight.promise); + const first = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(7), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + return { bg, pending, id, inFlight, first }; + } + + test("an approve on the port does not settle it", async () => { + const { bg, pending, id, inFlight, first } = await txMidBroadcast(); + + bg.connectApproval(id).decide(true, false); + await settle(); + expect(pending.result()).toBeNull(); + + inFlight.resolve({ hash: "0xfeed" }); + await settle(); + expect(pending.result()).toEqual({ result: "0xfeed" }); + expect(first.sendResponse).toHaveBeenCalledWith({ txHash: "0xfeed" }); + }); + + test("a reject on the port does not settle it", async () => { + const { bg, pending, id, inFlight, first } = await txMidBroadcast(); + + bg.connectApproval(id).decide(false, false); + await settle(); + expect(pending.result()).toBeNull(); + + inFlight.resolve({ hash: "0xfeed" }); + await settle(); + expect(pending.result()).toEqual({ result: "0xfeed" }); + expect(first.sendResponse).toHaveBeenCalledWith({ txHash: "0xfeed" }); + }); + + test("a port disconnect does not settle it", async () => { + const { bg, pending, id, inFlight, first } = await txMidBroadcast(); + + bg.connectApproval(id).disconnect(); + await settle(); + expect(pending.result()).toBeNull(); + + inFlight.resolve({ hash: "0xfeed" }); + await settle(); + expect(pending.result()).toEqual({ result: "0xfeed" }); + expect(first.sendResponse).toHaveBeenCalledWith({ txHash: "0xfeed" }); + }); +}); + +// AUTISTMASK_TX_RESPONSE signs and broadcasts a transaction, so it is honoured +// only for a transaction approval. A reject shaped as this message used to +// retire a sign or connection approval outright, and an approve carrying a +// signed artifact used to run the broadcast path against an approval that names +// no transaction, failing closed only by throwing deeper in. +describe("a transaction response is honoured only for a transaction approval", () => { + test("a reject does not retire a sign approval", async () => { + const bg = loadBackground(); + const pending = bg.requestSign(); + await settle(); + const id = pending.id(); + + bg.send( + { type: "AUTISTMASK_TX_RESPONSE", id, approved: false }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(pending.result()).toBeNull(); + + // Still live: its own reject settles it. + bg.send( + { type: "AUTISTMASK_SIGN_RESPONSE", id, approved: false }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(pending.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + }); + + test("a reject does not retire a connection approval", async () => { + const bg = loadBackground({ actionPopup: true }); + const pending = bg.requestSite(); + await settle(); + const id = pending.id(); + + bg.send( + { type: "AUTISTMASK_TX_RESPONSE", id, approved: false }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(pending.result()).toBeNull(); + + // Still live: the port that owns it connects the site. + const port = bg.connectApproval(id); + port.decide(true, false); + port.disconnect(); + await settle(); + expect(pending.result()).toEqual({ result: [signer.address] }); + }); + + test("an approve carrying a signed transaction never broadcasts against a sign approval", async () => { + const bg = loadBackground(); + const pending = bg.requestSign(); + await settle(); + const id = pending.id(); + + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx: await signedAtNonce(7), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(bg.broadcastTransaction).not.toHaveBeenCalled(); + expect(pending.result()).toBeNull(); + }); +}); + // 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