Five defects traced to one fact: src/background/index.js read and wrote the
module-level `state` singleton in src/shared/state.js, which the MV3 service
worker never populates and which answered an unpopulated read out of
DEFAULT_STATE in silence. Every previous fix added a loadState() before the
access, and that is what produced the fifth: a load detaches the objects an
in-flight handler is holding.
So the reachability goes rather than a sixth call site.
The background now has its own storage layer, src/background/state.js:
getState() is a detached, normalized per-call read, and updateState() is a
queued read-modify-write whose read is one storage round trip ahead of its
write. Nothing in the background holds an in-memory copy of the profile.
- Every handler takes one snapshot and answers from it, including the address
it names: activeAddressOf(s) replaced a second, later storage read that
could disagree with the first.
- wallet_switchEthereumChain applies applyChainSwitchFields() (split out of
chainSwitch.js, which keeps the singleton path for the popup) inside
updateState() instead of calling onChainSwitch() on the singleton.
- The remembered site decision is a read-modify-write, not a load-mutate-save
around a prompt the user takes seconds to answer.
- backgroundRefresh() refreshes a private copy of the wallets and applies the
balances that came back by address, so it never publishes an object other
in-flight work holds, and a wallet added or deleted during the round trip
survives its write.
- The transaction attempt takes its chain id and its endpoint from the same
snapshot. They used to come from different moments, so a chain switch
committed in between moved the endpoint under an artifact already verified
against the old chain.
getProvider(rpcUrl, networkId) now REQUIRES the network id and validates it
against networks.js. That closes the cold-worker wrong-chain send at its shape
rather than at one call site: the hint used to default to currentNetwork() off
the unpopulated singleton, so the endpoint was the user's chain and ethers
fixed chainId at 0x1, and the wallet's own verifySignedTx then refused every
non-mainnet dApp send. refreshBalances(), lookupTokenInfo(), scanForAddresses()
and resolveEnsName() carry the id through; balances.js no longer requires
state.js at all.
The prohibition is enforced mechanically, not by review, and it is enforced by
the bundler rather than by a guess at what the bundler does. The table of
modules an entry point's bundle may not contain lives in
script/lib/forbiddenBundleInputs.js — one copy, read by both layers that act on
it — and build.js's assertNoForbiddenInputs() fails the build when esbuild's
metafile reports src/shared/state.js as an input of a background bundle, naming
the import chain from the metafile's own graph. That is the resolution the
shipped bundle was built from, so no specifier syntax, no hop and no resolution
rule can slip past it; Dockerfile:42 runs make build, so it holds in CI.
Both halves of the table are checked for rot, because either one turns the
prohibition into a pass that checks nothing: a key no bundled entry point
matched fails, and so does a listed module this build bundled nowhere. The
second is what makes a rename of src/shared/state.js loud instead of silently
disarming the check, and it is stronger than an existsSync() because it also
fails when the module is still there but has dropped out of every bundle. What
the assertion does NOT cover is a COPY of the singleton at another path: it is
keyed by path, so a copy builds and lints clean. That is stated where the table
lives, with the reason it is accepted — a copy carries the singleton's own
guard, so a background read of it throws rather than being served DEFAULT_STATE.
make check does not run make build, so the assertion is unit tested against
synthetic metafiles in tests/buildForbiddenInputs.test.js: build.js runs its
build() only as a program now and exports the checks. Executing a check in CI
is not testing it — without that file, inverting the condition leaves every
check in this repo green with the singleton back in the worker.
A custom ESLint rule walks the CommonJS require graph from every src/background/
file and reports the same thing in the editor, before a full bundle. It reads
the same table, and it matches specifiers textually, so it is best-effort fast
feedback and not the guarantee — two earlier revisions of it shipped holes (a
template literal, a dynamic import(), a comment inside the call, a directory
resolved through package.json main). Those are covered now and pinned by
tests/backgroundStateLintRule.test.js. Two shapes it does not report are
recorded in its header as known divergences the build catches, both measured: a
computed specifier (require("../shared/" + "state"), which esbuild
constant-folds into the bundle) and a symlink to the module (esbuild reports the
real path). Each is make lint exit 0 and make build exit 2.
Reading a persisted field of the singleton before any load now throws
StateNotLoadedError instead of serving DEFAULT_STATE.
Test stubs: chrome.storage.local is a serialization boundary, and eight files
stubbed it with an aliasing get, so the object a module held and the object
"storage" held were one object — an assertion could pass on a build that never
wrote anything. Every test that drives real persistence now goes through
tests/support/storageStub.js, which structured-clones in both directions.
closes #320
424 lines
17 KiB
JavaScript
424 lines
17 KiB
JavaScript
// 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 shared 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).
|
|
|
|
const { makeStorageStub } = require("./support/storageStub");
|
|
|
|
// 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.local } };
|
|
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 = makeStorageStub();
|
|
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 = makeStorageStub();
|
|
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 = makeStorageStub();
|
|
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 = makeStorageStub();
|
|
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 = makeStorageStub();
|
|
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 = makeStorageStub();
|
|
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 = makeStorageStub();
|
|
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");
|
|
});
|
|
});
|