fix: floor malformed allowedSites, fraudContracts and selectedToken entries (closes #362)
A stored allowedSites whose value was not a list rendered a working popup and then made every subsequent save fail silently, so the user operated a wallet that persisted nothing -- worse than a blank popup, which is at least visibly broken. fraudContracts and selectedToken had the same shape: a container floored by truthiness or not at all, while its entries were dereferenced. Entries are now floored as well as containers, following the idiom #311 established, and a failed save raises a persistent banner instead of vanishing into a swallowed rejection. The per-field justifications that used to live in a hand-written header are replaced by a contract test that drives each field's hostile and falsy values through a real popup boot, so a claim about a field answers to the code rather than to prose. Its guarantee is stated narrowly and deliberately: no structural dereference on the code paths a wholly-corrupted profile takes, which is not every path a stored record takes. The paths it does not drive are named where the claim is made, and are tracked in #379.
This commit was merged in pull request #366.
This commit is contained in:
62
README.md
62
README.md
@@ -1036,17 +1036,49 @@ 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. Read the claim narrowly, as that file
|
||||
states it: what those boots prove is no structural dereference on the code paths
|
||||
a WHOLLY-CORRUPTED PROFILE takes, which is not every path a stored record takes.
|
||||
Not driven: any pairing of values the four slots do not produce, a view only
|
||||
forward navigation opens, anything behind a click, and everything a healthy
|
||||
profile reaches. Within that boundary the verdict is unconditional — if one of
|
||||
those boots leaves the popup unhealthy or off the view it stored, `make check`
|
||||
fails, including when it takes two corrupted fields at once, because the verdict
|
||||
is the combined boot and the per-field re-boot that names a culprit can only
|
||||
decorate the message. So does a field that gains a floor while its row still
|
||||
claims it has none, and so does a field added to `PERSISTED_FIELDS` with no row
|
||||
at all. 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 +1133,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
|
||||
|
||||
Reference in New Issue
Block a user