A persisted container was checked while its ENTRIES were dereferenced
unchecked. A stored `{"0x…": "notalist"}` in allowedSites passes the state
gate, renders a working popup, and then throws inside saveState()'s per-
hostname merge, so every save from that moment on fails while the UI looks
entirely healthy. deniedSites has the identical shape; fraudContracts is the
same class with a milder consequence.
The sweep for that class found four more:
- selectedToken, dereferenced as text behind a truthiness-only restore gate.
- rpcUrl, handed whole to `new JsonRpcProvider()` by getProvider(), which
throws SYNCHRONOUSLY for a non-string — from txStatus.js and addWallet.js,
neither inside a try, and the first reachable from a stored
`currentView: "wait-tx"` through the unguarded restoreView().
- The ENTRIES of viewData. Four restore branches gate on one truthy field and
hand the rest to a renderer that calls address.toLowerCase(): a stored
`{"currentView":"success-tx","viewData":{"hash":"0x1"}}` throws out of
restoreView(), skipping the rest of popup init.
- selectedWallet / selectedAddress. `wallets` is a real Array, so a stored
"map", "length", "constructor" or "__proto__" is TRUTHY: hasValidAddress()'s
`&&` does not short-circuit and `.addresses[…]` throws. A stale INTEGER index
is the safe case.
Floors, in src/shared/persistedState.js: allowedSites/deniedSites through
siteMap(), fraudContracts and each hostname list through textList(),
selectedToken and activeAddress as text-or-null, rpcUrl and blockscoutUrl as
non-empty text, selectedWallet and selectedAddress as a non-negative integer
or null, and each networkEndpoints pair's two URL fields — which
applyChainSwitchFields() assigns straight onto s.rpcUrl on the next switch.
Guards, in src/popup/viewRouter.js: the four restore branches that gate on one
truthy field now check the entries their renderer dereferences, as
txStatus.restoreWait() has always done for wait-tx. "confirm-tx" joins
ADDRESS_VIEWS, because its Sign button dereferences
state.wallets[state.selectedWallet] behind no guard of its own.
A stored own "__proto__" key is dropped by siteMap(): it can never be a wallet
address, so it grants nothing, and keeping it only keeps a value the next save
would hand to the prototype setter. networkEndpoints keeps unknown keys by
design, so mergeMapByKey() in src/shared/state.js now writes with
defineProperty as well — the guard in the floor was being undone one layer
downstream.
A save that fails is also told, not merely repaired: onSaveFailure() reports
every failed save, awaited or not (the save queue's own rejection handler is
what made a failure vanish), and the popup raises a persistent "NOT SAVED"
banner naming the reason. doRefreshAndRender() no longer rejects, since every
one of its call sites fires it and walks away.
The per-field justification in the header of src/shared/stateSchema.js is
replaced by tests/persistedFieldContract.test.js. That comment shipped a false
claim in three consecutive changes; the artifact was the problem. The test is
one row per persisted field, declaring the property that field's floor is
claimed to have and PROVING it by driving the real code with hostile values —
the gate for a field the gate refuses, normalizePersisted() for a field it
floors, the real JsonRpcProvider constructor for rpcUrl, and — for every field
whose only defence is that nothing dereferences it structurally — a boot of the
real popup entry point over that value onto EVERY view the popup can reopen
onto.
That last part is what makes the claim falsifiable, and it is why this defect
class is worth a harness at all: it lives on the RESTORE path and not on Home.
So the suite goes red whenever one of those boots reaches a structural
dereference on the view it restored onto — including one that takes TWO
corrupted fields at once, because the verdict is the combined boot itself and
the per-field re-boot that names a culprit can only decorate the message.
Every swept field is driven at both polarities, or proven unable to be falsy
after the floor: a value nothing in src/ writes is a wrong-typed one and
therefore truthy, so without a falsy slot a dereference behind `if (!state.x)`
is never reached on the very boot that corrupts x, and for three of these
fields the falsy answer is the DEFAULT_STATE default — the branch every
ordinary install takes. It goes red too on a field that gains a floor while its
row still claims it has none, and on a field added to PERSISTED_FIELDS with no
row.
What it does NOT drive, stated accurately rather than claiming total coverage:
every combination. Four value combinations per view are driven, not the product
of the twelve swept fields. The last of the four is itself a MIX rather than a
uniform polarity — every falsy-capable field is falsy on it while the ones that
cannot be falsy stay hostile-truthy — so many two-field interactions are driven
and fatal; one needing a pairing none of the four produces is not driven at
all. Nor is anything no stored record reaches by itself: a view only forward
navigation opens, and anything behind a click. The header and the README mirror
now point at the test instead of restating it.
The boots are cheap enough to keep by construction rather than by sampling.
Every field the router itself reads is driven onto each view individually,
since a hostile value in one of those legitimately changes which view renders;
every other unfloored field is corrupted on the SAME boot, and that boot has to
land on the view it stored — so a field that does move the routing cannot hide
in the crowd, and the failure path re-boots one field at a time to name a
culprit without ever being able to clear the failure. That is forty-four boots
instead of several hundred; the suite runs in 12.8s against a 30s cap.
The polarity guard counts only values driven onto EVERY restorable view. A
hostileRestore entry may carry `views: [...]`, and counting one would let a
future row satisfy the guard with a polarity that reaches a single renderer.
No current row does; this keeps it that way.
The DOM stub in tests/support/popupBoot.js gained one thing to make any of that
possible: an element's parentElement. Without it success-tx and transaction
threw on the first line that hides a field's wrapper, so neither renderer could
be booted onto at all — every boot aimed at them fell back to Home instead, and
the base profile the sweep starts from is now asserted to render each view
rather than fall back, so that cannot go unnoticed again.
188 lines
6.8 KiB
JavaScript
188 lines
6.8 KiB
JavaScript
// Every element id the popup views look up must exist in the markup they
|
|
// look it up in.
|
|
//
|
|
// The failure this catches: `$("settings-hide-dsut")` is valid JavaScript
|
|
// referring to a defined function, so neither jest (node environment, no
|
|
// DOM) nor a linter has anything to object to. At runtime `$()` returns
|
|
// null and the next property access throws, which in `init()` aborts the
|
|
// rest of that view's wiring and takes the whole screen down. Settings is
|
|
// the densest concentration of these lookups in the codebase.
|
|
//
|
|
// This is the cheap general half of the guard: it runs in `make check`
|
|
// with no browser and covers every id in every view, not the ones some
|
|
// test happens to click. The expensive specific half is the Settings
|
|
// section of the end-to-end suite (tests/e2e/run.js), which proves the
|
|
// screen actually comes up and its controls work.
|
|
//
|
|
// Scope and limits, stated rather than implied:
|
|
// - Only literal string arguments are resolvable statically. A call
|
|
// like `$(containerId)` is invisible here; those are covered by the
|
|
// e2e run instead.
|
|
// - `document.getElementById()` is checked too, minus the ids listed in
|
|
// RUNTIME_CREATED_IDS, which name nodes the code creates itself and
|
|
// which are legitimately absent from the static markup.
|
|
|
|
"use strict";
|
|
|
|
const fs = require("fs");
|
|
const path = require("path");
|
|
|
|
const POPUP_DIR = path.join(__dirname, "..", "src", "popup");
|
|
const POPUP_HTML_PATH = path.join(POPUP_DIR, "index.html");
|
|
|
|
// Nodes built at runtime rather than authored in index.html. Each one must
|
|
// be created unconditionally by the code before it is ever looked up.
|
|
const RUNTIME_CREATED_IDS = new Set([
|
|
// Created by updateDebugBanner() in src/popup/views/helpers.js.
|
|
"debug-banner",
|
|
// Created by showSaveFailureBanner() in the same file, on the first save
|
|
// that fails. Absent from the markup on purpose: a popup where nothing has
|
|
// failed must not have to carry an empty banner
|
|
// (https://git.eeqj.de/sneak/AutistMask/issues/362).
|
|
"save-failure-banner",
|
|
]);
|
|
|
|
// Every id lookup the popup performs with a literal argument, as
|
|
// {id, file, line, source} records.
|
|
//
|
|
// showView("x") is included because it resolves to the element id
|
|
// "view-x": a view name with no matching section is the same defect one
|
|
// indirection further out.
|
|
const PATTERNS = [
|
|
{ re: /\$\(\s*"([^"\n]+)"\s*\)/g, id: (m) => m[1], source: "$()" },
|
|
{
|
|
re: /document\.getElementById\(\s*"([^"\n]+)"\s*\)/g,
|
|
id: (m) => m[1],
|
|
source: "getElementById()",
|
|
},
|
|
{
|
|
re: /\b(?:showError|hideError)\(\s*"([^"\n]+)"/g,
|
|
id: (m) => m[1],
|
|
source: "showError()/hideError()",
|
|
},
|
|
{
|
|
re: /\bshowView\(\s*"([^"\n]+)"\s*\)/g,
|
|
id: (m) => "view-" + m[1],
|
|
source: "showView()",
|
|
},
|
|
];
|
|
|
|
function jsFilesUnder(dir) {
|
|
const out = [];
|
|
for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
|
|
const full = path.join(dir, entry.name);
|
|
if (entry.isDirectory()) {
|
|
out.push(...jsFilesUnder(full));
|
|
} else if (entry.name.endsWith(".js")) {
|
|
out.push(full);
|
|
}
|
|
}
|
|
return out.sort();
|
|
}
|
|
|
|
function lineOf(text, index) {
|
|
return text.slice(0, index).split("\n").length;
|
|
}
|
|
|
|
function collectReferences() {
|
|
const refs = [];
|
|
for (const file of jsFilesUnder(POPUP_DIR)) {
|
|
const text = fs.readFileSync(file, "utf8");
|
|
const rel = path.relative(path.join(__dirname, ".."), file);
|
|
for (const { re, id, source } of PATTERNS) {
|
|
re.lastIndex = 0;
|
|
let m;
|
|
while ((m = re.exec(text)) !== null) {
|
|
refs.push({
|
|
id: id(m),
|
|
file: rel,
|
|
line: lineOf(text, m.index),
|
|
source,
|
|
});
|
|
}
|
|
}
|
|
}
|
|
return refs;
|
|
}
|
|
|
|
function collectHtmlIds(html) {
|
|
const ids = [];
|
|
const re = /\bid="([^"]+)"/g;
|
|
let m;
|
|
while ((m = re.exec(html)) !== null) ids.push(m[1]);
|
|
return ids;
|
|
}
|
|
|
|
const HTML = fs.readFileSync(POPUP_HTML_PATH, "utf8");
|
|
const HTML_IDS = collectHtmlIds(HTML);
|
|
const HTML_ID_SET = new Set(HTML_IDS);
|
|
const REFERENCES = collectReferences();
|
|
|
|
describe("every element id the popup looks up exists in its markup", () => {
|
|
// A guard that found nothing to check would pass forever. If a
|
|
// refactor renames the directory, changes the helper, or moves the
|
|
// markup, this fails instead of quietly covering zero call sites.
|
|
// The floors are far below the counts measured when this was written
|
|
// (434 lookups across 20 of the 24 files under src/popup/, against 274
|
|
// ids in the markup), so ordinary churn does not trip them.
|
|
test("the scan actually found the code and the markup", () => {
|
|
const files = new Set(REFERENCES.map((r) => r.file));
|
|
expect(files.size).toBeGreaterThanOrEqual(15);
|
|
expect(REFERENCES.length).toBeGreaterThanOrEqual(300);
|
|
expect(HTML_IDS.length).toBeGreaterThanOrEqual(200);
|
|
|
|
// The densest screen, named explicitly: a scan that stopped
|
|
// covering src/popup/views/settings.js is the exact regression
|
|
// this file was written for.
|
|
expect(
|
|
files.has(path.join("src", "popup", "views", "settings.js")),
|
|
).toBe(true);
|
|
expect(
|
|
REFERENCES.some((r) => r.id === "settings-hide-spoofed-symbols"),
|
|
).toBe(true);
|
|
expect(REFERENCES.some((r) => r.id === "view-settings")).toBe(true);
|
|
});
|
|
|
|
test("no lookup names an id that src/popup/index.html does not define", () => {
|
|
const missing = REFERENCES.filter(
|
|
(r) => !HTML_ID_SET.has(r.id) && !RUNTIME_CREATED_IDS.has(r.id),
|
|
).map(
|
|
(r) =>
|
|
r.file +
|
|
":" +
|
|
r.line +
|
|
" " +
|
|
r.source +
|
|
' looks up id "' +
|
|
r.id +
|
|
'", which is not in src/popup/index.html',
|
|
);
|
|
|
|
expect(missing).toEqual([]);
|
|
});
|
|
|
|
test("every id excused as runtime-created is still looked up somewhere", () => {
|
|
// Otherwise the exception list becomes a place stale names
|
|
// accumulate, and the next real miss can be waved through by
|
|
// adding one more.
|
|
for (const id of RUNTIME_CREATED_IDS) {
|
|
expect(REFERENCES.some((r) => r.id === id)).toBe(true);
|
|
expect(HTML_ID_SET.has(id)).toBe(false);
|
|
}
|
|
});
|
|
|
|
test("index.html defines no id twice", () => {
|
|
// getElementById returns the first match, so a duplicate id means
|
|
// one of the two elements can never be reached by the code that
|
|
// thinks it owns it.
|
|
const seen = new Set();
|
|
const duplicated = [];
|
|
for (const id of HTML_IDS) {
|
|
if (seen.has(id)) duplicated.push(id);
|
|
seen.add(id);
|
|
}
|
|
|
|
expect(duplicated).toEqual([]);
|
|
});
|
|
});
|