sneak/AutistMask
1
0
Fork 0
Code Issues 66 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

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

New Issue
Closed
opened 2026-08-20 12:57:27 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-20 12:57:27 +02:00
Collaborator
Copy Link
Copy Source

Root-cause issue. Five defects now trace to one thing: src/background/index.js reads and writes the module-level state singleton in src/shared/state.js, which the MV3 service worker never populates and which nothing makes fail loudly when read unpopulated. It silently serves DEFAULT_STATE.

The five, in the order they surfaced:

  1. #316 — the chain switch persisted DEFAULT_STATE over the profile, destroying every wallet. Fixed.
  2. #317 — eth_chainId/net_version reported mainnet to a page whose user was on Sepolia. In flight.
  3. #320 — getProvider() gets a mainnet static hint on a cold worker, so every non-mainnet dApp send is prepared wrong and then refused by the wallet's own verifier.
  4. The tx broadcast path reads state.rpcUrl at src/background/index.js:1380, several awaits after its loadState() at :1306, so a concurrent committed chain switch can move the endpoint under it. Pre-existing, never filed separately.
  5. Found in review of #319: calling loadState() on a page-callable path detaches the objects backgroundRefresh() is mutating in place across its network round trip, so saveState() persists pre-refresh balances and still stamps lastBalanceRefresh, suppressing a redo. src/content/inpage.js:244 sends eth_chainId on every page load, so any page can trigger it.

Item 5 is the reason this issue exists rather than a sixth point fix. The remedy applied to the first four — add a loadState() before the access — is what created it. Each singleton fix is a new opportunity to mutate shared state at a moment some other in-flight handler depended on it not moving. Fixing site six the same way will produce site seven.

What is actually wrong

Two things, and only the second is hard:

  • The background has a correct per-call idiom already — getState(), a fresh storage read returning a detached object, used by eth_accounts, wallet_getPermissions, handleConnectionRequest and getRpcUrl. Handlers that use it have never produced a defect here.
  • The singleton remains reachable from background code, and an unpopulated read returns plausible defaults instead of throwing. Nothing structurally prevents the next handler from reaching for it.

Definition of done

  • No background code path reads or writes the module-level state singleton. Every handler uses a per-call getState() read, and every write is a read-modify-write that does not publish a shared object other in-flight work holds.
  • The prohibition is enforced mechanically, not by review: an ESLint rule (no-restricted-imports on the singleton export from background files, or equivalent) that fails make check. A convention nobody can violate beats a convention everyone remembers.
  • backgroundRefresh() no longer mutates address objects in place across a network round trip while another handler may replace them.
  • Reading unpopulated state fails loudly rather than returning DEFAULT_STATE — at minimum in the background context.
  • Test stubs for chrome.storage.local structured-clone on get, as the real API does. The aliasing stub in tests/coldWorkerChainId.test.js hides this entire defect class and made a real defect invisible; audit every storage stub in tests/ for the same flaw.
  • Items 4 and 5 above are demonstrably fixed, each with a test that fails first.
  • make check green.

Note for sequencing: #304 and #311 touch the same persistence layer. Whoever takes this should take those or coordinate closely; three units rewriting state handling in parallel will conflict badly.

Root-cause issue. Five defects now trace to one thing: `src/background/index.js` reads and writes the module-level `state` singleton in `src/shared/state.js`, which the MV3 service worker never populates and which nothing makes fail loudly when read unpopulated. It silently serves `DEFAULT_STATE`. The five, in the order they surfaced: 1. https://git.eeqj.de/sneak/AutistMask/issues/316 — the chain switch persisted `DEFAULT_STATE` over the profile, destroying every wallet. Fixed. 2. https://git.eeqj.de/sneak/AutistMask/issues/317 — `eth_chainId`/`net_version` reported mainnet to a page whose user was on Sepolia. In flight. 3. https://git.eeqj.de/sneak/AutistMask/issues/320 — `getProvider()` gets a mainnet static hint on a cold worker, so every non-mainnet dApp send is prepared wrong and then refused by the wallet's own verifier. 4. The tx broadcast path reads `state.rpcUrl` at `src/background/index.js:1380`, several awaits after its `loadState()` at `:1306`, so a concurrent committed chain switch can move the endpoint under it. Pre-existing, never filed separately. 5. Found in review of https://git.eeqj.de/sneak/AutistMask/pulls/319: calling `loadState()` on a page-callable path **detaches the objects `backgroundRefresh()` is mutating in place** across its network round trip, so `saveState()` persists pre-refresh balances and still stamps `lastBalanceRefresh`, suppressing a redo. `src/content/inpage.js:244` sends `eth_chainId` on every page load, so any page can trigger it. Item 5 is the reason this issue exists rather than a sixth point fix. **The remedy applied to the first four — add a `loadState()` before the access — is what created it.** Each singleton fix is a new opportunity to mutate shared state at a moment some other in-flight handler depended on it not moving. Fixing site six the same way will produce site seven. ## What is actually wrong Two things, and only the second is hard: - The background has a correct per-call idiom already — `getState()`, a fresh storage read returning a detached object, used by `eth_accounts`, `wallet_getPermissions`, `handleConnectionRequest` and `getRpcUrl`. Handlers that use it have never produced a defect here. - The singleton remains *reachable* from background code, and an unpopulated read returns plausible defaults instead of throwing. Nothing structurally prevents the next handler from reaching for it. ## Definition of done - [ ] No background code path reads or writes the module-level `state` singleton. Every handler uses a per-call `getState()` read, and every write is a read-modify-write that does not publish a shared object other in-flight work holds. - [ ] The prohibition is enforced mechanically, not by review: an ESLint rule (`no-restricted-imports` on the singleton export from background files, or equivalent) that fails `make check`. A convention nobody can violate beats a convention everyone remembers. - [ ] `backgroundRefresh()` no longer mutates address objects in place across a network round trip while another handler may replace them. - [ ] Reading unpopulated state fails loudly rather than returning `DEFAULT_STATE` — at minimum in the background context. - [ ] Test stubs for `chrome.storage.local` **structured-clone on `get`**, as the real API does. The aliasing stub in `tests/coldWorkerChainId.test.js` hides this entire defect class and made a real defect invisible; audit every storage stub in `tests/` for the same flaw. - [ ] Items 4 and 5 above are demonstrably fixed, each with a test that fails first. - [ ] `make check` green. Note for sequencing: https://git.eeqj.de/sneak/AutistMask/issues/304 and https://git.eeqj.de/sneak/AutistMask/issues/311 touch the same persistence layer. Whoever takes this should take those or coordinate closely; three units rewriting state handling in parallel will conflict badly.
clawbot added this to the 1.0.0 milestone 2026-08-20 12:57:27 +02:00
clawbot commented 2026-08-20 13:11:58 +02:00
Author
Collaborator
Copy Link
Copy Source

The stub audit this issue asks for has been done. Eight test files carry the aliasing flaw, so the DoD item is scoped, not open-ended.

Both get and set alias:

  • tests/txStatus.test.js
  • tests/alarms.test.js
  • tests/settingsUtcTimestamps.test.js
  • tests/coldWorkerChainSwitch.test.js
  • tests/networkEndpoints.test.js

Aliasing get with a no-op set:

  • tests/state.test.js
  • tests/chainSwitchGate.test.js
  • tests/backgroundApproval.test.js

Two are worth singling out. tests/coldWorkerChainSwitch.test.js is the direct sibling this issue's evidence came from — the stub fixed in #319 was cloned from it, and the original still aliases. And tests/networkEndpoints.test.js:124 feeds one module's live in-memory object back in as the next module's "persisted bytes".

Why this is load-bearing rather than tidiness: loadState() assigns nested references straight out of the get result (state.networkEndpoints = saved.networkEndpoints). Over an aliasing stub, a test can assert "the endpoint was persisted" and pass on a build that never calls saveState() at all. Measured, not theorised: with the aliasing get restored over the defective handler #319 fixed, the suite passes 794/794.

So these eight files may be asserting less than they appear to, and re-running them proves nothing until the stubs clone. Fix the stubs first, then see which assertions still hold.

The stub audit this issue asks for has been done. **Eight test files carry the aliasing flaw**, so the DoD item is scoped, not open-ended. Both `get` and `set` alias: - `tests/txStatus.test.js` - `tests/alarms.test.js` - `tests/settingsUtcTimestamps.test.js` - `tests/coldWorkerChainSwitch.test.js` - `tests/networkEndpoints.test.js` Aliasing `get` with a no-op `set`: - `tests/state.test.js` - `tests/chainSwitchGate.test.js` - `tests/backgroundApproval.test.js` Two are worth singling out. `tests/coldWorkerChainSwitch.test.js` is the direct sibling this issue's evidence came from — the stub fixed in https://git.eeqj.de/sneak/AutistMask/pulls/319 was cloned from it, and the original still aliases. And `tests/networkEndpoints.test.js:124` feeds one module's live in-memory object back in as the next module's "persisted bytes". Why this is load-bearing rather than tidiness: `loadState()` assigns nested references straight out of the get result (`state.networkEndpoints = saved.networkEndpoints`). Over an aliasing stub, a test can assert "the endpoint was persisted" and **pass on a build that never calls `saveState()` at all**. Measured, not theorised: with the aliasing `get` restored over the defective handler https://git.eeqj.de/sneak/AutistMask/pulls/319 fixed, the suite passes 794/794. So these eight files may be asserting less than they appear to, and re-running them proves nothing until the stubs clone. Fix the stubs first, then see which assertions still hold.
clawbot commented 2026-08-23 15:11:16 +02:00
Author
Collaborator
Copy Link
Copy Source

Scoping for the implementer: this unit also closes #320. That issue's cause is src/shared/balances.js:26 falling back to currentNetwork().id — the same unpopulated singleton — so fixing it as a sixth point fix is exactly what this issue forbids. Its DoD (explicit network hint at every background-reachable getProvider() call site, plus a cold-worker Sepolia approval test) is folded in and must be met here.

Sequenced ahead of #311, which rewrites the same persistence layer; 311 is dispatched only after this lands on next.

Scoping for the implementer: this unit also closes https://git.eeqj.de/sneak/AutistMask/issues/320. That issue's cause is `src/shared/balances.js:26` falling back to `currentNetwork().id` — the same unpopulated singleton — so fixing it as a sixth point fix is exactly what this issue forbids. Its DoD (explicit network hint at every background-reachable `getProvider()` call site, plus a cold-worker Sepolia approval test) is folded in and must be met here. Sequenced ahead of https://git.eeqj.de/sneak/AutistMask/issues/311, which rewrites the same persistence layer; 311 is dispatched only after this lands on `next`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#324