From 5c4a671d4a14b97b0851ea38a392f27a50b8b4fb Mon Sep 17 00:00:00 2001 From: clawbot Date: Thu, 20 Aug 2026 10:47:18 +0000 Subject: [PATCH] fix: answer eth_chainId and net_version from loaded state (closes #317) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both methods answered from currentNetwork(), which reads the module-level state singleton, and nothing populates that at module scope. A service worker revived by the page's own message therefore held DEFAULT_STATE and reported mainnet 0x1 / 1 to a page whose user was on Sepolia, so a dApp asking which chain the wallet is on built its interaction for the wrong one. Neither method is gated on a connection, so any page got the stale answer. Both now answer from getState() — the per-call storage read that returns a detached object, which every other read handler in this file already uses — rather than by loading the singleton. Loading it would fix the stale answer but introduce a worse defect on the same path: loadState() replaces state.wallets wholesale, and backgroundRefresh() hands the singleton's wallets to refreshBalances(), which mutates those address objects in place across a network round trip before stamping lastBalanceRefresh and saving. A load landing inside that round trip detaches the objects being mutated, so the save persists the pre-refresh balances while still marking the refresh done, and the freshness guard then suppresses the redo for half the alarm period. These two methods are reachable by any page, and the injected provider sends eth_chainId on every page load, so an ordinary page load would be enough to drop a refresh and a polling page could keep any refresh from ever persisting. getState() reads storage once per call and mutates nothing shared. networkById(undefined) already falls back to mainnet, which is the answer a profile with no stored networkId had before. Read-side audit of the background, which the fix was the occasion for: the other singleton reads are wallet_switchEthereumChain, the transaction verify/broadcast path and backgroundRefresh, and all three already load first. Every other handler answers from storage per call through getState(). One stale read remains and is deliberately not fixed here, being a different handler rather than the same one-line shape: handleSendTransaction calls getProvider() with no network name, so balances.js falls back to the same unloaded singleton for ethers' static network hint, and a cold-worker send on Sepolia is prepared with a mainnet hint. It is caught later — the artifact is verified against the loaded chain before broadcast — so it fails the send rather than sending on the wrong chain. The test's storage stub structured-clones in both directions, as the real chrome.storage.local does. A stub that hands back the live stored object aliases it into whatever reads it, which makes an in-place mutation of a detached copy look as though it reached storage and hides this entire class of defect: with an aliasing get, the whole suite passes against the loadState() version above. Verified failing first, two mutations, each with the rest of the tree untouched. Reverting the handler to the singleton read gives 3 failed / 791 passed: exactly the three cases that read the chain on a cold worker, each answering 0x1 / 1 instead of 0xaa36a7 / 11155111. Replacing getState() with await loadState() plus currentNetwork() gives 1 failed / 793 passed: the new mid-refresh case, with the persisted balance "0" where the refresh wrote "1.5". With the fix, 794 passed / 37 suites, and lint ran uncached in the pinned container. --- TODO.md | 18 +++ src/background/index.js | 34 ++++- tests/coldWorkerChainId.test.js | 258 ++++++++++++++++++++++++++++++++ 3 files changed, 303 insertions(+), 7 deletions(-) create mode 100644 tests/coldWorkerChainId.test.js diff --git a/TODO.md b/TODO.md index 5a7cafc..83c6d00 100644 --- a/TODO.md +++ b/TODO.md @@ -44,6 +44,24 @@ but the review is broader than any of them. # Completed Steps +- 2026-08-20: A page asking which chain the wallet is on is told the chain the + user is actually on ([#317](https://git.eeqj.de/sneak/AutistMask/issues/317)). + `eth_chainId` and `net_version` answered from `currentNetwork()`, which reads + the module-level `state` singleton that nothing populates at module scope, so + a service worker revived by the page's own message answered out of + `DEFAULT_STATE` and reported mainnet `0x1`/`1` to a user on Sepolia — a dApp + building its interaction for the wrong chain. Both now answer from + `getState()`, the per-call detached storage read the other read handlers use, + rather than from the singleton: these two are reachable by any page on every + provider init, and mutating the shared singleton on that path would detach the + wallet objects an in-flight `backgroundRefresh()` is mutating. The read side + of the background was audited with it: the remaining singleton reads are the + chain switch, the transaction verification path and `backgroundRefresh`, which + each already load, and everything else answers from storage per call through + `getState()`. One stale read is left named but unfixed, outside this issue's + scope: `handleSendTransaction` builds its provider with no network name, so + `getProvider()` falls back to the same unloaded singleton for ethers' static + network hint. - 2026-08-20: The dApp approval screen no longer shows a token transfer it cannot scale as `0.0000` ([#306](https://git.eeqj.de/sneak/AutistMask/issues/306)). `decodeCalldata` diff --git a/src/background/index.js b/src/background/index.js index 58c349e..63abee1 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -3,7 +3,11 @@ // non-sensitive calls to the configured Ethereum JSON-RPC endpoint. const { DEFAULT_RPC_URL } = require("../shared/constants"); -const { SUPPORTED_CHAIN_IDS, networkByChainId } = require("../shared/networks"); +const { + SUPPORTED_CHAIN_IDS, + networkById, + networkByChainId, +} = require("../shared/networks"); const { onChainSwitch } = require("../shared/chainSwitch"); const { state, @@ -663,12 +667,28 @@ async function handleRpc(method, params, origin) { return { result: [] }; } - if (method === "eth_chainId") { - return { result: currentNetwork().chainId }; - } - - if (method === "net_version") { - return { result: currentNetwork().networkVersion }; + // 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. + if (method === "eth_chainId" || method === "net_version") { + const s = await getState(); + const net = networkById(s.networkId); + return { + result: method === "eth_chainId" ? net.chainId : net.networkVersion, + }; } if (method === "wallet_switchEthereumChain") { diff --git a/tests/coldWorkerChainId.test.js b/tests/coldWorkerChainId.test.js new file mode 100644 index 0000000..a95f87f --- /dev/null +++ b/tests/coldWorkerChainId.test.js @@ -0,0 +1,258 @@ +// What eth_chainId and net_version answer on a worker that has not loaded +// state yet. +// +// The MV3 service worker is terminated when idle and revived by the next +// message, and nothing loads state at module scope. Both methods answered from +// currentNetwork(), which reads the module-level `state` singleton, so a +// worker revived by the page's own message answered out of DEFAULT_STATE and +// told a page it was on mainnet while the user was on Sepolia +// (https://git.eeqj.de/sneak/AutistMask/issues/317). +// +// This file therefore uses the REAL state module and never calls loadState() +// itself: the handler has to answer from storage on its own. Same shape as +// tests/coldWorkerChainSwitch.test.js, which covers the write side. + +const { networkById } = require("../src/shared/networks"); + +const ADDRESS = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; + +const CONNECTED_ORIGIN = "https://dapp.example"; +const CONNECTED_HOSTNAME = "dapp.example"; +const UNKNOWN_ORIGIN = "https://stranger.example"; + +const MAINNET = networkById("mainnet"); +const SEPOLIA = networkById("sepolia"); + +const REFRESHED_BALANCE = "1.5"; + +function storedProfile(networkId) { + return { + hasWallet: true, + wallets: [ + { + name: "Wallet 1", + type: "hd", + addresses: [ + { address: ADDRESS, balance: "0", tokenBalances: [] }, + ], + }, + ], + activeAddress: ADDRESS, + networkId, + rpcUrl: networkById(networkId).defaultRpcUrl, + blockscoutUrl: networkById(networkId).defaultBlockscoutUrl, + allowedSites: { [ADDRESS]: [CONNECTED_HOSTNAME] }, + deniedSites: {}, + trackedTokens: [], + }; +} + +async function settle() { + for (let i = 0; i < 50; i++) await Promise.resolve(); +} + +afterEach(() => { + delete global.chrome; +}); + +// Load the background worker with the real state module behind it, over a +// storage stub that keeps what is written. +// +// The stub structured-clones in both directions, as the real +// chrome.storage.local does. A stub that handed back the live stored object +// would alias it into whatever read it, so an in-place mutation of a detached +// copy would appear to have reached storage and this whole class of defect +// would be invisible here. +// +// opts.refreshBalances replaces the balances stub, so a test can hold a +// refresh open across a message. +function loadColdWorker(networkId, opts) { + jest.resetModules(); + + const options = opts || {}; + + jest.doMock("../src/shared/balances", () => ({ + getProvider: () => ({}), + refreshBalances: options.refreshBalances || jest.fn(async () => {}), + })); + 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 store = { autistmask: storedProfile(networkId) }; + + let messageListener = null; + const set = jest.fn(async (items) => { + store.autistmask = structuredClone(items.autistmask); + }); + + global.chrome = { + storage: { + local: { + get: jest.fn(async () => structuredClone(store)), + set, + }, + }, + runtime: { + getURL: (path) => "chrome-extension://autistmask/" + path, + onMessage: { + addListener: (fn) => { + messageListener = fn; + }, + }, + onConnect: { addListener: () => {} }, + lastError: null, + }, + windows: { + getLastFocused: (cb) => cb(null), + create: (options, cb) => cb({ id: 1 }), + 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"); + + async function rpc(method, origin) { + let result = null; + messageListener( + { type: "AUTISTMASK_RPC", method, params: [] }, + { origin: origin || CONNECTED_ORIGIN }, + (r) => { + result = r; + }, + ); + await settle(); + return result; + } + + return { + rpc, + persisted: () => store.autistmask, + storageSet: set, + fireBalanceAlarm: () => alarmHandlers.balance(), + }; +} + +describe("chain identity read by a worker that never loaded state", () => { + test("eth_chainId answers the stored chain, not the default", async () => { + // The first message this worker ever sees. Reading the unloaded + // singleton answers mainnet's 0x1 to a user who is on Sepolia. + const bg = loadColdWorker("sepolia"); + + expect(await bg.rpc("eth_chainId")).toEqual({ + result: SEPOLIA.chainId, + }); + }); + + test("net_version answers the stored chain, not the default", async () => { + const bg = loadColdWorker("sepolia"); + + expect(await bg.rpc("net_version")).toEqual({ + result: SEPOLIA.networkVersion, + }); + }); + + test("answers the stored chain to an origin that never connected", async () => { + // Neither method is gated on a connection, so the stale answer reached + // any page at all; the fixed answer has to as well. + const bg = loadColdWorker("sepolia"); + + expect(await bg.rpc("eth_chainId", UNKNOWN_ORIGIN)).toEqual({ + result: SEPOLIA.chainId, + }); + expect(await bg.rpc("net_version", UNKNOWN_ORIGIN)).toEqual({ + result: SEPOLIA.networkVersion, + }); + }); + + test("answers mainnet for a profile stored on mainnet", async () => { + // The default and the stored value agree here, so this case cannot + // catch the defect; it is what keeps the fix from being a swap. + const bg = loadColdWorker("mainnet"); + + expect(await bg.rpc("eth_chainId")).toEqual({ + result: MAINNET.chainId, + }); + expect(await bg.rpc("net_version")).toEqual({ + result: MAINNET.networkVersion, + }); + }); + + test("persists nothing: these are reads", async () => { + // The load must not turn a read into a write. saveState() persists + // every field of the singleton, and a read path that reached it would + // be the wipe https://git.eeqj.de/sneak/AutistMask/issues/316 fixed. + const bg = loadColdWorker("sepolia"); + + await bg.rpc("eth_chainId"); + await bg.rpc("net_version"); + + expect(bg.storageSet).not.toHaveBeenCalled(); + expect(bg.persisted()).toEqual(storedProfile("sepolia")); + }); + + test("a chain read arriving mid-refresh does not discard the refresh", async () => { + // Any page reaches these two methods, and the injected provider sends + // eth_chainId on every page load, so this overlap is ordinary traffic + // rather than a contrived race. + // + // backgroundRefresh() hands the singleton's wallets to + // refreshBalances(), which mutates those address objects in place once + // the network round trip resolves, and only then saves. Answering the + // page by calling loadState() would replace state.wallets mid-flight, + // so the refreshed balances would land on detached objects and the + // save that follows would persist the pre-refresh values — while still + // stamping lastBalanceRefresh, suppressing the redo. + let releaseRoundTrip; + const roundTrip = new Promise((resolve) => { + releaseRoundTrip = resolve; + }); + let refreshReachedNetwork; + const inFlight = new Promise((resolve) => { + refreshReachedNetwork = resolve; + }); + + const bg = loadColdWorker("sepolia", { + refreshBalances: async (wallets) => { + refreshReachedNetwork(); + await roundTrip; + // In place, on the objects handed in — as balances.js does. + wallets[0].addresses[0].balance = REFRESHED_BALANCE; + }, + }); + + const refresh = bg.fireBalanceAlarm(); + await inFlight; + + expect(await bg.rpc("eth_chainId", UNKNOWN_ORIGIN)).toEqual({ + result: SEPOLIA.chainId, + }); + + releaseRoundTrip(); + await refresh; + + expect(bg.persisted().wallets[0].addresses[0].balance).toBe( + REFRESHED_BALANCE, + ); + }); +}); -- 2.49.1