diff --git a/README.md b/README.md index f53e5b3..d5a5221 100644 --- a/README.md +++ b/README.md @@ -512,7 +512,10 @@ window of its own, and the password is required to be absent from every message the approval window sends to the background — with the message that would carry it required to be present, so that check cannot pass by observing nothing. That last one is the standing floor under -[#157](https://git.eeqj.de/sneak/AutistMask/issues/157). +[#157](https://git.eeqj.de/sneak/AutistMask/issues/157). Closing a transaction +or signature approval window is required to leave the dApp's request unanswered. +The suite then opens that approval at the URL the toolbar button holds, and +answers it there. The limits of that coverage and of the rest of the Chrome suite, none of them papered over: @@ -521,9 +524,9 @@ papered over: network with real funds; that remains a human pass before 1.0.0. - The site-connection prompt is raised through `chrome.action.openPopup()`, and headless Chromium's browser-action popup is not a page Playwright can see or - click, so that one prompt is driven at the URL the extension itself puts on - the action — the same page and the same approval id, but whether a real - toolbar click shows it is not observable here. + click. So that prompt, and an approval reopened after its window closed, are + opened in a tab at the URL the extension puts on the action. Whether a real + toolbar click shows that page is not observable here. - Each site-connection request is made 1.5 seconds after the tab it will be driven in is opened (`APPROVAL_TAB_SETTLE_MS` in `tests/e2e/run.js`), because opening that tab closes the previous prompt's toolbar popup; a request made @@ -642,15 +645,24 @@ above, it does not fail on page errors. `make test-e2e-firefox` builds `dist/firefox/` and drives the **real popup in a real Firefox**, installed as an unpacked MV2 temporary add-on via geckodriver. -It covers popup load, the StateRecovery screen (the same cases as the Chrome -suite), wallet creation through the UI, the Add Token screen, and the dApp round -trips — `eth_requestAccounts`, `personal_sign`, `wallet_switchEthereumChain` -rejected and then approved, `eth_sendTransaction`, and a closed approval window -rejecting with EIP-1193 4001 — driven through the real content script, -background page and approval windows. The popup open all along is required to -show the `[TESTNET]` banner once the switch to Sepolia is approved and to drop -it once the switch back is. The `personal_sign` prompt is also required to fit -its window without scrolling sideways, with the extension's pages at 170% zoom. +It covers: + +- popup load +- the StateRecovery screen, with the same cases as the Chrome suite +- wallet creation through the UI +- the Add Token screen +- `eth_requestAccounts` from a dApp +- `personal_sign` from a dApp +- `wallet_switchEthereumChain` from a dApp, rejected and then approved +- `eth_sendTransaction` from a dApp +- an approval window closed without an answer, which leaves its request pending + until it is rejected + +Each dApp request runs through the real content script and background page, and +opens a real approval window. The popup open all along is required to show the +`[TESTNET]` banner once the switch to Sepolia is approved and to drop it once +the switch back is. The `personal_sign` prompt is also required to fit its +window without scrolling sideways, with the extension's pages at 170% zoom. The suite lives in `tests/e2e/firefox/`. Its WebDriver client (`driver.js`) has **no npm dependencies at all**: it is built on global `fetch` and @@ -2644,8 +2656,14 @@ view would leave a wallet one click from deletion. Data Model) → full-sentence error on the error line saying why, nothing signed (no screen change) - "Reject" → closes popup (returns rejection to background) - - Popup window closed without answering → the request is rejected with - EIP-1193 code 4001 + - "Reject" after "Confirm", while the transaction is being sent → the + background's message on the error line, no screen change + - Popup window closed without answering → the request stays pending, and the + site gets no answer. The browser can close this window when the user + clicks away from it, for example to copy the password from a password + manager. Until the request is answered, the toolbar button opens this + screen instead of the wallet. When several requests wait with their + windows closed, the toolbar button opens the oldest first. #### SignApproval (`approve-sign`) @@ -2734,8 +2752,11 @@ view would leave a wallet one click from deletion. - "Sign" (wrong password, or a signing failure) → error line, no screen change - "Reject" → closes popup (returns rejection to background) - - Popup window closed without answering → the request is rejected with - EIP-1193 code 4001 + - "Reject" after "Sign", while the signature is being checked → the + background's message on the error line, no screen change + - Popup window closed without answering → the request stays pending, as on + TxApproval. Until the request is answered, the toolbar button opens this + screen instead of the wallet. #### NetworkApproval (`approve-network`) diff --git a/docs/README.md b/docs/README.md index 4c0cccd..4dc0812 100644 --- a/docs/README.md +++ b/docs/README.md @@ -353,6 +353,13 @@ enter your password and click "Confirm" to authorize it. Message and typed-data signature requests work the same way, with a "Sign" button, and also require your password. +If a transaction or signature popup closes before you answer, for example when +you click away to copy your password, the request stays open. Closing the popup +does not turn the request down. Click the AutistMask icon in your browser +toolbar to bring the request back. Until you answer, the icon opens the request +instead of your wallet. If several requests are waiting this way, the oldest +comes first. + A connected site can also ask to switch the wallet's network. A popup shows the site's origin, the current network and the requested network. Nothing changes unless you click "Switch"; clicking "Reject" or closing the popup leaves diff --git a/src/background/index.js b/src/background/index.js index 17adbf6..b196489 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -161,9 +161,9 @@ let txApprovalSlot = null; 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 -// or on screen. It does not claim the other one is displayed yet, because for -// the length of one network round trip it is not. +// the transaction is populated, so the other request is being prepared, is on +// screen, or waits with its window closed. It does not claim the other one is +// displayed yet, because for the length of one network round trip it is not. const TX_APPROVAL_PENDING_MESSAGE = "AutistMask handles one transaction at a time, and another one is" + " already in progress, so this one was not sent. Please finish that" + @@ -383,14 +383,26 @@ 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(). +// The approval the toolbar popup is set to open, or null while it opens the +// wallet. Set only by updateToolbarPopup() and showInToolbarPopup(). let toolbarPopupApprovalId = null; -function resetPopupUrl() { - toolbarPopupApprovalId = null; +// Point the toolbar popup at the oldest pending approval whose window closed, +// or at the wallet when there is none. The user can then answer that approval +// from the toolbar popup. pendingApprovals lists approvals in the order they +// were added, so the first one found is the oldest. An approval held by a +// running attempt is skipped until that attempt ends. +function updateToolbarPopup() { + const closed = Object.values(pendingApprovals).find( + (approval) => approval.windowClosed && !approval.attemptInFlight, + ); + toolbarPopupApprovalId = closed ? closed.id : null; if (actionNs && typeof actionNs.setPopup === "function") { - actionNs.setPopup({ popup: "src/popup/index.html" }); + actionNs.setPopup({ + popup: closed + ? "src/popup/index.html?approval=" + closed.id + : "src/popup/index.html", + }); } } @@ -420,15 +432,16 @@ function settleApproval(id, result, options) { // approval stops being alive. releaseTxApprovalSlotFor(id); approval.resolve(result); - resetPopupUrl(); + updateToolbarPopup(); return true; } // What a pending approval resolves to when it is given up on rather than -// answered: the window was closed, or could not be opened at all. A tx or sign -// approval answers the requesting page in EIP-1193 shape; a site-connection or -// network-switch approval answers its handler in the shape the popup's -// decision has, as a refusal. +// answered: a site-connection or network-switch window was closed, or the +// window could not be opened at all. An address switch in the wallet gives up +// pending approvals too. A tx or sign approval answers the requesting page in +// EIP-1193 shape; a site-connection or network-switch approval answers its +// handler in the shape the popup's decision has, as a refusal. function abandonedResult(approval, code, message) { if (approval.type === "tx" || approval.type === "sign") { return { error: { code, message } }; @@ -436,8 +449,9 @@ function abandonedResult(approval, code, message) { return { approved: false, remember: false }; } -// A window the user closed without answering is a refusal by the user, which -// is 4001 and the wording every other rejection path already uses. +// A site-connection or network-switch window the user closed without answering +// is a refusal by the user, which is 4001 and the wording every other rejection +// path already uses. const APPROVAL_REJECTED_CODE = 4001; const APPROVAL_REJECTED_MESSAGE = "User rejected the request."; @@ -465,31 +479,16 @@ const APPROVAL_WINDOW_FAILED_MESSAGE = function claimApproval(approval) { if (approval.attemptInFlight) return false; approval.attemptInFlight = true; + updateToolbarPopup(); return true; } // Release an approval whose attempt failed in a way the user can retry. -// Nothing was broadcast, so the next attempt may claim it. -// -// Unless the window it would be retried in is already gone. The user closed it -// while the attempt was running and settleApproval() declined then, correctly, -// because the attempt still owned the approval; the attempt has now failed, so -// nothing owns it and nothing can reach it. Left standing it would hold the -// requesting page's promise open forever and, with it, the transaction -// approval slot. It is settled here as the rejection the closed window -// already meant. +// Nothing was broadcast, so the next attempt may claim it. If the user closed +// its window while the attempt ran, the toolbar popup opens it for that retry. function releaseApproval(approval) { approval.attemptInFlight = false; - if (approval.windowClosed) { - settleApproval( - approval.id, - abandonedResult( - approval, - APPROVAL_REJECTED_CODE, - APPROVAL_REJECTED_MESSAGE, - ), - ); - } + updateToolbarPopup(); } // Open approval in a separate popup window, unless it is no longer pending. @@ -712,9 +711,7 @@ function isExtensionSender(sender) { // had refused. // // TX and sign approvals do not decide here. They stay pending across a -// disconnect. When their window closes, the windows.onRemoved listener below -// rejects them, unless an approved attempt is still running, in which case -// that attempt answers the page. +// disconnect, and across their window closing, until the user answers them. runtime.onConnect.addListener((port) => { if (port.name.startsWith("approval:")) { const id = port.name.split(":")[1]; @@ -742,9 +739,7 @@ runtime.onConnect.addListener((port) => { const approval = pendingApprovals[id]; if (approval) { if (approval.type === "tx" || approval.type === "sign") { - // When its window closes, windows.onRemoved rejects it, - // unless an approved attempt is still running, in which - // case that attempt answers the page. + // It stays pending until the user answers it. return; } settleApproval(id, { approved: false, remember: false }); @@ -1297,7 +1292,7 @@ async function broadcastAccountsChanged() { windowsRemove(approval.windowId).catch(() => {}); } } - resetPopupUrl(); + updateToolbarPopup(); // An address switch ends every connection allowed without "Remember". This // waits on storage, so it comes after the pending approvals above, which // are still rejected the moment the switch arrives. @@ -1489,36 +1484,44 @@ if (runtime.onStartup) { } startBackgroundJobs(); -// When approval window is closed without a response, treat as rejection. -// "Without a response" is the operative part: the popup stays open across the -// verify and broadcast it is waiting on, so a user closing an apparently-hung -// 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 — and the window is recorded as gone, so that an attempt -// which then fails retryably settles instead of waiting in a window that no -// longer exists. +// The toolbar popup keeps its URL when the worker stops. A worker that starts +// holds no approvals, so the toolbar popup is pointed back at the wallet. +updateToolbarPopup(); + +// A transaction or signature approval stays pending when its window closes. +// The browser can close that window when the user clicks away from it, for +// example to copy the password from a password manager. The approval is marked +// as closed, so the toolbar popup can open it (see updateToolbarPopup()). An +// attempt already running when the window closes goes on and reports its +// outcome to the page. // -// A site-connection or network-switch 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. +// This listener rejects a site-connection or network-switch approval whose +// window closes before its popup connected its port. Once the port is +// connected, the approval is left to the port. 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 (windowsNs && windowsNs.onRemoved) { windowsNs.onRemoved.addListener((windowId) => { for (const [id, approval] of Object.entries(pendingApprovals)) { if (approval.windowId !== windowId) continue; - const decidedOnPort = - approval.type !== "tx" && approval.type !== "sign"; - if (decidedOnPort && approval.portConnected) continue; - const rejection = abandonedResult( - approval, - APPROVAL_REJECTED_CODE, - APPROVAL_REJECTED_MESSAGE, + if (approval.type === "tx" || approval.type === "sign") { + approval.windowClosed = true; + updateToolbarPopup(); + continue; + } + if (approval.portConnected) continue; + settleApproval( + id, + abandonedResult( + approval, + APPROVAL_REJECTED_CODE, + APPROVAL_REJECTED_MESSAGE, + ), ); - if (!settleApproval(id, rejection)) approval.windowClosed = true; } }); } @@ -1663,7 +1666,9 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { }); return false; } - return true; + // The popup waits for this answer before it closes its window. + sendResponse(null); + return false; } // The popup signs; it reports back here when it could not. Keep the @@ -1863,7 +1868,8 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { }); return false; } - return true; + sendResponse(null); + return false; } // The popup signs; it reports back here when it could not. Keep the diff --git a/src/popup/index.js b/src/popup/index.js index f5888af..cbcf699 100644 --- a/src/popup/index.js +++ b/src/popup/index.js @@ -281,9 +281,9 @@ async function followStoredSettings() { // zoom it reports, 170% among them, so once the page has its new width (its // resize event) it is measured again, and the window widened again while the // page still falls short, three times at most. A window that cannot grow sends -// no resize event, and the page stops there. The connection prompt can also -// show in the toolbar popup, whose window is the user's browser window; that -// one is left alone. +// no resize event, and the page stops there. An approval can also show in the +// toolbar popup, whose window is the user's browser window; that one is left +// alone. async function widenApprovalWindow() { const page = document.documentElement; if (page.scrollWidth <= page.clientWidth) return; diff --git a/src/popup/views/approval.js b/src/popup/views/approval.js index 3dc1b59..4d001e0 100644 --- a/src/popup/views/approval.js +++ b/src/popup/views/approval.js @@ -55,7 +55,7 @@ const { } = require("../../shared/approvalVerify"); const txStatus = require("./txStatus"); const uniswap = require("../../shared/uniswap"); -const { notify, runtimeApi, sendMessage } = require("../../shared/browserApi"); +const { runtimeApi, sendMessage } = require("../../shared/browserApi"); const erc20Iface = new Interface(ERC20_ABI); @@ -1142,12 +1142,21 @@ function init(_ctx) { } }); - $("btn-reject-tx").addEventListener("click", () => { - notify({ + // The window closes only once the background has the rejection. A message + // sent as the window closes can be lost, and a closed window does not + // reject the request. After Confirm, the background refuses a rejection + // while the transaction is being sent. The window then stays open and + // shows the background's message. + $("btn-reject-tx").addEventListener("click", async () => { + const response = await sendMessage({ type: "AUTISTMASK_TX_RESPONSE", id: approvalId, approved: false, - }); + }).catch(() => null); + if (response && response.error) { + showError("approve-tx-error", response.error); + return; + } window.close(); }); @@ -1270,12 +1279,18 @@ function init(_ctx) { if (outcome.retryable) setSignButtonBusy(false); }); - $("btn-reject-sign").addEventListener("click", () => { - notify({ + // As for the transaction's Reject above. After Sign, the background + // refuses a rejection while the signature is being checked. + $("btn-reject-sign").addEventListener("click", async () => { + const response = await sendMessage({ type: "AUTISTMASK_SIGN_RESPONSE", id: approvalId, approved: false, - }); + }).catch(() => null); + if (response && response.error) { + showError("approve-sign-error", response.error); + return; + } window.close(); }); } diff --git a/tests/approvalWindow.test.js b/tests/approvalWindow.test.js index ff68313..5b33045 100644 --- a/tests/approvalWindow.test.js +++ b/tests/approvalWindow.test.js @@ -12,6 +12,7 @@ const { cleanupPopup, settle, unversionedValidProfile, + ADDRESS, } = require("./support/popupBoot"); afterEach(cleanupPopup); @@ -174,3 +175,136 @@ test("a connection prompt in the toolbar popup leaves the browser window alone", expect(windows.updates).toEqual([]); expect(env.pageErrors).toEqual([]); }); + +// Each row holds a transaction or signature approval as the background +// describes it, plus the element ids of the screen that shows it. `refusal` is +// the message the background refuses a Reject with. +const TX_AND_SIGN_APPROVALS = [ + { + kind: "transaction", + view: "approve-tx", + reject: "btn-reject-tx", + error: "approve-tx-error", + type: "AUTISTMASK_TX_RESPONSE", + refusal: "This transaction is already being sent.", + approval: { + type: "tx", + approvedFrom: ADDRESS, + approvedTx: { + chainId: 1, + to: ADDRESS, + value: "0", + data: "0x", + gasLimit: "21000", + maxFeePerGas: "1000000000", + maxPriorityFeePerGas: "1000000000", + nonce: 0, + }, + }, + }, + { + kind: "signature", + view: "approve-sign", + reject: "btn-reject-sign", + error: "approve-sign-error", + type: "AUTISTMASK_SIGN_RESPONSE", + refusal: "This request is already being signed.", + approval: { + type: "sign", + approvedFrom: ADDRESS, + signParams: { + method: "personal_sign", + message: "0x48656c6c6f", + from: ADDRESS, + }, + }, + }, +]; + +// A closed transaction or signature window leaves its request pending +// (https://git.eeqj.de/sneak/AutistMask/issues/673). So Reject closes the +// window only once the background has the rejection. A message sent as the +// window closes can be lost, and the request would stay pending although the +// user rejected it. +test.each(TX_AND_SIGN_APPROVALS)( + "Reject on a $kind approval closes the window only once the background has answered", + async ({ view, reject, type, approval }) => { + let describeApproval = null; + let answerReject = null; + const sent = []; + const env = await bootPopup(unversionedValidProfile(), { + search: "?approval=approval-1", + runtime: { + connect: () => ({ postMessage: () => {} }), + sendMessage: (msg, reply) => { + if (msg.type === "AUTISTMASK_GET_APPROVAL") { + describeApproval = reply; + } + if (msg.type === type) { + sent.push(msg); + answerReject = reply; + } + }, + }, + }); + describeApproval({ + ...approval, + origin: "https://dapp.example", + isPhishingDomain: false, + }); + await settle(); + expect(env.visibleViews()).toEqual([view]); + const close = jest.fn(); + globalThis.window.close = close; + + const clicked = env.click(reject); + await settle(); + expect(sent).toEqual([{ type, id: "approval-1", approved: false }]); + expect(close).not.toHaveBeenCalled(); + + answerReject(null); + await clicked; + expect(close).toHaveBeenCalledTimes(1); + expect(env.pageErrors).toEqual([]); + }, +); + +// After Confirm or Sign, the background refuses a Reject while the transaction +// is being sent or the signature checked. The request is not rejected then, so +// the window stays open and shows the background's message. +test.each(TX_AND_SIGN_APPROVALS)( + "Reject on a $kind approval that the background refuses shows the background's message and keeps the window open", + async ({ view, reject, error, type, refusal, approval }) => { + let describeApproval = null; + let answerReject = null; + const env = await bootPopup(unversionedValidProfile(), { + search: "?approval=approval-1", + runtime: { + connect: () => ({ postMessage: () => {} }), + sendMessage: (msg, reply) => { + if (msg.type === "AUTISTMASK_GET_APPROVAL") { + describeApproval = reply; + } + if (msg.type === type) answerReject = reply; + }, + }, + }); + describeApproval({ + ...approval, + origin: "https://dapp.example", + isPhishingDomain: false, + }); + await settle(); + const close = jest.fn(); + globalThis.window.close = close; + + const clicked = env.click(reject); + await settle(); + answerReject({ error: refusal, retryable: false }); + await clicked; + expect(close).not.toHaveBeenCalled(); + expect(env.visibleViews()).toEqual([view]); + expect(env.text(error)).toBe(refusal); + expect(env.pageErrors).toEqual([]); + }, +); diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index 0ae287a..1c110cc 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -241,10 +241,11 @@ function loadBackground(options) { 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 = []; + // Every URL the background put on the browser action. When a test gives + // the URL the action holds before the worker starts, that URL comes + // first. A site approval raised through action.openPopup() opens no + // window at all, so this is the only place its id appears. + const actionPopups = opts.toolbarPopup ? [opts.toolbarPopup] : []; const openPopup = jest.fn(() => Promise.resolve()); global.chrome = { @@ -774,10 +775,9 @@ describe("one transaction approval at a time", () => { // The user closes an approval window that looks hung while the attempt // behind it is still running, and that attempt then fails in a way that - // would normally leave the approval standing for a retry. There is no - // window left to retry in, so leaving it standing answers the requesting - // page never — and holds the slot for the life of the worker with it. - test("an approval whose window closed under a failed attempt is answered, and frees the next request", async () => { + // leaves the approval standing for a retry. The toolbar popup opens it, so + // the user can still retry it, and answering it frees the slot. + test("an approval whose window closed under a failed attempt is retried from the toolbar popup, and frees the next request", async () => { const stalled = deferred(); const bg = loadBackground(); @@ -808,11 +808,28 @@ describe("one transaction approval at a time", () => { stalled.resolve(); await settle(); - expect(first.result()).toEqual({ - error: { code: 4001, message: "User rejected the request." }, - }); + expect(first.result()).toBeNull(); + expect(bg.actionPopups[bg.actionPopups.length - 1]).toBe( + "src/popup/index.html?approval=" + first.id(), + ); bg.setStateReadHook(null); + bg.broadcastTransaction.mockResolvedValue({ hash: "0xfeed" }); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id: first.id(), + approved: true, + rawSignedTx: await signedAtNonce(NONCE), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(first.result()).toEqual({ result: "0xfeed" }); + expect(bg.actionPopups[bg.actionPopups.length - 1]).toBe( + "src/popup/index.html", + ); + const second = bg.requestTx(); await settle(); expect(second.result()).toBeNull(); @@ -853,8 +870,10 @@ describe("one transaction approval at a time", () => { const first = bg.requestTx(); await settle(); - // The user closes the approval window, which rejects it. - bg.closeWindow(1); + bg.send( + { type: "AUTISTMASK_TX_RESPONSE", id: first.id(), approved: false }, + { url: bg.fromPopup.url }, + ); await settle(); expect(first.result()).toEqual({ error: { code: 4001, message: "User rejected the request." }, @@ -897,7 +916,7 @@ describe("the worker keeps running while an approval is pending", () => { try { const bg = loadBackground(); const calls = global.chrome.runtime.getPlatformInfo; - bg.requestTx(); + const pending = bg.requestTx(); await settle(); expect(startTimer).toHaveBeenCalledTimes(1); expect(startTimer.mock.calls[0][1]).toBe(20000); @@ -906,11 +925,23 @@ describe("the worker keeps running while an approval is pending", () => { tick(); expect(calls).toHaveBeenCalledTimes(1); - // The user closes the approval window, which rejects it. + // A closed window leaves the approval pending, so the calls go on. bg.closeWindow(1); await settle(); tick(); - expect(calls).toHaveBeenCalledTimes(1); + expect(calls).toHaveBeenCalledTimes(2); + + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id: pending.id(), + approved: false, + }, + { url: bg.fromPopup.url }, + ); + await settle(); + tick(); + expect(calls).toHaveBeenCalledTimes(2); expect(stopTimer).toHaveBeenCalledWith(1); } finally { startTimer.mockRestore(); @@ -1841,6 +1872,237 @@ describe("the interlock releases a failed attempt", () => { }); }); +// A transaction or signature approval stays pending when its window closes. +// The browser can close that window when the user clicks away from it, for +// example to copy the password from a password manager. The toolbar popup then +// opens the approval, and the user answers it there +// (https://git.eeqj.de/sneak/AutistMask/issues/673). +describe("a closed approval window leaves the request pending", () => { + // The URL the toolbar popup opens now. + function toolbarPopup(bg) { + return bg.actionPopups[bg.actionPopups.length - 1]; + } + + // The timer that keeps the worker running while an approval is pending is + // stubbed, so no test here leaves that timer running when it ends. + let keepRunningTimer; + beforeEach(() => { + keepRunningTimer = jest + .spyOn(global, "setInterval") + .mockImplementation(() => 1); + }); + afterEach(() => { + keepRunningTimer.mockRestore(); + }); + + test("the toolbar popup opens a closed transaction approval, and approving it there sends", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + + bg.closeWindow(1); + await settle(); + expect(pending.result()).toBeNull(); + expect(toolbarPopup(bg)).toBe( + "src/popup/index.html?approval=" + pending.id(), + ); + + bg.broadcastTransaction.mockResolvedValue({ hash: "0xfeed" }); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id: pending.id(), + approved: true, + rawSignedTx: await signedAtNonce(NONCE), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(pending.result()).toEqual({ result: "0xfeed" }); + expect(toolbarPopup(bg)).toBe("src/popup/index.html"); + }); + + test("Reject in the toolbar popup answers 4001, and the toolbar popup opens the wallet again", async () => { + const bg = loadBackground(); + const pending = bg.requestSign(); + await settle(); + + bg.closeWindow(1); + await settle(); + expect(pending.result()).toBeNull(); + expect(toolbarPopup(bg)).toBe( + "src/popup/index.html?approval=" + pending.id(), + ); + + const reject = bg.send( + { + type: "AUTISTMASK_SIGN_RESPONSE", + id: pending.id(), + approved: false, + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(pending.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + // The popup closes its window only once this answer arrives. + expect(reject.sendResponse).toHaveBeenCalledWith(null); + expect(toolbarPopup(bg)).toBe("src/popup/index.html"); + }); + + test("with two closed, the toolbar popup opens the older one first, then the other", async () => { + const bg = loadBackground(); + const sign = bg.requestSign(); + await settle(); + const signId = sign.id(); + const tx = bg.requestTx(); + await settle(); + + // The signature request is in window 1 and the transaction in window + // 2. The newer window is closed first. + bg.closeWindow(2); + bg.closeWindow(1); + await settle(); + expect(toolbarPopup(bg)).toBe( + "src/popup/index.html?approval=" + signId, + ); + + bg.send( + { type: "AUTISTMASK_SIGN_RESPONSE", id: signId, approved: false }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(toolbarPopup(bg)).toBe( + "src/popup/index.html?approval=" + tx.id(), + ); + + const reject = bg.send( + { type: "AUTISTMASK_TX_RESPONSE", id: tx.id(), approved: false }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(tx.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + expect(reject.sendResponse).toHaveBeenCalledWith(null); + expect(toolbarPopup(bg)).toBe("src/popup/index.html"); + }); + + // The user can close the window while the attempt after Confirm runs. That + // attempt answers the request, so meanwhile the toolbar popup opens the + // wallet. Opened from the toolbar, the approval would look unanswered. + test("when the window closes during an attempt that sends, the toolbar popup keeps opening the wallet", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + + const inFlight = deferred(); + bg.broadcastTransaction.mockReturnValue(inFlight.promise); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id: pending.id(), + approved: true, + rawSignedTx: await signedAtNonce(NONCE), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + bg.closeWindow(1); + await settle(); + expect(toolbarPopup(bg)).toBe("src/popup/index.html"); + + inFlight.resolve({ hash: "0xfeed" }); + await settle(); + expect(pending.result()).toEqual({ result: "0xfeed" }); + expect(toolbarPopup(bg)).toBe("src/popup/index.html"); + }); + + test("when the window closes during an attempt that fails retryably, the toolbar popup opens the wallet until the attempt ends, then the approval", async () => { + const stalled = deferred(); + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + + bg.setStateReadHook(async () => { + await stalled.promise; + throw new Error("The wallet data could not be read."); + }); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id: pending.id(), + approved: true, + rawSignedTx: await signedAtNonce(NONCE), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + bg.closeWindow(1); + await settle(); + expect(toolbarPopup(bg)).toBe("src/popup/index.html"); + + stalled.resolve(); + await settle(); + expect(pending.result()).toBeNull(); + expect(toolbarPopup(bg)).toBe( + "src/popup/index.html?approval=" + pending.id(), + ); + + // A retry from the toolbar popup holds the approval again, so the + // toolbar popup opens the wallet while that retry runs. + bg.setStateReadHook(null); + const inFlight = deferred(); + bg.broadcastTransaction.mockReturnValue(inFlight.promise); + bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id: pending.id(), + approved: true, + rawSignedTx: await signedAtNonce(NONCE), + }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(toolbarPopup(bg)).toBe("src/popup/index.html"); + + inFlight.resolve({ hash: "0xfeed" }); + await settle(); + expect(pending.result()).toEqual({ result: "0xfeed" }); + expect(toolbarPopup(bg)).toBe("src/popup/index.html"); + }); + + test("an address switch still rejects a closed approval", async () => { + const bg = loadBackground(); + const pending = bg.requestTx(); + await settle(); + bg.closeWindow(1); + await settle(); + expect(pending.result()).toBeNull(); + + bg.send( + { type: "AUTISTMASK_ACTIVE_CHANGED" }, + { url: bg.fromPopup.url }, + ); + await settle(); + expect(pending.result()).toEqual({ + error: { code: 4001, message: "User rejected the request." }, + }); + expect(toolbarPopup(bg)).toBe("src/popup/index.html"); + }); + + // The worker can stop while an approval waits with its window closed. The + // approval is lost with the worker, but the toolbar popup keeps its URL. + // The next worker points the toolbar popup back at the wallet. + test("a worker that starts points the toolbar popup back at the wallet", () => { + const bg = loadBackground({ + toolbarPopup: "src/popup/index.html?approval=lost-approval", + }); + expect(toolbarPopup(bg)).toBe("src/popup/index.html"); + }); +}); + // The claim is what makes one approval one broadcast, so it has to hold // against everything else that retires an approval, not just against a second // AUTISTMASK_TX_RESPONSE. Each of these paths used to resolve the waiting @@ -1961,20 +2223,6 @@ describe("a claimed approval outlives every other retirement path", () => { }); // The refusals above must not cost the rejection its ordinary meaning. - test("with no attempt running, closing the window still rejects", async () => { - const bg = loadBackground(); - const pending = bg.requestTx(); - await settle(); - - bg.closeWindow(1); - await settle(); - - expect(pending.result()).toEqual({ - error: { code: 4001, message: "User rejected the request." }, - }); - expect(bg.broadcastTransaction).not.toHaveBeenCalled(); - }); - test("with no attempt running, an active-address switch still rejects and closes", async () => { const bg = loadBackground(); const pending = bg.requestTx(); @@ -2584,15 +2832,12 @@ describe("a site connection decided as the popup closes", () => { }); }); - // The `isSite &&` half of that skip, which is what keeps it from reaching - // a tx or sign approval. The popup connects its port in show() before it - // knows the approval's type, and the background sets portConnected without - // looking at the type either, so a tx approval in the fallback window - // carries the flag too. Without the conjunct the window event would skip - // it, windowClosed would never be set, releaseApproval() would never settle - // it, and the page would hang — the #271 regression this guard is written - // around. - test("a tx window closed with the port connected still rejects", async () => { + // That skip must not reach a tx or sign approval. The popup connects its + // port in show() before it knows the approval's type, and the background + // sets portConnected without looking at the type either, so a tx approval + // carries the flag too. If the window event skipped it, windowClosed would + // never be set, and the toolbar popup would never open the approval. + test("a tx window closed with the port connected is still opened by the toolbar popup", async () => { const bg = loadBackground(); const pending = bg.requestTx(); await settle(); @@ -2600,9 +2845,10 @@ describe("a site connection decided as the popup closes", () => { await settle(); bg.closeWindow(1); await settle(); - expect(pending.result()).toEqual({ - error: { code: 4001, message: "User rejected the request." }, - }); + expect(pending.result()).toBeNull(); + expect(bg.actionPopups[bg.actionPopups.length - 1]).toBe( + "src/popup/index.html?approval=" + pending.id(), + ); }); }); @@ -2994,13 +3240,14 @@ describe("where an approval window opens", () => { expect(bg.created[1].left).toBeUndefined(); expect(bg.created[1].top).toBeUndefined(); - // The request waits on the second window rather than failing: - // closing that window is refusing the prompt. + // The request waits on the second window rather than failing. Once + // that window closes, the toolbar popup opens the request. expect(sign.result()).toBeNull(); bg.closeWindow(2); await settle(); - expect(sign.result()).toEqual({ - error: { code: 4001, message: "User rejected the request." }, - }); + expect(sign.result()).toBeNull(); + expect(bg.actionPopups[bg.actionPopups.length - 1]).toBe( + "src/popup/index.html?approval=" + sign.id(), + ); }); }); diff --git a/tests/e2e/firefox/run.js b/tests/e2e/firefox/run.js index fc656e0..d2135e4 100644 --- a/tests/e2e/firefox/run.js +++ b/tests/e2e/firefox/run.js @@ -1035,11 +1035,14 @@ step( }, ); +// A closed approval window leaves its request pending, and the toolbar button +// opens it (https://git.eeqj.de/sneak/AutistMask/issues/673). WebDriver cannot +// see the toolbar popup, so its page is opened in a tab, at the URL the +// extension put on the button. step( - "closing an approval window rejects the request with 4001", + "a closed approval window leaves the request pending until it is rejected", async (env) => { const d = env.driver; - const before = env.server.broadcast.length; await d.switchToWindow(env.dappWindow); await startRequest(d, "sign-closed", "personal_sign", [ @@ -1053,15 +1056,45 @@ step( // Closed, not rejected: this is the windows.onRemoved path, which can // only fire if windows.create() handed back a window id for the - // approval to be matched against — call site 4 in the issue, and the - // reason suppressing that write-back turns this step red. + // approval to be matched against. Without that id the toolbar button + // never opens the approval, and this step fails. await d.closeWindow(env.dappWindow); - - await assertUserRejection(d, "sign-closed", "a closed approval window"); + const outcome = await settleRequest(d, "sign-closed", 2000); assert( - env.server.broadcast.length === before, - "a closed approval window still put a transaction on the node", + outcome.settled === "pending", + "closing the approval window answered the page: " + + JSON.stringify(outcome), ); + + await d.switchToWindow(env.popupWindow); + const url = await d.executeAsync( + `const done = arguments[arguments.length - 1]; + browser.browserAction.getPopup({}).then(done, (e) => done(String(e)));`, + ); + assert( + url.includes("?approval="), + "the toolbar button does not open the pending approval: " + url, + ); + const reopened = await d.newWindow("tab"); + await d.switchToWindow(reopened); + await d.navigate(url); + await d.waitVisible("#view-approve-sign"); + await d.click("#btn-reject-sign"); + + await d.switchToWindow(env.dappWindow); + await assertUserRejection( + d, + "sign-closed", + "the signature request rejected from the toolbar button", + ); + + // A page may close only a window a script opened, so the tab can + // still be open. + const left = await d.findWindow((u) => u.includes("?approval=")); + if (left) { + await d.switchToWindow(left); + await d.closeWindow(env.dappWindow); + } }, ); diff --git a/tests/e2e/run.js b/tests/e2e/run.js index b0a73e6..a2112e1 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -4110,11 +4110,12 @@ async function waitForApprovalWindow(ctx, timeout = 30000) { // reservation exists. // How long the tab creation above is given to take effect before the request // that raises the next prompt is issued. Dismissing a browser-action popup -// disconnects its port, and src/background/index.js calls resetPopupUrl() on -// that disconnect; issued too early, the next request's setPopup() is undone -// by the previous popup's teardown, chrome.action.openPopup() then rejects, -// and the fallback window it opens dismisses the popup that was just raised — -// which settles the fresh approval as a rejection before it can be seen. +// disconnects its port, and src/background/index.js calls +// updateToolbarPopup() on that disconnect; issued too early, the next +// request's setPopup() is undone by the previous popup's teardown, +// chrome.action.openPopup() then rejects, and the fallback window it opens +// dismisses the popup that was just raised — which settles the fresh approval +// as a rejection before it can be seen. // Every one of those steps was observed. 1.5s is comfortably past it. const APPROVAL_TAB_SETTLE_MS = 1500; @@ -4140,6 +4141,17 @@ async function reserveApprovalTab(env) { return env.approvalTab; } +// The URL the extension has put on the action: the page the toolbar button +// opens. +function toolbarPopupUrl(env) { + return env.page.evaluate( + () => + new Promise((resolve) => { + chrome.action.getPopup({}, (u) => resolve(String(u))); + }), + ); +} + // The site-connection prompt: the window the extension opened if it managed // to open one, and the reserved tab at the action's approval URL otherwise. async function openSiteApprovalPopup(env, timeout = 30000) { @@ -4150,12 +4162,7 @@ async function openSiteApprovalPopup(env, timeout = 30000) { .find((p) => !p.isClosed() && p.url().includes("?approval=")); if (existing) return existing; - const url = await env.page.evaluate( - () => - new Promise((resolve) => { - chrome.action.getPopup({}, (u) => resolve(String(u))); - }), - ); + const url = await toolbarPopupUrl(env); if (url.includes("?approval=")) { await env.approvalTab.goto(url); return env.approvalTab; @@ -5027,6 +5034,116 @@ test("eth_sendTransaction rejected broadcasts nothing (#183)", async (env) => { ); }); +// A transaction or signature approval stays pending when its window closes. +// The browser can close that window when the user clicks away from it, for +// example to copy the password from a password manager. The toolbar button then +// opens the approval, and the user answers it there +// (https://git.eeqj.de/sneak/AutistMask/issues/673). +// +// This closes the approval window of request `key` and checks that the page +// has no answer. It then opens the approval again from the toolbar button. +// Playwright cannot click that button, so its page is opened in a tab, at the +// URL the extension put on the button. +async function closeAndReopen(env, key, view) { + const approvalWindow = await waitForApprovalWindow(env.ctx); + await visible(approvalWindow, view); + await approvalWindow.close(); + + const outcome = await settleRequest(env.dapp, key, 2000); + assert( + outcome.settled === "pending", + "closing the approval window answered the page: " + + JSON.stringify(outcome), + ); + + const url = await toolbarPopupUrl(env); + assert( + url.includes("?approval="), + "the toolbar button does not open the pending approval: " + url, + ); + const reopened = await env.ctx.newPage(); + await reopened.goto(url); + await visible(reopened, view); + return reopened; +} + +// The stub node answers every nonce request with 0, and the wallet refuses a +// nonce it has already broadcast. A run can therefore approve one dApp +// transaction only, so here the signature is approved and the transaction +// rejected. +test("a closed signature approval is signed from the toolbar button (#673)", async (env) => { + await startRequest(env.dapp, "sign-closed", "personal_sign", [ + SIGN_HEX, + env.expectedAddress, + ]); + const popup = await closeAndReopen( + env, + "sign-closed", + "#view-approve-sign", + ); + let outcome; + try { + await popup.fill("#approve-sign-password", PASSWORD); + await clickAndClose(popup, "#btn-approve-sign"); + outcome = await settleRequest(env.dapp, "sign-closed"); + } finally { + await closeApprovalPages(env.ctx); + } + assert( + outcome.settled === "resolved", + "personal_sign did not resolve: " + JSON.stringify(outcome), + ); + const recovered = getAddress( + verifyMessage(getBytes(SIGN_HEX), outcome.result), + ); + assert( + recovered === env.expectedAddress, + "the personal_sign signature recovers to " + + recovered + + ", not to the approved address " + + env.expectedAddress, + ); + + const url = await toolbarPopupUrl(env); + assert( + !url.includes("?approval="), + "the toolbar button still opens the answered approval: " + url, + ); +}); + +test("a closed transaction approval is rejected from the toolbar button (#673)", async (env) => { + const before = env.routeOpts.broadcastTransactions.length; + await startRequest(env.dapp, "tx-closed", "eth_sendTransaction", [ + { + from: env.expectedAddress, + to: STUB_COUNTERPARTY, + value: toQuantity(TX_VALUE_WEI), + data: TX_DATA, + }, + ]); + const popup = await closeAndReopen(env, "tx-closed", "#view-approve-tx"); + try { + await clickAndClose(popup, "#btn-reject-tx"); + await assertUserRejection( + env.dapp, + "tx-closed", + "the transaction rejected from the toolbar button", + ); + } finally { + await closeApprovalPages(env.ctx); + } + assert( + env.routeOpts.broadcastTransactions.length === before, + "a rejected transaction still reached the RPC", + ); + + const url = await toolbarPopupUrl(env); + assert( + !url.includes("?approval="), + "the toolbar button still opens the answered approval: " + url, + ); +}); + // The extension used to centre every approval window on the last focused // window. Centred on another approval window, the new one landed where the // browser refused to create it, and the request failed with no window at all @@ -5070,8 +5187,8 @@ test("a prompt raised while another approval window has focus opens its own (#29 }); await visible(txWindow, "#view-approve-tx"); - // Closing a window is refusing its prompt, so both answer 4001. - await closeApprovalPages(env.ctx); + await clickAndClose(txWindow, "#btn-reject-tx"); + await clickAndClose(signWindow, "#btn-reject-sign"); await assertUserRejection(env.dapp, "focus-tx", "the transaction prompt"); await assertUserRejection(env.dapp, "focus-sign", "the sign prompt"); }); @@ -5678,9 +5795,11 @@ async function main() { // for the new one. After a test that passed, its windows get five // seconds to close, and one still open then fails the test: it should // have closed itself, and closing it here unreported would hide that. - // After a test that already failed, they are closed unreported, so - // that a prompt it left unanswered does not refuse the next one from - // its site. + // After a test that already failed, they are closed unreported. That + // rejects a site-connection or network-switch prompt it left + // unanswered, so the prompt does not refuse the next one from its + // site. A transaction or signature approval stays pending, and can + // fail the tests after it. if (!failure) { const deadline = Date.now() + 5000; let open;