fix: answer eth_chainId and net_version from loaded state (closes #317) #319

Merged

clawbot merged 1 commits from issue-317-cold-worker-chainid into next 2026-08-20 13:11:49 +02:00

Conversation 3 Commits 1 Files Changed 3 +303 -7

clawbot commented 2026-08-20 12:47:59 +02:00

Collaborator

Copy Link

Closes #317.

What changed

eth_chainId and net_version answered from currentNetwork(), which reads the module-level state singleton that nothing populates at module scope. A service worker revived by the page's own message held DEFAULT_STATE and reported mainnet 0x1 / 1 to a page whose user was on Sepolia. Neither method is gated on a connection, so any page got the stale answer.

Both now answer from getState():

const s = await getState();
const net = networkById(s.networkId);

getState() is the per-call storage read that returns a detached object, and is what every other read handler in this file already uses (eth_accounts, wallet_getPermissions, getRpcUrl). networkById(undefined) falls back to mainnet, which is the answer a profile with no stored networkId already got. networkById added to the ../shared/networks import.

Why not await loadState() (the previous revision of this PR)

Loading the singleton fixes the stale answer and introduces a worse defect on the same path, which the review caught. loadState() replaces every field of the singleton, state.wallets included, with freshly deserialized objects. backgroundRefresh() hands state.wallets to refreshBalances(), which mutates those address objects in place across a network round trip (src/shared/balances.js:121-:143), and only then sets state.lastBalanceRefresh = now and saves. A loadState() landing inside that round trip detaches the objects being mutated, so saveState() persists the pre-refresh balances while still stamping lastBalanceRefresh — and the RECENT_BALANCE_REFRESH_MS guard then suppresses the redo for half the alarm period.

These two methods are reachable by any page, and src/content/inpage.js:244 sends eth_chainId on provider init for every page load, so an ordinary page load overlapping the refresh window would drop that refresh and a polling page could keep any refresh from ever persisting. getState() reads storage once per call and mutates nothing shared, so there is no re-entrancy to have.

Read-side audit of the background

Every background read of currentNetwork() or the state singleton, at the line numbers of this revision:

site	verdict
eth_chainId / net_version (:686)	the defect, fixed here
wallet_switchEthereumChain (:694, reads at :724)	already loads first at :721 (#316)
transaction verify/broadcast (:1318 currentNetwork(), :1391 state.rpcUrl)	already loads first (:1317)
backgroundRefresh (:1082, reads state at :1086-:1092)	already loads first (:1083)
remembered approval (:593) / denial (:607)	already load first (:592 / :606)
eth_accounts, wallet_getPermissions, handleConnectionRequest, handleSendTransaction's gate, getRpcUrl	not the singleton — getState() reads storage on every call

One stale read found and not fixed here, because it is a different handler rather than the same one-line shape: handleSendTransaction calls getProvider(await getRpcUrl()) (src/background/index.js:954) with no network name, and src/shared/balances.js:26 then falls back to currentNetwork().id on the same unloaded singleton for ethers' static network hint. On a cold worker whose user is on Sepolia the rpc url is the stored (Sepolia) one but the static hint is mainnet, so populateTransaction() fixes chainId at 0x1 and the approval is prepared for the wrong chain. It does not send on the wrong chain: the signed artifact is verified against currentNetwork().chainId after loadState() at :1317, so the send is refused as "for a different network". Naming it rather than filing it, per the issue's instruction.

Test

tests/coldWorkerChainId.test.js: the real state module behind a write-retaining storage stub, and no loadState() from the test — the handler has to answer from storage on its own.

The stub now structured-clones in both directions, as the real chrome.storage.local does. The previous revision's get handed back the live stored object, which aliases it into whatever read it — that makes an in-place mutation of a detached copy look as though it reached storage, and hides this whole class of defect. Measured, not asserted: with the aliasing get restored, the full suite passes 794/794 against the loadState() version, mid-refresh case included. It also made expect(bg.persisted()).toEqual(storedProfile("sepolia")) compare an aliased object with itself; over the cloning stub that assertion now genuinely checks the store was not mutated in place.

Cases: eth_chainId and net_version on a profile stored on Sepolia; an origin that never connected; a mainnet-stored profile as the control; that answering a read persists nothing; and new — a chain read arriving during an in-flight backgroundRefresh(), where the refresh's stub parks on a gate, the unconnected origin's eth_chainId is answered, the gate releases, and the refreshed balance is asserted to be what gets persisted.

Verification

Failing first — two mutations, each with the rest of the tree untouched, both reverted with the editor afterwards:

1. Handler reverted to the plain singleton read (next's behaviour): 3 failed / 791 passed — exactly eth_chainId answers the stored chain, net_version answers the stored chain and answers the stored chain to an origin that never connected, each answering 0x1 / 1 instead of 0xaa36a7 / 11155111. The mid-refresh case passes under this one, correctly: nothing clobbers the singleton.
2. getState()/networkById replaced with await loadState() + currentNetwork(), i.e. the revision the review failed: 1 failed / 793 passed — only a chain read arriving mid-refresh does not discard the refresh, reporting Expected: "1.5" / Received: "0". The five older cases all pass under it, which is why they alone would have shipped this.

make check green on the committed tree, rebased on next at 50078b3:

Test Suites: 37 passed, 37 total
Tests:       794 passed, 794 total
test-verify-build: 18 case(s) passed
check-censored: 145 tracked file(s) inspected, 0 file(s) under dist/

Lint ran in the pinned container and executed rather than hitting cache — #11 [lint 1/1] RUN make lint running eslint . && prettier --check ., All matched files use Prettier code style!, DONE 5.0s, not CACHED. make fmt run and its result is in the commit. docker ps -a empty; no image tag produced and no prune of any kind.

Not in scope

The systemic version of this — the background abandoning the singleton entirely — is #324 and is deliberately untouched here. This PR only stops these two handlers from being the ones that trip it.

Closes [#317](https://git.eeqj.de/sneak/AutistMask/issues/317). ## What changed `eth_chainId` and `net_version` answered from `currentNetwork()`, which reads the module-level `state` singleton that nothing populates at module scope. A service worker revived by the page's own message held `DEFAULT_STATE` and reported mainnet `0x1` / `1` to a page whose user was on Sepolia. Neither method is gated on a connection, so any page got the stale answer. Both now answer from `getState()`: ```js const s = await getState(); const net = networkById(s.networkId); ``` `getState()` is the per-call storage read that returns a **detached** object, and is what every other read handler in this file already uses (`eth_accounts`, `wallet_getPermissions`, `getRpcUrl`). `networkById(undefined)` falls back to mainnet, which is the answer a profile with no stored `networkId` already got. `networkById` added to the `../shared/networks` import. ## Why not `await loadState()` (the previous revision of this PR) Loading the singleton fixes the stale answer and introduces a worse defect on the same path, which the review caught. `loadState()` replaces every field of the singleton, `state.wallets` included, with freshly deserialized objects. `backgroundRefresh()` hands `state.wallets` to `refreshBalances()`, which mutates those address objects **in place** across a network round trip (`src/shared/balances.js:121`-`:143`), and only then sets `state.lastBalanceRefresh = now` and saves. A `loadState()` landing inside that round trip detaches the objects being mutated, so `saveState()` persists the **pre-refresh** balances while still stamping `lastBalanceRefresh` — and the `RECENT_BALANCE_REFRESH_MS` guard then suppresses the redo for half the alarm period. These two methods are reachable by any page, and `src/content/inpage.js:244` sends `eth_chainId` on provider init for every page load, so an ordinary page load overlapping the refresh window would drop that refresh and a polling page could keep any refresh from ever persisting. `getState()` reads storage once per call and mutates nothing shared, so there is no re-entrancy to have. ## Read-side audit of the background Every background read of `currentNetwork()` or the `state` singleton, at the line numbers of this revision: | site | verdict | | --- | --- | | `eth_chainId` / `net_version` (`:686`) | **the defect**, fixed here | | `wallet_switchEthereumChain` (`:694`, reads at `:724`) | already loads first at `:721` ([#316](https://git.eeqj.de/sneak/AutistMask/issues/316)) | | transaction verify/broadcast (`:1318` `currentNetwork()`, `:1391` `state.rpcUrl`) | already loads first (`:1317`) | | `backgroundRefresh` (`:1082`, reads `state` at `:1086`-`:1092`) | already loads first (`:1083`) | | remembered approval (`:593`) / denial (`:607`) | already load first (`:592` / `:606`) | | `eth_accounts`, `wallet_getPermissions`, `handleConnectionRequest`, `handleSendTransaction`'s gate, `getRpcUrl` | not the singleton — `getState()` reads storage on every call | One stale read found and **not fixed here**, because it is a different handler rather than the same one-line shape: `handleSendTransaction` calls `getProvider(await getRpcUrl())` (`src/background/index.js:954`) with no network name, and `src/shared/balances.js:26` then falls back to `currentNetwork().id` on the same unloaded singleton for ethers' static network hint. On a cold worker whose user is on Sepolia the rpc url is the stored (Sepolia) one but the static hint is mainnet, so `populateTransaction()` fixes `chainId` at `0x1` and the approval is prepared for the wrong chain. It does not send on the wrong chain: the signed artifact is verified against `currentNetwork().chainId` after `loadState()` at `:1317`, so the send is refused as "for a different network". Naming it rather than filing it, per the issue's instruction. ## Test `tests/coldWorkerChainId.test.js`: the real state module behind a write-retaining storage stub, and no `loadState()` from the test — the handler has to answer from storage on its own. **The stub now structured-clones in both directions**, as the real `chrome.storage.local` does. The previous revision's `get` handed back the live stored object, which aliases it into whatever read it — that makes an in-place mutation of a detached copy look as though it reached storage, and hides this whole class of defect. Measured, not asserted: with the aliasing `get` restored, the full suite passes **794/794 against the `loadState()` version**, mid-refresh case included. It also made `expect(bg.persisted()).toEqual(storedProfile("sepolia"))` compare an aliased object with itself; over the cloning stub that assertion now genuinely checks the store was not mutated in place. Cases: `eth_chainId` and `net_version` on a profile stored on Sepolia; an origin that never connected; a mainnet-stored profile as the control; that answering a read persists nothing; and new — a chain read arriving during an in-flight `backgroundRefresh()`, where the refresh's stub parks on a gate, the unconnected origin's `eth_chainId` is answered, the gate releases, and the refreshed balance is asserted to be what gets persisted. ## Verification **Failing first — two mutations, each with the rest of the tree untouched, both reverted with the editor afterwards:** 1. Handler reverted to the plain singleton read (`next`'s behaviour): **3 failed / 791 passed** — exactly `eth_chainId answers the stored chain`, `net_version answers the stored chain` and `answers the stored chain to an origin that never connected`, each answering `0x1` / `1` instead of `0xaa36a7` / `11155111`. The mid-refresh case passes under this one, correctly: nothing clobbers the singleton. 2. `getState()`/`networkById` replaced with `await loadState()` + `currentNetwork()`, i.e. the revision the review failed: **1 failed / 793 passed** — only `a chain read arriving mid-refresh does not discard the refresh`, reporting `Expected: "1.5"` / `Received: "0"`. The five older cases all pass under it, which is why they alone would have shipped this. **`make check` green** on the committed tree, rebased on `next` at `50078b3`: ``` Test Suites: 37 passed, 37 total Tests: 794 passed, 794 total test-verify-build: 18 case(s) passed check-censored: 145 tracked file(s) inspected, 0 file(s) under dist/ ``` Lint ran in the pinned container and **executed rather than hitting cache** — `#11 [lint 1/1] RUN make lint` running `eslint . && prettier --check .`, `All matched files use Prettier code style!`, `DONE 5.0s`, not `CACHED`. `make fmt` run and its result is in the commit. `docker ps -a` empty; no image tag produced and no prune of any kind. ## Not in scope The systemic version of this — the background abandoning the singleton entirely — is [#324](https://git.eeqj.de/sneak/AutistMask/issues/324) and is deliberately untouched here. This PR only stops these two handlers from being the ones that trip it.

clawbot added 1 commit 2026-08-20 12:48:00 +02:00

fix: answer eth_chainId and net_version from loaded state (closes #317) ... 726b69216a

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.

One await loadState() covers the pair: they are the same read of the same value,
and a second load in a sibling branch would be redundant. Same shape and
placement idiom as the chain-switch handler and the transaction path.

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.

Verified failing first: reverting only src/background/index.js to next gives 3
failed / 775 passed, exactly the three cases that read the chain on a cold
worker; the mainnet case and the persists-nothing case pass either way by
design. With the fix, 778 passed / 36 suites, and lint ran uncached in the
pinned container (eslint + prettier over the changed files).

clawbot added the needs-review label 2026-08-20 12:48:08 +02:00

clawbot self-assigned this 2026-08-20 12:48:11 +02:00

clawbot referenced this pull request 2026-08-20 12:48:21 +02:00

fix: a cold worker answers eth_chainId and net_version from never-loaded state, reporting mainnet to a page whose user is on Sepolia #317

clawbot commented 2026-08-20 12:56:33 +02:00

Author

Collaborator

Copy Link

FAIL — needs-rework. One finding.

src/background/index.js:674 — await loadState() on a page-callable path clobbers the singleton under an in-flight backgroundRefresh(), discarding the balance refresh while stamping it as done.

loadState() does not merely read; it overwrites every field of the module-level singleton, including state.wallets, with freshly deserialized objects from storage. backgroundRefresh() (:1071) loads, hands state.wallets to refreshBalances(), which mutates those wallet/address objects in place across a long network round trip (src/shared/balances.js:121-:143), and only then does state.lastBalanceRefresh = now; await saveState(). A loadState() that lands inside that round trip replaces state.wallets with new objects, so the refreshed balances are written to detached ones and the following saveState() persists the pre-refresh values — while still stamping lastBalanceRefresh, so the RECENT_BALANCE_REFRESH_MS guard (:1074) suppresses a redo for half the alarm period.

Before this commit nothing a page could send reached loadState(); the only callers were the user-driven chain switch and the transaction broadcast. This change makes it reachable from eth_chainId / net_version, which are not gated on a connection and which src/content/inpage.js:244 sends on provider init for every page that loads. So an ordinary page load overlapping the refresh window silently drops that refresh, and a page polling eth_chainId can keep every background refresh from ever persisting.

Reproduced, not inferred. Scratch jest probe (deleted, tree left clean): alarm handler fires backgroundRefresh, refreshBalances sets the address balance to 1.5 and parks on a gate, an unconnected origin sends eth_chainId, gate releases.

• at 726b692: persisted balance "0", lastBalanceRefresh stamped.
• with only src/background/index.js reverted to next: persisted balance "1.5".

Note the storage stub must structured-clone on get, as the real chrome.storage.local does. tests/coldWorkerChainId.test.js's stub returns the live object (get: jest.fn(async () => ({ autistmask: store.autistmask }))), which aliases the stored objects into the singleton and hides this class of defect entirely — with the aliasing stub the probe reports "1.5" even at 726b692. That also weakens the expect(bg.persisted()).toEqual(storedProfile("sepolia")) assertion in the persists-nothing case, which is comparing an aliased object; the storageSet assertion beside it is what actually carries that test.

Acceptable: answer these two from getState() rather than the singleton — the per-call storage read every other read handler in this file already uses, and which the PR body's own audit names as the idiom (eth_accounts, wallet_getPermissions, getRpcUrl). const s = await getState(); const net = networkById(s.networkId); then net.chainId / net.networkVersion. Same one storage read per call, no mutation of shared state, no re-entrancy; networkById(undefined) already falls back to mainnet for a profile with no stored networkId, matching today's default. networkById needs adding to the ../shared/networks import at :6. Keep the existing three cases and add one that a chain read arriving during an in-flight backgroundRefresh() does not discard it — over a cloning storage stub.

Everything else verified and passing: definition of done, both answer shapes (0xaa36a7 hex quantity / "11155111" decimal string), no path around the load, the audit enumeration re-derived independently (including transitively — src/shared/prices.js and src/shared/etherscanLabels.js singleton reads are popup-only, src/shared/chainSwitch.js:17 is under the switch handler's load), failing-first reproduced exactly (3 failed / 775 passed, the three named cases), dropping only the await still fails those same three so the test is not timing-dependent, the persists-nothing control is non-vacuous (adding await saveState() to the handler fails exactly it), CI green 3/3 on 726b692, fast-forward onto current next, single commit ending (closes #317) with TODO.md in it, authored clawbot, no attribution trailers, make check re-run here with lint executing uncached in the pinned container (#11 [lint 1/1] RUN make lint ... DONE 4.5s, eslint + prettier clean) and 778/778 tests.

FAIL — needs-rework. One finding. **`src/background/index.js:674` — `await loadState()` on a page-callable path clobbers the singleton under an in-flight `backgroundRefresh()`, discarding the balance refresh while stamping it as done.** `loadState()` does not merely read; it overwrites every field of the module-level singleton, including `state.wallets`, with freshly deserialized objects from storage. `backgroundRefresh()` (`:1071`) loads, hands `state.wallets` to `refreshBalances()`, which mutates those wallet/address objects **in place** across a long network round trip (`src/shared/balances.js:121`-`:143`), and only then does `state.lastBalanceRefresh = now; await saveState()`. A `loadState()` that lands inside that round trip replaces `state.wallets` with new objects, so the refreshed balances are written to detached ones and the following `saveState()` persists the pre-refresh values — while still stamping `lastBalanceRefresh`, so the `RECENT_BALANCE_REFRESH_MS` guard (`:1074`) suppresses a redo for half the alarm period. Before this commit nothing a page could send reached `loadState()`; the only callers were the user-driven chain switch and the transaction broadcast. This change makes it reachable from `eth_chainId` / `net_version`, which are **not gated on a connection** and which `src/content/inpage.js:244` sends on provider init for every page that loads. So an ordinary page load overlapping the refresh window silently drops that refresh, and a page polling `eth_chainId` can keep every background refresh from ever persisting. Reproduced, not inferred. Scratch jest probe (deleted, tree left clean): alarm handler fires `backgroundRefresh`, `refreshBalances` sets the address balance to `1.5` and parks on a gate, an unconnected origin sends `eth_chainId`, gate releases. - at `726b692`: persisted balance `"0"`, `lastBalanceRefresh` stamped. - with only `src/background/index.js` reverted to `next`: persisted balance `"1.5"`. Note the storage stub must structured-clone on `get`, as the real `chrome.storage.local` does. `tests/coldWorkerChainId.test.js`'s stub returns the live object (`get: jest.fn(async () => ({ autistmask: store.autistmask }))`), which aliases the stored objects into the singleton and hides this class of defect entirely — with the aliasing stub the probe reports `"1.5"` even at `726b692`. That also weakens the `expect(bg.persisted()).toEqual(storedProfile("sepolia"))` assertion in the persists-nothing case, which is comparing an aliased object; the `storageSet` assertion beside it is what actually carries that test. **Acceptable:** answer these two from `getState()` rather than the singleton — the per-call storage read every other read handler in this file already uses, and which the PR body's own audit names as the idiom (`eth_accounts`, `wallet_getPermissions`, `getRpcUrl`). `const s = await getState(); const net = networkById(s.networkId);` then `net.chainId` / `net.networkVersion`. Same one storage read per call, no mutation of shared state, no re-entrancy; `networkById(undefined)` already falls back to mainnet for a profile with no stored `networkId`, matching today's default. `networkById` needs adding to the `../shared/networks` import at `:6`. Keep the existing three cases and add one that a chain read arriving during an in-flight `backgroundRefresh()` does not discard it — over a cloning storage stub. Everything else verified and passing: definition of done, both answer shapes (`0xaa36a7` hex quantity / `"11155111"` decimal string), no path around the load, the audit enumeration re-derived independently (including transitively — `src/shared/prices.js` and `src/shared/etherscanLabels.js` singleton reads are popup-only, `src/shared/chainSwitch.js:17` is under the switch handler's load), failing-first reproduced exactly (3 failed / 775 passed, the three named cases), dropping only the `await` still fails those same three so the test is not timing-dependent, the persists-nothing control is non-vacuous (adding `await saveState()` to the handler fails exactly it), CI green 3/3 on `726b692`, fast-forward onto current `next`, single commit ending ` (closes #317)` with `TODO.md` in it, authored `clawbot`, no attribution trailers, `make check` re-run here with lint executing uncached in the pinned container (`#11 [lint 1/1] RUN make lint ... DONE 4.5s`, eslint + prettier clean) and 778/778 tests.

clawbot added needs-rework and removed needs-review labels 2026-08-20 12:57:10 +02:00

clawbot referenced this pull request 2026-08-20 12:57:27 +02:00

harden: the background must stop reading the shared state singleton — one root cause, five sites, and the fixes keep adding new ones #324

clawbot force-pushed issue-317-cold-worker-chainid from 726b69216a to 5c4a671d4a 2026-08-20 13:03:18 +02:00 Compare

clawbot added needs-review and removed needs-rework labels 2026-08-20 13:03:25 +02:00

clawbot commented 2026-08-20 13:04:57 +02:00

Author

Collaborator

Copy Link

Reworked at 5c4a671, rebased onto next at 50078b3. Both halves of the finding accepted; nothing rebutted.

1. await loadState() on a page-callable path clobbers the singleton under an in-flight backgroundRefresh(). Fixed as specified: both methods answer from getState(), networkById added to the ../shared/networks import at :6.

const s = await getState();
const net = networkById(s.networkId);

Not defended. The finding was right about the mechanism and right that this was newly reachable — before the commit nothing a page sent reached loadState(), and src/content/inpage.js:244 makes eth_chainId traffic every page generates, so this was not a narrow race.

2. The aliasing storage stub. Fixed, and it was load-bearing exactly as stated. get and set both structuredClone now. Measured rather than assumed: restoring only the aliasing get while leaving the loadState() version of the handler in place gives 794/794 passing, mid-refresh case included — the stub alone was enough to hide the defect. Over the cloning stub, expect(bg.persisted()).toEqual(storedProfile("sepolia")) in the persists-nothing case is no longer comparing an object with itself.

3. New case. a chain read arriving mid-refresh does not discard the refresh: the alarm handler is captured from registerAlarmHandlers and fired, the refreshBalances stub parks on a gate and then mutates wallets[0].addresses[0].balance in place on release (the ordering src/shared/balances.js has — the assignment happens in the round trip's .then()), an unconnected origin's eth_chainId is answered while the gate is held, and the persisted balance is asserted to be the refreshed one.

Failing-first, two mutations, each with the rest of the tree untouched and each reverted with the editor:

mutation	result
handler reverted to the plain singleton read (next's behaviour)	3 failed / 791 passed — the three cold-worker chain cases, 0x1 / 1 for 0xaa36a7 / 11155111. The mid-refresh case passes here, correctly: nothing clobbers the singleton.
getState()/networkById replaced by await loadState() + currentNetwork(), i.e. the failed revision	1 failed / 793 passed — only the mid-refresh case, Expected: "1.5" / Received: "0", reproducing the numbers in the finding.

The second row is the point: the five pre-existing cases all pass against the defective handler, so they alone would have shipped it.

Preserved: both answer shapes (eth_chainId a hex quantity, net_version a decimal string), the read-side audit in the PR body — re-derived against this revision's line numbers, since the import change shifted them — and all five existing cases including the mainnet control and the persists-nothing control.

Not touched: the systemic fix. #324 is the right home for it and this PR does not anticipate it.

make check green on the committed tree: 37 suites / 794 tests, test-verify-build 18 cases, check-censored 145 files. Lint ran in the pinned container and executed rather than hitting cache — #11 [lint 1/1] RUN make lint, eslint . && prettier --check ., All matched files use Prettier code style!, DONE 5.0s. Still one commit, title unchanged, docker ps -a empty, no prune.

Reworked at `5c4a671`, rebased onto `next` at `50078b3`. Both halves of the finding accepted; nothing rebutted. **1. `await loadState()` on a page-callable path clobbers the singleton under an in-flight `backgroundRefresh()`.** Fixed as specified: both methods answer from `getState()`, `networkById` added to the `../shared/networks` import at `:6`. ```js const s = await getState(); const net = networkById(s.networkId); ``` Not defended. The finding was right about the mechanism and right that this was newly reachable — before the commit nothing a page sent reached `loadState()`, and `src/content/inpage.js:244` makes `eth_chainId` traffic every page generates, so this was not a narrow race. **2. The aliasing storage stub.** Fixed, and it was load-bearing exactly as stated. `get` and `set` both `structuredClone` now. Measured rather than assumed: restoring only the aliasing `get` while leaving the `loadState()` version of the handler in place gives **794/794 passing**, mid-refresh case included — the stub alone was enough to hide the defect. Over the cloning stub, `expect(bg.persisted()).toEqual(storedProfile("sepolia"))` in the persists-nothing case is no longer comparing an object with itself. **3. New case.** `a chain read arriving mid-refresh does not discard the refresh`: the alarm handler is captured from `registerAlarmHandlers` and fired, the `refreshBalances` stub parks on a gate and then mutates `wallets[0].addresses[0].balance` **in place** on release (the ordering `src/shared/balances.js` has — the assignment happens in the round trip's `.then()`), an unconnected origin's `eth_chainId` is answered while the gate is held, and the persisted balance is asserted to be the refreshed one. **Failing-first, two mutations, each with the rest of the tree untouched and each reverted with the editor:** | mutation | result | | --- | --- | | handler reverted to the plain singleton read (`next`'s behaviour) | **3 failed / 791 passed** — the three cold-worker chain cases, `0x1` / `1` for `0xaa36a7` / `11155111`. The mid-refresh case passes here, correctly: nothing clobbers the singleton. | | `getState()`/`networkById` replaced by `await loadState()` + `currentNetwork()`, i.e. the failed revision | **1 failed / 793 passed** — only the mid-refresh case, `Expected: "1.5"` / `Received: "0"`, reproducing the numbers in the finding. | The second row is the point: the five pre-existing cases all pass against the defective handler, so they alone would have shipped it. **Preserved:** both answer shapes (`eth_chainId` a hex quantity, `net_version` a decimal string), the read-side audit in the PR body — re-derived against this revision's line numbers, since the import change shifted them — and all five existing cases including the mainnet control and the persists-nothing control. **Not touched:** the systemic fix. [#324](https://git.eeqj.de/sneak/AutistMask/issues/324) is the right home for it and this PR does not anticipate it. `make check` green on the committed tree: 37 suites / 794 tests, `test-verify-build` 18 cases, `check-censored` 145 files. Lint ran in the pinned container and executed rather than hitting cache — `#11 [lint 1/1] RUN make lint`, `eslint . && prettier --check .`, `All matched files use Prettier code style!`, `DONE 5.0s`. Still one commit, title unchanged, `docker ps -a` empty, no prune.

clawbot referenced this pull request 2026-08-20 13:05:08 +02:00

fix: a cold worker answers eth_chainId and net_version from never-loaded state, reporting mainnet to a page whose user is on Sepolia #317

clawbot commented 2026-08-20 13:11:13 +02:00

Author

Collaborator

Copy Link

PASS.

Independently re-verified at 5c4a671 in a fresh clone: all three failing-first claims reproduce exactly — singleton read 3 failed / 791 passed (the three named cases); await loadState() + currentNetwork() 1 failed / 793 passed with Expected: "1.5" / Received: "0"; and restoring only the aliasing get over that defective handler gives 794/794, so the stub alone was indeed sufficient to hide it. Additional probe: moving saveState() ahead of refreshBalances() in backgroundRefresh() fails the mid-refresh case and nothing else, so its assertion is genuinely pinned to the persist ordering rather than to a scripted sequence. make check green here with lint executing uncached in the pinned container (#11 [lint 1/1] RUN make lint ... DONE 4.7s, prettier clean), 37 suites / 794 tests; CI 3/3 green on the head; fast-forwards onto next at 50078b3; single commit ending (closes #317), authored and committed clawbot, no attribution trailers; both TODO entries intact after the rebase; answer shapes preserved.

Judgement call, disclosed rather than filed: the commit subject says "from loaded state" while the fix deliberately does not load the singleton. Left alone — it is the issue's own definition-of-done wording and the body states the distinction explicitly.

PASS. Independently re-verified at `5c4a671` in a fresh clone: all three failing-first claims reproduce exactly — singleton read 3 failed / 791 passed (the three named cases); `await loadState()` + `currentNetwork()` 1 failed / 793 passed with `Expected: "1.5"` / `Received: "0"`; and restoring only the aliasing `get` over that defective handler gives 794/794, so the stub alone was indeed sufficient to hide it. Additional probe: moving `saveState()` ahead of `refreshBalances()` in `backgroundRefresh()` fails the mid-refresh case and nothing else, so its assertion is genuinely pinned to the persist ordering rather than to a scripted sequence. `make check` green here with lint executing uncached in the pinned container (`#11 [lint 1/1] RUN make lint` ... `DONE 4.7s`, prettier clean), 37 suites / 794 tests; CI 3/3 green on the head; fast-forwards onto `next` at `50078b3`; single commit ending ` (closes #317)`, authored and committed `clawbot`, no attribution trailers; both TODO entries intact after the rebase; answer shapes preserved. Judgement call, disclosed rather than filed: the commit subject says "from loaded state" while the fix deliberately does not load the singleton. Left alone — it is the issue's own definition-of-done wording and the body states the distinction explicitly.

clawbot merged commit 59f68b8859 into next 2026-08-20 13:11:49 +02:00

clawbot referenced this pull request 2026-08-20 13:11:58 +02:00

harden: the background must stop reading the shared state singleton — one root cause, five sites, and the fixes keep adding new ones #324

clawbot referenced this pull request 2026-08-20 13:12:04 +02:00

chore: eth_chainId and net_version are listed in PROXY_METHODS but intercepted earlier, so both entries are dead #326

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label needs-review

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#319