diff --git a/README.md b/README.md index 2a52fc9..9c4364e 100644 --- a/README.md +++ b/README.md @@ -1844,7 +1844,11 @@ view would leave a wallet one click from deletion. says nothing about `http://dapp.example` or another port of that host. The background script prefers the toolbar popup (`action.openPopup()`) and falls back to a separate popup window (`src/background/index.js`, - `requestApproval()`). + `requestApproval()`). Only one exists per site at a time: a further connection + request from a site whose prompt is still unanswered is refused with EIP-1193 + code `-32002` and opens no new prompt. If that prompt was in a toolbar popup + that closed before it connected, and the toolbar popup has since been set to + open something else, the refused request shows that prompt again. - **Elements**: - "Connection Request" heading - Phishing warning banner (shown when the hostname is on the phishing @@ -1910,7 +1914,10 @@ view would leave a wallet one click from deletion. - **When**: A connected website requests a message signature via `personal_sign`, `eth_sign`, or `eth_signTypedData_v4`. Opened the same way as - TxApproval, in a separate popup window. + TxApproval, in a separate popup window. Only one exists per site at a time: a + further signature request, by any of these methods, from a site whose + signature request is still unanswered is refused with EIP-1193 code `-32002` + and opens no window. - **Elements**: - "Signature Request" heading - Phishing warning banner (shown when the hostname is on the phishing diff --git a/TODO.md b/TODO.md index 4ef8c30..ab2810c 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,18 @@ but the review is broader than any of them. # Completed Steps +- 2026-10-04: A site has at most one connection prompt and one signature prompt + open at a time ([#405](https://git.eeqj.de/sneak/AutistMask/issues/405)). Each + `eth_requestAccounts` or `personal_sign` call opened another approval window, + so a page calling in a loop could cover the screen with identical prompts. A + further request of the same kind from a site whose prompt is still unanswered + is now refused with EIP-1193 `-32002`, the code a second transaction already + gets, and opens no window. Signing by `personal_sign`, `eth_sign` and + `eth_signTypedData_v4` counts as one kind. Other sites are not affected, and + the site may ask again once the user has answered. A connection prompt whose + toolbar popup closed before it connected, and which nothing shows any more, is + shown again when the site asks again. + - 2026-10-04: A nonce the site supplies with `eth_sendTransaction` is ignored ([#404](https://git.eeqj.de/sneak/AutistMask/issues/404)). It was passed on to the transaction, so a site could replace one of the user's pending diff --git a/src/background/index.js b/src/background/index.js index fd84680..4c66c85 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -87,7 +87,8 @@ const pendingApprovals = {}; // authority on a nonce the network has not accepted, which an abandoned // approval then leaves a hole in. // -// Sign approvals are not gated: a signature consumes no nonce. +// Sign approvals do not take this slot: a signature consumes no nonce. They are +// limited per site instead; see findPendingApproval(). // // The slot is null when free, and otherwise the handle of the request holding // it. Once that request has raised its approval the handle carries the @@ -99,7 +100,7 @@ let txApprovalSlot = null; // EIP-1474 "resource unavailable": the standard code for a request that is // refused because another one is already pending. -const TX_APPROVAL_PENDING_CODE = -32002; +const APPROVAL_PENDING_CODE = -32002; // True at every moment this can be sent: the slot is taken immediately before // the transaction is populated, so the other request is either being prepared @@ -136,6 +137,22 @@ function releaseTxApprovalSlotFor(approvalId) { } } +// One site-connection approval and one sign approval per site at a time: a +// page that asks again before the user has answered is refused with the code +// above instead of opening another window, so it cannot bury the user in +// prompts. The pending approval itself holds the place, so a caller must test +// this and raise its approval with nothing awaited in between. +function findPendingApproval(origin, type) { + return Object.values(pendingApprovals).find( + (approval) => approval.origin === origin && approval.type === type, + ); +} + +const APPROVAL_PENDING_MESSAGE = + "AutistMask is already waiting for your answer to a request of this kind" + + " from this site, so this one was not shown. Please answer that one," + + " then send this one again."; + // Nonces this worker has already handed to the node, per chain and address. // This is the wallet's own knowledge that a nonce is spent, and it is checked // before a broadcast rather than after: a node's pending count can lag a @@ -288,7 +305,12 @@ async function proxyRpc(method, params) { return json.result; } +// The site-connection approval the toolbar popup is set to open, or null while +// it opens the wallet. Set only by resetPopupUrl() and showInToolbarPopup(). +let toolbarPopupApprovalId = null; + function resetPopupUrl() { + toolbarPopupApprovalId = null; if (actionNs && typeof actionNs.setPopup === "function") { actionNs.setPopup({ popup: "src/popup/index.html" }); } @@ -466,26 +488,33 @@ async function openApprovalWindow(id) { function requestApproval(origin) { return new Promise((resolve) => { const id = crypto.randomUUID(); - pendingApprovals[id] = { id, origin, resolve }; + pendingApprovals[id] = { id, origin, resolve, type: "site" }; if (actionNs && typeof actionNs.openPopup === "function") { - actionNs.setPopup({ - popup: "src/popup/index.html?approval=" + id, - }); - try { - const result = actionNs.openPopup(); - if (result && typeof result.catch === "function") { - result.catch(() => openApprovalWindow(id)); - } - } catch { - openApprovalWindow(id); - } + showInToolbarPopup(id); } else { openApprovalWindow(id); } }); } +// Show a site-connection approval in the toolbar popup, or in a separate popup +// window when the browser will not open the toolbar popup. +function showInToolbarPopup(id) { + toolbarPopupApprovalId = id; + actionNs.setPopup({ + popup: "src/popup/index.html?approval=" + id, + }); + try { + const result = actionNs.openPopup(); + if (result && typeof result.catch === "function") { + result.catch(() => openApprovalWindow(id)); + } + } catch { + openApprovalWindow(id); + } +} + // Open a tx-approval popup and return a promise that resolves with txHash or error. // Uses windows.create() directly because tx approvals are triggered programmatically // (from a dApp RPC call), not from a user gesture, so action.openPopup() is @@ -641,6 +670,31 @@ async function handleConnectionRequest(origin) { return { result: [activeAddress] }; } + const pending = findPendingApproval(origin, "site"); + if (pending) { + // A toolbar popup that closed before it connected leaves its prompt + // pending, and once the toolbar popup is set to open something else + // nothing shows that prompt: the site would be refused until the + // address changed. Show it again. A prompt in a window or in a + // connected popup is settled when that closes, and one the toolbar + // popup is still set to open is a click away, so those are left alone. + if ( + actionNs && + typeof actionNs.openPopup === "function" && + !pending.windowId && + !pending.portConnected && + toolbarPopupApprovalId !== pending.id + ) { + showInToolbarPopup(pending.id); + } + return { + error: { + code: APPROVAL_PENDING_CODE, + message: APPROVAL_PENDING_MESSAGE, + }, + }; + } + // Open approval popup const decision = await requestApproval(origin); @@ -865,6 +919,14 @@ async function handleRpc(method, params, origin) { "Only proceed if you fully understand what you are signing."; } + if (findPendingApproval(origin, "sign")) { + return { + error: { + code: APPROVAL_PENDING_CODE, + message: APPROVAL_PENDING_MESSAGE, + }, + }; + } const decision = await requestSignApproval( origin, signParams, @@ -898,6 +960,14 @@ async function handleRpc(method, params, origin) { }, }; } + if (findPendingApproval(origin, "sign")) { + return { + error: { + code: APPROVAL_PENDING_CODE, + message: APPROVAL_PENDING_MESSAGE, + }, + }; + } const decision = await requestSignApproval( origin, signParams, @@ -969,7 +1039,7 @@ async function handleSendTransaction(params, origin) { if (!slot) { return { error: { - code: TX_APPROVAL_PENDING_CODE, + code: APPROVAL_PENDING_CODE, message: TX_APPROVAL_PENDING_MESSAGE, }, }; diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index f8ea91b..3997c10 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -74,6 +74,22 @@ const NONCE = 7; // "Hello AutistMask" as the hex string a dApp passes to personal_sign. const MESSAGE = "0x48656c6c6f204175746973744d61736b"; +// An EIP-712 document as a dApp passes it to eth_signTypedData_v4. The +// background only carries it to the approval screen, so a small one does. +const TYPED_DATA = JSON.stringify({ + domain: { name: "AutistMask Test", version: "1", chainId: 1 }, + primaryType: "Note", + types: { + EIP712Domain: [ + { name: "name", type: "string" }, + { name: "version", type: "string" }, + { name: "chainId", type: "uint256" }, + ], + Note: [{ name: "contents", type: "string" }], + }, + message: { contents: "Hello AutistMask" }, +}); + // 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. @@ -229,6 +245,7 @@ function loadBackground(options) { // raised through action.openPopup() opens no window at all, so this is // the only place its id appears. const actionPopups = []; + const openPopup = jest.fn(() => Promise.resolve()); global.chrome = { storage, @@ -283,7 +300,7 @@ function loadBackground(options) { // popup: no window is created, so windows.onRemoved can never // fire for it and the port disconnect is the only close signal // that exists. - ...(opts.actionPopup ? { openPopup: () => Promise.resolve() } : {}), + ...(opts.actionPopup ? { openPopup } : {}), }, }; @@ -330,7 +347,7 @@ 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) { + function requestSign(from, origin) { let rpcResult = null; messageListener( { @@ -338,7 +355,7 @@ function loadBackground(options) { method: "personal_sign", params: [MESSAGE, from || signer.address], }, - { origin: ORIGIN }, + { origin: origin || ORIGIN }, (r) => { rpcResult = r; }, @@ -352,6 +369,24 @@ function loadBackground(options) { }; } + // The same through eth_signTypedData_v4, whose params name the address + // first and the typed data second. + function requestTypedData() { + let rpcResult = null; + messageListener( + { + type: "AUTISTMASK_RPC", + method: "eth_signTypedData_v4", + params: [signer.address, TYPED_DATA], + }, + { origin: ORIGIN }, + (r) => { + rpcResult = r; + }, + ); + return { result: () => rpcResult }; + } + // A dApp asking to connect. The origin defaults to one the persisted // state has never allowed, so the request really does raise a prompt // instead of being answered from allowedSites. @@ -426,12 +461,15 @@ function loadBackground(options) { send, requestTx, requestSign, + requestTypedData, requestSite, connectApproval, closeWindow, broadcastTransaction, created, removed, + actionPopups, + openPopup, storage, // The user switching account in the toolbar popup, as the background // sees it: the persisted active address changes underneath a pending @@ -821,6 +859,238 @@ describe("one transaction approval at a time", () => { }); }); +// A page that asks again before the user has answered its last connection or +// signature request is refused, instead of opening one more window per call. +describe("one connection and one signature approval per site at a time", () => { + const PENDING_REFUSAL = { + error: { + code: -32002, + message: expect.stringMatching(/already waiting for your answer/), + }, + }; + + test("a loop of eth_requestAccounts opens one approval and refuses the rest", async () => { + const bg = loadBackground(); + const requests = []; + for (let i = 0; i < 5; i++) requests.push(bg.requestSite()); + await settle(); + + expect(bg.created).toHaveLength(1); + expect(requests[0].result()).toBeNull(); + for (const extra of requests.slice(1)) { + expect(extra.result()).toEqual(PENDING_REFUSAL); + } + + // Once the user has answered, the site may ask again. + bg.closeWindow(1); + await settle(); + expect(requests[0].result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + const again = bg.requestSite(); + await settle(); + expect(again.result()).toBeNull(); + expect(bg.created).toHaveLength(2); + }); + + test("a loop of personal_sign opens one approval and refuses the rest", async () => { + const bg = loadBackground(); + const requests = []; + for (let i = 0; i < 5; i++) requests.push(bg.requestSign()); + await settle(); + + expect(bg.created).toHaveLength(1); + expect(requests[0].result()).toBeNull(); + for (const extra of requests.slice(1)) { + expect(extra.result()).toEqual(PENDING_REFUSAL); + } + }); + + test("a loop of eth_signTypedData_v4 opens one approval and refuses the rest", async () => { + const bg = loadBackground(); + const requests = []; + for (let i = 0; i < 5; i++) requests.push(bg.requestTypedData()); + await settle(); + + expect(bg.created).toHaveLength(1); + expect(requests[0].result()).toBeNull(); + for (const extra of requests.slice(1)) { + expect(extra.result()).toEqual(PENDING_REFUSAL); + } + }); + + test("a pending personal_sign also refuses eth_signTypedData_v4", async () => { + const bg = loadBackground(); + bg.requestSign(); + const typed = bg.requestTypedData(); + await settle(); + + expect(typed.result()).toEqual(PENDING_REFUSAL); + expect(bg.created).toHaveLength(1); + }); + + test("in the toolbar popup, a loop of eth_requestAccounts opens it once", async () => { + const bg = loadBackground({ actionPopup: true }); + const requests = []; + for (let i = 0; i < 5; i++) requests.push(bg.requestSite()); + await settle(); + + expect(bg.openPopup).toHaveBeenCalledTimes(1); + for (const extra of requests.slice(1)) { + expect(extra.result()).toEqual(PENDING_REFUSAL); + } + }); + + // A toolbar popup that closes before it connects tells the background + // nothing, so its prompt stays pending. Once the toolbar popup has been set + // to open something else, nothing shows that prompt, and the site asking + // again must show it again rather than be refused for good. + test("a toolbar prompt nothing shows any more is shown again when the site asks again", async () => { + const bg = loadBackground({ actionPopup: true }); + const first = bg.requestSite(); + await settle(); + const id = first.id(); + + // Its popup closed without connecting. Another site's prompt takes the + // toolbar popup and is answered, which sets it back to the wallet. + const other = bg.requestSite(UNCONNECTED_ORIGIN); + await settle(); + const otherPort = bg.connectApproval(other.id()); + otherPort.decide(false, false); + otherPort.disconnect(); + await settle(); + expect(bg.actionPopups[bg.actionPopups.length - 1]).toBe( + "src/popup/index.html", + ); + + const repeat = bg.requestSite(); + await settle(); + expect(repeat.result()).toEqual(PENDING_REFUSAL); + expect(bg.openPopup).toHaveBeenCalledTimes(3); + expect(bg.actionPopups[bg.actionPopups.length - 1]).toBe( + "src/popup/index.html?approval=" + id, + ); + + // The user answers it, and the first request gets that answer. + bg.connectApproval(id).decide(true, false); + await settle(); + expect(first.result()).toEqual({ result: [signer.address] }); + }); + + // The user rejects a signature request from another site, which is + // connected already. Answering any approval sets the toolbar popup back to + // the wallet. + async function rejectSignatureFromAnotherSite(bg) { + const sign = bg.requestSign(undefined, ORIGIN); + await settle(); + bg.send( + { + type: "AUTISTMASK_SIGN_RESPONSE", + id: sign.id(), + approved: false, + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(sign.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + expect(bg.actionPopups[bg.actionPopups.length - 1]).toBe( + "src/popup/index.html", + ); + } + + test("a toolbar prompt is shown again after another site's signature request is answered", async () => { + const bg = loadBackground({ actionPopup: true }); + const first = bg.requestSite(); + await settle(); + const id = first.id(); + + // Its popup closed without connecting. + await rejectSignatureFromAnotherSite(bg); + + const repeat = bg.requestSite(); + await settle(); + expect(repeat.result()).toEqual(PENDING_REFUSAL); + expect(bg.openPopup).toHaveBeenCalledTimes(2); + expect(bg.actionPopups[bg.actionPopups.length - 1]).toBe( + "src/popup/index.html?approval=" + id, + ); + }); + + test("a prompt in a window is not opened again when the site asks again", async () => { + const bg = loadBackground({ actionPopup: true }); + // The browser will not open the toolbar popup, so the prompt goes to a + // window of its own. + bg.openPopup.mockImplementation(() => + Promise.reject(new Error("no toolbar popup")), + ); + bg.requestSite(); + await settle(); + expect(bg.created).toHaveLength(1); + + await rejectSignatureFromAnotherSite(bg); + expect(bg.created).toHaveLength(2); + + const repeat = bg.requestSite(); + await settle(); + expect(repeat.result()).toEqual(PENDING_REFUSAL); + expect(bg.openPopup).toHaveBeenCalledTimes(1); + expect(bg.created).toHaveLength(2); + }); + + test("a prompt in a connected toolbar popup is not opened again when the site asks again", async () => { + const bg = loadBackground({ actionPopup: true }); + const first = bg.requestSite(); + await settle(); + // The popup is open and showing the prompt. + bg.connectApproval(first.id()); + + await rejectSignatureFromAnotherSite(bg); + + const repeat = bg.requestSite(); + await settle(); + expect(repeat.result()).toEqual(PENDING_REFUSAL); + expect(bg.openPopup).toHaveBeenCalledTimes(1); + expect(bg.actionPopups[bg.actionPopups.length - 1]).toBe( + "src/popup/index.html", + ); + }); + + test("another site's connection request is not held up", async () => { + const bg = loadBackground(); + bg.requestSite(); + const repeat = bg.requestSite(); + const other = bg.requestSite(UNCONNECTED_ORIGIN); + await settle(); + + expect(repeat.result()).toEqual(PENDING_REFUSAL); + expect(other.result()).toBeNull(); + expect(bg.created).toHaveLength(2); + }); + + test("another site's signature request is not held up", async () => { + const bg = loadBackground(); + // Connect a second site, so that it may ask for a signature at all. + const connecting = bg.requestSite(); + await settle(); + const port = bg.connectApproval(connecting.id()); + port.decide(true, false); + port.disconnect(); + await settle(); + expect(connecting.result()).toEqual({ result: [signer.address] }); + + bg.requestSign(); + const repeat = bg.requestSign(); + const other = bg.requestSign(signer.address, FRESH_ORIGIN); + await settle(); + + expect(repeat.result()).toEqual(PENDING_REFUSAL); + expect(other.result()).toBeNull(); + expect(bg.created).toHaveLength(3); + }); +}); + // 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