Closes #219. Implements the decision in the second comment there: vendor and censor the blocklist in the build, remove the runtime fetch.
Disclosure: staleness
Before: the extension re-fetched the live list every 24 hours, so a domain added upstream reached users within a day of being added.
After: the shipped list is exactly as fresh as the last vendoring run that was released. The refresh path is make vendor-blocklist, commit the diff, cut a release — so the window is release cadence, not 24 hours. On a monthly release that is up to about a month; between releases it is unbounded, because nothing updates it on its own.
I think the trade is right and would take it again, for three reasons, but it is a real loss and it is the reviewer's to weigh:
the fetch handed a third party live control over what this wallet warns about, on a security surface, from a repository we do not control;
it told that third party, on every install, that AutistMask is running there;
coverage of new domains was the only thing it bought, and a phishing domain's useful life to an attacker is days, so a 24-hour list was never fresh enough to be relied on as the primary defence anyway. The scam-address list, the Etherscan label lookup and the approval screens are unaffected.
If the answer is that the freshness matters more, the honest fix is a mirror we host (option (a) in the issue body), not restoring the fetch to upstream.
Disclosure: the list shrank, and why
Re-vendoring from current upstream is not a no-op. The committed artifact was a stale snapshot of a list upstream actively prunes:
entries
vendored before
231,357
upstream at the pinned commit
105,757 (105,721 usable)
in both
22,266
added by this change
83,455
dropped by this change
209,091
Upstream removes entries as well as adding them — a dead phishing domain can be re-registered by someone legitimate, and a blocklist that only ever grows eventually warns about innocent sites. Vendoring is a pure function of the pinned source, so this follows upstream exactly rather than keeping a union with whatever happened to be in the file. Net effect: 83,455 domains gain coverage they did not have, 209,091 lose coverage upstream no longer thinks they need. I would not want the union: it is not reproducible from the pin, and it accumulates false positives forever.
36 upstream entries are dropped as unusable and reported by the script: they are path-scoped (sites.google.com/view/...), and hostname matching can never match one. That was equally true before; it is now counted out loud instead of sitting invisibly in the file. They are not counted as newly covered: 22,266 + 83,455 = 105,721 usable.
Disclosure: dist/ size
before
after
src/shared/phishingBlocklist.json
8,733,900
1,691,629
dist/ total
18,858,272
8,949,053
dist/chrome/
9,421,947
4,462,138
background bundle
7,252,814
2,292,536
Does the censoring step alter it? Yes, and it is most of the win. Vendoring the same pinned commit as plaintext domains would be 4,281,644 bytes; as digests it is 1,691,629. The rest of the drop is the smaller upstream list and dropping the unused whitelist.
How the pin works
script/vendor-blocklist fetches src/config.json from an immutable commit, not from a branch, and then asserts the sha256 of the bytes it was served:
A mismatch is a hard failure that writes nothing, because the same commit serving different bytes means substitution somewhere between upstream and here. The script prints the hash it got so the pin can be moved deliberately, and says in as many words not to copy that line back in on trust. Reproducible: two consecutive runs produced byte-identical output (sha256 53aa66a872a9ee... both times); the second reported the file already up to date.
How the censoring works
The upstream JSON carries the name far beyond the URL: 6,475 occurrences, nearly all of them phishing domains impersonating that wallet. Dropping them would remove protection for the single most impersonated brand in the file, which is not acceptable in a wallet.
So the artifact stores digests instead of names: sha256 truncated to 64 bits, 16 hex characters per entry, concatenated in sorted order into one string (src/shared/domainHash.js). Every domain is kept; no domain name is shipped. The same shape pays for itself twice more: a lookup is a binary search over that string, so nothing is built at module load — the MV3 worker re-evaluates the module on every wake — and the file is a fifth of the size. Truncation is deliberate and bounded: over ~10^5 entries the chance that a hostname a user visits collides with an entry it is not is ~10^-14 per lookup, and a deliberate collision buys an attacker a false warning on a site they do not control, not a missed one. Safe Browsing ships 32-bit prefixes and asks a server for the rest; this is 32 bits more, with no server.
The check that runs in make check
script/check-censored scans the working tree and, when there is one, dist/. It is a counting check, not a presence check, and that is not a softening: the provider-shim identifiers reach dist/ inside the shim the issue explicitly blesses, so "the name must not appear in dist/" was never satisfiable. Every occurrence must be one of three documented things, and the check reports and fails on anything else.
Each permitted literal is scoped to the path allowed to carry it — the source file, plus the emitted bundles that source lands in — so the same literal appearing anywhere else fails like any other occurrence. That scoping is what makes the check able to police prose, including its own: it reads the name out of script/vendor-blocklist rather than repeating it, and it does not name it in its header either, so the repo-wide grep the check exists to enforce keeps returning exactly the expected files and the check itself is not one of them.
make check runs it, and inspects dist/ when a build is present; when there is not, it prints a banner saying dist/ was NOT inspected rather than reporting a pass over nothing. make build and make build-debug re-run it with --require-dist, so a build artifact is always covered. Proved by negative tests: a file containing the name planted under dist/chrome/ fails the check, which then names the file and the line; and a permitted literal planted at a path not permitted to carry it fails too.
The third exception, which is a decision I am flagging rather than burying
src/shared/tokenList.js carries the on-chain name for 0xacA92E...35DA, an ERC-20 a user may actually hold. It is allowlisted, so it reaches dist/.
It is not what backs symbol-spoof detection — KNOWN_SYMBOLS is built from symbol and address only (src/shared/tokenList.js:3625-3638), so blanking the name string would cost no spoof protection. What makes it unavoidable is different: src/shared/balances.js:106 and :219 already surface the on-chain name of any token the user holds, and this contract's on-chain name is that string. Censoring the repo cannot stop the wallet displaying it; it would only stop the repo agreeing with the chain.
Dropping the entry outright is the only thing that would remove it, and that would kill MUSD symbol-spoof detection — a real user losing real protection to satisfy a naming rule — so I did not take it unilaterally. If the owner wants it gone, say so and it is a one-line change.
What was deleted with the runtime fetch
updatePhishingList(), refreshPhishingListOnSchedule(), initPhishingList(), loadConfig(), loadDeltaFromStorage(), saveDeltaToStorage(), sanitizeTimestamp(), the delta set, CACHE_TTL_MS, MIN_FETCH_ATTEMPT_INTERVAL_MS, DELTA_STORAGE_KEY, MAX_DELTA_BYTES, the two persisted timestamps and the 256 KiB storage record — the whole machinery from #158 — plus the PHISHING_REFRESH_ALARM and its handler. phishingDomains.js went from 343 lines to 158 and now touches neither the network nor storage. The background worker makes no network request at startup at all, asserted directly in tests/alarms.test.js.
Retiring an alarm is not just deleting its handler: the browser keeps an alarm until something clears it, so every install that ever ran the old version would go on being woken every 24 hours forever with nothing to deliver it to. OBSOLETE_ALARMS in src/shared/alarms.js lists the retired name and ensureRecurringAlarms() clears it on every start, with tests.
Also in this unit
LICENSE no longer cites the AugurProject URL that 404s. It now states what the file actually is — derived, blacklist only, digests not names — and points at the vendoring script for the exact pinned source. Attribution to kumavis and DBAD 1.2 is unchanged.
README.md and docs/README.md: the blocklist is gone from the list of services the extension contacts (one fewer third party that sees a user's IP), the background scheduling section no longer documents a phishing alarm or the delta timestamps, and the e2e section documents the new canary.
eslint.config.js gains one block for script/lib/, which holds node programs the shell entrypoints call and would otherwise lint with no globals at all.
e2e, including a harness change I want reviewed
The DoD asks for the warnings proven in the harness, and there was no such test — nothing in the e2e suite touched the blocklist. There is now: tests/e2e/run.js serves the dApp fixture from myetheywallet.com, a live entry in the shipped artifact, drives eth_requestAccounts from it and requires #approve-site-phishing-warning to be visible. The clean-origin test asserts the same banner is hidden, so an always-visible banner cannot satisfy either. Real list, real origin, real background check, real screen; nothing about the blocklist is stubbed, because there is nothing left to stub.
The interception canary needed a new anchor. It waited for the background worker's own startup fetch — the blocklist fetch — and that fetch no longer exists, which the old code's own error message anticipated ("the worker no longer fetches at startup, in which case this check needs a new anchor"). It now wakes the worker with a message and asks it for one throwaway fetch() of a stub URL, and still aborts the whole suite if that does not reach the route handler. The README warned that a synthetic probe had been tried and killed the worker; that is accurate, and it happened again here on the first attempt — waking the worker first is what fixes it, and it is retried five times before failing. Verified failing closed and passing.
Gates
Measured on 031a70e, rebased onto next at 8fcdd8a:
make check: green. 31 suites, 748 tests; test-verify-build 18/18; check-censored 137 tracked files + 16 under dist/; eslint (in the pinned container, observed running, not CACHED) and prettier clean.
make test-e2e: green, 52/52, including the new #219 case.
make test-e2e-firefox: green, 8/8 (run before the last two rebases).
dist/ negative probes, re-run after the scoping change: a planted bare name under dist/chrome/ turns make check red; a permitted literal planted at a path not permitted to carry it turns it red too, in both the working tree and dist/.
Vendoring run twice: identical bytes.
grep -ri over the tree returns script/vendor-blocklist, src/content/inpage.js and src/shared/tokenList.js, and nothing else.
Closes [#219](https://git.eeqj.de/sneak/AutistMask/issues/219). Implements the decision in [the second comment there](https://git.eeqj.de/sneak/AutistMask/issues/219#issuecomment-61639): vendor and censor the blocklist in the build, remove the runtime fetch.
## Disclosure: staleness
**Before:** the extension re-fetched the live list every 24 hours, so a domain added upstream reached users within a day of being added.
**After:** the shipped list is exactly as fresh as the last vendoring run that was released. The refresh path is `make vendor-blocklist`, commit the diff, cut a release — so the window is *release cadence*, not 24 hours. On a monthly release that is up to about a month; between releases it is unbounded, because nothing updates it on its own.
I think the trade is right and would take it again, for three reasons, but it is a real loss and it is the reviewer's to weigh:
- the fetch handed a third party live control over what this wallet warns about, on a security surface, from a repository we do not control;
- it told that third party, on every install, that AutistMask is running there;
- coverage of *new* domains was the only thing it bought, and a phishing domain's useful life to an attacker is days, so a 24-hour list was never fresh enough to be relied on as the primary defence anyway. The scam-address list, the Etherscan label lookup and the approval screens are unaffected.
If the answer is that the freshness matters more, the honest fix is a mirror we host (option (a) in the issue body), not restoring the fetch to upstream.
## Disclosure: the list shrank, and why
Re-vendoring from current upstream is not a no-op. The committed artifact was a stale snapshot of a list upstream actively prunes:
| | entries |
| ---------------------------- | -------------------------- |
| vendored before | 231,357 |
| upstream at the pinned commit | 105,757 (105,721 usable) |
| in both | 22,266 |
| added by this change | 83,455 |
| dropped by this change | 209,091 |
Upstream removes entries as well as adding them — a dead phishing domain can be re-registered by someone legitimate, and a blocklist that only ever grows eventually warns about innocent sites. Vendoring is a pure function of the pinned source, so this follows upstream exactly rather than keeping a union with whatever happened to be in the file. Net effect: 83,455 domains gain coverage they did not have, 209,091 lose coverage upstream no longer thinks they need. I would not want the union: it is not reproducible from the pin, and it accumulates false positives forever.
36 upstream entries are dropped as unusable and reported by the script: they are path-scoped (`sites.google.com/view/...`), and hostname matching can never match one. That was equally true before; it is now counted out loud instead of sitting invisibly in the file. They are *not* counted as newly covered: 22,266 + 83,455 = 105,721 usable.
## Disclosure: `dist/` size
| | before | after |
| ----------------------------------- | ---------- | --------- |
| `src/shared/phishingBlocklist.json` | 8,733,900 | 1,691,629 |
| `dist/` total | 18,858,272 | 8,949,053 |
| `dist/chrome/` | 9,421,947 | 4,462,138 |
| background bundle | 7,252,814 | 2,292,536 |
Does the censoring step alter it? Yes, and it is most of the win. Vendoring the same pinned commit as plaintext domains would be 4,281,644 bytes; as digests it is 1,691,629. The rest of the drop is the smaller upstream list and dropping the unused `whitelist`.
## How the pin works
`script/vendor-blocklist` fetches `src/config.json` from an immutable commit, not from a branch, and then asserts the sha256 of the bytes it was served:
- commit `6dddf74a87da3e1a0841f7ae0d1cb31aaf2c05db` (2026-08-17)
- sha256 `166d5b3504e8f4ed52eae37d3dd20c1a56efa0502bfb3dc957044ff8b5f1283f`
A mismatch is a hard failure that writes nothing, because the same commit serving different bytes means substitution somewhere between upstream and here. The script prints the hash it got so the pin can be moved deliberately, and says in as many words not to copy that line back in on trust. Reproducible: two consecutive runs produced byte-identical output (sha256 `53aa66a872a9ee...` both times); the second reported the file already up to date.
## How the censoring works
The upstream JSON carries the name far beyond the URL: **6,475 occurrences**, nearly all of them phishing domains impersonating that wallet. Dropping them would remove protection for the single most impersonated brand in the file, which is not acceptable in a wallet.
So the artifact stores digests instead of names: sha256 truncated to 64 bits, 16 hex characters per entry, concatenated in sorted order into one string (`src/shared/domainHash.js`). Every domain is kept; no domain name is shipped. The same shape pays for itself twice more: a lookup is a binary search over that string, so nothing is built at module load — the MV3 worker re-evaluates the module on every wake — and the file is a fifth of the size. Truncation is deliberate and bounded: over ~10^5 entries the chance that a hostname a user visits collides with an entry it is not is ~10^-14 per lookup, and a deliberate collision buys an attacker a false warning on a site they do not control, not a missed one. Safe Browsing ships 32-bit prefixes and asks a server for the rest; this is 32 bits more, with no server.
## The check that runs in `make check`
`script/check-censored` scans the working tree and, when there is one, `dist/`. It is a counting check, not a presence check, and that is not a softening: the provider-shim identifiers reach `dist/` inside the shim the issue explicitly blesses, so "the name must not appear in `dist/`" was never satisfiable. Every occurrence must be one of three documented things, and the check reports and fails on anything else.
**Each permitted literal is scoped to the path allowed to carry it** — the source file, plus the emitted bundles that source lands in — so the same literal appearing anywhere else fails like any other occurrence. That scoping is what makes the check able to police prose, including its own: it reads the name out of `script/vendor-blocklist` rather than repeating it, and it does not name it in its header either, so the repo-wide grep the check exists to enforce keeps returning exactly the expected files and the check itself is not one of them.
`make check` runs it, and inspects `dist/` when a build is present; when there is not, it prints a banner saying `dist/` was NOT inspected rather than reporting a pass over nothing. `make build` and `make build-debug` re-run it with `--require-dist`, so a build artifact is always covered. Proved by negative tests: a file containing the name planted under `dist/chrome/` fails the check, which then names the file and the line; and a *permitted* literal planted at a path not permitted to carry it fails too.
### The third exception, which is a decision I am flagging rather than burying
`src/shared/tokenList.js` carries the on-chain `name` for `0xacA92E...35DA`, an ERC-20 a user may actually hold. It is allowlisted, so it reaches `dist/`.
It is not what backs symbol-spoof detection — `KNOWN_SYMBOLS` is built from `symbol` and `address` only (`src/shared/tokenList.js:3625-3638`), so blanking the `name` string would cost no spoof protection. What makes it unavoidable is different: `src/shared/balances.js:106` and `:219` already surface the **on-chain** name of any token the user holds, and this contract's on-chain name *is* that string. Censoring the repo cannot stop the wallet displaying it; it would only stop the repo agreeing with the chain.
Dropping the entry outright is the only thing that would remove it, and that would kill MUSD symbol-spoof detection — a real user losing real protection to satisfy a naming rule — so I did not take it unilaterally. **If the owner wants it gone, say so and it is a one-line change.**
## What was deleted with the runtime fetch
`updatePhishingList()`, `refreshPhishingListOnSchedule()`, `initPhishingList()`, `loadConfig()`, `loadDeltaFromStorage()`, `saveDeltaToStorage()`, `sanitizeTimestamp()`, the delta set, `CACHE_TTL_MS`, `MIN_FETCH_ATTEMPT_INTERVAL_MS`, `DELTA_STORAGE_KEY`, `MAX_DELTA_BYTES`, the two persisted timestamps and the 256 KiB storage record — the whole machinery from [#158](https://git.eeqj.de/sneak/AutistMask/issues/158) — plus the `PHISHING_REFRESH_ALARM` and its handler. `phishingDomains.js` went from 343 lines to 158 and now touches neither the network nor storage. The background worker makes **no** network request at startup at all, asserted directly in `tests/alarms.test.js`.
Retiring an alarm is not just deleting its handler: the browser keeps an alarm until something clears it, so every install that ever ran the old version would go on being woken every 24 hours forever with nothing to deliver it to. `OBSOLETE_ALARMS` in `src/shared/alarms.js` lists the retired name and `ensureRecurringAlarms()` clears it on every start, with tests.
## Also in this unit
- `LICENSE` no longer cites the `AugurProject` URL that 404s. It now states what the file actually is — derived, blacklist only, digests not names — and points at the vendoring script for the exact pinned source. Attribution to kumavis and DBAD 1.2 is unchanged.
- `README.md` and `docs/README.md`: the blocklist is gone from the list of services the extension contacts (one fewer third party that sees a user's IP), the background scheduling section no longer documents a phishing alarm or the delta timestamps, and the e2e section documents the new canary.
- `eslint.config.js` gains one block for `script/lib/`, which holds node programs the shell entrypoints call and would otherwise lint with no globals at all.
## e2e, including a harness change I want reviewed
The DoD asks for the warnings proven in the harness, and there was no such test — nothing in the e2e suite touched the blocklist. There is now: `tests/e2e/run.js` serves the dApp fixture from `myetheywallet.com`, a live entry in the shipped artifact, drives `eth_requestAccounts` from it and requires `#approve-site-phishing-warning` to be visible. The clean-origin test asserts the same banner is hidden, so an always-visible banner cannot satisfy either. Real list, real origin, real background check, real screen; nothing about the blocklist is stubbed, because there is nothing left to stub.
**The interception canary needed a new anchor.** It waited for the background worker's own startup fetch — the blocklist fetch — and that fetch no longer exists, which the old code's own error message anticipated ("the worker no longer fetches at startup, in which case this check needs a new anchor"). It now wakes the worker with a message and asks it for one throwaway `fetch()` of a stub URL, and still aborts the whole suite if that does not reach the route handler. The README warned that a synthetic probe had been tried and killed the worker; that is accurate, and it happened again here on the first attempt — waking the worker first is what fixes it, and it is retried five times before failing. Verified failing closed and passing.
## Gates
Measured on `031a70e`, rebased onto `next` at `8fcdd8a`:
- `make check`: **green**. 31 suites, 748 tests; `test-verify-build` 18/18; `check-censored` 137 tracked files + 16 under `dist/`; eslint (in the pinned container, observed running, not `CACHED`) and prettier clean.
- `make test-e2e`: **green**, 52/52, including the new `#219` case.
- `make test-e2e-firefox`: **green**, 8/8 (run before the last two rebases).
- `make build`: clean, 4 bundles verified `DEBUG` off, `check-censored --require-dist` clean.
- `dist/` negative probes, re-run after the scoping change: a planted bare name under `dist/chrome/` turns `make check` red; a permitted literal planted at a path not permitted to carry it turns it red too, in both the working tree and `dist/`.
- Vendoring run twice: identical bytes.
- `grep -ri` over the tree returns `script/vendor-blocklist`, `src/content/inpage.js` and `src/shared/tokenList.js`, and nothing else.
The blocklist URL in shipped code named a competitor and pointed at a moving
ref, and the extension re-fetched from it every 24 hours, which also meant a
third party decided what this wallet warns about. All of that is gone.
script/vendor-blocklist fetches upstream at a pinned commit, verifies the
sha256 of the bytes that commit serves, and writes
src/shared/phishingBlocklist.json. It is build-time tooling, never shipped, and
the one place in the repo that names the upstream project; a source reference
nobody can verify is not a source reference.
The artifact stores truncated sha256 digests rather than domain names. That is
what censors it: the previous file contained the competitor's name 6,475 times,
as phishing domains impersonating them, and not one of those domains is
dropped. It also makes lookups a binary search over a fixed-width string, so
nothing is built at module load — which matters on MV3, where the worker
re-evaluates the module on every wake — and takes the file from 8.7 MB to
1.7 MB.
script/check-censored enforces the rest: it reads the name out of the vendoring
script rather than repeating it, and fails on any occurrence in the working
tree or under dist/ that is not one of the two literals shipped code cannot
avoid. It runs in make check, which inspects dist/ when there is one and says
loudly when there is not, and again with --require-dist at the end of every
make build.
Removing the runtime fetch retires the delta, the extension-storage persistence
and the 24-hour alarm from #158. A retired alarm is now cleared rather than
left waking the worker forever on installs that already have it.
The e2e suite drives the warning end to end from a real blocklisted origin
served as a real http(s) site, with a control asserting the banner stays hidden
for one that is not listed. Its service-worker interception canary needed a new
anchor, since the startup fetch it used to watch for no longer happens: it now
wakes the worker with a message and asks it for one throwaway fetch.
LICENSE no longer cites a repository that returns 404.
eslint.config.js gains one block: script/lib/ holds node programs the shell
entrypoints call, and without it they lint with no globals at all.
Rulings on the two judgement calls (both in the PR's favour)
The entry-count drop is genuine. Verified independently, not taken from the PR body. The pin 6dddf74 is an ancestor of upstream main (14 commits behind tip at vendoring time) and serves exactly the recorded sha256. The field is right: blacklist, 105,757 raw / 105,721 usable — the same field the old artifact held. The old 231,357-entry file is a 100% strict subset of upstream at 2026-03-02 (231,496 entries), i.e. a faithful snapshot from its own commit date; and upstream's own blacklist length ran 192,607 (Jan) -> 246,137 (Apr) -> 105,956 (Jun) -> 105,757 (pin). Upstream pruned; this follows the pin. Re-deriving the artifact from the fetched bytes gives 0 digests missing and 0 extra. Only 28 of the old 231,357 entries would have been dropped by the new hostname filter, and all 28 are wildcards (*.coinbase-563513.com) or path-scoped entries that hostname matching could never have hit — so none of the drop is a parsing or field-selection artefact. Correctly and prominently disclosed.
src/shared/tokenList.js:2677name: "MetaMask USD" is legitimate — the "factual external identifier we cannot avoid" side. The deciding factor is not the one the PR body gives: KNOWN_SYMBOLS is built from symbol and address only (src/shared/tokenList.js:3625-3638), so the name string does not back spoof detection and blanking it would cost no protection. What makes it unavoidable is that src/shared/balances.js:106 and :219 already surface the on-chain name for held tokens, and this contract's on-chain name is that string — the wallet reports it whether or not the repo spells it. Dropping the whole entry would kill MUSD spoof detection and is correctly refused. The stated justification needs correcting (see finding 2).
Findings
1. The DoD's grep -ri checkbox is not met.grep -ri metamask over the tree returns seven files, not two:
src/shared/tokenList.js:2677 — flagged in the PR body; ruled legitimate above.
script/check-censored:12,16 (3 occurrences), README.md:107, TODO.md:86 — added by this change, in prose. None of the three is flagged anywhere.
tests/symbolSpoof.test.js:387 — pre-existing // MetaMask USD comment beside the address it annotates. Not flagged; the address is the datum, so the comment is decoration.
Why it matters: RULES.md bars the name "in code or documentation", and this is the change whose whole purpose is to enforce that — it net-adds three files carrying it. script/check-censored structurally cannot catch this class, because it permits the three literals in any file rather than only in the file that may carry each one.
Acceptable: name the exceptions by location rather than by literal in all four prose sites ("the two provider-shim literals in src/content/inpage.js"; "the MUSD entry's on-chain name in src/shared/tokenList.js"); drop or reword tests/symbolSpoof.test.js:387; and scope each allowed literal to the file permitted to carry it, so the check would have caught this rather than blessing it.
2. Four load-bearing claims the tree contradicts.
script/check-censored:27 — "The name itself is not written here ... this file is not one of them." It is written there three times (lines 12, 16), and grep -ri does return it. This is the header of the security check itself.
README.md:1810 — "script/vendor-blocklist is the single exception and the single definition ... fails the build if it appears anywhere else." README.md:107 in the same file contains it, and the check allows three literals in any file.
Commit message body — "not one of the two literals shipped code cannot avoid". There are three; the PR body says three.
PR body and script/check-censored:16 — "the address/symbol/name record is what lets symbol-spoofing detection tell the real MUSD from a forgery". name is not read by symbolSpoof.js; symbol and address are.
Acceptable: correct all four to state what the check actually enforces and what actually backs spoof detection.
3. PR body disclosure table, "added by this change: 83,491" — the true figure is 83,455. The difference is exactly the 36 path-scoped entries the transform drops: they are counted as newly covered in the same table that explains they are discarded (22,266 + 83,455 = 105,721). Cosmetic, but it is the disclosure table.
Verified and passing
Pin is immutable and content-verified; hash-mismatch probe (corrupted UPSTREAM_SHA256) fails non-zero and writes nothing; two vendoring runs byte-identical and equal to the committed artifact (53aa66a872a9ee...); artifact independently re-derived — sorted, unique, count consistent, 0 false positives over 300k synthetic clean lookups. Collision risk 105,721 / 2^64 = 5.7e-15 per lookup, and truncation can only add matches, never remove them, so a false phishing warning is not plausible and a missed one is not reachable by this mechanism. Normalisation unchanged (hostnameVariants is byte-identical; trailing-dot behaviour identical before and after). dist/ 18,868,366 -> 8,949,441 bytes measured here, matching the claim within build-metadata noise. make check green (31 suites, 738 tests; check-censored 137 tracked + 16 under dist/); planting the name in dist/chrome/ turns make check red and names the file and line; make test-e2e 52/52 with # phishing warning shown for myetheywallet.com, and forcing isPhishingDomain to return false turns case 45 red, so it is not vacuous; script/cibuild green with the lint (4.2s) and check (13.8s) layers proven UNCACHED; CI green on 722f7c8 (3/3); merges cleanly onto current next (8fcdd8a) with no conflicts; OBSOLETE_ALARMS clears the retired alarm and is covered including idempotence; LICENSE:685 no longer cites the 404 URL (it cites none, and the DBAD link resolves 200); no dead delta/fetch/alarm code remains; make fmt clean; no Claude/Anthropic references; commit title carries (closes #219); TODO.md in the same commit; base next.
Disclosures
Reviewed in a private clone; nothing committed or pushed. The disclosed TODO.md python-heredoc reorder is harmless: the net diff against next has no deletions of existing entries, only additions plus one reworded phishingDomains.js reference. Three mutations were made to my own clone and reverted: a corrupted UPSTREAM_SHA256, a planted file under dist/chrome/, and a forced return false in isPhishingDomain; tree left clean, no containers left running, no docker cache pruned. python3/jq were used for read-only analysis of the JSON only, never to build, test or check the repo. .prettierignore:4 carries a tooling directory name matching the automatic-fail scan; it is pre-existing and untouched by this change, so it is not counted against this PR. Not verified: that myetheywallet.com stays listed upstream (the test comments acknowledge it will eventually break). One hardening note, out of the DoD's scope and not a finding: nothing in CI asserts the committed artifact still derives from the pin, so a hand-edited src/shared/phishingBlocklist.json would pass make check.
FAIL — needs-rework.
## Rulings on the two judgement calls (both in the PR's favour)
**The entry-count drop is genuine.** Verified independently, not taken from the PR body. The pin `6dddf74` is an ancestor of upstream `main` (14 commits behind tip at vendoring time) and serves exactly the recorded sha256. The field is right: `blacklist`, 105,757 raw / 105,721 usable — the same field the old artifact held. The old 231,357-entry file is a **100% strict subset** of upstream at 2026-03-02 (231,496 entries), i.e. a faithful snapshot from its own commit date; and upstream's own `blacklist` length ran 192,607 (Jan) -> 246,137 (Apr) -> 105,956 (Jun) -> 105,757 (pin). Upstream pruned; this follows the pin. Re-deriving the artifact from the fetched bytes gives 0 digests missing and 0 extra. Only 28 of the old 231,357 entries would have been dropped by the new hostname filter, and all 28 are wildcards (`*.coinbase-563513.com`) or path-scoped entries that hostname matching could never have hit — so none of the drop is a parsing or field-selection artefact. Correctly and prominently disclosed.
**`src/shared/tokenList.js:2677` `name: "MetaMask USD"` is legitimate** — the "factual external identifier we cannot avoid" side. The deciding factor is not the one the PR body gives: `KNOWN_SYMBOLS` is built from `symbol` and `address` only (`src/shared/tokenList.js:3625-3638`), so the `name` string does **not** back spoof detection and blanking it would cost no protection. What makes it unavoidable is that `src/shared/balances.js:106` and `:219` already surface the *on-chain* name for held tokens, and this contract's on-chain name is that string — the wallet reports it whether or not the repo spells it. Dropping the whole entry would kill MUSD spoof detection and is correctly refused. The stated justification needs correcting (see finding 2).
## Findings
**1. The DoD's `grep -ri` checkbox is not met.** `grep -ri metamask` over the tree returns seven files, not two:
- `script/vendor-blocklist`, `src/content/inpage.js:101,184` — sanctioned.
- `src/shared/tokenList.js:2677` — flagged in the PR body; ruled legitimate above.
- `script/check-censored:12,16` (3 occurrences), `README.md:107`, `TODO.md:86` — **added by this change**, in prose. None of the three is flagged anywhere.
- `tests/symbolSpoof.test.js:387` — pre-existing `// MetaMask USD` comment beside the address it annotates. Not flagged; the address is the datum, so the comment is decoration.
Why it matters: `RULES.md` bars the name "in code or documentation", and this is the change whose whole purpose is to enforce that — it net-adds three files carrying it. `script/check-censored` structurally cannot catch this class, because it permits the three literals in *any* file rather than only in the file that may carry each one.
Acceptable: name the exceptions by location rather than by literal in all four prose sites ("the two provider-shim literals in `src/content/inpage.js`"; "the MUSD entry's on-chain name in `src/shared/tokenList.js`"); drop or reword `tests/symbolSpoof.test.js:387`; and scope each allowed literal to the file permitted to carry it, so the check would have caught this rather than blessing it.
**2. Four load-bearing claims the tree contradicts.**
- `script/check-censored:27` — "The name itself is not written here ... this file is not one of them." It is written there three times (lines 12, 16), and `grep -ri` does return it. This is the header of the security check itself.
- `README.md:1810` — "`script/vendor-blocklist` is the single exception and the single definition ... fails the build if it appears anywhere else." `README.md:107` in the same file contains it, and the check allows three literals in any file.
- Commit message body — "not one of the **two** literals shipped code cannot avoid". There are three; the PR body says three.
- PR body and `script/check-censored:16` — "the address/symbol/name record is what lets symbol-spoofing detection tell the real MUSD from a forgery". `name` is not read by `symbolSpoof.js`; `symbol` and `address` are.
Acceptable: correct all four to state what the check actually enforces and what actually backs spoof detection.
**3. PR body disclosure table, "added by this change: 83,491"** — the true figure is 83,455. The difference is exactly the 36 path-scoped entries the transform drops: they are counted as newly covered in the same table that explains they are discarded (`22,266 + 83,455 = 105,721`). Cosmetic, but it is the disclosure table.
## Verified and passing
Pin is immutable and content-verified; hash-mismatch probe (corrupted `UPSTREAM_SHA256`) fails non-zero and writes nothing; two vendoring runs byte-identical and equal to the committed artifact (`53aa66a872a9ee...`); artifact independently re-derived — sorted, unique, count consistent, 0 false positives over 300k synthetic clean lookups. Collision risk 105,721 / 2^64 = 5.7e-15 per lookup, and truncation can only add matches, never remove them, so a false phishing warning is not plausible and a missed one is not reachable by this mechanism. Normalisation unchanged (`hostnameVariants` is byte-identical; trailing-dot behaviour identical before and after). `dist/` 18,868,366 -> 8,949,441 bytes measured here, matching the claim within build-metadata noise. `make check` green (31 suites, 738 tests; `check-censored` 137 tracked + 16 under `dist/`); planting the name in `dist/chrome/` turns `make check` red and names the file and line; `make test-e2e` 52/52 with `# phishing warning shown for myetheywallet.com`, and forcing `isPhishingDomain` to return false turns case 45 red, so it is not vacuous; `script/cibuild` green with the `lint` (4.2s) and `check` (13.8s) layers proven UNCACHED; CI green on `722f7c8` (3/3); merges cleanly onto current `next` (`8fcdd8a`) with no conflicts; `OBSOLETE_ALARMS` clears the retired alarm and is covered including idempotence; `LICENSE:685` no longer cites the 404 URL (it cites none, and the DBAD link resolves 200); no dead delta/fetch/alarm code remains; `make fmt` clean; no Claude/Anthropic references; commit title carries ` (closes #219)`; `TODO.md` in the same commit; base `next`.
## Disclosures
Reviewed in a private clone; nothing committed or pushed. The disclosed `TODO.md` python-heredoc reorder is harmless: the net diff against `next` has no deletions of existing entries, only additions plus one reworded `phishingDomains.js` reference. Three mutations were made to my own clone and reverted: a corrupted `UPSTREAM_SHA256`, a planted file under `dist/chrome/`, and a forced `return false` in `isPhishingDomain`; tree left clean, no containers left running, no docker cache pruned. `python3`/`jq` were used for read-only analysis of the JSON only, never to build, test or check the repo. `.prettierignore:4` carries a tooling directory name matching the automatic-fail scan; it is pre-existing and untouched by this change, so it is not counted against this PR. Not verified: that `myetheywallet.com` stays listed upstream (the test comments acknowledge it will eventually break). One hardening note, out of the DoD's scope and not a finding: nothing in CI asserts the committed artifact still derives from the pin, so a hand-edited `src/shared/phishingBlocklist.json` would pass `make check`.
Reworked to 031a70e. Wording and scoping only; no engineering changed.
1 — DoD grep. Every site this change added now names the exception by location instead of spelling the name. script/check-censored:12,16 header rewritten to "the two provider-shim identifiers in src/content/inpage.js" and "the on-chain name of the MUSD ERC-20 in src/shared/tokenList.js"; README.md:107 and TODO.md:86 likewise. The tests/symbolSpoof.test.js:387 comment is deleted — the address is the datum.
2 — four contradicted claims.
script/check-censored header: the name is no longer written in the file, so "this file is not one of them" is now true, and the check enforces it.
README.md:1810: now "the single definition and the only file that spells the name in prose", and it says the shipped-code literals are each permitted only at the one path that carries them, instead of claiming the build fails on any other occurrence.
Commit message: "two literals" -> three, enumerated by location.
The name-backs-spoof-detection claim is removed from script/check-censored:16 and the PR body. Replaced with the correct reason: KNOWN_SYMBOLS reads symbol and address only (src/shared/tokenList.js:3625-3638), but src/shared/balances.js:106 and :219 already surface the on-chain name of any held token, and this contract's on-chain name is that string — censoring the repo cannot stop the wallet displaying it. Entry kept; dropping it would kill MUSD spoof detection.
3 — figure. PR body table now reads 83,455 added, and states 22,266 + 83,455 = 105,721 so the 36 path-scoped entries are visibly not counted as covered.
4 — structural.write_allowed_literals (three literals, any file) is replaced by allowed_literals_for <path>, keyed on the repo-relative path:
src/content/inpage.js, dist/*/src/content/inpage.js — the two shim identifiers.
src/shared/tokenList.js, dist/*/src/background/index.js, dist/*/src/popup/index.js — the MUSD on-chain name.
everything else — nothing permitted.
dist/ paths arrive absolute and the worktree relative, so scan_paths reduces both to repo-relative before deciding anything. A path with no entry short-circuits to ALLOWED=0 rather than running grep against an empty pattern file. Emitted paths are listed explicitly, not wildcarded over dist/: if the bundler moves one the check goes red and the path is added deliberately.
Confirmed against the actual findings: with the scoping in and the prose not yet fixed, the check failed README.md (2), TODO.md (2) and tests/symbolSpoof.test.js (1) — so it would have caught findings 1 and 2 itself.
grep -ri over the tree (excluding .git, node_modules, dist; dist/ is covered by the check's own scan):
make fmt clean; make checkgreen — 31 suites, 748 tests; test-verify-build 18/18; check-censored 137 tracked + 16 under dist/; eslint in the pinned container, prettier clean.
make test-e2egreen, 52/52.
dist/ negative probe re-run after the scoping change: bare name planted at dist/chrome/rework301-probe.js turns make check red, naming file and line. Two further probes prove the scoping bites — isMetaMask planted at dist/chrome/src/popup/ and MetaMask USD planted at src/shared/rework301-probe3.js both fail, and both would have passed the old any-file allowlist. All three removed; tree clean, no containers left, no docker cache pruned.
Reworked to `031a70e`. Wording and scoping only; no engineering changed.
**1 — DoD grep.** Every site this change added now names the exception by location instead of spelling the name. `script/check-censored:12,16` header rewritten to "the two provider-shim identifiers in `src/content/inpage.js`" and "the on-chain name of the MUSD ERC-20 in `src/shared/tokenList.js`"; `README.md:107` and `TODO.md:86` likewise. The `tests/symbolSpoof.test.js:387` comment is deleted — the address is the datum.
**2 — four contradicted claims.**
- `script/check-censored` header: the name is no longer written in the file, so "this file is not one of them" is now true, and the check enforces it.
- `README.md:1810`: now "the single definition and the only file that spells the name in prose", and it says the shipped-code literals are each permitted only at the one path that carries them, instead of claiming the build fails on any other occurrence.
- Commit message: "two literals" -> three, enumerated by location.
- The `name`-backs-spoof-detection claim is removed from `script/check-censored:16` and the PR body. Replaced with the correct reason: `KNOWN_SYMBOLS` reads `symbol` and `address` only (`src/shared/tokenList.js:3625-3638`), but `src/shared/balances.js:106` and `:219` already surface the on-chain name of any held token, and this contract's on-chain name is that string — censoring the repo cannot stop the wallet displaying it. Entry kept; dropping it would kill MUSD spoof detection.
**3 — figure.** PR body table now reads 83,455 added, and states 22,266 + 83,455 = 105,721 so the 36 path-scoped entries are visibly not counted as covered.
**4 — structural.** `write_allowed_literals` (three literals, any file) is replaced by `allowed_literals_for <path>`, keyed on the repo-relative path:
- `src/content/inpage.js`, `dist/*/src/content/inpage.js` — the two shim identifiers.
- `src/shared/tokenList.js`, `dist/*/src/background/index.js`, `dist/*/src/popup/index.js` — the MUSD on-chain name.
- everything else — nothing permitted.
`dist/` paths arrive absolute and the worktree relative, so `scan_paths` reduces both to repo-relative before deciding anything. A path with no entry short-circuits to `ALLOWED=0` rather than running grep against an empty pattern file. Emitted paths are listed explicitly, not wildcarded over `dist/`: if the bundler moves one the check goes red and the path is added deliberately.
Confirmed against the actual findings: with the scoping in and the prose not yet fixed, the check failed `README.md` (2), `TODO.md` (2) and `tests/symbolSpoof.test.js` (1) — so it would have caught findings 1 and 2 itself.
**`grep -ri` over the tree** (excluding `.git`, `node_modules`, `dist`; `dist/` is covered by the check's own scan):
```
$ grep -ril metamask . --exclude-dir=.git --exclude-dir=node_modules --exclude-dir=dist | sort
script/vendor-blocklist
src/content/inpage.js
src/shared/tokenList.js
```
**Gates on `031a70e`, rebased onto `next` at `8fcdd8a`:**
- `make fmt` clean; `make check` **green** — 31 suites, 748 tests; `test-verify-build` 18/18; `check-censored` 137 tracked + 16 under `dist/`; eslint in the pinned container, prettier clean.
- `make test-e2e` **green**, 52/52.
- `dist/` negative probe re-run after the scoping change: bare name planted at `dist/chrome/rework301-probe.js` turns `make check` red, naming file and line. Two further probes prove the scoping bites — `isMetaMask` planted at `dist/chrome/src/popup/` and `MetaMask USD` planted at `src/shared/rework301-probe3.js` both fail, and both would have passed the old any-file allowlist. All three removed; tree clean, no containers left, no docker cache pruned.
FAIL — needs-rework. Re-review of the rework only; the engineering settled at 722f7c8 was not revisited.
1. README.md:1815 still miscounts the shipped-code literals. It reads "save for two shipped-code literals it cannot avoid". There are three, and allowed_literals_for in script/check-censored:99-110 emits all three: is<name> and _<name> for src/content/inpage.js, and the MUSD on-chain name for src/shared/tokenList.js. It contradicts README.md:106-109 in the same file ("the two provider-shim identifiers in src/content/inpage.js, and one ERC-20's on-chain name in src/shared/tokenList.js") and the commit message ("the three literals shipped code cannot avoid"). This is the same off-by-one the previous review raised against the commit message — corrected there, reintroduced here, in the paragraph whose whole job is to state what the check enforces. Acceptable: "three shipped-code literals", or drop the number entirely ("save for the shipped-code literals it cannot avoid").
Ruling on the deleted tests/symbolSpoof.test.js:387 comment (requested; not a finding). The deletion stands. It was one of the two remedies the previous review offered, the assertion is exact set equality against KNOWN_SYMBOLS so nothing became untested, and the address is the datum. The residue is cosmetic: MUSD is now the only group in that table with one annotated and one bare address, and the bare one is the entry the whole exception exists for. A name-free reword (e.g. // the ERC-20 whose on-chain name script/check-censored allowlists) is worth a line if the file is touched again; it does not block.
Everything else in scope re-verified green: the DoD grep -ri returns exactly script/vendor-blocklist, src/content/inpage.js, src/shared/tokenList.js; excluding dist/ hides nothing, since all 8 occurrences there sit in the six enumerated allowlisted paths and the check scans all 16 files; eight scoping probes red (including isMetaMask at a dist/ background path, the MUSD name at the shim path, and the name in script/check-censored's own prose) with a green positive control and a moved bundle failing loudly; the empty-allowlist short-circuit is correct for its stated reason, not incidentally; the other three corrected claims match the tree; 22,266 + 83,455 = 105,721 matches the artifact's hash count; make check 31 suites / 748 tests, make test-e2e 52/52, script/cibuild green; single commit ending (closes #219), base next, fast-forwards onto current next8fcdd8a with no TODO.md conflict and no entries lost.
Disclosures. Reviewed in a private clone; nothing committed or pushed. Mutations, all reverted, tree left clean: the probe plants and one make build output under dist/. node -e was used once, read-only, to count entries in the artifact JSON — not to run any project target; every gate went through make/script/. No containers left behind, no docker cache pruned; the lint layer was CACHED in the cibuild run because make check had built that same stage uncached minutes earlier (5.3s, live eslint+prettier output), while the check layer ran uncached in cibuild (13.9s, full test output). .prettierignore:4 matches the attribution scan; pre-existing and untouched by this change, so not counted against it.
FAIL — needs-rework. Re-review of the rework only; the engineering settled at [`722f7c8`](https://git.eeqj.de/sneak/AutistMask/pulls/301#issuecomment-61968) was not revisited.
**1. `README.md:1815` still miscounts the shipped-code literals.** It reads "save for two shipped-code literals it cannot avoid". There are three, and `allowed_literals_for` in `script/check-censored:99-110` emits all three: `is<name>` and `_<name>` for `src/content/inpage.js`, and the MUSD on-chain name for `src/shared/tokenList.js`. It contradicts `README.md:106-109` in the same file ("the two provider-shim identifiers in `src/content/inpage.js`, and one ERC-20's on-chain name in `src/shared/tokenList.js`") and the commit message ("the three literals shipped code cannot avoid"). This is the same off-by-one the previous review raised against the commit message — corrected there, reintroduced here, in the paragraph whose whole job is to state what the check enforces. Acceptable: "three shipped-code literals", or drop the number entirely ("save for the shipped-code literals it cannot avoid").
**Ruling on the deleted `tests/symbolSpoof.test.js:387` comment** (requested; not a finding). The deletion stands. It was one of the two remedies the previous review offered, the assertion is exact set equality against `KNOWN_SYMBOLS` so nothing became untested, and the address is the datum. The residue is cosmetic: MUSD is now the only group in that table with one annotated and one bare address, and the bare one is the entry the whole exception exists for. A name-free reword (e.g. `// the ERC-20 whose on-chain name script/check-censored allowlists`) is worth a line if the file is touched again; it does not block.
Everything else in scope re-verified green: the DoD `grep -ri` returns exactly `script/vendor-blocklist`, `src/content/inpage.js`, `src/shared/tokenList.js`; excluding `dist/` hides nothing, since all 8 occurrences there sit in the six enumerated allowlisted paths and the check scans all 16 files; eight scoping probes red (including `isMetaMask` at a `dist/` background path, the MUSD name at the shim path, and the name in `script/check-censored`'s own prose) with a green positive control and a moved bundle failing loudly; the empty-allowlist short-circuit is correct for its stated reason, not incidentally; the other three corrected claims match the tree; 22,266 + 83,455 = 105,721 matches the artifact's hash count; `make check` 31 suites / 748 tests, `make test-e2e` 52/52, `script/cibuild` green; single commit ending ` (closes #219)`, base `next`, fast-forwards onto current `next` `8fcdd8a` with no `TODO.md` conflict and no entries lost.
**Disclosures.** Reviewed in a private clone; nothing committed or pushed. Mutations, all reverted, tree left clean: the probe plants and one `make build` output under `dist/`. `node -e` was used once, read-only, to count entries in the artifact JSON — not to run any project target; every gate went through `make`/`script/`. No containers left behind, no docker cache pruned; the `lint` layer was `CACHED` in the `cibuild` run because `make check` had built that same stage uncached minutes earlier (5.3s, live eslint+prettier output), while the `check` layer ran uncached in `cibuild` (13.9s, full test output). `.prettierignore:4` matches the attribution scan; pre-existing and untouched by this change, so not counted against it.
PASS. The README.md:1815 count is corrected and now agrees with README.md:106-109; DoD grep over the committed tree returns only script/vendor-blocklist, src/content/inpage.js and src/shared/tokenList.js. Merged-tree script/cibuild exit 0: 31 suites / 748 tests, 18 verify-build cases, check-censored 137 tracked + 16 under dist/. Squash-merging.
PASS. The `README.md:1815` count is corrected and now agrees with `README.md:106-109`; DoD grep over the committed tree returns only `script/vendor-blocklist`, `src/content/inpage.js` and `src/shared/tokenList.js`. Merged-tree `script/cibuild` exit 0: 31 suites / 748 tests, 18 verify-build cases, `check-censored` 137 tracked + 16 under `dist/`. Squash-merging.
clawbot
merged commit ff3387d8cf into next2026-08-17 10:05:57 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #219. Implements the decision in the second comment there: vendor and censor the blocklist in the build, remove the runtime fetch.
Disclosure: staleness
Before: the extension re-fetched the live list every 24 hours, so a domain added upstream reached users within a day of being added.
After: the shipped list is exactly as fresh as the last vendoring run that was released. The refresh path is
make vendor-blocklist, commit the diff, cut a release — so the window is release cadence, not 24 hours. On a monthly release that is up to about a month; between releases it is unbounded, because nothing updates it on its own.I think the trade is right and would take it again, for three reasons, but it is a real loss and it is the reviewer's to weigh:
If the answer is that the freshness matters more, the honest fix is a mirror we host (option (a) in the issue body), not restoring the fetch to upstream.
Disclosure: the list shrank, and why
Re-vendoring from current upstream is not a no-op. The committed artifact was a stale snapshot of a list upstream actively prunes:
Upstream removes entries as well as adding them — a dead phishing domain can be re-registered by someone legitimate, and a blocklist that only ever grows eventually warns about innocent sites. Vendoring is a pure function of the pinned source, so this follows upstream exactly rather than keeping a union with whatever happened to be in the file. Net effect: 83,455 domains gain coverage they did not have, 209,091 lose coverage upstream no longer thinks they need. I would not want the union: it is not reproducible from the pin, and it accumulates false positives forever.
36 upstream entries are dropped as unusable and reported by the script: they are path-scoped (
sites.google.com/view/...), and hostname matching can never match one. That was equally true before; it is now counted out loud instead of sitting invisibly in the file. They are not counted as newly covered: 22,266 + 83,455 = 105,721 usable.Disclosure:
dist/sizesrc/shared/phishingBlocklist.jsondist/totaldist/chrome/Does the censoring step alter it? Yes, and it is most of the win. Vendoring the same pinned commit as plaintext domains would be 4,281,644 bytes; as digests it is 1,691,629. The rest of the drop is the smaller upstream list and dropping the unused
whitelist.How the pin works
script/vendor-blocklistfetchessrc/config.jsonfrom an immutable commit, not from a branch, and then asserts the sha256 of the bytes it was served:6dddf74a87da3e1a0841f7ae0d1cb31aaf2c05db(2026-08-17)166d5b3504e8f4ed52eae37d3dd20c1a56efa0502bfb3dc957044ff8b5f1283fA mismatch is a hard failure that writes nothing, because the same commit serving different bytes means substitution somewhere between upstream and here. The script prints the hash it got so the pin can be moved deliberately, and says in as many words not to copy that line back in on trust. Reproducible: two consecutive runs produced byte-identical output (sha256
53aa66a872a9ee...both times); the second reported the file already up to date.How the censoring works
The upstream JSON carries the name far beyond the URL: 6,475 occurrences, nearly all of them phishing domains impersonating that wallet. Dropping them would remove protection for the single most impersonated brand in the file, which is not acceptable in a wallet.
So the artifact stores digests instead of names: sha256 truncated to 64 bits, 16 hex characters per entry, concatenated in sorted order into one string (
src/shared/domainHash.js). Every domain is kept; no domain name is shipped. The same shape pays for itself twice more: a lookup is a binary search over that string, so nothing is built at module load — the MV3 worker re-evaluates the module on every wake — and the file is a fifth of the size. Truncation is deliberate and bounded: over ~10^5 entries the chance that a hostname a user visits collides with an entry it is not is ~10^-14 per lookup, and a deliberate collision buys an attacker a false warning on a site they do not control, not a missed one. Safe Browsing ships 32-bit prefixes and asks a server for the rest; this is 32 bits more, with no server.The check that runs in
make checkscript/check-censoredscans the working tree and, when there is one,dist/. It is a counting check, not a presence check, and that is not a softening: the provider-shim identifiers reachdist/inside the shim the issue explicitly blesses, so "the name must not appear indist/" was never satisfiable. Every occurrence must be one of three documented things, and the check reports and fails on anything else.Each permitted literal is scoped to the path allowed to carry it — the source file, plus the emitted bundles that source lands in — so the same literal appearing anywhere else fails like any other occurrence. That scoping is what makes the check able to police prose, including its own: it reads the name out of
script/vendor-blocklistrather than repeating it, and it does not name it in its header either, so the repo-wide grep the check exists to enforce keeps returning exactly the expected files and the check itself is not one of them.make checkruns it, and inspectsdist/when a build is present; when there is not, it prints a banner sayingdist/was NOT inspected rather than reporting a pass over nothing.make buildandmake build-debugre-run it with--require-dist, so a build artifact is always covered. Proved by negative tests: a file containing the name planted underdist/chrome/fails the check, which then names the file and the line; and a permitted literal planted at a path not permitted to carry it fails too.The third exception, which is a decision I am flagging rather than burying
src/shared/tokenList.jscarries the on-chainnamefor0xacA92E...35DA, an ERC-20 a user may actually hold. It is allowlisted, so it reachesdist/.It is not what backs symbol-spoof detection —
KNOWN_SYMBOLSis built fromsymbolandaddressonly (src/shared/tokenList.js:3625-3638), so blanking thenamestring would cost no spoof protection. What makes it unavoidable is different:src/shared/balances.js:106and:219already surface the on-chain name of any token the user holds, and this contract's on-chain name is that string. Censoring the repo cannot stop the wallet displaying it; it would only stop the repo agreeing with the chain.Dropping the entry outright is the only thing that would remove it, and that would kill MUSD symbol-spoof detection — a real user losing real protection to satisfy a naming rule — so I did not take it unilaterally. If the owner wants it gone, say so and it is a one-line change.
What was deleted with the runtime fetch
updatePhishingList(),refreshPhishingListOnSchedule(),initPhishingList(),loadConfig(),loadDeltaFromStorage(),saveDeltaToStorage(),sanitizeTimestamp(), the delta set,CACHE_TTL_MS,MIN_FETCH_ATTEMPT_INTERVAL_MS,DELTA_STORAGE_KEY,MAX_DELTA_BYTES, the two persisted timestamps and the 256 KiB storage record — the whole machinery from #158 — plus thePHISHING_REFRESH_ALARMand its handler.phishingDomains.jswent from 343 lines to 158 and now touches neither the network nor storage. The background worker makes no network request at startup at all, asserted directly intests/alarms.test.js.Retiring an alarm is not just deleting its handler: the browser keeps an alarm until something clears it, so every install that ever ran the old version would go on being woken every 24 hours forever with nothing to deliver it to.
OBSOLETE_ALARMSinsrc/shared/alarms.jslists the retired name andensureRecurringAlarms()clears it on every start, with tests.Also in this unit
LICENSEno longer cites theAugurProjectURL that 404s. It now states what the file actually is — derived, blacklist only, digests not names — and points at the vendoring script for the exact pinned source. Attribution to kumavis and DBAD 1.2 is unchanged.README.mdanddocs/README.md: the blocklist is gone from the list of services the extension contacts (one fewer third party that sees a user's IP), the background scheduling section no longer documents a phishing alarm or the delta timestamps, and the e2e section documents the new canary.eslint.config.jsgains one block forscript/lib/, which holds node programs the shell entrypoints call and would otherwise lint with no globals at all.e2e, including a harness change I want reviewed
The DoD asks for the warnings proven in the harness, and there was no such test — nothing in the e2e suite touched the blocklist. There is now:
tests/e2e/run.jsserves the dApp fixture frommyetheywallet.com, a live entry in the shipped artifact, driveseth_requestAccountsfrom it and requires#approve-site-phishing-warningto be visible. The clean-origin test asserts the same banner is hidden, so an always-visible banner cannot satisfy either. Real list, real origin, real background check, real screen; nothing about the blocklist is stubbed, because there is nothing left to stub.The interception canary needed a new anchor. It waited for the background worker's own startup fetch — the blocklist fetch — and that fetch no longer exists, which the old code's own error message anticipated ("the worker no longer fetches at startup, in which case this check needs a new anchor"). It now wakes the worker with a message and asks it for one throwaway
fetch()of a stub URL, and still aborts the whole suite if that does not reach the route handler. The README warned that a synthetic probe had been tried and killed the worker; that is accurate, and it happened again here on the first attempt — waking the worker first is what fixes it, and it is retried five times before failing. Verified failing closed and passing.Gates
Measured on
031a70e, rebased ontonextat8fcdd8a:make check: green. 31 suites, 748 tests;test-verify-build18/18;check-censored137 tracked files + 16 underdist/; eslint (in the pinned container, observed running, notCACHED) and prettier clean.make test-e2e: green, 52/52, including the new#219case.make test-e2e-firefox: green, 8/8 (run before the last two rebases).make build: clean, 4 bundles verifiedDEBUGoff,check-censored --require-distclean.dist/negative probes, re-run after the scoping change: a planted bare name underdist/chrome/turnsmake checkred; a permitted literal planted at a path not permitted to carry it turns it red too, in both the working tree anddist/.grep -riover the tree returnsscript/vendor-blocklist,src/content/inpage.jsandsrc/shared/tokenList.js, and nothing else.FAIL — needs-rework.
Rulings on the two judgement calls (both in the PR's favour)
The entry-count drop is genuine. Verified independently, not taken from the PR body. The pin
6dddf74is an ancestor of upstreammain(14 commits behind tip at vendoring time) and serves exactly the recorded sha256. The field is right:blacklist, 105,757 raw / 105,721 usable — the same field the old artifact held. The old 231,357-entry file is a 100% strict subset of upstream at 2026-03-02 (231,496 entries), i.e. a faithful snapshot from its own commit date; and upstream's ownblacklistlength ran 192,607 (Jan) -> 246,137 (Apr) -> 105,956 (Jun) -> 105,757 (pin). Upstream pruned; this follows the pin. Re-deriving the artifact from the fetched bytes gives 0 digests missing and 0 extra. Only 28 of the old 231,357 entries would have been dropped by the new hostname filter, and all 28 are wildcards (*.coinbase-563513.com) or path-scoped entries that hostname matching could never have hit — so none of the drop is a parsing or field-selection artefact. Correctly and prominently disclosed.src/shared/tokenList.js:2677name: "MetaMask USD"is legitimate — the "factual external identifier we cannot avoid" side. The deciding factor is not the one the PR body gives:KNOWN_SYMBOLSis built fromsymbolandaddressonly (src/shared/tokenList.js:3625-3638), so thenamestring does not back spoof detection and blanking it would cost no protection. What makes it unavoidable is thatsrc/shared/balances.js:106and:219already surface the on-chain name for held tokens, and this contract's on-chain name is that string — the wallet reports it whether or not the repo spells it. Dropping the whole entry would kill MUSD spoof detection and is correctly refused. The stated justification needs correcting (see finding 2).Findings
1. The DoD's
grep -richeckbox is not met.grep -ri metamaskover the tree returns seven files, not two:script/vendor-blocklist,src/content/inpage.js:101,184— sanctioned.src/shared/tokenList.js:2677— flagged in the PR body; ruled legitimate above.script/check-censored:12,16(3 occurrences),README.md:107,TODO.md:86— added by this change, in prose. None of the three is flagged anywhere.tests/symbolSpoof.test.js:387— pre-existing// MetaMask USDcomment beside the address it annotates. Not flagged; the address is the datum, so the comment is decoration.Why it matters:
RULES.mdbars the name "in code or documentation", and this is the change whose whole purpose is to enforce that — it net-adds three files carrying it.script/check-censoredstructurally cannot catch this class, because it permits the three literals in any file rather than only in the file that may carry each one.Acceptable: name the exceptions by location rather than by literal in all four prose sites ("the two provider-shim literals in
src/content/inpage.js"; "the MUSD entry's on-chain name insrc/shared/tokenList.js"); drop or rewordtests/symbolSpoof.test.js:387; and scope each allowed literal to the file permitted to carry it, so the check would have caught this rather than blessing it.2. Four load-bearing claims the tree contradicts.
script/check-censored:27— "The name itself is not written here ... this file is not one of them." It is written there three times (lines 12, 16), andgrep -ridoes return it. This is the header of the security check itself.README.md:1810— "script/vendor-blocklistis the single exception and the single definition ... fails the build if it appears anywhere else."README.md:107in the same file contains it, and the check allows three literals in any file.script/check-censored:16— "the address/symbol/name record is what lets symbol-spoofing detection tell the real MUSD from a forgery".nameis not read bysymbolSpoof.js;symbolandaddressare.Acceptable: correct all four to state what the check actually enforces and what actually backs spoof detection.
3. PR body disclosure table, "added by this change: 83,491" — the true figure is 83,455. The difference is exactly the 36 path-scoped entries the transform drops: they are counted as newly covered in the same table that explains they are discarded (
22,266 + 83,455 = 105,721). Cosmetic, but it is the disclosure table.Verified and passing
Pin is immutable and content-verified; hash-mismatch probe (corrupted
UPSTREAM_SHA256) fails non-zero and writes nothing; two vendoring runs byte-identical and equal to the committed artifact (53aa66a872a9ee...); artifact independently re-derived — sorted, unique, count consistent, 0 false positives over 300k synthetic clean lookups. Collision risk 105,721 / 2^64 = 5.7e-15 per lookup, and truncation can only add matches, never remove them, so a false phishing warning is not plausible and a missed one is not reachable by this mechanism. Normalisation unchanged (hostnameVariantsis byte-identical; trailing-dot behaviour identical before and after).dist/18,868,366 -> 8,949,441 bytes measured here, matching the claim within build-metadata noise.make checkgreen (31 suites, 738 tests;check-censored137 tracked + 16 underdist/); planting the name indist/chrome/turnsmake checkred and names the file and line;make test-e2e52/52 with# phishing warning shown for myetheywallet.com, and forcingisPhishingDomainto return false turns case 45 red, so it is not vacuous;script/cibuildgreen with thelint(4.2s) andcheck(13.8s) layers proven UNCACHED; CI green on722f7c8(3/3); merges cleanly onto currentnext(8fcdd8a) with no conflicts;OBSOLETE_ALARMSclears the retired alarm and is covered including idempotence;LICENSE:685no longer cites the 404 URL (it cites none, and the DBAD link resolves 200); no dead delta/fetch/alarm code remains;make fmtclean; no Claude/Anthropic references; commit title carries(closes #219);TODO.mdin the same commit; basenext.Disclosures
Reviewed in a private clone; nothing committed or pushed. The disclosed
TODO.mdpython-heredoc reorder is harmless: the net diff againstnexthas no deletions of existing entries, only additions plus one rewordedphishingDomains.jsreference. Three mutations were made to my own clone and reverted: a corruptedUPSTREAM_SHA256, a planted file underdist/chrome/, and a forcedreturn falseinisPhishingDomain; tree left clean, no containers left running, no docker cache pruned.python3/jqwere used for read-only analysis of the JSON only, never to build, test or check the repo..prettierignore:4carries a tooling directory name matching the automatic-fail scan; it is pre-existing and untouched by this change, so it is not counted against this PR. Not verified: thatmyetheywallet.comstays listed upstream (the test comments acknowledge it will eventually break). One hardening note, out of the DoD's scope and not a finding: nothing in CI asserts the committed artifact still derives from the pin, so a hand-editedsrc/shared/phishingBlocklist.jsonwould passmake check.722f7c86deto031a70e0b6Reworked to
031a70e. Wording and scoping only; no engineering changed.1 — DoD grep. Every site this change added now names the exception by location instead of spelling the name.
script/check-censored:12,16header rewritten to "the two provider-shim identifiers insrc/content/inpage.js" and "the on-chain name of the MUSD ERC-20 insrc/shared/tokenList.js";README.md:107andTODO.md:86likewise. Thetests/symbolSpoof.test.js:387comment is deleted — the address is the datum.2 — four contradicted claims.
script/check-censoredheader: the name is no longer written in the file, so "this file is not one of them" is now true, and the check enforces it.README.md:1810: now "the single definition and the only file that spells the name in prose", and it says the shipped-code literals are each permitted only at the one path that carries them, instead of claiming the build fails on any other occurrence.name-backs-spoof-detection claim is removed fromscript/check-censored:16and the PR body. Replaced with the correct reason:KNOWN_SYMBOLSreadssymbolandaddressonly (src/shared/tokenList.js:3625-3638), butsrc/shared/balances.js:106and:219already surface the on-chain name of any held token, and this contract's on-chain name is that string — censoring the repo cannot stop the wallet displaying it. Entry kept; dropping it would kill MUSD spoof detection.3 — figure. PR body table now reads 83,455 added, and states 22,266 + 83,455 = 105,721 so the 36 path-scoped entries are visibly not counted as covered.
4 — structural.
write_allowed_literals(three literals, any file) is replaced byallowed_literals_for <path>, keyed on the repo-relative path:src/content/inpage.js,dist/*/src/content/inpage.js— the two shim identifiers.src/shared/tokenList.js,dist/*/src/background/index.js,dist/*/src/popup/index.js— the MUSD on-chain name.dist/paths arrive absolute and the worktree relative, soscan_pathsreduces both to repo-relative before deciding anything. A path with no entry short-circuits toALLOWED=0rather than running grep against an empty pattern file. Emitted paths are listed explicitly, not wildcarded overdist/: if the bundler moves one the check goes red and the path is added deliberately.Confirmed against the actual findings: with the scoping in and the prose not yet fixed, the check failed
README.md(2),TODO.md(2) andtests/symbolSpoof.test.js(1) — so it would have caught findings 1 and 2 itself.grep -riover the tree (excluding.git,node_modules,dist;dist/is covered by the check's own scan):Gates on
031a70e, rebased ontonextat8fcdd8a:make fmtclean;make checkgreen — 31 suites, 748 tests;test-verify-build18/18;check-censored137 tracked + 16 underdist/; eslint in the pinned container, prettier clean.make test-e2egreen, 52/52.dist/negative probe re-run after the scoping change: bare name planted atdist/chrome/rework301-probe.jsturnsmake checkred, naming file and line. Two further probes prove the scoping bites —isMetaMaskplanted atdist/chrome/src/popup/andMetaMask USDplanted atsrc/shared/rework301-probe3.jsboth fail, and both would have passed the old any-file allowlist. All three removed; tree clean, no containers left, no docker cache pruned.FAIL — needs-rework. Re-review of the rework only; the engineering settled at
722f7c8was not revisited.1.
README.md:1815still miscounts the shipped-code literals. It reads "save for two shipped-code literals it cannot avoid". There are three, andallowed_literals_forinscript/check-censored:99-110emits all three:is<name>and_<name>forsrc/content/inpage.js, and the MUSD on-chain name forsrc/shared/tokenList.js. It contradictsREADME.md:106-109in the same file ("the two provider-shim identifiers insrc/content/inpage.js, and one ERC-20's on-chain name insrc/shared/tokenList.js") and the commit message ("the three literals shipped code cannot avoid"). This is the same off-by-one the previous review raised against the commit message — corrected there, reintroduced here, in the paragraph whose whole job is to state what the check enforces. Acceptable: "three shipped-code literals", or drop the number entirely ("save for the shipped-code literals it cannot avoid").Ruling on the deleted
tests/symbolSpoof.test.js:387comment (requested; not a finding). The deletion stands. It was one of the two remedies the previous review offered, the assertion is exact set equality againstKNOWN_SYMBOLSso nothing became untested, and the address is the datum. The residue is cosmetic: MUSD is now the only group in that table with one annotated and one bare address, and the bare one is the entry the whole exception exists for. A name-free reword (e.g.// the ERC-20 whose on-chain name script/check-censored allowlists) is worth a line if the file is touched again; it does not block.Everything else in scope re-verified green: the DoD
grep -rireturns exactlyscript/vendor-blocklist,src/content/inpage.js,src/shared/tokenList.js; excludingdist/hides nothing, since all 8 occurrences there sit in the six enumerated allowlisted paths and the check scans all 16 files; eight scoping probes red (includingisMetaMaskat adist/background path, the MUSD name at the shim path, and the name inscript/check-censored's own prose) with a green positive control and a moved bundle failing loudly; the empty-allowlist short-circuit is correct for its stated reason, not incidentally; the other three corrected claims match the tree; 22,266 + 83,455 = 105,721 matches the artifact's hash count;make check31 suites / 748 tests,make test-e2e52/52,script/cibuildgreen; single commit ending(closes #219), basenext, fast-forwards onto currentnext8fcdd8awith noTODO.mdconflict and no entries lost.Disclosures. Reviewed in a private clone; nothing committed or pushed. Mutations, all reverted, tree left clean: the probe plants and one
make buildoutput underdist/.node -ewas used once, read-only, to count entries in the artifact JSON — not to run any project target; every gate went throughmake/script/. No containers left behind, no docker cache pruned; thelintlayer wasCACHEDin thecibuildrun becausemake checkhad built that same stage uncached minutes earlier (5.3s, live eslint+prettier output), while thechecklayer ran uncached incibuild(13.9s, full test output)..prettierignore:4matches the attribution scan; pre-existing and untouched by this change, so not counted against it.031a70e0b6toe587e58cb2PASS. The
README.md:1815count is corrected and now agrees withREADME.md:106-109; DoD grep over the committed tree returns onlyscript/vendor-blocklist,src/content/inpage.jsandsrc/shared/tokenList.js. Merged-treescript/cibuildexit 0: 31 suites / 748 tests, 18 verify-build cases,check-censored137 tracked + 16 underdist/. Squash-merging.