fix: floor the persisted fields a restore dereferences, and make each field's floor an executable claim (closes #362)
All checks were successful
check / check (push) Successful in 40s
e2e / e2e-chrome (push) Successful in 1m45s
e2e / e2e-firefox (push) Successful in 32s

A persisted container was checked while its ENTRIES were dereferenced
unchecked. A stored `{"0x…": "notalist"}` in allowedSites passes the state
gate, renders a working popup, and then throws inside saveState()'s per-
hostname merge, so every save from that moment on fails while the UI looks
entirely healthy. deniedSites has the identical shape; fraudContracts is the
same class with a milder consequence.

The sweep for that class found four more:

- selectedToken, dereferenced as text behind a truthiness-only restore gate.
- rpcUrl, handed whole to `new JsonRpcProvider()` by getProvider(), which
  throws SYNCHRONOUSLY for a non-string — from txStatus.js and addWallet.js,
  neither inside a try, and the first reachable from a stored
  `currentView: "wait-tx"` through the unguarded restoreView().
- The ENTRIES of viewData. Four restore branches gate on one truthy field and
  hand the rest to a renderer that calls address.toLowerCase(): a stored
  `{"currentView":"success-tx","viewData":{"hash":"0x1"}}` throws out of
  restoreView(), skipping the rest of popup init.
- selectedWallet / selectedAddress. `wallets` is a real Array, so a stored
  "map", "length", "constructor" or "__proto__" is TRUTHY: hasValidAddress()'s
  `&&` does not short-circuit and `.addresses[…]` throws. A stale INTEGER index
  is the safe case.

Floors, in src/shared/persistedState.js: allowedSites/deniedSites through
siteMap(), fraudContracts and each hostname list through textList(),
selectedToken and activeAddress as text-or-null, rpcUrl and blockscoutUrl as
non-empty text, selectedWallet and selectedAddress as a non-negative integer
or null, and each networkEndpoints pair's two URL fields — which
applyChainSwitchFields() assigns straight onto s.rpcUrl on the next switch.

Guards, in src/popup/viewRouter.js: the four restore branches that gate on one
truthy field now check the entries their renderer dereferences, as
txStatus.restoreWait() has always done for wait-tx. "confirm-tx" joins
ADDRESS_VIEWS, because its Sign button dereferences
state.wallets[state.selectedWallet] behind no guard of its own.

A stored own "__proto__" key is dropped by siteMap(): it can never be a wallet
address, so it grants nothing, and keeping it only keeps a value the next save
would hand to the prototype setter. networkEndpoints keeps unknown keys by
design, so mergeMapByKey() in src/shared/state.js now writes with
defineProperty as well — the guard in the floor was being undone one layer
downstream.

A save that fails is also told, not merely repaired: onSaveFailure() reports
every failed save, awaited or not (the save queue's own rejection handler is
what made a failure vanish), and the popup raises a persistent "NOT SAVED"
banner naming the reason. doRefreshAndRender() no longer rejects, since every
one of its call sites fires it and walks away.

The per-field justification in the header of src/shared/stateSchema.js is
replaced by tests/persistedFieldContract.test.js. That comment shipped a false
claim in three consecutive changes; the artifact was the problem. The test is
one row per persisted field, declaring the property that field's floor is
claimed to have and PROVING it by driving the real code with hostile values —
the gate for a field the gate refuses, normalizePersisted() for a field it
floors, the real JsonRpcProvider constructor for rpcUrl, and — for every field
whose only defence is that nothing dereferences it structurally — a boot of the
real popup entry point over that value onto EVERY view the popup can reopen
onto.

That last part is what makes the claim falsifiable, and it is why this defect
class is worth a harness at all: it lives on the RESTORE path and not on Home.
So the suite goes red on a field any restorable view dereferences on render, on
a field that gains a floor while its row still claims it has none, and on a
field added to PERSISTED_FIELDS with no row. What it does not reach is what no
stored record reaches by itself: a view only forward navigation opens, and
anything behind a click. The header and the README mirror now point at it
instead of restating it.

The boots are cheap enough to keep by construction rather than by sampling.
Every field the router itself reads is driven onto each view individually,
since a hostile value in one of those legitimately changes which view renders;
every other unfloored field is corrupted on the SAME boot, and that boot has to
land on the view it stored — so a field that does move the routing cannot hide
in the crowd, and the failure path re-boots one field at a time to name it.
That is thirty-three boots instead of six hundred; the suite runs in about 13s
against a 30s cap.

The DOM stub in tests/support/popupBoot.js gained one thing to make any of that
possible: an element's parentElement. Without it success-tx and transaction
threw on the first line that hides a field's wrapper, so neither renderer could
be booted onto at all — every boot aimed at them fell back to Home instead, and
the base profile the sweep starts from is now asserted to render each view
rather than fall back, so that cannot go unnoticed again.
This commit is contained in:
2026-08-23 18:22:01 +00:00
committed by sneak
parent 45500e66cf
commit 6a01688106
14 changed files with 1940 additions and 354 deletions

View File

@@ -1036,17 +1036,43 @@ saved data cannot be read and that nothing was signed or sent, rather than the
generic `-32603` every request used to answer.
Every other field of the record is floored in `normalizePersisted()` rather than
gated. That floor is a type check for the fields something dereferences
structurally — `trackedTokens`, each address's `tokenBalances`, `networkId`,
`networkEndpoints`, `activeAddress`, `viewStack` — and it checks the ENTRIES as
well as the container, because `[1, 2]` is a list and `t.address` is one level
below an `Array.isArray()`. The remaining fields get a `saved.x || default` or a
present-or-default passthrough that takes the stored value verbatim, with no
type check at all; which field is in which category is listed in the header of
`src/shared/stateSchema.js`. A truthy value of the wrong type in a field that IS
dereferenced walks through truthiness and throws on the first read, which is the
blank popup again by a longer route — so adding a field means choosing between
the two by what reads it.
gated, and the floor is not the same for every field. Some are type-checked as a
container AND entry by entry, because `[1, 2]` is a list, `{"0x…": "notalist"}`
is an object, and the dereference is one level below the container check; a
malformed entry is dropped, except in `networkEndpoints`, where the entry is
coerced so an unknown network's endpoints are not lost, and in `viewStack`,
where the stack is truncated at the first entry the popup will not reopen onto.
Some are type-checked as a scalar. The rest take the stored value verbatim,
because nothing dereferences them structurally.
Which field is which is not written in prose anywhere, deliberately.
`tests/persistedFieldContract.test.js` is the list: one row per persisted field,
naming the property that field's floor is claimed to have and proving it by
driving the real code with hostile values — and, for every field whose only
defence is that nothing dereferences it, by booting the real popup entry point
over that value onto every view the popup can reopen onto. That last part is
what makes the claim falsifiable, because this defect class lives on the restore
path rather than on the home screen: a field that gains a structural dereference
in any restorable view's render fails `make check`, as does a field that gains a
floor while its row still claims it has none, and as does a field added to
`PERSISTED_FIELDS` with no row at all. What the boot does not reach is what no
stored record reaches by itself — a view only forward navigation opens, and
anything behind a click. The per-field justification that used to live in the
header of `src/shared/stateSchema.js` shipped a false claim in three consecutive
changes, each caught only by a reviewer re-deriving thirty fields by hand.
The `allowedSites` case is why the entry check is not optional. A stored
`{"0x…": "notalist"}` is a well-formed object holding a malformed entry: it
passed the gate, rendered a completely healthy popup, and then threw inside
`saveState()`'s per-hostname merge, so every save from that moment on failed and
the user went on operating a wallet that was persisting nothing
([#362](https://git.eeqj.de/sneak/AutistMask/issues/362)). A save that fails is
now also reported rather than swallowed: `onSaveFailure()` in
`src/shared/state.js` is called for every failed save, awaited or not, and the
popup puts up a persistent "NOT SAVED" banner (`showSaveFailureBanner()` in
`src/popup/views/helpers.js`). Storage can still fail for reasons no floor
covers — a quota, a revoked permission, a record a newer build wrote — and the
wallet must never look healthy while that is true.
The `networkId` check is not cosmetic: that value is an object KEY into
`state.networkEndpoints`, so an unvalidated `"__proto__"` would set the map's
@@ -1101,6 +1127,14 @@ than only unhiding it, through the same dispatch and data guards as the restore
(`src/popup/viewRouter.js`), and falls back to Home when the state the target
would render is gone.
Those data guards check the ENTRIES of the stored `viewData`, not just the one
field each branch gates on, and the same goes for `selectedWallet` and
`selectedAddress`. `restoreView()` is not inside a `try`, so a `TypeError` in a
renderer skips the rest of popup init and leaves the user with no view, no
message and no control — the same blank popup by a longer route. Anything the
restore path dereferences is therefore either floored in `normalizePersisted()`
or refused by the guard, and the screen falls back to Home instead.
It renders only a screen this page load has not rendered yet. Forward navigation
renders as it goes, and `viewRouter.js` records every screen that reaches
`showView()`, so "Back" onto a screen already on the page unhides it and nothing