harden: make the background physically unable to read the shared state singleton (closes #324)
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: a custom ESLint rule walks the CommonJS require graph from every src/background/ file and fails the lint when src/shared/state.js is reachable, naming the chain. A re-export from any shared module cannot put the singleton back in the bundle unnoticed. 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. They all go through tests/support/storageStub.js now, which structured-clones in both directions. closes #320
This commit is contained in:
130
script/lib/eslint/noStateSingletonInBackground.js
Normal file
130
script/lib/eslint/noStateSingletonInBackground.js
Normal file
@@ -0,0 +1,130 @@
|
||||
// ESLint rule: the background bundle may not reach the shared state singleton.
|
||||
//
|
||||
// src/shared/state.js holds a module-level `state` object, loaded once by
|
||||
// loadState() and mutated in place from then on. That is the popup's model. In
|
||||
// the MV3 service worker there is no "once": the worker is terminated when
|
||||
// idle and revived by the next message, nothing loads state at module scope,
|
||||
// and an unpopulated read used to be served DEFAULT_STATE without complaint —
|
||||
// five defects, one cause
|
||||
// (https://git.eeqj.de/sneak/AutistMask/issues/324). The background has its
|
||||
// own per-call storage layer in src/background/state.js instead.
|
||||
//
|
||||
// A convention nobody can violate beats a convention everyone remembers, so
|
||||
// this is a lint error rather than a review item. It checks REACHABILITY, not
|
||||
// just the direct require: the singleton is one `require()` away from any
|
||||
// shared module the background pulls in, and a re-export would put it back in
|
||||
// the bundle without any background file naming it. So each background file is
|
||||
// the root of a walk over the CommonJS require graph, and the error names the
|
||||
// whole chain that brought the singleton in.
|
||||
//
|
||||
// The walk reads sources from disk and matches `require("...")` textually.
|
||||
// That over-approximates — a require inside a comment or a string counts — and
|
||||
// over-approximating is the safe direction for a prohibition: the failure mode
|
||||
// is a spurious error naming an exact file and line, not a silent hole.
|
||||
|
||||
const fs = require("fs");
|
||||
const path = require("path");
|
||||
|
||||
// The module this rule exists to keep out, relative to the repo root.
|
||||
const FORBIDDEN = path.join("src", "shared", "state.js");
|
||||
|
||||
const REQUIRE_RE = /\brequire\(\s*["']([^"']+)["']\s*\)/g;
|
||||
|
||||
// Resolve a relative require to a file path, trying the extensions node would.
|
||||
function resolveRelative(fromFile, spec) {
|
||||
if (!spec.startsWith(".")) return null; // a package, not our tree
|
||||
const base = path.resolve(path.dirname(fromFile), spec);
|
||||
for (const candidate of [
|
||||
base,
|
||||
base + ".js",
|
||||
base + ".json",
|
||||
path.join(base, "index.js"),
|
||||
]) {
|
||||
try {
|
||||
if (fs.statSync(candidate).isFile()) return candidate;
|
||||
} catch {
|
||||
// Not this candidate.
|
||||
}
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
function requiresOf(file) {
|
||||
let source;
|
||||
try {
|
||||
source = fs.readFileSync(file, "utf8");
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
const out = [];
|
||||
for (const match of source.matchAll(REQUIRE_RE)) {
|
||||
const resolved = resolveRelative(file, match[1]);
|
||||
if (resolved) out.push(resolved);
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
// Breadth-first from `entry`, returning the shortest chain of files that ends
|
||||
// at the forbidden module, or null when it is not reachable.
|
||||
function chainToForbidden(entry, forbidden) {
|
||||
const seen = new Set([entry]);
|
||||
const queue = [[entry]];
|
||||
while (queue.length > 0) {
|
||||
const chain = queue.shift();
|
||||
for (const next of requiresOf(chain[chain.length - 1])) {
|
||||
if (next === forbidden) return chain.concat([next]);
|
||||
if (seen.has(next)) continue;
|
||||
seen.add(next);
|
||||
queue.push(chain.concat([next]));
|
||||
}
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
const rule = {
|
||||
meta: {
|
||||
type: "problem",
|
||||
docs: {
|
||||
description:
|
||||
"the background bundle must not be able to reach the" +
|
||||
" module-level state singleton in src/shared/state.js",
|
||||
},
|
||||
schema: [],
|
||||
messages: {
|
||||
reachable:
|
||||
"The background must not reach the shared state singleton:" +
|
||||
" {{chain}}. The MV3 worker never populates it, so reading it" +
|
||||
" serves DEFAULT_STATE. Use getState()/updateState() from" +
|
||||
" src/background/state.js instead.",
|
||||
},
|
||||
},
|
||||
|
||||
create(context) {
|
||||
return {
|
||||
"Program:exit"(node) {
|
||||
const filename = context.filename;
|
||||
// ESLint lints from the repo root, which is also where the
|
||||
// forbidden path is anchored.
|
||||
const forbidden = path.resolve(context.cwd, FORBIDDEN);
|
||||
const chain = chainToForbidden(
|
||||
path.resolve(filename),
|
||||
forbidden,
|
||||
);
|
||||
if (!chain) return;
|
||||
context.report({
|
||||
node,
|
||||
messageId: "reachable",
|
||||
data: {
|
||||
chain: chain
|
||||
.map((file) => path.relative(context.cwd, file))
|
||||
.join(" -> "),
|
||||
},
|
||||
});
|
||||
},
|
||||
};
|
||||
},
|
||||
};
|
||||
|
||||
module.exports = {
|
||||
rules: { "no-state-singleton-in-background": rule },
|
||||
};
|
||||
Reference in New Issue
Block a user