fix: add a Settings toggle for known-symbol spoof verification (closes #176) #226

Merged

clawbot merged 1 commits from fix/issue-176-spoof-verification-toggle into next 2026-08-11 15:38:06 +02:00

Conversation 2 Commits 1 Files Changed 12 +234 -28

clawbot commented 2026-08-11 15:06:53 +02:00

Collaborator

Copy Link

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

clawbot added 1 commit 2026-08-11 15:06:54 +02:00

fix: add a Settings toggle for known-symbol spoof verification (closes #176) ... f4ee5f779f

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.

clawbot added the needs-review label 2026-08-11 15:07:05 +02:00

clawbot self-assigned this 2026-08-11 15:07:33 +02:00

clawbot force-pushed fix/issue-176-spoof-verification-toggle from f4ee5f779f to d0202fde58 2026-08-11 15:07:42 +02:00 Compare

clawbot commented 2026-08-11 15:19:35 +02:00

Author

Collaborator

Copy Link

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

clawbot added needs-rework and removed needs-review labels 2026-08-11 15:20:22 +02:00

clawbot referenced this pull request 2026-08-11 15:20:53 +02:00

fix: a fake token impersonating ETH is filtered from history and send, but still shows in the balance list #235

clawbot force-pushed fix/issue-176-spoof-verification-toggle from d0202fde58 to 9b7e18063c 2026-08-11 15:23:52 +02:00 Compare

clawbot force-pushed fix/issue-176-spoof-verification-toggle from 9b7e18063c to 19f2eda768 2026-08-11 15:28:36 +02:00 Compare

clawbot added needs-review and removed needs-rework labels 2026-08-11 15:32:27 +02:00

clawbot commented 2026-08-11 15:37:37 +02:00

Author

Collaborator

Copy Link

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

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 next 2026-08-11 15:38:06 +02:00

clawbot deleted branch fix/issue-176-spoof-verification-toggle 2026-08-11 15:38:06 +02:00

clawbot referenced this pull request 2026-08-11 15:38:18 +02:00

docs: the bundled token list is described as "top 250" in four places and "roughly 500" in another; it is 512 #239

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label needs-review

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#226