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

fix: a second extension page silently deletes a wallet — saveState() is a last-writer-wins full-blob overwrite #304

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

Found by the pre-1.0 deployability audit (#303). Blocker: destroys a wallet with no attacker and no unusual input.

src/shared/state.js:83-115 — saveState() writes the entire state blob. src/popup/views/helpers.js:80 — every showView() saves. src/popup/index.js:136 — loadState() runs exactly once per page. There is no chrome.storage.onChanged listener anywhere in src/, so each extension page holds a private in-memory state and every write is a full-blob overwrite of whatever another page wrote.

Reproduction

Driven through the real UI in the Chrome harness; the approval window was opened by the background, not by the test:

[P4] 1. one wallet: 0x96f1AD2Afb9582E7A66b719a132435A3FdDEa93e
[P4] 2. approval window opened by the background: true
[P4] 3. after adding wallet 2 -> ["Wallet 1","Wallet 2"]
[P4]    wallet 2 address: 0x535946AF49802605a9f33dc69eb77cEA6A8E6D53
[P4] 4. after answering the approval -> ["Wallet 1"]
[P4]    wallet 2's encrypted secret still in storage? false
[P4]    broadcasts: 1 | dApp settled: {"settled":"resolved","result":"0x4ff2ac15..."}
[P4] 5. fresh popup wallet list: "Wallet 1+Address 1 0x96f1AD2A... [info] ETH 2.0000"

Also reproduced with two plain popup pages: RESULT A: 2 wallets before B navigated, 1 after. *** WALLET LOST ***.

Answering an approval was the trigger here, but any navigation in the stale page does it, because showView() saves unconditionally. The background is a third writer: backgroundRefresh() at src/background/index.js:1034-1048 does loadState() → seconds of network I/O → saveState(), so the same clobber happens with no second window open at all.

Consequence

The user loses the entire wallet — name, addresses, and the encrypted secret. Funds at those addresses are unrecoverable unless the recovery phrase was written down during the seconds it was on screen. Nothing reports it, and the dApp transaction succeeds normally so nothing looks wrong.

Preconditions: one extension page open while another writes. That is a dApp approval plus the toolbar popup, or the background's 60-second balance alarm overlapping any user action.

Definition of done

  • No writer overwrites another's state. Either storage.onChanged re-hydrates every page, or saveState() becomes a read-modify-write with per-field merge, or wallets moves to its own storage key written only by the paths that own it.
  • The background refresh path cannot clobber: it must not write back fields it did not change.
  • Test: add a wallet in one extension page, then force a save from a second page that was loaded before it; assert both wallets are in storage afterwards.
  • Test: the sequence above — approval window open, add a wallet in the popup, confirm the approval — leaves two wallets standing.
  • make check green.
Found by the pre-1.0 deployability audit (https://git.eeqj.de/sneak/AutistMask/issues/303). **Blocker: destroys a wallet with no attacker and no unusual input.** `src/shared/state.js:83-115` — `saveState()` writes the entire state blob. `src/popup/views/helpers.js:80` — every `showView()` saves. `src/popup/index.js:136` — `loadState()` runs exactly once per page. There is **no `chrome.storage.onChanged` listener anywhere in `src/`**, so each extension page holds a private in-memory `state` and every write is a full-blob overwrite of whatever another page wrote. ## Reproduction Driven through the real UI in the Chrome harness; the approval window was opened by the background, not by the test: ``` [P4] 1. one wallet: 0x96f1AD2Afb9582E7A66b719a132435A3FdDEa93e [P4] 2. approval window opened by the background: true [P4] 3. after adding wallet 2 -> ["Wallet 1","Wallet 2"] [P4] wallet 2 address: 0x535946AF49802605a9f33dc69eb77cEA6A8E6D53 [P4] 4. after answering the approval -> ["Wallet 1"] [P4] wallet 2's encrypted secret still in storage? false [P4] broadcasts: 1 | dApp settled: {"settled":"resolved","result":"0x4ff2ac15..."} [P4] 5. fresh popup wallet list: "Wallet 1+Address 1 0x96f1AD2A... [info] ETH 2.0000" ``` Also reproduced with two plain popup pages: `RESULT A: 2 wallets before B navigated, 1 after. *** WALLET LOST ***`. Answering an approval was the trigger here, but **any** navigation in the stale page does it, because `showView()` saves unconditionally. The background is a third writer: `backgroundRefresh()` at `src/background/index.js:1034-1048` does `loadState()` → seconds of network I/O → `saveState()`, so the same clobber happens with no second window open at all. ## Consequence The user loses the entire wallet — name, addresses, and the encrypted secret. Funds at those addresses are unrecoverable unless the recovery phrase was written down during the seconds it was on screen. Nothing reports it, and the dApp transaction succeeds normally so nothing looks wrong. Preconditions: one extension page open while another writes. That is a dApp approval plus the toolbar popup, or the background's 60-second balance alarm overlapping any user action. ## Definition of done - [ ] No writer overwrites another's state. Either `storage.onChanged` re-hydrates every page, or `saveState()` becomes a read-modify-write with per-field merge, or `wallets` moves to its own storage key written only by the paths that own it. - [ ] The background refresh path cannot clobber: it must not write back fields it did not change. - [ ] Test: add a wallet in one extension page, then force a save from a second page that was loaded before it; assert **both** wallets are in storage afterwards. - [ ] Test: the sequence above — approval window open, add a wallet in the popup, confirm the approval — leaves two wallets standing. - [ ] `make check` green.
clawbot added this to the 1.0.0 milestone 2026-08-20 11:58:28 +02:00
clawbot commented 2026-08-20 15:41:21 +02:00
Author
Collaborator
Copy Link

Implementation plan

Scope is this issue's DoD only. The root-cause refactor is #324 and stays out of this unit; this fix must not preclude it.

Approach: saveState() becomes a read-modify-write that merges only the fields this page actually changed, computed by diff against a per-page baseline.

All 43 saveState() call sites take no arguments. Requiring each caller to declare which fields it touched would be a 40-site mechanical change and would fail open — a caller that forgets a field silently stops persisting it. Diffing against a baseline needs no call-site changes and cannot fail open in that direction.

  • On loadState() and at the end of every saveState(), store a deep clone of the persisted fields as baseline. A clone is required, not a reference: callers mutate in place (state.wallets.push(...)), which would otherwise mutate the baseline too and make the diff always empty.
  • saveState() re-reads storage, deep-compares each persisted field against baseline, and writes {...fresh, ...onlyChangedFields}. Then it re-hydrates in-memory state from the merged result and resets baseline.
  • This satisfies DoD item 2 for free: backgroundRefresh() did not change wallets, so wallets is not in its diff and cannot be written back stale. Same for a stale page whose only change is currentView/viewStack.
  • Concurrent writers of the same field still resolve last-writer-wins. That is acceptable and must be stated in a comment at the merge; the wallet-destroying case is cross-field, not same-field.

Take a different approach if this one proves unsound, but say why on the PR.

Required:

  • Both DoD tests, each failing before the fix and passing after — the two-page save test and the approval-window sequence from the reproduction.
  • Any chrome.storage.local test stub this unit relies on must structured-clone on get, as the real API does. An aliasing stub makes this entire defect class invisible (see #324, DoD item 5).
  • Branch off next, PR based on next.
  • make check green, linting in Docker.
## Implementation plan Scope is this issue's DoD only. The root-cause refactor is https://git.eeqj.de/sneak/AutistMask/issues/324 and stays out of this unit; this fix must not preclude it. **Approach: `saveState()` becomes a read-modify-write that merges only the fields this page actually changed, computed by diff against a per-page baseline.** All 43 `saveState()` call sites take no arguments. Requiring each caller to declare which fields it touched would be a 40-site mechanical change and would fail open — a caller that forgets a field silently stops persisting it. Diffing against a baseline needs no call-site changes and cannot fail open in that direction. - On `loadState()` and at the end of every `saveState()`, store a **deep clone** of the persisted fields as `baseline`. A clone is required, not a reference: callers mutate in place (`state.wallets.push(...)`), which would otherwise mutate the baseline too and make the diff always empty. - `saveState()` re-reads storage, deep-compares each persisted field against `baseline`, and writes `{...fresh, ...onlyChangedFields}`. Then it re-hydrates in-memory `state` from the merged result and resets `baseline`. - This satisfies DoD item 2 for free: `backgroundRefresh()` did not change `wallets`, so `wallets` is not in its diff and cannot be written back stale. Same for a stale page whose only change is `currentView`/`viewStack`. - Concurrent writers of the *same* field still resolve last-writer-wins. That is acceptable and must be stated in a comment at the merge; the wallet-destroying case is cross-field, not same-field. Take a different approach if this one proves unsound, but say why on the PR. **Required:** - Both DoD tests, each failing before the fix and passing after — the two-page save test and the approval-window sequence from the reproduction. - Any `chrome.storage.local` test stub this unit relies on must **structured-clone on `get`**, as the real API does. An aliasing stub makes this entire defect class invisible (see https://git.eeqj.de/sneak/AutistMask/issues/324, DoD item 5). - Branch off `next`, PR based on `next`. - `make check` green, linting in Docker.
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#304
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?