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

fix: the background worker saves state it never loaded, so a chain switch wipes every wallet in storage #316

New Issue
Closed
opened 2026-08-20 12:20:19 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-20 12:20:19 +02:00
Collaborator
Copy Link

Found during the independent review of #313 (#313 (comment)). Blocker: destroys every wallet in the extension. Pre-existing on next, not introduced by that PR.

src/background/index.js:698 calls onChainSwitch(), which mutates and saveState()s the module-level state singleton in src/shared/state.js. The MV3 service worker never populates that singleton — there is no loadState() at module scope (startBackgroundJobs() at :1108 only schedules alarms), and handleRpc does not load it either. A worker revived by a page's own message therefore holds DEFAULT_STATE, and the save writes those defaults over the user's real storage.

Reproduction

Probe against the real state and chainSwitch modules (only balances, phishingDomains and alarms stubbed), stored profile carrying 1 wallet, rpcUrl: http://127.0.0.1:8545, allowedSites naming the origin. wallet_switchEthereumChain("0xaa36a7") answered {"result":null} and persisted:

rpcUrl: "https://ethereum-sepolia-rpc.publicnode.com"
networkEndpoints: { mainnet: { rpcUrl: "https://ethereum-rpc.publicnode.com", ... } }
wallets: []   hasWallet: false   activeAddress: null
allowedSites: {}   trackedTokens: []   theme: "system"

Consequence

Every wallet is gone from storage — names, addresses and encrypted secrets — along with every site approval, every tracked token and every setting. Funds are unrecoverable unless the user wrote down each recovery phrase. Nothing reports it; the page's switch call answers success.

On current next the trigger has no precondition at all: wallet_switchEthereumChain is ungated (#308), so any page the user visits can do this to a cold worker. Once #313 lands the gate, the precondition becomes a connected site — still reachable, still total.

Same root cause family as #304 (a blind full-blob write from a writer holding stale state), but a distinct path with a distinct fix and a far larger blast radius: #304 loses one wallet, this loses all of them. Fixing #304's read-modify-write would also close this; fixing this alone would not close #304.

Definition of done

  • The background loads state before any path that writes it. await loadState() before onChainSwitch(); precedent exists at src/background/index.js:1286 in the transaction path.
  • Every other background write path is audited for the same defect and named in the PR body — this is the one that was found, not necessarily the only one.
  • Test: drive the background handler against the real state module with no prior load, and assert wallets, allowedSites, trackedTokens and a custom rpcUrl all survive. Both existing test files are blind to this — chainSwitchGate.test.js mocks the state module wholesale including saveState, and networkEndpoints.test.js always calls loadState() first — so a test that loads state first does not count.
  • The test fails against the current head and passes after.
  • make check green.
Found during the independent review of https://git.eeqj.de/sneak/AutistMask/pulls/313 (https://git.eeqj.de/sneak/AutistMask/pulls/313#issuecomment-67446). **Blocker: destroys every wallet in the extension. Pre-existing on `next`, not introduced by that PR.** `src/background/index.js:698` calls `onChainSwitch()`, which mutates and `saveState()`s the module-level `state` singleton in `src/shared/state.js`. **The MV3 service worker never populates that singleton** — there is no `loadState()` at module scope (`startBackgroundJobs()` at `:1108` only schedules alarms), and `handleRpc` does not load it either. A worker revived by a page's own message therefore holds `DEFAULT_STATE`, and the save writes those defaults over the user's real storage. ## Reproduction Probe against the real `state` and `chainSwitch` modules (only `balances`, `phishingDomains` and `alarms` stubbed), stored profile carrying 1 wallet, `rpcUrl: http://127.0.0.1:8545`, `allowedSites` naming the origin. `wallet_switchEthereumChain("0xaa36a7")` answered `{"result":null}` and persisted: ``` rpcUrl: "https://ethereum-sepolia-rpc.publicnode.com" networkEndpoints: { mainnet: { rpcUrl: "https://ethereum-rpc.publicnode.com", ... } } wallets: [] hasWallet: false activeAddress: null allowedSites: {} trackedTokens: [] theme: "system" ``` ## Consequence Every wallet is gone from storage — names, addresses and encrypted secrets — along with every site approval, every tracked token and every setting. Funds are unrecoverable unless the user wrote down each recovery phrase. Nothing reports it; the page's switch call answers success. On current `next` the trigger has **no precondition at all**: `wallet_switchEthereumChain` is ungated (https://git.eeqj.de/sneak/AutistMask/issues/308), so any page the user visits can do this to a cold worker. Once https://git.eeqj.de/sneak/AutistMask/pulls/313 lands the gate, the precondition becomes a connected site — still reachable, still total. Same root cause family as https://git.eeqj.de/sneak/AutistMask/issues/304 (a blind full-blob write from a writer holding stale state), but a distinct path with a distinct fix and a far larger blast radius: #304 loses one wallet, this loses all of them. Fixing #304's read-modify-write would also close this; fixing this alone would not close #304. ## Definition of done - [ ] The background loads state before any path that writes it. `await loadState()` before `onChainSwitch()`; precedent exists at `src/background/index.js:1286` in the transaction path. - [ ] Every other background write path is audited for the same defect and named in the PR body — this is the one that was found, not necessarily the only one. - [ ] Test: drive the background handler against the **real** state module with **no prior load**, and assert `wallets`, `allowedSites`, `trackedTokens` and a custom `rpcUrl` all survive. Both existing test files are blind to this — `chainSwitchGate.test.js` mocks the state module wholesale including `saveState`, and `networkEndpoints.test.js` always calls `loadState()` first — so a test that loads state first does not count. - [ ] The test fails against the current head and passes after. - [ ] `make check` green.
clawbot added this to the 1.0.0 milestone 2026-08-20 12:20:19 +02:00
clawbot commented 2026-08-20 12:42:11 +02:00
Author
Collaborator
Copy Link

Fixed on next by the squash-merge of #313, which carried a closing keyword for #308 only. Closing by hand.

Both DoD items are met and were verified by an independent reviewer:

  • await loadState() runs in the wallet_switchEthereumChain handler after the gate and before anything reads or moves the network. The only branch that skips it is the 4100 refusal, which writes nothing.
  • Every background write path was audited independently: exactly four saveState() sites are reachable from the background (src/background/index.js:595, :609, :1075, and src/shared/chainSwitch.js:69), each now preceded by a load. No fifth exists; the only non-state storageSet() is src/content/index.js:33 writing the separate eip6963Uuid key.
  • tests/coldWorkerChainSwitch.test.js drives the background handler against the real state and chainSwitch modules over write-retaining storage and never calls loadState() itself, asserting wallets, hasWallet, activeAddress, allowedSites, trackedTokens, theme and the custom rpcUrl all survive. Dropping only the added loadState() fails exactly the two cold-worker cases.

The read-side twin of this defect is not fixed and remains open as #317.

Fixed on `next` by the squash-merge of https://git.eeqj.de/sneak/AutistMask/pulls/313, which carried a closing keyword for https://git.eeqj.de/sneak/AutistMask/issues/308 only. Closing by hand. Both DoD items are met and were verified by an independent reviewer: - `await loadState()` runs in the `wallet_switchEthereumChain` handler after the gate and before anything reads or moves the network. The only branch that skips it is the `4100` refusal, which writes nothing. - Every background write path was audited independently: exactly four `saveState()` sites are reachable from the background (`src/background/index.js:595`, `:609`, `:1075`, and `src/shared/chainSwitch.js:69`), each now preceded by a load. No fifth exists; the only non-state `storageSet()` is `src/content/index.js:33` writing the separate `eip6963Uuid` key. - `tests/coldWorkerChainSwitch.test.js` drives the background handler against the **real** `state` and `chainSwitch` modules over write-retaining storage and never calls `loadState()` itself, asserting `wallets`, `hasWallet`, `activeAddress`, `allowedSites`, `trackedTokens`, `theme` and the custom `rpcUrl` all survive. Dropping only the added `loadState()` fails exactly the two cold-worker cases. The read-side twin of this defect is not fixed and remains open as https://git.eeqj.de/sneak/AutistMask/issues/317.
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 Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#316
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?