diff --git a/TODO.md b/TODO.md index 15419fd..5398877 100644 --- a/TODO.md +++ b/TODO.md @@ -44,6 +44,32 @@ but the review is broader than any of them. # Completed Steps +- 2026-08-20: A second extension page can no longer silently delete a wallet + ([#304](https://git.eeqj.de/sneak/AutistMask/issues/304)). `saveState()` wrote + the entire state blob, and every extension page — the toolbar popup, a dApp + approval window, `backgroundRefresh()` — holds its own in-memory `state`, + loaded once, with `showView()` saving on every navigation; a second page that + saved after a first had written something new overwrote it, no attacker or + unusual input required. `saveState()` is now a read-modify-write: it re-reads + storage, diffs the persisted fields against a deep-cloned `baseline` snapshot + taken at the last `loadState()`/`saveState()` on that page, and writes only + the fields that actually changed — everything else 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 else entirely. `showView()` fires `saveState()` on every navigation + without awaiting it, so two saves from the same page can be in flight at once; + a FIFO queue serializes them rather than letting a slow one finish after a + later one and re-derive a stale answer. Deliberately not done: 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, caught by + `tests/txStatus.test.js` red. Two writers of the same field still resolve + last-writer-wins, documented at the merge point. `tests/stateMerge.test.js` + covers the two-page save and the approval-window reproduction from the issue — + add a wallet in one page, force a save from a second page loaded before it, + both wallets survive — each demonstrated failing against the unfixed full-blob + write. - 2026-08-20: A forgotten password no longer wedges the wallet ([#312](https://git.eeqj.de/sneak/AutistMask/issues/312)). Deleting a wallet was password-gated and importing its recovery phrase again was refused as a diff --git a/src/shared/state.js b/src/shared/state.js index a774e6b..6cc5384 100644 --- a/src/shared/state.js +++ b/src/shared/state.js @@ -6,6 +6,7 @@ const { networkById } = require("./networks"); const { RESTORABLE_VIEWS } = require("../popup/restorableViews"); const { storageGet, storageSet } = require("./browserApi"); +const { log } = require("./log"); const DEFAULT_STATE = { hasWallet: false, @@ -88,135 +89,529 @@ function currentNetwork() { return networkById(state.networkId); } -async function saveState() { - const persisted = { - hasWallet: state.hasWallet, - wallets: state.wallets, - trackedTokens: state.trackedTokens, - networkId: state.networkId, - rpcUrl: state.rpcUrl, - blockscoutUrl: state.blockscoutUrl, - networkEndpoints: state.networkEndpoints, - lastBalanceRefresh: state.lastBalanceRefresh, - activeAddress: state.activeAddress, - allowedSites: state.allowedSites, - deniedSites: state.deniedSites, - rememberSiteChoice: state.rememberSiteChoice, - showZeroBalanceTokens: state.showZeroBalanceTokens, - hideSpoofedSymbols: state.hideSpoofedSymbols, - hideLowHolderTokens: state.hideLowHolderTokens, - hideFraudContracts: state.hideFraudContracts, - hideDustTransactions: state.hideDustTransactions, - dustThresholdGwei: state.dustThresholdGwei, - utcTimestamps: state.utcTimestamps, - fraudContracts: state.fraudContracts, - tokenHolderCache: state.tokenHolderCache, - theme: state.theme, - debugMode: state.debugMode, - currentView: state.currentView, - selectedWallet: state.selectedWallet, - selectedAddress: state.selectedAddress, - selectedToken: state.selectedToken, - viewData: state.viewData, - viewStack: state.viewStack, - }; - await storageSet({ autistmask: persisted }); +// Every field written to and read from the single "autistmask" storage key. +// hasWallet is deliberately excluded from the diffing/merge logic below — +// like loadState() does, it is always derived from `wallets`, never carried +// as an independent value. +const PERSISTED_FIELDS = Object.keys(DEFAULT_STATE) + .filter((key) => key !== "hasWallet") + .concat([ + "currentView", + "selectedWallet", + "selectedAddress", + "selectedToken", + "viewData", + "viewStack", + ]); + +// Turn a raw stored (or missing) record into the full, defaulted shape +// loadState() used to assign directly onto `state`. Pulled out as a pure +// function so saveState() can apply it too: the fields THIS page did not +// change still have to come from storage in their loaded-and-normalized +// form, not as the raw bytes another page (or an old release) left there — +// otherwise a legacy shape a load has always self-healed in memory (a +// missing networkEndpoints map, an out-of-range flag) is dropped right back +// into storage unfixed every time the page that DID normalize it saves +// something unrelated, because that field's value never "changed" for that +// page to notice. +function normalizePersisted(saved) { + saved = saved || {}; + const out = {}; + out.wallets = saved.wallets || []; + // Derived, never trusted verbatim off storage — see loadState(). + out.hasWallet = out.wallets.length > 0; + out.trackedTokens = saved.trackedTokens || []; + out.networkId = saved.networkId || DEFAULT_STATE.networkId; + out.rpcUrl = saved.rpcUrl || DEFAULT_STATE.rpcUrl; + out.blockscoutUrl = saved.blockscoutUrl || DEFAULT_STATE.blockscoutUrl; + // An actual object is required, not merely a truthy non-array: the code + // below and onChainSwitch() index and ASSIGN INTO this value, and + // assigning a property to a string or a number is a silent no-op in + // sloppy mode. Copied rather than referenced, nested pairs included, so + // normalizing never mutates the object a caller handed in. + const rawEndpoints = + typeof saved.networkEndpoints === "object" && + saved.networkEndpoints !== null && + !Array.isArray(saved.networkEndpoints) + ? saved.networkEndpoints + : {}; + out.networkEndpoints = {}; + for (const netId of Object.keys(rawEndpoints)) { + out.networkEndpoints[netId] = { ...rawEndpoints[netId] }; + } + // A profile written before this map existed carries exactly one pair of + // endpoints, belonging to whatever network it was last on. Adopt it as + // that network's remembered pair, so a custom endpoint set on the old + // build is not lost by the first switch away and back. + if (!out.networkEndpoints[out.networkId]) { + out.networkEndpoints[out.networkId] = { + rpcUrl: out.rpcUrl, + blockscoutUrl: out.blockscoutUrl, + }; + } + out.lastBalanceRefresh = saved.lastBalanceRefresh || 0; + out.activeAddress = saved.activeAddress || null; + out.allowedSites = + saved.allowedSites && !Array.isArray(saved.allowedSites) + ? saved.allowedSites + : {}; + out.deniedSites = + saved.deniedSites && !Array.isArray(saved.deniedSites) + ? saved.deniedSites + : {}; + out.rememberSiteChoice = + saved.rememberSiteChoice !== undefined + ? saved.rememberSiteChoice + : true; + out.showZeroBalanceTokens = + saved.showZeroBalanceTokens !== undefined + ? saved.showZeroBalanceTokens + : true; + // A profile written before this setting existed has no key for it. It + // is a safety filter, so absent must load as on, not as undefined. + out.hideSpoofedSymbols = + saved.hideSpoofedSymbols !== undefined + ? saved.hideSpoofedSymbols + : true; + out.hideLowHolderTokens = + saved.hideLowHolderTokens !== undefined + ? saved.hideLowHolderTokens + : true; + out.hideFraudContracts = + saved.hideFraudContracts !== undefined + ? saved.hideFraudContracts + : true; + out.hideDustTransactions = + saved.hideDustTransactions !== undefined + ? saved.hideDustTransactions + : true; + out.dustThresholdGwei = + saved.dustThresholdGwei !== undefined + ? saved.dustThresholdGwei + : 100000; + out.utcTimestamps = + saved.utcTimestamps !== undefined ? saved.utcTimestamps : false; + out.fraudContracts = saved.fraudContracts || []; + out.tokenHolderCache = saved.tokenHolderCache || {}; + out.theme = saved.theme || "system"; + out.debugMode = saved.debugMode !== undefined ? saved.debugMode : false; + out.currentView = saved.currentView || null; + out.selectedWallet = + saved.selectedWallet !== undefined ? saved.selectedWallet : null; + out.selectedAddress = + saved.selectedAddress !== undefined ? saved.selectedAddress : null; + out.selectedToken = saved.selectedToken || null; + out.viewData = saved.viewData || {}; + out.viewStack = restorableStack(saved.viewStack, out.currentView); + return out; +} + +// The persisted fields as they stood at the end of this page's last +// loadState() or saveState(). saveState() diffs the live state against this +// to find only the fields THIS page actually changed. +// +// Deep-cloned, not a reference: callers mutate persisted objects and arrays +// in place (state.wallets.push(...)), and a reference baseline would mutate +// right along with `state`, so the diff would always come out empty. +let baseline = null; + +function snapshotPersisted() { + const out = {}; + for (const key of PERSISTED_FIELDS) out[key] = state[key]; + return out; +} + +function deepEqual(a, b) { + if (a === b) return true; + if (typeof a !== "object" || typeof b !== "object") return false; + if (a === null || b === null) return false; + if (Array.isArray(a) !== Array.isArray(b)) return false; + const aKeys = Object.keys(a); + const bKeys = Object.keys(b); + if (aKeys.length !== bKeys.length) return false; + for (const key of aKeys) { + if (!Object.prototype.hasOwnProperty.call(b, key)) return false; + if (!deepEqual(a[key], b[key])) return false; + } + return true; +} + +// Stable identity for a wallet, independent of its position in the array +// (which shifts under a concurrent add/delete elsewhere) and independent of +// its mutable fields (name is user-editable; addresses gains/loses entries +// via scanning and deleteAddress.js). An "hd"/"xprv" wallet's xpub never +// changes for its lifetime and is already enforced unique +// (findWalletByXpub() in addWallet.js). A "key" wallet has no xpub, exactly +// one address for its whole lifetime (nothing ever adds to or removes from +// a key wallet's address list), and that address is already enforced +// unique (findWalletByAddress()) — so it stands in for identity there. +// Neither invariant is enforced by this function or by +// mergeListByIdentity() below — they hold only because every wallet- +// creation path in addWallet.js happens to populate one or the other before +// the wallet ever reaches state.wallets, and because canRemoveAddress() in +// walletDelete.js never lets a wallet's address list go to zero. A wallet +// with neither (an empty/legacy/corrupt record) falls back to the same +// "addr:" identity as every other such record, which is a genuine +// collision, not a proxy for one — see the collision handling in +// mergeListByIdentity(). +function walletIdentity(wallet) { + if (wallet.xpub) return "xpub:" + wallet.xpub; + const first = wallet.addresses && wallet.addresses[0]; + return "addr:" + (first ? String(first.address).toLowerCase() : ""); +} + +// Stable identity for an address within one wallet's address list. An +// address is unique within its wallet and, once derived or imported, never +// changes — only whether it is present. +function addressIdentity(addr) { + return String(addr.address).toLowerCase(); +} + +// Merge one array of identity-bearing objects (wallets, or the addresses +// inside one wallet) by identity rather than by array index — an index +// shifts under a concurrent insert/delete elsewhere, which would merge the +// wrong pair of objects entirely. +// +// `theirs` (fresh storage) sets the membership baseline and the order: +// - An item this page never had baseline knowledge of, but that is in +// `theirs`, was added by someone else — kept as-is. +// - An item `base` had and `ours` no longer has was deleted by THIS page +// — dropped even though `theirs` still has it (this page's own delete +// must win over a background save that only touched leaf fields). +// - An item present in both `ours` and `theirs` is merged leaf-by-leaf via +// `mergeItem`, so a leaf this page changed (e.g. a renamed wallet) lands +// on top of `theirs`' otherwise-current copy (e.g. a refreshed balance). +// Anything left in `ours` that `base` never had and `theirs` does not have +// yet is this page's own new addition — appended. +// +// `identityOf` is not guaranteed collision-free (walletIdentity() falls +// back to one shared "addr:" value for any wallet with neither an xpub nor +// a populated first address). Two records that collide under it must never +// silently collapse into one — that is exactly how this function used to +// drop a wallet, encryptedSecret included, with no error and no log. Two +// defenses: +// - `ours` is indexed into GROUPS, not a single item per identity, so two +// colliding live items on this page can't overwrite each other in the +// index before the merge below even runs. +// - A matched pair with no shared `base` entry (neither page ever agreed +// on this identity) is only merged leaf-by-leaf when the two sides are +// already equal. If they differ, that is not "the same record edited +// twice", it is two different records that happen to share an identity +// — both are kept, unmerged, rather than guessing which one is real. +function mergeListByIdentity(base, ours, theirs, identityOf, mergeItem) { + base = base || []; + ours = ours || []; + theirs = theirs || []; + const baseIndex = new Map(base.map((item) => [identityOf(item), item])); + const oursIndex = new Map(); + for (const item of ours) { + const id = identityOf(item); + if (!oursIndex.has(id)) oursIndex.set(id, []); + oursIndex.get(id).push(item); + } + + const result = []; + const seen = new Set(); + + for (const theirItem of theirs) { + const id = identityOf(theirItem); + seen.add(id); + const oursGroup = oursIndex.get(id); + if (baseIndex.has(id) && !oursGroup) continue; + if (oursGroup) { + const baseItem = baseIndex.get(id); + if (!baseItem && !deepEqual(oursGroup[0], theirItem)) { + log.errorf( + "state: identity collision merging", + JSON.stringify(id), + "- keeping both records instead of dropping one", + ); + result.push(theirItem, ...oursGroup); + } else { + result.push(mergeItem(baseItem, oursGroup[0], theirItem)); + for (let i = 1; i < oursGroup.length; i++) { + result.push(oursGroup[i]); + } + } + } else { + result.push(theirItem); + } + } + + for (const item of ours) { + const id = identityOf(item); + if (seen.has(id)) continue; + if (!baseIndex.has(id)) result.push(item); + } + + return result; +} + +// Merge one wallet's scalar/leaf fields (name, encryptedSecret, nextIndex, +// ...) against base, then recurse into its address list by identity. `base` +// is null when this page created the wallet itself and no other page has +// (yet) produced a same-identity record — nothing to merge in that case, +// this page's own copy wins outright. mergeListByIdentity() only ever calls +// this with `!base` when `ours` and `theirs` are already equal (a genuine +// collision between two DIFFERENT same-identity records is caught and kept +// as two separate entries before this function is reached), so returning +// `ours` here can't discard a different wallet's data. +function mergeWallet(base, ours, theirs) { + if (!base) return ours; + const merged = { ...theirs }; + for (const key of Object.keys(ours)) { + if (key === "addresses") continue; + if (!deepEqual(ours[key], base[key])) merged[key] = ours[key]; + } + merged.addresses = mergeListByIdentity( + base.addresses, + ours.addresses, + theirs.addresses, + addressIdentity, + mergeAddress, + ); + return merged; +} + +// Merge one address's leaf fields (balance, ensName, tokenBalances, ...). +// tokenBalances is itself an array, but only backgroundRefresh() ever +// writes it and always wholesale (refreshBalances() in +// src/shared/balances.js), so there is no membership to reconcile within +// it — it is a leaf like balance or ensName, not a list with its own +// identity. +function mergeAddress(base, ours, theirs) { + if (!base) return ours; + const merged = { ...theirs }; + for (const key of Object.keys(ours)) { + if (!deepEqual(ours[key], base[key])) merged[key] = ours[key]; + } + return merged; +} + +// Merge a plain object keyed by string (allowedSites/deniedSites: address -> +// hostname list; networkEndpoints: networkId -> {rpcUrl, blockscoutUrl}) the +// same way mergeListByIdentity() merges an array — by key, not by whole- +// object diff — so a key one page added or removed applies independently of +// a key another page edited. Unlike an array's identity function, an object +// key can't collide with a different logical entry (Object.keys() is +// already deduplicated), so this needs no collision floor of its own. +function mergeMapByKey(base, ours, theirs, mergeLeaf) { + base = base || {}; + ours = ours || {}; + theirs = theirs || {}; + const result = {}; + const seen = new Set(); + + for (const key of Object.keys(theirs)) { + seen.add(key); + const inBase = Object.prototype.hasOwnProperty.call(base, key); + const inOurs = Object.prototype.hasOwnProperty.call(ours, key); + if (inBase && !inOurs) continue; // this page deleted the whole entry + if (inOurs) { + result[key] = mergeLeaf(base[key], ours[key], theirs[key]); + } else { + result[key] = theirs[key]; + } + } + + for (const key of Object.keys(ours)) { + if (seen.has(key)) continue; + if (!Object.prototype.hasOwnProperty.call(base, key)) { + result[key] = ours[key]; + } + } + + return result; +} + +// allowedSites/deniedSites: { [address]: [hostname, ...] }. The hostname +// list is itself membership, not a leaf — src/background/index.js pushes a +// newly approved/denied hostname onto it in place, and the Settings "revoke" +// button (src/popup/views/settings.js) filters a hostname out of it in +// place, from a different page. Merge it the same way wallets are merged: +// identity is the hostname itself, so a merged pair is always equal and +// mergeItem is a no-op pick. +function mergeHostnameList(base, ours, theirs) { + return mergeListByIdentity( + base, + ours, + theirs, + (hostname) => hostname, + (b, o, t) => t, + ); +} + +function mergeSiteMap(base, ours, theirs) { + return mergeMapByKey(base, ours, theirs, mergeHostnameList); +} + +// networkEndpoints: { [networkId]: {rpcUrl, blockscoutUrl} }. onChainSwitch() +// (src/shared/chainSwitch.js) writes state.networkEndpoints[networkId] in +// place before saving. No code path ever removes a key from this map, so the +// membership collision that matters for allowedSites/wallets (an add on one +// page racing a delete on another) can't happen here — but two pages +// switching to two different networks concurrently still race a whole-field +// diff the same way, so it gets the same per-key merge for the leaf edit +// case (e.g. Settings saving a custom RPC URL for the active network). +function mergeEndpointEntry(base, ours, theirs) { + if (!base) return ours; + const merged = { ...theirs }; + for (const key of Object.keys(ours)) { + if (!deepEqual(ours[key], base[key])) merged[key] = ours[key]; + } + return merged; +} + +function mergeNetworkEndpoints(base, ours, theirs) { + return mergeMapByKey(base, ours, theirs, mergeEndpointEntry); +} + +// Read-modify-write, merged per field, rather than one full-blob write. +// +// 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. A full-blob write here +// clobbers whatever a second page had written since — including, in the +// worst case, an entire wallet and its encrypted secret with no attacker +// and no unusual input (see the issue this fixes). +// +// Only the fields this page actually changed — those that differ from +// `baseline`, captured at the last loadState()/saveState() on this page — +// are written; every other field is carried forward from whatever is in +// storage right now, which may already be a value another page wrote. +// +// `wallets` is merged structurally (mergeListByIdentity(), by wallet +// identity and then by address identity within each wallet), not as one +// whole field: backgroundRefresh() mutates wallets IN PLACE (addr.balance / +// ensName / tokenBalances, via refreshBalances()), so a whole-field diff +// would mark all of `wallets` "changed" the moment any balance moved and +// write 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. Merging by identity lets +// background's leaf changes and another page's membership changes +// (add/delete a wallet or an address) apply independently instead of +// colliding as the same field. +// +// `allowedSites` and `deniedSites` get the same treatment (mergeSiteMap(), +// by address key and then by hostname within each address's list), for the +// identical reason: src/background/index.js pushes a newly +// approved/denied hostname onto them in place, and the Settings "revoke" +// button (src/popup/views/settings.js) filters one out in place, from a +// different page. A whole-field diff here doesn't just lose data, it is a +// security defect — a stale page's save can resurrect a just-revoked site +// permission, or silently wipe a permission just granted elsewhere. +// +// `networkEndpoints` gets the same treatment too (mergeNetworkEndpoints(), +// by network id), since onChainSwitch() writes into it in place; the value +// per key is a small leaf object with no membership of its own; see the +// comment at mergeEndpointEntry() for why the collision this closes is +// milder than the other two. +// +// Every other persisted field stays a whole-field diff: +// `trackedTokens`/`fraudContracts`/`viewStack` are arrays of scalars with no +// per-element identity to merge by; `tokenHolderCache` is a map shaped like +// the ones above, but nothing in src/ ever writes an entry into it — it is +// only ever reset wholesale to `{}` (onChainSwitch()) — so there is no +// in-place mutation for a whole-field diff to collide with; `viewData` is +// this page's own UI scratch space, not data another page has any reason to +// share membership of. +// +// This does not make two pages that both change the SAME leaf concurrently +// safe: last write wins on that one leaf, same as before. What it removes +// is the cross-field (and now cross-membership-vs-leaf) clobber — a page +// that only navigated, or only refreshed a balance, overwriting a wallet or +// address list it never touched the membership of. +// +// This page's own live `state` is deliberately NOT rehydrated from a field +// another page changed — only the record written to storage is merged. +// showView() fires saveState() on every navigation without awaiting it, +// which is what makes the queue above necessary in the first place, and a +// save that is slow to come back has no way to tell whether the field it +// is about to hand back is still the current answer or has since been +// overtaken by something this very page did in the meantime; writing it +// into `state` regardless reintroduced exactly the clobber this function +// exists to remove, just delayed and confined to one page instead of two +// (caught by tests/txStatus.test.js). A page's live picture of a field it +// does not own goes on being whatever its last loadState() saw, same as +// before this fix; only the persisted record is guaranteed current. +async function saveStateOnce() { + const current = snapshotPersisted(); + const result = await storageGet("autistmask"); + // Normalized, not raw: a field this page did not change still has to + // come from storage in its loaded (self-healed) shape. See + // normalizePersisted() above. + const fresh = normalizePersisted(result.autistmask); + + const merged = { ...fresh }; + for (const key of PERSISTED_FIELDS) { + if (key === "wallets") { + merged.wallets = mergeListByIdentity( + baseline ? baseline.wallets : [], + current.wallets, + fresh.wallets, + walletIdentity, + mergeWallet, + ); + } else if (key === "allowedSites" || key === "deniedSites") { + merged[key] = mergeSiteMap( + baseline ? baseline[key] : {}, + current[key], + fresh[key], + ); + } else if (key === "networkEndpoints") { + merged.networkEndpoints = mergeNetworkEndpoints( + baseline ? baseline.networkEndpoints : {}, + current.networkEndpoints, + fresh.networkEndpoints, + ); + } else if ( + baseline === null || + !deepEqual(current[key], baseline[key]) + ) { + merged[key] = current[key]; + } + } + merged.hasWallet = Boolean(merged.wallets && merged.wallets.length > 0); + + await storageSet({ autistmask: merged }); + + // Derived from this page's own wallets, never adopted off the wire — + // see loadState(). Everything else this page did not change is left + // exactly as it stood; see the note above. + state.hasWallet = state.wallets.length > 0; + + baseline = structuredClone(snapshotPersisted()); +} + +// showView() calls 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 has come back. Left concurrent, the first save's turn would +// finish after the second's live-state mutation and then re-hydrate `state` +// from what IT read, stomping the second, later change back to a stale +// value — the same clobber this function exists to prevent, just between +// two saves on one page instead of two pages. Queuing makes every save's +// snapshot-diff-write-rehydrate run start to finish before the next one +// begins, so each one only ever sees the true live state at its turn. +let saveQueue = Promise.resolve(); + +function saveState() { + const turn = saveQueue.then(saveStateOnce); + // The queue must advance even when a save rejects, or every save after + // it queues behind a promise that never settles. + saveQueue = turn.catch(() => {}); + return turn; } async function loadState() { const result = await storageGet("autistmask"); if (result.autistmask) { - const saved = result.autistmask; - state.wallets = saved.wallets || []; - // Derived, never read from storage: a profile persisted with the flag - // out of step with the wallet list would otherwise stay broken on - // every load. Nothing depends on the two disagreeing. - state.hasWallet = state.wallets.length > 0; - state.trackedTokens = saved.trackedTokens || []; - state.networkId = saved.networkId || DEFAULT_STATE.networkId; - state.rpcUrl = saved.rpcUrl || DEFAULT_STATE.rpcUrl; - state.blockscoutUrl = - saved.blockscoutUrl || DEFAULT_STATE.blockscoutUrl; - // An actual object is required, not merely a truthy non-array: the - // code below and onChainSwitch() index and ASSIGN INTO this value, - // and assigning a property to a string or a number is a silent no-op - // in sloppy mode. A stored primitive would therefore be re-persisted - // unchanged forever, and every switch would fall back to the network - // default — the endpoint loss this map exists to prevent, with no - // self-healing. The allowedSites/deniedSites guards below are only - // read from, which is why they can be looser. - state.networkEndpoints = - typeof saved.networkEndpoints === "object" && - saved.networkEndpoints !== null && - !Array.isArray(saved.networkEndpoints) - ? saved.networkEndpoints - : {}; - // A profile written before this map existed carries exactly one pair - // of endpoints, belonging to whatever network it was last on. Adopt - // it as that network's remembered pair, so a custom endpoint set on - // the old build is not lost by the first switch away and back. - if (!state.networkEndpoints[state.networkId]) { - state.networkEndpoints[state.networkId] = { - rpcUrl: state.rpcUrl, - blockscoutUrl: state.blockscoutUrl, - }; - } - state.lastBalanceRefresh = saved.lastBalanceRefresh || 0; - state.activeAddress = saved.activeAddress || null; - state.allowedSites = - saved.allowedSites && !Array.isArray(saved.allowedSites) - ? saved.allowedSites - : {}; - state.deniedSites = - saved.deniedSites && !Array.isArray(saved.deniedSites) - ? saved.deniedSites - : {}; - state.rememberSiteChoice = - saved.rememberSiteChoice !== undefined - ? saved.rememberSiteChoice - : true; - state.showZeroBalanceTokens = - saved.showZeroBalanceTokens !== undefined - ? saved.showZeroBalanceTokens - : true; - // A profile written before this setting existed has no key for it. - // It is a safety filter, so absent must load as on, not as undefined. - state.hideSpoofedSymbols = - saved.hideSpoofedSymbols !== undefined - ? saved.hideSpoofedSymbols - : true; - state.hideLowHolderTokens = - saved.hideLowHolderTokens !== undefined - ? saved.hideLowHolderTokens - : true; - state.hideFraudContracts = - saved.hideFraudContracts !== undefined - ? saved.hideFraudContracts - : true; - state.hideDustTransactions = - saved.hideDustTransactions !== undefined - ? saved.hideDustTransactions - : true; - state.dustThresholdGwei = - saved.dustThresholdGwei !== undefined - ? saved.dustThresholdGwei - : 100000; - state.utcTimestamps = - saved.utcTimestamps !== undefined ? saved.utcTimestamps : false; - state.fraudContracts = saved.fraudContracts || []; - state.tokenHolderCache = saved.tokenHolderCache || {}; - state.theme = saved.theme || "system"; - state.debugMode = - saved.debugMode !== undefined ? saved.debugMode : false; - state.currentView = saved.currentView || null; - state.selectedWallet = - saved.selectedWallet !== undefined ? saved.selectedWallet : null; - state.selectedAddress = - saved.selectedAddress !== undefined ? saved.selectedAddress : null; - state.selectedToken = saved.selectedToken || null; - state.viewData = saved.viewData || {}; - state.viewStack = restorableStack(saved.viewStack, state.currentView); + Object.assign(state, normalizePersisted(result.autistmask)); } + // The point of comparison every saveState() on this page diffs against, + // whether storage had a profile or was empty. See PERSISTED_FIELDS above + // saveState() for why a reference here would be wrong. + baseline = structuredClone(snapshotPersisted()); } function currentAddress() { diff --git a/tests/stateMerge.test.js b/tests/stateMerge.test.js new file mode 100644 index 0000000..fb02e73 --- /dev/null +++ b/tests/stateMerge.test.js @@ -0,0 +1,443 @@ +// saveState() used to write the entire state blob every time +// (src/shared/state.js). Every extension page — the toolbar popup, a dApp +// approval window opened by the background, backgroundRefresh() in +// src/background/index.js — holds its own in-memory `state`, loaded once, +// and src/popup/views/helpers.js showView() saves on EVERY navigation. So +// any second page that saved after a first page had written something new +// overwrote it, with no attacker and no unusual input: a whole wallet, name, +// addresses and encrypted secret included, silently gone +// (https://git.eeqj.de/sneak/AutistMask/issues/304). +// +// Both cases below drive the real state.js module through two independent +// module registries sharing one storage backend, the way two real extension +// pages share one chrome.storage.local. The storage stub structured-clones +// on both get and set — a stub that hands back the object it was given +// aliases the caller's own mutation and would make this entire defect class +// invisible (see https://git.eeqj.de/sneak/AutistMask/issues/324). + +function makeStorage() { + let store = {}; + return { + get: async (keys) => { + const wanted = + keys === undefined || keys === null + ? Object.keys(store) + : [].concat(keys); + const out = {}; + for (const key of wanted) { + if (key in store) out[key] = structuredClone(store[key]); + } + return out; + }, + set: async (items) => { + for (const [key, value] of Object.entries(items)) { + store[key] = structuredClone(value); + } + }, + }; +} + +// One extension page: a fresh module registry over the shared storage. +// state.js resolves the storage API at require time, so the stub has to be +// installed before the module is loaded, and `state` is a module-level +// singleton, so each page needs its own registry to hold its own copy. +function loadPage(storage) { + jest.resetModules(); + globalThis.chrome = { storage: { local: storage } }; + return { + state: require("../src/shared/state"), + helpers: require("../src/popup/views/helpers"), + }; +} + +function wallet(name, secret, address) { + return { + type: "hd", + name, + xpub: "xpub-" + name, + encryptedSecret: secret, + nextIndex: 1, + addresses: [{ address, balance: "0", tokenBalances: [] }], + }; +} + +const W1 = wallet( + "Wallet 1", + "secret-one", + "0x66133E8ea0f5D1d612D2502a968757D1048c214a", +); +const W2 = wallet( + "Wallet 2", + "secret-two", + "0xdAC17F958D2ee523a2206206994597C13D831ec7", +); + +// Minimal DOM: showView() toggles view elements, clears the flash line and +// creates/removes the debug banner. Nothing here is asserted; it only has to +// answer without throwing, the way the popup's own index.html would. +function makeElement(id) { + const classes = new Set(); + return { + id, + textContent: "", + style: {}, + classList: { + add: (...n) => n.forEach((c) => classes.add(c)), + remove: (...n) => n.forEach((c) => classes.delete(c)), + toggle: (c, force) => { + const on = force === undefined ? !classes.has(c) : force; + if (on) classes.add(c); + else classes.delete(c); + return on; + }, + }, + remove: () => {}, + }; +} + +function makeDocument() { + const els = new Map(); + return { + getElementById(id) { + if (id === "debug-banner") return null; + if (!els.has(id)) els.set(id, makeElement(id)); + return els.get(id); + }, + createElement: () => makeElement("created"), + body: { prepend: () => {} }, + }; +} + +afterEach(() => { + delete globalThis.chrome; + delete globalThis.document; +}); + +describe("a save from a page that never saw a wallet another page added", () => { + // The first DoD case on the issue: add a wallet in one page, then force + // a save from a second page loaded before that wallet existed. Both + // wallets must survive. + test("both wallets are in storage afterwards", async () => { + const storage = makeStorage(); + await storage.set({ autistmask: { wallets: [W1] } }); + + // Loaded while storage held only Wallet 1, and never reloads — + // the approval window in the reproduction, or a second popup that + // has been open for a while. + const stale = loadPage(storage); + await stale.state.loadState(); + expect(stale.state.state.wallets).toHaveLength(1); + + // A second page, loaded after, adds a wallet — the exact sequence + // src/popup/views/addWallet.js uses. + const fresh = loadPage(storage); + await fresh.state.loadState(); + fresh.state.state.wallets.push(W2); + fresh.state.state.hasWallet = true; + await fresh.state.saveState(); + + expect( + (await storage.get("autistmask")).autistmask.wallets, + ).toHaveLength(2); + + // The stale page saves something that has nothing to do with + // wallets — exactly what showView() does on every navigation, and + // what backgroundRefresh() does after a balance poll. + stale.state.state.currentView = "settings"; + await stale.state.saveState(); + + const persisted = (await storage.get("autistmask")).autistmask; + expect(persisted.wallets.map((w) => w.name)).toEqual([ + "Wallet 1", + "Wallet 2", + ]); + expect(persisted.wallets.map((w) => w.encryptedSecret)).toEqual([ + "secret-one", + "secret-two", + ]); + }); +}); + +describe("the approval-window reproduction", () => { + // approval window open, add a wallet in the popup, confirm the approval + // — the exact sequence from the issue. The approval window and the + // popup are the same popup code with a different starting view, so + // showView() is the real save path in both: src/popup/views/approval.js + // showTxApproval() calls showView("approve-tx") when the window opens, + // and a successful confirm calls + // src/popup/views/txStatus.js showWait() -> startWait(), which calls + // showView("wait-tx") — the save that clobbered the second wallet in + // the reproduction on the issue. + test("the wallet added in the popup survives confirming the approval", async () => { + globalThis.document = makeDocument(); + + const storage = makeStorage(); + await storage.set({ autistmask: { wallets: [W1] } }); + + // The background opens the approval window on the approve-tx + // screen; nothing else has happened yet. + const approvalWindow = loadPage(storage); + await approvalWindow.state.loadState(); + approvalWindow.helpers.showView("approve-tx"); + // showView() does not await its own saveState(); an extra save + // joins the same queue and only resolves once that one has too, + // which is the black-box way to know it landed. + await approvalWindow.state.saveState(); + + // The user adds a wallet in the popup — a separate page, loaded + // after the approval window. + const popup = loadPage(storage); + await popup.state.loadState(); + popup.state.state.wallets.push(W2); + popup.state.state.hasWallet = true; + await popup.state.saveState(); + expect( + (await storage.get("autistmask")).autistmask.wallets, + ).toHaveLength(2); + + // The user confirms the approval. The approval window navigates + // approve-tx -> wait-tx, saving again from state it loaded before + // Wallet 2 ever existed. + approvalWindow.helpers.showView("wait-tx"); + await approvalWindow.state.saveState(); + + const persisted = (await storage.get("autistmask")).autistmask; + expect(persisted.wallets.map((w) => w.name)).toEqual([ + "Wallet 1", + "Wallet 2", + ]); + expect(persisted.wallets.map((w) => w.encryptedSecret)).toEqual([ + "secret-one", + "secret-two", + ]); + }); +}); + +// backgroundRefresh() (src/background/index.js) loads state, spends seconds +// on network I/O in refreshBalances() (src/shared/balances.js) mutating +// addr.balance/ensName/tokenBalances IN PLACE on the wallets it already +// knew about, then saves. Precondition 2 on the issue: that refresh window +// overlapping a membership change (add or delete) on another page must not +// clobber or resurrect a wallet — a whole-field diff on `wallets` failed +// this, because "background changed a balance" and "another page changed +// membership" collided as the same field. +describe("background refresh racing a wallet added on another page", () => { + test("the wallet added elsewhere survives background's stale balance save", async () => { + const storage = makeStorage(); + await storage.set({ autistmask: { wallets: [W1] } }); + + // "background": loads first, and its save is the one that lands + // last, modeling the multi-second network round trip in between. + const background = loadPage(storage); + await background.state.loadState(); + background.state.state.wallets[0].addresses[0].balance = "1.2345"; + + // A second page, loaded after, adds a wallet while background's + // refresh is still in flight. + const popup = loadPage(storage); + await popup.state.loadState(); + popup.state.state.wallets.push(W2); + popup.state.state.hasWallet = true; + await popup.state.saveState(); + expect( + (await storage.get("autistmask")).autistmask.wallets, + ).toHaveLength(2); + + // background's save lands last, carrying only its balance update. + await background.state.saveState(); + + const persisted = (await storage.get("autistmask")).autistmask; + expect(persisted.wallets.map((w) => w.name)).toEqual([ + "Wallet 1", + "Wallet 2", + ]); + expect(persisted.wallets.map((w) => w.encryptedSecret)).toEqual([ + "secret-one", + "secret-two", + ]); + // The balance update itself must not be lost either — this is a + // merge, not deletion-always-wins. + expect(persisted.wallets[0].addresses[0].balance).toBe("1.2345"); + }); +}); + +describe("background refresh racing a wallet deleted on another page", () => { + test("the wallet deleted elsewhere stays deleted after background's stale balance save", async () => { + const storage = makeStorage(); + await storage.set({ autistmask: { wallets: [W1, W2] } }); + + const background = loadPage(storage); + await background.state.loadState(); + background.state.state.wallets[0].addresses[0].balance = "1.2345"; + + // A second page deletes Wallet 2 while background's refresh is in + // flight — the same splice deleteWallet.js's removeWalletFromState() + // does. + const popup = loadPage(storage); + await popup.state.loadState(); + popup.state.state.wallets.splice(1, 1); + popup.state.state.hasWallet = popup.state.state.wallets.length > 0; + await popup.state.saveState(); + expect( + (await storage.get("autistmask")).autistmask.wallets, + ).toHaveLength(1); + + await background.state.saveState(); + + const persisted = (await storage.get("autistmask")).autistmask; + expect(persisted.wallets.map((w) => w.name)).toEqual(["Wallet 1"]); + expect(persisted.wallets[0].addresses[0].balance).toBe("1.2345"); + }); +}); + +// allowedSites/deniedSites: { [address]: [hostname, ...] }. Mutated in place +// from two different contexts — src/background/index.js:592-599 pushes a +// newly approved hostname onto state.allowedSites[activeAddress], and the +// Settings "revoke" button (src/popup/views/settings.js:55-68) filters a +// hostname out of state[key][addr] in place, deleting the address key +// entirely once its list is empty — the exact membership-vs-whole-field +// pattern that made the whole-field `wallets` diff unsafe, on a +// security-relevant field: a stale whole-field save here can resurrect a +// revoked permission or wipe a freshly granted one. +const ADDR1 = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; +const ADDR2 = "0xdAC17F958D2ee523a2206206994597C13D831ec7"; + +function approveSite(pageState, address, hostname) { + if (!pageState.allowedSites[address]) { + pageState.allowedSites[address] = []; + } + if (!pageState.allowedSites[address].includes(hostname)) { + pageState.allowedSites[address].push(hostname); + } +} + +function revokeSite(pageState, hostname) { + for (const addr of Object.keys(pageState.allowedSites)) { + pageState.allowedSites[addr] = pageState.allowedSites[addr].filter( + (h) => h !== hostname, + ); + if (pageState.allowedSites[addr].length === 0) { + delete pageState.allowedSites[addr]; + } + } +} + +describe("a dApp approval racing a stale Settings page's later save", () => { + test("the fresh approval survives Settings revoking an unrelated site", async () => { + const storage = makeStorage(); + await storage.set({ + autistmask: { + wallets: [W1], + allowedSites: { [ADDR2]: ["other.example"] }, + }, + }); + + // Settings loads first, and its save lands last — before either has + // any idea a dApp approval happened elsewhere in between. + const settings = loadPage(storage); + await settings.state.loadState(); + + // A dApp approval window, opened later, approves a new site for a + // different address and saves — the real sequence at + // src/background/index.js:592-599. + const approval = loadPage(storage); + await approval.state.loadState(); + approveSite(approval.state.state, ADDR1, "dapp.example"); + await approval.state.saveState(); + expect( + (await storage.get("autistmask")).autistmask.allowedSites[ADDR1], + ).toEqual(["dapp.example"]); + + // Settings revokes its own, unrelated site — the real sequence at + // src/popup/views/settings.js:55-68 — and saves from state loaded + // before the dApp approval ever happened. + revokeSite(settings.state.state, "other.example"); + await settings.state.saveState(); + + const persisted = (await storage.get("autistmask")).autistmask; + expect(persisted.allowedSites[ADDR1]).toEqual(["dapp.example"]); + expect(persisted.allowedSites[ADDR2]).toBeUndefined(); + }); +}); + +describe("a revoked site permission against a stale page's later save", () => { + test("the revocation holds even when the stale page approves something else", async () => { + const storage = makeStorage(); + await storage.set({ + autistmask: { + wallets: [W1], + allowedSites: { [ADDR1]: ["evil.example"] }, + }, + }); + + // A stale page loads while the permission still stands. + const stale = loadPage(storage); + await stale.state.loadState(); + + // Settings revokes it — src/popup/views/settings.js:55-68 — from a + // second page. + const settings = loadPage(storage); + await settings.state.loadState(); + revokeSite(settings.state.state, "evil.example"); + await settings.state.saveState(); + expect( + (await storage.get("autistmask")).autistmask.allowedSites[ADDR1], + ).toBeUndefined(); + + // The stale page, unaware of the revoke, approves an unrelated site + // for a different address and saves — src/background/index.js:592-599. + approveSite(stale.state.state, ADDR2, "good.example"); + await stale.state.saveState(); + + const persisted = (await storage.get("autistmask")).autistmask; + expect(persisted.allowedSites[ADDR2]).toEqual(["good.example"]); + expect(persisted.allowedSites[ADDR1]).toBeUndefined(); + }); +}); + +// mergeListByIdentity()'s identity function is not guaranteed collision-free +// — walletIdentity() falls back to one shared "addr:" value for any wallet +// with neither an xpub nor a populated first address (a legacy or corrupt +// record). Two such records created independently on two different pages +// must not silently collapse into one, dropping the loser's +// encryptedSecret with no error and no log. +function legacyWallet(name, secret) { + return { + type: "legacy", + name, + encryptedSecret: secret, + nextIndex: 0, + addresses: [], + }; +} + +describe("two wallets independently created with a colliding identity", () => { + test("both survive, encryptedSecret included, instead of one silently replacing the other", async () => { + const storage = makeStorage(); + await storage.set({ autistmask: { wallets: [W1] } }); + + // Both pages load before either has created their malformed wallet, + // so neither has baseline knowledge of the other's. + const pageA = loadPage(storage); + await pageA.state.loadState(); + const pageB = loadPage(storage); + await pageB.state.loadState(); + + pageA.state.state.wallets.push(legacyWallet("Legacy A", "secret-a")); + pageA.state.state.hasWallet = true; + await pageA.state.saveState(); + expect( + (await storage.get("autistmask")).autistmask.wallets, + ).toHaveLength(2); + + pageB.state.state.wallets.push(legacyWallet("Legacy B", "secret-b")); + pageB.state.state.hasWallet = true; + await pageB.state.saveState(); + + const persisted = (await storage.get("autistmask")).autistmask; + const secrets = persisted.wallets.map((w) => w.encryptedSecret); + expect(secrets).toContain("secret-one"); + expect(secrets).toContain("secret-a"); + expect(secrets).toContain("secret-b"); + }); +});