From 34c1b0071042465c7a54422f4adb63c02e4255b2 Mon Sep 17 00:00:00 2001 From: clawbot Date: Wed, 12 Aug 2026 11:52:36 +0000 Subject: [PATCH] refactor: one shared extension-API module, and drive the dApp flows on Firefox (closes #153) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every call site that touched `browser.*` or `chrome.*` now goes through `src/shared/browserApi.js`, the only file in the tree that names either. It exposes lazily-resolved namespace handles for events and synchronous methods, and promise-returning wrappers for everything that is callback-shaped on Chrome. Callers await; `runtime.lastError` is gone, folded into the rejection the wrapper produces on the Chrome path. The Firefox suite gains the four dApp round trips the issue's definition of done asks for — `eth_requestAccounts`, `personal_sign`, `eth_sendTransaction`, and a closed approval window rejecting with EIP-1193 4001 — driven through the real content script, background page and approval windows. `--network none` was thought to rule that out because it leaves no `http://` origin to inject into; loopback survives it, so the page and a JSON-RPC node are served from 127.0.0.1 inside the container and the run still reaches nothing but itself. That harness refutes the premise it was built to verify. On Firefox 153.0.3, `browser.*` honours a trailing Chrome-style callback and does populate `runtime.lastError`, both measured directly, and all four flows pass against the unconverted code. So this is a uniformity and coverage change, not a repair of a broken target; the PR records the measurement in full. One real defect is fixed on the way past: the window id written back into a pending approval after `windows.create()` was unguarded, so an approval settled during the open — an address switch will do it — dereferenced a deleted entry. --- README.md | 78 ++-- TODO.md | 13 + src/background/index.js | 202 +++++----- src/content/index.js | 72 ++-- src/popup/views/approval.js | 155 ++++---- src/popup/views/home.js | 7 +- src/popup/views/settings.js | 5 +- src/shared/alarms.js | 12 +- src/shared/browserApi.js | 245 +++++++++++++ src/shared/phishingDomains.js | 21 +- src/shared/state.js | 9 +- src/shared/walletDelete.js | 6 +- tests/e2e/firefox/dapp.js | 238 ++++++++++++ tests/e2e/firefox/driver.js | 100 ++++- tests/e2e/firefox/run.js | 673 +++++++++++++++++++++++++++++++++- tests/e2e/network.js | 8 + 16 files changed, 1584 insertions(+), 260 deletions(-) create mode 100644 src/shared/browserApi.js create mode 100644 tests/e2e/firefox/dapp.js diff --git a/README.md b/README.md index edeae29..119e544 100644 --- a/README.md +++ b/README.md @@ -243,10 +243,18 @@ and this suite exists because exactly that class of bug shipped twice. `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, wallet creation through the UI, and the Add Token screen. -The suite lives in `tests/e2e/firefox/` and has **no npm dependencies at all**: -it is a small WebDriver client built on global `fetch` and `child_process` -against geckodriver's HTTP API. +It covers popup load, wallet creation through the UI, the Add Token screen, and +the four dApp round trips — `eth_requestAccounts`, `personal_sign`, +`eth_sendTransaction`, and a closed approval window rejecting with EIP-1193 4001 +— driven through the real content script, background page and approval windows. + +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 +`child_process` against geckodriver's HTTP API. The dApp fixture (`dapp.js`) and +the assertions do use `ethers`, and have to — a signature is recovered in the +runner rather than believed from the extension, and the stub node has to answer +`eth_sendRawTransaction` with the hash `ethers` computes for the artifact it +sent, or `provider.broadcastTransaction()` refuses the answer. Unlike the Chrome suite it builds its own container image rather than pulling a published one, because no published image carries both a pinned Firefox and a @@ -268,20 +276,30 @@ because BiDi's `browsingContext.navigate` refuses `moz-extension://` outright. **Any uncaught error from a `moz-extension://` source fails the run**, including errors from the background page, which the suite never navigates to: a `throw` at the top of `src/background/index.js` kills the background page and fails -step 1. Content-script errors should arrive by the same route, but this suite -does not exercise it and does not claim it — with `--network none` there is no -`http://` page for a content script to be injected into. Errors from add-on -install and background startup are folded into step 1 rather than discarded. -Errors are read from the privileged `nsIConsoleService` in Marionette's chrome -context and filtered to non-warning entries whose `sourceName` is the extension -origin. That mechanism is not a stylistic choice. WebDriver BiDi's -`log.entryAdded` delivers **nothing** for extension pages: on a plain `http://` -page it reports uncaught errors with stack traces, and on the `moz-extension://` -popup it reports zero events, because Firefox's remote agent excludes extension -browsing contexts from BiDi observation. Any harness built on Playwright-BiDi or -Puppeteer-BiDi would therefore see nothing and report success, which is exactly -the vacuous check this repo has already shipped twice. Do not migrate this suite -to BiDi. +step 1. Content scripts **are** exercised now — the dApp steps drive a page +served from loopback, which survives `--network none` — but the _capture_ of a +content-script error by this route is still unproven: no probe has forced a +throw inside one and watched it fail the run, so it remains an expectation +rather than a demonstrated fact. Errors from add-on install and background +startup are folded into step 1 rather than discarded. + +One error is tolerated rather than fatal, listed in `ALLOWED_ERRORS` in +`tests/e2e/firefox/run.js` with the issue that will delete it, and printed on +every occurrence so the concession stays visible in the run output. It is +Firefox reporting the site-approval popup's unawaited `sendMessage` settling +after `window.close()` unloaded the context — the same teardown ordering as +[#275](https://git.eeqj.de/sneak/AutistMask/issues/275), and unsuppressable from +the calling code, because `BaseContext.wrapPromise` reports it whether or not a +handler is attached. Errors are read from the privileged `nsIConsoleService` in +Marionette's chrome context and filtered to non-warning entries whose +`sourceName` is the extension origin. That mechanism is not a stylistic choice. +WebDriver BiDi's `log.entryAdded` delivers **nothing** for extension pages: on a +plain `http://` page it reports uncaught errors with stack traces, and on the +`moz-extension://` popup it reports zero events, because Firefox's remote agent +excludes extension browsing contexts from BiDi observation. Any harness built on +Playwright-BiDi or Puppeteer-BiDi would therefore see nothing and report +success, which is exactly the vacuous check this repo has already shipped twice. +Do not migrate this suite to BiDi. Two limits are worth knowing, both real differences from the Chrome suite: @@ -305,17 +323,19 @@ Two limits are worth knowing, both real differences from the Chrome suite: but a step that logs heavily could evict unread errors. What poll-based costs is location, not coverage: an error cannot be placed within a step the way the Chrome suite's `pageerror` events place it. -- **Nothing is stubbed, which inverts the coverage of network-dependent code.** - There is no fixture layer; the container runs with `--network none` instead, - so the run is offline and deterministic and no request can escape. The - extension swallows its own fetch failures, so the flows are unaffected — but - every network call fails, so only the _failure_ branches of code that depends - on one are ever executed. A `ReferenceError` in the success path of - `renderTransactions`, or of price or balance rendering, passes this suite - green. The offline run is also weaker than the Chrome suite's interception: it - proves nothing got out, but it cannot report which requests were attempted. - Closing that gap needs a fixture layer, deliberately out of scope for this - harness. +- **Almost nothing is stubbed, which inverts the coverage of network-dependent + code.** The container still runs with `--network none`, so the run is offline + and no request can escape. The one thing it can reach is the loopback fixture + in `tests/e2e/firefox/dapp.js`, which serves the dApp page and a JSON-RPC node + and which the extension's `rpcUrl` is pointed at for the dApp steps; a + JSON-RPC method that fixture does not model fails the run rather than + answering `null`. Everything else — Blockscout, the price feed, the phishing + blocklist — has no fixture and simply fails, and the extension swallows its + own fetch failures, so only the _failure_ branches of that code are ever + executed. A `ReferenceError` in the success path of `renderTransactions`, or + of price rendering, passes this suite green. The offline run is also weaker + than the Chrome suite's interception for those calls: it proves nothing got + out, but it cannot report which requests were attempted. Neither `make test-e2e` nor `make test-e2e-firefox` is part of `make check` or `make test`. `REPO_POLICIES.md` caps `make test` at 20 seconds and a browser diff --git a/TODO.md b/TODO.md index d3898e5..85cd61b 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,19 @@ undefined identifiers, which is how # Completed Steps +- 2026-08-12: One shared extension-API module, + [`src/shared/browserApi.js`](src/shared/browserApi.js), is the only place in + the tree that names `browser` or `chrome`. Every call site returns a promise; + `runtime.lastError` is gone. The same commit gives the Firefox suite the four + dApp round trips — `eth_requestAccounts`, `personal_sign`, + `eth_sendTransaction` and a closed approval window rejecting with EIP-1193 + 4001 — against a page and a JSON-RPC node served from loopback, which survives + `--network none`. **The premise of + [#153](https://git.eeqj.de/sneak/AutistMask/issues/153) does not survive that + harness**: Firefox's `browser.*` honours a trailing Chrome-style callback and + populates `runtime.lastError`, both measured directly on Firefox 153.0.3, and + all four flows pass against the unconverted code. What landed is a uniformity + and coverage change, not a repair of a broken target. - 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..f84a698 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -39,17 +39,21 @@ const { registerAlarmHandlers, } = require("../shared/alarms"); -const storageApi = - typeof browser !== "undefined" - ? browser.storage.local - : chrome.storage.local; -const runtime = - typeof browser !== "undefined" ? browser.runtime : chrome.runtime; -const windowsApi = - typeof browser !== "undefined" ? browser.windows : chrome.windows; -const tabsApi = typeof browser !== "undefined" ? browser.tabs : chrome.tabs; -const actionApi = - typeof browser !== "undefined" ? browser.browserAction : chrome.action; +const { + actionApi, + runtimeApi, + storageGet, + tabsQuery, + tabsSendMessage, + windowsApi, + windowsCreate, + windowsGetLastFocused, + windowsRemove, +} = require("../shared/browserApi"); + +const runtime = runtimeApi(); +const windowsNs = windowsApi(); +const actionNs = actionApi(); // Connected sites (in-memory, non-persisted): { "origin:address": true } const connectedSites = {}; @@ -58,7 +62,7 @@ const connectedSites = {}; const pendingApprovals = {}; async function getState() { - const result = await storageApi.get("autistmask"); + const result = await storageGet("autistmask"); return ( result.autistmask || { wallets: [], @@ -122,8 +126,8 @@ async function proxyRpc(method, params) { } function resetPopupUrl() { - if (actionApi && typeof actionApi.setPopup === "function") { - actionApi.setPopup({ popup: "src/popup/index.html" }); + if (actionNs && typeof actionNs.setPopup === "function") { + actionNs.setPopup({ popup: "src/popup/index.html" }); } } @@ -179,32 +183,55 @@ function releaseApproval(approval) { // Open approval in a separate popup window. // This is the primary mechanism for tx/sign approvals (triggered programmatically, // not from a user gesture) and the fallback for site-connection approvals. -function openApprovalWindow(id) { +// Never rejects. Its callers raise it from inside a Promise executor and drop +// the result on the floor, so a rejection here would be unhandled. +async function openApprovalWindow(id) { const popupUrl = runtime.getURL("src/popup/index.html?approval=" + id); const popupWidth = 360; const popupHeight = 600; - windowsApi.getLastFocused((currentWin) => { - const opts = { - url: popupUrl, - type: "popup", - width: popupWidth, - height: popupHeight, - }; - if (currentWin) { - opts.left = Math.round( - currentWin.left + (currentWin.width - popupWidth) / 2, - ); - opts.top = Math.round( - currentWin.top + (currentWin.height - popupHeight) / 2, - ); - } - windowsApi.create(opts, (win) => { - if (win) { - pendingApprovals[id].windowId = win.id; - } - }); - }); + let currentWin = null; + try { + currentWin = await windowsGetLastFocused(); + } catch { + // Nothing focused to centre on. The window still opens, at whatever + // position the browser picks. + } + + const opts = { + url: popupUrl, + type: "popup", + width: popupWidth, + height: popupHeight, + }; + if (currentWin) { + opts.left = Math.round( + currentWin.left + (currentWin.width - popupWidth) / 2, + ); + opts.top = Math.round( + currentWin.top + (currentWin.height - popupHeight) / 2, + ); + } + + let win = null; + try { + win = await windowsCreate(opts); + } catch (e) { + // No window means no approval screen and no way for the user to + // answer. The request stays pending rather than being settled behind + // their back; say so rather than failing silently. + log.errorf("could not open the approval window:", e); + return; + } + + // The id the onRemoved listener matches on to turn a closed window into a + // rejection. Guarded because the create() above is a real await now: an + // address switch can settle and remove the approval while the window is + // opening, and writing the id back would resurrect a bare entry that + // nothing would ever resolve. + if (win && pendingApprovals[id]) { + pendingApprovals[id].windowId = win.id; + } } // Open an approval popup and return a promise that resolves with the user decision. @@ -214,12 +241,12 @@ function requestApproval(origin, hostname) { const id = crypto.randomUUID(); pendingApprovals[id] = { origin, hostname, resolve }; - if (actionApi && typeof actionApi.openPopup === "function") { - actionApi.setPopup({ + if (actionNs && typeof actionNs.openPopup === "function") { + actionNs.setPopup({ popup: "src/popup/index.html?approval=" + id, }); try { - const result = actionApi.openPopup(); + const result = actionNs.openPopup(); if (result && typeof result.catch === "function") { result.catch(() => openApprovalWindow(id)); } @@ -281,7 +308,7 @@ 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 +// windows.onRemoved listener below, but we still handle site-connection // approval disconnects here. runtime.onConnect.addListener((port) => { if (port.name.startsWith("approval:")) { @@ -663,24 +690,26 @@ async function handleRpc(method, params, origin) { } // Broadcast chainChanged to all tabs when the network is switched. -function broadcastChainChanged(chainId) { - tabsApi.query({}, (tabs) => { - for (const tab of tabs) { - tabsApi.sendMessage( - tab.id, - { - type: "AUTISTMASK_EVENT", - eventName: "chainChanged", - data: chainId, - }, - () => { - if (runtime.lastError) { - // expected for tabs without our content script - } - }, - ); - } - }); +// +// Never rejects: its caller is an RPC handler that must answer the page +// whatever the browser made of the broadcast. +async function broadcastChainChanged(chainId) { + let tabs; + try { + tabs = await tabsQuery({}); + } catch { + return; + } + for (const tab of tabs) { + // A tab with no content script has no receiver, and that is the + // ordinary case rather than a fault. The rejection it produces is the + // promise-shaped form of the runtime.lastError this used to read. + tabsSendMessage(tab.id, { + type: "AUTISTMASK_EVENT", + eventName: "chainChanged", + data: chainId, + }).catch(() => {}); + } } // Broadcast accountsChanged to all tabs, respecting per-address permissions @@ -705,41 +734,36 @@ async function broadcastAccountsChanged() { : { approved: false, remember: false }; if (!settleApproval(id, rejection)) continue; if (approval.windowId) { - windowsApi.remove(approval.windowId, () => { - if (runtime.lastError) { - // window already closed - } - }); + // Rejects when the window has already gone, which is a race the + // user wins routinely by closing it themselves. + windowsRemove(approval.windowId).catch(() => {}); } } resetPopupUrl(); const s = await getState(); const activeAddress = await getActiveAddress(); const allowed = activeAddress ? s.allowedSites[activeAddress] || [] : []; - tabsApi.query({}, (tabs) => { - for (const tab of tabs) { - const origin = tab.url ? new URL(tab.url).origin : ""; - const hostname = extractHostname(origin); - const hasPermission = - activeAddress && - (allowed.includes(hostname) || - connectedSites[origin + ":" + activeAddress]); - tabsApi.sendMessage( - tab.id, - { - type: "AUTISTMASK_EVENT", - eventName: "accountsChanged", - data: hasPermission ? [activeAddress] : [], - }, - () => { - // Ignore errors for tabs without content script - if (runtime.lastError) { - // expected for tabs without our content script - } - }, - ); - } - }); + let tabs; + try { + tabs = await tabsQuery({}); + } catch { + return; + } + for (const tab of tabs) { + const origin = tab.url ? new URL(tab.url).origin : ""; + const hostname = extractHostname(origin); + const hasPermission = + activeAddress && + (allowed.includes(hostname) || + connectedSites[origin + ":" + activeAddress]); + // Same as chainChanged above: a tab without our content script + // rejects, and that is expected rather than a fault. + tabsSendMessage(tab.id, { + type: "AUTISTMASK_EVENT", + eventName: "accountsChanged", + data: hasPermission ? [activeAddress] : [], + }).catch(() => {}); + } } // Background balance refresh: every 60 seconds when the popup isn't open. @@ -832,8 +856,8 @@ 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. -if (windowsApi && windowsApi.onRemoved) { - windowsApi.onRemoved.addListener((windowId) => { +if (windowsNs && windowsNs.onRemoved) { + windowsNs.onRemoved.addListener((windowId) => { for (const [id, approval] of Object.entries(pendingApprovals)) { if (approval.windowId !== windowId) continue; const rejection = diff --git a/src/content/index.js b/src/content/index.js index a31aed7..489848a 100644 --- a/src/content/index.js +++ b/src/content/index.js @@ -1,12 +1,20 @@ // AutistMask content script — bridges between inpage (window.ethereum) // and the background service worker via extension messaging. +const { + hasBrowserNamespace, + runtimeApi, + sendMessage, + storageGet, + storageSet, +} = require("../shared/browserApi"); + // In Chrome (MV3), inpage.js runs as a MAIN-world content script declared // in the manifest, so no injection is needed here. In Firefox (MV2), the // "world" key is not supported, so we inject via a