harden: parseHoldersCount partial-parses a malformed count into a reported low count, hiding the token #251

New Issue

2026-08-12T10:35:02+02:00

clawbot commented

2026-08-12 10:35:02 +02:00

Three findings from the independent review of #244, all non-blocking there and filed rather than dropped.

1. The parse contract is overstated, and the gap hides tokens

src/shared/holders.js:13-18 says "Anything unparseable is unknown too". parseInt partial-parses instead:

"1,000" → 1
"0x10" → 0
"1e3" → 1

A malformed count that merely starts with a digit becomes a reported low count, which triggers exactly the hide that #230 exists to prevent — and it does so on the confident path, not the unknown path, so the new holders !== null protection never sees it.

Not a live bug today: this is the same parseInt as before the change, and Blockscout emits plain decimal strings. It is a latent one, and the comment currently promises a guarantee the code does not provide. Either make the parse total (reject anything that is not wholly a decimal integer, yielding null) or correct the comment to state what parseInt actually does. The first is better — "unparseable means unknown" is the rule the rest of the module is built on.

2. An inert guard

src/shared/balances.js:87 — holders !== null && is dead. null >= 1000 is already false, so removing the clause survives all 388 tests. Either drop it or make it load-bearing; as written it reads as a protection that is doing nothing.

3. Undocumented behaviour, same drift class as #239

README.md:1223-1263 and docs/README.md:344-347 enumerate each filter's reach in detail and say nothing about how an unknown holder count is treated — even though the treatment now deliberately differs between the history filter, the Send selector and the balance-list spam gate. Not falsified (an unknown count is not "fewer than 1,000"), but this is precisely the documentation drift #239 was filed for.

Also worth recording while in here: the review noted src/popup/views/addressToken.js:189 now omits the "Holders:" row rather than showing a false "Holders: 0". That is an improvement and was an undisclosed side effect of #244; it should be documented rather than reverted.

Definition of done

A holders_count that is not wholly a decimal integer yields null, not a partial parse. Tests cover "1,000", "0x10", "1e3" and a trailing-garbage case.
The comment in src/shared/holders.js matches what the code does.
The inert clause at src/shared/balances.js:87 is either removed or made load-bearing, with a test either way.
README.md and docs/README.md state how an unknown holder count is treated in each of the three filters.
TODO.md updated in the same commit.
make check passes.

Three findings from the independent review of https://git.eeqj.de/sneak/AutistMask/pulls/244, all non-blocking there and filed rather than dropped. ## 1. The parse contract is overstated, and the gap hides tokens `src/shared/holders.js:13-18` says "Anything unparseable is unknown too". `parseInt` partial-parses instead: - `"1,000"` → `1` - `"0x10"` → `0` - `"1e3"` → `1` A malformed count that merely starts with a digit becomes a **reported low count**, which triggers exactly the hide that https://git.eeqj.de/sneak/AutistMask/issues/230 exists to prevent — and it does so on the confident path, not the unknown path, so the new `holders !== null` protection never sees it. Not a live bug today: this is the same `parseInt` as before the change, and Blockscout emits plain decimal strings. It is a latent one, and the comment currently promises a guarantee the code does not provide. Either make the parse total (reject anything that is not wholly a decimal integer, yielding `null`) or correct the comment to state what `parseInt` actually does. The first is better — "unparseable means unknown" is the rule the rest of the module is built on. ## 2. An inert guard `src/shared/balances.js:87` — `holders !== null &&` is dead. `null >= 1000` is already `false`, so removing the clause survives all 388 tests. Either drop it or make it load-bearing; as written it reads as a protection that is doing nothing. ## 3. Undocumented behaviour, same drift class as #239 `README.md:1223-1263` and `docs/README.md:344-347` enumerate each filter's reach in detail and say nothing about how an unknown holder count is treated — even though the treatment now deliberately differs between the history filter, the Send selector and the balance-list spam gate. Not falsified (an unknown count is not "fewer than 1,000"), but this is precisely the documentation drift https://git.eeqj.de/sneak/AutistMask/issues/239 was filed for. Also worth recording while in here: the review noted `src/popup/views/addressToken.js:189` now omits the "Holders:" row rather than showing a false "Holders: 0". That is an improvement and was an undisclosed side effect of #244; it should be documented rather than reverted. ## Definition of done - [ ] A `holders_count` that is not wholly a decimal integer yields `null`, not a partial parse. Tests cover `"1,000"`, `"0x10"`, `"1e3"` and a trailing-garbage case. - [ ] The comment in `src/shared/holders.js` matches what the code does. - [ ] The inert clause at `src/shared/balances.js:87` is either removed or made load-bearing, with a test either way. - [ ] `README.md` and `docs/README.md` state how an unknown holder count is treated in each of the three filters. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.

clawbot commented

2026-08-12 10:48:55 +02:00

Add to the documentation item: the README.md source-tree listing omits src/shared/holders.js entirely. Found while landing #257, which added its own new module to that listing rather than fixing the neighbouring gap.

Add to the documentation item: the `README.md` source-tree listing omits `src/shared/holders.js` entirely. Found while landing https://git.eeqj.de/sneak/AutistMask/pulls/257, which added its own new module to that listing rather than fixing the neighbouring gap.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#251