diff --git a/TODO.md b/TODO.md index 86fd8b7..611faea 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,18 @@ undefined identifiers, which is how # Completed Steps +- 2026-08-14: Approving a site connection is no longer a race against the popup + closing. The decision now rides the approval port the popup already holds, + which is the same channel the close disconnects, so it is delivered ahead of + that disconnect however fast the teardown is; `windows.onRemoved` no longer + decides a site approval whose port is connected, since that event is ordered + against nothing either. Rejecting and closing without deciding both still + report a rejection, and the popup delays its own close by nothing. The e2e + harness's deferred-`window.close()` accommodation is gone with it, so the two + site-prompt tests now drive the shipped decide-then-close in a real Chromium; + against the unfixed code the approval came back to the page as + `{"settled":"rejected","code":4001}` + ([#275](https://git.eeqj.de/sneak/AutistMask/issues/275)). - 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 f89e386..e01fb8d 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -279,13 +279,50 @@ function requestSignApproval(origin, hostname, signParams, approvedFrom) { }); } -// Detect when an approval popup (browser-action) closes without a response. -// TX and sign approvals now use windows.create() and are handled by the -// windowsApi.onRemoved listener below, but we still handle site-connection -// approval disconnects here. +// Anything only the extension's own pages may say. A content script speaks +// with the page's URL, so this is what separates the popup from the site the +// popup is being asked about. +function isExtensionSender(sender) { + const extUrl = runtime.getURL(""); + return !!(sender && sender.url && sender.url.startsWith(extUrl)); +} + +// The approval popup's port: it carries the user's decision on a +// site-connection approval, and its disconnect is how that approval learns the +// popup closed without one. +// +// The decision travels this port rather than a one-off runtime.sendMessage() +// for exactly one reason: the port is also what the popup's window.close() +// disconnects. A message posted on a port is delivered before that port's +// disconnect, so approve-then-close settles as an approval no matter how fast +// the teardown is. Sent as a one-off message the two crossed on independent +// channels with nothing ordering them, and the teardown won every time when +// the prompt was driven in a tab: the user approved and the dApp was told they +// had refused. +// +// TX and sign approvals do not decide here. They stay pending across a +// disconnect — the user can reopen the toolbar popup — and are rejected by the +// windowsApi.onRemoved listener below. runtime.onConnect.addListener((port) => { if (port.name.startsWith("approval:")) { const id = port.name.split(":")[1]; + if (pendingApprovals[id]) { + // This approval has a popup that can speak for it, so its + // disconnect is a trustworthy "closed"; see onRemoved below. + pendingApprovals[id].portConnected = true; + } + port.onMessage.addListener((msg) => { + if (!msg || msg.type !== "AUTISTMASK_APPROVAL_DECISION") return; + if (!isExtensionSender(port.sender)) return; + const approval = pendingApprovals[id]; + if (!approval || approval.type === "tx" || approval.type === "sign") + return; + settleApproval(id, { + approved: !!msg.approved, + remember: !!msg.remember, + }); + resetPopupUrl(); + }); port.onDisconnect.addListener(() => { const approval = pendingApprovals[id]; if (approval) { @@ -832,20 +869,32 @@ startBackgroundJobs(); // window is an ordinary event with an attempt already in flight behind it. // settleApproval() refuses those, which leaves the attempt to report its real // outcome to the page. +// +// A site-connection approval whose popup connected its port is not decided +// here. That popup approves and closes in the same breath, and this event +// races the decision on a channel of its own — the same race the port exists +// to end. Its port disconnect says the same thing this event does, in an order +// that is defined, so the disconnect is left to say it. The window closing +// before any port connected is the one case with nothing else to speak for it, +// and is rejected here so the dApp is not left waiting on a window that is +// gone. if (windowsApi && windowsApi.onRemoved) { windowsApi.onRemoved.addListener((windowId) => { for (const [id, approval] of Object.entries(pendingApprovals)) { if (approval.windowId !== windowId) continue; - const rejection = - approval.type === "tx" || approval.type === "sign" - ? { + const isSite = approval.type !== "tx" && approval.type !== "sign"; + if (isSite && approval.portConnected) continue; + settleApproval( + id, + isSite + ? { approved: false, remember: false } + : { error: { code: 4001, message: "User rejected the request.", }, - } - : { approved: false, remember: false }; - settleApproval(id, rejection); + }, + ); } }); } @@ -872,18 +921,16 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { } // Validate that popup-only messages originate from the extension itself. + // The site-connection decision is not here: it is a port message, and it + // is checked the same way where the port is served. const POPUP_ONLY_TYPES = [ "AUTISTMASK_GET_APPROVAL", - "AUTISTMASK_APPROVAL_RESPONSE", "AUTISTMASK_TX_RESPONSE", "AUTISTMASK_SIGN_RESPONSE", ]; - if (POPUP_ONLY_TYPES.includes(msg.type)) { - const extUrl = runtime.getURL(""); - if (!sender.url || !sender.url.startsWith(extUrl)) { - sendResponse({ error: "Unauthorized sender" }); - return false; - } + if (POPUP_ONLY_TYPES.includes(msg.type) && !isExtensionSender(sender)) { + sendResponse({ error: "Unauthorized sender" }); + return false; } if (msg.type === "AUTISTMASK_GET_APPROVAL") { @@ -915,15 +962,6 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { return false; } - if (msg.type === "AUTISTMASK_APPROVAL_RESPONSE") { - settleApproval(msg.id, { - approved: msg.approved, - remember: msg.remember, - }); - resetPopupUrl(); - return false; - } - if (msg.type === "AUTISTMASK_TX_RESPONSE") { const approval = pendingApprovals[msg.id]; if (!approval) return false; diff --git a/src/popup/views/approval.js b/src/popup/views/approval.js index ebbc25c..009b277 100644 --- a/src/popup/views/approval.js +++ b/src/popup/views/approval.js @@ -441,7 +441,7 @@ function showSignApproval(details) { function show(id) { approvalId = id; - runtime.connect({ name: "approval:" + id }); + approvalPort = runtime.connect({ name: "approval:" + id }); runtime.sendMessage({ type: "AUTISTMASK_GET_APPROVAL", id }, (details) => { if (!details) { window.close(); @@ -470,6 +470,14 @@ function show(id) { } let approvalId = null; +// The port this approval was opened on. Closing this window disconnects it, +// and the background treats that disconnect as "closed without deciding" for a +// site connection — so the decision goes out on this same port and not as a +// one-off message. One channel is ordered: a message posted on it is delivered +// before its own disconnect, however immediately the close follows. Two +// channels were not, and the close won, reporting a user who approved as +// having refused. +let approvalPort = null; let pendingTxDetails = null; // The exact objects shown to the user, kept so the popup signs what it // displayed rather than re-fetching or re-populating anything at approval @@ -537,6 +545,20 @@ function clearSignPassword() { hideError("approve-sign-error"); } +// Answer a site-connection approval and close. The decision goes out on the +// approval port — see approvalPort above for why — and carries no approval id, +// because the port name already names the approval the background will settle. +function decideSite(approved) { + if (approvalPort) { + approvalPort.postMessage({ + type: "AUTISTMASK_APPROVAL_DECISION", + approved, + remember: $("approve-remember").checked, + }); + } + window.close(); +} + function init(ctx) { onViewLeave("approve-tx", clearTxPassword); onViewLeave("approve-sign", clearSignPassword); @@ -547,25 +569,11 @@ function init(ctx) { }); $("btn-approve").addEventListener("click", () => { - const remember = $("approve-remember").checked; - runtime.sendMessage({ - type: "AUTISTMASK_APPROVAL_RESPONSE", - id: approvalId, - approved: true, - remember, - }); - window.close(); + decideSite(true); }); $("btn-reject").addEventListener("click", () => { - const remember = $("approve-remember").checked; - runtime.sendMessage({ - type: "AUTISTMASK_APPROVAL_RESPONSE", - id: approvalId, - approved: false, - remember, - }); - window.close(); + decideSite(false); }); $("btn-approve-tx").addEventListener("click", async () => { diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index d326b81..1915e61 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -32,6 +32,21 @@ const ORIGIN = "https://dapp.example"; const HOSTNAME = "dapp.example"; const EXT_URL = "chrome-extension://autistmask/"; +// An origin the persisted state has never allowed, so asking to connect from +// it raises a prompt rather than being answered from allowedSites. +const FRESH_ORIGIN = "https://fresh.example"; + +// The approval id in the most recent popup URL of a list, or null when none +// of them carries one. Takes both shapes: the absolute URL windows.create() +// is given and the extension-relative one action.setPopup() is given. +function approvalIdIn(urls) { + for (let i = urls.length - 1; i >= 0; i--) { + if (!urls[i] || !urls[i].includes("?approval=")) continue; + return new URL(urls[i], EXT_URL).searchParams.get("approval"); + } + return null; +} + // What the dApp asks for: no nonce, no gas, no fees. This is the shape that // makes a duplicate broadcast possible at all. const TX_PARAMS = { @@ -146,8 +161,13 @@ function loadBackground(options) { let messageListener = null; let windowRemovedListener = null; + let connectListener = null; const created = []; const removed = []; + // Every URL the background put on the browser action. A site approval + // raised through action.openPopup() opens no window at all, so this is + // the only place its id appears. + const actionPopups = []; global.chrome = { storage: { @@ -163,7 +183,14 @@ function loadBackground(options) { messageListener = fn; }, }, - onConnect: { addListener: () => {} }, + // Captured, not swallowed: the approval port is what carries a + // site connection's decision and the popup teardown that races + // it, so a no-op stub here hides the whole subject of #275. + onConnect: { + addListener: (fn) => { + connectListener = fn; + }, + }, lastError: null, }, windows: { @@ -189,7 +216,17 @@ function loadBackground(options) { query: (q, cb) => cb([]), sendMessage: () => {}, }, - action: { setPopup: () => {} }, + action: { + setPopup: (o) => { + actionPopups.push(o.popup); + }, + // The production route for a site connection. Present only when + // a test asks for it, because with it the prompt is the toolbar + // 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() } : {}), + }, }; require("../src/background/index"); @@ -248,6 +285,70 @@ function loadBackground(options) { }; } + // 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. + function requestSite(origin) { + let rpcResult = null; + messageListener( + { + type: "AUTISTMASK_RPC", + method: "eth_requestAccounts", + params: [], + }, + { origin: origin || FRESH_ORIGIN }, + (r) => { + rpcResult = r; + }, + ); + return { + // Wherever the prompt went: the toolbar popup URL when + // action.openPopup() carried it, the created window otherwise. + id: () => + approvalIdIn(actionPopups) || + approvalIdIn(created.map((c) => c.url)), + result: () => rpcResult, + }; + } + + // The popup's approval port, as the browser delivers it. Messages posted + // on a port and that port's disconnect travel one channel in FIFO order, + // which is exactly the property the fix rests on, so this stub delivers + // them in the order the caller emits them and never reorders them. + function connectApproval(id, senderUrl) { + const onMessage = []; + const onDisconnect = []; + const port = { + name: "approval:" + id, + sender: { + url: + senderUrl === undefined + ? EXT_URL + "src/popup/index.html?approval=" + id + : senderUrl, + }, + onMessage: { addListener: (fn) => onMessage.push(fn) }, + onDisconnect: { addListener: (fn) => onDisconnect.push(fn) }, + }; + connectListener(port); + return { + decide: (approved, remember) => { + for (const fn of onMessage) { + fn( + { + type: "AUTISTMASK_APPROVAL_DECISION", + approved, + remember: !!remember, + }, + port, + ); + } + }, + disconnect: () => { + for (const fn of onDisconnect) fn(port); + }, + }; + } + // 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) { @@ -258,6 +359,8 @@ function loadBackground(options) { send, requestTx, requestSign, + requestSite, + connectApproval, closeWindow, broadcastTransaction, loadState, @@ -1058,3 +1161,136 @@ describe("popup-only messages", () => { }); }); }); + +// A site connection decided in a popup that closes on the next line. +// +// The decision and the teardown are two events the popup emits back to back, +// and the background must not be able to reach different outcomes depending on +// which of them it processes first. It cannot, because they are now one +// channel: the decision is posted on the approval port that the close then +// disconnects, so it is delivered first. Every test here therefore emits the +// close IMMEDIATELY after the decision, with nothing awaited in between — +// which is what the popup does, and what used to report a user who approved as +// having refused (#275). +describe("a site connection decided as the popup closes", () => { + // The production route: chrome.action.openPopup() put the prompt in the + // toolbar popup, which is not a window, so nothing but the port + // disconnect can tell the background this prompt is gone. + test("approving in the toolbar popup connects the site", async () => { + const bg = loadBackground({ actionPopup: true }); + const pending = bg.requestSite(); + await settle(); + const id = pending.id(); + expect(id).toBeTruthy(); + expect(bg.created).toHaveLength(0); + + const port = bg.connectApproval(id); + port.decide(true, false); + port.disconnect(); + await settle(); + + expect(pending.result()).toEqual({ result: [signer.address] }); + }); + + test("closing the toolbar popup without deciding is a rejection", async () => { + const bg = loadBackground({ actionPopup: true }); + const pending = bg.requestSite(); + await settle(); + + const port = bg.connectApproval(pending.id()); + port.disconnect(); + await settle(); + + expect(pending.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + }); + + test("rejecting is a rejection, and the close that follows adds nothing", async () => { + const bg = loadBackground({ actionPopup: true }); + const pending = bg.requestSite(); + await settle(); + + const port = bg.connectApproval(pending.id()); + port.decide(false, false); + port.disconnect(); + await settle(); + + expect(pending.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + }); + + // The port carries a decision now, so it carries the sender check the + // one-off message used to carry. A content script that guessed an + // approval id must not be able to connect the site it is running on. + test("a decision from a page sender is ignored, and the close rejects", async () => { + const bg = loadBackground({ actionPopup: true }); + const pending = bg.requestSite(); + await settle(); + + const port = bg.connectApproval(pending.id(), FRESH_ORIGIN + "/x.html"); + port.decide(true, true); + await settle(); + expect(pending.result()).toBeNull(); + + port.disconnect(); + await settle(); + expect(pending.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + }); + + // The fallback shape, where openPopup() is unavailable and the prompt is + // a window the extension opened. Closing it fires windows.onRemoved as + // well, on a channel of its own that is ordered against nothing — so the + // window event must not be allowed to decide a site approval either. + test("approving in the fallback window survives the window event too", async () => { + const bg = loadBackground(); + const pending = bg.requestSite(); + await settle(); + expect(bg.created).toHaveLength(1); + + const port = bg.connectApproval(pending.id()); + port.decide(true, false); + bg.closeWindow(1); + port.disconnect(); + await settle(); + + expect(pending.result()).toEqual({ result: [signer.address] }); + }); + + // Same shape, and the same window event arriving before the popup has + // said anything at all — which is a user closing the window rather than + // deciding, and still has to reach the dApp as a rejection. + test("closing the fallback window without deciding is a rejection", async () => { + const bg = loadBackground(); + const pending = bg.requestSite(); + await settle(); + + const port = bg.connectApproval(pending.id()); + bg.closeWindow(1); + port.disconnect(); + await settle(); + + expect(pending.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + }); + + // The net under the paragraph above: a prompt whose page never got as far + // as connecting the port has no disconnect to reject it, so the window + // event has to. Otherwise the dApp waits forever on a window that is gone. + test("a window that closes before its popup ever connected still rejects", async () => { + const bg = loadBackground(); + const pending = bg.requestSite(); + await settle(); + + bg.closeWindow(1); + await settle(); + + expect(pending.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + }); +}); diff --git a/tests/e2e/run.js b/tests/e2e/run.js index 3998316..1c029fc 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -1596,32 +1596,13 @@ async function reserveApprovalTab(env) { // one down with it. env.approvalTab = await env.ctx.newPage(); - // The one accommodation this section makes to the shipped code, and the - // reason for it. - // - // Both approval buttons call runtime.sendMessage() and then window.close() - // on the next line. Closing this page disconnects the approval port, and - // the disconnect handler in src/background/index.js settles a pending - // site approval as a rejection. In a tab those two race and the teardown - // wins: the approve message is never acted on, and the page is told the - // user rejected. Measured — with the close left in place the approval - // resolves as a rejection every time; with it deferred it resolves as an - // approval every time. - // - // It is deferred, not removed: the harness closes the page itself once - // the outcome has been observed, which is what window.close() would have - // done, only after the message it was racing has been processed. - // - // This affects the site-connection prompt only. The sign and transaction - // prompts run in windows the extension opens itself, with window.close() - // untouched, and their disconnect handler deliberately keeps a tx or sign - // approval pending rather than rejecting it — so there is no race there - // to accommodate. Whether the same ordering holds in a real toolbar popup - // is not observable from a headless harness and is reported rather than - // assumed either way. - await env.approvalTab.addInitScript(() => { - window.close = function () {}; - }); + // This tab runs the shipped popup with nothing patched. The site + // approval buttons decide and then close on the next line, and the two + // site-approval tests below are therefore the real-browser + // approve-then-immediate-close and reject-then-immediate-close cases: the + // decision rides the approval port, which also carries the disconnect the + // close causes, so it is delivered ahead of it and the outcome does not + // depend on the teardown timing (#275). await env.approvalTab.goto("about:blank"); await sleep(APPROVAL_TAB_SETTLE_MS); return env.approvalTab; @@ -1670,6 +1651,28 @@ async function closeApprovalPages(ctx) { } } +// Click a button whose own handler closes the window it lives in — every +// Reject, and Allow on the site prompt. +// +// page.click() dispatches the click and then waits for the renderer to +// acknowledge it, and a page torn down by the handler never gets to. The +// dispatch is what the test needs and the log shows it happening ("performing +// click action") immediately before the failure; the page going away is the +// button working, not the click failing. Observed on #btn-reject-sign and +// #btn-reject-tx, whose windows have always closed themselves. +// +// This swallows nothing that matters: a click that did not land leaves the +// dApp promise unsettled and the assertion after the call still fails. A +// button that is missing or unclickable raises a different error, which is +// rethrown. +async function clickAndClose(page, selector) { + try { + await page.click(selector); + } catch (e) { + if (!String((e && e.message) || e).includes("has been closed")) throw e; + } +} + // Record every message the approval window sends to the background worker. // // This is the direct observation the password check needs. It is installed @@ -1890,7 +1893,7 @@ test("eth_requestAccounts rejected at the prompt returns a rejection (#183)", as // origin in deniedSites and every later test in this section is // auto-rejected with no prompt at all, which would look like a pass. await popup.uncheck("#approve-remember"); - await popup.click("#btn-reject"); + await clickAndClose(popup, "#btn-reject"); await assertUserRejection( env.dapp, @@ -1921,7 +1924,7 @@ test("eth_requestAccounts approved returns the selected address (#183)", async ( // does not, and the sign and transaction tests below all require the // origin to still be authorized. await popup.check("#approve-remember"); - await popup.click("#btn-approve"); + await clickAndClose(popup, "#btn-approve"); outcome = await settleRequest(env.dapp, "accounts"); } finally { @@ -2029,7 +2032,7 @@ test("personal_sign rejected returns a rejection to the page (#183)", async (env ]); const popup = await waitForApprovalWindow(env.ctx); await visible(popup, "#view-approve-sign"); - await popup.click("#btn-reject-sign"); + await clickAndClose(popup, "#btn-reject-sign"); await assertUserRejection( env.dapp, @@ -2132,7 +2135,7 @@ test("eth_signTypedData_v4 rejected returns a rejection to the page (#183)", asy ]); const popup = await waitForApprovalWindow(env.ctx); await visible(popup, "#view-approve-sign"); - await popup.click("#btn-reject-sign"); + await clickAndClose(popup, "#btn-reject-sign"); await assertUserRejection( env.dapp, @@ -2290,7 +2293,7 @@ test("eth_sendTransaction rejected broadcasts nothing (#183)", async (env) => { ]); const popup = await waitForApprovalWindow(env.ctx); await visible(popup, "#view-approve-tx"); - await popup.click("#btn-reject-tx"); + await clickAndClose(popup, "#btn-reject-tx"); await assertUserRejection( env.dapp,