From 8c92c143e14dd83f3a97724bd9e8d1437480830a 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. `storageGet()` and `storageSet()` reject where `storage.local` is absent. They carry the wallet: resolving `{}` would make an existing wallet read back as no wallet, and a no-op write would discard the user's state with nothing logged. The one caller that genuinely degrades, `src/shared/phishingDomains.js`, takes `storageLocal()` directly and keeps its own null check. 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, and the comments in `browserApi.js` and the Firefox suite say that rather than the refuted claim. An unhandled promise rejection fails a run on both harnesses, measured by throwing past the first await of the unawaited `approval.show()`: Firefox reports it from the console-service drain and Chrome as a `pageerror`. README.md records the demonstration. 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 | 87 +++-- TODO.md | 18 + src/background/index.js | 244 ++++++------ src/content/index.js | 72 ++-- src/popup/index.js | 6 + 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 | 262 +++++++++++++ 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 | 675 +++++++++++++++++++++++++++++++++- tests/e2e/network.js | 8 + 17 files changed, 1639 insertions(+), 286 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 76fd568..36302e5 100644 --- a/README.md +++ b/README.md @@ -240,10 +240,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 @@ -265,20 +273,39 @@ 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. + +An **unhandled promise rejection counts as an uncaught error** on both suites, +which matters because a good deal of popup code is now `async` and called +without an `await`. Demonstrated, not assumed: a `throw` placed past the first +`await` of `approval.show()` — which nothing awaits — turns the +`eth_requestAccounts` step red on Firefox +(`uncaught extension errors during this step`, from the console-service drain) +and on Chrome (`pageerror`), with the rest of the run unaffected because the +approval view had already rendered. + +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: @@ -302,17 +329,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 b448a45..4f4baec 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,24 @@ undefined identifiers, which is how # Completed Steps +- 2026-08-17: 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. `storageGet()` and + `storageSet()` **reject** where `storage.local` is absent rather than + resolving `{}` and a no-op write — they carry the wallet, and defaulting would + read an existing wallet back as none. The one caller that genuinely degrades, + [`src/shared/phishingDomains.js`](src/shared/phishingDomains.js), takes + `storageLocal()` directly and keeps its own null check. - 2026-08-17: An address total no longer reports `$0.00` for holdings it cannot price. Prices exist for the top 25 tokens only, so the priced-only sum was printed as the total and an address holding nothing but unpriced ERC-20s was diff --git a/src/background/index.js b/src/background/index.js index cbb6f40..e761403 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -40,17 +40,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 = {}; @@ -167,7 +171,7 @@ function approvedNonce(approvedTx) { } async function getState() { - const result = await storageApi.get("autistmask"); + const result = await storageGet("autistmask"); return ( result.autistmask || { wallets: [], @@ -231,8 +235,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" }); } } @@ -336,58 +340,71 @@ 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) => { - const approval = pendingApprovals[id]; - if (!approval) { - // Settled while the window was opening — an address switch, - // say. Nothing is waiting on it, and a window showing an - // approval that no longer exists is not left on screen. - if (win) { - windowsApi.remove(win.id, () => { - if (runtime.lastError) { - // window already closed - } - }); - } - return; - } - if (!win) { - // No window means no way to ever answer this approval, and an - // approval nothing can answer holds the requesting page's - // promise open forever. Settle it now instead. - settleApproval( - id, - abandonedResult( - approval, - APPROVAL_WINDOW_FAILED_CODE, - APPROVAL_WINDOW_FAILED_MESSAGE, - ), - ); - return; - } - approval.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) { + // The promise namespace reports the failure by rejecting where the + // callback namespace reported it by handing back no window; both land + // on the !win branch below, which settles the approval. + log.errorf("could not open the approval window:", e); + } + + const approval = pendingApprovals[id]; + if (!approval) { + // Settled while the window was opening — an address switch, say. + // Nothing is waiting on it, and a window showing an approval that no + // longer exists is not left on screen. The await above makes this a + // real race: writing the id back would resurrect a bare entry that + // nothing would ever resolve. + if (win) windowsRemove(win.id).catch(() => {}); + return; + } + if (!win) { + // No window means no way to ever answer this approval, and an + // approval nothing can answer holds the requesting page's promise + // open forever. Settle it now instead. + settleApproval( + id, + abandonedResult( + approval, + APPROVAL_WINDOW_FAILED_CODE, + APPROVAL_WINDOW_FAILED_MESSAGE, + ), + ); + return; + } + approval.windowId = win.id; } // Open an approval popup and return a promise that resolves with the user decision. @@ -397,12 +414,12 @@ function requestApproval(origin, hostname) { const id = crypto.randomUUID(); pendingApprovals[id] = { 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)); } @@ -469,7 +486,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:")) { @@ -883,24 +900,26 @@ async function handleSendTransaction(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 @@ -921,41 +940,36 @@ async function broadcastAccountsChanged() { ); 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. @@ -1050,8 +1064,8 @@ startBackgroundJobs(); // 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. -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 = abandonedResult( 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