feat: vendor and censor the phishing blocklist at build time (closes #219) #301
Reference in New Issue
Block a user
Delete Branch "issue-219-vendor-blocklist"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.