Two defects in src/shared/transactions.js, both silent over-filtering: the
wallet hid transactions the user had asked to see.
1. A dust threshold of 0
filters.dustThresholdGwei || 100000 swallowed the one value a user would
pick to mean "show everything". It is now ??, computed once before the
filter loop, so 0 is a real threshold: no transaction has a value below
0 gwei, so nothing is hidden. That makes it exactly equivalent to clearing the
hide-dust checkbox, and neither control can override the other. README.md
now says so in the dust-filtering bullet.
src/shared/state.js already distinguished unset from zero on load
(saved.dustThresholdGwei !== undefined), so a stored 0 survives a reload.
Settings input (src/popup/views/settings.js:305)
Was parseInt(value, 10) accepted when !isNaN(val) && val >= 0. What each
input does now, and what changed:
input
before
now
0
stored as 0, then ignored by the filter
stored as 0, honoured — hides nothing
empty / whitespace
rejected, field kept showing the empty box
rejected, field resyncs to the stored threshold
negative (-5)
rejected, field kept showing -5
rejected, field resyncs
non-numeric (abc)
rejected, field kept showing abc
rejected, field resyncs
trailing junk (100 gwei)
accepted as 100 by parseInt
rejected, field resyncs
fractional (1.5)
accepted as 1 by parseInt
rejected, field resyncs
Rejected input never mutates state, and the field is put back to the stored
value so it cannot display a threshold the wallet is not using.
2. isSpoofedSymbol was case-sensitive on the contract address
EIP-55 mixed case is a checksum over the address, not part of its identity, so tx.contractAddress !== legit classified a genuine token arriving checksummed
as a spoof — hiding a real transfer and recording the real contract on the
fraud blocklist. Every address comparison in the module now goes through one normalizeAddress() helper (parseTx direction and token lookup, parseTokenTransfer direction and contract, fetchRecentTransactions, isSpoofedSymbol, the fraud set and its lookups). The contract recorded in newFraudContracts is normalised too, so the persisted blocklist matches
later transfers in any casing.
mergeTransactions() and its tests from #177 are untouched.
Failing first
Six new tests, run against the current src/shared/transactions.js with only
the test file changed:
● known-symbol spoof verification › a genuine contract in EIP-55 checksummed form is not a spoof
● known-symbol spoof verification › a genuine contract in all-uppercase form is not a spoof
● known-symbol spoof verification › a genuinely different contract claiming USDC is still a spoof in any casing
● dust threshold filtering › a threshold of 0 hides nothing, leaving the toggle on
● dust threshold filtering › a threshold of 0 agrees with clearing the hide-dust checkbox
● dust threshold filtering › 0, unset and a set threshold are three distinct behaviours
Test Suites: 1 failed, 8 passed, 9 total
Tests: 6 failed, 173 passed, 179 total
The checksummed-genuine and threshold-0 cases fail the same way — the
transaction the user should see is missing:
● dust threshold filtering › a threshold of 0 hides nothing, leaving the toggle on
expect(received).toEqual(expected) // deep equality
- Expected - 22
+ Received + 1
- Array [
- Object {
- "blockNumber": 21000000,
- "contractAddress": null,
- "valueGwei": 50,
...
+ Array []
And the fraud contract was recorded in whatever casing it arrived in:
● known-symbol spoof verification › a genuinely different contract claiming USDC is still a spoof in any casing
- Expected - 1
+ Received + 1
Array [
- "0xd05339f9ea5ab9d9f03b9d57f671d2abd1f55c82",
+ "0xD05339F9EA5AB9D9F03B9D57F671D2ABD1F55C82",
]
All six pass with the fix. Both current behaviour: tests named in the issue
(the checksummed spoof and the threshold-0 fallback) are inverted into
regression guards; the other two current behaviour: tests, which document
spoof filtering not being disableable, are left alone — that is #176's territory, and no
settings toggle is added here.
Mutation survivor from the issue comment
tx.holders !== null at the low-holder rule had no fixture reaching it. Added
one pairing a real contractAddress with holders: null, asserting it is not
filtered. Deleting the guard now fails it:
● low-holder token filtering (the 1,000-holder rule) › a token whose holder count is unknown is not filtered
- Expected - 20
+ Received + 1
- Array [
- Object {
- "contractAddress": "0x1111111111111111111111111111111111111111",
...
+ Array []
Tests: 1 failed, 178 passed, 179 total
Guard restored afterwards; the rest of the suite was unaffected by the
mutation, so that test is what kills it.
holders: undefined is not reachable from the Blockscout response shape,
so no fixture was added for it: parseTokenTransfer computes parseInt(tt.token?.holders_count || "0", 10), and parseTx sets holders: null outright. Checked, not assumed.
||-as-default audit of src/shared/
Only one numeric-default instance was in src/shared/, and it is the one
fixed here. Two adjacent cases found and not fixed (out of scope, worth a
decision):
src/shared/transactions.js:112 — parseInt(tt.token?.holders_count || "0", 10).
A token whose holder count the explorer omits (rate limit, self-hosted
instance) is parsed as 0 holders and then hidden by the low-holder rule.
This is the same over-filtering harm the holders !== null guard exists to
prevent, one layer earlier: the guard can never fire for a token transfer,
because the parser never produces null for one. Fixing it means emitting null when the field is absent.
src/popup/views/send.js:135 — (t.holders || 0) < 1000 hides a token with
an unknown holder count from the send selector, same shape, outside src/shared/.
Benign (0 is the intended default): src/shared/state.js:97 and src/background/index.js:599, both lastBalanceRefresh || 0.
Verification
make check on the rebased branch: 10 suites, 251 passed, 1 skipped, 252
total; prettier clean. The skip is the pre-existing test.skip at tests/wallet.test.js:314, from #159, not from this
branch.
docker build --no-cache . (via the Dockerfile's make check and make build) executed uncached and green: Tests: 179 passed, 179 total, All matched files use Prettier code style!, and verify-build confirmed 4
bundles with autistmask-build-debug=off. Run before the rebase onto f455b0a; the host make check above is post-rebase.
Rebased onto next at f455b0a immediately before pushing. The only
conflict was the TODO.md Completed Steps list; all sides' entries kept.
Closes [#179](https://git.eeqj.de/sneak/AutistMask/issues/179).
Two defects in `src/shared/transactions.js`, both silent over-filtering: the
wallet hid transactions the user had asked to see.
## 1. A dust threshold of `0`
`filters.dustThresholdGwei || 100000` swallowed the one value a user would
pick to mean "show everything". It is now `??`, computed once before the
filter loop, so `0` is a real threshold: no transaction has a value below
0 gwei, so nothing is hidden. That makes it exactly equivalent to clearing the
hide-dust checkbox, and neither control can override the other. `README.md`
now says so in the dust-filtering bullet.
`src/shared/state.js` already distinguished unset from zero on load
(`saved.dustThresholdGwei !== undefined`), so a stored `0` survives a reload.
### Settings input (`src/popup/views/settings.js:305`)
Was `parseInt(value, 10)` accepted when `!isNaN(val) && val >= 0`. What each
input does now, and what changed:
| input | before | now |
| --- | --- | --- |
| `0` | stored as 0, then ignored by the filter | stored as 0, honoured — hides nothing |
| empty / whitespace | rejected, field kept showing the empty box | rejected, field resyncs to the stored threshold |
| negative (`-5`) | rejected, field kept showing `-5` | rejected, field resyncs |
| non-numeric (`abc`) | rejected, field kept showing `abc` | rejected, field resyncs |
| trailing junk (`100 gwei`) | **accepted as 100** by `parseInt` | rejected, field resyncs |
| fractional (`1.5`) | **accepted as 1** by `parseInt` | rejected, field resyncs |
Rejected input never mutates state, and the field is put back to the stored
value so it cannot display a threshold the wallet is not using.
## 2. `isSpoofedSymbol` was case-sensitive on the contract address
EIP-55 mixed case is a checksum over the address, not part of its identity, so
`tx.contractAddress !== legit` classified a genuine token arriving checksummed
as a spoof — hiding a real transfer and recording the real contract on the
fraud blocklist. Every address comparison in the module now goes through one
`normalizeAddress()` helper (`parseTx` direction and token lookup,
`parseTokenTransfer` direction and contract, `fetchRecentTransactions`,
`isSpoofedSymbol`, the fraud set and its lookups). The contract recorded in
`newFraudContracts` is normalised too, so the persisted blocklist matches
later transfers in any casing.
`mergeTransactions()` and its tests from
[#177](https://git.eeqj.de/sneak/AutistMask/issues/177) are untouched.
## Failing first
Six new tests, run against the current `src/shared/transactions.js` with only
the test file changed:
```
● known-symbol spoof verification › a genuine contract in EIP-55 checksummed form is not a spoof
● known-symbol spoof verification › a genuine contract in all-uppercase form is not a spoof
● known-symbol spoof verification › a genuinely different contract claiming USDC is still a spoof in any casing
● dust threshold filtering › a threshold of 0 hides nothing, leaving the toggle on
● dust threshold filtering › a threshold of 0 agrees with clearing the hide-dust checkbox
● dust threshold filtering › 0, unset and a set threshold are three distinct behaviours
Test Suites: 1 failed, 8 passed, 9 total
Tests: 6 failed, 173 passed, 179 total
```
The checksummed-genuine and threshold-0 cases fail the same way — the
transaction the user should see is missing:
```
● dust threshold filtering › a threshold of 0 hides nothing, leaving the toggle on
expect(received).toEqual(expected) // deep equality
- Expected - 22
+ Received + 1
- Array [
- Object {
- "blockNumber": 21000000,
- "contractAddress": null,
- "valueGwei": 50,
...
+ Array []
```
And the fraud contract was recorded in whatever casing it arrived in:
```
● known-symbol spoof verification › a genuinely different contract claiming USDC is still a spoof in any casing
- Expected - 1
+ Received + 1
Array [
- "0xd05339f9ea5ab9d9f03b9d57f671d2abd1f55c82",
+ "0xD05339F9EA5AB9D9F03B9D57F671D2ABD1F55C82",
]
```
All six pass with the fix. Both `current behaviour:` tests named in the issue
(the checksummed spoof and the threshold-0 fallback) are inverted into
regression guards; the other two `current behaviour:` tests, which document
spoof filtering not being disableable, are left alone — that is
[#176](https://git.eeqj.de/sneak/AutistMask/issues/176)'s territory, and no
settings toggle is added here.
## Mutation survivor from the issue comment
`tx.holders !== null` at the low-holder rule had no fixture reaching it. Added
one pairing a real `contractAddress` with `holders: null`, asserting it is not
filtered. Deleting the guard now fails it:
```
● low-holder token filtering (the 1,000-holder rule) › a token whose holder count is unknown is not filtered
- Expected - 20
+ Received + 1
- Array [
- Object {
- "contractAddress": "0x1111111111111111111111111111111111111111",
...
+ Array []
Tests: 1 failed, 178 passed, 179 total
```
Guard restored afterwards; the rest of the suite was unaffected by the
mutation, so that test is what kills it.
`holders: undefined` is **not** reachable from the Blockscout response shape,
so no fixture was added for it: `parseTokenTransfer` computes
`parseInt(tt.token?.holders_count || "0", 10)`, and `parseTx` sets
`holders: null` outright. Checked, not assumed.
## `||`-as-default audit of `src/shared/`
Only one numeric-default instance was in `src/shared/`, and it is the one
fixed here. Two adjacent cases found and **not** fixed (out of scope, worth a
decision):
- `src/shared/transactions.js:112` — `parseInt(tt.token?.holders_count || "0", 10)`.
A token whose holder count the explorer omits (rate limit, self-hosted
instance) is parsed as **0 holders** and then hidden by the low-holder rule.
This is the same over-filtering harm the `holders !== null` guard exists to
prevent, one layer earlier: the guard can never fire for a token transfer,
because the parser never produces `null` for one. Fixing it means emitting
`null` when the field is absent.
- `src/popup/views/send.js:135` — `(t.holders || 0) < 1000` hides a token with
an unknown holder count from the send selector, same shape, outside
`src/shared/`.
Benign (0 is the intended default): `src/shared/state.js:97` and
`src/background/index.js:599`, both `lastBalanceRefresh || 0`.
## Verification
- `make check` on the rebased branch: 10 suites, 251 passed, 1 skipped, 252
total; prettier clean. The skip is the pre-existing `test.skip` at
`tests/wallet.test.js:314`, from
[#159](https://git.eeqj.de/sneak/AutistMask/issues/159), not from this
branch.
- `docker build --no-cache .` (via the Dockerfile's `make check` and
`make build`) executed uncached and green: `Tests: 179 passed, 179 total`,
`All matched files use Prettier code style!`, and `verify-build` confirmed 4
bundles with `autistmask-build-debug=off`. Run before the rebase onto
`f455b0a`; the host `make check` above is post-rebase.
- Rebased onto `next` at `f455b0a` immediately before pushing. The only
conflict was the `TODO.md` Completed Steps list; all sides' entries kept.
Two defects in the anti-poisoning filters, both silent over-filtering: the
wallet hid transactions the user had asked to see.
A dust threshold of 0 was read as `filters.dustThresholdGwei || 100000`, so
the one value a user would pick to mean "show everything" was swallowed and
replaced by the default. It is now `??`, making 0 a real threshold that hides
nothing and agrees exactly with clearing the hide-dust checkbox. The Settings
input rejects empty, negative, fractional and non-numeric entries outright
instead of coercing them, and resyncs the field to the stored value so it
never displays a threshold the wallet is not using.
isSpoofedSymbol compared the contract address with `===` against a lowercased
known address. EIP-55 mixed case is a checksum, not identity, so a genuine
token arriving checksummed was classified as a spoof and hidden. All address
comparisons in the module now go through one normalizeAddress helper, which
also normalises the fraud contracts recorded from a detected spoof.
Tests cover threshold 0 versus unset versus a set value, and the contract
comparison in lowercase, uppercase and EIP-55 form as well as against a
genuinely different address. The two `current behaviour:` tests pinning the
old behaviour are inverted into regression guards, and a fixture pairing a
real contract address with a null holder count pins the `tx.holders !== null`
guard that no fixture previously reached.
PASS — independent review of #228 against #179: all DoD items met, every normalized site verified behaviour-neutral except the two intended ones, no fraud-set migration break, tests have teeth, make check and script/cibuild green on ea1fcd4, mergeable on next, policy clean.
Flags (none blocking):
src/popup/views/settings.js:307-317 — rejected input reverts the field silently, but this same file already uses showFlash() with full-sentence messages for invalid RPC and Blockscout URLs (:198, :243), which is the idiom README "Language & Labeling" describes. A user typing 100 gwei or 1.5 (both accepted before this PR) now sees the box snap back to the stored number with no explanation. Not a documented-rule break, so not a fail — but a showFlash("Please enter a whole number of gwei, or 0 to hide nothing.") alongside the resync would be strictly better and is a one-line change.
src/popup/views/settings.js:308 — Number(raw) accepts non-decimal notation the old parseInt(raw, 10) did not: 0x10 stores 16 and 1e3 stores 1000. Harmless and no worse than before (parseInt("0x10", 10) stored 0), just wider than the PR table states.
src/shared/transactions.js:305 — a stored non-numeric threshold makes tx.valueGwei < dustThresholdGwei NaN-false, i.e. it fails open and hides nothing rather than defaulting. Unreachable through the new Settings validation and identical to the pre-PR behaviour under ||; noted only because it is silent.
Disclosure: mutation testing was done in a scratch clone working tree and reverted; git status verified clean before the script/cibuild run whose result is cited (layer #11 make check executed, not CACHED: 251 passed, 1 skipped, prettier clean).
PASS — independent review of [#228](https://git.eeqj.de/sneak/AutistMask/pulls/228) against [#179](https://git.eeqj.de/sneak/AutistMask/issues/179): all DoD items met, every normalized site verified behaviour-neutral except the two intended ones, no fraud-set migration break, tests have teeth, `make check` and `script/cibuild` green on `ea1fcd4`, mergeable on `next`, policy clean.
Flags (none blocking):
- `src/popup/views/settings.js:307-317` — rejected input reverts the field silently, but this same file already uses `showFlash()` with full-sentence messages for invalid RPC and Blockscout URLs (`:198`, `:243`), which is the idiom README "Language & Labeling" describes. A user typing `100 gwei` or `1.5` (both accepted before this PR) now sees the box snap back to the stored number with no explanation. Not a documented-rule break, so not a fail — but a `showFlash("Please enter a whole number of gwei, or 0 to hide nothing.")` alongside the resync would be strictly better and is a one-line change.
- `src/popup/views/settings.js:308` — `Number(raw)` accepts non-decimal notation the old `parseInt(raw, 10)` did not: `0x10` stores 16 and `1e3` stores 1000. Harmless and no worse than before (`parseInt("0x10", 10)` stored 0), just wider than the PR table states.
- `src/shared/transactions.js:305` — a stored non-numeric threshold makes `tx.valueGwei < dustThresholdGwei` NaN-false, i.e. it fails open and hides nothing rather than defaulting. Unreachable through the new Settings validation and identical to the pre-PR behaviour under `||`; noted only because it is silent.
Disclosure: mutation testing was done in a scratch clone working tree and reverted; `git status` verified clean before the `script/cibuild` run whose result is cited (layer `#11 make check` executed, not `CACHED`: 251 passed, 1 skipped, prettier clean).
clawbot
merged commit 12acf4dc8c into next2026-08-11 15:16:49 +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 #179.
Two defects in
src/shared/transactions.js, both silent over-filtering: thewallet hid transactions the user had asked to see.
1. A dust threshold of
0filters.dustThresholdGwei || 100000swallowed the one value a user wouldpick to mean "show everything". It is now
??, computed once before thefilter loop, so
0is a real threshold: no transaction has a value below0 gwei, so nothing is hidden. That makes it exactly equivalent to clearing the
hide-dust checkbox, and neither control can override the other.
README.mdnow says so in the dust-filtering bullet.
src/shared/state.jsalready distinguished unset from zero on load(
saved.dustThresholdGwei !== undefined), so a stored0survives a reload.Settings input (
src/popup/views/settings.js:305)Was
parseInt(value, 10)accepted when!isNaN(val) && val >= 0. What eachinput does now, and what changed:
0-5)-5abc)abc100 gwei)parseInt1.5)parseIntRejected input never mutates state, and the field is put back to the stored
value so it cannot display a threshold the wallet is not using.
2.
isSpoofedSymbolwas case-sensitive on the contract addressEIP-55 mixed case is a checksum over the address, not part of its identity, so
tx.contractAddress !== legitclassified a genuine token arriving checksummedas a spoof — hiding a real transfer and recording the real contract on the
fraud blocklist. Every address comparison in the module now goes through one
normalizeAddress()helper (parseTxdirection and token lookup,parseTokenTransferdirection and contract,fetchRecentTransactions,isSpoofedSymbol, the fraud set and its lookups). The contract recorded innewFraudContractsis normalised too, so the persisted blocklist matcheslater transfers in any casing.
mergeTransactions()and its tests from#177 are untouched.
Failing first
Six new tests, run against the current
src/shared/transactions.jswith onlythe test file changed:
The checksummed-genuine and threshold-0 cases fail the same way — the
transaction the user should see is missing:
And the fraud contract was recorded in whatever casing it arrived in:
All six pass with the fix. Both
current behaviour:tests named in the issue(the checksummed spoof and the threshold-0 fallback) are inverted into
regression guards; the other two
current behaviour:tests, which documentspoof filtering not being disableable, are left alone — that is
#176's territory, and no
settings toggle is added here.
Mutation survivor from the issue comment
tx.holders !== nullat the low-holder rule had no fixture reaching it. Addedone pairing a real
contractAddresswithholders: null, asserting it is notfiltered. Deleting the guard now fails it:
Guard restored afterwards; the rest of the suite was unaffected by the
mutation, so that test is what kills it.
holders: undefinedis not reachable from the Blockscout response shape,so no fixture was added for it:
parseTokenTransfercomputesparseInt(tt.token?.holders_count || "0", 10), andparseTxsetsholders: nulloutright. Checked, not assumed.||-as-default audit ofsrc/shared/Only one numeric-default instance was in
src/shared/, and it is the onefixed here. Two adjacent cases found and not fixed (out of scope, worth a
decision):
src/shared/transactions.js:112—parseInt(tt.token?.holders_count || "0", 10).A token whose holder count the explorer omits (rate limit, self-hosted
instance) is parsed as 0 holders and then hidden by the low-holder rule.
This is the same over-filtering harm the
holders !== nullguard exists toprevent, one layer earlier: the guard can never fire for a token transfer,
because the parser never produces
nullfor one. Fixing it means emittingnullwhen the field is absent.src/popup/views/send.js:135—(t.holders || 0) < 1000hides a token withan unknown holder count from the send selector, same shape, outside
src/shared/.Benign (0 is the intended default):
src/shared/state.js:97andsrc/background/index.js:599, bothlastBalanceRefresh || 0.Verification
make checkon the rebased branch: 10 suites, 251 passed, 1 skipped, 252total; prettier clean. The skip is the pre-existing
test.skipattests/wallet.test.js:314, from#159, not from this
branch.
docker build --no-cache .(via the Dockerfile'smake checkandmake build) executed uncached and green:Tests: 179 passed, 179 total,All matched files use Prettier code style!, andverify-buildconfirmed 4bundles with
autistmask-build-debug=off. Run before the rebase ontof455b0a; the hostmake checkabove is post-rebase.nextatf455b0aimmediately before pushing. The onlyconflict was the
TODO.mdCompleted Steps list; all sides' entries kept.PASS — independent review of #228 against #179: all DoD items met, every normalized site verified behaviour-neutral except the two intended ones, no fraud-set migration break, tests have teeth,
make checkandscript/cibuildgreen onea1fcd4, mergeable onnext, policy clean.Flags (none blocking):
src/popup/views/settings.js:307-317— rejected input reverts the field silently, but this same file already usesshowFlash()with full-sentence messages for invalid RPC and Blockscout URLs (:198,:243), which is the idiom README "Language & Labeling" describes. A user typing100 gweior1.5(both accepted before this PR) now sees the box snap back to the stored number with no explanation. Not a documented-rule break, so not a fail — but ashowFlash("Please enter a whole number of gwei, or 0 to hide nothing.")alongside the resync would be strictly better and is a one-line change.src/popup/views/settings.js:308—Number(raw)accepts non-decimal notation the oldparseInt(raw, 10)did not:0x10stores 16 and1e3stores 1000. Harmless and no worse than before (parseInt("0x10", 10)stored 0), just wider than the PR table states.src/shared/transactions.js:305— a stored non-numeric threshold makestx.valueGwei < dustThresholdGweiNaN-false, i.e. it fails open and hides nothing rather than defaulting. Unreachable through the new Settings validation and identical to the pre-PR behaviour under||; noted only because it is silent.Disclosure: mutation testing was done in a scratch clone working tree and reverted;
git statusverified clean before thescript/cibuildrun whose result is cited (layer#11 make checkexecuted, notCACHED: 251 passed, 1 skipped, prettier clean).