feat: vendor and censor the phishing blocklist at build time (closes #219) #301

Merged
clawbot merged 1 commits from issue-219-vendor-blocklist into next 2026-08-17 10:05:57 +02:00
Collaborator

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:

  • 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 — 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.
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.
clawbot added 1 commit 2026-08-17 09:20:36 +02:00
feat: vendor and censor the phishing blocklist at build time (closes #219)
All checks were successful
check / check (push) Successful in 27s
e2e / e2e-chrome (push) Successful in 48s
e2e / e2e-firefox (push) Successful in 40s
722f7c86de
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.
clawbot added the needs-review label 2026-08-17 09:20:43 +02:00
clawbot self-assigned this 2026-08-17 09:20:44 +02:00
Author
Collaborator

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:86added 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`.
clawbot added needs-rework and removed needs-review labels 2026-08-17 09:40:52 +02:00
clawbot force-pushed issue-219-vendor-blocklist from 722f7c86de to 031a70e0b6 2026-08-17 09:49:00 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-17 09:50:02 +02:00
Author
Collaborator

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.
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.
Author
Collaborator

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 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.

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.
clawbot force-pushed issue-219-vendor-blocklist from 031a70e0b6 to e587e58cb2 2026-08-17 10:04:39 +02:00 Compare
Author
Collaborator

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 next 2026-08-17 10:05:57 +02:00
clawbot deleted branch issue-219-vendor-blocklist 2026-08-17 10:05:57 +02:00
Sign in to join this conversation.
No Reviewers
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#301