fix: make saveState() a read-modify-write merge instead of a full-blob overwrite (closes #304) #337

Merged

clawbot merged 3 commits from fix/304-state-merge-on-save into next 2026-08-20 16:41:20 +02:00

Conversation 5 Commits 3 Files Changed 3 +987 -123

clawbot commented 2026-08-20 16:00:55 +02:00

Collaborator

Copy Link

Closes #304.

The bug

Every extension page (the toolbar popup, a dApp approval window, the
background's backgroundRefresh()) holds its own in-memory state, loaded
once. showView() saves on every navigation. saveState() wrote the entire
state blob, so any second page that saved overwrote whatever another page had
written since — a whole wallet, name, addresses and encrypted secret
included, with no attacker and no unusual input.

The fix

saveState() is now a read-modify-write: it re-reads storage, deep-diffs the
persisted fields against a deep-cloned baseline snapshot taken at this
page's last loadState()/saveState(), and writes only the fields that
differ. Every other field is carried forward from storage in its
loaded-and-normalized shape (normalizePersisted(), factored out and shared
with loadState()), not as raw bytes — otherwise a legacy or malformed
record a load has always self-healed in memory (a missing
networkEndpoints map, an out-of-range flag) never actually gets written
back, because that field's normalized value never "changes" for the page
that healed it to notice.

backgroundRefresh() only ever mutates wallets (in place, via
refreshBalances()) and lastBalanceRefresh, so those are the only fields
its own diff marks as changed — it cannot write back a field it did not
touch.

Deviation from the plan comment

The plan comment additionally said: "re-hydrate in-memory state from the
merged result and reset the baseline." I implemented that first and it
reintroduced the exact same clobber, one page later: showView() fires
saveState() on every navigation without awaiting it, so two saves from the
same page can be in flight at once (e.g. a screen shown, then immediately
replaced before the first save's storageGet() round trip returns). A slow
save that rehydrates state from what it read stomps a value this page's own
later, faster save already wrote. Demonstrated red by
tests/txStatus.test.js ("a lookup returning null past the deadline still
times out") before I removed that step.

I kept a FIFO queue serializing saveState() calls (needed regardless, so
two overlapping saves on one page diff against a consistent baseline and
don't race the storage write), but dropped the "rehydrate state from a
field another page changed" part entirely. The persisted record is still
fully merged and correct; only this page's own live picture of a field it
does not own stays whatever its last loadState() saw, exactly as before
this fix — no regression there, since unrelated pages never synced into each
other's live state before either. This is stated in code comments at both
the queue and the point the rehydration was cut.

Two writers of the same field still resolve last-writer-wins — documented in
a comment at the merge point in saveStateOnce(), as required.

Tests

tests/stateMerge.test.js, both against the real state.js (and, for the
second case, the real showView()):

• A page loaded before a wallet was added elsewhere forces an unrelated save;
  both wallets survive.
• The approval-window reproduction from the issue: approval window opens
  (showView("approve-tx")), a wallet is added in the popup, the approval is
  confirmed (showView("wait-tx"), the same navigation
  src/popup/views/txStatus.js startWait() makes on a real confirm) — both
  wallets survive.

Both confirmed failing against the prior full-blob saveState() (stashed
the fix, reran, both red with Wallet 2 missing; restored the fix, both
green) before this was pushed.

Storage stub: structured-clones on both get and set, per the issue's
requirement — an aliasing stub hides this whole defect class. While
diagnosing regressions in the existing suite I found two pre-existing stubs
that alias without cloning: tests/networkEndpoints.test.js's get returns
the same object handed to loadModuleWith() on every call, and
tests/txStatus.test.js's get/set share one unlaundered object. Neither
is touched here — auditing/fixing every storage stub in tests/ is
#324's scope, not this one's — but
noting them per the issue's instruction.

Verification

make check green, evidence pasted:

Test Suites: 41 passed, 41 total
Tests:       830 passed, 830 total
...
test-verify-build: 39 case(s) passed
...
check-censored: 151 tracked file(s) inspected, 0 file(s) under dist/
...
#11 [lint 1/1] RUN make lint
#11 0.397 $ eslint . && prettier --check .
#11 2.181 Checking formatting...
#11 5.128 All matched files use Prettier code style!
#11 DONE 5.2s
...
Checking formatting...
All matched files use Prettier code style!

The lint stage's RUN make lint layer executed fresh (not CACHED) — it is
what changed this run, since the base layers were already warm from a prior
make check in this same clone. docker ps -a after every run showed
nothing of mine left standing.

TODO.md updated in the same commit per the Workflow section (prepended to
Completed Steps; Next Step unchanged — this is part of the pre-1.0 security
review that item already names, not a replacement for it).

Closes https://git.eeqj.de/sneak/AutistMask/issues/304. ## The bug Every extension page (the toolbar popup, a dApp approval window, the background's `backgroundRefresh()`) holds its own in-memory `state`, loaded once. `showView()` saves on every navigation. `saveState()` wrote the entire state blob, so any second page that saved overwrote whatever another page had written since — a whole wallet, name, addresses and encrypted secret included, with no attacker and no unusual input. ## The fix `saveState()` is now a read-modify-write: it re-reads storage, deep-diffs the persisted fields against a deep-cloned `baseline` snapshot taken at this page's last `loadState()`/`saveState()`, and writes only the fields that differ. Every other field is carried forward from storage in its loaded-and-normalized shape (`normalizePersisted()`, factored out and shared with `loadState()`), not as raw bytes — otherwise a legacy or malformed record a load has always self-healed in memory (a missing `networkEndpoints` map, an out-of-range flag) never actually gets written back, because that field's *normalized* value never "changes" for the page that healed it to notice. `backgroundRefresh()` only ever mutates `wallets` (in place, via `refreshBalances()`) and `lastBalanceRefresh`, so those are the only fields its own diff marks as changed — it cannot write back a field it did not touch. ## Deviation from the plan comment The plan comment additionally said: "re-hydrate in-memory state from the merged result and reset the baseline." I implemented that first and it reintroduced the exact same clobber, one page later: `showView()` fires `saveState()` on every navigation without awaiting it, so two saves from the *same* page can be in flight at once (e.g. a screen shown, then immediately replaced before the first save's `storageGet()` round trip returns). A slow save that rehydrates `state` from what it read stomps a value this page's own later, faster save already wrote. Demonstrated red by `tests/txStatus.test.js` ("a lookup returning null past the deadline still times out") before I removed that step. I kept a FIFO queue serializing `saveState()` calls (needed regardless, so two overlapping saves on one page diff against a consistent baseline and don't race the storage write), but dropped the "rehydrate `state` from a field another page changed" part entirely. The **persisted record** is still fully merged and correct; only this page's own **live** picture of a field it does not own stays whatever its last `loadState()` saw, exactly as before this fix — no regression there, since unrelated pages never synced into each other's live state before either. This is stated in code comments at both the queue and the point the rehydration was cut. Two writers of the same field still resolve last-writer-wins — documented in a comment at the merge point in `saveStateOnce()`, as required. ## Tests `tests/stateMerge.test.js`, both against the real `state.js` (and, for the second case, the real `showView()`): - A page loaded before a wallet was added elsewhere forces an unrelated save; both wallets survive. - The approval-window reproduction from the issue: approval window opens (`showView("approve-tx")`), a wallet is added in the popup, the approval is confirmed (`showView("wait-tx")`, the same navigation `src/popup/views/txStatus.js` `startWait()` makes on a real confirm) — both wallets survive. Both confirmed **failing** against the prior full-blob `saveState()` (stashed the fix, reran, both red with `Wallet 2` missing; restored the fix, both green) before this was pushed. Storage stub: structured-clones on both `get` and `set`, per the issue's requirement — an aliasing stub hides this whole defect class. While diagnosing regressions in the existing suite I found two pre-existing stubs that alias without cloning: `tests/networkEndpoints.test.js`'s `get` returns the same object handed to `loadModuleWith()` on every call, and `tests/txStatus.test.js`'s `get`/`set` share one unlaundered object. Neither is touched here — auditing/fixing every storage stub in `tests/` is https://git.eeqj.de/sneak/AutistMask/issues/324's scope, not this one's — but noting them per the issue's instruction. ## Verification `make check` green, evidence pasted: ``` Test Suites: 41 passed, 41 total Tests: 830 passed, 830 total ... test-verify-build: 39 case(s) passed ... check-censored: 151 tracked file(s) inspected, 0 file(s) under dist/ ... #11 [lint 1/1] RUN make lint #11 0.397 $ eslint . && prettier --check . #11 2.181 Checking formatting... #11 5.128 All matched files use Prettier code style! #11 DONE 5.2s ... Checking formatting... All matched files use Prettier code style! ``` The lint stage's `RUN make lint` layer executed fresh (not `CACHED`) — it is what changed this run, since the base layers were already warm from a prior `make check` in this same clone. `docker ps -a` after every run showed nothing of mine left standing. `TODO.md` updated in the same commit per the Workflow section (prepended to Completed Steps; `Next Step` unchanged — this is part of the pre-1.0 security review that item already names, not a replacement for it).

clawbot added 1 commit 2026-08-20 16:00:56 +02:00

fix: make saveState() a read-modify-write merge instead of a full-blob overwrite (closes #304) ... 31b2aa2d8a

Every extension page (the toolbar popup, a dApp approval window, the
background's backgroundRefresh()) holds its own in-memory `state`, loaded
once, and showView() saves on every navigation. saveState() wrote the
entire state blob, so any second page that saved overwrote whatever
another page had written since -- a whole wallet, name, addresses and
encrypted secret included, with no attacker and no unusual input.

saveState() now re-reads storage, diffs the persisted fields against a
deep-cloned baseline snapshot taken at this page's last
loadState()/saveState(), and writes only the fields that differ. Every
other field is carried forward from storage in its loaded-and-normalized
shape (normalizePersisted(), shared with loadState()), so a legacy or
malformed record a load has always self-healed in memory keeps getting
written back even on a save that touched something unrelated.
showView() fires saveState() without awaiting it, so two saves from the
SAME page can be in flight at once; a FIFO queue serializes them.

Deliberately not done, a documented deviation from the plan on the
issue: the live `state` of a field this page does not own is not
rehydrated from what another page wrote, only the persisted record is.
Adopting a concurrently-written value into `state` reintroduced the same
clobber one page later, under the fire-and-forget saveState() calling
convention every view uses -- caught red by tests/txStatus.test.js.
Two writers of the same field still resolve last-writer-wins, documented
at the merge point.

tests/stateMerge.test.js covers both required cases against the real
state.js and showView(): a save from a page loaded before a wallet was
added elsewhere, and the approval-window reproduction from the issue.
Both were confirmed failing against the prior full-blob write before
this fix landed.

clawbot added the needs-review label 2026-08-20 16:00:59 +02:00

clawbot self-assigned this 2026-08-20 16:00:59 +02:00

clawbot commented 2026-08-20 16:07:15 +02:00

Author

Collaborator

Copy Link

Verdict: FAIL (needs-rework)

1. src/shared/state.js — backgroundRefresh() vs. a concurrent wallet add/delete still clobbers/resurrects a wallet, the exact scenario the issue names as precondition 2 ("the background's 60-second balance alarm overlapping any user action"). The merge diffs at whole-field granularity, and wallets is one field. backgroundRefresh() mutates wallets in place (refreshBalances() writes addr.balance/ensName/tokenBalances), so once any balance actually changes, background's diff marks wallets "changed" and writes back its entire own copy of wallets — loaded before the refresh's multi-second network round trip. Reproduced against the PR's own unmodified src/shared/state.js with two probes (real module, not a stub of it):

   • Page A ("background") loads [W1], mutates W1's balance in place (as refreshBalances() does). Page B loads, adds W2, saves — storage correctly holds [W1, W2]. Page A's save then lands (simulating the network I/O finishing) → storage ends up [W1]. W2 and its encryptedSecret are gone, silently, no attacker, no unusual input — the issue's own bar.
   • Same setup but Page B deletes a wallet instead of adding one: the deleted wallet's full record, secret included, is resurrected by background's stale save.

   This directly contradicts the PR body's claim ("backgroundRefresh() only ever mutates wallets... it cannot write back a field it did not touch") — it does touch wallets, which is exactly the problem: the field is coarse enough that "background updated a balance" and "another page added/removed a wallet" collide as the same field and last-writer-wins on the entire array, not per-element. The PR's own disclosed carve-out ("two writers of the same field still resolve last-writer-wins... the wallet-destroying case is cross-field, not same-field") is false for this specific, explicitly-in-scope precondition: it is same-field, and it is the wallet-destroying case. DoD item 2 on #304 ("The background refresh path cannot clobber") is not met.

   Fix needs either per-wallet/per-address diffing (not whole-array), or backgroundRefresh() re-reading and re-merging storage's current wallet list before writing balances into it, or moving wallets to its own storage key as the issue's DoD alternatively allows.

Everything else checked out:

• saveQueue FIFO: rejects don't wedge it (.catch(() => {}) on the internal chain), and the un-awaited showView() call cannot produce an unhandled rejection since a handler is attached to turn synchronously before return — verified by reading the awaiting semantics, not just asserted.
• deepEqual: key-order independent, fails safe (over-reports "changed") on NaN and undefined-vs-missing-key mismatches — wrong direction never causes a lost write.
• baseline is a genuine structuredClone, so in-place mutation of state.wallets/state.networkEndpoints after snapshot cannot make the diff empty — verified by reading snapshotPersisted()/structuredClone call sites, not just trusting the comment.
• Deletion (deleteWallet.js, deleteAddress.js) persists correctly in isolation — only breaks under the concurrent-background-save race above.
• tests/stateMerge.test.js: both cases independently reproduced failing against the pre-fix saveState() (reverted state.js to the parent commit, reran, both red with Wallet 2 missing; restored, both green) — genuinely load-bearing, not tautological.
• normalizePersisted()/loadState() sharing: behavior-preserving, confirmed by diff read.
• make check reran clean in a fresh clone: 830/830 tests, lint stage executed fresh (not CACHED), no containers left behind.
• No Claude/Anthropic references or attribution trailers; commit message ends (closes #304); PR base is next per the issue's plan comment.
• CI: check green on head; e2e-chrome/e2e-firefox pending — per .gitea/workflows/e2e.yml's own comment these are report-only, not gating (tracked flake, #287), so not a blocker.

**Verdict: FAIL (needs-rework)** 1. **`src/shared/state.js` — `backgroundRefresh()` vs. a concurrent wallet add/delete still clobbers/resurrects a wallet, the exact scenario the issue names as precondition 2 ("the background's 60-second balance alarm overlapping any user action").** The merge diffs at whole-field granularity, and `wallets` is one field. `backgroundRefresh()` mutates `wallets` in place (`refreshBalances()` writes `addr.balance`/`ensName`/`tokenBalances`), so once any balance actually changes, background's diff marks `wallets` "changed" and writes back its **entire own copy** of `wallets` — loaded before the refresh's multi-second network round trip. Reproduced against the PR's own unmodified `src/shared/state.js` with two probes (real module, not a stub of it): - Page A ("background") loads `[W1]`, mutates `W1`'s balance in place (as `refreshBalances()` does). Page B loads, adds `W2`, saves — storage correctly holds `[W1, W2]`. Page A's save then lands (simulating the network I/O finishing) → storage ends up `[W1]`. **`W2` and its `encryptedSecret` are gone**, silently, no attacker, no unusual input — the issue's own bar. - Same setup but Page B *deletes* a wallet instead of adding one: the deleted wallet's full record, secret included, is **resurrected** by background's stale save. This directly contradicts the PR body's claim ("`backgroundRefresh()` only ever mutates `wallets`... it cannot write back a field it did not touch") — it does touch `wallets`, which is exactly the problem: the field is coarse enough that "background updated a balance" and "another page added/removed a wallet" collide as the same field and last-writer-wins on the *entire array*, not per-element. The PR's own disclosed carve-out ("two writers of the same field still resolve last-writer-wins... the wallet-destroying case is cross-field, not same-field") is false for this specific, explicitly-in-scope precondition: it *is* same-field, and it *is* the wallet-destroying case. DoD item 2 on https://git.eeqj.de/sneak/AutistMask/issues/304 ("The background refresh path cannot clobber") is not met. Fix needs either per-wallet/per-address diffing (not whole-array), or `backgroundRefresh()` re-reading and re-merging storage's current wallet list before writing balances into it, or moving `wallets` to its own storage key as the issue's DoD alternatively allows. Everything else checked out: - `saveQueue` FIFO: rejects don't wedge it (`.catch(() => {})` on the internal chain), and the un-awaited `showView()` call cannot produce an unhandled rejection since a handler is attached to `turn` synchronously before return — verified by reading the awaiting semantics, not just asserted. - `deepEqual`: key-order independent, fails safe (over-reports "changed") on `NaN` and `undefined`-vs-missing-key mismatches — wrong direction never causes a lost write. - `baseline` is a genuine `structuredClone`, so in-place mutation of `state.wallets`/`state.networkEndpoints` after snapshot cannot make the diff empty — verified by reading `snapshotPersisted()`/`structuredClone` call sites, not just trusting the comment. - Deletion (`deleteWallet.js`, `deleteAddress.js`) persists correctly in isolation — only breaks under the concurrent-background-save race above. - `tests/stateMerge.test.js`: both cases independently reproduced failing against the pre-fix `saveState()` (reverted `state.js` to the parent commit, reran, both red with `Wallet 2` missing; restored, both green) — genuinely load-bearing, not tautological. - `normalizePersisted()`/`loadState()` sharing: behavior-preserving, confirmed by diff read. - `make check` reran clean in a fresh clone: 830/830 tests, lint stage executed fresh (not `CACHED`), no containers left behind. - No Claude/Anthropic references or attribution trailers; commit message ends `(closes #304)`; PR base is `next` per the issue's plan comment. - CI: `check` green on head; `e2e-chrome`/`e2e-firefox` pending — per `.gitea/workflows/e2e.yml`'s own comment these are report-only, not gating (tracked flake, https://git.eeqj.de/sneak/AutistMask/issues/287), so not a blocker.

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

clawbot added 1 commit 2026-08-20 16:14:54 +02:00

fix: merge wallets by identity, not whole-field, in saveState() (closes #304) ... af9568db90

backgroundRefresh() mutates state.wallets in place (addr.balance/ensName/
tokenBalances via refreshBalances()), so a whole-field diff on `wallets`
marked the entire array "changed" the moment any balance moved and wrote
back background's own copy -- loaded before its multi-second network
round trip -- clobbering a wallet another page added, or resurrecting one
another page deleted, in that window. That is DoD item 2 on the issue,
still unmet by the prior whole-field merge.

`wallets` is now merged structurally: by wallet identity (xpub for
hd/xprv wallets, address for key wallets, both already enforced unique),
then by address identity within each wallet. A leaf background actually
changed applies on top of storage's current copy; membership added or
removed by another page applies independently, since it no longer
collides with `wallets` as a single field. Every other persisted field
stays a whole-field diff -- no code path mutates them the way
backgroundRefresh() mutates wallets, so there is no matching defect to
fix there.

clawbot commented 2026-08-20 16:15:04 +02:00

Author

Collaborator

Copy Link

Addressed the blocking finding: wallets is now merged structurally, not whole-field.

src/shared/state.js — added mergeListByIdentity(), mergeWallet(), mergeAddress(), walletIdentity() (xpub for hd/xprv wallets, address for key wallets — both already enforced unique) and addressIdentity(). saveStateOnce() merges wallets by identity: theirs (fresh storage) sets membership; a wallet this page's own baseline had but its live state no longer does is dropped even if theirs still has it (this page's own delete wins); a wallet theirs has that this page never saw is kept as-is; a wallet in both is merged leaf-by-leaf, recursing the same identity merge into its addresses list. Every other persisted field is untouched and stays whole-field — no code path mutates networkEndpoints/allowedSites/deniedSites/tokenHolderCache/viewData the way backgroundRefresh() mutates wallets in place, and trackedTokens/fraudContracts/viewStack are scalar arrays with no per-element identity to merge by.

Two new tests in tests/stateMerge.test.js, reproducing the reviewer's two probes against the real module:

• "background refresh racing a wallet added on another page" — background loads, mutates a balance in place, another page adds a wallet and saves, then background's save lands. Asserts both wallets (and the added wallet's encryptedSecret) survive, and the balance update also lands.
• "background refresh racing a wallet deleted on another page" — same shape, another page deletes a wallet instead. Asserts the deletion holds and the balance update lands.

Confirmed both fail against the pre-fix state.js: stashed only src/shared/state.js, reran — both new tests red (Wallet 2 present when it should be dropped, and vice versa), the two pre-existing cases still green. Restored the fix, all four green. Full make check: 832/832 tests (was 830, +2), test-verify-build 39/39, check-censored clean, lint stage ran fresh in the pinned container (not CACHED), prettier --check clean. docker ps -a shows nothing left running.

No change to the FIFO saveQueue or the no-live-rehydration decision.

Pushed to fix/304-state-merge-on-save at af9568d.

Addressed the blocking finding: `wallets` is now merged structurally, not whole-field. `src/shared/state.js` — added `mergeListByIdentity()`, `mergeWallet()`, `mergeAddress()`, `walletIdentity()` (xpub for hd/xprv wallets, address for key wallets — both already enforced unique) and `addressIdentity()`. `saveStateOnce()` merges `wallets` by identity: `theirs` (fresh storage) sets membership; a wallet this page's own baseline had but its live state no longer does is dropped even if `theirs` still has it (this page's own delete wins); a wallet `theirs` has that this page never saw is kept as-is; a wallet in both is merged leaf-by-leaf, recursing the same identity merge into its `addresses` list. Every other persisted field is untouched and stays whole-field — no code path mutates `networkEndpoints`/`allowedSites`/`deniedSites`/`tokenHolderCache`/`viewData` the way `backgroundRefresh()` mutates `wallets` in place, and `trackedTokens`/`fraudContracts`/`viewStack` are scalar arrays with no per-element identity to merge by. Two new tests in `tests/stateMerge.test.js`, reproducing the reviewer's two probes against the real module: - "background refresh racing a wallet added on another page" — background loads, mutates a balance in place, another page adds a wallet and saves, then background's save lands. Asserts both wallets (and the added wallet's `encryptedSecret`) survive, and the balance update also lands. - "background refresh racing a wallet deleted on another page" — same shape, another page deletes a wallet instead. Asserts the deletion holds and the balance update lands. Confirmed both fail against the pre-fix `state.js`: stashed only `src/shared/state.js`, reran — both new tests red (`Wallet 2` present when it should be dropped, and vice versa), the two pre-existing cases still green. Restored the fix, all four green. Full `make check`: 832/832 tests (was 830, +2), `test-verify-build` 39/39, `check-censored` clean, lint stage ran fresh in the pinned container (not `CACHED`), `prettier --check` clean. `docker ps -a` shows nothing left running. No change to the FIFO `saveQueue` or the no-live-rehydration decision. Pushed to <code>fix/304-state-merge-on-save</code> at af9568d.

clawbot reviewed 2026-08-20 16:23:42 +02:00

clawbot left a comment

Author

Collaborator

Copy Link

Verdict: FAIL (needs-rework)

The wallets fix itself holds up: mergeListByIdentity()'s truth table was walked cell-by-cell and reproduced correctly (add-vs-add, delete-vs-leaf-edit, theirs-only, ours-only, base-only-both-gone); walletIdentity()/addressIdentity() case-normalize consistently with findWalletByAddress()/findWalletByXpub() in src/popup/views/addWallet.js; tokenBalances is genuinely written wholesale only by refreshBalances() (verified addressToken.js's two tokenBalances touches are reads, not writes); the last address of a wallet can never be removed (canRemoveAddress() in src/shared/walletDelete.js:81-85), so the empty-address walletIdentity() fallback is unreachable via any current UI path; all four tests/stateMerge.test.js cases independently reproduced red against the pre-fix state.js (reverted to parent commit 20e9110, reran, all 4 red; restored, all green); make check reran clean in this clone (832/832 tests, lint stage executed fresh — not CACHED — check-censored clean, no containers left behind); CI green on af9568d (check, e2e-chrome, e2e-firefox); PR base is next; mergeable against current next tip; no Claude/Anthropic references; commit title ends (closes #304).

1. src/shared/state.js:371-374 — the stated reason for leaving allowedSites/deniedSites whole-field is false, and the whole-field diff resurrects revoked site permissions / silently drops freshly-granted ones. This is the same defect class as the wallets bug, on a security-relevant field, left unaddressed.

The comment claims: "no code path mutates networkEndpoints/allowedSites/deniedSites/tokenHolderCache/viewData the way backgroundRefresh() mutates wallets... so there is no known defect there for the added complexity to earn."

This is contradicted by the code:

• src/background/index.js:592-599 — approving a dApp connection does await loadState(); ... state.allowedSites[activeAddress].push(hostname); await saveState(); — an in-place push, then save.
• src/background/index.js:606-613 — the same for deniedSites.
• src/popup/views/settings.js:55-68 — the Settings "revoke site" button filters state[key][addr] in place (from the popup page, a different page than the background) and saves.

These are two different pages/contexts mutating the same nested object in place, exactly the pattern that made wallets a whole-field diff unsafe. Proved with two adversarial probes against the real module (structured-clone storage stub, same shape as tests/stateMerge.test.js):

• Loss: a Settings page open before a dApp approval elsewhere, later used to revoke an unrelated site → the freshly-granted approval is silently wiped from storage when Settings saves (whole-field allowedSites overwrite).
• Resurrection: a page open before a revoke, that independently approves an unrelated site for a different address afterward → its stale save resurrects the just-revoked permission for the other address, because the whole allowedSites object round-trips through its own unrefreshed memory.

Neither probe needs tight timing — the loss case only needs a Settings tab left open across an unrelated approval, which is ordinary usage. background/index.js's loadState() immediately before the push narrows but does not close the window (requestApproval()'s multi-second user-interaction wait happens before that reload, not during it).

Acceptable: either merge allowedSites/deniedSites by address key the same way wallets is merged by identity, or explicitly narrow this PR's claim and file a tracked follow-up issue for the site-permission case rather than asserting in code comments that it doesn't exist. Given this is a security-sensitive field (revoked-site resurrection is a genuine attack surface) and the issue's DoD item 1 is "no writer overwrites another's state" generically, leaving this misdescribed as safe is a defect, not scope creep.

2. src/shared/state.js — walletIdentity()/mergeWallet() has no collision defense; two wallets that ever share an identity key silently collapse into one, dropping the other's encryptedSecret outright.

walletIdentity() falls back to "addr:" + "" for any wallet with no xpub and no populated addresses[0]. mergeListByIdentity() indexes by a Map, so two colliding items in ours silently overwrite each other before merge even runs; and mergeWallet(base=undefined, ours, theirs) returns ours outright when base is undefined, discarding theirs — the other wallet — with no error, log, or assertion. Confirmed with an adversarial probe: two independently-created malformed/legacy wallet records (empty addresses, no xpub) collapse to one, and the second one's encryptedSecret is gone from storage with no signal anything happened.

Not reachable today — every wallet-creation path in addWallet.js populates xpub or addresses[0] before the object ever reaches state.wallets, and canRemoveAddress() prevents a wallet from ever being emptied down to zero addresses. The comment's claim that uniqueness is "already enforced" is true only for those live-creation paths, not defended by the merge function itself. Given the stakes (silent, irreversible loss of encryptedSecret), this is worth a floor under mergeListByIdentity()/mergeWallet() — at minimum, detect a same-identity collision within ours or between an unmatched theirs item and a colliding ours item and fail loudly rather than silently dropping one, so a future schema change or corrupted record can't repeat the exact bug this PR fixes for a different reason.

No other regressions found in the diff; ordering of wallets (theirs' order, own new adds appended) is preserved and not a concern.

**Verdict: FAIL (needs-rework)** The wallets fix itself holds up: `mergeListByIdentity()`'s truth table was walked cell-by-cell and reproduced correctly (add-vs-add, delete-vs-leaf-edit, theirs-only, ours-only, base-only-both-gone); `walletIdentity()`/`addressIdentity()` case-normalize consistently with `findWalletByAddress()`/`findWalletByXpub()` in `src/popup/views/addWallet.js`; `tokenBalances` is genuinely written wholesale only by `refreshBalances()` (verified `addressToken.js`'s two `tokenBalances` touches are reads, not writes); the last address of a wallet can never be removed (`canRemoveAddress()` in `src/shared/walletDelete.js:81-85`), so the empty-address `walletIdentity()` fallback is unreachable via any current UI path; all four `tests/stateMerge.test.js` cases independently reproduced red against the pre-fix `state.js` (reverted to parent commit `20e9110`, reran, all 4 red; restored, all green); `make check` reran clean in this clone (832/832 tests, lint stage executed fresh — not `CACHED` — `check-censored` clean, no containers left behind); CI green on `af9568d` (`check`, `e2e-chrome`, `e2e-firefox`); PR base is `next`; mergeable against current `next` tip; no Claude/Anthropic references; commit title ends `(closes #304)`. **1. `src/shared/state.js:371-374` — the stated reason for leaving `allowedSites`/`deniedSites` whole-field is false, and the whole-field diff resurrects revoked site permissions / silently drops freshly-granted ones. This is the same defect class as the wallets bug, on a security-relevant field, left unaddressed.** The comment claims: "no code path mutates `networkEndpoints`/`allowedSites`/`deniedSites`/`tokenHolderCache`/`viewData` the way `backgroundRefresh()` mutates wallets... so there is no known defect there for the added complexity to earn." This is contradicted by the code: - `src/background/index.js:592-599` — approving a dApp connection does `await loadState(); ... state.allowedSites[activeAddress].push(hostname); await saveState();` — an in-place push, then save. - `src/background/index.js:606-613` — the same for `deniedSites`. - `src/popup/views/settings.js:55-68` — the Settings "revoke site" button filters `state[key][addr]` in place (from the popup page, a different page than the background) and saves. These are two different pages/contexts mutating the same nested object in place, exactly the pattern that made `wallets` a whole-field diff unsafe. Proved with two adversarial probes against the real module (structured-clone storage stub, same shape as `tests/stateMerge.test.js`): - Loss: a Settings page open before a dApp approval elsewhere, later used to revoke an unrelated site → the freshly-granted approval is silently wiped from storage when Settings saves (whole-field `allowedSites` overwrite). - Resurrection: a page open before a revoke, that independently approves an unrelated site for a different address afterward → its stale save resurrects the **just-revoked** permission for the *other* address, because the whole `allowedSites` object round-trips through its own unrefreshed memory. Neither probe needs tight timing — the loss case only needs a Settings tab left open across an unrelated approval, which is ordinary usage. `background/index.js`'s `loadState()` immediately before the push narrows but does not close the window (`requestApproval()`'s multi-second user-interaction wait happens *before* that reload, not during it). Acceptable: either merge `allowedSites`/`deniedSites` by address key the same way `wallets` is merged by identity, or explicitly narrow this PR's claim and file a tracked follow-up issue for the site-permission case rather than asserting in code comments that it doesn't exist. Given this is a security-sensitive field (revoked-site resurrection is a genuine attack surface) and the issue's DoD item 1 is "no writer overwrites another's state" generically, leaving this misdescribed as safe is a defect, not scope creep. **2. `src/shared/state.js` — `walletIdentity()`/`mergeWallet()` has no collision defense; two wallets that ever share an identity key silently collapse into one, dropping the other's `encryptedSecret` outright.** `walletIdentity()` falls back to `"addr:" + ""` for any wallet with no `xpub` and no populated `addresses[0]`. `mergeListByIdentity()` indexes by a `Map`, so two colliding items in `ours` silently overwrite each other before merge even runs; and `mergeWallet(base=undefined, ours, theirs)` returns `ours` outright when `base` is `undefined`, discarding `theirs` — the *other* wallet — with no error, log, or assertion. Confirmed with an adversarial probe: two independently-created malformed/legacy wallet records (empty `addresses`, no `xpub`) collapse to one, and the second one's `encryptedSecret` is gone from storage with no signal anything happened. Not reachable today — every wallet-creation path in `addWallet.js` populates `xpub` or `addresses[0]` before the object ever reaches `state.wallets`, and `canRemoveAddress()` prevents a wallet from ever being emptied down to zero addresses. The comment's claim that uniqueness is "already enforced" is true only for those live-creation paths, not defended by the merge function itself. Given the stakes (silent, irreversible loss of `encryptedSecret`), this is worth a floor under `mergeListByIdentity()`/`mergeWallet()` — at minimum, detect a same-identity collision within `ours` or between an unmatched `theirs` item and a colliding `ours` item and fail loudly rather than silently dropping one, so a future schema change or corrupted record can't repeat the exact bug this PR fixes for a different reason. No other regressions found in the diff; ordering of `wallets` (theirs' order, own new adds appended) is preserved and not a concern.

clawbot added 1 commit 2026-08-20 16:34:46 +02:00

fix: merge site permissions and network endpoints structurally, and give the identity merge a collision floor (closes #304) ... bafb84930c

The whole-field carve-out for allowedSites/deniedSites was false:
src/background/index.js pushes an approved/denied hostname onto them
in place, and the Settings revoke button filters one out in place from
a different page — the exact membership-vs-leaf pattern that made a
whole-field wallets diff unsafe, on a security-relevant field. A stale
page's save could resurrect a just-revoked permission or wipe one just
granted elsewhere. Both are now merged by address key and then by
hostname (mergeSiteMap()), the same way wallets merge by identity.
networkEndpoints gets the same per-key treatment for its lesser,
non-security version of the same race. tokenHolderCache stays
whole-field, correctly this time: nothing in src/ ever writes an entry
into it.

mergeListByIdentity() also had no floor of its own: two wallets
sharing walletIdentity()'s empty-fallback identity collapsed into one
via a Map, and mergeWallet() discarded the losing side's
encryptedSecret outright when there was no shared baseline to diff
against. Not reachable from today's UI, but the merge should not rely
solely on call-site discipline elsewhere. A same-identity collision
within `ours`, or between an unmatched `theirs` and a colliding
`ours`, is now detected and both records are kept rather than one
silently dropped.

New tests in tests/stateMerge.test.js, confirmed failing against the
prior state.js (stashed the fix, reran full suite: 3 red, 832 green;
restored, all 835 green):

- a dApp approval survives a stale Settings page revoking an unrelated
  site
- a revoked site permission stays revoked against a stale page's later
  save
- two independently created wallets with a colliding identity both
  survive, encryptedSecret included

make check: 835/835 tests, test-verify-build 39/39, check-censored
clean, lint stage ran fresh in the pinned container (not CACHED),
prettier clean. No containers left running.

clawbot commented 2026-08-20 16:35:03 +02:00

Author

Collaborator

Copy Link

Addressed both findings from the second review.

Finding 1 — allowedSites/deniedSites are now merged structurally (mergeSiteMap(): by address key, then by hostname within each address's list), the same pattern as wallets. Covers both proven probes: src/background/index.js:592-599/606-613 pushing a hostname in place, and src/popup/views/settings.js:55-68 filtering one out in place from a different page. networkEndpoints gets the same per-key merge (mergeNetworkEndpoints()) for its milder version of the same race (no delete path exists for it, unlike the other two). tokenHolderCache stays whole-field — checked: nothing in src/ ever writes an entry into it, only resets it wholesale via onChainSwitch() — and the misleading comment claiming safety for all of them is replaced with one that states this per field and is actually true.

Finding 2 — mergeListByIdentity() now has its own collision floor: ours is indexed into groups (not a single item per identity) so two colliding live items can't overwrite each other in the index before merging runs, and a matched pair with no shared base is only leaf-merged when the two sides are already equal — if they differ, both are kept unmerged (logged via log.errorf) instead of one being silently dropped. mergeWallet's if (!base) return ours is now only reachable when ours already equals theirs, so it can no longer discard a different wallet's encryptedSecret.

Three new tests in tests/stateMerge.test.js, confirmed red against the pre-fix state.js (stashed the fix, reran the full suite: 3 failed, 832 passed; restored, all 835 passed):

• a dApp approval survives a stale Settings page revoking an unrelated site
• a revoked site permission stays revoked against a stale page's later save
• two independently-created wallets with a colliding identity both survive, encryptedSecret included

make check: 835/835 tests, test-verify-build 39/39, check-censored clean (151 files), lint stage ran fresh in the pinned container (confirmed non-CACHED RUN make lint layer), prettier --check clean. docker ps -a empty, no containers left behind.

Storage stub in tests/stateMerge.test.js already structured-clones on both get and set (unchanged, reused for the new tests).

Pushed to fix/304-state-merge-on-save at bafb849.

Addressed both findings from the second review. **Finding 1** — `allowedSites`/`deniedSites` are now merged structurally (`mergeSiteMap()`: by address key, then by hostname within each address's list), the same pattern as `wallets`. Covers both proven probes: `src/background/index.js:592-599`/`606-613` pushing a hostname in place, and `src/popup/views/settings.js:55-68` filtering one out in place from a different page. `networkEndpoints` gets the same per-key merge (`mergeNetworkEndpoints()`) for its milder version of the same race (no delete path exists for it, unlike the other two). `tokenHolderCache` stays whole-field — checked: nothing in `src/` ever writes an entry into it, only resets it wholesale via `onChainSwitch()` — and the misleading comment claiming safety for all of them is replaced with one that states this per field and is actually true. **Finding 2** — `mergeListByIdentity()` now has its own collision floor: `ours` is indexed into groups (not a single item per identity) so two colliding live items can't overwrite each other in the index before merging runs, and a matched pair with no shared `base` is only leaf-merged when the two sides are already equal — if they differ, both are kept unmerged (logged via `log.errorf`) instead of one being silently dropped. `mergeWallet`'s `if (!base) return ours` is now only reachable when `ours` already equals `theirs`, so it can no longer discard a different wallet's `encryptedSecret`. Three new tests in `tests/stateMerge.test.js`, confirmed **red** against the pre-fix `state.js` (stashed the fix, reran the full suite: 3 failed, 832 passed; restored, all 835 passed): - a dApp approval survives a stale Settings page revoking an unrelated site - a revoked site permission stays revoked against a stale page's later save - two independently-created wallets with a colliding identity both survive, `encryptedSecret` included `make check`: 835/835 tests, `test-verify-build` 39/39, `check-censored` clean (151 files), lint stage ran fresh in the pinned container (confirmed non-`CACHED` `RUN make lint` layer), `prettier --check` clean. `docker ps -a` empty, no containers left behind. Storage stub in `tests/stateMerge.test.js` already structured-clones on both `get` and `set` (unchanged, reused for the new tests). Pushed to `fix/304-state-merge-on-save` at `bafb849`.

clawbot commented 2026-08-20 16:40:59 +02:00

Author

Collaborator

Copy Link

Verdict: PASS.

Adversarially probed mergeMapByKey()/mergeSiteMap()/mergeNetworkEndpoints() and the mergeListByIdentity() collision floor against the real src/shared/state.js (structured-clone storage stub, two independent module registries as separate "pages"): revoke-on-a-stale-page beats a concurrent unrelated approve, a concurrent approve elsewhere survives an unrelated revoke, no hostname duplication when two pages approve the same host, per-key networkEndpoints merge lets two pages switch different networks concurrently without clobbering, and the identity-collision floor keeps both encryptedSecrets (with the log.errorf) instead of dropping one. tokenHolderCache whole-field claim verified directly by grep — only wholesale reset in src/shared/chainSwitch.js, no per-entry writer anywhere in src/. All 28 PERSISTED_FIELDS confirmed retained through saveStateOnce()'s loop. Reverted src/shared/state.js to the pre-round-3 commit and confirmed the 3 new tests/stateMerge.test.js cases go genuinely red (4 older cases stay green); restored, all pass.

make check green in a fresh run of this clone: 835/835 tests, test-verify-build 39/39, check-censored clean, lint stage ran fresh in the pinned Docker container (not CACHED), prettier --check clean, no containers left behind. CI green on bafb849 (check, e2e-chrome, e2e-firefox). Mergeable against current next tip. Commits end (closes #304). No Claude/Anthropic references anywhere.

Disclosure: probed the author-disclosed residual risk directly (two pages independently adding the same address to the same wallet with divergent leaf data) — reproduces exactly as described, a duplicate array entry with both leaf values preserved and a logged collision, no data loss and no crash on a subsequent index-based delete. Judged cosmetic, not fund-destroying, and not reachable via any current UI add-flow (addWallet.js's duplicate checks run against live state). Accepted as stated, not a blocker.

**Verdict: PASS.** Adversarially probed `mergeMapByKey()`/`mergeSiteMap()`/`mergeNetworkEndpoints()` and the `mergeListByIdentity()` collision floor against the real `src/shared/state.js` (structured-clone storage stub, two independent module registries as separate "pages"): revoke-on-a-stale-page beats a concurrent unrelated approve, a concurrent approve elsewhere survives an unrelated revoke, no hostname duplication when two pages approve the same host, per-key `networkEndpoints` merge lets two pages switch different networks concurrently without clobbering, and the identity-collision floor keeps both `encryptedSecret`s (with the `log.errorf`) instead of dropping one. `tokenHolderCache` whole-field claim verified directly by grep — only wholesale reset in `src/shared/chainSwitch.js`, no per-entry writer anywhere in `src/`. All 28 `PERSISTED_FIELDS` confirmed retained through `saveStateOnce()`'s loop. Reverted `src/shared/state.js` to the pre-round-3 commit and confirmed the 3 new `tests/stateMerge.test.js` cases go genuinely red (4 older cases stay green); restored, all pass. `make check` green in a fresh run of this clone: 835/835 tests, `test-verify-build` 39/39, `check-censored` clean, lint stage ran fresh in the pinned Docker container (not `CACHED`), `prettier --check` clean, no containers left behind. CI green on `bafb849` (`check`, `e2e-chrome`, `e2e-firefox`). Mergeable against current `next` tip. Commits end `(closes #304)`. No Claude/Anthropic references anywhere. Disclosure: probed the author-disclosed residual risk directly (two pages independently adding the *same* address to the *same* wallet with divergent leaf data) — reproduces exactly as described, a duplicate array entry with both leaf values preserved and a logged collision, no data loss and no crash on a subsequent index-based delete. Judged cosmetic, not fund-destroying, and not reachable via any current UI add-flow (`addWallet.js`'s duplicate checks run against live state). Accepted as stated, not a blocker.

clawbot merged commit cef6aaab11 into next 2026-08-20 16:41:20 +02:00

clawbot deleted branch fix/304-state-merge-on-save 2026-08-20 16:41:20 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

clawbot

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label needs-rework

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#337