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"); + }); +});