diff --git a/README.md b/README.md index f4e8ae7..f7cd7e0 100644 --- a/README.md +++ b/README.md @@ -470,7 +470,9 @@ 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). A transaction or +signature approval whose window is closed is required to leave the page without +an answer. The suite then answers it from the page the toolbar button opens. The limits of that coverage and of the rest of the Chrome suite, none of them papered over: @@ -479,9 +481,10 @@ 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 + 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. - 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 @@ -604,11 +607,12 @@ 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. +that leaves its request pending until it is rejected — 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. 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 @@ -2482,8 +2486,12 @@ 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 + - 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. With several closed requests waiting, the + toolbar button opens the oldest first. #### SignApproval (`approve-sign`) @@ -2569,8 +2577,9 @@ 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 + - 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 424d7a2..8793f78 100644 --- a/docs/README.md +++ b/docs/README.md @@ -306,6 +306,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 f9d6b9d..8924813 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -379,14 +379,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 resetPopupUrl() and showInToolbarPopup(). let toolbarPopupApprovalId = null; +// Set the toolbar popup back to the wallet. While a transaction or signature +// approval whose window was closed is pending, the toolbar popup opens that +// approval instead, so the user can answer it. pendingApprovals lists +// approvals in the order they were added, so the first one found is the +// oldest. function resetPopupUrl() { - toolbarPopupApprovalId = null; + const closed = Object.values(pendingApprovals).find( + (approval) => approval.windowClosed, + ); + 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", + }); } } @@ -421,10 +433,11 @@ function settleApproval(id, result, options) { } // 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. 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 } }; @@ -432,8 +445,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,27 +479,10 @@ function claimApproval(approval) { } // 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, - ), - ); - } } // Open approval in a separate popup window, unless it is no longer pending. @@ -708,9 +705,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]; @@ -738,9 +733,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 }); @@ -1478,36 +1471,39 @@ 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. +// 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, and from then on the toolbar popup opens it (see resetPopupUrl()). +// 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. +// A site-connection or network-switch approval whose window closes is +// rejected, unless its popup connected its 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; + resetPopupUrl(); + continue; + } + if (approval.portConnected) continue; + settleApproval( + id, + abandonedResult( + approval, + APPROVAL_REJECTED_CODE, + APPROVAL_REJECTED_MESSAGE, + ), ); - if (!settleApproval(id, rejection)) approval.windowClosed = true; } }); } @@ -1652,7 +1648,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 @@ -1852,7 +1850,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 f05a031..3b100a7 100644 --- a/src/popup/index.js +++ b/src/popup/index.js @@ -252,9 +252,9 @@ async function followNetworkSwitch() { // 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 bc3bc1c..024466f 100644 --- a/src/popup/views/approval.js +++ b/src/popup/views/approval.js @@ -54,7 +54,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); @@ -1139,12 +1139,15 @@ 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. + $("btn-reject-tx").addEventListener("click", async () => { + await sendMessage({ type: "AUTISTMASK_TX_RESPONSE", id: approvalId, approved: false, - }); + }).catch(() => {}); window.close(); }); @@ -1267,12 +1270,13 @@ function init(_ctx) { if (outcome.retryable) setSignButtonBusy(false); }); - $("btn-reject-sign").addEventListener("click", () => { - notify({ + // As for the transaction's Reject above. + $("btn-reject-sign").addEventListener("click", async () => { + await sendMessage({ type: "AUTISTMASK_SIGN_RESPONSE", id: approvalId, approved: false, - }); + }).catch(() => {}); window.close(); }); } diff --git a/tests/approvalWindow.test.js b/tests/approvalWindow.test.js index ff68313..3f37be1 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,86 @@ test("a connection prompt in the toolbar popup leaves the browser window alone", expect(windows.updates).toEqual([]); expect(env.pageErrors).toEqual([]); }); + +// 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 site would then wait for good. +test.each([ + { + kind: "transaction", + view: "approve-tx", + reject: "btn-reject-tx", + type: "AUTISTMASK_TX_RESPONSE", + 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", + type: "AUTISTMASK_SIGN_RESPONSE", + approval: { + type: "sign", + approvedFrom: ADDRESS, + signParams: { + method: "personal_sign", + message: "0x48656c6c6f", + from: ADDRESS, + }, + }, + }, +])( + "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([]); + }, +); diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index 0ae287a..020eb89 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -774,10 +774,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 +807,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 +869,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 +915,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 +924,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 +1871,131 @@ 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]; + } + + 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"); + }); + + 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 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 +2116,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 +2725,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 +2738,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 +3133,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 651bea5..5067f06 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -3822,6 +3822,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) { @@ -3832,12 +3843,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; @@ -4709,6 +4715,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 @@ -4752,8 +4868,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"); }); @@ -5353,9 +5469,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;