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.
This commit is contained in:
214
tests/stateMerge.test.js
Normal file
214
tests/stateMerge.test.js
Normal file
@@ -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",
|
||||
]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user