diff --git a/README.md b/README.md index 3092e1e..dcf5ca6 100644 --- a/README.md +++ b/README.md @@ -101,6 +101,16 @@ intercepted at the browser level and served from fixtures in unrecognised outbound requests are reported as failures rather than silently allowed. +That interception covers the MV3 background service worker as well as the popup +page, which it does not by default — `script/test-e2e` sets +`PW_EXPERIMENTAL_SERVICE_WORKER_NETWORK_EVENTS=1` for it. Because that flag is +experimental, the harness does not take it on trust: at launch it fetches a +`.invalid` URL from inside the service worker that only the route handler can +answer, and aborts the entire suite if the answer does not come back. Escaping +traffic fails the run instead of disappearing. To see what is actually being +intercepted, run with `E2E_TRACE_NETWORK=1` and every routed request is printed, +tagged `[sw]` or `[page]`. + **Any uncaught page error or `console.error` fails the run.** That is the point: a `ReferenceError` from a used-but-not-imported identifier is invisible to `make check` (`script/lint` is only `prettier --check`) but fatal in a browser, diff --git a/TODO.md b/TODO.md index 8a40ed3..0f0f652 100644 --- a/TODO.md +++ b/TODO.md @@ -32,7 +32,9 @@ review. `script/test-e2e`) driving the real popup with all network intercepted, plus the two used-but-not-imported crashes it caught: AddToken unreachable (#150) and TransactionDetail broken for every ERC-20 transfer (#151). Harness - demonstrated failing before the fixes and passing after (#181). + demonstrated failing before the fixes and passing after (#181). Interception + covers the MV3 background service worker, not just the popup page, and a + launch-time canary aborts the suite if worker traffic starts escaping. - 2026-08-09: Reviewed the repo end to end and filed the 1.0.0 backlog (#149-#168). - 2026-07-26: About well in settings with build info, repo link and the version @@ -63,9 +65,12 @@ review. - Decide the libsodium backend that actually ships (#182) and delete the single allowlist entry it owns in `tests/e2e/harness.js`. - Extend the end-to-end suite to the dApp approval signing path (EIP-1193 - through the real content script, background worker and approval popup), and - decide separately whether docker-in-docker makes `make test-e2e` runnable in - the Gitea workflow. + through the real content script, background worker and approval popup). That + path needs a CDP-based route to background-worker console output first: + Playwright exposes no error event for service workers, so an uncaught + exception in the worker cannot fail the run today (worker network traffic is + already covered). Decide separately whether docker-in-docker makes + `make test-e2e` runnable in the Gitea workflow. - Make the Firefox target functional: Chrome callback APIs are used against the promise-only `browser` namespace (#153). - Send and transaction-flow correctness: gas fee excluded from the diff --git a/script/test-e2e b/script/test-e2e index 2a604d0..ece9a5d 100755 --- a/script/test-e2e +++ b/script/test-e2e @@ -35,10 +35,26 @@ main() { # 64MB /dev/shm or renderers crash. # --user: keep files the suite touches owned by the caller, not root. # HOME=/tmp: the mapped uid has no home directory in the image. + # PW_EXPERIMENTAL_SERVICE_WORKER_NETWORK_EVENTS=1: without it, + # ctx.route() intercepts page requests only, and every fetch made by + # the MV3 background service worker — including the phishing + # blocklist fetch that src/background/index.js issues at worker + # startup — goes to the real internet. The flag is experimental and + # Playwright may drop or rename it. It cannot break silently: the + # harness probes service-worker interception at launch and aborts + # the whole suite if it is not in effect (see the interception + # canary in tests/e2e/harness.js). If a future Playwright removes + # the flag, that probe is what will fail, and the fix is either a + # replacement mechanism or an honest downgrade of the isolation + # claim in tests/e2e/network.js and README.md — not deleting the + # probe. The image is pinned by digest, so this can only ever bite + # on a deliberate bump. docker run --rm \ --ipc=host \ --user "$(id -u):$(id -g)" \ -e HOME=/tmp \ + -e PW_EXPERIMENTAL_SERVICE_WORKER_NETWORK_EVENTS=1 \ + -e "E2E_TRACE_NETWORK=${E2E_TRACE_NETWORK:-0}" \ -v "$ROOT:/work" \ -w /work \ "$IMAGE" \ diff --git a/tests/e2e/harness.js b/tests/e2e/harness.js index 14491cc..24e3da6 100644 --- a/tests/e2e/harness.js +++ b/tests/e2e/harness.js @@ -74,19 +74,61 @@ function attachErrorListeners(ctx, errors) { ctx.on("page", attachPage); // Per-page listeners only: the context-level "weberror" event covers // the same page exceptions and would double-report them. Playwright - // exposes no error event for service workers, so an uncaught error in - // the background worker is not visible here — everything this suite - // drives lives in the popup page. + // exposes no error EVENT for service workers, so an uncaught + // exception in the background worker is not visible here — everything + // this suite drives lives in the popup page. That is an error-channel + // gap only: worker NETWORK traffic is intercepted and reported like + // any other, and assertWorkerTrafficIntercepted() below fails the run + // if it ever stops being. } -// The extension id is derived from the unpacked path, so it changes and -// must never be hardcoded. It is the host part of the service worker URL. -async function extensionId(ctx) { - let [sw] = ctx.serviceWorkers(); - if (!sw) { - sw = await ctx.waitForEvent("serviceworker", { timeout: 30000 }); - } - return new URL(sw.url()).host; +async function serviceWorker(ctx) { + const [existing] = ctx.serviceWorkers(); + if (existing) return existing; + return ctx.waitForEvent("serviceworker", { timeout: 30000 }); +} + +// How long to wait for the background worker's first outbound request. +// Measured at roughly 650ms after the route is installed; the margin is +// for a loaded machine, not for hope. +const WORKER_TRAFFIC_TIMEOUT_MS = 30000; + +// ctx.route() only sees service-worker requests when Playwright runs with +// PW_EXPERIMENTAL_SERVICE_WORKER_NETWORK_EVENTS=1, which script/test-e2e +// sets. Without it the worker's traffic — notably the phishing blocklist +// fetch src/background/index.js issues at startup — goes to the real +// internet, and nothing says so, because src/shared/phishingDomains.js +// swallows fetch failures. A harness whose isolation can lapse in silence +// is worthless, so this does not take the flag on trust: the background +// worker's own startup fetch has to show up in the route handler, or the +// suite refuses to run. +// +// Deliberately NOT a synthetic probe fetched through worker.evaluate(): +// evaluating in an extension worker this early kills it (the call fails +// with "Target page, context or browser has been closed" and the worker +// disappears), which would break the very thing being measured. Observing +// traffic the extension already generates costs nothing and cannot +// perturb it. +async function assertWorkerTrafficIntercepted(stubs) { + const seen = await stubs.waitForServiceWorkerTraffic( + WORKER_TRAFFIC_TIMEOUT_MS, + ); + if (seen) return seen; + + throw new Error( + "no service-worker request reached the route handler within " + + WORKER_TRAFFIC_TIMEOUT_MS + + "ms, so background worker traffic is escaping this harness and " + + "going to the real internet. Run the suite through " + + "script/test-e2e, which sets " + + "PW_EXPERIMENTAL_SERVICE_WORKER_NETWORK_EVENTS=1. If a " + + "Playwright upgrade dropped that flag, replace the mechanism or " + + "downgrade the isolation claims in tests/e2e/network.js and " + + "README.md — do not delete this check. If instead the " + + "background worker legitimately stopped making startup " + + "requests, this check needs a new anchor, because there is no " + + "longer any worker traffic to observe", + ); } async function launch(routeOpts) { @@ -112,27 +154,50 @@ async function launch(routeOpts) { // The container runs unprivileged; Chrome's sandbox needs // capabilities the harness deliberately does not grant it. "--no-sandbox", + // Belt to the interception braces: nothing that slips past + // the route handler can resolve a name, so a request that + // escapes cannot actually reach the internet. Detection is + // still assertWorkerTrafficIntercepted()'s job — this only + // bounds the damage while a gap goes unnoticed. Playwright + // fulfils routed requests without touching the resolver, and + // it drives the browser over a pipe, so neither is affected. + "--host-resolver-rules=MAP * ~NOTFOUND", ], }); - const errors = new ErrorCollector(); - attachErrorListeners(ctx, errors); - routeOpts.report = (text) => errors.record("network", text); - await installNetworkStubs(ctx, routeOpts); - - const id = await extensionId(ctx); - const popupUrl = "chrome-extension://" + id + "/src/popup/index.html"; - - return { - ctx, - errors, - extensionId: id, - popupUrl, - async close() { - await ctx.close(); - fs.rmSync(userDir, { recursive: true, force: true }); - }, + const cleanup = async () => { + await ctx.close().catch(() => {}); + fs.rmSync(userDir, { recursive: true, force: true }); }; + + try { + const errors = new ErrorCollector(); + attachErrorListeners(ctx, errors); + routeOpts.report = (text) => errors.record("network", text); + const stubs = await installNetworkStubs(ctx, routeOpts); + + await assertWorkerTrafficIntercepted(stubs); + + // The extension id is derived from the unpacked path, so it + // changes and must never be hardcoded. It is the host part of the + // service worker URL. + const sw = await serviceWorker(ctx); + const id = new URL(sw.url()).host; + + return { + ctx, + errors, + extensionId: id, + popupUrl: "chrome-extension://" + id + "/src/popup/index.html", + close: cleanup, + }; + } catch (e) { + // Anything that fails after the browser is up has to tear it down + // on the way out: an orphaned context keeps node alive forever, + // turning a clean failure into a hung run. + await cleanup(); + throw e; + } } // ---------------------------------------------------------------- flows @@ -177,9 +242,6 @@ async function openAddressDetail(page) { } module.exports = { - ALLOWED_ERRORS, - EXT_PATH, - REPO_ROOT, createWallet, launch, openAddressDetail, diff --git a/tests/e2e/network.js b/tests/e2e/network.js index 8387c19..f5abf0e 100644 --- a/tests/e2e/network.js +++ b/tests/e2e/network.js @@ -1,11 +1,22 @@ // Browser-level network interception for the end-to-end suite. // -// Every http(s) request the extension makes is fulfilled from these +// Every http(s) request the extension makes — from the popup page AND +// from the MV3 background service worker — is fulfilled from these // fixtures, so the suite is deterministic and runs entirely offline. The // probe that motivated this harness (see issue #181) observed live calls // to Blockscout returning 401 inside the container, which would make any // assertion about rendered transaction data worthless. // +// Service-worker coverage is not free: ctx.route() only sees worker +// traffic when PW_EXPERIMENTAL_SERVICE_WORKER_NETWORK_EVENTS=1 is set in +// the environment, which script/test-e2e does. Without it the phishing +// blocklist fetch that src/background/index.js issues at worker startup +// silently reaches raw.githubusercontent.com on the open internet, and +// src/shared/phishingDomains.js swallows the failure so nothing surfaces +// it. That is not left to trust: waitForServiceWorkerTraffic() below +// backs the launch-time canary in harness.js, which fails the entire +// suite if worker requests stop being visible here. +// // Anything not explicitly stubbed here is aborted AND reported to the // error collector, so a newly added outbound call shows up as a test // failure rather than as intermittent flakiness. @@ -144,16 +155,45 @@ function handleRpc(route, postData, report) { * @param {boolean} [opts.seedTokenTransfer] serve the stubbed ERC-20 * transfer. Read at request time, so a test can flip it on the same * options object without re-registering the route. + * @returns {Promise<{waitForServiceWorkerTraffic: (ms: number) => + * Promise}>} */ async function installNetworkStubs(ctx, opts) { const report = opts.report; + // First request seen that originated in a service worker, and the + // resolver waiting for it. This is what proves worker interception is + // actually in force; see waitForServiceWorkerTraffic below. + let firstWorkerRequest = null; + let announceWorkerRequest = null; + + // E2E_TRACE_NETWORK=1 prints every request that reaches this handler, + // tagged [sw] when it originated in the background service worker. + // It exists so the isolation claim above can be re-checked by anyone + // in one command, without editing files: the phishing blocklist fetch + // showing up with an [sw] tag is the proof that the worker really is + // intercepted and that the raw.githubusercontent.com stub below is + // live code rather than decoration. + const trace = process.env.E2E_TRACE_NETWORK === "1"; + // Regex rather than a glob so chrome-extension:// resource loads are // never touched — routing those would break the popup itself. await ctx.route(/^https?:\/\//, async (route) => { const req = route.request(); const url = new URL(req.url()); const p = url.pathname; + const fromWorker = !!req.serviceWorker(); + + if (fromWorker && !firstWorkerRequest) { + firstWorkerRequest = req.method() + " " + req.url(); + if (announceWorkerRequest) + announceWorkerRequest(firstWorkerRequest); + } + + if (trace) { + const origin = fromWorker ? "[sw] " : "[page] "; + console.log("# routed " + origin + req.method() + " " + req.url()); + } // JSON-RPC endpoint (any host): a POST with a JSON-RPC body. if (req.method() === "POST") { @@ -213,11 +253,38 @@ async function installNetworkStubs(ctx, opts) { report("unstubbed request: " + req.method() + " " + req.url()); return route.abort(); }); + + return { + /** + * Resolve with the first service-worker-originated request this + * handler saw, or null if none arrives within `ms`. + * + * The background worker fetches the phishing blocklist at + * startup, unconditionally, within about a second of the context + * coming up — so under working interception this resolves almost + * immediately. Nothing arriving means worker traffic is bypassing + * the handler entirely and going to the real internet, which the + * caller turns into a hard failure of the whole suite. + */ + waitForServiceWorkerTraffic(ms) { + if (firstWorkerRequest) return Promise.resolve(firstWorkerRequest); + return new Promise((resolve) => { + const timer = setTimeout(() => { + announceWorkerRequest = null; + resolve(null); + }, ms); + announceWorkerRequest = (req) => { + clearTimeout(timer); + announceWorkerRequest = null; + resolve(req); + }; + }); + }, + }; } module.exports = { installNetworkStubs, STUB_TOKEN, - STUB_COUNTERPARTY, STUB_TX_HASH, }; diff --git a/tests/e2e/run.js b/tests/e2e/run.js index 8cf6e56..8fc6a49 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -116,15 +116,28 @@ test("transaction detail renders an ERC-20 transfer (#151)", async (env) => { // ---------------------------------------------------------------- runner async function main() { + // A suite that runs nothing must never report success. If a refactor + // drops the registrations above, or a require() of this file stops + // reaching them, the only honest outcome is a red run — reporting + // "0/0 passed" and exiting 0 is the same vacuous-check failure this + // whole harness exists to prevent. + if (tests.length === 0) { + console.log("1..0"); + console.log("# FAILED: the e2e suite registered no tests"); + process.exitCode = 1; + return; + } + const routeOpts = { seedTokenTransfer: false }; let session; try { session = await launch(routeOpts); } catch (e) { - // Never skip and report success: a browser we cannot start is a - // failure of the suite, not an absent one. - console.error("e2e: could not start the browser: " + e.message); + // Never skip and report success: a browser we cannot start, or + // one whose network interception is not in force, is a failure of + // the suite, not an absent one. + console.error("e2e: cannot run the suite: " + e.message); process.exitCode = 1; return; } @@ -141,9 +154,13 @@ async function main() { let failed = 0; let n = 0; + // Starts at zero rather than at the current mark on purpose: errors + // and escaping requests recorded during launch — before any test ran, + // which is when the background worker does its startup fetches — are + // attributed to the first test instead of being discarded. + let mark = 0; for (const t of tests) { n += 1; - const mark = session.errors.mark(); let failure = null; try { await withTimeout(t.fn(env), t.name); @@ -155,6 +172,7 @@ async function main() { // provoked it, whether or not its assertions passed. This is the // mechanism that caught #150. const newErrors = session.errors.since(mark); + mark = session.errors.mark(); if (!failure && newErrors.length > 0) { failure = "uncaught browser errors during this test"; }