fix: make saveState() a read-modify-write merge instead of a full-blob overwrite (closes #304) #337
3
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 sneak
|
bafb84930c |
fix: merge site permissions and network endpoints structurally, and give the identity merge a collision floor (closes #304)
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. |
||
|
sneak
|
af9568db90 |
fix: merge wallets by identity, not whole-field, in saveState() (closes #304)
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. |
||
|
sneak
|
31b2aa2d8a |
fix: make saveState() a read-modify-write merge instead of a full-blob overwrite (closes #304)
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. |