harden: stop the background reading the shared state singleton, and enforce it at build time (closes #324)
Five defects, one of which destroyed every wallet, came from src/background reading and writing the module-level state singleton the MV3 worker never populates, which silently served DEFAULT_STATE. Each point fix created the next defect. The background now has its own per-call getState() and a queued read-modify-write updateState(); the singleton is unreachable from it, and an unpopulated read throws instead of serving defaults. The prohibition is enforced by the build, not by review: build.js asserts over esbuild's own metafile that no forbidden module is an input of a background bundle, so every specifier syntax esbuild resolves is covered, and both halves of the table are checked for rot -- a stale key, a stale module, an empty list, or an unlisted entry point under src/background/ all fail the build. The ESLint rule remains as fast local feedback and reads the same shared table. Known bounds are documented where the table lives. Also closes #320: getProvider() now requires a validated network id, so a cold worker no longer prepares a non-mainnet dApp transaction for mainnet and gets refused by the wallet's own verifier. backgroundRefresh() no longer mutates address objects across a network round trip, the broadcast path takes its endpoint and chain id from one snapshot, and eight test storage stubs now structured-clone on get as the real chrome.storage.local does. closes #320
This commit was merged in pull request #344.
This commit is contained in:
270
tests/backgroundStateLintRule.test.js
Normal file
270
tests/backgroundStateLintRule.test.js
Normal file
@@ -0,0 +1,270 @@
|
||||
// The lint rule that keeps src/shared/state.js out of the background bundle
|
||||
// (script/lib/eslint/noStateSingletonInBackground.js).
|
||||
//
|
||||
// Five defects, one of which destroyed a wallet, came from background code
|
||||
// reaching that singleton, and each point fix created the next site
|
||||
// (https://git.eeqj.de/sneak/AutistMask/issues/324).
|
||||
//
|
||||
// What this file does NOT do is establish that the singleton cannot reach the
|
||||
// background bundle. That is build.js's FORBIDDEN_INPUTS assertion, which reads
|
||||
// esbuild's metafile and so cannot be evaded by a syntax a matcher does not
|
||||
// know; it is pinned by tests/buildForbiddenInputs.test.js. The rule under test
|
||||
// here is fast local feedback in front of that, and these cases pin the shapes
|
||||
// it is known to catch, so a regression in the matcher is a failing test rather
|
||||
// than a quietly narrower rule.
|
||||
//
|
||||
// Every shape below was measured against a real `make build`: each one puts
|
||||
// state.js in the shipped background bundles, and each one was invisible to
|
||||
// some earlier revision of the matcher — the quoted-only regex missed the
|
||||
// backtick, the dynamic import and the `from` clause; its successor missed a
|
||||
// comment inside the call and a directory resolved through package.json `main`.
|
||||
//
|
||||
// Two shapes the rule does NOT report are pinned below as non-reports, in
|
||||
// "the divergences from the build's answer": a computed specifier such as
|
||||
// `require("../shared/" + "state")`, which esbuild constant-folds, and a
|
||||
// symlink to the module, whose real path esbuild reports. Both put state.js in
|
||||
// the shipped background bundle and both are `make build` exit 2 with
|
||||
// `make lint` exit 0 (measured). Pinning them as non-reports is what makes the
|
||||
// rule's stated bounds a measured description rather than a claim: if either
|
||||
// starts being reported, or the matcher is widened until one is, a test says
|
||||
// so. Their catch is the build's, and is pinned in
|
||||
// tests/buildForbiddenInputs.test.js against the metafile that catches it.
|
||||
|
||||
const fs = require("fs");
|
||||
const os = require("os");
|
||||
const path = require("path");
|
||||
const { Linter } = require("eslint");
|
||||
|
||||
const plugin = require("../script/lib/eslint/noStateSingletonInBackground");
|
||||
|
||||
const RULE = "background/no-state-singleton-in-background";
|
||||
|
||||
// The three files a fixture tree always has. `src/background/index.js` is
|
||||
// supplied per case; the other two stand in for the real modules.
|
||||
const SHARED_STATE = "const state = {};\nmodule.exports = { state };\n";
|
||||
const SHARED_HOP =
|
||||
"// A shared module the background legitimately imports.\n" +
|
||||
"module.exports = { applyChainSwitchFields() {} };\n";
|
||||
|
||||
let roots = [];
|
||||
|
||||
function fixture(files) {
|
||||
const root = fs.realpathSync(
|
||||
fs.mkdtempSync(path.join(os.tmpdir(), "autistmask-state-rule-")),
|
||||
);
|
||||
roots.push(root);
|
||||
const tree = {
|
||||
"src/shared/state.js": SHARED_STATE,
|
||||
"src/shared/chainSwitchFields.js": SHARED_HOP,
|
||||
...files,
|
||||
};
|
||||
for (const [rel, source] of Object.entries(tree)) {
|
||||
const abs = path.join(root, rel);
|
||||
fs.mkdirSync(path.dirname(abs), { recursive: true });
|
||||
fs.writeFileSync(abs, source);
|
||||
}
|
||||
return root;
|
||||
}
|
||||
|
||||
// Run the rule exactly as eslint.config.js runs it, over a real tree: the walk
|
||||
// reads its sources from disk, so a virtual RuleTester would not exercise it.
|
||||
// `sourceType` is the fixture's own, not the rule's business: the walk is
|
||||
// textual and never parses the files it follows. The two ESM cases below pass
|
||||
// "module" only so espree can parse the fixture at all — in this repo those
|
||||
// shapes are also a parse error under the commonjs config, but the rule must
|
||||
// not be left depending on that.
|
||||
function lintBackground(root, { sourceType = "commonjs" } = {}) {
|
||||
const file = path.join(root, "src/background/index.js");
|
||||
const linter = new Linter({ cwd: root });
|
||||
return linter.verify(
|
||||
fs.readFileSync(file, "utf8"),
|
||||
{
|
||||
plugins: { background: plugin },
|
||||
languageOptions: { ecmaVersion: 2024, sourceType },
|
||||
rules: { [RULE]: "error" },
|
||||
},
|
||||
file,
|
||||
);
|
||||
}
|
||||
|
||||
function chainOf(messages) {
|
||||
expect(messages).toHaveLength(1);
|
||||
expect(messages[0].ruleId).toBe(RULE);
|
||||
// "...singleton: <chain>. The MV3 worker..." — the chain is what the
|
||||
// message exists to hand the reader, so assert on it rather than on the
|
||||
// fact that something was reported.
|
||||
return messages[0].message.split("singleton: ")[1].split(". The MV3")[0];
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
for (const root of roots) fs.rmSync(root, { recursive: true, force: true });
|
||||
roots = [];
|
||||
});
|
||||
|
||||
describe("the specifier syntaxes the matcher is known to catch", () => {
|
||||
test("a quoted require", () => {
|
||||
const root = fixture({
|
||||
"src/background/index.js":
|
||||
'const { state } = require("../shared/state");\n' +
|
||||
"module.exports = { state };\n",
|
||||
});
|
||||
expect(chainOf(lintBackground(root))).toBe(
|
||||
"src/background/index.js -> src/shared/state.js",
|
||||
);
|
||||
});
|
||||
|
||||
test("a backtick require", () => {
|
||||
const root = fixture({
|
||||
"src/background/index.js":
|
||||
"const { state } = require(`../shared/state`);\n" +
|
||||
"module.exports = { state };\n",
|
||||
});
|
||||
expect(chainOf(lintBackground(root))).toBe(
|
||||
"src/background/index.js -> src/shared/state.js",
|
||||
);
|
||||
});
|
||||
|
||||
test("a dynamic import inside an async function", () => {
|
||||
const root = fixture({
|
||||
"src/background/index.js":
|
||||
"async function readState() {\n" +
|
||||
' const m = await import("../shared/state");\n' +
|
||||
" return m.state;\n" +
|
||||
"}\n" +
|
||||
"module.exports = { readState };\n",
|
||||
});
|
||||
expect(chainOf(lintBackground(root))).toBe(
|
||||
"src/background/index.js -> src/shared/state.js",
|
||||
);
|
||||
});
|
||||
|
||||
test("a static import from-clause", () => {
|
||||
const root = fixture({
|
||||
"src/background/index.js":
|
||||
'import { state } from "../shared/state";\n' +
|
||||
"export { state };\n",
|
||||
});
|
||||
expect(chainOf(lintBackground(root, { sourceType: "module" }))).toBe(
|
||||
"src/background/index.js -> src/shared/state.js",
|
||||
);
|
||||
});
|
||||
|
||||
// `import(/* webpackChunkName: "x" */ "./x")` is the standard bundler
|
||||
// annotation idiom, and prettier leaves both of these exactly as written,
|
||||
// so nothing else in the repo would object to them either.
|
||||
test("a comment between the paren and the specifier", () => {
|
||||
const root = fixture({
|
||||
"src/background/index.js":
|
||||
'globalThis.__probe = require(/* probe */ "../shared/state").state;\n',
|
||||
});
|
||||
expect(chainOf(lintBackground(root))).toBe(
|
||||
"src/background/index.js -> src/shared/state.js",
|
||||
);
|
||||
});
|
||||
|
||||
test("a comment between the specifier and the closing paren", () => {
|
||||
const root = fixture({
|
||||
"src/background/index.js":
|
||||
'globalThis.__probe = require("../shared/state" /* probe */).state;\n',
|
||||
});
|
||||
expect(chainOf(lintBackground(root))).toBe(
|
||||
"src/background/index.js -> src/shared/state.js",
|
||||
);
|
||||
});
|
||||
|
||||
test("a bare side-effect import", () => {
|
||||
const root = fixture({
|
||||
"src/background/index.js": 'import "../shared/state";\n',
|
||||
});
|
||||
expect(chainOf(lintBackground(root, { sourceType: "module" }))).toBe(
|
||||
"src/background/index.js -> src/shared/state.js",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("reachability, not just the direct specifier", () => {
|
||||
// The shape a no-restricted-imports could never see: no background file
|
||||
// names state.js, and the singleton is in the bundle anyway. In a backtick
|
||||
// require, so this fails on the specifier widening as well as on the walk.
|
||||
test("a two-hop re-export through a shared module", () => {
|
||||
const root = fixture({
|
||||
"src/background/index.js":
|
||||
'const { applyChainSwitchFields } = require("../shared/chainSwitchFields");\n' +
|
||||
"module.exports = { applyChainSwitchFields };\n",
|
||||
"src/shared/chainSwitchFields.js":
|
||||
SHARED_HOP +
|
||||
"module.exports.state = require(`./state`).state;\n",
|
||||
});
|
||||
expect(chainOf(lintBackground(root))).toBe(
|
||||
"src/background/index.js -> src/shared/chainSwitchFields.js" +
|
||||
" -> src/shared/state.js",
|
||||
);
|
||||
});
|
||||
|
||||
// Resolution, not syntax: the specifier names a directory, and the file it
|
||||
// resolves to is chosen by that directory's package.json `main`. A walk
|
||||
// that only tries `<dir>/index.js` stops on a specifier it matched.
|
||||
test("a directory resolved through its package.json main", () => {
|
||||
const root = fixture({
|
||||
"src/background/index.js":
|
||||
'globalThis.__probe = require("../shared/probepkg").state;\n',
|
||||
"src/shared/probepkg/package.json": '{"main": "./bridge.js"}\n',
|
||||
"src/shared/probepkg/bridge.js":
|
||||
'const { state } = require("../state");\n' +
|
||||
"module.exports = { state };\n",
|
||||
});
|
||||
expect(chainOf(lintBackground(root))).toBe(
|
||||
"src/background/index.js -> src/shared/probepkg/bridge.js" +
|
||||
" -> src/shared/state.js",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// These two are holes in the rule, and they are pinned as holes on purpose:
|
||||
// the build catches both, the rule is fast feedback in front of it, and a
|
||||
// written-down bound that nothing measures is how the previous three rounds of
|
||||
// this change ended up with claims that were false.
|
||||
describe("the divergences from the build's answer", () => {
|
||||
test("a computed specifier is not reported (esbuild folds it; the build fails)", () => {
|
||||
const root = fixture({
|
||||
"src/background/index.js":
|
||||
'globalThis.__probe = require("../shared/" + "state").state;\n',
|
||||
});
|
||||
expect(lintBackground(root)).toEqual([]);
|
||||
});
|
||||
|
||||
test("a symlink to the module is not reported (esbuild reports the real path)", () => {
|
||||
const root = fixture({
|
||||
"src/background/index.js":
|
||||
'const { state } = require("../shared/stateLink");\n' +
|
||||
"module.exports = { state };\n",
|
||||
});
|
||||
fs.symlinkSync(
|
||||
path.join(root, "src/shared/state.js"),
|
||||
path.join(root, "src/shared/stateLink.js"),
|
||||
);
|
||||
expect(lintBackground(root)).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("what the rule must not report", () => {
|
||||
test("a background file that reaches only its own state layer", () => {
|
||||
const root = fixture({
|
||||
"src/background/index.js":
|
||||
'const { getState } = require("./state");\n' +
|
||||
'const { applyChainSwitchFields } = require("../shared/chainSwitchFields");\n' +
|
||||
"module.exports = { getState, applyChainSwitchFields };\n",
|
||||
"src/background/state.js":
|
||||
"async function getState() {}\nmodule.exports = { getState };\n",
|
||||
});
|
||||
expect(lintBackground(root)).toEqual([]);
|
||||
});
|
||||
|
||||
// The tree as it actually stands. This is the assertion that would catch a
|
||||
// widened matcher that resolves something it should not: it runs the rule
|
||||
// over the real background entrypoint, from the real repo root.
|
||||
test("the repository's own background entrypoint", () => {
|
||||
const root = path.resolve(__dirname, "..");
|
||||
expect(lintBackground(root)).toEqual([]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user