The issue offered (a) add the setting or (b) correct the README. I took (a).
The README's philosophy paragraph is explicit, and the asymmetry was the real
problem: a user who had turned all three other filters off and still saw rows
vanish had no way to find out why, because the responsible check had no
presence in the UI at all. (b) would have had to argue that a definite
symbol/contract mismatch is categorically unlike the three heuristics — true as
far as it goes, but it does not survive the fact that this check is not purely
definite either. KNOWN_SYMBOLS maps ETH to null, so every ERC-20 claiming ETH is dropped including a real WETH contract emitting that symbol; and the
bundled 250-token list is a snapshot, so a genuine token whose entry is stale or
absent is indistinguishable from a spoof. Those are exactly the false-positive
shapes the sharp-tool escape hatch exists for.
The default: ON, and only an explicit false turns it off
hideSpoofedSymbols defaults to true in DEFAULT_STATE, and loadState()
maps an absent stored key to true so a profile written before the setting
existed loads protected rather than undefined (which would read as off and
silently unfilter every existing user on upgrade).
The pure function is deliberately fail-safe in a way the other three flags are
not. filterTransactions() reads filters.hideSpoofedSymbols !== false, so a
caller that omits the key keeps the check; the other three are plain truthiness
and default off when absent. The reason for the difference is blast radius: the
other three flags failing open means a slightly noisier history, while this one
failing open means the documented fake-ETH attack renders as a real outbound
transfer to a look-alike address. A safety filter should require an explicit act
to disable, and this makes the code shape match that. It also keeps the existing with no filters argument only spoof filtering runs test true.
The newFraudContracts interaction: off means off, learning included
Disabling the setting stops both the hiding and the blocklist learning.
This is not a free choice — the alternative is not merely worse, it does not
work. The blocklist's only writer is this check. If learning continued while
display filtering was off, the contract would be added to fraudSet during the
same pass and the very next rule, hideFraudContracts, is on by default and
would hide the row anyway. The user would toggle the setting and observe no
change. Even ordering around that within a pass, the list is persisted, so the
next refresh hides it. Keeping learning on makes the setting a no-op for anyone
on defaults.
The issue rightly warned that quietly disabling blocklist learning would be a
surprising side effect. It is therefore not quiet: it is stated in README.md,
in the user guide, and in a comment at the call site. An already-populated
blocklist is untouched and keeps filtering — hideFraudContracts is a separate
setting and this one does not reach it. There is a test pinning that.
In-UI indication that a protection is off: no, deliberately
I considered a persistent banner or a Home-screen marker and decided against
one.
The three sibling filters have no such indicator. Adding one for only the
fourth asserts a hierarchy the README does not draw — it presents all four as
peers, and the off-state of the one this PR adds is still partly covered
elsewhere: the send selector applies the same check unconditionally, and the
balance list applies it for every symbol except ETH (see the table below).
A permanent banner costs fixed vertical space in a 360x600 popup. Under the
No Layout Shift policy it would have to reserve that space unconditionally,
i.e. shrink the useful viewport for every user in order to annotate a state
almost none of them are in.
The checkbox is itself the indication, and it is unchecked in the same well as
its three peers, reached the same way it was set.
A cross-cutting "protections are disabled" summary covering all four filters
would be a coherent design, but it is a design decision about the set, not this
one member, and introducing it asymmetrically here would be worse than not
having it. I have not filed it as an issue, since the current arrangement is
deliberate rather than defective.
Scope: the setting governs the transaction history
The known-symbol check exists in three places. Only the transaction-history one
is now gated:
filterTransactions() in src/shared/transactions.js — gated by the setting.
fetchTokenBalances() in src/shared/balances.js — unchanged, ungated, and not the same predicate: it requires legitAddr !== null, so it exempts
the one symbol KNOWN_SYMBOLS maps to null, ETH. A fake-ETH ERC-20 is
therefore filtered from history and from the send selector but not from the
balance list. That divergence is pre-existing and out of scope here; it is
filed as #235. The three
documentation sentences that previously claimed uniform coverage now state
the real reach.
renderSendTokenSelect() in src/popup/views/send.js — unchanged,
ungated, and the same predicate as the history check.
The latter two decide which tokens the user can act on rather than what the
history displays, and the README's philosophy paragraph is about showing
everything unfiltered, not about removing guards from an action surface. This
also follows the precedent already set for the balance list, whose
1,000-holder floor is unconditional and was documented as such rather than
being wired to a setting.
What each existing toggle actually gates
Every cell below was re-derived from the code after review found a third
divergence the first pass missed. "Ungated" means the setting is never read on
that surface, which is not the same as the check being identical there.
setting
tx history
balance list
send selector
hideSpoofedSymbols (new)
gated, fail-closed — off only on an explicit false
ungated, and a weaker check: exempts symbols mapped to null (ETH alone), so a fake-ETH ERC-20 is not filtered
ungated, same check as history — null-mapped symbols are caught
hideLowHolderTokens
gated; a null holder count is never filtered, a 0 one is
setting never read; a separate unconditional floor drops a token only if it is not on the bundled list, not user-tracked, and under 1,000 holders
gated; (t.holders || 0) reads an absent or null count as 0 and drops it, where history shows it
hideFraudContracts
gated
blocklist never consulted at all
applied unconditionally
hideDustTransactions
gated; threshold is dustThresholdGwei ?? 100000, and 0 means hide nothing
n/a — no value threshold, only an unconditional zero-balance skip
n/a
Four divergences worth flagging, all pre-existing and all untouched here: the
balance list's weaker known-symbol check (#235),
the balance list never consulting the fraud blocklist, the send selector
applying that blocklist regardless of hideFraudContracts, and the two
surfaces disagreeing on what an unknown holder count means.
Tests, failing first
Tests were written before the implementation. make test on the unmodified
source, 9 failing:
✕ the spoofed transfer is shown when hideSpoofedSymbols is false (4 ms)
✕ no fraud contract is learned when hideSpoofedSymbols is false (2 ms)
✕ all four toggles default to on and the threshold to 100,000 gwei (1 ms)
✕ defaults to on with empty storage (1 ms)
✕ a profile stored without the key loads with it on (1 ms)
✕ an explicit false survives the load (1 ms)
✕ saveState persists the flag (3 ms)
✕ the flag round-trips off through save and load (1 ms)
✕ the flag round-trips back on through save and load (2 ms)
Test Suites: 2 failed, 7 passed, 9 total
Tests: 9 failed, 177 passed, 186 total
The two current behaviour: tests in tests/transactions.test.js that pinned
the filter as undisableable are inverted rather than deleted, per the issue. New
coverage: active by default, active when the other three are off, active when
the key is absent or undefined, suppressed on an explicit false, no
blocklist entry learned while suppressed, the other three rules unaffected, an
already-blocklisted contract still hidden while suppressed, and the flag
round-tripping through saveState/loadState in both directions plus the
absent-key migration.
Verification
Rebased onto next at fb9e8f5. Two conflicts across the rebases, both
resolved keeping every side: the spoof branch now reads if (hideSpoofed && isSpoofedSymbol(tx)) over the normalised contract local
that #179 introduced in the
same loop body, and TODO.md keeps its own entry alongside those from #179, #161 and #223.
make check re-run after the rebase — green. The counts rise against the
failing-first run above because the rebase brought in the suites from #179, #161 and the crypto
known-answer suite from #159; the one skip is
pre-existing in the last of those and not from this branch.
Test Suites: 11 passed, 11 total
Tests: 1 skipped, 274 passed, 275 total
All matched files use Prettier code style!
All matched files use Prettier code style!
make test-e2e — green, real Chrome in the pinned Playwright container, re-run
after the rebase against a build of the branch head 19f2eda from a clean
tree, since the rebase pulled real src/ changes in.
1..13
ok 1 - popup loads and reaches the welcome view
ok 2 - wallet creation through the UI reaches the main view
ok 3 - add token screen opens from address detail (#150)
ok 4 - transaction detail renders an ERC-20 transfer (#151)
ok 5 - only an HD wallet is offered the recovery phrase action (#161)
ok 6 - a key wallet is not offered the recovery phrase action (#161)
ok 7 - the recovery phrase screen holds nothing before the password (#161)
ok 8 - a wrong password reveals nothing (#161)
ok 9 - the correct password reveals the full phrase, and nothing logs it (#161)
ok 10 - "Back" wipes the revealed phrase (#161)
ok 11 - leaving by the settings gear wipes it too (#161)
ok 12 - leaving while the decrypt is in flight reveals nothing (#161)
ok 13 - reopening the popup never lands on the phrase screen (#161)
# 13/13 tests passed
Not verified
The Settings view was not exercised in a browser by me. The e2e suite has
no coverage of the Settings filter well, and jest runs in the node
environment with no DOM, so the new $("settings-hide-spoofed-symbols")
lookup is not covered by a running test in this branch; if that id were
wrong, init() would throw and take the whole Settings view with it. I
verified the id statically only — it appears once in src/popup/index.html
and three times in src/popup/views/settings.js, all identical. Review has
since closed this gap independently, driving the real view in the pinned
container and confirming that a one-character id typo does break it.
Rendered layout not visually inspected. The new label reuses the exact
classes of its three siblings, so it should not shift anything, but I did not
look at the popup.
Tracker CI status on this repo is currently unreliable (a runner fault
attaches unrelated jobs' results to commits), so whatever it reports here
should be read against the local runs above.
Note for concurrent work
isSpoofedSymbol() is untouched — the gate is added around the call. The
dust-coalescing and address-case bugs in #179 have since landed on next and this branch is rebased onto them; nothing here re-touches that work.
Closes [#176](https://git.eeqj.de/sneak/AutistMask/issues/176).
## The choice: (a), add the setting
The issue offered (a) add the setting or (b) correct the README. I took **(a)**.
The README's philosophy paragraph is explicit, and the asymmetry was the real
problem: a user who had turned all three other filters off and still saw rows
vanish had no way to find out why, because the responsible check had no
presence in the UI at all. (b) would have had to argue that a definite
symbol/contract mismatch is categorically unlike the three heuristics — true as
far as it goes, but it does not survive the fact that this check is not purely
definite either. `KNOWN_SYMBOLS` maps `ETH` to `null`, so every ERC-20 claiming
`ETH` is dropped including a real WETH contract emitting that symbol; and the
bundled 250-token list is a snapshot, so a genuine token whose entry is stale or
absent is indistinguishable from a spoof. Those are exactly the false-positive
shapes the sharp-tool escape hatch exists for.
## The default: ON, and only an explicit `false` turns it off
`hideSpoofedSymbols` defaults to `true` in `DEFAULT_STATE`, and `loadState()`
maps an absent stored key to `true` so a profile written before the setting
existed loads protected rather than `undefined` (which would read as off and
silently unfilter every existing user on upgrade).
The pure function is deliberately fail-safe in a way the other three flags are
not. `filterTransactions()` reads `filters.hideSpoofedSymbols !== false`, so a
caller that omits the key keeps the check; the other three are plain truthiness
and default off when absent. The reason for the difference is blast radius: the
other three flags failing open means a slightly noisier history, while this one
failing open means the documented fake-`ETH` attack renders as a real outbound
transfer to a look-alike address. A safety filter should require an explicit act
to disable, and this makes the code shape match that. It also keeps the existing
`with no filters argument only spoof filtering runs` test true.
## The `newFraudContracts` interaction: off means off, learning included
Disabling the setting stops both the hiding and the blocklist learning.
This is not a free choice — the alternative is not merely worse, it does not
work. The blocklist's only writer is this check. If learning continued while
display filtering was off, the contract would be added to `fraudSet` during the
same pass and the very next rule, `hideFraudContracts`, is on by default and
would hide the row anyway. The user would toggle the setting and observe no
change. Even ordering around that within a pass, the list is persisted, so the
next refresh hides it. Keeping learning on makes the setting a no-op for anyone
on defaults.
The issue rightly warned that quietly disabling blocklist learning would be a
surprising side effect. It is therefore not quiet: it is stated in `README.md`,
in the user guide, and in a comment at the call site. An already-populated
blocklist is untouched and keeps filtering — `hideFraudContracts` is a separate
setting and this one does not reach it. There is a test pinning that.
## In-UI indication that a protection is off: no, deliberately
I considered a persistent banner or a Home-screen marker and decided against
one.
- The three sibling filters have no such indicator. Adding one for only the
fourth asserts a hierarchy the README does not draw — it presents all four as
peers, and the off-state of the one this PR adds is still partly covered
elsewhere: the send selector applies the same check unconditionally, and the
balance list applies it for every symbol except `ETH` (see the table below).
- A permanent banner costs fixed vertical space in a 360x600 popup. Under the
No Layout Shift policy it would have to reserve that space unconditionally,
i.e. shrink the useful viewport for every user in order to annotate a state
almost none of them are in.
- The checkbox is itself the indication, and it is unchecked in the same well as
its three peers, reached the same way it was set.
A cross-cutting "protections are disabled" summary covering all four filters
would be a coherent design, but it is a design decision about the set, not this
one member, and introducing it asymmetrically here would be worse than not
having it. I have not filed it as an issue, since the current arrangement is
deliberate rather than defective.
## Scope: the setting governs the transaction history
The known-symbol check exists in three places. Only the transaction-history one
is now gated:
- `filterTransactions()` in `src/shared/transactions.js` — gated by the setting.
- `fetchTokenBalances()` in `src/shared/balances.js` — unchanged, ungated, and
**not the same predicate**: it requires `legitAddr !== null`, so it exempts
the one symbol `KNOWN_SYMBOLS` maps to `null`, `ETH`. A fake-`ETH` ERC-20 is
therefore filtered from history and from the send selector but not from the
balance list. That divergence is pre-existing and out of scope here; it is
filed as [#235](https://git.eeqj.de/sneak/AutistMask/issues/235). The three
documentation sentences that previously claimed uniform coverage now state
the real reach.
- `renderSendTokenSelect()` in `src/popup/views/send.js` — unchanged,
ungated, and the same predicate as the history check.
The latter two decide which tokens the user can act on rather than what the
history displays, and the README's philosophy paragraph is about showing
everything unfiltered, not about removing guards from an action surface. This
also follows the precedent already set for the balance list, whose
1,000-holder floor is unconditional and was documented as such rather than
being wired to a setting.
## What each existing toggle actually gates
Every cell below was re-derived from the code after review found a third
divergence the first pass missed. "Ungated" means the setting is never read on
that surface, which is not the same as the check being identical there.
| setting | tx history | balance list | send selector |
| --- | --- | --- | --- |
| `hideSpoofedSymbols` (new) | gated, fail-closed — off only on an explicit `false` | ungated, and a **weaker check**: exempts symbols mapped to `null` (`ETH` alone), so a fake-`ETH` ERC-20 is not filtered | ungated, same check as history — `null`-mapped symbols are caught |
| `hideLowHolderTokens` | gated; a `null` holder count is never filtered, a `0` one is | setting never read; a separate unconditional floor drops a token only if it is not on the bundled list, not user-tracked, **and** under 1,000 holders | gated; `(t.holders \|\| 0)` reads an absent or `null` count as `0` and drops it, where history shows it |
| `hideFraudContracts` | gated | blocklist never consulted at all | applied unconditionally |
| `hideDustTransactions` | gated; threshold is `dustThresholdGwei ?? 100000`, and `0` means hide nothing | n/a — no value threshold, only an unconditional zero-balance skip | n/a |
Four divergences worth flagging, all pre-existing and all untouched here: the
balance list's weaker known-symbol check ([#235](https://git.eeqj.de/sneak/AutistMask/issues/235)),
the balance list never consulting the fraud blocklist, the send selector
applying that blocklist regardless of `hideFraudContracts`, and the two
surfaces disagreeing on what an unknown holder count means.
## Tests, failing first
Tests were written before the implementation. `make test` on the unmodified
source, 9 failing:
```
✕ the spoofed transfer is shown when hideSpoofedSymbols is false (4 ms)
✕ no fraud contract is learned when hideSpoofedSymbols is false (2 ms)
✕ all four toggles default to on and the threshold to 100,000 gwei (1 ms)
✕ defaults to on with empty storage (1 ms)
✕ a profile stored without the key loads with it on (1 ms)
✕ an explicit false survives the load (1 ms)
✕ saveState persists the flag (3 ms)
✕ the flag round-trips off through save and load (1 ms)
✕ the flag round-trips back on through save and load (2 ms)
Test Suites: 2 failed, 7 passed, 9 total
Tests: 9 failed, 177 passed, 186 total
```
The two `current behaviour:` tests in `tests/transactions.test.js` that pinned
the filter as undisableable are inverted rather than deleted, per the issue. New
coverage: active by default, active when the other three are off, active when
the key is absent or `undefined`, suppressed on an explicit `false`, no
blocklist entry learned while suppressed, the other three rules unaffected, an
already-blocklisted contract still hidden while suppressed, and the flag
round-tripping through `saveState`/`loadState` in both directions plus the
absent-key migration.
## Verification
Rebased onto `next` at `fb9e8f5`. Two conflicts across the rebases, both
resolved keeping every side: the spoof branch now reads
`if (hideSpoofed && isSpoofedSymbol(tx))` over the normalised `contract` local
that [#179](https://git.eeqj.de/sneak/AutistMask/issues/179) introduced in the
same loop body, and `TODO.md` keeps its own entry alongside those from
[#179](https://git.eeqj.de/sneak/AutistMask/issues/179),
[#161](https://git.eeqj.de/sneak/AutistMask/issues/161) and
[#223](https://git.eeqj.de/sneak/AutistMask/issues/223).
`make check` re-run after the rebase — green. The counts rise against the
failing-first run above because the rebase brought in the suites from
[#179](https://git.eeqj.de/sneak/AutistMask/issues/179),
[#161](https://git.eeqj.de/sneak/AutistMask/issues/161) and the crypto
known-answer suite from
[#159](https://git.eeqj.de/sneak/AutistMask/issues/159); the one skip is
pre-existing in the last of those and not from this branch.
```
Test Suites: 11 passed, 11 total
Tests: 1 skipped, 274 passed, 275 total
All matched files use Prettier code style!
All matched files use Prettier code style!
```
`make test-e2e` — green, real Chrome in the pinned Playwright container, re-run
after the rebase against a build of the branch head `19f2eda` from a clean
tree, since the rebase pulled real `src/` changes in.
```
1..13
ok 1 - popup loads and reaches the welcome view
ok 2 - wallet creation through the UI reaches the main view
ok 3 - add token screen opens from address detail (#150)
ok 4 - transaction detail renders an ERC-20 transfer (#151)
ok 5 - only an HD wallet is offered the recovery phrase action (#161)
ok 6 - a key wallet is not offered the recovery phrase action (#161)
ok 7 - the recovery phrase screen holds nothing before the password (#161)
ok 8 - a wrong password reveals nothing (#161)
ok 9 - the correct password reveals the full phrase, and nothing logs it (#161)
ok 10 - "Back" wipes the revealed phrase (#161)
ok 11 - leaving by the settings gear wipes it too (#161)
ok 12 - leaving while the decrypt is in flight reveals nothing (#161)
ok 13 - reopening the popup never lands on the phrase screen (#161)
# 13/13 tests passed
```
## Not verified
- **The Settings view was not exercised in a browser by me.** The e2e suite has
no coverage of the Settings filter well, and jest runs in the node
environment with no DOM, so the new `$("settings-hide-spoofed-symbols")`
lookup is not covered by a running test in this branch; if that id were
wrong, `init()` would throw and take the whole Settings view with it. I
verified the id statically only — it appears once in `src/popup/index.html`
and three times in `src/popup/views/settings.js`, all identical. Review has
since closed this gap independently, driving the real view in the pinned
container and confirming that a one-character id typo does break it.
- **Rendered layout not visually inspected.** The new label reuses the exact
classes of its three siblings, so it should not shift anything, but I did not
look at the popup.
- Tracker CI status on this repo is currently unreliable (a runner fault
attaches unrelated jobs' results to commits), so whatever it reports here
should be read against the local runs above.
## Note for concurrent work
`isSpoofedSymbol()` is untouched — the gate is added around the call. The
dust-coalescing and address-case bugs in
[#179](https://git.eeqj.de/sneak/AutistMask/issues/179) have since landed on
`next` and this branch is rebased onto them; nothing here re-touches that work.
The README promises all four token-spam filters "default to on but can be
individually disabled". Known-symbol spoof verification had no state flag, no
checkbox and no consulted setting: filterTransactions() applied it before any
filter setting was read, so three of the four documented filters were
configurable and the fourth was mandatory.
Adds hideSpoofedSymbols, default on, persisted and migrated so a profile
written before the setting existed loads it as on rather than undefined. The
flag is fail-safe in the pure function too: only an explicit false disables
the check, so a caller that omits the key keeps it.
Turning the setting off also stops the fraud-contract learning. That learning
is fed only by this check, and leaving it on would make the setting a no-op:
the contract it recorded would hide the very row the user asked to see, via
the fraud-contract rule that is on by default.
Scope: the setting governs the transaction history. The same check on the
balance list and the send-screen token selector stays unconditional — those
decide which tokens the user can act on, not what the history displays. The
README's user-configurable paragraph now states what each of the four
settings actually reaches, which is not uniform.
The two `current behaviour:` tests pinning the filter as undisableable are
inverted rather than deleted, and joined by coverage for the bypass, the
halted learning, the untouched sibling rules and the storage round-trip.
README.md:1090, README.md:1126, docs/README.md:330 — newly written text asserts the known-symbol check is applied uniformly on the balance list. It is not, for the exact token the same paragraph uses as its worked example.
The three implementations are not the same check:
src/shared/transactions.js:245 — if (legit === null) return true;
src/popup/views/send.js:122 — if (legit === null) return true;
KNOWN_SYMBOLS.set("ETH", null) (src/shared/tokenList.js:3613) is the only null-mapped entry, and balances.js explicitly exempts it. So an ERC-20 claiming symbol ETH is filtered from the transaction history and from the send selector, and is NOT filtered from the balance list.
That is precisely the token the same README bullet describes two sentences earlier — "The fake 'Ethereum' token in the attack above used symbol 'ETH' ... so it would be caught by this check" — after which the new sentences tell the reader "The same check on the balance list and on the send-screen token selector is unconditional" (README.md:1090), "The known-symbol check also runs unconditionally on the balance list" (README.md:1126) and "Your balances and the send token list always apply the check" (docs/README.md:330). Read together these say a user who switches the new setting off is still covered on balances. For the fake-ETH case they are not.
Reachability: src/shared/balances.js:79 drops unknown and untracked tokens under 1,000 holders, so the gap needs a poisoning contract with an inflated holder count, or a token the user has tracked manually. Both are ordinary — inflating the holder count is what the attack already does to reach the history.
Why it matters: this PR's purpose is to make the README true about which protections are configurable and how far each reaches, and the PR body states the table was "checked at each call site rather than assumed from the name" and reports the surprises found. The third call site was audited as identical when it is not, so the diff replaces one inaccurate README claim with another, in the security section, about the headline attack.
Acceptable: state the balance list's real reach in README.md and docs/README.md — the known-symbol check runs on it unconditionally except for symbols mapped to null (ETH), which it does not filter — and correct the PR body's table row. The balances.js divergence itself is pre-existing and rightly out of scope; file it as its own issue rather than fixing it here.
Verified and passing. The disclosed blind spot is closed: I drove the Settings view for real in the pinned Playwright container — the view renders, the checkbox is present and checked by default, unchecking persists hideSpoofedSymbols: false to extension storage, the state survives a popup reopen, a profile blob with the key deleted migrates back to on, and zero uncaught browser errors or console.errors were recorded. That probe has teeth: a one-character id typo in settings.js reproduced pageerror: Cannot set properties of null (setting 'checked') with the view never rendering, while make test stayed fully green. The fail-closed !== false at transactions.js:263 is the only gate on the flag, so nothing reintroduces fail-open, and it is what keeps the check on even if the loadState migration is removed. The coupling claim holds at transactions.js:267-282: learning without hiding adds the contract to fraudSet and the very next rule, on by default, hides the row. Tests have teeth — flipping the DEFAULT_STATE default, dropping !== false, dropping the loadState migration and removing the gate each failed 1-2 targeted tests. make check green (258 passed, 1 pre-existing skip), make test-e2e 4/4 against a build of d0202fd, make fmt clean, single commit titled (closes #176), base next, authored clawbot, fast-forwards onto next, no Claude or Anthropic references, no attribution trailers, TODO.md grew 21 to 22 entries with none removed, and isSpoofedSymbol plus the dust rule are untouched as #179 requires.
Disclosure: to drive the Settings view I created and then deleted an untracked scratch probe file in my own clone and ran it against the same pinned image script/test-e2e uses; nothing tracked was modified and the working tree was verified clean afterwards. Tracker CI status was ignored per the known runner fault — the runs above are my own.
FAIL — `needs-rework`. One finding.
**`README.md:1090`, `README.md:1126`, `docs/README.md:330` — newly written text asserts the known-symbol check is applied uniformly on the balance list. It is not, for the exact token the same paragraph uses as its worked example.**
The three implementations are not the same check:
- `src/shared/transactions.js:245` — `if (legit === null) return true;`
- `src/popup/views/send.js:122` — `if (legit === null) return true;`
- `src/shared/balances.js:84-89` — `legitAddr !== undefined && legitAddr !== null && tokenAddr !== legitAddr`
`KNOWN_SYMBOLS.set("ETH", null)` (`src/shared/tokenList.js:3613`) is the only null-mapped entry, and `balances.js` explicitly exempts it. So an ERC-20 claiming symbol `ETH` is filtered from the transaction history and from the send selector, and is NOT filtered from the balance list.
That is precisely the token the same README bullet describes two sentences earlier — "The fake 'Ethereum' token in the attack above used symbol 'ETH' ... so it would be caught by this check" — after which the new sentences tell the reader "The same check on the balance list and on the send-screen token selector is unconditional" (`README.md:1090`), "The known-symbol check also runs unconditionally on the balance list" (`README.md:1126`) and "Your balances and the send token list always apply the check" (`docs/README.md:330`). Read together these say a user who switches the new setting off is still covered on balances. For the fake-`ETH` case they are not.
Reachability: `src/shared/balances.js:79` drops unknown and untracked tokens under 1,000 holders, so the gap needs a poisoning contract with an inflated holder count, or a token the user has tracked manually. Both are ordinary — inflating the holder count is what the attack already does to reach the history.
Why it matters: this PR's purpose is to make the README true about which protections are configurable and how far each reaches, and the PR body states the table was "checked at each call site rather than assumed from the name" and reports the surprises found. The third call site was audited as identical when it is not, so the diff replaces one inaccurate README claim with another, in the security section, about the headline attack.
Acceptable: state the balance list's real reach in `README.md` and `docs/README.md` — the known-symbol check runs on it unconditionally except for symbols mapped to `null` (`ETH`), which it does not filter — and correct the PR body's table row. The `balances.js` divergence itself is pre-existing and rightly out of scope; file it as its own issue rather than fixing it here.
Verified and passing. The disclosed blind spot is closed: I drove the Settings view for real in the pinned Playwright container — the view renders, the checkbox is present and checked by default, unchecking persists `hideSpoofedSymbols: false` to extension storage, the state survives a popup reopen, a profile blob with the key deleted migrates back to on, and zero uncaught browser errors or `console.error`s were recorded. That probe has teeth: a one-character id typo in `settings.js` reproduced `pageerror: Cannot set properties of null (setting 'checked')` with the view never rendering, while `make test` stayed fully green. The fail-closed `!== false` at `transactions.js:263` is the only gate on the flag, so nothing reintroduces fail-open, and it is what keeps the check on even if the `loadState` migration is removed. The coupling claim holds at `transactions.js:267-282`: learning without hiding adds the contract to `fraudSet` and the very next rule, on by default, hides the row. Tests have teeth — flipping the `DEFAULT_STATE` default, dropping `!== false`, dropping the `loadState` migration and removing the gate each failed 1-2 targeted tests. `make check` green (258 passed, 1 pre-existing skip), `make test-e2e` 4/4 against a build of `d0202fd`, `make fmt` clean, single commit titled ` (closes #176)`, base `next`, authored `clawbot`, fast-forwards onto `next`, no Claude or Anthropic references, no attribution trailers, `TODO.md` grew 21 to 22 entries with none removed, and `isSpoofedSymbol` plus the dust rule are untouched as [#179](https://git.eeqj.de/sneak/AutistMask/issues/179) requires.
Disclosure: to drive the Settings view I created and then deleted an untracked scratch probe file in my own clone and ran it against the same pinned image `script/test-e2e` uses; nothing tracked was modified and the working tree was verified clean afterwards. Tracker CI status was ignored per the known runner fault — the runs above are my own.
PASS at 19f2eda — the three corrected sentences (README.md:1121-1133, README.md:1163-1175, docs/README.md:326-333) are each true at the call sites, both newly-stated table rows check out (transactions.js:296-300 vs send.js:135; balances.js never references fraudContracts), ETH is provably the only null mapping (tokenList.js:3612-3619, seeded then has()-guarded, and KNOWN_SYMBOLS is never mutated outside that file), balances.js is untouched, nothing from #179 was lost (transactions.js differs from next only by the gate over the normalised contract local) and TODO.md went 24 to 25 entries with none removed; make check green (274 passed, 1 pre-existing skip, prettier clean), make test-e2e 13/13 against a build of 19f2eda, merges into next with no conflicts, single commit ending (closes #176), base next, authored clawbot, no attribution trailers.
Three non-blocking notes, none of them defects in this diff:
README.md:1115 (unchanged first sentence of the very bullet this PR extends) says the bundled list is the "top 250 ERC-20 tokens", and README.md:376, :932, :1055 repeat "top-250", while docs/README.md:323 says "roughly 500". TOKENS in src/shared/tokenList.js has 512 entries, so the docs/ figure is the right one and four README sites are stale. Pre-existing and out of scope here; worth its own issue.
The history/send-selector holder-count divergence in the table is real at the predicates but currently unreachable on live data: both producers coerce an absent count (parseInt(... || "0") at transactions.js:119 and balances.js:73), and the only nullholders comes from parseTx for native transfers, which the history rule already skips via its tx.contractAddress guard. The row is accurate as written; flagging so it is not read as a live bug.
tests/transactions.test.js — the test named "a truthy-but-not-true hideSpoofedSymbols leaves the check on" passes undefined, which is falsy, not truthy. The assertion is correct and worth keeping; only the name misdescribes the case.
Disclosure: tracker CI status ignored per the known runner fault; the runs above are my own in a fresh clone, which was left unmodified.
PASS at `19f2eda` — the three corrected sentences (`README.md:1121-1133`, `README.md:1163-1175`, `docs/README.md:326-333`) are each true at the call sites, both newly-stated table rows check out (`transactions.js:296-300` vs `send.js:135`; `balances.js` never references `fraudContracts`), `ETH` is provably the only `null` mapping (`tokenList.js:3612-3619`, seeded then `has()`-guarded, and `KNOWN_SYMBOLS` is never mutated outside that file), `balances.js` is untouched, nothing from [#179](https://git.eeqj.de/sneak/AutistMask/issues/179) was lost (`transactions.js` differs from `next` only by the gate over the normalised `contract` local) and `TODO.md` went 24 to 25 entries with none removed; `make check` green (274 passed, 1 pre-existing skip, prettier clean), `make test-e2e` 13/13 against a build of `19f2eda`, merges into `next` with no conflicts, single commit ending ` (closes #176)`, base `next`, authored `clawbot`, no attribution trailers.
Three non-blocking notes, none of them defects in this diff:
- `README.md:1115` (unchanged first sentence of the very bullet this PR extends) says the bundled list is the "top 250 ERC-20 tokens", and `README.md:376`, `:932`, `:1055` repeat "top-250", while `docs/README.md:323` says "roughly 500". `TOKENS` in `src/shared/tokenList.js` has 512 entries, so the `docs/` figure is the right one and four README sites are stale. Pre-existing and out of scope here; worth its own issue.
- The history/send-selector holder-count divergence in the table is real at the predicates but currently unreachable on live data: both producers coerce an absent count (`parseInt(... || "0")` at `transactions.js:119` and `balances.js:73`), and the only `null` `holders` comes from `parseTx` for native transfers, which the history rule already skips via its `tx.contractAddress` guard. The row is accurate as written; flagging so it is not read as a live bug.
- `tests/transactions.test.js` — the test named "a truthy-but-not-true hideSpoofedSymbols leaves the check on" passes `undefined`, which is falsy, not truthy. The assertion is correct and worth keeping; only the name misdescribes the case.
Disclosure: tracker CI status ignored per the known runner fault; the runs above are my own in a fresh clone, which was left unmodified.
clawbot
merged commit 74c137dadf into next2026-08-11 15:38:06 +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 #176.
The choice: (a), add the setting
The issue offered (a) add the setting or (b) correct the README. I took (a).
The README's philosophy paragraph is explicit, and the asymmetry was the real
problem: a user who had turned all three other filters off and still saw rows
vanish had no way to find out why, because the responsible check had no
presence in the UI at all. (b) would have had to argue that a definite
symbol/contract mismatch is categorically unlike the three heuristics — true as
far as it goes, but it does not survive the fact that this check is not purely
definite either.
KNOWN_SYMBOLSmapsETHtonull, so every ERC-20 claimingETHis dropped including a real WETH contract emitting that symbol; and thebundled 250-token list is a snapshot, so a genuine token whose entry is stale or
absent is indistinguishable from a spoof. Those are exactly the false-positive
shapes the sharp-tool escape hatch exists for.
The default: ON, and only an explicit
falseturns it offhideSpoofedSymbolsdefaults totrueinDEFAULT_STATE, andloadState()maps an absent stored key to
trueso a profile written before the settingexisted loads protected rather than
undefined(which would read as off andsilently unfilter every existing user on upgrade).
The pure function is deliberately fail-safe in a way the other three flags are
not.
filterTransactions()readsfilters.hideSpoofedSymbols !== false, so acaller that omits the key keeps the check; the other three are plain truthiness
and default off when absent. The reason for the difference is blast radius: the
other three flags failing open means a slightly noisier history, while this one
failing open means the documented fake-
ETHattack renders as a real outboundtransfer to a look-alike address. A safety filter should require an explicit act
to disable, and this makes the code shape match that. It also keeps the existing
with no filters argument only spoof filtering runstest true.The
newFraudContractsinteraction: off means off, learning includedDisabling the setting stops both the hiding and the blocklist learning.
This is not a free choice — the alternative is not merely worse, it does not
work. The blocklist's only writer is this check. If learning continued while
display filtering was off, the contract would be added to
fraudSetduring thesame pass and the very next rule,
hideFraudContracts, is on by default andwould hide the row anyway. The user would toggle the setting and observe no
change. Even ordering around that within a pass, the list is persisted, so the
next refresh hides it. Keeping learning on makes the setting a no-op for anyone
on defaults.
The issue rightly warned that quietly disabling blocklist learning would be a
surprising side effect. It is therefore not quiet: it is stated in
README.md,in the user guide, and in a comment at the call site. An already-populated
blocklist is untouched and keeps filtering —
hideFraudContractsis a separatesetting and this one does not reach it. There is a test pinning that.
In-UI indication that a protection is off: no, deliberately
I considered a persistent banner or a Home-screen marker and decided against
one.
fourth asserts a hierarchy the README does not draw — it presents all four as
peers, and the off-state of the one this PR adds is still partly covered
elsewhere: the send selector applies the same check unconditionally, and the
balance list applies it for every symbol except
ETH(see the table below).No Layout Shift policy it would have to reserve that space unconditionally,
i.e. shrink the useful viewport for every user in order to annotate a state
almost none of them are in.
its three peers, reached the same way it was set.
A cross-cutting "protections are disabled" summary covering all four filters
would be a coherent design, but it is a design decision about the set, not this
one member, and introducing it asymmetrically here would be worse than not
having it. I have not filed it as an issue, since the current arrangement is
deliberate rather than defective.
Scope: the setting governs the transaction history
The known-symbol check exists in three places. Only the transaction-history one
is now gated:
filterTransactions()insrc/shared/transactions.js— gated by the setting.fetchTokenBalances()insrc/shared/balances.js— unchanged, ungated, andnot the same predicate: it requires
legitAddr !== null, so it exemptsthe one symbol
KNOWN_SYMBOLSmaps tonull,ETH. A fake-ETHERC-20 istherefore filtered from history and from the send selector but not from the
balance list. That divergence is pre-existing and out of scope here; it is
filed as #235. The three
documentation sentences that previously claimed uniform coverage now state
the real reach.
renderSendTokenSelect()insrc/popup/views/send.js— unchanged,ungated, and the same predicate as the history check.
The latter two decide which tokens the user can act on rather than what the
history displays, and the README's philosophy paragraph is about showing
everything unfiltered, not about removing guards from an action surface. This
also follows the precedent already set for the balance list, whose
1,000-holder floor is unconditional and was documented as such rather than
being wired to a setting.
What each existing toggle actually gates
Every cell below was re-derived from the code after review found a third
divergence the first pass missed. "Ungated" means the setting is never read on
that surface, which is not the same as the check being identical there.
hideSpoofedSymbols(new)falsenull(ETHalone), so a fake-ETHERC-20 is not filterednull-mapped symbols are caughthideLowHolderTokensnullholder count is never filtered, a0one is(t.holders || 0)reads an absent ornullcount as0and drops it, where history shows ithideFraudContractshideDustTransactionsdustThresholdGwei ?? 100000, and0means hide nothingFour divergences worth flagging, all pre-existing and all untouched here: the
balance list's weaker known-symbol check (#235),
the balance list never consulting the fraud blocklist, the send selector
applying that blocklist regardless of
hideFraudContracts, and the twosurfaces disagreeing on what an unknown holder count means.
Tests, failing first
Tests were written before the implementation.
make teston the unmodifiedsource, 9 failing:
The two
current behaviour:tests intests/transactions.test.jsthat pinnedthe filter as undisableable are inverted rather than deleted, per the issue. New
coverage: active by default, active when the other three are off, active when
the key is absent or
undefined, suppressed on an explicitfalse, noblocklist entry learned while suppressed, the other three rules unaffected, an
already-blocklisted contract still hidden while suppressed, and the flag
round-tripping through
saveState/loadStatein both directions plus theabsent-key migration.
Verification
Rebased onto
nextatfb9e8f5. Two conflicts across the rebases, bothresolved keeping every side: the spoof branch now reads
if (hideSpoofed && isSpoofedSymbol(tx))over the normalisedcontractlocalthat #179 introduced in the
same loop body, and
TODO.mdkeeps its own entry alongside those from#179,
#161 and
#223.
make checkre-run after the rebase — green. The counts rise against thefailing-first run above because the rebase brought in the suites from
#179,
#161 and the crypto
known-answer suite from
#159; the one skip is
pre-existing in the last of those and not from this branch.
make test-e2e— green, real Chrome in the pinned Playwright container, re-runafter the rebase against a build of the branch head
19f2edafrom a cleantree, since the rebase pulled real
src/changes in.Not verified
no coverage of the Settings filter well, and jest runs in the node
environment with no DOM, so the new
$("settings-hide-spoofed-symbols")lookup is not covered by a running test in this branch; if that id were
wrong,
init()would throw and take the whole Settings view with it. Iverified the id statically only — it appears once in
src/popup/index.htmland three times in
src/popup/views/settings.js, all identical. Review hassince closed this gap independently, driving the real view in the pinned
container and confirming that a one-character id typo does break it.
classes of its three siblings, so it should not shift anything, but I did not
look at the popup.
attaches unrelated jobs' results to commits), so whatever it reports here
should be read against the local runs above.
Note for concurrent work
isSpoofedSymbol()is untouched — the gate is added around the call. Thedust-coalescing and address-case bugs in
#179 have since landed on
nextand this branch is rebased onto them; nothing here re-touches that work.f4ee5f779ftod0202fde58FAIL —
needs-rework. One finding.README.md:1090,README.md:1126,docs/README.md:330— newly written text asserts the known-symbol check is applied uniformly on the balance list. It is not, for the exact token the same paragraph uses as its worked example.The three implementations are not the same check:
src/shared/transactions.js:245—if (legit === null) return true;src/popup/views/send.js:122—if (legit === null) return true;src/shared/balances.js:84-89—legitAddr !== undefined && legitAddr !== null && tokenAddr !== legitAddrKNOWN_SYMBOLS.set("ETH", null)(src/shared/tokenList.js:3613) is the only null-mapped entry, andbalances.jsexplicitly exempts it. So an ERC-20 claiming symbolETHis filtered from the transaction history and from the send selector, and is NOT filtered from the balance list.That is precisely the token the same README bullet describes two sentences earlier — "The fake 'Ethereum' token in the attack above used symbol 'ETH' ... so it would be caught by this check" — after which the new sentences tell the reader "The same check on the balance list and on the send-screen token selector is unconditional" (
README.md:1090), "The known-symbol check also runs unconditionally on the balance list" (README.md:1126) and "Your balances and the send token list always apply the check" (docs/README.md:330). Read together these say a user who switches the new setting off is still covered on balances. For the fake-ETHcase they are not.Reachability:
src/shared/balances.js:79drops unknown and untracked tokens under 1,000 holders, so the gap needs a poisoning contract with an inflated holder count, or a token the user has tracked manually. Both are ordinary — inflating the holder count is what the attack already does to reach the history.Why it matters: this PR's purpose is to make the README true about which protections are configurable and how far each reaches, and the PR body states the table was "checked at each call site rather than assumed from the name" and reports the surprises found. The third call site was audited as identical when it is not, so the diff replaces one inaccurate README claim with another, in the security section, about the headline attack.
Acceptable: state the balance list's real reach in
README.mdanddocs/README.md— the known-symbol check runs on it unconditionally except for symbols mapped tonull(ETH), which it does not filter — and correct the PR body's table row. Thebalances.jsdivergence itself is pre-existing and rightly out of scope; file it as its own issue rather than fixing it here.Verified and passing. The disclosed blind spot is closed: I drove the Settings view for real in the pinned Playwright container — the view renders, the checkbox is present and checked by default, unchecking persists
hideSpoofedSymbols: falseto extension storage, the state survives a popup reopen, a profile blob with the key deleted migrates back to on, and zero uncaught browser errors orconsole.errors were recorded. That probe has teeth: a one-character id typo insettings.jsreproducedpageerror: Cannot set properties of null (setting 'checked')with the view never rendering, whilemake teststayed fully green. The fail-closed!== falseattransactions.js:263is the only gate on the flag, so nothing reintroduces fail-open, and it is what keeps the check on even if theloadStatemigration is removed. The coupling claim holds attransactions.js:267-282: learning without hiding adds the contract tofraudSetand the very next rule, on by default, hides the row. Tests have teeth — flipping theDEFAULT_STATEdefault, dropping!== false, dropping theloadStatemigration and removing the gate each failed 1-2 targeted tests.make checkgreen (258 passed, 1 pre-existing skip),make test-e2e4/4 against a build ofd0202fd,make fmtclean, single commit titled(closes #176), basenext, authoredclawbot, fast-forwards ontonext, no Claude or Anthropic references, no attribution trailers,TODO.mdgrew 21 to 22 entries with none removed, andisSpoofedSymbolplus the dust rule are untouched as #179 requires.Disclosure: to drive the Settings view I created and then deleted an untracked scratch probe file in my own clone and ran it against the same pinned image
script/test-e2euses; nothing tracked was modified and the working tree was verified clean afterwards. Tracker CI status was ignored per the known runner fault — the runs above are my own.d0202fde58to9b7e18063c9b7e18063cto19f2eda768PASS at
19f2eda— the three corrected sentences (README.md:1121-1133,README.md:1163-1175,docs/README.md:326-333) are each true at the call sites, both newly-stated table rows check out (transactions.js:296-300vssend.js:135;balances.jsnever referencesfraudContracts),ETHis provably the onlynullmapping (tokenList.js:3612-3619, seeded thenhas()-guarded, andKNOWN_SYMBOLSis never mutated outside that file),balances.jsis untouched, nothing from #179 was lost (transactions.jsdiffers fromnextonly by the gate over the normalisedcontractlocal) andTODO.mdwent 24 to 25 entries with none removed;make checkgreen (274 passed, 1 pre-existing skip, prettier clean),make test-e2e13/13 against a build of19f2eda, merges intonextwith no conflicts, single commit ending(closes #176), basenext, authoredclawbot, no attribution trailers.Three non-blocking notes, none of them defects in this diff:
README.md:1115(unchanged first sentence of the very bullet this PR extends) says the bundled list is the "top 250 ERC-20 tokens", andREADME.md:376,:932,:1055repeat "top-250", whiledocs/README.md:323says "roughly 500".TOKENSinsrc/shared/tokenList.jshas 512 entries, so thedocs/figure is the right one and four README sites are stale. Pre-existing and out of scope here; worth its own issue.parseInt(... || "0")attransactions.js:119andbalances.js:73), and the onlynullholderscomes fromparseTxfor native transfers, which the history rule already skips via itstx.contractAddressguard. The row is accurate as written; flagging so it is not read as a live bug.tests/transactions.test.js— the test named "a truthy-but-not-true hideSpoofedSymbols leaves the check on" passesundefined, which is falsy, not truthy. The assertion is correct and worth keeping; only the name misdescribes the case.Disclosure: tracker CI status ignored per the known runner fault; the runs above are my own in a fresh clone, which was left unmodified.