diff --git a/TODO.md b/TODO.md index 68c9dcd..e6e84a4 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,37 @@ but the review is broader than any of them. # Completed Steps +- 2026-08-23: The background no longer reads or writes the shared `state` + singleton ([#324](https://git.eeqj.de/sneak/AutistMask/issues/324)), which + also closes the cold-worker wrong-chain send + ([#320](https://git.eeqj.de/sneak/AutistMask/issues/320)). One in-memory copy + loaded once is the popup's lifetime, not the MV3 worker's: the worker is + killed when idle, nothing loaded state at module scope, and an unpopulated + read was answered out of `DEFAULT_STATE` in silence. Five defects traced to + that, and every point fix added a `loadState()` that created the next one — a + load detaches the objects an in-flight handler is holding. The background now + has its own storage layer (`src/background/state.js`): `getState()` for a + detached per-call read, `updateState()` for a queued read-modify-write. + `backgroundRefresh()` refreshes a private copy and applies the balances that + came back by address, so a wallet added, renamed or deleted during the round + trip survives. The transaction attempt takes its chain id and its endpoint + from one snapshot, so a committed chain switch can no longer move the endpoint + under an artifact already verified against the old chain. `getProvider()` now + REQUIRES the network id, which is what closes + [#320](https://git.eeqj.de/sneak/AutistMask/issues/320) at the shape rather + than at the call site. The prohibition is enforced by `build.js`, which fails + the build when esbuild's own metafile reports `src/shared/state.js` as an + input of either background bundle — the resolution the shipped bundle was + actually built from, so no specifier syntax and no resolution rule can slip + past it, and `make build` runs in CI. An ESLint rule that walks the require + graph textually gives the same answer in the editor, before a full bundle; it + is fast feedback rather than the guarantee, and the shapes it is known to + catch are pinned by `tests/backgroundStateLintRule.test.js`. Reading an + unloaded singleton now throws `StateNotLoadedError` instead of serving + defaults. The `chrome.storage.local` stubs in eight test files aliased instead + of structured-cloning, which could let an assertion pass on a build that never + wrote anything; every test that drives real persistence now goes through + `tests/support/storageStub.js`. - 2026-08-23: A swap always names its output token ([#346](https://git.eeqj.de/sneak/AutistMask/issues/346)). The `Token Out` detail line in `src/shared/uniswap.js` was pushed only when a symbol was diff --git a/build.js b/build.js index 5cefa60..4bc0ef0 100644 --- a/build.js +++ b/build.js @@ -16,6 +16,26 @@ const SRC = path.join(__dirname, "src"); // rotting with it. const AUDITED_MODULE = "src/shared/constants.js"; +// Modules a given entry point's bundle may not contain, keyed by the +// repo-relative entry point. +// +// src/shared/state.js is a module-level object loaded once by loadState() and +// mutated in place from then on. That is the popup's model. The MV3 service +// worker has no "once" — it is killed when idle and revived by the next +// message — so a background read of it is answered out of DEFAULT_STATE. Five +// defects came from that, one of which destroyed a wallet +// (https://git.eeqj.de/sneak/AutistMask/issues/324); the background has its own +// per-call storage layer in src/background/state.js instead. +// +// This is the authoritative check, and it is here rather than in the linter +// because it consults the resolution esbuild actually performed. Any specifier +// syntax, any hop, any resolution rule that puts the module in the bundle fails +// the build, whether or not a text matcher would have recognized it. +// Dockerfile:42 runs `make build`, so it is enforced in CI. +const FORBIDDEN_INPUTS = { + "src/background/index.js": ["src/shared/state.js"], +}; + // The build receipt: every file this build emits, with its sha256 and whether // it is one of the audited bundles. script/verify-build is handed this and // checks dist/ against it, so the file list comes from the build that just ran @@ -65,6 +85,71 @@ function outputsContainingAuditedModule(metafile) { .map(([outFile]) => repoRelative(outFile)); } +// Shortest import chain from `entryInput` to `target` through the metafile's +// own input graph, or null when there is none. The message this feeds is the +// point of the check: "state.js is in the worker bundle" is not actionable on +// its own, "index.js -> chainSwitchFields.js -> state.js" is. +function importChain(metafile, entryInput, target) { + const graph = new Map( + Object.entries(metafile.inputs).map(([input, info]) => [ + repoRelative(input), + (info.imports || []).map((i) => repoRelative(i.path)), + ]), + ); + const start = repoRelative(entryInput); + const seen = new Set([start]); + const queue = [[start]]; + while (queue.length > 0) { + const chain = queue.shift(); + for (const next of graph.get(chain[chain.length - 1]) || []) { + if (next === target) return chain.concat([next]); + if (seen.has(next)) continue; + seen.add(next); + queue.push(chain.concat([next])); + } + } + return null; +} + +// Entry points from FORBIDDEN_INPUTS that this build actually bundled. A key +// that matches nothing means the table has rotted away from the entry point +// list — the prohibition would then be silently unenforced, so the build fails +// on it rather than passing vacuously. +const forbiddenEntriesSeen = new Set(); + +// Fail the build when an entry point's bundle contains a module it is +// prohibited from reaching. The inputs come from esbuild's metafile, so this is +// the resolution the shipped bundle was built from and not a guess at it. +function assertNoForbiddenInputs(entryPoint, outfile, metafile) { + const entry = repoRelative(entryPoint); + const forbidden = FORBIDDEN_INPUTS[entry]; + if (!forbidden) return; + forbiddenEntriesSeen.add(entry); + + const out = repoRelative(outfile); + const entryOutput = Object.entries(metafile.outputs).find( + ([outFile]) => repoRelative(outFile) === out, + ); + if (!entryOutput) { + throw new Error(`esbuild reported no metafile output for ${out}`); + } + const inputs = new Set( + Object.keys(entryOutput[1].inputs).map(repoRelative), + ); + + for (const module of forbidden) { + if (!inputs.has(module)) continue; + const chain = importChain(metafile, entryPoint, module); + throw new Error( + `${out} bundles ${module}, which ${entry} must not reach` + + `${chain ? `: ${chain.join(" -> ")}` : ""}. The MV3 worker ` + + `never populates the shared state singleton, so reading it ` + + `serves DEFAULT_STATE. Use getState()/updateState() from ` + + `src/background/state.js instead.`, + ); + } +} + // Every file this build writes under dist/, recorded as it is written. This is // the build's own account of what it emitted; it is never recovered by // listing dist/, because a file that is in dist/ without this build having put @@ -293,6 +378,9 @@ async function build() { metafile: true, define, }); + // Before the output is recorded as emitted: a bundle that violates a + // prohibition must abort the build, not be written into a receipt. + assertNoForbiddenInputs(entryPoint, outfile, result.metafile); recordEmitted(outfile); auditedBundles.push(...outputsContainingAuditedModule(result.metafile)); } @@ -349,6 +437,15 @@ async function build() { path.join(DIST_FIREFOX, "manifest.json"), ); + for (const entry of Object.keys(FORBIDDEN_INPUTS)) { + if (!forbiddenEntriesSeen.has(entry)) { + throw new Error( + `${entry} is listed in FORBIDDEN_INPUTS but was not bundled, ` + + `so nothing checked it`, + ); + } + } + // Written last so a build that died partway through leaves no receipt at // all, which script/verify-build treats as a hard failure rather than as // "nothing to check". diff --git a/eslint.config.js b/eslint.config.js index 9f0b57f..b9cfdac 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -8,6 +8,7 @@ const js = require("@eslint/js"); const globals = require("globals"); +const backgroundState = require("./script/lib/eslint/noStateSingletonInBackground"); // The extension APIs. MV3 Chrome exposes `chrome`; Firefox exposes both, and // the code feature-detects between them. @@ -78,12 +79,28 @@ module.exports = [ }, // MV3 background: a service worker, with no window and no document. + // + // It also may not reach src/shared/state.js. That module's `state` export + // is a per-bundle singleton loaded once and mutated in place, which is the + // popup's lifetime and not the worker's: the worker is killed when idle, + // nothing loads state at module scope, and an unpopulated read used to be + // served DEFAULT_STATE silently. Five defects came from background code + // reading or writing it (https://git.eeqj.de/sneak/AutistMask/issues/324), + // and each point fix added a loadState() that created the next one. The + // rule below checks reachability through the whole require graph, not just + // the direct require, because a re-export from any shared module the + // background already pulls in would put the singleton back in the bundle + // with no background file naming it. { files: ["src/background/**/*.js"], + plugins: { background: backgroundState }, languageOptions: { ...commonjs, globals: { ...globals.serviceworker, ...extensionGlobals }, }, + rules: { + "background/no-state-singleton-in-background": "error", + }, }, // src/shared is bundled into both, so it may only use what both provide: @@ -107,9 +124,9 @@ module.exports = [ }, }, - // Unit tests: jest on node. + // Unit tests, and the helpers they require: jest on node. { - files: ["tests/**/*.test.js"], + files: ["tests/**/*.test.js", "tests/support/**/*.js"], languageOptions: { ...commonjs, globals: { ...globals.node, ...globals.jest }, diff --git a/script/lib/eslint/noStateSingletonInBackground.js b/script/lib/eslint/noStateSingletonInBackground.js new file mode 100644 index 0000000..de0bcf2 --- /dev/null +++ b/script/lib/eslint/noStateSingletonInBackground.js @@ -0,0 +1,184 @@ +// ESLint rule: the background bundle may not reach the shared state singleton. +// +// src/shared/state.js holds a module-level `state` object, loaded once by +// loadState() and mutated in place from then on. That is the popup's model. In +// the MV3 service worker there is no "once": the worker is terminated when +// idle and revived by the next message, nothing loads state at module scope, +// and an unpopulated read used to be served DEFAULT_STATE without complaint — +// five defects, one cause +// (https://git.eeqj.de/sneak/AutistMask/issues/324). The background has its +// own per-call storage layer in src/background/state.js instead. +// +// THIS RULE IS NOT THE GUARANTEE, and must not be described as one. The +// guarantee is in build.js: FORBIDDEN_INPUTS / assertNoForbiddenInputs() fails +// the build when esbuild's own metafile reports src/shared/state.js as an input +// of a background bundle. That consults the resolution esbuild actually +// performed, so no specifier syntax and no resolution rule can slip past it, +// and Dockerfile:42 runs `make build` in CI. +// +// What this rule is: fast local feedback, in the editor and in `make lint`, +// before a full bundle. It reads sources from disk and matches import +// specifiers TEXTUALLY, so it is a best-effort approximation of module +// resolution — a hand-rolled matcher will diverge from a real bundler, and two +// earlier revisions of this file proved it by shipping holes (a template +// literal, a dynamic `import()`, a comment inside the call, a directory +// resolved through `package.json` `main`). Those are all covered now, and the +// next divergence is caught by the build rather than by widening this again. +// +// It checks REACHABILITY, not just the direct require: the singleton is one +// `require()` away from any shared module the background pulls in, and a +// re-export would put it back in the bundle without any background file naming +// it. So each background file is the root of a walk over the CommonJS require +// graph, and the error names the whole chain that brought the singleton in. +// +// Matching textually over-approximates — a specifier inside a comment or a +// string counts — which is the safe direction here: the failure mode is a +// spurious error naming an exact file and line, not a silent hole. +// +// Deliberately not matched: a computed specifier, `require("../shared/" + +// "state")`. esbuild cannot resolve that statically either, so it never +// reaches the bundle. + +const fs = require("fs"); +const path = require("path"); + +// The module this rule exists to keep out, relative to the repo root. +const FORBIDDEN = path.join("src", "shared", "state.js"); + +// Whatever may sit between a keyword, a paren and a specifier: whitespace and +// comments. `import(/* webpackChunkName: "x" */ "./x")` is a standard bundler +// idiom, and an inline `/* eslint-… */` is just as ordinary, so a matcher that +// allows only \s there is not strict, it is broken. Each alternative starts +// with a distinct character, so this cannot backtrack quadratically. +const GAP = "(?:\\s|/\\*[^]*?\\*/|//[^\\n]*)"; +const SPECIFIER = "[\"'`]([^\"'`]+)[\"'`]"; + +// Both alternatives capture the specifier: call form first +// (`require(...)`/`import(...)`), then clause form (`from "x"`, and the bare +// side-effect `import "x"`). Nothing after the specifier is matched, so a +// trailing comment or a trailing comma cannot break the match either. +const SPECIFIER_RE = new RegExp( + `\\b(?:require|import)${GAP}*\\(${GAP}*${SPECIFIER}` + + `|\\b(?:from|import)${GAP}+${SPECIFIER}`, + "g", +); + +// The `main` of a directory's package.json, as a specifier relative to that +// directory, or null. esbuild resolves a directory through it, so a walk that +// stops at `/index.js` reports a specifier it matched perfectly well as +// unresolvable. +function packageMain(dir) { + try { + const pkg = JSON.parse( + fs.readFileSync(path.join(dir, "package.json"), "utf8"), + ); + return typeof pkg.main === "string" && pkg.main ? pkg.main : null; + } catch { + return null; + } +} + +// Resolve a relative require to a file path, trying what node and esbuild would +// in the order they would: the path itself, then extensions, then the directory +// (its package.json `main`, then its index.js). +function resolveRelative(fromFile, spec) { + if (!spec.startsWith(".")) return null; // a package, not our tree + const base = path.resolve(path.dirname(fromFile), spec); + const main = packageMain(base); + for (const candidate of [ + base, + base + ".js", + base + ".json", + ...(main + ? [path.resolve(base, main), path.resolve(base, main) + ".js"] + : []), + path.join(base, "index.js"), + ]) { + try { + if (fs.statSync(candidate).isFile()) return candidate; + } catch { + // Not this candidate. + } + } + return null; +} + +function requiresOf(file) { + let source; + try { + source = fs.readFileSync(file, "utf8"); + } catch { + return []; + } + const out = []; + for (const match of source.matchAll(SPECIFIER_RE)) { + const resolved = resolveRelative(file, match[1] ?? match[2]); + if (resolved) out.push(resolved); + } + return out; +} + +// Breadth-first from `entry`, returning the shortest chain of files that ends +// at the forbidden module, or null when it is not reachable. +function chainToForbidden(entry, forbidden) { + const seen = new Set([entry]); + const queue = [[entry]]; + while (queue.length > 0) { + const chain = queue.shift(); + for (const next of requiresOf(chain[chain.length - 1])) { + if (next === forbidden) return chain.concat([next]); + if (seen.has(next)) continue; + seen.add(next); + queue.push(chain.concat([next])); + } + } + return null; +} + +const rule = { + meta: { + type: "problem", + docs: { + description: + "the background bundle must not be able to reach the" + + " module-level state singleton in src/shared/state.js", + }, + schema: [], + messages: { + reachable: + "The background must not reach the shared state singleton:" + + " {{chain}}. The MV3 worker never populates it, so reading it" + + " serves DEFAULT_STATE. Use getState()/updateState() from" + + " src/background/state.js instead.", + }, + }, + + create(context) { + return { + "Program:exit"(node) { + const filename = context.filename; + // ESLint lints from the repo root, which is also where the + // forbidden path is anchored. + const forbidden = path.resolve(context.cwd, FORBIDDEN); + const chain = chainToForbidden( + path.resolve(filename), + forbidden, + ); + if (!chain) return; + context.report({ + node, + messageId: "reachable", + data: { + chain: chain + .map((file) => path.relative(context.cwd, file)) + .join(" -> "), + }, + }); + }, + }; + }, +}; + +module.exports = { + rules: { "no-state-singleton-in-background": rule }, +}; diff --git a/src/background/index.js b/src/background/index.js index 63abee1..014ac2e 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -2,19 +2,17 @@ // Handles EIP-1193 RPC requests from content scripts and proxies // non-sensitive calls to the configured Ethereum JSON-RPC endpoint. -const { DEFAULT_RPC_URL } = require("../shared/constants"); const { SUPPORTED_CHAIN_IDS, networkById, networkByChainId, } = require("../shared/networks"); -const { onChainSwitch } = require("../shared/chainSwitch"); -const { - state, - loadState, - saveState, - currentNetwork, -} = require("../shared/state"); +const { applyChainSwitchFields } = require("../shared/chainSwitchFields"); +// The background's own storage layer. src/shared/state.js — the module-level +// `state` singleton, loadState() and saveState() — is deliberately NOT +// imported here and must never be: see the header of src/background/state.js, +// and the lint rule that enforces it in eslint.config.js. +const { getState, updateState } = require("./state"); const { refreshBalances, getProvider } = require("../shared/balances"); const { debugFetch, log } = require("../shared/log"); const { @@ -42,7 +40,6 @@ const { const { actionApi, runtimeApi, - storageGet, tabsQuery, tabsSendMessage, windowsApi, @@ -179,21 +176,12 @@ const INTERNAL_ERROR_CODE = -32603; const INTERNAL_ERROR_MESSAGE = "AutistMask could not complete this request because of an internal error."; -async function getState() { - const result = await storageGet("autistmask"); - return ( - result.autistmask || { - wallets: [], - rpcUrl: DEFAULT_RPC_URL, - activeAddress: null, - allowedSites: {}, - deniedSites: {}, - } - ); -} - -async function getActiveAddress() { - const s = await getState(); +// The active address of a profile snapshot. Pure, and taking the snapshot as +// an argument rather than reading storage itself: a handler that has already +// read state must not answer "which account is this" from a SECOND, later read +// — the two can disagree, and the checks that compare them would then be +// comparing two different moments. +function activeAddressOf(s) { if (s.activeAddress) return s.activeAddress; // Fall back to first address if (s.wallets.length > 0 && s.wallets[0].addresses.length > 0) { @@ -202,6 +190,11 @@ async function getActiveAddress() { return null; } +// For the few call sites that need only the address and hold no snapshot. +async function getActiveAddress() { + return activeAddressOf(await getState()); +} + // Whether a request names a signing address other than the active one. Such a // request is refused rather than quietly signed as whichever address happens // to be active: the page asked for account A and would otherwise be handed @@ -210,9 +203,14 @@ function namesAnotherAddress(requested, activeAddress) { return !!requested && !sameAddress(requested, activeAddress); } +// The endpoint alone, for the one caller that needs nothing else. Anything +// that also needs the network the endpoint belongs to must take both from ONE +// snapshot — see handleSendTransaction() — because a chain switch moves them +// together and a provider built from two different reads can end up pointed at +// one chain and told it is on another +// (https://git.eeqj.de/sneak/AutistMask/issues/320). async function getRpcUrl() { - const s = await getState(); - return s.rpcUrl || DEFAULT_RPC_URL; + return (await getState()).rpcUrl; } function extractHostname(origin) { @@ -553,10 +551,26 @@ runtime.onConnect.addListener((port) => { } }); +// Record a remembered site decision under one address. +// +// A read-modify-write against storage, not a load-mutate-save of a shared +// singleton: the user takes seconds to answer the prompt, and everything else +// in the worker — a balance refresh in flight, another site's approval — has +// gone on running the whole time. Loading here used to replace the very +// objects that work was holding. +async function rememberSiteChoice(field, address, hostname) { + await updateState((s) => { + if (!s[field][address]) s[field][address] = []; + if (!s[field][address].includes(hostname)) { + s[field][address].push(hostname); + } + }); +} + // Handle connection requests (eth_requestAccounts, wallet_requestPermissions) async function handleConnectionRequest(origin) { const s = await getState(); - const activeAddress = await getActiveAddress(); + const activeAddress = activeAddressOf(s); if (!activeAddress) { return { error: { message: "No accounts available" } }; } @@ -588,29 +602,14 @@ async function handleConnectionRequest(origin) { if (decision.approved) { if (decision.remember) { - // Reload state to get latest, add to allowed, persist - await loadState(); - if (!state.allowedSites[activeAddress]) { - state.allowedSites[activeAddress] = []; - } - if (!state.allowedSites[activeAddress].includes(hostname)) { - state.allowedSites[activeAddress].push(hostname); - } - await saveState(); + await rememberSiteChoice("allowedSites", activeAddress, hostname); } else { connectedSites[origin + ":" + activeAddress] = true; } return { result: [activeAddress] }; } else { if (decision.remember) { - await loadState(); - if (!state.deniedSites[activeAddress]) { - state.deniedSites[activeAddress] = []; - } - if (!state.deniedSites[activeAddress].includes(hostname)) { - state.deniedSites[activeAddress].push(hostname); - } - await saveState(); + await rememberSiteChoice("deniedSites", activeAddress, hostname); } return { error: { @@ -654,7 +653,7 @@ async function handleRpc(method, params, origin) { if (method === "eth_accounts") { const s = await getState(); - const activeAddress = await getActiveAddress(); + const activeAddress = activeAddressOf(s); if (!activeAddress) return { result: [] }; const hostname = extractHostname(origin); const allowed = s.allowedSites[activeAddress] || []; @@ -667,22 +666,11 @@ async function handleRpc(method, params, origin) { return { result: [] }; } - // Both answered from currentNetwork(), which reads the module-level state - // singleton, and nothing populates that at module scope. A worker revived - // by the page's own message therefore held DEFAULT_STATE and told a page - // it was on mainnet while the user was on Sepolia - // (https://git.eeqj.de/sneak/AutistMask/issues/317). - // - // Answered from getState() rather than by loading the singleton. Any page - // reaches these two — neither is gated on a connection, and the injected - // provider sends eth_chainId on every page load — and loadState() replaces - // state.wallets wholesale, which would detach the address objects an - // in-flight backgroundRefresh() is mutating across its network round trip, - // so its saveState() would persist the pre-refresh balances while still - // stamping lastBalanceRefresh. getState() is the detached per-call storage - // read the other read handlers here already use. - // networkById(undefined) falls back to mainnet, matching the default for a - // profile with no stored networkId. + // Both used to be answered from currentNetwork(), which reads the + // module-level state singleton, and nothing populates that at module + // scope. A worker revived by the page's own message therefore held + // DEFAULT_STATE and told a page it was on mainnet while the user was on + // Sepolia (https://git.eeqj.de/sneak/AutistMask/issues/317). if (method === "eth_chainId" || method === "net_version") { const s = await getState(); const net = networkById(s.networkId); @@ -699,7 +687,7 @@ async function handleRpc(method, params, origin) { // not be able to do it. Ungated, any page could clear the // [TESTNET] banner under a user who believed they were on Sepolia. const s = await getState(); - const activeAddress = await getActiveAddress(); + const activeAddress = activeAddressOf(s); const hostname = extractHostname(origin); const allowed = s.allowedSites[activeAddress] || []; if ( @@ -709,24 +697,25 @@ async function handleRpc(method, params, origin) { return { error: { code: 4100, message: "Unauthorized" } }; } - // onChainSwitch() mutates the module-level state singleton and then - // saves every field of it, and currentNetwork() reads the same - // singleton. This worker may have been started by this very message: - // nothing loads state at module scope, so without this the singleton - // is DEFAULT_STATE, the same-chain check compares against the wrong - // network, and the save writes empty wallets, empty allowedSites and - // the default endpoints over the user's stored profile - // (https://git.eeqj.de/sneak/AutistMask/issues/316). Same precedent - // as the transaction path below. - await loadState(); - + // The chain in force is read from the snapshot above, not from the + // singleton: this worker may have been started by this very message, + // and the singleton would then be DEFAULT_STATE, so the same-chain + // check compared against mainnet whatever the user was on + // (https://git.eeqj.de/sneak/AutistMask/issues/316). const chainId = params?.[0]?.chainId; - if (chainId === currentNetwork().chainId) { + if (chainId === networkById(s.networkId).chainId) { return { result: null }; } if (SUPPORTED_CHAIN_IDS.has(chainId)) { const target = networkByChainId(chainId); - await onChainSwitch(target.id); + // Read-modify-write against storage. The old path went through + // onChainSwitch(), which mutates the singleton and then persists + // every field of it — on an unloaded worker that wrote empty + // wallets, empty allowedSites and the default endpoints over the + // user's stored profile, encrypted secrets included. + await updateState((fresh) => + applyChainSwitchFields(fresh, target.id), + ); broadcastChainChanged(target.chainId); return { result: null }; } @@ -773,7 +762,7 @@ async function handleRpc(method, params, origin) { if (method === "wallet_getPermissions") { const s = await getState(); - const activeAddress = await getActiveAddress(); + const activeAddress = activeAddressOf(s); const hostname = extractHostname(origin); const allowed = s.allowedSites[activeAddress] || []; const isConnected = @@ -799,7 +788,7 @@ async function handleRpc(method, params, origin) { if (method === "personal_sign" || method === "eth_sign") { const s = await getState(); - const activeAddress = await getActiveAddress(); + const activeAddress = activeAddressOf(s); if (!activeAddress) return { error: { message: "No accounts available" } }; @@ -848,7 +837,7 @@ async function handleRpc(method, params, origin) { if (method === "eth_signTypedData_v4" || method === "eth_signTypedData") { const s = await getState(); - const activeAddress = await getActiveAddress(); + const activeAddress = activeAddressOf(s); if (!activeAddress) return { error: { message: "No accounts available" } }; @@ -904,7 +893,7 @@ async function handleRpc(method, params, origin) { // page has its answer. async function handleSendTransaction(params, origin) { const s = await getState(); - const activeAddress = await getActiveAddress(); + const activeAddress = activeAddressOf(s); if (!activeAddress) return { error: { message: "No accounts available" } }; const hostname = extractHostname(origin); @@ -948,10 +937,19 @@ async function handleSendTransaction(params, origin) { // user is shown is a complete one and is the same object the signed // artifact is checked against. A failure raises no approval at all and // is reported to the requesting page; see approvalTx.js. + // + // The provider is built from ONE snapshot — the endpoint and the + // network name both come from `s`. It used to be + // getProvider(await getRpcUrl()) with no network name at all, so + // getProvider fell back to the unpopulated singleton's mainnet: the + // endpoint was the user's chain and the static hint was 0x1, ethers + // fixed chainId at 0x1, and the wallet's own verifySignedTx then + // refused every non-mainnet dApp send + // (https://git.eeqj.de/sneak/AutistMask/issues/320). let approvedTx; try { approvedTx = await prepareApprovalTx( - getProvider(await getRpcUrl()), + getProvider(s.rpcUrl, s.networkId), activeAddress, txParams, ); @@ -1039,7 +1037,7 @@ async function broadcastAccountsChanged() { } resetPopupUrl(); const s = await getState(); - const activeAddress = await getActiveAddress(); + const activeAddress = activeAddressOf(s); const allowed = activeAddress ? s.allowedSites[activeAddress] || [] : []; let tabs; try { @@ -1079,20 +1077,60 @@ async function broadcastAccountsChanged() { const BALANCE_REFRESH_PERIOD_MS = BALANCE_REFRESH_PERIOD_MINUTES * 60 * 1000; const RECENT_BALANCE_REFRESH_MS = Math.floor(BALANCE_REFRESH_PERIOD_MS / 2); +// The wallets this refresh works on are its OWN, and nothing else in the +// worker can reach them. +// +// refreshBalances() mutates address objects in place across a multi-second +// network round trip. It used to be handed the module-level singleton's +// wallets, which meant any concurrent handler that called loadState() replaced +// state.wallets underneath it: the refreshed balances landed on detached +// objects, and the save that followed persisted the PRE-refresh values while +// still stamping lastBalanceRefresh, suppressing the redo. Every point fix for +// the singleton added such a loadState(), so the next one would have done it +// again (https://git.eeqj.de/sneak/AutistMask/issues/324). +// +// So: read a snapshot, refresh a private copy of its wallets, then apply the +// balances that came back — by address, onto whatever storage holds NOW. +// Applying by address rather than writing the array back is what keeps a +// wallet or address added, renamed or deleted during the round trip. async function backgroundRefresh() { - await loadState(); + const s = await getState(); const now = Date.now(); - if (now - (state.lastBalanceRefresh || 0) < RECENT_BALANCE_REFRESH_MS) - return; - if (state.wallets.length === 0) return; + if (now - (s.lastBalanceRefresh || 0) < RECENT_BALANCE_REFRESH_MS) return; + if (s.wallets.length === 0) return; + + const wallets = s.wallets; await refreshBalances( - state.wallets, - state.rpcUrl, - state.blockscoutUrl, - state.trackedTokens, + wallets, + s.rpcUrl, + s.blockscoutUrl, + s.trackedTokens, + s.networkId, ); - state.lastBalanceRefresh = now; - await saveState(); + + const refreshed = new Map(); + for (const wallet of wallets) { + for (const addr of wallet.addresses || []) { + refreshed.set(String(addr.address).toLowerCase(), addr); + } + } + + await updateState((fresh) => { + for (const wallet of fresh.wallets) { + for (const addr of wallet.addresses || []) { + const got = refreshed.get(String(addr.address).toLowerCase()); + if (!got) continue; + // Only fields the refresh actually produced. refreshBalances() + // leaves a field untouched when its lookup failed, so an + // undefined here means "no answer", not "the answer is empty", + // and must not overwrite what is stored. + for (const key of ["balance", "ensName", "tokenBalances"]) { + if (got[key] !== undefined) addr[key] = got[key]; + } + } + } + fresh.lastBalanceRefresh = now; + }); } // The recurring job runs off an alarm, not a timer. On Chrome MV3 this file is @@ -1307,16 +1345,29 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { // so an escape from there must not tell the user it might have. let lastResortStage = TX_STAGE_VERIFY; (async () => { - // The chain this attempt is on, read once. Verification below - // refuses an artifact signed for any other chain, and the nonce - // record is both consulted and written under this one, so a - // network switch part-way through cannot make the check and the - // record disagree about which chain the nonce was spent on. + // The chain this attempt is on, read once — and the endpoint it + // will be broadcast to comes from the SAME read. + // + // Verification below refuses an artifact signed for any other + // chain, and the nonce record is both consulted and written under + // this one, so a network switch part-way through cannot make the + // check and the record disagree about which chain the nonce was + // spent on. The endpoint used to be read separately, several + // awaits later (`state.rpcUrl` off the singleton), so a chain + // switch committed in that window moved the endpoint out from + // under a transaction already verified against the old chain: the + // artifact would be sent to the new chain's node, which is + // precisely the "signed for a different network" case the + // verification exists to prevent. let chainId; + let rpcUrl; + let networkId; try { - await loadState(); - chainId = currentNetwork().chainId; - const activeAddress = await getActiveAddress(); + const s = await getState(); + networkId = s.networkId; + chainId = networkById(networkId).chainId; + rpcUrl = s.rpcUrl; + const activeAddress = activeAddressOf(s); // An address switch between approval and signing refuses. The // approval named one account; signing from whichever account // is active now would send funds from an account this screen @@ -1388,7 +1439,7 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { } try { - const provider = getProvider(state.rpcUrl); + const provider = getProvider(rpcUrl, networkId); lastResortStage = TX_STAGE_BROADCAST; const tx = await provider.broadcastTransaction(msg.rawSignedTx); if (nonce !== null) spent.add(nonce); diff --git a/src/background/state.js b/src/background/state.js new file mode 100644 index 0000000..ec65559 --- /dev/null +++ b/src/background/state.js @@ -0,0 +1,76 @@ +// The background's access to the persisted profile. +// +// There is no in-memory copy here, and that is the whole design. The MV3 +// service worker is terminated when idle and revived by the next message, so +// anything held at module scope is either absent or arbitrarily stale, and +// src/shared/state.js's module-level `state` singleton — which nothing in the +// worker ever populates — silently served DEFAULT_STATE to whoever read it. +// Five defects came out of that (https://git.eeqj.de/sneak/AutistMask/issues/324), +// and every point fix for one of them added a loadState() that created the +// next: loading detaches the objects an in-flight handler is holding. +// +// So the background reads per call and writes read-modify-write: +// +// getState() one storage read, normalized, detached. Nothing else +// holds the object it returns, so a handler may keep it +// across any number of awaits and no concurrent work can +// move it. +// updateState(fn) read fresh, apply fn to that fresh record, write it +// back — all inside a queue, so two background writes +// never interleave, and the read is one storage round trip +// ahead of the write rather than a page lifetime ahead of +// it (which is what made the popup's saveState() need a +// per-field merge against a baseline at all). +// +// A handler that must both read and write therefore does its network work +// against a snapshot it owns, and applies the RESULT inside updateState(). +// It never publishes an object other in-flight work is holding. + +const { storageGet, storageSet } = require("../shared/browserApi"); +const { normalizePersisted } = require("../shared/persistedState"); + +// A fresh, fully-normalized, detached copy of the persisted profile. +// +// Normalized rather than raw: a legacy or malformed record is self-healed the +// same way loadState() heals it for the popup, so the background is never the +// one context reasoning about a shape the rest of the extension repairs. +async function getState() { + const result = await storageGet("autistmask"); + return normalizePersisted(result.autistmask); +} + +// Serializes the read-modify-write turns below. Two of them interleaved would +// each read before the other wrote, and the second write would carry the first +// one's fields back to their pre-turn values. +let updateQueue = Promise.resolve(); + +async function updateStateOnce(mutate) { + const s = await getState(); + await mutate(s); + s.hasWallet = Boolean(s.wallets && s.wallets.length > 0); + await storageSet({ autistmask: s }); + return s; +} + +// Apply `mutate` to a record read fresh from storage and write the result +// back. `mutate` receives a detached, normalized profile and mutates it in +// place; it may be async, but it must not do anything slow — the window +// between the read and the write is the window in which another context's +// write is lost, and keeping it to one storage round trip is what makes a +// whole-record write safe here. +// +// `mutate` must also not call updateState() itself, directly or through +// anything it awaits: the queue is strictly serial, so the inner turn waits on +// the outer one, which is waiting on the inner one. That deadlocks the whole +// background, not just the caller. Mutate the record you were handed. +// +// Resolves with the record that was written. +function updateState(mutate) { + const turn = updateQueue.then(() => updateStateOnce(mutate)); + // The queue must advance even when a turn rejects, or every update after + // it queues behind a promise that never settles. + updateQueue = turn.catch(() => {}); + return turn; +} + +module.exports = { getState, updateState }; diff --git a/src/popup/index.js b/src/popup/index.js index 3e20887..671c12d 100644 --- a/src/popup/index.js +++ b/src/popup/index.js @@ -53,6 +53,7 @@ async function doRefreshAndRender() { state.rpcUrl, state.blockscoutUrl, state.trackedTokens, + state.networkId, ), ]); state.lastBalanceRefresh = Date.now(); diff --git a/src/popup/views/addToken.js b/src/popup/views/addToken.js index 5f5f0ff..a408d9b 100644 --- a/src/popup/views/addToken.js +++ b/src/popup/views/addToken.js @@ -49,7 +49,11 @@ function init(ctx) { infoEl.style.visibility = "visible"; log.debugf("Looking up token contract", contractAddr); try { - const info = await lookupTokenInfo(contractAddr, state.rpcUrl); + const info = await lookupTokenInfo( + contractAddr, + state.rpcUrl, + state.networkId, + ); log.infof("Adding token", info.symbol, contractAddr); state.trackedTokens.push({ address: contractAddr, diff --git a/src/popup/views/addWallet.js b/src/popup/views/addWallet.js index 2d54a94..0d7165a 100644 --- a/src/popup/views/addWallet.js +++ b/src/popup/views/addWallet.js @@ -179,7 +179,7 @@ async function importMnemonic(ctx) { // Scan for used HD addresses beyond index 0. showFlash("Scanning for addresses...", 30000); - const scan = await scanForAddresses(xpub, state.rpcUrl); + const scan = await scanForAddresses(xpub, state.rpcUrl, state.networkId); if (scan.addresses.length > 1) { wallet.addresses = scan.addresses.map((a) => ({ address: a.address, @@ -298,7 +298,7 @@ async function importXprvKey(ctx) { // Scan for used HD addresses beyond index 0. showFlash("Scanning for addresses...", 30000); - const scan = await scanForAddresses(xpub, state.rpcUrl); + const scan = await scanForAddresses(xpub, state.rpcUrl, state.networkId); if (scan.addresses.length > 1) { wallet.addresses = scan.addresses.map((a) => ({ address: a.address, diff --git a/src/popup/views/addressDetail.js b/src/popup/views/addressDetail.js index 7f7bf45..58a7da7 100644 --- a/src/popup/views/addressDetail.js +++ b/src/popup/views/addressDetail.js @@ -188,6 +188,7 @@ async function loadTransactions(address) { ensNameMap = await resolveEnsNames( counterparties, state.rpcUrl, + state.networkId, ); } catch { ensNameMap = new Map(); diff --git a/src/popup/views/addressToken.js b/src/popup/views/addressToken.js index 2b41dae..a73e651 100644 --- a/src/popup/views/addressToken.js +++ b/src/popup/views/addressToken.js @@ -268,6 +268,7 @@ async function loadTransactions(address, tokenId) { ensNameMap = await resolveEnsNames( counterparties, state.rpcUrl, + state.networkId, ); } catch { ensNameMap = new Map(); diff --git a/src/popup/views/confirmTx.js b/src/popup/views/confirmTx.js index d6ea0a6..7395f26 100644 --- a/src/popup/views/confirmTx.js +++ b/src/popup/views/confirmTx.js @@ -304,7 +304,7 @@ function formatFeeEth(wei) { async function estimateGas(txInfo) { try { - const provider = getProvider(state.rpcUrl); + const provider = getProvider(state.rpcUrl, state.networkId); const feeData = await provider.getFeeData(); let gasLimit; @@ -386,7 +386,7 @@ async function estimateGas(txInfo) { async function checkRecipientHistory(txInfo) { try { - const provider = getProvider(state.rpcUrl); + const provider = getProvider(state.rpcUrl, state.networkId); const asyncWarnings = await getFullWarnings(txInfo.to, provider, { fromAddress: txInfo.from, }); @@ -454,7 +454,7 @@ function init(_ctx) { state.selectedAddress, decryptedSecret, ); - const provider = getProvider(state.rpcUrl); + const provider = getProvider(state.rpcUrl, state.networkId); const connectedSigner = signer.connect(provider); if (pendingTx.token === "ETH") { diff --git a/src/popup/views/send.js b/src/popup/views/send.js index 72d84ad..d88ac05 100644 --- a/src/popup/views/send.js +++ b/src/popup/views/send.js @@ -202,7 +202,7 @@ function init(_ctx) { let ensName = null; if (to.includes(".") && !to.startsWith("0x")) { try { - const provider = getProvider(state.rpcUrl); + const provider = getProvider(state.rpcUrl, state.networkId); const resolved = await provider.resolveName(to); if (!resolved) { showFlash("Could not resolve " + to); diff --git a/src/popup/views/settingsAddToken.js b/src/popup/views/settingsAddToken.js index 3c65641..205cb38 100644 --- a/src/popup/views/settingsAddToken.js +++ b/src/popup/views/settingsAddToken.js @@ -133,7 +133,11 @@ function init(_ctx) { infoEl.style.visibility = "visible"; log.debugf("Looking up token contract", addr); try { - const info = await lookupTokenInfo(addr, state.rpcUrl); + const info = await lookupTokenInfo( + addr, + state.rpcUrl, + state.networkId, + ); log.infof("Adding token", info.symbol, addr); state.trackedTokens.push({ address: addr, diff --git a/src/popup/views/txStatus.js b/src/popup/views/txStatus.js index 8587679..a79404c 100644 --- a/src/popup/views/txStatus.js +++ b/src/popup/views/txStatus.js @@ -113,7 +113,7 @@ function startWait(txInfo, txHash, broadcastTime, pollNow) { renderElapsed(); }, 1000); - const provider = getProvider(state.rpcUrl); + const provider = getProvider(state.rpcUrl, state.networkId); let consecutiveFailures = 0; async function poll() { diff --git a/src/shared/balances.js b/src/shared/balances.js index 5974603..a74f8f9 100644 --- a/src/shared/balances.js +++ b/src/shared/balances.js @@ -9,6 +9,7 @@ const { formatUnits, } = require("ethers"); const { ERC20_ABI } = require("./constants"); +const { NETWORKS } = require("./networks"); const { log, debugFetch } = require("./log"); const { deriveAddressFromXpub } = require("./wallet"); const { TOKEN_BY_ADDRESS } = require("./tokenList"); @@ -17,17 +18,38 @@ const { isSpoofedSymbol } = require("./symbolSpoof"); // Use a static network to skip auto-detection (which can fail and cause // "could not coalesce error" on some RPC endpoints like Cloudflare). -// Accepts an optional networkName ("mainnet" or "sepolia") for the static -// network hint so ethers picks the right chain parameters. When omitted, -// reads the currently selected network from extension state. -function getProvider(rpcUrl, networkName) { - // Lazy require to avoid circular dependency issues at module scope. - const { currentNetwork } = require("./state"); - const name = networkName || currentNetwork().id; - const net = Network.from(name); +// +// `networkId` is REQUIRED, and is one of the ids in networks.js. It used to be +// optional, falling back to currentNetwork() — the module-level `state` +// singleton, which the MV3 service worker never populates. The endpoint then +// came out right and the static hint came out mainnet, so ethers fixed +// `chainId` at 0x1 and every non-mainnet dApp send was prepared for the wrong +// chain and then refused by the wallet's own verifier +// (https://git.eeqj.de/sneak/AutistMask/issues/320). Requiring it is what +// stops that from coming back: a caller that has no network to name has no +// business constructing a provider, and there is no longer a default for it +// to get silently wrong. +// +// Validated against NETWORKS rather than passed straight to Network.from(): +// ethers knows chains this wallet does not, so an id that is not one of ours +// is a caller bug and must not resolve to a working provider for some other +// chain. +function getProvider(rpcUrl, networkId) { + const net = Network.from(requireNetworkId(networkId).id); return new JsonRpcProvider(rpcUrl, net, { staticNetwork: net }); } +function requireNetworkId(networkId) { + const net = NETWORKS[networkId]; + if (!net) { + throw new Error( + "getProvider requires the id of a supported network; got " + + JSON.stringify(networkId), + ); + } + return net; +} + function formatBalance(wei) { const eth = formatEther(wei); const parts = eth.split("."); @@ -118,9 +140,15 @@ async function fetchTokenBalances(address, blockscoutUrl, trackedTokens) { } // Fetch ETH balances, ENS names, and ERC-20 token balances for all addresses. -async function refreshBalances(wallets, rpcUrl, blockscoutUrl, trackedTokens) { +async function refreshBalances( + wallets, + rpcUrl, + blockscoutUrl, + trackedTokens, + networkId, +) { log.debugf("refreshBalances start, rpc:", rpcUrl); - const provider = getProvider(rpcUrl); + const provider = getProvider(rpcUrl, networkId); const updates = []; for (const wallet of wallets) { @@ -193,9 +221,9 @@ async function refreshBalances(wallets, rpcUrl, blockscoutUrl, trackedTokens) { // Look up token metadata from its contract. // Calls symbol() and decimals() to verify it implements ERC-20. -async function lookupTokenInfo(contractAddress, rpcUrl) { +async function lookupTokenInfo(contractAddress, rpcUrl, networkId) { log.debugf("lookupTokenInfo", contractAddress, "rpc:", rpcUrl); - const provider = getProvider(rpcUrl); + const provider = getProvider(rpcUrl, networkId); const contract = new Contract(contractAddress, ERC20_ABI, provider); let name, symbol, decimals; @@ -235,9 +263,9 @@ async function lookupTokenInfo(contractAddress, rpcUrl) { // Checks gapLimit addresses in parallel per batch. Stops when an entire // batch has no used addresses (i.e. gapLimit consecutive empty addresses). // Returns { addresses: [{ address, index }], nextIndex }. -async function scanForAddresses(xpub, rpcUrl, gapLimit = 5) { +async function scanForAddresses(xpub, rpcUrl, networkId, gapLimit = 5) { log.debugf("scanForAddresses start, gapLimit:", gapLimit); - const provider = getProvider(rpcUrl); + const provider = getProvider(rpcUrl, networkId); const used = []; let checked = 0; let checkUpTo = gapLimit; diff --git a/src/shared/chainSwitch.js b/src/shared/chainSwitch.js index feeda38..6226e65 100644 --- a/src/shared/chainSwitch.js +++ b/src/shared/chainSwitch.js @@ -1,14 +1,23 @@ -// Consolidated chain-switch handler. +// Consolidated chain-switch handler for the popup. // // Every state change required when the active network changes is -// performed here so that callers (settings UI, background -// wallet_switchEthereumChain, future chain additions) all go +// performed here so that callers (settings UI, future chain additions) all go // through a single code path. // // Adding a new chain (e.g. ETC) requires only a new entry in // networks.js — no per-caller wiring is needed. +// +// The background does NOT come through here: this function mutates the +// module-level `state` singleton, which the MV3 service worker never +// populates, and a background switch performed on it wrote DEFAULT_STATE over +// the user's whole profile +// (https://git.eeqj.de/sneak/AutistMask/issues/316). The field mutations +// themselves live in chainSwitchFields.js, which takes the record to mutate as +// an argument; src/background/state.js applies them inside a read-modify-write +// against storage, and the singleton is not reachable from the background +// bundle at all (enforced by the ESLint rule in eslint.config.js). -const { networkById } = require("./networks"); +const { applyChainSwitchFields } = require("./chainSwitchFields"); const { clearPrices } = require("./prices"); // Switch the active chain and reset all chain-specific cached state. @@ -16,56 +25,14 @@ const { clearPrices } = require("./prices"); async function onChainSwitch(newNetworkId) { const { state, saveState } = require("./state"); - const net = networkById(newNetworkId); - - // --- core identity --- - // Endpoints are remembered per network rather than reset to the - // defaults, because a user who points the wallet at their own node has - // no way to get that URL back once it is gone: overwriting it moved - // every address and every transaction onto a third-party endpoint - // silently and permanently. - // - // state.rpcUrl / state.blockscoutUrl stay the live endpoints of the - // active network, so nothing that reads them changes. The invariant is - // that for the ACTIVE network those two fields are authoritative and - // the map entry may be stale (Settings writes the fields directly); - // for every other network the map is authoritative. Snapshotting the - // outgoing network here, before the switch, is what reconciles them. - state.networkEndpoints[state.networkId] = { - rpcUrl: state.rpcUrl, - blockscoutUrl: state.blockscoutUrl, - }; - const remembered = state.networkEndpoints[net.id] || {}; - state.networkId = net.id; - state.rpcUrl = remembered.rpcUrl || net.defaultRpcUrl; - state.blockscoutUrl = remembered.blockscoutUrl || net.defaultBlockscoutUrl; + const net = applyChainSwitchFields(state, newNetworkId); // --- price cache --- // Prices are chain-specific (testnet tokens are worthless, - // ETC has different pricing, etc.). + // ETC has different pricing, etc.). In-memory and per bundle, so this is + // the popup's own cache — the only context that ever fills it. clearPrices(); - // --- balance / refresh state --- - // Reset last-refresh timestamp so the next polling cycle - // triggers an immediate balance refresh on the new chain. - state.lastBalanceRefresh = 0; - - // Clear per-address balances and token balances so stale data - // from the previous chain is never displayed while the first - // refresh on the new chain is in flight. - for (const wallet of state.wallets) { - for (const addr of wallet.addresses) { - addr.balance = "0"; - addr.tokenBalances = []; - } - } - - // --- chain-specific caches --- - // Token holder counts and fraud contract lists are - // chain-specific and must not carry over. - state.tokenHolderCache = {}; - state.fraudContracts = []; - await saveState(); return net; diff --git a/src/shared/chainSwitchFields.js b/src/shared/chainSwitchFields.js new file mode 100644 index 0000000..e4d46d3 --- /dev/null +++ b/src/shared/chainSwitchFields.js @@ -0,0 +1,69 @@ +// The field mutations a chain switch performs, applied to a state record +// handed in rather than to the module-level `state` singleton. +// +// Split out of chainSwitch.js so the background can perform a chain switch +// without the singleton being reachable from its bundle at all. The popup +// still goes through onChainSwitch() (chainSwitch.js), which applies this to +// the singleton and saves; the background applies it to the detached record of +// its own read-modify-write (src/background/state.js). +// +// Everything here is synchronous and touches nothing but the object it is +// given: no storage, no caches, no imports beyond the network table. That is +// what makes it usable on a record that has been read fresh from storage +// microseconds earlier and is about to be written back. + +const { networkById } = require("./networks"); + +// Switch `s` to `newNetworkId` and reset every piece of chain-specific state +// it carries. Returns the network configuration object for the new chain. +function applyChainSwitchFields(s, newNetworkId) { + const net = networkById(newNetworkId); + + // --- core identity --- + // Endpoints are remembered per network rather than reset to the + // defaults, because a user who points the wallet at their own node has + // no way to get that URL back once it is gone: overwriting it moved + // every address and every transaction onto a third-party endpoint + // silently and permanently. + // + // s.rpcUrl / s.blockscoutUrl stay the live endpoints of the active + // network, so nothing that reads them changes. The invariant is that for + // the ACTIVE network those two fields are authoritative and the map entry + // may be stale (Settings writes the fields directly); for every other + // network the map is authoritative. Snapshotting the outgoing network + // here, before the switch, is what reconciles them. + if (!s.networkEndpoints) s.networkEndpoints = {}; + s.networkEndpoints[s.networkId] = { + rpcUrl: s.rpcUrl, + blockscoutUrl: s.blockscoutUrl, + }; + const remembered = s.networkEndpoints[net.id] || {}; + s.networkId = net.id; + s.rpcUrl = remembered.rpcUrl || net.defaultRpcUrl; + s.blockscoutUrl = remembered.blockscoutUrl || net.defaultBlockscoutUrl; + + // --- balance / refresh state --- + // Reset last-refresh timestamp so the next polling cycle + // triggers an immediate balance refresh on the new chain. + s.lastBalanceRefresh = 0; + + // Clear per-address balances and token balances so stale data + // from the previous chain is never displayed while the first + // refresh on the new chain is in flight. + for (const wallet of s.wallets || []) { + for (const addr of wallet.addresses || []) { + addr.balance = "0"; + addr.tokenBalances = []; + } + } + + // --- chain-specific caches --- + // Token holder counts and fraud contract lists are + // chain-specific and must not carry over. + s.tokenHolderCache = {}; + s.fraudContracts = []; + + return net; +} + +module.exports = { applyChainSwitchFields }; diff --git a/src/shared/ens.js b/src/shared/ens.js index ff88f24..4b95660 100644 --- a/src/shared/ens.js +++ b/src/shared/ens.js @@ -32,11 +32,11 @@ function setCache(address, name) { localStorage.setItem(key, JSON.stringify({ name, ts: Date.now() })); } -async function resolveEnsName(address, rpcUrl) { +async function resolveEnsName(address, rpcUrl, networkId) { const cached = getCached(address); if (cached !== undefined) return cached; - const provider = getProvider(rpcUrl); + const provider = getProvider(rpcUrl, networkId); try { const name = (await provider.lookupAddress(address)) || null; setCache(address, name); @@ -48,11 +48,11 @@ async function resolveEnsName(address, rpcUrl) { } } -async function resolveEnsNames(addresses, rpcUrl) { +async function resolveEnsNames(addresses, rpcUrl, networkId) { const results = new Map(); await Promise.all( addresses.map(async (addr) => { - results.set(addr, await resolveEnsName(addr, rpcUrl)); + results.set(addr, await resolveEnsName(addr, rpcUrl, networkId)); }), ); return results; diff --git a/src/shared/persistedState.js b/src/shared/persistedState.js new file mode 100644 index 0000000..36a6d27 --- /dev/null +++ b/src/shared/persistedState.js @@ -0,0 +1,204 @@ +// The shape of the persisted profile, and the normalization every read of it +// goes through. No singleton, no storage access, no browser API: just the +// record definition and pure functions over it. +// +// Split out of state.js so that a context which must never touch the +// module-level `state` singleton can still speak the same record format. +// src/background/state.js is that context — the MV3 service worker never +// populates the singleton, and every defect in +// https://git.eeqj.de/sneak/AutistMask/issues/324 came from background code +// reaching it anyway and being served DEFAULT_STATE. + +const { DEFAULT_RPC_URL, DEFAULT_BLOCKSCOUT_URL } = require("./constants"); +// Dependency-free constant module; safe to pull into a background bundle. +const { RESTORABLE_VIEWS } = require("../popup/restorableViews"); + +const DEFAULT_STATE = { + hasWallet: false, + wallets: [], + trackedTokens: [], + networkId: "mainnet", + rpcUrl: DEFAULT_RPC_URL, + blockscoutUrl: DEFAULT_BLOCKSCOUT_URL, + // Endpoints remembered per network: { [networkId]: { rpcUrl, + // blockscoutUrl } }. rpcUrl/blockscoutUrl above are the live endpoints + // of the active network; this is what the others are restored from + // when the active network changes. See applyChainSwitchFields(). + networkEndpoints: {}, + lastBalanceRefresh: 0, + activeAddress: null, + allowedSites: {}, + deniedSites: {}, + rememberSiteChoice: true, + showZeroBalanceTokens: true, + hideSpoofedSymbols: true, + hideLowHolderTokens: true, + hideFraudContracts: true, + hideDustTransactions: true, + dustThresholdGwei: 100000, + utcTimestamps: false, + fraudContracts: [], + tokenHolderCache: {}, + theme: "system", + debugMode: false, +}; + +// Every field written to and read from the single "autistmask" storage key. +// hasWallet is deliberately excluded from the diffing/merge logic in +// state.js — like loadState() does, it is always derived from `wallets`, +// never carried as an independent value. +const PERSISTED_FIELDS = Object.keys(DEFAULT_STATE) + .filter((key) => key !== "hasWallet") + .concat([ + "currentView", + "selectedWallet", + "selectedAddress", + "selectedToken", + "viewData", + "viewStack", + ]); + +// Keep only the leading run of stored views the popup is willing to render. +// +// restoreView() refuses to reopen ONTO a non-restorable view, but the stack +// behind it used to be restored verbatim, so Back could walk onto a screen +// whose content is deliberately never re-rendered — and "show-phrase" has no +// Back control to leave by. Truncating at the first such entry instead of +// splicing it out keeps the result a prefix of the stored stack, so every +// surviving entry's Back target is exactly the one it had; splicing would +// silently re-point the entry above the hole at a different screen. +// +// Filtering happens here on load rather than in saveState(): the live +// in-session stack is legitimate (the screen really is rendered while the +// popup is open), and only a load-side filter also repairs the stacks +// already in storage, including ones written before a view left the set. +function restorableStack(stored, currentView) { + // A stored stack that is missing or not an array keeps nothing, but it + // still goes through the never-empty rule below rather than returning + // early: otherwise a corrupt stack would depend on exactly the goBack() + // fallback that the explicit ["main"] exists in order not to depend on. + const source = Array.isArray(stored) ? stored : []; + const cut = source.findIndex((view) => !RESTORABLE_VIEWS.has(view)); + const kept = cut === -1 ? source.slice() : source.slice(0, cut); + // A view restored below the root still needs somewhere for Back to go. + if ( + kept.length === 0 && + currentView !== "main" && + RESTORABLE_VIEWS.has(currentView) + ) { + return ["main"]; + } + return kept; +} + +// Turn a raw stored (or missing) record into the full, defaulted shape +// loadState() used to assign directly onto `state`. A pure function so that +// saveState() can apply it too: the fields THIS page did not change still have +// to come from storage in their loaded-and-normalized form, not as the raw +// bytes another page (or an old release) left there — otherwise a legacy shape +// a load has always self-healed in memory (a missing networkEndpoints map, an +// out-of-range flag) is dropped right back into storage unfixed every time the +// page that DID normalize it saves something unrelated, because that field's +// value never "changed" for that page to notice. +// +// The result never shares structure with `saved`, so a caller may mutate it +// freely: it is the detached record every per-call read in the background is +// built on. +function normalizePersisted(saved) { + saved = saved || {}; + const out = {}; + out.wallets = structuredClone(saved.wallets || []); + // Derived, never trusted verbatim off storage — see loadState(). + out.hasWallet = out.wallets.length > 0; + out.trackedTokens = structuredClone(saved.trackedTokens || []); + out.networkId = saved.networkId || DEFAULT_STATE.networkId; + out.rpcUrl = saved.rpcUrl || DEFAULT_STATE.rpcUrl; + out.blockscoutUrl = saved.blockscoutUrl || DEFAULT_STATE.blockscoutUrl; + // An actual object is required, not merely a truthy non-array: the code + // below and applyChainSwitchFields() index and ASSIGN INTO this value, and + // assigning a property to a string or a number is a silent no-op in + // sloppy mode. Copied rather than referenced, nested pairs included, so + // normalizing never mutates the object a caller handed in. + const rawEndpoints = + typeof saved.networkEndpoints === "object" && + saved.networkEndpoints !== null && + !Array.isArray(saved.networkEndpoints) + ? saved.networkEndpoints + : {}; + out.networkEndpoints = {}; + for (const netId of Object.keys(rawEndpoints)) { + out.networkEndpoints[netId] = { ...rawEndpoints[netId] }; + } + // A profile written before this map existed carries exactly one pair of + // endpoints, belonging to whatever network it was last on. Adopt it as + // that network's remembered pair, so a custom endpoint set on the old + // build is not lost by the first switch away and back. + if (!out.networkEndpoints[out.networkId]) { + out.networkEndpoints[out.networkId] = { + rpcUrl: out.rpcUrl, + blockscoutUrl: out.blockscoutUrl, + }; + } + out.lastBalanceRefresh = saved.lastBalanceRefresh || 0; + out.activeAddress = saved.activeAddress || null; + out.allowedSites = + saved.allowedSites && !Array.isArray(saved.allowedSites) + ? structuredClone(saved.allowedSites) + : {}; + out.deniedSites = + saved.deniedSites && !Array.isArray(saved.deniedSites) + ? structuredClone(saved.deniedSites) + : {}; + out.rememberSiteChoice = + saved.rememberSiteChoice !== undefined + ? saved.rememberSiteChoice + : true; + out.showZeroBalanceTokens = + saved.showZeroBalanceTokens !== undefined + ? saved.showZeroBalanceTokens + : true; + // A profile written before this setting existed has no key for it. It + // is a safety filter, so absent must load as on, not as undefined. + out.hideSpoofedSymbols = + saved.hideSpoofedSymbols !== undefined + ? saved.hideSpoofedSymbols + : true; + out.hideLowHolderTokens = + saved.hideLowHolderTokens !== undefined + ? saved.hideLowHolderTokens + : true; + out.hideFraudContracts = + saved.hideFraudContracts !== undefined + ? saved.hideFraudContracts + : true; + out.hideDustTransactions = + saved.hideDustTransactions !== undefined + ? saved.hideDustTransactions + : true; + out.dustThresholdGwei = + saved.dustThresholdGwei !== undefined + ? saved.dustThresholdGwei + : 100000; + out.utcTimestamps = + saved.utcTimestamps !== undefined ? saved.utcTimestamps : false; + out.fraudContracts = structuredClone(saved.fraudContracts || []); + out.tokenHolderCache = structuredClone(saved.tokenHolderCache || {}); + out.theme = saved.theme || "system"; + out.debugMode = saved.debugMode !== undefined ? saved.debugMode : false; + out.currentView = saved.currentView || null; + out.selectedWallet = + saved.selectedWallet !== undefined ? saved.selectedWallet : null; + out.selectedAddress = + saved.selectedAddress !== undefined ? saved.selectedAddress : null; + out.selectedToken = saved.selectedToken || null; + out.viewData = structuredClone(saved.viewData || {}); + out.viewStack = restorableStack(saved.viewStack, out.currentView); + return out; +} + +module.exports = { + DEFAULT_STATE, + PERSISTED_FIELDS, + normalizePersisted, + restorableStack, +}; diff --git a/src/shared/state.js b/src/shared/state.js index 6cc5384..e33a5e8 100644 --- a/src/shared/state.js +++ b/src/shared/state.js @@ -1,46 +1,36 @@ // State management and extension storage persistence. +// +// The `state` export is a module-level singleton: ONE in-memory copy of the +// profile per bundle, loaded once by loadState() and mutated in place from +// then on. That is the popup's model — one page, one load at boot, one +// lifetime. +// +// It is NOT the background's model, and the background must not reach it. The +// MV3 service worker is torn down when idle and revived by the next message, +// nothing loads state at module scope, and an unpopulated read used to hand +// back DEFAULT_STATE with no complaint — five defects came out of that one +// fact (https://git.eeqj.de/sneak/AutistMask/issues/324). Two things close it: +// this module is unreachable from the background bundle (enforced by the +// ESLint rule in eslint.config.js, and by the background having its own +// per-call storage layer in src/background/state.js), and reading a persisted +// field of the singleton before a load now THROWS instead of quietly serving +// a default. -const { DEFAULT_RPC_URL, DEFAULT_BLOCKSCOUT_URL } = require("./constants"); const { networkById } = require("./networks"); -// Dependency-free constant module; safe to pull into a background bundle. -const { RESTORABLE_VIEWS } = require("../popup/restorableViews"); +const { + DEFAULT_STATE, + PERSISTED_FIELDS, + normalizePersisted, +} = require("./persistedState"); const { storageGet, storageSet } = require("./browserApi"); const { log } = require("./log"); -const DEFAULT_STATE = { - hasWallet: false, - wallets: [], - trackedTokens: [], - networkId: "mainnet", - rpcUrl: DEFAULT_RPC_URL, - blockscoutUrl: DEFAULT_BLOCKSCOUT_URL, - // Endpoints remembered per network: { [networkId]: { rpcUrl, - // blockscoutUrl } }. rpcUrl/blockscoutUrl above are the live endpoints - // of the active network; this is what the others are restored from - // when the active network changes. See onChainSwitch(). - networkEndpoints: {}, - lastBalanceRefresh: 0, - activeAddress: null, - allowedSites: {}, - deniedSites: {}, - rememberSiteChoice: true, - showZeroBalanceTokens: true, - hideSpoofedSymbols: true, - hideLowHolderTokens: true, - hideFraudContracts: true, - hideDustTransactions: true, - dustThresholdGwei: 100000, - utcTimestamps: false, - fraudContracts: [], - tokenHolderCache: {}, - theme: "system", - debugMode: false, -}; - -const state = { +// The live record the proxy below guards. Everything inside this module reads +// and writes THIS object, never the proxy: the guard is for callers. +const rawState = { ...DEFAULT_STATE, - // Its own object, not the one DEFAULT_STATE holds: onChainSwitch() + // Its own object, not the one DEFAULT_STATE holds: applyChainSwitchFields() // mutates this map in place, and a spread copies the reference. networkEndpoints: {}, currentView: null, @@ -51,161 +41,72 @@ const state = { viewStack: [], }; -// Keep only the leading run of stored views the popup is willing to render. +// False until loadState() has completed in this bundle. Until then, a +// persisted field that has not been assigned in this context cannot be READ: +// see StateNotLoadedError. +let loaded = false; + +// True once this context has assigned anything into the singleton. // -// restoreView() refuses to reopen ONTO a non-restorable view, but the stack -// behind it used to be restored verbatim, so Back could walk onto a screen -// whose content is deliberately never re-rendered — and "show-phrase" has no -// Back control to leave by. Truncating at the first such entry instead of -// splicing it out keeps the result a prefix of the stored stack, so every -// surviving entry's Back target is exactly the one it had; splicing would -// silently re-point the entry above the hole at a different screen. +// What the guard is for is a context that READS a profile nobody put there — +// every one of the five defects was a pure read of an untouched singleton, +// answered out of DEFAULT_STATE. A context that has written into it is +// managing it deliberately (the popup does, via loadState() at boot and by +// hand thereafter), and reading back what you yourself put there is not the +// mistake being caught. // -// Filtering happens here on load rather than in saveState(): the live -// in-session stack is legitimate (the screen really is rendered while the -// popup is open), and only a load-side filter also repairs the stacks -// already in storage, including ones written before a view left the set. -function restorableStack(stored, currentView) { - // A stored stack that is missing or not an array keeps nothing, but it - // still goes through the never-empty rule below rather than returning - // early: otherwise a corrupt stack would depend on exactly the goBack() - // fallback that the explicit ["main"] exists in order not to depend on. - const source = Array.isArray(stored) ? stored : []; - const cut = source.findIndex((view) => !RESTORABLE_VIEWS.has(view)); - const kept = cut === -1 ? source.slice() : source.slice(0, cut); - // A view restored below the root still needs somewhere for Back to go. - if ( - kept.length === 0 && - currentView !== "main" && - RESTORABLE_VIEWS.has(currentView) - ) { - return ["main"]; +// The cost of that is honest and worth naming: a context that writes one field +// and then reads a different, untouched one is still served that field's +// default. Nothing closes that here — what closes it for the background is +// that the background cannot reach this module at all (eslint.config.js). +let adopted = false; + +// Every field whose pre-load value would be a plausible-looking default rather +// than the user's data. The view scratch fields are guarded too: currentView +// and viewStack are persisted, and a save that carried their pre-load values +// would overwrite a real stored stack with an empty one. +const GUARDED_FIELDS = new Set(PERSISTED_FIELDS.concat(["hasWallet"])); + +class StateNotLoadedError extends Error { + constructor(field) { + super( + "state." + + field + + " was read before loadState(); this context has no profile" + + " loaded and must not be served DEFAULT_STATE", + ); + this.name = "StateNotLoadedError"; } - return kept; } +// Loud, not defaulted. The whole defect class this guard closes looks exactly +// like working code at the call site: the read succeeds, the value is +// well-formed, and it describes a wallet that is not the user's. +const state = new Proxy(rawState, { + get(target, prop, receiver) { + if ( + !loaded && + !adopted && + typeof prop === "string" && + GUARDED_FIELDS.has(prop) + ) { + throw new StateNotLoadedError(prop); + } + return Reflect.get(target, prop, receiver); + }, + set(target, prop, value, receiver) { + if (typeof prop === "string" && GUARDED_FIELDS.has(prop)) { + adopted = true; + } + return Reflect.set(target, prop, value, receiver); + }, +}); + // Return the network configuration for the currently selected network. function currentNetwork() { return networkById(state.networkId); } -// Every field written to and read from the single "autistmask" storage key. -// hasWallet is deliberately excluded from the diffing/merge logic below — -// like loadState() does, it is always derived from `wallets`, never carried -// as an independent value. -const PERSISTED_FIELDS = Object.keys(DEFAULT_STATE) - .filter((key) => key !== "hasWallet") - .concat([ - "currentView", - "selectedWallet", - "selectedAddress", - "selectedToken", - "viewData", - "viewStack", - ]); - -// Turn a raw stored (or missing) record into the full, defaulted shape -// loadState() used to assign directly onto `state`. Pulled out as a pure -// function so saveState() can apply it too: the fields THIS page did not -// change still have to come from storage in their loaded-and-normalized -// form, not as the raw bytes another page (or an old release) left there — -// otherwise a legacy shape a load has always self-healed in memory (a -// missing networkEndpoints map, an out-of-range flag) is dropped right back -// into storage unfixed every time the page that DID normalize it saves -// something unrelated, because that field's value never "changed" for that -// page to notice. -function normalizePersisted(saved) { - saved = saved || {}; - const out = {}; - out.wallets = saved.wallets || []; - // Derived, never trusted verbatim off storage — see loadState(). - out.hasWallet = out.wallets.length > 0; - out.trackedTokens = saved.trackedTokens || []; - out.networkId = saved.networkId || DEFAULT_STATE.networkId; - out.rpcUrl = saved.rpcUrl || DEFAULT_STATE.rpcUrl; - out.blockscoutUrl = saved.blockscoutUrl || DEFAULT_STATE.blockscoutUrl; - // An actual object is required, not merely a truthy non-array: the code - // below and onChainSwitch() index and ASSIGN INTO this value, and - // assigning a property to a string or a number is a silent no-op in - // sloppy mode. Copied rather than referenced, nested pairs included, so - // normalizing never mutates the object a caller handed in. - const rawEndpoints = - typeof saved.networkEndpoints === "object" && - saved.networkEndpoints !== null && - !Array.isArray(saved.networkEndpoints) - ? saved.networkEndpoints - : {}; - out.networkEndpoints = {}; - for (const netId of Object.keys(rawEndpoints)) { - out.networkEndpoints[netId] = { ...rawEndpoints[netId] }; - } - // A profile written before this map existed carries exactly one pair of - // endpoints, belonging to whatever network it was last on. Adopt it as - // that network's remembered pair, so a custom endpoint set on the old - // build is not lost by the first switch away and back. - if (!out.networkEndpoints[out.networkId]) { - out.networkEndpoints[out.networkId] = { - rpcUrl: out.rpcUrl, - blockscoutUrl: out.blockscoutUrl, - }; - } - out.lastBalanceRefresh = saved.lastBalanceRefresh || 0; - out.activeAddress = saved.activeAddress || null; - out.allowedSites = - saved.allowedSites && !Array.isArray(saved.allowedSites) - ? saved.allowedSites - : {}; - out.deniedSites = - saved.deniedSites && !Array.isArray(saved.deniedSites) - ? saved.deniedSites - : {}; - out.rememberSiteChoice = - saved.rememberSiteChoice !== undefined - ? saved.rememberSiteChoice - : true; - out.showZeroBalanceTokens = - saved.showZeroBalanceTokens !== undefined - ? saved.showZeroBalanceTokens - : true; - // A profile written before this setting existed has no key for it. It - // is a safety filter, so absent must load as on, not as undefined. - out.hideSpoofedSymbols = - saved.hideSpoofedSymbols !== undefined - ? saved.hideSpoofedSymbols - : true; - out.hideLowHolderTokens = - saved.hideLowHolderTokens !== undefined - ? saved.hideLowHolderTokens - : true; - out.hideFraudContracts = - saved.hideFraudContracts !== undefined - ? saved.hideFraudContracts - : true; - out.hideDustTransactions = - saved.hideDustTransactions !== undefined - ? saved.hideDustTransactions - : true; - out.dustThresholdGwei = - saved.dustThresholdGwei !== undefined - ? saved.dustThresholdGwei - : 100000; - out.utcTimestamps = - saved.utcTimestamps !== undefined ? saved.utcTimestamps : false; - out.fraudContracts = saved.fraudContracts || []; - out.tokenHolderCache = saved.tokenHolderCache || {}; - out.theme = saved.theme || "system"; - out.debugMode = saved.debugMode !== undefined ? saved.debugMode : false; - out.currentView = saved.currentView || null; - out.selectedWallet = - saved.selectedWallet !== undefined ? saved.selectedWallet : null; - out.selectedAddress = - saved.selectedAddress !== undefined ? saved.selectedAddress : null; - out.selectedToken = saved.selectedToken || null; - out.viewData = saved.viewData || {}; - out.viewStack = restorableStack(saved.viewStack, out.currentView); - return out; -} - // The persisted fields as they stood at the end of this page's last // loadState() or saveState(). saveState() diffs the live state against this // to find only the fields THIS page actually changed. @@ -217,7 +118,7 @@ let baseline = null; function snapshotPersisted() { const out = {}; - for (const key of PERSISTED_FIELDS) out[key] = state[key]; + for (const key of PERSISTED_FIELDS) out[key] = rawState[key]; return out; } @@ -374,11 +275,10 @@ function mergeWallet(base, ours, theirs) { } // Merge one address's leaf fields (balance, ensName, tokenBalances, ...). -// tokenBalances is itself an array, but only backgroundRefresh() ever -// writes it and always wholesale (refreshBalances() in -// src/shared/balances.js), so there is no membership to reconcile within -// it — it is a leaf like balance or ensName, not a list with its own -// identity. +// tokenBalances is itself an array, but only a balance refresh ever writes +// it and always wholesale (refreshBalances() in src/shared/balances.js), so +// there is no membership to reconcile within it — it is a leaf like balance +// or ensName, not a list with its own identity. function mergeAddress(base, ours, theirs) { if (!base) return ours; const merged = { ...theirs }; @@ -425,12 +325,12 @@ function mergeMapByKey(base, ours, theirs, mergeLeaf) { } // allowedSites/deniedSites: { [address]: [hostname, ...] }. The hostname -// list is itself membership, not a leaf — src/background/index.js pushes a -// newly approved/denied hostname onto it in place, and the Settings "revoke" -// button (src/popup/views/settings.js) filters a hostname out of it in -// place, from a different page. Merge it the same way wallets are merged: -// identity is the hostname itself, so a merged pair is always equal and -// mergeItem is a no-op pick. +// list is itself membership, not a leaf — the background appends a newly +// approved/denied hostname to it, and the Settings "revoke" button +// (src/popup/views/settings.js) filters a hostname out of it in place, from a +// different page. Merge it the same way wallets are merged: identity is the +// hostname itself, so a merged pair is always equal and mergeItem is a no-op +// pick. function mergeHostnameList(base, ours, theirs) { return mergeListByIdentity( base, @@ -445,14 +345,15 @@ function mergeSiteMap(base, ours, theirs) { return mergeMapByKey(base, ours, theirs, mergeHostnameList); } -// networkEndpoints: { [networkId]: {rpcUrl, blockscoutUrl} }. onChainSwitch() -// (src/shared/chainSwitch.js) writes state.networkEndpoints[networkId] in -// place before saving. No code path ever removes a key from this map, so the -// membership collision that matters for allowedSites/wallets (an add on one -// page racing a delete on another) can't happen here — but two pages -// switching to two different networks concurrently still race a whole-field -// diff the same way, so it gets the same per-key merge for the leaf edit -// case (e.g. Settings saving a custom RPC URL for the active network). +// networkEndpoints: { [networkId]: {rpcUrl, blockscoutUrl} }. +// applyChainSwitchFields() (src/shared/chainSwitchFields.js) writes +// networkEndpoints[networkId] in place before saving. No code path ever +// removes a key from this map, so the membership collision that matters for +// allowedSites/wallets (an add on one page racing a delete on another) can't +// happen here — but two pages switching to two different networks +// concurrently still race a whole-field diff the same way, so it gets the same +// per-key merge for the leaf edit case (e.g. Settings saving a custom RPC URL +// for the active network). function mergeEndpointEntry(base, ours, theirs) { if (!base) return ours; const merged = { ...theirs }; @@ -468,12 +369,12 @@ function mergeNetworkEndpoints(base, ours, theirs) { // Read-modify-write, merged per field, rather than one full-blob write. // -// Every extension page (the toolbar popup, a dApp approval window, the -// background's backgroundRefresh()) holds its own in-memory `state`, loaded -// once, and showView() saves on every navigation. A full-blob write here -// clobbers whatever a second page had written since — including, in the -// worst case, an entire wallet and its encrypted secret with no attacker -// and no unusual input (see the issue this fixes). +// Every extension page (the toolbar popup, a dApp approval window) holds its +// own in-memory `state`, loaded once, and showView() saves on every +// navigation. A full-blob write here clobbers whatever a second page had +// written since — including, in the worst case, an entire wallet and its +// encrypted secret with no attacker and no unusual input (see the issue this +// fixes). // // Only the fields this page actually changed — those that differ from // `baseline`, captured at the last loadState()/saveState() on this page — @@ -482,28 +383,27 @@ function mergeNetworkEndpoints(base, ours, theirs) { // // `wallets` is merged structurally (mergeListByIdentity(), by wallet // identity and then by address identity within each wallet), not as one -// whole field: backgroundRefresh() mutates wallets IN PLACE (addr.balance / +// whole field: a balance refresh mutates wallets IN PLACE (addr.balance / // ensName / tokenBalances, via refreshBalances()), so a whole-field diff // would mark all of `wallets` "changed" the moment any balance moved and -// write back background's own copy — loaded before its multi-second network +// write back that page's own copy — loaded before its multi-second network // round trip — clobbering a wallet another page added, or resurrecting one -// another page deleted, in that window. Merging by identity lets -// background's leaf changes and another page's membership changes -// (add/delete a wallet or an address) apply independently instead of -// colliding as the same field. +// another page deleted, in that window. Merging by identity lets the leaf +// changes and another page's membership changes (add/delete a wallet or an +// address) apply independently instead of colliding as the same field. // // `allowedSites` and `deniedSites` get the same treatment (mergeSiteMap(), // by address key and then by hostname within each address's list), for the -// identical reason: src/background/index.js pushes a newly -// approved/denied hostname onto them in place, and the Settings "revoke" -// button (src/popup/views/settings.js) filters one out in place, from a -// different page. A whole-field diff here doesn't just lose data, it is a -// security defect — a stale page's save can resurrect a just-revoked site -// permission, or silently wipe a permission just granted elsewhere. +// identical reason: the background appends a newly approved/denied hostname +// to them, and the Settings "revoke" button (src/popup/views/settings.js) +// filters one out in place, from a different page. A whole-field diff here +// doesn't just lose data, it is a security defect — a stale page's save can +// resurrect a just-revoked site permission, or silently wipe a permission just +// granted elsewhere. // // `networkEndpoints` gets the same treatment too (mergeNetworkEndpoints(), -// by network id), since onChainSwitch() writes into it in place; the value -// per key is a small leaf object with no membership of its own; see the +// by network id), since applyChainSwitchFields() writes into it in place; the +// value per key is a small leaf object with no membership of its own; see the // comment at mergeEndpointEntry() for why the collision this closes is // milder than the other two. // @@ -511,8 +411,8 @@ function mergeNetworkEndpoints(base, ours, theirs) { // `trackedTokens`/`fraudContracts`/`viewStack` are arrays of scalars with no // per-element identity to merge by; `tokenHolderCache` is a map shaped like // the ones above, but nothing in src/ ever writes an entry into it — it is -// only ever reset wholesale to `{}` (onChainSwitch()) — so there is no -// in-place mutation for a whole-field diff to collide with; `viewData` is +// only ever reset wholesale to `{}` (applyChainSwitchFields()) — so there is +// no in-place mutation for a whole-field diff to collide with; `viewData` is // this page's own UI scratch space, not data another page has any reason to // share membership of. // @@ -539,7 +439,7 @@ async function saveStateOnce() { const result = await storageGet("autistmask"); // Normalized, not raw: a field this page did not change still has to // come from storage in its loaded (self-healed) shape. See - // normalizePersisted() above. + // normalizePersisted() in persistedState.js. const fresh = normalizePersisted(result.autistmask); const merged = { ...fresh }; @@ -578,7 +478,7 @@ async function saveStateOnce() { // Derived from this page's own wallets, never adopted off the wire — // see loadState(). Everything else this page did not change is left // exactly as it stood; see the note above. - state.hasWallet = state.wallets.length > 0; + rawState.hasWallet = rawState.wallets.length > 0; baseline = structuredClone(snapshotPersisted()); } @@ -606,14 +506,21 @@ function saveState() { async function loadState() { const result = await storageGet("autistmask"); if (result.autistmask) { - Object.assign(state, normalizePersisted(result.autistmask)); + Object.assign(rawState, normalizePersisted(result.autistmask)); } - // The point of comparison every saveState() on this page diffs against, - // whether storage had a profile or was empty. See PERSISTED_FIELDS above - // saveState() for why a reference here would be wrong. + // Whether storage had a profile or was empty, this context has now read + // it, and the defaults standing in for an empty profile are the right + // answer rather than a stand-in for one nobody looked for. + loaded = true; + // The point of comparison every saveState() on this page diffs against. + // See PERSISTED_FIELDS in persistedState.js for why a reference here + // would be wrong. baseline = structuredClone(snapshotPersisted()); } +// Through the guarded proxy, not rawState: a caller asking which address is +// selected before anything was loaded gets the same loud failure it would get +// reading the fields itself. function currentAddress() { if (state.selectedWallet === null || state.selectedAddress === null) { return null; @@ -627,4 +534,5 @@ module.exports = { loadState, currentAddress, currentNetwork, + StateNotLoadedError, }; diff --git a/tests/alarms.test.js b/tests/alarms.test.js index 6f905ad..759e961 100644 --- a/tests/alarms.test.js +++ b/tests/alarms.test.js @@ -8,6 +8,8 @@ // A controllable clock plus a stubbed balance refresh, so a cadence test can // measure the interval between refreshes that actually happened rather than // asserting the interval someone intended. +const { makeStorageStub } = require("./support/storageStub"); + let mockNow = 0; const mockBalanceRefreshAt = []; @@ -247,32 +249,17 @@ describe("alarms module", () => { // Loads the background worker against stubbed browser APIs. The returned // store is the extension storage the worker sees, so a test can seed wallet // state and read back what the worker persisted. +// The stub clones in both directions, as the real chrome.storage.local does, +// and carries the latency simulation above on every operation. It used to +// alias, which for this file meant the worker's in-memory wallets and the +// "stored" ones were one object — see tests/support/storageStub.js. function loadBackground(initialStore = {}) { - const storageStore = initialStore; + const storage = makeStorageStub(initialStore, mockStorageTick); const alarmsStub = makeAlarmsStub(); const listeners = { onInstalled: [], onStartup: [] }; global.chrome = { alarms: alarmsStub, - storage: { - local: { - get: async (key) => { - mockStorageTick(); - return Object.prototype.hasOwnProperty.call( - storageStore, - key, - ) - ? { [key]: storageStore[key] } - : {}; - }, - set: async (items) => { - mockStorageTick(); - Object.assign(storageStore, items); - }, - remove: async (key) => { - delete storageStore[key]; - }, - }, - }, + storage, runtime: { onMessage: { addListener: jest.fn() }, onConnect: { addListener: jest.fn() }, @@ -301,7 +288,7 @@ function loadBackground(initialStore = {}) { })); jest.resetModules(); require("../src/background/index"); - return { alarmsStub, listeners, store: storageStore }; + return { alarmsStub, listeners, storage }; } // Flush the promise chains the startup path and the alarm handlers run on. @@ -476,12 +463,16 @@ describe("balance refresh steady-state cadence", () => { // The guard's actual job, and the reason it is shortened rather than // removed: while the popup is open it refreshes every 10 seconds and // stamps the same field, and the background job has nothing to add. - const store = seededStore(); - const { alarmsStub } = loadBackground(store); + const { alarmsStub, storage } = loadBackground(seededStore()); await settle(); mockNow += PERIOD_MS; - store.autistmask.lastBalanceRefresh = mockNow - 10 * 1000; + // As the open popup's own refresh would leave it: written to storage, + // not poked into an object the worker happens to share. + storage.write("autistmask", { + ...storage.read("autistmask"), + lastBalanceRefresh: mockNow - 10 * 1000, + }); alarmsStub.fire(BALANCE_REFRESH_ALARM); await settle(); diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index a2a8684..b67ba79 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -24,6 +24,7 @@ const { Network, Wallet } = require("ethers"); // before any jest.doMock() of the module, so the copy assertions below check // what the user is actually shown. const { describeSigningFailure } = require("../src/shared/approvalVerify"); +const { makeStorageStub } = require("./support/storageStub"); const SIGNER_KEY = "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d"; @@ -137,22 +138,22 @@ function loadBackground(options) { jest.resetModules(); const broadcastTransaction = jest.fn(); - const loadState = jest.fn(opts.loadState || (async () => {})); - // The network the wallet is on, which the tests switch under a pending - // approval. The node the transaction is populated against is on the same - // one, as it would be: switching networks switches the RPC endpoint too. - let chain = MAINNET; + // The node the transaction is populated against is on whatever chain the + // stored profile says, as it would be: switching networks switches the RPC + // endpoint too. The background takes the network from storage per call — + // it holds no in-memory copy — so this reads the record rather than a + // variable the test keeps alongside it. + const chainOf = (networkId) => + networkId === "sepolia" ? SEPOLIA : MAINNET; - jest.doMock("../src/shared/state", () => ({ - state: { rpcUrl: "https://rpc.invalid", wallets: [] }, - loadState, - saveState: jest.fn(async () => {}), - currentNetwork: () => ({ chainId: chain.hex }), - })); jest.doMock("../src/shared/balances", () => ({ - getProvider: () => - fakeProvider(broadcastTransaction, opts.provider, chain.num), + getProvider: (rpcUrl, networkId) => + fakeProvider( + broadcastTransaction, + opts.provider, + chainOf(networkId).num, + ), refreshBalances: jest.fn(async () => {}), })); jest.doMock("../src/shared/phishingDomains", () => ({ @@ -177,12 +178,31 @@ function loadBackground(options) { wallets: [ { name: "Wallet 1", type: "hd", addresses: [signer.address] }, ], + networkId: "mainnet", rpcUrl: "https://rpc.invalid", activeAddress: signer.address, allowedSites: { [signer.address]: [HOSTNAME] }, deniedSites: {}, }; + // The one wallet state there is. The background reads it per call and + // writes it read-modify-write; it holds no in-memory copy and cannot reach + // the shared singleton. Clones in both directions, as the real API does — + // the stub here used to hand back the live record and drop every write on + // the floor, so a test could neither see what was persisted nor be sure + // what it read had crossed the boundary + // (https://git.eeqj.de/sneak/AutistMask/issues/324). + const storage = makeStorageStub({ autistmask: persisted }); + + // A test that needs the state read itself to misbehave installs a hook — + // a stall, a throw — in place of the next reads. Armed after setup so + // that raising the approval is not what fails. + let storageGetHook = opts.storageGet || null; + const realGet = storage.local.get; + storage.local.get = jest.fn(async (key) => + storageGetHook ? storageGetHook(key) : realGet(key), + ); + let messageListener = null; let windowRemovedListener = null; let connectListener = null; @@ -194,15 +214,7 @@ function loadBackground(options) { const actionPopups = []; global.chrome = { - storage: { - local: { - get: jest.fn( - opts.storageGet || - (async () => ({ autistmask: persisted })), - ), - set: jest.fn(async () => {}), - }, - }, + storage, runtime: { getURL: (path) => EXT_URL + path, onMessage: { @@ -401,18 +413,32 @@ function loadBackground(options) { connectApproval, closeWindow, broadcastTransaction, - loadState, created, removed, + storage, // The user switching account in the toolbar popup, as the background // sees it: the persisted active address changes underneath a pending // approval. setActiveAddress: (address) => { - persisted.activeAddress = address; + storage.write("autistmask", { + ...storage.read("autistmask"), + activeAddress: address, + }); }, - // The user switching network in the toolbar popup. + // The user switching network in the toolbar popup. It moves the stored + // network and the endpoint together, as a real switch does. setNetwork: (network) => { - chain = network; + const networkId = network === SEPOLIA ? "sepolia" : "mainnet"; + storage.write("autistmask", { + ...storage.read("autistmask"), + networkId, + rpcUrl: "https://rpc-" + networkId + ".invalid", + }); + }, + // Make the next state reads misbehave — stall, throw — without + // touching the reads that raised the approval. Pass null to restore. + setStateReadHook: (hook) => { + storageGetHook = hook; }, fromPopup: { url: EXT_URL + "src/popup/index.html" }, }; @@ -677,15 +703,16 @@ describe("one transaction approval at a time", () => { // 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 () => { const stalled = deferred(); - const bg = loadBackground({ - loadState: async () => { - await stalled.promise; - throw new Error("The wallet data could not be read."); - }, - }); + const bg = loadBackground(); const first = bg.requestTx(); await settle(); + // Armed only now: the approval was raised against a working state + // read, and it is the ATTEMPT's read that hangs and then fails. + bg.setStateReadHook(async () => { + await stalled.promise; + throw new Error("The wallet data could not be read."); + }); bg.send( { type: "AUTISTMASK_TX_RESPONSE", @@ -709,6 +736,7 @@ describe("one transaction approval at a time", () => { error: { code: 4001, message: "User rejected the request." }, }); + bg.setStateReadHook(null); const second = bg.requestTx(); await settle(); expect(second.result()).toBeNull(); @@ -1226,19 +1254,18 @@ describe("what the approval is verified against", () => { // The interlock must not cost the retry the approval exists to allow. describe("the interlock releases a failed attempt", () => { test("a retryable failure before the broadcast leaves the approval usable", async () => { - let failNext = true; - const bg = loadBackground({ - loadState: async () => { - if (failNext) { - failNext = false; - throw new Error("storage unavailable"); - } - }, - }); + const bg = loadBackground(); const pending = bg.requestTx(); await settle(); const id = pending.id(); + // The attempt's state read fails once, then works: nothing was + // broadcast, so the approval must survive for the retry. + bg.setStateReadHook(() => { + bg.setStateReadHook(null); + throw new Error("storage unavailable"); + }); + const first = bg.send( { type: "AUTISTMASK_TX_RESPONSE", diff --git a/tests/backgroundStateIsolation.test.js b/tests/backgroundStateIsolation.test.js new file mode 100644 index 0000000..86ff232 --- /dev/null +++ b/tests/backgroundStateIsolation.test.js @@ -0,0 +1,398 @@ +// What one background handler's state can do to another's while both are in +// flight. +// +// The background used to read and write the module-level `state` singleton in +// src/shared/state.js — one object, shared by every handler in the worker, +// replaced wholesale by any loadState(). Two consequences, both covered here +// and both from https://git.eeqj.de/sneak/AutistMask/issues/324: +// +// - A transaction attempt captured the chain id at its loadState() and then +// read the ENDPOINT off the singleton several awaits later. A chain switch +// committed in that window moved the endpoint under an artifact already +// verified against the old chain, so it would have gone to the new chain's +// node — the very thing the verification exists to prevent. +// +// - backgroundRefresh() handed the singleton's wallets to refreshBalances(), +// which mutates address objects in place across a multi-second network +// round trip. Any concurrent handler that loaded state replaced those +// objects, so the refreshed balances landed on detached ones and the save +// that followed persisted the pre-refresh values — while still stamping +// lastBalanceRefresh, suppressing the redo. +// +// Both use the real persistence path over a cloning storage stub. Nothing here +// asserts the absence of a loadState() call; each asserts the OUTCOME, so it +// holds against any implementation that gets the outcome right. + +const { Wallet } = require("ethers"); +const { networkById } = require("../src/shared/networks"); +const { makeStorageStub } = require("./support/storageStub"); + +const SIGNER_KEY = + "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d"; +const signer = new Wallet(SIGNER_KEY); +const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; + +const CONNECTED_ORIGIN = "https://dapp.example"; +const CONNECTED_HOSTNAME = "dapp.example"; +const EXT_URL = "chrome-extension://autistmask/"; + +const MAINNET = networkById("mainnet"); +const SEPOLIA = networkById("sepolia"); + +const NONCE = 7; +const REFRESHED_BALANCE = "1.5"; + +// The transaction the background populates, and the artifact signed from it. +// Its chain is a parameter because the whole subject here is a chain moving +// under work already committed to one. +function populated(chainId) { + return { + type: 2, + chainId, + nonce: NONCE, + gasLimit: 100000n, + maxFeePerGas: 2000000000n, + maxPriorityFeePerGas: 1000000000n, + to: RECIPIENT, + value: 10000000000000000n, + data: "0x", + }; +} + +function storedProfile(networkId) { + const net = networkById(networkId); + return { + hasWallet: true, + wallets: [ + { + name: "Wallet 1", + type: "hd", + xpub: "xpub-1", + addresses: [ + { + address: signer.address, + balance: "0.0", + tokenBalances: [], + }, + ], + }, + ], + activeAddress: signer.address, + networkId, + rpcUrl: net.defaultRpcUrl, + blockscoutUrl: net.defaultBlockscoutUrl, + allowedSites: { [signer.address]: [CONNECTED_HOSTNAME] }, + deniedSites: {}, + trackedTokens: [], + lastBalanceRefresh: 0, + }; +} + +async function settle() { + for (let i = 0; i < 60; i++) await Promise.resolve(); +} + +function deferred() { + let resolve; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; +} + +afterEach(() => { + delete global.chrome; +}); + +// The background worker over a cloning storage stub, with the network and the +// clock stubbed out. `opts.refreshBalances` replaces the balance refresh so a +// test can hold one open across another handler's whole turn. +function loadWorker(networkId, opts) { + const options = opts || {}; + jest.resetModules(); + + // Every provider this worker constructs, in order, with the endpoint and + // the network id it was given. The subject of the first test is which pair + // reaches the broadcast. + const providers = []; + const broadcastTransaction = jest.fn(async () => ({ hash: "0xfeed" })); + + jest.doMock("../src/shared/balances", () => ({ + getProvider: (rpcUrl, networkId2) => { + const provider = { + rpcUrl, + networkId: networkId2, + broadcastTransaction, + getNetwork: async () => ({ + chainId: BigInt(networkById(networkId2).networkVersion), + }), + getTransactionCount: async () => NONCE, + estimateGas: async () => 100000n, + getFeeData: async () => ({ + gasPrice: 2000000000n, + maxFeePerGas: 2000000000n, + maxPriorityFeePerGas: 1000000000n, + }), + }; + providers.push(provider); + return provider; + }, + refreshBalances: + options.refreshBalances || jest.fn(async () => undefined), + })); + jest.doMock("../src/shared/phishingDomains", () => ({ + isPhishingDomain: () => false, + })); + let alarmHandlers = {}; + jest.doMock("../src/shared/alarms", () => ({ + BALANCE_REFRESH_ALARM: "balance", + BALANCE_REFRESH_PERIOD_MINUTES: 1, + ensureRecurringAlarms: jest.fn(async () => {}), + registerAlarmHandlers: jest.fn((handlers) => { + alarmHandlers = handlers; + }), + })); + + const storage = makeStorageStub({ autistmask: storedProfile(networkId) }); + // A hook the tests use to suspend one handler mid-flight, so the other one + // runs entirely inside its window. + let getHook = null; + const realGet = storage.local.get; + storage.local.get = jest.fn(async (key) => { + if (getHook) await getHook(); + return realGet(key); + }); + + let messageListener = null; + // Every popup URL the background opened. The approval id is in it, and + // that is how the popup learns which approval it is answering. + const createdUrls = []; + global.chrome = { + storage, + runtime: { + getURL: (path) => EXT_URL + path, + onMessage: { + addListener: (fn) => { + messageListener = fn; + }, + }, + onConnect: { addListener: () => {} }, + lastError: null, + }, + windows: { + getLastFocused: (cb) => cb(null), + create: (createOpts, cb) => { + createdUrls.push(createOpts.url); + cb({ id: createdUrls.length }); + }, + remove: (id, cb) => { + if (cb) cb(); + }, + onRemoved: { addListener: () => {} }, + }, + tabs: { + query: (queryInfo, cb) => cb([{ id: 1 }]), + sendMessage: (tabId, message, cb) => { + if (cb) cb(); + }, + }, + action: { setPopup: () => {} }, + }; + + require("../src/background/index"); + + function send(msg, sender) { + let result = null; + const kept = messageListener(msg, sender, (r) => { + result = r; + }); + return { kept, result: () => result }; + } + + function rpc(method, params, origin) { + return send( + { type: "AUTISTMASK_RPC", method, params }, + { origin: origin || CONNECTED_ORIGIN }, + ); + } + + return { + rpc, + send, + providers, + broadcastTransaction, + persisted: () => storage.read("autistmask"), + setGetHook: (hook) => { + getHook = hook; + }, + fromPopup: { url: EXT_URL + "src/popup/index.html" }, + fireBalanceAlarm: () => alarmHandlers.balance(), + lastApprovalId: () => { + const url = createdUrls[createdUrls.length - 1]; + if (!url) return null; + return new URL(url, EXT_URL).searchParams.get("approval"); + }, + }; +} + +describe("a chain switch under a transaction already committed to a chain", () => { + // Item 4 of https://git.eeqj.de/sneak/AutistMask/issues/324. + // + // The artifact is verified against the chain read at the top of the + // attempt. Whatever endpoint it is then broadcast to has to be that same + // chain's — otherwise the wallet checks a transaction against Sepolia and + // sends it to a mainnet node. A connected site can switch the chain at any + // moment, including this one. + test("the artifact is broadcast to the endpoint of the chain it was verified against", async () => { + const bg = loadWorker("sepolia"); + + // Raise the approval, then find its id from the popup's own fetch. + bg.rpc("eth_sendTransaction", [ + { + from: signer.address, + to: RECIPIENT, + value: "0x2386f26fc10000", + data: "0x", + }, + ]); + await settle(); + + const id = bg.lastApprovalId(); + expect(id).toBeTruthy(); + + // The popup signs what it was shown: Sepolia. + const rawSignedTx = await signer.signTransaction( + populated(Number(SEPOLIA.networkVersion)), + ); + + // A connected site switches the chain while the attempt is running, + // and the switch is committed to storage in full before the attempt + // goes any further. + // + // It is fired from inside the attempt's SECOND state read, because + // that is where the window used to be: the chain id was captured at + // the first read and the endpoint was taken off the singleton several + // awaits later, so a switch landing between them moved the endpoint + // under an artifact already verified against the old chain. An + // implementation that takes both from one read has no second read for + // this to fire on, and the switch below runs after the attempt is + // done instead — which is the point. + let reads = 0; + let switched = null; + const doSwitch = async () => { + switched = bg.rpc("wallet_switchEthereumChain", [ + { chainId: MAINNET.chainId }, + ]); + await settle(); + }; + bg.setGetHook(async () => { + reads++; + if (reads !== 2) return; + bg.setGetHook(null); + await doSwitch(); + }); + + const attempt = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx, + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + bg.setGetHook(null); + if (!switched) await doSwitch(); + expect(switched.result()).toEqual({ result: null }); + expect(bg.persisted().networkId).toBe("mainnet"); + await settle(); + + // It went out, and it went out to Sepolia's node — the chain the + // artifact was verified against. Reading the endpoint separately from + // the chain id put mainnet's here. + expect(attempt.result()).toEqual({ txHash: "0xfeed" }); + expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1); + const used = bg.providers[bg.providers.length - 1]; + expect(used.rpcUrl).toBe(SEPOLIA.defaultRpcUrl); + expect(used.networkId).toBe("sepolia"); + }); +}); + +describe("a balance refresh under another handler's state read", () => { + // Item 5 of https://git.eeqj.de/sneak/AutistMask/issues/324. + // + // The trigger is a same-chain wallet_switchEthereumChain from a connected + // site: it answers { result: null } and changes nothing, so the ONLY thing + // it can do to the refresh is what its state read does. On the singleton + // that read replaced state.wallets, detaching the objects the refresh was + // mutating. + test("a chain read arriving mid-refresh does not discard the refresh", async () => { + const roundTrip = deferred(); + const reachedNetwork = deferred(); + + const bg = loadWorker("sepolia", { + refreshBalances: async (wallets) => { + reachedNetwork.resolve(); + await roundTrip.promise; + // In place, on the objects handed in — as balances.js does. + wallets[0].addresses[0].balance = REFRESHED_BALANCE; + }, + }); + + const refresh = bg.fireBalanceAlarm(); + await reachedNetwork.promise; + + const answered = bg.rpc("wallet_switchEthereumChain", [ + { chainId: SEPOLIA.chainId }, + ]); + await settle(); + expect(answered.result()).toEqual({ result: null }); + + roundTrip.resolve(); + await refresh; + + expect(bg.persisted().wallets[0].addresses[0].balance).toBe( + REFRESHED_BALANCE, + ); + expect(bg.persisted().lastBalanceRefresh).toBeGreaterThan(0); + }); + + // The other half of "does not publish a shared object": a wallet added + // while the refresh was in flight must survive the refresh's own write. + test("a wallet added mid-refresh survives the refresh's write", async () => { + const roundTrip = deferred(); + const reachedNetwork = deferred(); + + const bg = loadWorker("sepolia", { + refreshBalances: async (wallets) => { + reachedNetwork.resolve(); + await roundTrip.promise; + wallets[0].addresses[0].balance = REFRESHED_BALANCE; + }, + }); + + const refresh = bg.fireBalanceAlarm(); + await reachedNetwork.promise; + + // Another extension page adds a wallet while the round trip is out. + const during = bg.persisted(); + during.wallets.push({ + name: "Wallet 2", + type: "hd", + xpub: "xpub-2", + addresses: [ + { address: RECIPIENT, balance: "0.0", tokenBalances: [] }, + ], + }); + global.chrome.storage.write("autistmask", during); + + roundTrip.resolve(); + await refresh; + + const after = bg.persisted(); + expect(after.wallets).toHaveLength(2); + expect(after.wallets[0].addresses[0].balance).toBe(REFRESHED_BALANCE); + }); +}); diff --git a/tests/backgroundStateLintRule.test.js b/tests/backgroundStateLintRule.test.js new file mode 100644 index 0000000..29b0b4d --- /dev/null +++ b/tests/backgroundStateLintRule.test.js @@ -0,0 +1,235 @@ +// The lint rule that keeps src/shared/state.js out of the background bundle +// (script/lib/eslint/noStateSingletonInBackground.js). +// +// Five defects, one of which destroyed a wallet, came from background code +// reaching that singleton, and each point fix created the next site +// (https://git.eeqj.de/sneak/AutistMask/issues/324). +// +// What this file does NOT do is establish that the singleton cannot reach the +// background bundle. That is build.js's FORBIDDEN_INPUTS assertion, which reads +// esbuild's metafile and so cannot be evaded by a syntax a matcher does not +// know. The rule under test here is fast local feedback in front of that, and +// these cases pin the shapes it is known to catch, so a regression in the +// matcher is a failing test rather than a quietly narrower rule. +// +// Every shape below was measured against a real `make build`: each one puts +// state.js in the shipped background bundles, and each one was invisible to +// some earlier revision of the matcher — the quoted-only regex missed the +// backtick, the dynamic import and the `from` clause; its successor missed a +// comment inside the call and a directory resolved through package.json `main`. +// +// NOT covered, deliberately: a computed specifier such as +// `require("../shared/" + "state")`. esbuild cannot resolve that statically +// either, so it never reaches the bundle — there is nothing to block. + +const fs = require("fs"); +const os = require("os"); +const path = require("path"); +const { Linter } = require("eslint"); + +const plugin = require("../script/lib/eslint/noStateSingletonInBackground"); + +const RULE = "background/no-state-singleton-in-background"; + +// The three files a fixture tree always has. `src/background/index.js` is +// supplied per case; the other two stand in for the real modules. +const SHARED_STATE = "const state = {};\nmodule.exports = { state };\n"; +const SHARED_HOP = + "// A shared module the background legitimately imports.\n" + + "module.exports = { applyChainSwitchFields() {} };\n"; + +let roots = []; + +function fixture(files) { + const root = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), "autistmask-state-rule-")), + ); + roots.push(root); + const tree = { + "src/shared/state.js": SHARED_STATE, + "src/shared/chainSwitchFields.js": SHARED_HOP, + ...files, + }; + for (const [rel, source] of Object.entries(tree)) { + const abs = path.join(root, rel); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, source); + } + return root; +} + +// Run the rule exactly as eslint.config.js runs it, over a real tree: the walk +// reads its sources from disk, so a virtual RuleTester would not exercise it. +// `sourceType` is the fixture's own, not the rule's business: the walk is +// textual and never parses the files it follows. The two ESM cases below pass +// "module" only so espree can parse the fixture at all — in this repo those +// shapes are also a parse error under the commonjs config, but the rule must +// not be left depending on that. +function lintBackground(root, { sourceType = "commonjs" } = {}) { + const file = path.join(root, "src/background/index.js"); + const linter = new Linter({ cwd: root }); + return linter.verify( + fs.readFileSync(file, "utf8"), + { + plugins: { background: plugin }, + languageOptions: { ecmaVersion: 2024, sourceType }, + rules: { [RULE]: "error" }, + }, + file, + ); +} + +function chainOf(messages) { + expect(messages).toHaveLength(1); + expect(messages[0].ruleId).toBe(RULE); + // "...singleton: . The MV3 worker..." — the chain is what the + // message exists to hand the reader, so assert on it rather than on the + // fact that something was reported. + return messages[0].message.split("singleton: ")[1].split(". The MV3")[0]; +} + +afterEach(() => { + for (const root of roots) fs.rmSync(root, { recursive: true, force: true }); + roots = []; +}); + +describe("the specifier syntaxes the matcher is known to catch", () => { + test("a quoted require", () => { + const root = fixture({ + "src/background/index.js": + 'const { state } = require("../shared/state");\n' + + "module.exports = { state };\n", + }); + expect(chainOf(lintBackground(root))).toBe( + "src/background/index.js -> src/shared/state.js", + ); + }); + + test("a backtick require", () => { + const root = fixture({ + "src/background/index.js": + "const { state } = require(`../shared/state`);\n" + + "module.exports = { state };\n", + }); + expect(chainOf(lintBackground(root))).toBe( + "src/background/index.js -> src/shared/state.js", + ); + }); + + test("a dynamic import inside an async function", () => { + const root = fixture({ + "src/background/index.js": + "async function readState() {\n" + + ' const m = await import("../shared/state");\n' + + " return m.state;\n" + + "}\n" + + "module.exports = { readState };\n", + }); + expect(chainOf(lintBackground(root))).toBe( + "src/background/index.js -> src/shared/state.js", + ); + }); + + test("a static import from-clause", () => { + const root = fixture({ + "src/background/index.js": + 'import { state } from "../shared/state";\n' + + "export { state };\n", + }); + expect(chainOf(lintBackground(root, { sourceType: "module" }))).toBe( + "src/background/index.js -> src/shared/state.js", + ); + }); + + // `import(/* webpackChunkName: "x" */ "./x")` is the standard bundler + // annotation idiom, and prettier leaves both of these exactly as written, + // so nothing else in the repo would object to them either. + test("a comment between the paren and the specifier", () => { + const root = fixture({ + "src/background/index.js": + 'globalThis.__probe = require(/* probe */ "../shared/state").state;\n', + }); + expect(chainOf(lintBackground(root))).toBe( + "src/background/index.js -> src/shared/state.js", + ); + }); + + test("a comment between the specifier and the closing paren", () => { + const root = fixture({ + "src/background/index.js": + 'globalThis.__probe = require("../shared/state" /* probe */).state;\n', + }); + expect(chainOf(lintBackground(root))).toBe( + "src/background/index.js -> src/shared/state.js", + ); + }); + + test("a bare side-effect import", () => { + const root = fixture({ + "src/background/index.js": 'import "../shared/state";\n', + }); + expect(chainOf(lintBackground(root, { sourceType: "module" }))).toBe( + "src/background/index.js -> src/shared/state.js", + ); + }); +}); + +describe("reachability, not just the direct specifier", () => { + // The shape a no-restricted-imports could never see: no background file + // names state.js, and the singleton is in the bundle anyway. In a backtick + // require, so this fails on the specifier widening as well as on the walk. + test("a two-hop re-export through a shared module", () => { + const root = fixture({ + "src/background/index.js": + 'const { applyChainSwitchFields } = require("../shared/chainSwitchFields");\n' + + "module.exports = { applyChainSwitchFields };\n", + "src/shared/chainSwitchFields.js": + SHARED_HOP + + "module.exports.state = require(`./state`).state;\n", + }); + expect(chainOf(lintBackground(root))).toBe( + "src/background/index.js -> src/shared/chainSwitchFields.js" + + " -> src/shared/state.js", + ); + }); + + // Resolution, not syntax: the specifier names a directory, and the file it + // resolves to is chosen by that directory's package.json `main`. A walk + // that only tries `/index.js` stops on a specifier it matched. + test("a directory resolved through its package.json main", () => { + const root = fixture({ + "src/background/index.js": + 'globalThis.__probe = require("../shared/probepkg").state;\n', + "src/shared/probepkg/package.json": '{"main": "./bridge.js"}\n', + "src/shared/probepkg/bridge.js": + 'const { state } = require("../state");\n' + + "module.exports = { state };\n", + }); + expect(chainOf(lintBackground(root))).toBe( + "src/background/index.js -> src/shared/probepkg/bridge.js" + + " -> src/shared/state.js", + ); + }); +}); + +describe("what the rule must not report", () => { + test("a background file that reaches only its own state layer", () => { + const root = fixture({ + "src/background/index.js": + 'const { getState } = require("./state");\n' + + 'const { applyChainSwitchFields } = require("../shared/chainSwitchFields");\n' + + "module.exports = { getState, applyChainSwitchFields };\n", + "src/background/state.js": + "async function getState() {}\nmodule.exports = { getState };\n", + }); + expect(lintBackground(root)).toEqual([]); + }); + + // The tree as it actually stands. This is the assertion that would catch a + // widened matcher that resolves something it should not: it runs the rule + // over the real background entrypoint, from the real repo root. + test("the repository's own background entrypoint", () => { + const root = path.resolve(__dirname, ".."); + expect(lintBackground(root)).toEqual([]); + }); +}); diff --git a/tests/chainSwitchGate.test.js b/tests/chainSwitchGate.test.js index f2213bc..eafcbdd 100644 --- a/tests/chainSwitchGate.test.js +++ b/tests/chainSwitchGate.test.js @@ -8,10 +8,12 @@ // broadcast — because an error code alone would not distinguish a gate from // a switch that happened and then reported a failure. // -// The endpoint half of that issue lives in tests/networkEndpoints.test.js; -// this file mocks the state module, which that one exercises for real. +// The endpoint half of that issue lives in tests/networkEndpoints.test.js, +// which covers the popup's chain switch; this file covers the background's, +// which goes through storage rather than the shared state singleton. const { networkById } = require("../src/shared/networks"); +const { makeStorageStub } = require("./support/storageStub"); const ADDRESS = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; @@ -51,29 +53,11 @@ afterEach(() => { // --------------------------------------------------------------------------- // Load the background worker against stubbed browser APIs, with the real -// chain-switch module behind it, and return the handles to drive it. The -// wallet state is a plain object so that a switch that DID happen is visible -// as a mutation of it, and one that did not is visible as its absence. +// chain-switch and persistence modules behind it, and return the handles to +// drive it. function loadBackground() { jest.resetModules(); - const walletState = { - networkId: "mainnet", - rpcUrl: CUSTOM_RPC, - blockscoutUrl: MAINNET.defaultBlockscoutUrl, - networkEndpoints: {}, - wallets: walletFixture(), - lastBalanceRefresh: 1, - tokenHolderCache: {}, - fraudContracts: [], - }; - - jest.doMock("../src/shared/state", () => ({ - state: walletState, - loadState: jest.fn(async () => {}), - saveState: jest.fn(async () => {}), - currentNetwork: () => networkById(walletState.networkId), - })); jest.doMock("../src/shared/balances", () => ({ getProvider: () => ({}), refreshBalances: jest.fn(async () => {}), @@ -88,12 +72,24 @@ function loadBackground() { registerAlarmHandlers: jest.fn(), })); + // Storage is the only wallet state there is. The background reads and + // writes it per call — it holds no in-memory copy and cannot reach the + // shared singleton — so a switch that happened is visible here as a + // written record, and one that did not is visible as its absence. const persisted = { + networkId: "mainnet", + rpcUrl: CUSTOM_RPC, + blockscoutUrl: MAINNET.defaultBlockscoutUrl, + networkEndpoints: {}, wallets: walletFixture(), + lastBalanceRefresh: 1, + tokenHolderCache: {}, + fraudContracts: [], activeAddress: ADDRESS, allowedSites: { [ADDRESS]: [CONNECTED_HOSTNAME] }, deniedSites: {}, }; + const storage = makeStorageStub({ autistmask: persisted }); let messageListener = null; // Every message the background pushed at a content script. chainChanged @@ -102,12 +98,7 @@ function loadBackground() { const toTabs = []; global.chrome = { - storage: { - local: { - get: jest.fn(async () => ({ autistmask: persisted })), - set: jest.fn(async () => {}), - }, - }, + storage, runtime: { getURL: (path) => "chrome-extension://autistmask/" + path, onMessage: { @@ -157,7 +148,7 @@ function loadBackground() { return { switchChain, - walletState, + walletState: () => storage.read("autistmask"), chainChangedEvents: () => toTabs.filter((m) => m.eventName === "chainChanged"), }; @@ -174,8 +165,8 @@ describe("wallet_switchEthereumChain is gated on the connection", () => { // The refusal has to be a refusal to ACT, not just an error string: // the wallet is still on mainnet, still on the user's own node, and // no page was told the chain moved. - expect(bg.walletState.networkId).toBe("mainnet"); - expect(bg.walletState.rpcUrl).toBe(CUSTOM_RPC); + expect(bg.walletState().networkId).toBe("mainnet"); + expect(bg.walletState().rpcUrl).toBe(CUSTOM_RPC); expect(bg.chainChangedEvents()).toEqual([]); }); @@ -201,7 +192,7 @@ describe("wallet_switchEthereumChain is gated on the connection", () => { const result = await bg.switchChain(SEPOLIA.chainId, CONNECTED_ORIGIN); expect(result).toEqual({ result: null }); - expect(bg.walletState.networkId).toBe("sepolia"); + expect(bg.walletState().networkId).toBe("sepolia"); expect(bg.chainChangedEvents()).toEqual([ { type: "AUTISTMASK_EVENT", @@ -217,16 +208,16 @@ describe("wallet_switchEthereumChain is gated on the connection", () => { const result = await bg.switchChain("0x89", CONNECTED_ORIGIN); expect(result.error.code).toBe(4902); - expect(bg.walletState.networkId).toBe("mainnet"); + expect(bg.walletState().networkId).toBe("mainnet"); }); test("a switch by a connected origin keeps the user's endpoint", async () => { const bg = loadBackground(); await bg.switchChain(SEPOLIA.chainId, CONNECTED_ORIGIN); - expect(bg.walletState.rpcUrl).toBe(SEPOLIA.defaultRpcUrl); + expect(bg.walletState().rpcUrl).toBe(SEPOLIA.defaultRpcUrl); await bg.switchChain(MAINNET.chainId, CONNECTED_ORIGIN); - expect(bg.walletState.rpcUrl).toBe(CUSTOM_RPC); + expect(bg.walletState().rpcUrl).toBe(CUSTOM_RPC); }); }); diff --git a/tests/coldWorkerChainSwitch.test.js b/tests/coldWorkerChainSwitch.test.js index 36f2dbf..e2a8f6b 100644 --- a/tests/coldWorkerChainSwitch.test.js +++ b/tests/coldWorkerChainSwitch.test.js @@ -16,6 +16,7 @@ // first, so neither can see this. const { networkById } = require("../src/shared/networks"); +const { makeStorageStub } = require("./support/storageStub"); const ADDRESS = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; @@ -64,9 +65,12 @@ afterEach(() => { delete global.chrome; }); -// Load the background worker with the real state and chain-switch modules -// behind it, over a storage stub that actually keeps what is written — a -// wipe is only observable against storage that remembers. +// Load the background worker with the real chain-switch and persistence +// modules behind it, over a storage stub that actually keeps what is written — +// a wipe is only observable against storage that remembers — and that clones +// in both directions, as the real API does. It used to alias, so the record +// the worker held and the "stored" one were a single object; see +// tests/support/storageStub.js. function loadColdWorker(networkId) { jest.resetModules(); @@ -84,20 +88,13 @@ function loadColdWorker(networkId) { registerAlarmHandlers: jest.fn(), })); - const store = { autistmask: storedProfile(networkId) }; + const storage = makeStorageStub({ autistmask: storedProfile(networkId) }); let messageListener = null; const toTabs = []; global.chrome = { - storage: { - local: { - get: jest.fn(async () => ({ autistmask: store.autistmask })), - set: jest.fn(async (items) => { - store.autistmask = items.autistmask; - }), - }, - }, + storage, runtime: { getURL: (path) => "chrome-extension://autistmask/" + path, onMessage: { @@ -147,7 +144,7 @@ function loadColdWorker(networkId) { return { switchChain, - persisted: () => store.autistmask, + persisted: () => storage.read("autistmask"), chainChangedEvents: () => toTabs.filter((m) => m.eventName === "chainChanged"), }; diff --git a/tests/coldWorkerSendTransaction.test.js b/tests/coldWorkerSendTransaction.test.js new file mode 100644 index 0000000..faf6158 --- /dev/null +++ b/tests/coldWorkerSendTransaction.test.js @@ -0,0 +1,279 @@ +// Which chain a dApp transaction is PREPARED for on a worker that has not +// loaded state. +// +// The MV3 service worker is terminated when idle — roughly 30 seconds, which +// is its normal condition — and revived by the page's own message. Nothing +// loads state at module scope, so handleSendTransaction() used to build its +// provider with `getProvider(await getRpcUrl())`: the endpoint came from +// storage and was right, and the static network hint was omitted, so +// src/shared/balances.js fell back to currentNetwork() — the unpopulated +// singleton — and answered mainnet. ethers then fixed `chainId` at 0x1. +// +// The transaction was not sent on the wrong chain: verifySignedTx() compares +// the artifact against the selected chain and refused it. So the guard held +// and the feature did not — a user on any non-mainnet network could not send +// from a dApp at all, and the error described the symptom +// (https://git.eeqj.de/sneak/AutistMask/issues/320). +// +// This drives the real balances module and the real approval preparation and +// verification. Only ethers' JsonRpcProvider is replaced, so the static +// network hint getProvider() computes is the hint the population sees. + +const { Network, Wallet, Transaction } = require("ethers"); +const { networkById } = require("../src/shared/networks"); +const { makeStorageStub } = require("./support/storageStub"); + +const SIGNER_KEY = + "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d"; +const signer = new Wallet(SIGNER_KEY); +const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; + +const CONNECTED_ORIGIN = "https://dapp.example"; +const CONNECTED_HOSTNAME = "dapp.example"; +const EXT_URL = "chrome-extension://autistmask/"; + +const SEPOLIA = networkById("sepolia"); +const MAINNET = networkById("mainnet"); + +const NONCE = 7; +const TX_HASH = "0xfeed"; + +const TX_PARAMS = { + from: signer.address, + to: RECIPIENT, + value: "0x2386f26fc10000", + data: "0x", +}; + +function storedProfile(networkId) { + const net = networkById(networkId); + return { + hasWallet: true, + wallets: [ + { + name: "Wallet 1", + type: "hd", + xpub: "xpub-1", + addresses: [ + { + address: signer.address, + balance: "0.0", + tokenBalances: [], + }, + ], + }, + ], + activeAddress: signer.address, + networkId, + rpcUrl: net.defaultRpcUrl, + blockscoutUrl: net.defaultBlockscoutUrl, + allowedSites: { [signer.address]: [CONNECTED_HOSTNAME] }, + deniedSites: {}, + trackedTokens: [], + }; +} + +async function settle() { + for (let i = 0; i < 60; i++) await Promise.resolve(); +} + +afterEach(() => { + delete global.chrome; +}); + +// A worker whose only wallet state is what is in storage, with ethers' +// JsonRpcProvider replaced by a stub that answers out of the static network it +// was constructed with — which is exactly what a real staticNetwork provider +// does, and what makes the chain id on the approval screen observable here. +function loadColdWorker(networkId) { + jest.resetModules(); + + const constructed = []; + const broadcast = []; + + jest.doMock("ethers", () => { + const actual = jest.requireActual("ethers"); + class StubJsonRpcProvider { + constructor(url, network) { + this._network = network; + constructed.push({ url, network }); + } + async getNetwork() { + return this._network; + } + async getTransactionCount() { + return NONCE; + } + async estimateGas() { + return 100000n; + } + async getFeeData() { + return { + gasPrice: 2000000000n, + maxFeePerGas: 2000000000n, + maxPriorityFeePerGas: 1000000000n, + }; + } + async broadcastTransaction(raw) { + broadcast.push(raw); + return { hash: TX_HASH }; + } + } + return { ...actual, JsonRpcProvider: StubJsonRpcProvider }; + }); + jest.doMock("../src/shared/phishingDomains", () => ({ + isPhishingDomain: () => false, + })); + jest.doMock("../src/shared/alarms", () => ({ + BALANCE_REFRESH_ALARM: "balance", + BALANCE_REFRESH_PERIOD_MINUTES: 1, + ensureRecurringAlarms: jest.fn(async () => {}), + registerAlarmHandlers: jest.fn(), + })); + + const storage = makeStorageStub({ autistmask: storedProfile(networkId) }); + + let messageListener = null; + const createdUrls = []; + + global.chrome = { + storage, + runtime: { + getURL: (path) => EXT_URL + path, + onMessage: { + addListener: (fn) => { + messageListener = fn; + }, + }, + onConnect: { addListener: () => {} }, + lastError: null, + }, + windows: { + getLastFocused: (cb) => cb(null), + create: (opts, cb) => { + createdUrls.push(opts.url); + cb({ id: createdUrls.length }); + }, + remove: (id, cb) => { + if (cb) cb(); + }, + onRemoved: { addListener: () => {} }, + }, + tabs: { + query: (queryInfo, cb) => cb([{ id: 1 }]), + sendMessage: (tabId, message, cb) => { + if (cb) cb(); + }, + }, + action: { setPopup: () => {} }, + }; + + require("../src/background/index"); + + function send(msg, sender) { + let result = null; + messageListener(msg, sender, (r) => { + result = r; + }); + return () => result; + } + + return { + send, + constructed, + broadcast, + fromPopup: { url: EXT_URL + "src/popup/index.html" }, + // The first message this worker ever sees, as the injected provider + // sends it. + sendTransaction: () => + send( + { + type: "AUTISTMASK_RPC", + method: "eth_sendTransaction", + params: [TX_PARAMS], + }, + { origin: CONNECTED_ORIGIN }, + ), + approvalId: () => { + const url = createdUrls[createdUrls.length - 1]; + return url + ? new URL(url, EXT_URL).searchParams.get("approval") + : null; + }, + }; +} + +// What the approval window does: fetch the approval and sign the transaction +// it was handed, exactly as given. +function signApproved(approvedTx) { + const tx = {}; + for (const [key, value] of Object.entries(approvedTx)) { + if (key === "from") continue; + tx[key] = value; + } + return signer.signTransaction(tx); +} + +describe("a dApp transaction prepared by a worker that never loaded state", () => { + test("a cold send on Sepolia reaches the approval screen and goes out", async () => { + const bg = loadColdWorker("sepolia"); + + const answer = bg.sendTransaction(); + await settle(); + + // The provider was built for Sepolia, endpoint and static hint + // together. Omitting the hint made this mainnet. + expect(bg.constructed).toHaveLength(1); + expect(bg.constructed[0].url).toBe(SEPOLIA.defaultRpcUrl); + expect(bg.constructed[0].network.chainId).toBe( + Network.from("sepolia").chainId, + ); + + // So the approval the user is shown is a Sepolia transaction. + const id = bg.approvalId(); + expect(id).toBeTruthy(); + const approval = bg.send( + { type: "AUTISTMASK_GET_APPROVAL", id }, + { url: bg.fromPopup.url }, + )(); + expect(approval.type).toBe("tx"); + expect(approval.approvedTx.chainId).toBe(SEPOLIA.chainId); + + // And it survives the wallet's own verification, which is where a + // 0x1-stamped artifact was refused as "for a different network". + const rawSignedTx = await signApproved(approval.approvedTx); + const response = bg.send( + { + type: "AUTISTMASK_TX_RESPONSE", + id, + approved: true, + rawSignedTx, + }, + { url: bg.fromPopup.url }, + ); + await settle(); + + expect(response()).toEqual({ txHash: TX_HASH }); + expect(bg.broadcast).toEqual([rawSignedTx]); + expect(Number(Transaction.from(rawSignedTx).chainId)).toBe( + Number(SEPOLIA.networkVersion), + ); + expect(answer()).toEqual({ result: TX_HASH }); + }); + + test("a cold send on mainnet is prepared for mainnet", async () => { + // The stored value and the old fallback agree here, so this case + // cannot catch the defect; it is what keeps the fix from being a swap. + const bg = loadColdWorker("mainnet"); + + bg.sendTransaction(); + await settle(); + + expect(bg.constructed[0].url).toBe(MAINNET.defaultRpcUrl); + const approval = bg.send( + { type: "AUTISTMASK_GET_APPROVAL", id: bg.approvalId() }, + { url: bg.fromPopup.url }, + )(); + expect(approval.approvedTx.chainId).toBe(MAINNET.chainId); + }); +}); diff --git a/tests/deleteAddress.test.js b/tests/deleteAddress.test.js index 1d89ce5..f59455e 100644 --- a/tests/deleteAddress.test.js +++ b/tests/deleteAddress.test.js @@ -19,6 +19,14 @@ const { balanceWarningHtml, } = require("../src/popup/views/deleteAddress"); const { prices, clearPrices } = require("../src/shared/prices"); +const { state } = require("../src/shared/state"); + +// The screen prices holdings, and pricing asks which chain it is on. Reading +// the singleton's network before anything loaded it now throws rather than +// answering mainnet by default +// (https://git.eeqj.de/sneak/AutistMask/issues/324), so the network this +// fixture is on is stated instead of assumed. +state.networkId = "mainnet"; const USDC = "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48"; diff --git a/tests/deleteWalletLostPassword.test.js b/tests/deleteWalletLostPassword.test.js index f020aeb..8fa3d8e 100644 --- a/tests/deleteWalletLostPassword.test.js +++ b/tests/deleteWalletLostPassword.test.js @@ -13,13 +13,15 @@ // never persisting it looks identical from `state`, and a build that never // wrote at all would pass a check that only reads `state` back. // -// That makes the storage stub load-bearing, so it is a real store that -// structured-clones on both `set` and `get`. A stub whose `get` hands back -// the same object its `set` was given aliases the caller's own array: the -// test then reads its own in-memory mutation and calls it persistence, and -// passes against a build that persists nothing (see issue #324). The -// aliasing is closed off explicitly by the first test below rather than -// left as an assumption about `structuredClone`. +// That makes the storage stub load-bearing, so it is the shared one from +// tests/support/storageStub.js, a real store that structured-clones on both +// `set` and `get`. A stub whose `get` hands back the same object its `set` +// was given aliases the caller's own array: the test then reads its own +// in-memory mutation and calls it persistence, and passes against a build +// that persists nothing +// (https://git.eeqj.de/sneak/AutistMask/issues/324). The aliasing is closed +// off explicitly by the first test below rather than left as an assumption +// about `structuredClone`. // // The view is driven against a minimal DOM stub, in the same shape as // tests/exportPrivkey.test.js: the module reads and writes named nodes and @@ -34,6 +36,7 @@ jest.mock("../src/shared/vault", () => ({ })); const { RESTORABLE_VIEWS } = require("../src/popup/restorableViews"); +const { makeStorageStub } = require("./support/storageStub"); const VIEW = "delete-wallet-lost-password"; @@ -94,35 +97,6 @@ function makeDocument() { }; } -// --------------------------------------------------------- storage stub - -// A store that behaves the way `chrome.storage.local` does: what goes in is -// serialized, so the caller keeps no handle on what came to rest there, and -// what comes out is a fresh object the caller may mutate freely. -function makeStorage() { - let store = {}; - return { - get: async (keys) => { - const wanted = - keys === undefined || keys === null - ? Object.keys(store) - : [].concat(keys); - const out = {}; - for (const key of wanted) { - if (key in store) out[key] = structuredClone(store[key]); - } - return out; - }, - set: async (items) => { - for (const [key, value] of Object.entries(items)) { - store[key] = structuredClone(value); - } - }, - // Test-only: what the extension would find on a cold start. - _raw: () => structuredClone(store), - }; -} - // ------------------------------------------------------------ harness function wallet(name, secret, addresses) { @@ -144,10 +118,10 @@ function load() { jest.resetModules(); mockSettingsShow.mockClear(); - const storage = makeStorage(); + const storage = makeStorageStub(); const sent = []; globalThis.chrome = { - storage: { local: storage }, + storage: { local: storage.local }, runtime: { sendMessage: (msg) => sent.push(msg) }, }; globalThis.document = makeDocument(); @@ -205,7 +179,7 @@ async function openLostPassword(deleteWallet, walletIdx) { // every other test in this file against a build that never writes. describe("the storage stub", () => { test("does not hand back the object it was given", async () => { - const storage = makeStorage(); + const storage = makeStorageStub(); const written = { wallets: [{ name: "Wallet 1" }] }; await storage.set({ autistmask: written }); @@ -393,8 +367,8 @@ describe("deleting without the password", () => { // The deleted wallet's secret is gone from storage entirely, not // merely unreferenced by the wallet list. - expect(JSON.stringify(storage._raw())).not.toContain("secret-two"); - expect(JSON.stringify(storage._raw())).not.toContain("xpub-Wallet 2"); + expect(JSON.stringify(storage.read())).not.toContain("secret-two"); + expect(JSON.stringify(storage.read())).not.toContain("xpub-Wallet 2"); }); test("only the deleted wallet's site permissions are dropped", async () => { @@ -462,7 +436,7 @@ describe("deleting without the password", () => { expect(saved.activeAddress).toBeNull(); expect(saved.allowedSites).toEqual({}); expect(state.currentView).toBe("welcome"); - expect(JSON.stringify(storage._raw())).not.toContain("secret-one"); + expect(JSON.stringify(storage.read())).not.toContain("secret-one"); }); }); diff --git a/tests/networkEndpoints.test.js b/tests/networkEndpoints.test.js index 8ef4388..d403505 100644 --- a/tests/networkEndpoints.test.js +++ b/tests/networkEndpoints.test.js @@ -10,6 +10,7 @@ // happened. const { networkById } = require("../src/shared/networks"); +const { makeStorageStub } = require("./support/storageStub"); const ADDRESS = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; @@ -34,25 +35,19 @@ function walletFixture() { // saveState() wrote — so a case can reload a fresh module from the bytes an // earlier one persisted, which is what an extension restart does. `state` is // a module-level singleton, so the registry has to be reset per load. +// The stub clones in both directions, as the real chrome.storage.local does. +// It used to alias, and written() then handed the NEXT module load the live +// in-memory object of the previous one as its "persisted bytes" — an extension +// restart that never crossed a serialization boundary. See +// tests/support/storageStub.js. function loadModuleWith(persisted) { jest.resetModules(); - let written = null; - global.chrome = { - storage: { - local: { - get: jest.fn(async () => - persisted ? { autistmask: persisted } : {}, - ), - set: jest.fn(async (items) => { - written = items.autistmask; - }), - }, - }, - }; + const storage = makeStorageStub(persisted ? { autistmask: persisted } : {}); + global.chrome = { storage }; return { mod: require("../src/shared/state"), chainSwitch: require("../src/shared/chainSwitch"), - written: () => written, + written: () => storage.read("autistmask"), }; } diff --git a/tests/settingsUtcTimestamps.test.js b/tests/settingsUtcTimestamps.test.js index 91526db..129033a 100644 --- a/tests/settingsUtcTimestamps.test.js +++ b/tests/settingsUtcTimestamps.test.js @@ -9,6 +9,8 @@ const fs = require("fs"); const path = require("path"); +const { makeStorageStub } = require("./support/storageStub"); + const POPUP_HTML = fs.readFileSync( path.join(__dirname, "..", "src", "popup", "index.html"), "utf8", @@ -51,19 +53,17 @@ describe("the UTC Timestamps checkbox placement", () => { }); describe("the UTC Timestamps setting round-trips through storage", () => { - let store; + let storage; + // The stub clones in both directions, as the real chrome.storage.local + // does. It used to alias, which is fatal to a round-trip test in + // particular: the object the module holds and the object "storage" holds + // are then the same object, so the setting appears to have been persisted + // and read back on a build where neither happened. See + // tests/support/storageStub.js. function loadStateModule() { - store = {}; - global.chrome = { - storage: { - local: { - get: async (key) => - key in store ? { [key]: store[key] } : {}, - set: async (obj) => Object.assign(store, obj), - }, - }, - }; + storage = makeStorageStub(); + global.chrome = { storage }; jest.resetModules(); return require("../src/shared/state"); } @@ -86,12 +86,18 @@ describe("the UTC Timestamps setting round-trips through storage", () => { // What the change handler in views/settings.js does. first.state.utcTimestamps = true; await first.saveState(); - expect(store.autistmask.utcTimestamps).toBe(true); + expect(storage.read("autistmask").utcTimestamps).toBe(true); - // A fresh popup load sees it. + // A fresh popup load sees it — and, before that load, refuses to + // answer at all rather than reporting the default. That refusal is + // what makes the assertion below evidence of a read from storage + // instead of a value that was already sitting in memory + // (https://git.eeqj.de/sneak/AutistMask/issues/324). jest.resetModules(); const second = require("../src/shared/state"); - expect(second.state.utcTimestamps).toBe(false); + expect(() => second.state.utcTimestamps).toThrow( + second.StateNotLoadedError, + ); await second.loadState(); expect(second.state.utcTimestamps).toBe(true); }); diff --git a/tests/state.test.js b/tests/state.test.js index af67fc8..61c8ad4 100644 --- a/tests/state.test.js +++ b/tests/state.test.js @@ -4,23 +4,24 @@ function oneWallet() { return [{ name: "Wallet 1", type: "hd", addresses: [ADDRESS] }]; } +const { makeStorageStub } = require("./support/storageStub"); + // state.js resolves the storage API at require time, so the stub has to exist // before the module is loaded, and the module registry has to be reset between // cases because `state` is a module-level singleton. +// +// The stub clones in both directions, as the real chrome.storage.local does — +// see tests/support/storageStub.js for why an aliasing one made this file +// assert less than it appears to. function loadModuleWith(persisted) { jest.resetModules(); - const set = jest.fn(async () => {}); - global.chrome = { - storage: { - local: { - get: jest.fn(async () => - persisted ? { autistmask: persisted } : {}, - ), - set, - }, - }, + const storage = makeStorageStub(persisted ? { autistmask: persisted } : {}); + global.chrome = { storage }; + return { + mod: require("../src/shared/state"), + set: storage.set, + stored: () => storage.read("autistmask"), }; - return { mod: require("../src/shared/state"), set }; } afterEach(() => { diff --git a/tests/stateMerge.test.js b/tests/stateMerge.test.js index fb02e73..6afa786 100644 --- a/tests/stateMerge.test.js +++ b/tests/stateMerge.test.js @@ -10,32 +10,12 @@ // // Both cases below drive the real state.js module through two independent // module registries sharing one storage backend, the way two real extension -// pages share one chrome.storage.local. The storage stub structured-clones -// on both get and set — a stub that hands back the object it was given -// aliases the caller's own mutation and would make this entire defect class -// invisible (see https://git.eeqj.de/sneak/AutistMask/issues/324). +// pages share one chrome.storage.local. The shared stub structured-clones on +// both get and set — a stub that hands back the object it was given aliases +// the caller's own mutation and would make this entire defect class invisible +// (see https://git.eeqj.de/sneak/AutistMask/issues/324). -function makeStorage() { - let store = {}; - return { - get: async (keys) => { - const wanted = - keys === undefined || keys === null - ? Object.keys(store) - : [].concat(keys); - const out = {}; - for (const key of wanted) { - if (key in store) out[key] = structuredClone(store[key]); - } - return out; - }, - set: async (items) => { - for (const [key, value] of Object.entries(items)) { - store[key] = structuredClone(value); - } - }, - }; -} +const { makeStorageStub } = require("./support/storageStub"); // One extension page: a fresh module registry over the shared storage. // state.js resolves the storage API at require time, so the stub has to be @@ -43,7 +23,7 @@ function makeStorage() { // singleton, so each page needs its own registry to hold its own copy. function loadPage(storage) { jest.resetModules(); - globalThis.chrome = { storage: { local: storage } }; + globalThis.chrome = { storage: { local: storage.local } }; return { state: require("../src/shared/state"), helpers: require("../src/popup/views/helpers"), @@ -118,7 +98,7 @@ describe("a save from a page that never saw a wallet another page added", () => // a save from a second page loaded before that wallet existed. Both // wallets must survive. test("both wallets are in storage afterwards", async () => { - const storage = makeStorage(); + const storage = makeStorageStub(); await storage.set({ autistmask: { wallets: [W1] } }); // Loaded while storage held only Wallet 1, and never reloads — @@ -171,7 +151,7 @@ describe("the approval-window reproduction", () => { test("the wallet added in the popup survives confirming the approval", async () => { globalThis.document = makeDocument(); - const storage = makeStorage(); + const storage = makeStorageStub(); await storage.set({ autistmask: { wallets: [W1] } }); // The background opens the approval window on the approve-tx @@ -223,7 +203,7 @@ describe("the approval-window reproduction", () => { // membership" collided as the same field. describe("background refresh racing a wallet added on another page", () => { test("the wallet added elsewhere survives background's stale balance save", async () => { - const storage = makeStorage(); + const storage = makeStorageStub(); await storage.set({ autistmask: { wallets: [W1] } }); // "background": loads first, and its save is the one that lands @@ -263,7 +243,7 @@ describe("background refresh racing a wallet added on another page", () => { describe("background refresh racing a wallet deleted on another page", () => { test("the wallet deleted elsewhere stays deleted after background's stale balance save", async () => { - const storage = makeStorage(); + const storage = makeStorageStub(); await storage.set({ autistmask: { wallets: [W1, W2] } }); const background = loadPage(storage); @@ -324,7 +304,7 @@ function revokeSite(pageState, hostname) { describe("a dApp approval racing a stale Settings page's later save", () => { test("the fresh approval survives Settings revoking an unrelated site", async () => { - const storage = makeStorage(); + const storage = makeStorageStub(); await storage.set({ autistmask: { wallets: [W1], @@ -362,7 +342,7 @@ describe("a dApp approval racing a stale Settings page's later save", () => { describe("a revoked site permission against a stale page's later save", () => { test("the revocation holds even when the stale page approves something else", async () => { - const storage = makeStorage(); + const storage = makeStorageStub(); await storage.set({ autistmask: { wallets: [W1], @@ -413,7 +393,7 @@ function legacyWallet(name, secret) { describe("two wallets independently created with a colliding identity", () => { test("both survive, encryptedSecret included, instead of one silently replacing the other", async () => { - const storage = makeStorage(); + const storage = makeStorageStub(); await storage.set({ autistmask: { wallets: [W1] } }); // Both pages load before either has created their malformed wallet, diff --git a/tests/support/storageStub.js b/tests/support/storageStub.js new file mode 100644 index 0000000..bce6354 --- /dev/null +++ b/tests/support/storageStub.js @@ -0,0 +1,73 @@ +// A chrome.storage.local stub that behaves like the real one. +// +// The real extension storage API is a serialization boundary: `set` writes a +// structured clone of what it is given, and `get` hands back a structured +// clone of what is stored. Nothing an extension page holds is ever the object +// storage holds. +// +// A stub that skips the clone aliases them together, and that hides an entire +// class of defect rather than merely being imprecise. loadState() assigns +// nested references straight out of the get result, so over an aliasing stub a +// test can assert "the endpoint was persisted" and pass on a build that never +// called saveState() at all: the in-memory mutation IS the stored record. +// Measured, not theorised — with an aliasing `get` restored over the handler +// fixed in https://git.eeqj.de/sneak/AutistMask/pulls/319, the whole suite +// passed 794/794 (https://git.eeqj.de/sneak/AutistMask/issues/324). +// +// So every test that drives real persistence uses this, and nothing rebuilds +// a storage stub by hand. + +// `initial` is the starting contents, keyed as storage is: { autistmask: {...} }. +// `onOp` runs before each operation, for a test that needs to advance a clock +// or count round trips. +function makeStorageStub(initial, onOp) { + const store = initial ? structuredClone(initial) : {}; + const tick = onOp || (() => {}); + + const get = jest.fn(async (key) => { + tick(); + if (key === undefined || key === null) return structuredClone(store); + const keys = Array.isArray(key) ? key : [key]; + const out = {}; + for (const k of keys) { + if (Object.prototype.hasOwnProperty.call(store, k)) { + out[k] = structuredClone(store[k]); + } + } + return out; + }); + + const set = jest.fn(async (items) => { + tick(); + for (const k of Object.keys(items)) { + store[k] = structuredClone(items[k]); + } + }); + + const remove = jest.fn(async (key) => { + tick(); + for (const k of Array.isArray(key) ? key : [key]) delete store[k]; + }); + + return { + // Drop this straight in as chrome.storage. + local: { get, set, remove }, + get, + set, + remove, + // What is stored, cloned on the way out: a test can neither observe a + // later write through an object it read nor reach into the store by + // mutating one. + read: (key) => + key === undefined + ? structuredClone(store) + : structuredClone(store[key]), + // Seed or replace a record without going through the module under + // test — for standing in as "another page wrote this". + write: (key, value) => { + store[key] = structuredClone(value); + }, + }; +} + +module.exports = { makeStorageStub }; diff --git a/tests/symbolSpoof.test.js b/tests/symbolSpoof.test.js index a122183..94163be 100644 --- a/tests/symbolSpoof.test.js +++ b/tests/symbolSpoof.test.js @@ -682,6 +682,7 @@ describe("surface 3: the balance list", () => { "https://rpc.example.invalid", BLOCKSCOUT, [], + "mainnet", ); expect(addr.balance).toBe("1.2345"); expect(addr.tokenBalances).toEqual([]); diff --git a/tests/transactions.test.js b/tests/transactions.test.js index cd66568..e1353ba 100644 --- a/tests/transactions.test.js +++ b/tests/transactions.test.js @@ -30,8 +30,11 @@ global.fetch = jest.fn(() => { }); // state.js reads chrome.storage.local at module load; stub it so the -// default settings can be asserted against what the README promises. -global.chrome = { storage: { local: {} } }; +// default settings can be asserted against what the README promises. Empty +// storage, so a load produces exactly the defaults. +const { makeStorageStub } = require("./support/storageStub"); + +global.chrome = { storage: makeStorageStub() }; const { fetchRecentTransactions, @@ -745,6 +748,16 @@ describe("dust threshold filtering", () => { }); describe("filter defaults promised by the README and Settings", () => { + // The defaults are what a load of empty storage produces, so the load is + // part of the assertion rather than an incantation before it: reading the + // singleton before any load now throws (StateNotLoadedError), because a + // context served DEFAULT_STATE without asking for it is the whole subject + // of https://git.eeqj.de/sneak/AutistMask/issues/324. The stub at the top + // of this file has storage empty. + beforeAll(async () => { + await require("../src/shared/state").loadState(); + }); + test("all four toggles default to on and the threshold to 100,000 gwei", () => { expect(state.hideSpoofedSymbols).toBe(true); expect(state.hideLowHolderTokens).toBe(true); diff --git a/tests/txStatus.test.js b/tests/txStatus.test.js index 5232bcc..2f92947 100644 --- a/tests/txStatus.test.js +++ b/tests/txStatus.test.js @@ -96,18 +96,14 @@ global.document = { global.window = { location: { search: "" } }; -const stored = {}; -global.chrome = { - storage: { - local: { - set: (obj) => { - Object.assign(stored, obj); - return Promise.resolve(); - }, - get: () => Promise.resolve(stored), - }, - }, -}; +// Clones in both directions, as the real chrome.storage.local does; the stub +// here used to hand back the live stored object, so an in-memory mutation +// looked like a write that had reached storage. See +// tests/support/storageStub.js. +const { makeStorageStub } = require("./support/storageStub"); + +const storage = makeStorageStub(); +global.chrome = { storage }; const txStatus = require("../src/popup/views/txStatus"); const { state } = require("../src/shared/state");