decision: the phishing blocklist URL embeds a competitor's org name, and its documented upstream no longer exists #219
Reference in New Issue
Block a user
Delete Branch "%!s()"
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?
BLOCKLIST_URLatsrc/shared/phishingDomains.js:17embeds a competitor's org name in the URL string.RULES.md:121bars competitor names in code and documentation, and says the code must change to match the rule. The same name appears in a comment attests/e2e/network.js:274.There is no drop-in replacement:
AugurProject/eth-phishing-detect, whichREADME.mdandLICENSE:685both cite as the source, returns 404 — that repo is gone. The competitor's org fork is the only live upstream, so the vendoredphishingBlocklist.jsoncan only have come from there. Established while correcting the README in #213, which removed the dead link but deliberately left the code and theLICENSEline alone.This is not the same question as the
isMetaMaskshim in #165: that one is a protocol identifier dApps feature-detect, whereas this is a hostname we choose to fetch from, and we could choose otherwise.Options
Recommendation
(a). A wallet's anti-phishing feed is security-relevant infrastructure, and fetching it from a competitor's repository means they can change what your users are warned about. The naming rule is the smaller reason to move it. If the hosting cost is unwanted, (b) is honest and defensible; (c) is not — it hides the fact rather than changing it.
Whichever you pick,
LICENSE:685needs its deadAugurProjectURL corrected in the same unit.Definition of done (once decided)
grep -rifor the competitor name across the repo returns only the deliberateisMetaMaskshim insrc/content/inpage.js, or documented exceptions.LICENSE:685no longer cites a URL that 404s.TODO.mdupdated in the same commit.make checkpasses.i think we should automate vendoring it (and censoring it) as part of the build process.
Decision recorded: automate the vendoring and the censoring in the build. Implementation requirements below; this supersedes options (a), (b) and (c) in the issue body.
The naming violation lives in SHIPPED code —
BLOCKLIST_URLatsrc/shared/phishingDomains.js:17is fetched by the extension at runtime. A build-time vendoring script is repo tooling, not shipped to users, so the name can remain there where it is a factual source reference and cannot be avoided. That distinction is what makes this work.Requirements
script/entrypoint that fetches the upstream list, censors it, and writessrc/shared/phishingBlocklist.json. POSIX sh, per scripts-to-rule-them-all.REPO_POLICIES.md:22requires it for all external references, and the current URL tracksmain, a moving ref that cannot be pinned. Fetch a pinned commit, and assert the fetched content's hash matches a recorded expectation — a vendoring step that silently accepts whatever it is served is a supply-chain hole in a security feed.src/shared/phishingBlocklist.json, insrc/shared/phishingDomains.js, or anywhere else that reachesdist/. Note the upstream JSON carries metadata fields beyondblacklist; check what is actually in there rather than assuming only the URL matters.LICENSE:685cites a URL that 404s; correct it in the same unit.dist/size, and whether the censoring step alters it.Disclosure required in the PR body, not buried
Removing the runtime refresh means the blocklist is only as fresh as the last build. Today it self-updates every 24 hours. Say plainly what the staleness window becomes and how a refresh reaches users. If that trade is wrong, this is the unit to say so on rather than after it lands.
Definition of done
grep -rifor the competitor name across the repo returns only the build-time vendoring script and the deliberateisMetaMaskshim insrc/content/inpage.js.dist/contains the name, asserted by a check that runs inmake check.LICENSE:685no longer cites a URL that 404s.TODO.mdupdated in the same commit.make checkpasses.