From 31b2aa2d8ac8bf79c8db78bc0f32a4b4e6a2cafa Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 20 Aug 2026 14:00:00 +0000 Subject: [PATCH 1/3] 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. --- TODO.md | 26 +++ src/shared/state.js | 350 +++++++++++++++++++++++++-------------- tests/stateMerge.test.js | 214 ++++++++++++++++++++++++ 3 files changed, 467 insertions(+), 123 deletions(-) create mode 100644 tests/stateMerge.test.js 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..34b9e7d 100644 --- a/src/shared/state.js +++ b/src/shared/state.js @@ -88,135 +88,239 @@ 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; +} + +// 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. +// +// This does not make two pages that both change the SAME field concurrently +// safe: last write wins on that one field, same as before. What it removes +// is the cross-field clobber — a page that only navigated overwriting a +// wallet list it never touched. +// +// 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 (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..066833d --- /dev/null +++ b/tests/stateMerge.test.js @@ -0,0 +1,214 @@ +// 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", + ]); + }); +}); -- 2.49.1 From af9568db903a316a1fbbea9e7c27d39afdd8ac54 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 20 Aug 2026 14:14:29 +0000 Subject: [PATCH 2/3] 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. --- src/shared/state.js | 146 +++++++++++++++++++++++++++++++++++++-- tests/stateMerge.test.js | 77 +++++++++++++++++++++ 2 files changed, 218 insertions(+), 5 deletions(-) diff --git a/src/shared/state.js b/src/shared/state.js index 34b9e7d..3ae5d5d 100644 --- a/src/shared/state.js +++ b/src/shared/state.js @@ -235,6 +235,112 @@ function deepEqual(a, b) { 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. +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. +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(ours.map((item) => [identityOf(item), item])); + const result = []; + const seen = new Set(); + + for (const theirItem of theirs) { + const id = identityOf(theirItem); + seen.add(id); + if (baseIndex.has(id) && !oursIndex.has(id)) continue; + if (oursIndex.has(id)) { + result.push( + mergeItem(baseIndex.get(id), oursIndex.get(id), theirItem), + ); + } 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 (never had a baseline to +// diff against) — nothing to merge in that case, this page's own copy wins +// outright. +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; +} + // Read-modify-write, merged per field, rather than one full-blob write. // // Every extension page (the toolbar popup, a dApp approval window, the @@ -249,10 +355,29 @@ function deepEqual(a, b) { // are written; every other field is carried forward from whatever is in // storage right now, which may already be a value another page wrote. // -// This does not make two pages that both change the SAME field concurrently -// safe: last write wins on that one field, same as before. What it removes -// is the cross-field clobber — a page that only navigated overwriting a -// wallet list it never touched. +// `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. Every other persisted field stays a +// whole-field diff: `trackedTokens`/`fraudContracts`/`viewStack` are arrays +// of scalars with no per-element identity to merge by, and no code path +// mutates `networkEndpoints`/`allowedSites`/`deniedSites`/`tokenHolderCache` +// /`viewData` the way backgroundRefresh() mutates wallets (in place, +// concurrently with another page's membership change to the same field), so +// there is no known defect there for the added complexity to earn. +// +// 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. @@ -276,7 +401,18 @@ async function saveStateOnce() { const merged = { ...fresh }; for (const key of PERSISTED_FIELDS) { - if (baseline === null || !deepEqual(current[key], baseline[key])) { + if (key === "wallets") { + merged.wallets = mergeListByIdentity( + baseline ? baseline.wallets : [], + current.wallets, + fresh.wallets, + walletIdentity, + mergeWallet, + ); + } else if ( + baseline === null || + !deepEqual(current[key], baseline[key]) + ) { merged[key] = current[key]; } } diff --git a/tests/stateMerge.test.js b/tests/stateMerge.test.js index 066833d..eec09a9 100644 --- a/tests/stateMerge.test.js +++ b/tests/stateMerge.test.js @@ -212,3 +212,80 @@ describe("the approval-window reproduction", () => { ]); }); }); + +// 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"); + }); +}); -- 2.49.1 From bafb84930c804ceeeea9fd775b0e7993b8eae357 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 20 Aug 2026 14:34:20 +0000 Subject: [PATCH 3/3] fix: merge site permissions and network endpoints structurally, and give the identity merge a collision floor (closes #304) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/shared/state.js | 187 +++++++++++++++++++++++++++++++++++---- tests/stateMerge.test.js | 152 +++++++++++++++++++++++++++++++ 2 files changed, 323 insertions(+), 16 deletions(-) diff --git a/src/shared/state.js b/src/shared/state.js index 3ae5d5d..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, @@ -244,6 +245,15 @@ function deepEqual(a, b) { // 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]; @@ -273,23 +283,56 @@ function addressIdentity(addr) { // 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(ours.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); - if (baseIndex.has(id) && !oursIndex.has(id)) continue; - if (oursIndex.has(id)) { - result.push( - mergeItem(baseIndex.get(id), oursIndex.get(id), theirItem), - ); + 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); } @@ -306,9 +349,13 @@ function mergeListByIdentity(base, ours, theirs, identityOf, mergeItem) { // 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 (never had a baseline to -// diff against) — nothing to merge in that case, this page's own copy wins -// outright. +// 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 }; @@ -341,6 +388,84 @@ function mergeAddress(base, ours, theirs) { 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 @@ -365,13 +490,31 @@ function mergeAddress(base, ours, theirs) { // 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. Every other persisted field stays a -// whole-field diff: `trackedTokens`/`fraudContracts`/`viewStack` are arrays -// of scalars with no per-element identity to merge by, and no code path -// mutates `networkEndpoints`/`allowedSites`/`deniedSites`/`tokenHolderCache` -// /`viewData` the way backgroundRefresh() mutates wallets (in place, -// concurrently with another page's membership change to the same field), so -// there is no known defect there for the added complexity to earn. +// 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 @@ -409,6 +552,18 @@ async function saveStateOnce() { 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]) diff --git a/tests/stateMerge.test.js b/tests/stateMerge.test.js index eec09a9..fb02e73 100644 --- a/tests/stateMerge.test.js +++ b/tests/stateMerge.test.js @@ -289,3 +289,155 @@ describe("background refresh racing a wallet deleted on another page", () => { 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"); + }); +}); -- 2.49.1