diff --git a/README.md b/README.md index 6e109c6..ffee2b9 100644 --- a/README.md +++ b/README.md @@ -1036,17 +1036,48 @@ 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 one of those boots corrupts and a +restorable view dereferences on its render fails `make check`, at either +polarity — a value nothing writes is a wrong-typed one and therefore truthy, so +each such field is also driven falsy, or proven unable to be falsy after the +floor. 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. Two +things the boots do not drive: a MIX of polarities, since one boot puts every +corrupted field on the same slot, so a branch reached only when one is truthy +and another falsy is not entered; and whatever 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 +1132,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 diff --git a/TODO.md b/TODO.md index bb91c7b..970896b 100644 --- a/TODO.md +++ b/TODO.md @@ -57,6 +57,37 @@ but the review is broader than any of them. reached a manifest but not the archive fails packaging. The artwork is original: a flat dark-navy rounded field with a teal triangular "A", drawn from geometry and rasterised into PNG, nothing traced or downloaded. +- 2026-08-23: A persisted container whose ENTRIES were dereferenced unchecked no + longer reaches a `.map()` or a `.toLowerCase()` + ([#362](https://git.eeqj.de/sneak/AutistMask/issues/362)). `allowedSites` was + the worst shape available: a stored `{"0x…": "notalist"}` 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 silently and the + user went on operating a wallet that was persisting nothing — measured as + `chrome.storage.local.set` never being called at all. `deniedSites` has the + same shape, `fraudContracts` the same class with a milder consequence, and the + sweep for the class turned up `selectedToken`, `rpcUrl` (handed whole to + `new JsonRpcProvider()`, which throws synchronously outside any `try`), the + entries of `viewData` (four restore branches gate on one truthy field and then + dereference an address), and `selectedWallet`/`selectedAddress` (a stored + `"map"` is TRUTHY against a real Array, so the restore guard does not + short-circuit). All of them are now floored in `src/shared/persistedState.js` + or refused by the per-branch guards in `src/popup/viewRouter.js`. Separately, + a save that fails is no longer swallowed: `onSaveFailure()` in + `src/shared/state.js` reports every failed save, awaited or not, and the popup + raises a persistent "NOT SAVED" banner. The hand-written per-field + justification in the header of `src/shared/stateSchema.js` — which had shipped + a false claim in three consecutive changes — is replaced by + `tests/persistedFieldContract.test.js`, one row per persisted field, each + proven by driving the real code with hostile values — and, for a 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, since that is + the path this whole class of defect lives on. Each such field is driven at + both polarities — a value nothing writes is wrong-typed and so truthy, so a + falsy slot is driven too, or the field is proven unable to be falsy after the + floor. A field with no row, a field that gains a floor while its row still + claims it has none, and a field one of those boots corrupts and a restorable + view dereferences on its render now all fail `make check`. - 2026-08-23: A swap amount and the token it is counted in now always come from the same hop, on both sides of the approval screen ([#359](https://git.eeqj.de/sneak/AutistMask/issues/359) and diff --git a/src/popup/index.js b/src/popup/index.js index 1a7e01e..a1c2112 100644 --- a/src/popup/index.js +++ b/src/popup/index.js @@ -1,9 +1,14 @@ // AutistMask popup entry point. // Loads state, initializes views, triggers first render. -const { state, saveState, loadState } = require("../shared/state"); +const { + state, + saveState, + onSaveFailure, + loadState, +} = require("../shared/state"); const { StateUnusableError } = require("../shared/stateSchema"); -const { setRuntimeDebug } = require("../shared/log"); +const { log, setRuntimeDebug } = require("../shared/log"); const { refreshPrices } = require("../shared/prices"); const { refreshBalances } = require("../shared/balances"); const { @@ -11,6 +16,7 @@ const { showView, updateDebugBanner, setBackRenderer, + showSaveFailureBanner, pushCurrentView, goBack, } = require("./views/helpers"); @@ -61,6 +67,14 @@ async function doRefreshAndRender() { state.lastBalanceRefresh = Date.now(); await saveState(); renderWalletList(); + } catch (e) { + // Every call site fires this and walks away — the boot below, the ten + // second interval, and eight views through ctx — so it must never + // reject: an unhandled rejection is not a report of anything. The save + // inside it reports its own failure through onSaveFailure() (see + // src/shared/state.js); what is left here is a failed network round + // trip, which the next tick retries. + log.errorf("popup: background refresh failed:", e); } finally { refreshInFlight = false; } @@ -136,6 +150,12 @@ function fallbackView() { } async function init() { + // First, before anything can save: showView() saves on every navigation + // without awaiting, so a save that fails from here on has somewhere to be + // reported rather than being swallowed by the save queue + // (https://git.eeqj.de/sneak/AutistMask/issues/362). Registered ahead of + // the approval-window branch below too, since that window saves as well. + onSaveFailure(showSaveFailureBanner); try { await loadState(); } catch (e) { diff --git a/src/popup/viewRouter.js b/src/popup/viewRouter.js index bcfbf35..ed7def4 100644 --- a/src/popup/viewRouter.js +++ b/src/popup/viewRouter.js @@ -53,12 +53,17 @@ function resetRenderedViews() { const ALWAYS_RENDER_ON_BACK = new Set(["main"]); // Views that render an address the user picked and cannot be rendered -// without one. +// without one. "confirm-tx" is here because its Sign button dereferences +// `state.wallets[state.selectedWallet].encryptedSecret` +// (src/popup/views/confirmTx.js) behind no guard of its own — a screen that +// can only throw when the user presses its one button must not be restored +// onto. const ADDRESS_VIEWS = new Set([ "address", "address-token", "receive", "transaction", + "confirm-tx", ]); function needsAddress(view) { @@ -74,6 +79,73 @@ function hasValidAddress(state) { ); } +// The stored viewData ENTRIES each branch below dereferences, as opposed to +// the one field it gates on. +// +// A gate on a single truthy field checks the container, not the entries, and +// the dereference is one level below it: a stored `{"currentView": +// "success-tx","viewData":{"hash":"0x1"}}` passes `data.hash` and then throws +// on `address.toLowerCase()` inside addressTitle() (src/popup/views/ +// helpers.js), out of restoreView(), which src/popup/index.js does not guard — +// so the rest of popup init never runs. txStatus.restoreWait() has checked its +// own branch's fields since it was written; these are the other four. +// +// Only what actually throws is required. Fields that are compared, +// concatenated or escaped coerce (escapeHtml() and displaySymbol() both +// String() their argument), so requiring them would refuse a restorable screen +// over a cosmetic value. +function isText(value) { + return typeof value === "string"; +} + +function isRecord(value) { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +// An address handed to renderAddressHtml()/addressTitle(): both reach +// `address.slice()` and `address.toLowerCase()` with no guard. +function isAddressText(value) { + return isText(value); +} + +// Decoded calldata, as decodedDetailsHtml() (src/popup/views/txStatus.js) +// walks it: `for (const d of decoded.details)` needs an iterable, and each +// entry's `address` reaches toAddressHtml(). Absent or falsy is the ordinary +// case and short-circuits before either. +function isRenderableDecoded(value) { + if (!value) return true; + if (!isRecord(value)) return false; + if (!value.details) return true; + if (!Array.isArray(value.details)) return false; + return value.details.every( + (entry) => + isRecord(entry) && (!entry.address || isAddressText(entry.address)), + ); +} + +// The pending transaction confirmTx.show() renders: `token` reaches +// renderAddressHtml() when it is not "ETH", and `from`/`to` reach +// addressTitle(), makeBlockie() and getLocalWarnings(). +function isRenderablePendingTx(value) { + return ( + isRecord(value) && + isText(value.token) && + isAddressText(value.from) && + isAddressText(value.to) + ); +} + +// The stored transaction transactionDetail.render() shows. contractAddress is +// optional on an ETH transfer, and reaches addressDotHtml() when it is there. +function isRenderableTx(value) { + return ( + isRecord(value) && + isAddressText(value.from) && + isAddressText(value.to) && + (!value.contractAddress || isAddressText(value.contractAddress)) + ); +} + // Render `view` from persisted state. Each view module shows itself, so a // true return means the view is both rendered and on screen. // @@ -107,11 +179,11 @@ function renderView(view, state, views) { views.settingsAddToken.show(); return true; case "confirm-tx": - if (!data.pendingTx) return false; + if (!isRenderablePendingTx(data.pendingTx)) return false; views.confirmTx.restore(); return true; case "transaction": - if (!data.tx) return false; + if (!isRenderableTx(data.tx)) return false; views.transactionDetail.render(); return true; case "wait-tx": @@ -120,10 +192,13 @@ function renderView(view, state, views) { return Boolean(views.txStatus.restoreWait()); case "success-tx": if (!data.hash) return false; + if (!isAddressText(data.to)) return false; + if (!isRenderableDecoded(data.decoded)) return false; views.txStatus.renderSuccess(); return true; case "error-tx": if (!data.message) return false; + if (!isAddressText(data.to)) return false; views.txStatus.renderError(); return true; default: diff --git a/src/popup/views/helpers.js b/src/popup/views/helpers.js index 6315e1e..4f17deb 100644 --- a/src/popup/views/helpers.js +++ b/src/popup/views/helpers.js @@ -132,6 +132,38 @@ function updateDebugBanner(viewName) { } } +// The banner shown when a save has failed, registered as the save-failure +// reporter by src/popup/index.js. +// +// Persistent and not dismissable, unlike showFlash(): what it says is true +// until the popup is closed, and a message that clears itself after two seconds +// is how the user goes on operating a wallet that is persisting nothing +// (https://git.eeqj.de/sneak/AutistMask/issues/362). It survives navigation +// because it hangs off document.body rather than off a view. +// +// Created on demand rather than authored in index.html, the same way +// updateDebugBanner() creates its own: it is absent from a popup where nothing +// has failed, which is the state that must not need markup to be in. +// +// textContent, never innerHTML: `detail` carries an error message, which may +// come from the browser's storage layer. +function showSaveFailureBanner(detail) { + let banner = document.getElementById("save-failure-banner"); + if (!banner) { + banner = document.createElement("div"); + banner.id = "save-failure-banner"; + banner.style.cssText = + "background:#c00;color:#fff;text-align:center;font-size:10px;padding:2px 4px;font-family:monospace;position:sticky;top:0;z-index:10000;"; + document.body.prepend(banner); + } + const message = (detail && (detail.message || detail.problem)) || detail; + banner.textContent = + "NOT SAVED — AutistMask could not write to storage, so recent" + + " changes are not stored. Close and reopen the popup; if this keeps" + + " happening, do not rely on anything you change now." + + (message ? " (" + String(message) + ")" : ""); +} + // Callback that renders a view being navigated BACK onto. Set once by // index.js via setBackRenderer(), which routes the view through the same // per-view render and data guards restoreView() uses. @@ -544,6 +576,7 @@ module.exports = { showView, onViewLeave, updateDebugBanner, + showSaveFailureBanner, setBackRenderer, pushCurrentView, goBack, diff --git a/src/shared/persistedState.js b/src/shared/persistedState.js index 17ff82b..0b485b8 100644 --- a/src/shared/persistedState.js +++ b/src/shared/persistedState.js @@ -100,6 +100,107 @@ function tokenRefs(value) { ); } +// A list of strings, for the fields whose entries are dereferenced as text: +// fraudContracts (`a.toLowerCase()` in src/popup/views/send.js and +// src/shared/transactions.js) and each address's hostname list in the site maps +// below (`h !== host` filters, `list.includes(hostname)` in the background). +// +// Same rule as tokenRefs(), for the same reason: the container AND the entries, +// with a malformed entry DROPPED rather than repaired. A number in a hostname +// list names no site and a number in fraudContracts names no contract, so there +// is nothing to repair either to, and the empty list is a legitimate value that +// survives. The result is a fresh array of primitives, so it shares no +// structure with `saved`. +function textList(value) { + if (!Array.isArray(value)) return []; + return value.filter((entry) => typeof entry === "string"); +} + +// allowedSites / deniedSites: { [address]: [hostname, ...] }. +// +// The container check these had (truthy and not an array) is not the floor: +// `{"0xabc…": "notalist"}` IS a non-array object, and the dereference is one +// level below it. saveState() merges these maps per key and then per hostname +// WITHIN each key, so a stored value that is not a list reaches `base.map()` in +// mergeListByIdentity() (src/shared/state.js) and throws — after the popup has +// rendered, which is why every save from then on failed while the UI looked +// healthy (https://git.eeqj.de/sneak/AutistMask/issues/362). The Settings +// revoke button (`list.filter()`), and the background's +// `allowed.includes(hostname)` gate, dereference it the same way; on that last +// one a stored string would also answer a SUBSTRING match, so a corrupt map +// could widen a site permission rather than merely throw. +// +// An address key whose value is not a list of hostnames is dropped entirely: it +// grants and denies nothing, and dropping it fails closed. A stored own +// "__proto__" key — which JSON can carry — is dropped for the same reason: it +// can never be a wallet address, so it grants nothing either, and keeping it +// only keeps a value that saveState()'s merge would hand to the prototype +// setter on the next write. Keys are written with defineProperty so that no key +// reaching this function can consult a setter at all, whatever the rule above +// it becomes; mergeMapByKey() in src/shared/state.js writes the same way. +function siteMap(value) { + const out = {}; + if (!isRecord(value)) return out; + for (const address of Object.keys(value)) { + if (address === "__proto__") continue; + const hostnames = textList(value[address]); + if (hostnames.length === 0) continue; + defineOwn(out, address, hostnames); + } + return out; +} + +// An endpoint URL: non-empty text, or the fallback. +function url(value, fallback) { + return typeof value === "string" && value !== "" ? value : fallback; +} + +// One remembered endpoint pair out of networkEndpoints, floored on the two +// fields applyChainSwitchFields() (src/shared/chainSwitchFields.js) assigns +// STRAIGHT ONTO s.rpcUrl / s.blockscoutUrl on the next chain switch: flooring +// the live fields alone would leave a non-string sitting one switch away from +// them. A field that is not text is deleted rather than replaced, so the +// switch falls through its own `|| net.defaultRpcUrl`. Anything else the pair +// carries is kept: a profile that has been on a build storing more per-network +// fields must not lose them by passing through this one. +function endpointPair(value) { + const pair = { ...(isRecord(value) ? value : {}) }; + for (const field of ["rpcUrl", "blockscoutUrl"]) { + if (typeof pair[field] !== "string" || pair[field] === "") { + delete pair[field]; + } + } + return pair; +} + +// A list index into wallets / a wallet's addresses: a non-negative integer, or +// null for "nothing selected". +// +// hasValidAddress() (src/popup/viewRouter.js) guards the restore path with +// `state.wallets[state.selectedWallet] && …addresses[state.selectedAddress]`, +// which is safe for a stale INTEGER — out of range is undefined, and the `&&` +// short-circuits — and NOT safe for a string naming an Array.prototype member. +// `wallets["map"]` is truthy, so the guard does not short-circuit and +// `.addresses[…]` throws out of restoreView(): the dead popup. "length", +// "constructor" and "__proto__" answer the same way, and +// src/popup/views/confirmTx.js dereferences selectedWallet behind no guard at +// all. +function listIndex(value) { + return Number.isInteger(value) && value >= 0 ? value : null; +} + +// Write `key` as an own data property, never through a setter. Plain +// assignment of "__proto__" replaces the object's prototype and records no +// entry; every map built from stored keys goes through this. +function defineOwn(obj, key, value) { + Object.defineProperty(obj, key, { + value: value, + writable: true, + enumerable: true, + configurable: true, + }); +} + // Keep only the leading run of stored views the popup is willing to render. // // restoreView() refuses to reopen ONTO a non-restorable view, but the stack @@ -195,8 +296,15 @@ function normalizePersisted(saved) { out.networkId = isKnownNetworkId(saved.networkId) ? saved.networkId : DEFAULT_STATE.networkId; - out.rpcUrl = saved.rpcUrl || DEFAULT_STATE.rpcUrl; - out.blockscoutUrl = saved.blockscoutUrl || DEFAULT_STATE.blockscoutUrl; + // Non-empty text or the default, never anything else. getProvider() + // (src/shared/balances.js) hands rpcUrl straight to `new + // JsonRpcProvider()`, which throws SYNCHRONOUSLY for a value that is not a + // string — out of src/popup/views/txStatus.js and src/popup/views/ + // addWallet.js, neither of which is inside a try, and the first of which a + // stored `currentView: "wait-tx"` reaches through restoreView(). It is a + // scalar, so the type check is the whole fix. + out.rpcUrl = url(saved.rpcUrl, DEFAULT_STATE.rpcUrl); + out.blockscoutUrl = url(saved.blockscoutUrl, DEFAULT_STATE.blockscoutUrl); // An actual object is required, not merely a truthy non-array: the code // below and applyChainSwitchFields() index and ASSIGN INTO this value, and // assigning a property to a string or a number is a silent no-op in @@ -210,19 +318,16 @@ function normalizePersisted(saved) { : {}; out.networkEndpoints = {}; for (const netId of Object.keys(rawEndpoints)) { - // defineProperty, not assignment: a stored map with an own - // "__proto__" key — which JSON can carry and assignment treats as the - // prototype setter — would otherwise replace this object's prototype - // and record no entry at all. Keys other than the known network ids - // are kept rather than dropped, so a profile that has been on a build - // with more networks does not lose their endpoints by passing through - // this one. - Object.defineProperty(out.networkEndpoints, netId, { - value: { ...rawEndpoints[netId] }, - writable: true, - enumerable: true, - configurable: true, - }); + // Keys other than the known network ids are kept rather than dropped, + // so a profile that has been on a build with more networks does not + // lose their endpoints by passing through this one. That is why an own + // "__proto__" key survives here where siteMap() drops it, and why the + // write has to go through defineOwn(). + defineOwn( + out.networkEndpoints, + netId, + endpointPair(rawEndpoints[netId]), + ); } // A profile written before this map existed carries exactly one pair of // endpoints, belonging to whatever network it was last on. Adopt it as @@ -247,14 +352,8 @@ function normalizePersisted(saved) { typeof saved.activeAddress === "string" && saved.activeAddress !== "" ? saved.activeAddress : null; - out.allowedSites = - saved.allowedSites && !Array.isArray(saved.allowedSites) - ? structuredClone(saved.allowedSites) - : {}; - out.deniedSites = - saved.deniedSites && !Array.isArray(saved.deniedSites) - ? structuredClone(saved.deniedSites) - : {}; + out.allowedSites = siteMap(saved.allowedSites); + out.deniedSites = siteMap(saved.deniedSites); out.rememberSiteChoice = saved.rememberSiteChoice !== undefined ? saved.rememberSiteChoice @@ -287,16 +386,32 @@ function normalizePersisted(saved) { : 100000; out.utcTimestamps = saved.utcTimestamps !== undefined ? saved.utcTimestamps : false; - out.fraudContracts = structuredClone(saved.fraudContracts || []); + // A list of contract addresses, floored the same way: send.js builds its + // fraud set as `(state.fraudContracts || []).map((a) => a.toLowerCase())` + // and filterTransactions() maps the same list through normalizeAddress(), + // so a stored string walks through the `|| []` and a stored number walks + // through an Array.isArray(). + out.fraudContracts = textList(saved.fraudContracts); out.tokenHolderCache = structuredClone(saved.tokenHolderCache || {}); out.theme = saved.theme || "system"; out.debugMode = saved.debugMode !== undefined ? saved.debugMode : false; out.currentView = saved.currentView || null; - out.selectedWallet = - saved.selectedWallet !== undefined ? saved.selectedWallet : null; - out.selectedAddress = - saved.selectedAddress !== undefined ? saved.selectedAddress : null; - out.selectedToken = saved.selectedToken || null; + out.selectedWallet = listIndex(saved.selectedWallet); + out.selectedAddress = listIndex(saved.selectedAddress); + // "ETH", or a contract address, or null — never anything else. The popup + // restores onto "address-token" behind a truthiness check on this field and + // then dereferences it as text (`tokenId.toLowerCase()` in + // src/popup/views/addressToken.js, `state.selectedToken.toLowerCase()` in + // src/popup/views/receive.js), so a stored number is truthy, passes the + // restore gate, and throws on the screen it restores onto. Found by the + // sweep for this same defect class in + // https://git.eeqj.de/sneak/AutistMask/issues/362; floored to null, which + // is what the restore gate already treats as "nothing selected". The empty + // string was already falsy here and stays null. + out.selectedToken = + typeof saved.selectedToken === "string" && saved.selectedToken !== "" + ? saved.selectedToken + : null; out.viewData = structuredClone(saved.viewData || {}); out.viewStack = restorableStack(saved.viewStack, out.currentView); return out; diff --git a/src/shared/state.js b/src/shared/state.js index 08442c6..df3d891 100644 --- a/src/shared/state.js +++ b/src/shared/state.js @@ -310,12 +310,26 @@ function mergeAddress(base, ours, theirs) { // a key another page edited. Unlike an array's identity function, an object // key can't collide with a different logical entry (Object.keys() is // already deduplicated), so this needs no collision floor of its own. +// +// Every write goes through defineProperty rather than assignment. The keys are +// whatever the stored record carries, and plain assignment of "__proto__" — +// which JSON can carry and normalizePersisted() keeps for networkEndpoints — +// replaces this object's prototype and records no entry. That would undo one +// layer downstream exactly what defineOwn() does in +// src/shared/persistedState.js. function mergeMapByKey(base, ours, theirs, mergeLeaf) { base = base || {}; ours = ours || {}; theirs = theirs || {}; const result = {}; const seen = new Set(); + const put = (key, value) => + Object.defineProperty(result, key, { + value: value, + writable: true, + enumerable: true, + configurable: true, + }); for (const key of Object.keys(theirs)) { seen.add(key); @@ -323,16 +337,16 @@ function mergeMapByKey(base, ours, theirs, mergeLeaf) { const inOurs = Object.prototype.hasOwnProperty.call(ours, key); if (inBase && !inOurs) continue; // this page deleted the whole entry if (inOurs) { - result[key] = mergeLeaf(base[key], ours[key], theirs[key]); + put(key, mergeLeaf(base[key], ours[key], theirs[key])); } else { - result[key] = theirs[key]; + put(key, theirs[key]); } } for (const key of Object.keys(ours)) { if (seen.has(key)) continue; if (!Object.prototype.hasOwnProperty.call(base, key)) { - result[key] = ours[key]; + put(key, ours[key]); } } @@ -522,11 +536,51 @@ async function saveStateOnce() { // begins, so each one only ever sees the true live state at its turn. let saveQueue = Promise.resolve(); +// Where a failed save is REPORTED, set once by the context that has a screen +// to say it on (src/popup/index.js). +// +// A save that fails must not fail silently. showView() fires saveState() on +// every navigation without awaiting it, and the queue below has to attach a +// rejection handler to keep advancing — so a failing save was swallowed +// entirely: no throw, no message, nothing on screen. The wallet kept running +// against storage that was rejecting every write, which is the data-loss half +// of https://git.eeqj.de/sneak/AutistMask/issues/362. The awaited callers were +// no better off: `await saveState()` inside an unguarded event handler surfaces +// in the console and nowhere the user looks. +// +// This is the "tell the user" half; the other half is the floor in +// normalizePersisted(), which stops the malformed-record cause from arising in +// the first place. Both, because a floor only covers the causes it knows about +// and storage can still fail for reasons of its own (quota, a revoked +// permission, a record a newer build wrote). +let saveFailureHandler = null; + +function onSaveFailure(fn) { + saveFailureHandler = fn; +} + +function reportSaveFailure(err) { + log.errorf("state: saving failed, changes were NOT persisted:", err); + if (!saveFailureHandler) return; + try { + saveFailureHandler(err); + } catch (e) { + // The reporter is the last thing standing between a failed save and + // silence; a reporter that throws must not become an unhandled + // rejection of its own on top of it. + log.errorf("state: the save-failure reporter itself failed:", e); + } +} + function saveState() { const turn = saveQueue.then(saveStateOnce); // The queue must advance even when a save rejects, or every save after // it queues behind a promise that never settles. saveQueue = turn.catch(() => {}); + // Every failed save is reported, whether or not the caller awaited this + // one. The returned promise still rejects, so a caller that DOES await + // keeps its own error handling. + turn.catch(reportSaveFailure); return turn; } @@ -574,6 +628,7 @@ function currentAddress() { module.exports = { state, saveState, + onSaveFailure, loadState, currentAddress, currentNetwork, diff --git a/src/shared/stateSchema.js b/src/shared/stateSchema.js index fd7b884..0d2dc3b 100644 --- a/src/shared/stateSchema.js +++ b/src/shared/stateSchema.js @@ -26,35 +26,41 @@ // // What is checked HERE is what nothing downstream can floor: the wallet list, // the version, and the network id that keys an object. Every other field is -// normalizePersisted()'s to make safe, and what that function does today is -// NOT uniform. The four kinds of floor it applies, listed so a reader can tell -// which one a given field has without reading it off: +// normalizePersisted()'s to make safe, and what that function does is NOT +// uniform across the record. // -// Type-checked, container AND entries: trackedTokens, each address's -// tokenBalances, networkId, networkEndpoints, activeAddress, viewStack. -// These are the fields something dereferences structurally — iterated, -// indexed, assigned into, or .toLowerCase()'d — where a truthy value of -// the wrong type throws on the first read. The entries matter as much as -// the container: [1, 2] IS a list, and `t.address` is one level below the -// Array.isArray(). What is checked on an ENTRY is the field the check -// exists for and no more — for trackedTokens and tokenBalances that is -// `address` alone; the rest of an entry is taken verbatim. So an entry's -// `decimals` and `balance` may be null, which is how balances.js records -// that nothing knows the token's scale -// (https://git.eeqj.de/sneak/AutistMask/issues/349), and every reader -// handles that null rather than being defended from it here. -// Container shape only: allowedSites, deniedSites. A falsy value or a list -// becomes {}; anything else is taken as stored and the entries are not -// checked. -// `saved.x || default`, no type check: rpcUrl, blockscoutUrl, -// lastBalanceRefresh, fraudContracts, tokenHolderCache, theme, -// currentView, selectedToken, viewData. -// Present-or-default, value taken verbatim: every boolean flag, -// dustThresholdGwei, selectedWallet, selectedAddress. +// WHICH FLOOR A GIVEN FIELD HAS IS NOT WRITTEN HERE. It is +// tests/persistedFieldContract.test.js: 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 — the gate for a field the gate +// refuses, normalizePersisted() for a field it floors, and, for a field whose +// only defence is that nothing dereferences it structurally, a boot of the +// real popup entry point onto EVERY view the popup can reopen onto. // -// A field added to the record needs a check here or a floor there, chosen by -// what reads it: anything dereferenced structurally needs the type check, and -// neither of the last two kinds is one. +// That last part is the whole point, because this defect class lives on the +// RESTORE path and not on Home: a field one of those boots corrupts and a +// restorable view dereferences on its render turns that suite red, at either +// polarity — a value nothing writes is wrong-typed and so truthy, so each such +// field is also driven falsy, or proven unable to be falsy after the floor. So +// does a field that gains a floor while its row still claims it has none. Two +// things the boots do not drive: a MIX of polarities, since one boot puts every +// corrupted field on the same slot; and whatever no stored record reaches by +// itself — a view only forward navigation opens, and anything behind a click. A +// field added to PERSISTED_FIELDS with no row fails the suite too. +// +// That test exists because this comment did not work. It carried a +// hand-written justification per field, and it shipped a false one in three +// consecutive changes — a different field each time, each caught only by a +// reviewer re-deriving thirty fields by hand. A claim nobody can execute is +// worse than no claim, because it is believed. +// +// The trap is worth stating here, since it is what all three got wrong: a +// check on a CONTAINER is not a check on its ENTRIES, and the dereference is +// one level below the container. `[1, 2]` is a list, `{"0x…": "notalist"}` is +// a record, and `{"currentView":"success-tx","viewData":{"hash":"0x1"}}` +// passes the restore gate and throws on the address the renderer below it +// reads. A field added to the record needs a decision about its entries as +// well as its shape — and then a row in that test. const { isKnownNetworkId } = require("./networks"); diff --git a/tests/backNavigation.test.js b/tests/backNavigation.test.js index edcaae7..1b4ff48 100644 --- a/tests/backNavigation.test.js +++ b/tests/backNavigation.test.js @@ -153,7 +153,7 @@ describe("Back onto a view the reopened popup never rendered", () => { test("Back onto the transaction detail renders it", () => { reopenedOn("settings", ["main", "transaction"], { - viewData: { tx: { hash: "0xdead" } }, + viewData: { tx: { hash: "0xdead", from: ADDRESS, to: ADDRESS } }, }); goBack(); expect(calls).toEqual(["transactionDetail"]); @@ -162,7 +162,14 @@ describe("Back onto a view the reopened popup never rendered", () => { test("Back onto the transaction confirmation restores it", () => { reopenedOn("settings", ["main", "confirm-tx"], { - viewData: { pendingTx: { to: ADDRESS, amount: "1" } }, + viewData: { + pendingTx: { + token: "ETH", + from: ADDRESS, + to: ADDRESS, + amount: "1", + }, + }, }); goBack(); expect(calls).toEqual(["confirmTx"]); @@ -171,7 +178,7 @@ describe("Back onto a view the reopened popup never rendered", () => { test("Back onto the success screen renders it", () => { reopenedOn("settings", ["main", "success-tx"], { - viewData: { hash: "0xdead" }, + viewData: { hash: "0xdead", to: ADDRESS }, }); goBack(); expect(calls).toEqual(["successTx"]); @@ -180,7 +187,7 @@ describe("Back onto a view the reopened popup never rendered", () => { test("Back onto the failure screen renders it", () => { reopenedOn("settings", ["main", "error-tx"], { - viewData: { message: "execution reverted" }, + viewData: { message: "execution reverted", to: ADDRESS }, }); goBack(); expect(calls).toEqual(["errorTx"]); diff --git a/tests/persistedEntryFloors.test.js b/tests/persistedEntryFloors.test.js new file mode 100644 index 0000000..6468b70 --- /dev/null +++ b/tests/persistedEntryFloors.test.js @@ -0,0 +1,404 @@ +// A persisted container that is checked while its ENTRIES are dereferenced +// unchecked (https://git.eeqj.de/sneak/AutistMask/issues/362). +// +// https://git.eeqj.de/sneak/AutistMask/issues/311 settled the idiom — floor the +// container AND its entries, dropping anything that cannot be safely +// dereferenced — and applied it to trackedTokens and tokenBalances. These are +// the fields it did not reach. +// +// allowedSites is the worst shape in the codebase, and it is what the boot +// tests below measure: a stored `{"0x…": "notalist"}` passes the gate, renders +// a WORKING popup, and then throws `base.map is not a function` inside +// saveState()'s merge — so every save from then on fails while the UI looks +// entirely healthy and the user goes on operating a wallet that is persisting +// nothing. A blank popup is at least visibly broken; this is not. So the +// assertion here is never merely "the popup rendered": it is "the popup +// rendered AND the write actually landed in storage". +// +// Observed at ad6aa7b, with the floors below removed: +// allowedSites: {"0x…": "notalist"} -> views=["main"], errors=[], and +// storage.set NEVER called: the stored record kept no schemaVersion, so +// nothing the user did was persisted. +// fraudContracts: "0x…" -> renderSendTokenSelect() threw +// "(state.fraudContracts || []).map is not a function" +// fraudContracts: [42] -> threw "a.toLowerCase is not a +// function" +// selectedToken: 42 (restoring onto address-token) -> views=[], errors= +// ["tokenId.toLowerCase is not a function"] — a blank popup. + +const { normalizePersisted } = require("../src/shared/persistedState"); +const { makeStorageStub } = require("./support/storageStub"); +const { + bootPopup, + cleanupPopup, + unversionedValidProfile, + ADDRESS, + TOKEN_ADDRESS, +} = require("./support/popupBoot"); + +// One extension page: a fresh module registry over the given storage. state.js +// resolves the storage API at require time, so the stub has to be installed +// before the module is loaded. +function loadStateModule(storage) { + jest.resetModules(); + globalThis.chrome = { storage: { local: storage.local } }; + return require("../src/shared/state"); +} + +afterEach(() => { + cleanupPopup(); +}); + +// ------------------------------------------------------- the floor itself + +describe("the floor under allowedSites and deniedSites", () => { + for (const field of ["allowedSites", "deniedSites"]) { + test(`${field} that is not a record becomes an empty record`, () => { + for (const bad of ["nope", 42, true, [ADDRESS], null]) { + expect(normalizePersisted({ [field]: bad })[field]).toEqual({}); + } + }); + + test(`an ${field} entry whose value is not a hostname list is dropped`, () => { + for (const bad of ["dapp.example", 42, null, { a: 1 }, true]) { + expect( + normalizePersisted({ [field]: { [ADDRESS]: bad } })[field], + ).toEqual({}); + } + }); + + test(`a hostname that is not text is dropped from an ${field} entry`, () => { + expect( + normalizePersisted({ + [field]: { [ADDRESS]: [42, null, "dapp.example", {}] }, + })[field], + ).toEqual({ [ADDRESS]: ["dapp.example"] }); + }); + + test(`a real ${field} map survives, copied not shared`, () => { + const saved = { [field]: { [ADDRESS]: ["dapp.example"] } }; + + const out = normalizePersisted(saved); + + expect(out[field]).toEqual(saved[field]); + expect(out[field]).not.toBe(saved[field]); + expect(out[field][ADDRESS]).not.toBe(saved[field][ADDRESS]); + }); + + test(`a good ${field} entry beside a malformed one survives`, () => { + const out = normalizePersisted({ + [field]: { [ADDRESS]: ["dapp.example"], [TOKEN_ADDRESS]: 42 }, + }); + + expect(out[field]).toEqual({ [ADDRESS]: ["dapp.example"] }); + }); + + test(`a stored own "__proto__" key in ${field} is dropped`, () => { + // JSON can carry the key. It can never be a wallet address, so it + // grants and denies nothing and goes the way of every other key + // whose value is unusable; keeping it would only keep a value that + // saveState()'s merge hands to the prototype setter on the next + // write. + const saved = JSON.parse( + '{"' + field + '":{"__proto__":["evil.invalid"]}}', + ); + + const out = normalizePersisted(saved); + + expect(Object.getPrototypeOf(out[field])).toBe(Object.prototype); + expect(Object.keys(out[field])).toEqual([]); + }); + } +}); + +describe('a stored own "__proto__" key surviving a save', () => { + // The floor writes map keys with defineProperty; saveState()'s merge is one + // layer downstream of it and used to write them with plain assignment, + // which hands "__proto__" to the prototype setter and records no entry. + // networkEndpoints is where a key that is not a known id is deliberately + // KEPT, so it is where that undoing shows. + test("does not move a prototype or vanish from networkEndpoints", async () => { + const profile = unversionedValidProfile(); + profile.networkEndpoints = JSON.parse( + '{"__proto__":{"rpcUrl":"https://kept.invalid"}}', + ); + const storage = makeStorageStub({ autistmask: profile }); + const { state, loadState, saveState } = loadStateModule(storage); + + await loadState(); + state.theme = "dark"; + await saveState(); + + // Not compared against Object.prototype by identity: the storage stub + // clones through structuredClone, which builds the result in the host + // realm, so the two Object.prototypes are different objects. + const stored = storage.read("autistmask").networkEndpoints; + expect(Object.getPrototypeOf(stored).rpcUrl).toBeUndefined(); + expect(Object.keys(stored)).toContain("__proto__"); + }); +}); + +describe("the floor under fraudContracts", () => { + test("fraudContracts that is not a list becomes an empty list", () => { + for (const bad of ["nope", 42, true, { a: 1 }]) { + expect( + normalizePersisted({ fraudContracts: bad }).fraudContracts, + ).toEqual([]); + } + }); + + test("a fraudContracts entry that is not text is dropped", () => { + expect( + normalizePersisted({ + fraudContracts: [42, null, TOKEN_ADDRESS, {}, []], + }).fraudContracts, + ).toEqual([TOKEN_ADDRESS]); + }); + + test("a real fraudContracts list survives, copied not shared", () => { + const saved = { fraudContracts: [TOKEN_ADDRESS] }; + + const out = normalizePersisted(saved); + + expect(out.fraudContracts).toEqual(saved.fraudContracts); + expect(out.fraudContracts).not.toBe(saved.fraudContracts); + }); +}); + +describe("the floor under selectedToken", () => { + // Found by the sweep for this defect class, not named in the issue: the + // restore gate in src/popup/viewRouter.js checks truthiness only, and both + // src/popup/views/addressToken.js and src/popup/views/receive.js then + // dereference it as text. + test("a selectedToken that is not text becomes null", () => { + for (const bad of [42, true, { a: 1 }, [TOKEN_ADDRESS]]) { + expect( + normalizePersisted({ selectedToken: bad }).selectedToken, + ).toBeNull(); + } + }); + + test("a real selectedToken survives; the empty string becomes null", () => { + expect( + normalizePersisted({ selectedToken: TOKEN_ADDRESS }).selectedToken, + ).toBe(TOKEN_ADDRESS); + expect(normalizePersisted({ selectedToken: "ETH" }).selectedToken).toBe( + "ETH", + ); + expect( + normalizePersisted({ selectedToken: "" }).selectedToken, + ).toBeNull(); + }); +}); + +// ----------------------------------------- what the user actually gets + +describe("a malformed allowedSites entry", () => { + const MALFORMED = [ + { name: "a string", value: "notalist" }, + { name: "a number", value: 42 }, + { name: "a record", value: { hostnames: ["dapp.example"] } }, + ]; + + for (const { name, value } of MALFORMED) { + test(`whose value is ${name}: a working popup whose writes persist`, async () => { + const env = await bootPopup( + unversionedValidProfile({ + allowedSites: { [ADDRESS]: value }, + }), + ); + + expect({ + visibleViews: env.visibleViews(), + errors: env.pageErrors, + }).toEqual({ visibleViews: ["main"], errors: [] }); + + // The half that matters. A popup that renders and never persists + // again is worse than one that renders nothing, because nothing + // tells the user. The version stamp is proof a write landed: it + // is absent from the stored record until saveState() writes one. + expect(env.storage.set).toHaveBeenCalled(); + const stored = env.storage.read("autistmask"); + expect(stored.schemaVersion).toBe(1); + expect(stored.wallets[0].encryptedSecret).toBe( + "encrypted-secret-1", + ); + expect(stored.allowedSites).toEqual({}); + }); + } + + test("the well-formed entries beside it keep working", async () => { + const env = await bootPopup( + unversionedValidProfile({ + allowedSites: { + [ADDRESS]: ["dapp.example"], + [TOKEN_ADDRESS]: "notalist", + }, + }), + ); + + expect(env.pageErrors).toEqual([]); + expect(env.storage.read("autistmask").allowedSites).toEqual({ + [ADDRESS]: ["dapp.example"], + }); + }); + + test("a later save still lands, not just the first", async () => { + // The failure this closes was in the MERGE, which runs on every save + // against whatever is in storage at the time. One write landing is not + // enough: the field has to stay mergeable. + const storage = makeStorageStub({ + autistmask: unversionedValidProfile({ + allowedSites: { [ADDRESS]: "notalist" }, + }), + }); + const { state, loadState, saveState } = loadStateModule(storage); + + await loadState(); + state.theme = "dark"; + await saveState(); + state.utcTimestamps = true; + await saveState(); + + const stored = storage.read("autistmask"); + expect(stored.theme).toBe("dark"); + expect(stored.utcTimestamps).toBe(true); + expect(stored.allowedSites).toEqual({}); + expect(stored.wallets[0].encryptedSecret).toBe("encrypted-secret-1"); + }); +}); + +describe("a malformed fraudContracts", () => { + // The send screen, which is where this one lands: the boot path only + // reaches fraudContracts through loadHomeTxs(), which catches, so the + // consequence is an unusable send screen rather than silent data loss. + function stubSendDocument() { + const select = { innerHTML: "", children: [] }; + select.appendChild = (child) => select.children.push(child); + globalThis.document = { + getElementById: (id) => (id === "send-token" ? select : null), + createElement: () => ({ value: "", textContent: "" }), + }; + return select; + } + + const HELD = { + address: TOKEN_ADDRESS, + symbol: "AAA", + decimals: 18, + balance: "12.5", + holders: 50000, + }; + + async function sendScreenTokens(fraudContracts) { + const storage = makeStorageStub({ + autistmask: unversionedValidProfile({ fraudContracts }), + }); + const { loadState } = loadStateModule(storage); + await loadState(); + const select = stubSendDocument(); + const { renderSendTokenSelect } = require("../src/popup/views/send"); + + renderSendTokenSelect({ address: ADDRESS, tokenBalances: [HELD] }); + + return select.children.map((opt) => opt.value); + } + + for (const bad of ["notalist", 42, { a: 1 }, [42], [null], [{}]]) { + test(`${JSON.stringify(bad)}: a usable send screen`, async () => { + await expect(sendScreenTokens(bad)).resolves.toEqual([ + TOKEN_ADDRESS, + ]); + }); + } + + test("a real fraud entry beside a malformed one still hides its token", async () => { + await expect( + sendScreenTokens([42, TOKEN_ADDRESS.toLowerCase()]), + ).resolves.toEqual([]); + }); +}); + +describe("a malformed selectedToken", () => { + test("does not blank the popup on restore", async () => { + const env = await bootPopup( + unversionedValidProfile({ + currentView: "address-token", + selectedWallet: 0, + selectedAddress: 0, + selectedToken: 42, + viewStack: ["main", "address"], + }), + ); + + expect({ + visibleViews: env.visibleViews(), + errors: env.pageErrors, + }).toEqual({ visibleViews: ["main"], errors: [] }); + }); +}); + +// --------------------------------------------- a save that fails is told + +describe("a save that fails", () => { + function failingStorage(profile) { + const storage = makeStorageStub({ autistmask: profile }); + const realSet = storage.local.set; + storage.local.set = jest.fn(async () => { + throw new Error("QUOTA_BYTES quota exceeded"); + }); + storage.restoreWrites = () => { + storage.local.set = realSet; + }; + return storage; + } + + test("is reported, not swallowed by the save queue", async () => { + const storage = failingStorage(unversionedValidProfile()); + const { state, loadState, saveState, onSaveFailure } = + loadStateModule(storage); + const failures = []; + onSaveFailure((e) => failures.push(String(e && e.message))); + + await loadState(); + state.theme = "dark"; + // Not awaited, which is how showView() saves on every navigation and + // how the failure used to disappear entirely. + saveState(); + for (let i = 0; i < 50; i++) await Promise.resolve(); + + expect(failures).toEqual(["QUOTA_BYTES quota exceeded"]); + }); + + test("still rejects for a caller that awaits it", async () => { + const storage = failingStorage(unversionedValidProfile()); + const { state, loadState, saveState, onSaveFailure } = + loadStateModule(storage); + onSaveFailure(() => {}); + + await loadState(); + state.theme = "dark"; + + await expect(saveState()).rejects.toThrow("QUOTA_BYTES"); + }); + + test("puts a banner on the popup saying nothing is being saved", async () => { + const env = await bootPopup(undefined, { + storage: failingStorage(unversionedValidProfile()), + }); + + // The popup is still usable — the point is that it no longer looks + // healthy while silently persisting nothing. + expect(env.visibleViews()).toEqual(["main"]); + const banner = env.node("save-failure-banner"); + expect(banner).not.toBeNull(); + expect(banner.textContent).toContain("NOT SAVED"); + expect(banner.textContent).toContain("QUOTA_BYTES quota exceeded"); + }); + + test("no banner appears on a popup whose saves work", async () => { + const env = await bootPopup(unversionedValidProfile()); + + expect(env.node("save-failure-banner")).toBeNull(); + }); +}); diff --git a/tests/persistedFieldContract.test.js b/tests/persistedFieldContract.test.js new file mode 100644 index 0000000..0b4e4b8 --- /dev/null +++ b/tests/persistedFieldContract.test.js @@ -0,0 +1,825 @@ +// What the floor under each persisted field actually guarantees — as a table +// that RUNS, one row per field. +// +// This file replaces a hand-written per-field justification in the header of +// src/shared/stateSchema.js. That comment shipped a false claim in three +// consecutive changes: every author wrote plausible prose about thirty fields, +// every reviewer re-derived it by hand, and it kept being wrong in a different +// place each time. The artifact was the problem. A claim nobody can execute is +// worse than no claim, because it is believed. +// +// So the claim is a row here instead: +// +// KIND.REFUSED assertStateUsable() refuses the record outright. Proven by +// stateProblem() naming a problem for every hostile value. +// KIND.ENTRIES normalizePersisted() floors the container AND its entries. +// Proven by holds() over the normalized value. +// KIND.SCALAR normalizePersisted() floors it to one scalar type, or to a +// fixed fallback. Proven the same way. +// KIND.LOOSE `saved.x || default`, no type check at all. The claim is +// that no structural dereference of it is reachable from a +// stored record — which cannot be argued, only driven, so the +// proof is a boot of the REAL popup entry point over a stored +// record carrying the hostile value, ONTO EVERY RESTORABLE +// VIEW. Home is not where this class of defect lives. +// +// Every row is driven through a boot regardless of kind, but only a LOOSE row +// (or a row that sets `alsoSweep`) is swept across the restore path: that is +// what declaring LOOSE costs. ENTRIES and SCALAR rows are proven by their +// holds() instead, because a floored value is not hostile by the time a +// renderer sees it. A LOOSE row must additionally prove it is loose: if +// someone floors the field — even partially — and leaves the row saying LOOSE, +// the "survives verbatim" assertion fails. A field added to PERSISTED_FIELDS +// with no row fails the first test in the file. +// +// The sweep is what makes a LOOSE row falsifiable, so read how it is driven +// before trusting it. A row the ROUTER reads (`routes`) gets its own boot per +// view, because a hostile value in it legitimately changes which view renders. +// Every other swept field is corrupted on the SAME boot, one boot per view per +// slot, and that boot has to land on the view it stored — so a field that does +// move the routing cannot hide in the crowd. Every swept field is driven at +// BOTH POLARITIES: a value nothing in src/ writes is a wrong-typed one and so +// always truthy, which leaves `if (!state.x) { state.y.deref() }` unentered on +// the very boot that corrupts x. The last slot is the falsy one for that +// reason, and a field that cannot be falsy after the floor says so in its row +// and is proven so. +// +// What that buys: a field any restorable view dereferences on that view's +// render turns this file red, at either polarity. What it does not buy is a +// MIX of polarities — one boot puts every swept field on the same slot, so a +// branch reached only when one swept field is truthy and another falsy is not +// entered. Booting every field separately at every value would be several +// hundred boots and most of the suite's budget; this is forty-four. +// +// The three claims this replaced, all false, all caught here by construction: +// rpcUrl reaching `new JsonRpcProvider()` (a synchronous throw, not a caught +// request); viewData's ENTRIES being dereferenced by four restore branches +// that gate on one truthy field each; and selectedWallet, where a stale +// integer index is the SAFE case and `wallets["map"]` is the throwing one. + +const { + PERSISTED_FIELDS, + normalizePersisted, +} = require("../src/shared/persistedState"); +const { stateProblem } = require("../src/shared/stateSchema"); +const { RESTORABLE_VIEWS } = require("../src/shared/restorableViews"); +const { + bootPopup, + cleanupPopup, + unversionedValidProfile, + ADDRESS, + TOKEN_ADDRESS, +} = require("./support/popupBoot"); + +const KIND = { + REFUSED: "refused by the gate", + ENTRIES: "container and entries type-checked", + SCALAR: "scalar type-checked", + LOOSE: "loosely floored; safety proven by driving the popup", +}; + +const isText = (v) => typeof v === "string"; +const isRecord = (v) => + typeof v === "object" && v !== null && !Array.isArray(v); +const isIndexOrNull = (v) => v === null || (Number.isInteger(v) && v >= 0); +const isTextOrNull = (v) => v === null || (isText(v) && v !== ""); +const everyEntry = (v, fn) => Array.isArray(v) && v.every(fn); + +// A row is SWEPT — driven onto every restorable view rather than only onto +// Home — when its claim is that no restore path dereferences the field. That +// is what LOOSE means. The two index rows opt in with `alsoSweep` although +// they are floored, because the restore path is precisely why they gained a +// floor and the sweep is the regression guard on it. +const swept = (row) => row.kind === KIND.LOOSE || Boolean(row.alsoSweep); + +// Every value a swept row drives through a boot: the hostile set, plus the +// falsy slot that gives the field its other polarity. `hostile` values are all +// TRUTHY by nature — a value nothing in src/ writes is a wrong-typed one, and +// wrong-typed values are objects, non-empty strings and non-zero numbers. A +// field that is only ever truthy on the boot that corrupts it cannot falsify +// `if (!state.x) { state.y.deref() }`, so the falsy slot is not optional. +const sweptValues = (row) => [...row.hostile, ...(row.falsy || [])]; + +// ------------------------------------------------------------------ the table +// +// `hostile` is values a stored record can carry that nothing in src/ ever +// writes. Each one is driven through the floor AND through a real popup boot — +// and, for a swept row, through one boot per restorable view — so keep the +// list short and pointed. `floorOnly` is extra values checked against the +// floor alone, which is pure and free. `hostileRestore` is extra values driven +// through the restore path only, for a value that means nothing until a +// particular branch's gate has let it past. +// +// `falsy` is the other POLARITY of a swept field, driven for the same reason. +// It is not a value src/ never writes — for three of these fields it is the +// DEFAULT_STATE default, which is the branch every ordinary install takes — +// and that is the point: without it, a dereference behind `if (!state.x)` is +// unreachable on the one boot that corrupts x. A swept row that cannot supply +// one says `neverFalsy` instead, which is proven rather than asserted: every +// falsy value stored under that field comes back TRUTHY from the floor, so no +// `!state.x` branch is reachable from a stored record at all. + +const CONTRACT = [ + { + field: "wallets", + kind: KIND.REFUSED, + hostile: [42, "notastructure", { a: 1 }, [null], [{ addresses: 1 }]], + }, + { + field: "networkId", + kind: KIND.REFUSED, + hostile: [42, "notanetwork", { a: 1 }, "__proto__"], + }, + { + field: "trackedTokens", + kind: KIND.ENTRIES, + hostile: [42, "notalist", { a: 1 }], + floorOnly: [[1, 2], [null], [{}], [[TOKEN_ADDRESS]]], + holds: (v) => everyEntry(v, (t) => isRecord(t) && isText(t.address)), + }, + { + field: "allowedSites", + kind: KIND.ENTRIES, + hostile: [42, "notarecord", { [ADDRESS]: "notalist" }], + floorOnly: [ + [ADDRESS], + { [ADDRESS]: 42 }, + { [ADDRESS]: [42, null, {}] }, + JSON.parse('{"__proto__":["evil.invalid"]}'), + ], + holds: siteMapHolds, + }, + { + field: "deniedSites", + kind: KIND.ENTRIES, + hostile: [42, "notarecord", { [ADDRESS]: "notalist" }], + floorOnly: [ + [ADDRESS], + { [ADDRESS]: 42 }, + { [ADDRESS]: [42, null, {}] }, + JSON.parse('{"__proto__":["evil.invalid"]}'), + ], + holds: siteMapHolds, + }, + { + field: "fraudContracts", + kind: KIND.ENTRIES, + hostile: [42, "notalist", { a: 1 }], + floorOnly: [[42], [null], [{}], [[TOKEN_ADDRESS]]], + holds: (v) => everyEntry(v, isText), + }, + { + field: "viewStack", + kind: KIND.ENTRIES, + hostile: [42, "notalist", ["main", "show-phrase", "settings"]], + floorOnly: [[1, 2], [null], [{}], ["export-privkey"]], + // Truncated at the first entry the popup will not reopen onto, rather + // than filtered: every surviving entry's Back target has to stay the + // one it had. restorableStack() may also substitute ["main"] under a + // view restored below the root, so this is the one ENTRIES field whose + // result is not always a subset of what was stored. + holds: (v) => everyEntry(v, (e) => RESTORABLE_VIEWS.has(e)), + }, + { + field: "networkEndpoints", + kind: KIND.ENTRIES, + hostile: [42, "notarecord", { mainnet: "notapair" }], + floorOnly: [ + [1, 2], + { mainnet: { rpcUrl: 42, blockscoutUrl: {} } }, + { mainnet: { rpcUrl: "", blockscoutUrl: [] } }, + { sepolia: 42 }, + ], + // Entries are coerced rather than dropped: an unknown network id is + // KEPT, so a profile that has been on a build with more networks does + // not lose their endpoints here. What is floored is the two URL fields + // inside the pair, which applyChainSwitchFields() assigns straight onto + // s.rpcUrl / s.blockscoutUrl on the next switch. + holds: (v) => + isRecord(v) && + Object.keys(v).every((id) => { + const pair = v[id]; + return ( + isRecord(pair) && + (pair.rpcUrl === undefined || + (isText(pair.rpcUrl) && pair.rpcUrl !== "")) && + (pair.blockscoutUrl === undefined || + (isText(pair.blockscoutUrl) && + pair.blockscoutUrl !== "")) + ); + }), + }, + { + field: "rpcUrl", + kind: KIND.SCALAR, + hostile: [42, true, { a: 1 }], + floorOnly: [[], "", null], + holds: (v) => isText(v) && v !== "", + // The claim this row replaced said a bad value "fails the request on a + // path that already catches". It does not: getProvider() hands rpcUrl + // to `new JsonRpcProvider()`, which throws SYNCHRONOUSLY, from two call + // sites outside any try — and a stored `currentView: "wait-tx"` reaches + // one of them through restoreView(). So the row proves the claim + // against the real constructor rather than describing it. + alsoProven: (normalized, hostile) => { + // requireActual: bootPopup() mocks this module out for the boots + // above, and a mocked getProvider() would prove nothing at all + // about the constructor this row is a claim about. + const { getProvider } = jest.requireActual( + "../src/shared/balances", + ); + expect(() => getProvider(hostile, "mainnet")).toThrow(); + const provider = getProvider(normalized, "mainnet"); + expect(provider).toBeTruthy(); + provider.destroy(); + }, + }, + { + field: "blockscoutUrl", + kind: KIND.SCALAR, + hostile: [42, true, { a: 1 }], + floorOnly: [[], "", null], + holds: (v) => isText(v) && v !== "", + }, + { + field: "activeAddress", + kind: KIND.SCALAR, + hostile: [42, true, { a: 1 }], + floorOnly: [[ADDRESS], ""], + holds: isTextOrNull, + }, + { + field: "selectedToken", + kind: KIND.SCALAR, + hostile: [42, true, { a: 1 }], + floorOnly: [[TOKEN_ADDRESS], ""], + holds: isTextOrNull, + }, + { + field: "selectedWallet", + kind: KIND.SCALAR, + // The prototype members are the whole point: `wallets["map"]` is + // TRUTHY, so hasValidAddress()'s `&&` does not short-circuit and + // `.addresses[…]` throws. A stale INTEGER is the safe case. + hostile: ["map", "__proto__", { a: 1 }], + floorOnly: ["length", "constructor", "toString", "0", -1, 1.5, true], + holds: isIndexOrNull, + // SCALAR, and swept anyway: the restore path is precisely why this + // field gained a floor, so the sweep is the regression guard on it. + alsoSweep: true, + routes: true, + // A stale INTEGER index, which reaches the restore path by a different + // route from the prototype members above — falsy or out of range + // rather than truthy — and has to keep being the safe case. + hostileRestore: [{ value: "length" }, { value: 5 }], + }, + { + field: "selectedAddress", + kind: KIND.SCALAR, + hostile: ["map", "__proto__", { a: 1 }], + floorOnly: ["length", "constructor", "toString", "0", -1, 1.5, true], + holds: isIndexOrNull, + alsoSweep: true, + routes: true, + hostileRestore: [{ value: 5 }], + }, + { + field: "currentView", + kind: KIND.LOOSE, + routes: true, + // Compared, and concatenated into the debug banner's textContent + // (src/popup/views/helpers.js) with no gate in front of it, which + // coerces. Nothing renders FROM it without RESTORABLE_VIEWS.has() + // first, and Set.has() answers false for any value. + hostile: [42, "no-such-view", { a: 1 }], + // `saved.currentView || null`: the falsy polarity is the popup landing + // on Home, which every boot in "booting onto Home" below also drives. + falsy: [""], + }, + { + field: "viewData", + kind: KIND.LOOSE, + routes: true, + // The container is taken verbatim; what makes its ENTRIES safe is the + // per-branch guard in src/popup/viewRouter.js. The sweep drives the + // container shapes below onto every restorable view; hostileRestore + // adds the records that PASS a branch's gate and then hand its + // renderer something it dereferences, which is where the entries are + // actually decided. + hostile: [42, "notarecord", { a: 1 }, [1, 2]], + // `structuredClone(saved.viewData || {})`: the container is never falsy + // in state whatever was stored, so no `!state.viewData` branch exists to + // drive. + neverFalsy: true, + hostileRestore: [ + // success-tx passes on `data.hash`, and renderSuccess() then calls + // toAddressHtml(d.to) -> addressTitle() -> address.toLowerCase(). + { value: { hash: "0x1" }, views: ["success-tx"] }, + { value: { hash: "0x1", to: 42 }, views: ["success-tx"] }, + { + value: { hash: "0x1", to: ADDRESS, decoded: { details: 7 } }, + views: ["success-tx"], + }, + { + value: { + hash: "0x1", + to: ADDRESS, + decoded: { details: [{ address: 42 }] }, + }, + views: ["success-tx"], + }, + // error-tx passes on `data.message`, same dereference. + { value: { message: "boom" }, views: ["error-tx"] }, + { value: { message: "boom", to: 42 }, views: ["error-tx"] }, + // transaction passes on `data.tx`. + { value: { tx: { hash: "0x1" } }, views: ["transaction"] }, + { + value: { + tx: { + hash: "0x1", + from: ADDRESS, + to: ADDRESS, + contractAddress: 42, + }, + }, + views: ["transaction"], + }, + // confirm-tx passes on `data.pendingTx`. + { value: { pendingTx: { amount: "1" } }, views: ["confirm-tx"] }, + { + value: { + pendingTx: { + token: 42, + from: ADDRESS, + to: ADDRESS, + amount: "1", + }, + }, + views: ["confirm-tx"], + }, + // wait-tx passes on `pendingWait.hash`; restoreWait() has checked + // the fields below it since it was written, and this is the + // regression guard. + { + value: { + pendingWait: { + hash: "0x1", + txInfo: { to: 42, amount: "1" }, + }, + }, + views: ["wait-tx"], + }, + // A record that passes EVERY branch's gate at once, driven onto + // every restorable view: a branch a view does not read must stay + // one it does not read, and each renderer must survive the fields + // another branch left behind. + { + value: { + hash: "0x1", + message: "boom", + tx: { hash: "0x1" }, + pendingTx: { amount: "1" }, + pendingWait: { hash: "0x1" }, + }, + }, + ], + }, + { + field: "lastBalanceRefresh", + kind: KIND.LOOSE, + // Arithmetic only: `now - (s.lastBalanceRefresh || 0)` compares false + // for a non-number and forces a refresh. + hostile: [true, "notatime", { a: 1 }], + // `|| 0` collapses every falsy stored value to 0, so 0 IS the whole + // falsy polarity of this field — and it is the DEFAULT_STATE default, + // the value a profile carries until its first refresh lands. + falsy: [0], + }, + { + field: "tokenHolderCache", + kind: KIND.LOOSE, + // Nothing DEREFERENCES it structurally. It is read by the + // field-agnostic snapshotPersisted()/deepEqual() in + // src/shared/state.js, which are safe for any value, and otherwise + // only reset wholesale in src/shared/chainSwitchFields.js. + hostile: [42, "notarecord", [1, 2]], + // `structuredClone(saved.tokenHolderCache || {})`. + neverFalsy: true, + }, + { + field: "theme", + kind: KIND.LOOSE, + // Compared against "dark"/"light" in applyTheme() and otherwise falls + // to the system branch; assigned into an input .value, which coerces. + hostile: [42, "chartreuse", { a: 1 }], + // `saved.theme || "system"`. + neverFalsy: true, + }, + { + field: "dustThresholdGwei", + kind: KIND.LOOSE, + hostile: ["notanumber", true, { a: 1 }], + // Survives verbatim, so the falsy slot is also wrong-typed: "" reaches + // filterTransactions() as a comparand and a settings input .value. + falsy: [""], + }, + ...[ + "rememberSiteChoice", + "showZeroBalanceTokens", + "hideSpoofedSymbols", + "hideLowHolderTokens", + "hideFraudContracts", + "hideDustTransactions", + "utcTimestamps", + "debugMode", + ].map((field) => ({ + field, + kind: KIND.LOOSE, + // A flag: only ever tested for truthiness, and written back verbatim. + hostile: [42, "notabool", { a: 1 }], + // Both answers to that truthiness test have to be driven, and 0 is a + // value src/ never writes for a flag. For utcTimestamps and debugMode + // the falsy answer is also the DEFAULT_STATE default. + falsy: [0], + })), +]; + +function siteMapHolds(v) { + return ( + isRecord(v) && + Object.getPrototypeOf(v) === Object.prototype && + !Object.prototype.hasOwnProperty.call(v, "__proto__") && + Object.keys(v).every((key) => everyEntry(v[key], isText)) + ); +} + +afterEach(() => { + cleanupPopup(); +}); + +// -------------------------------------------------------------- exhaustive + +describe("the contract covers the record", () => { + test("every persisted field has exactly one row, and no row invents one", () => { + const rows = CONTRACT.map((row) => row.field); + + expect([...rows].sort()).toEqual([...PERSISTED_FIELDS].sort()); + }); + + test("every row declares a kind this file knows how to prove", () => { + const kinds = Object.values(KIND); + for (const row of CONTRACT) { + expect(kinds).toContain(row.kind); + expect(row.hostile.length).toBeGreaterThan(0); + } + }); +}); + +// ------------------------------------------------------------- the floors + +function profileWith(field, value) { + return unversionedValidProfile({ [field]: value }); +} + +describe("the floor each row claims", () => { + // The falsy slot is deliberately NOT in here. `saved.x || default` is a + // floor on falsy values and on nothing else, so a falsy value is the one + // thing a LOOSE field need not carry through verbatim; what it has to carry + // through is being falsy, which "both polarities" below asserts. + for (const row of CONTRACT) { + const values = [...row.hostile, ...(row.floorOnly || [])]; + + if (row.kind === KIND.REFUSED) { + test(`${row.field}: the gate refuses it`, () => { + for (const value of values) { + expect( + typeof stateProblem(profileWith(row.field, value)), + ).toBe("string"); + } + }); + continue; + } + + test(`${row.field}: ${row.kind}`, () => { + for (const value of values) { + const out = normalizePersisted(profileWith(row.field, value)); + if (row.kind === KIND.LOOSE) { + // The claim IS that there is no floor. A field that grows + // one has to move to another kind rather than keep a row + // saying its readers are what make it safe. + continue; + } + expect({ + value: value, + holds: row.holds(out[row.field]), + }).toEqual({ value: value, holds: true }); + } + }); + + if (row.kind === KIND.LOOSE) { + test(`${row.field}: is genuinely unfloored`, () => { + // EVERY value, not some: a PARTIAL floor is still a floor, and + // a row that keeps saying LOOSE because one hostile value out + // of three still survives is exactly the stale claim this file + // exists to stop. + for (const value of values) { + const out = normalizePersisted( + profileWith(row.field, value), + ); + expect({ + value: value, + survived: JSON.stringify(out[row.field]), + }).toEqual({ + value: value, + survived: JSON.stringify(value), + }); + } + }); + } + } +}); + +// --------------------------------------------- driving the real popup boot + +// A booted popup is healthy when nothing threw out of init() and something is +// on screen. A throw out of restoreView() is neither: init() does not guard it, +// so the rest of popup init never runs and the user gets a popup with no view, +// no message and no control on it. +async function bootHealth(profile) { + const env = await bootPopup(profile); + return { + errors: env.pageErrors, + blank: env.visibleViews().length === 0, + }; +} + +const HEALTHY = { errors: [], blank: false }; + +// unversionedValidProfile() stores no currentView, so every boot in here lands +// on Home. That is the cheap half of the proof; the restore path below is the +// half that matters. +// Both polarities of every swept field are driven, or the field is proven +// unable to take one of them. This is the guard on the sweep itself: a hostile +// set is all-truthy by construction, so without a falsy slot a dereference +// behind `if (!state.x)` is never reached on the boot that corrupts x — the +// same falsy-collapse blind spot the fields below were floored for. +describe("both polarities of every swept field are driven", () => { + const FALSY_STORED = [0, "", false, null]; + const floored = (field, value) => + normalizePersisted(profileWith(field, value))[field]; + + for (const row of CONTRACT) { + if (!swept(row)) continue; + + if (row.neverFalsy) { + test(`${row.field}: cannot be falsy in state at all`, () => { + for (const value of FALSY_STORED) { + expect({ + stored: value, + truthy: Boolean(floored(row.field, value)), + }).toEqual({ stored: value, truthy: true }); + } + }); + continue; + } + + test(`${row.field}: truthy and falsy`, () => { + // What the boots below actually drive, floored the way a renderer + // sees it — not what the row says it drives. + const driven = [ + ...sweptValues(row), + ...(row.hostileRestore || []).map((entry) => entry.value), + ].map((value) => floored(row.field, value)); + + expect({ + truthy: driven.some((value) => Boolean(value)), + falsy: driven.some((value) => !value), + }).toEqual({ truthy: true, falsy: true }); + }); + } +}); + +describe("a hostile value for one field, booting onto Home", () => { + for (const row of CONTRACT) { + for (const value of sweptValues(row)) { + test(`${row.field} = ${JSON.stringify(value)}`, async () => { + await expect( + bootHealth(profileWith(row.field, value)), + ).resolves.toEqual(HEALTHY); + }); + } + } +}); + +describe("a row's extra proof against the real reader", () => { + for (const row of CONTRACT) { + if (!row.alsoProven) continue; + test(row.field, () => { + for (const value of row.hostile) { + const out = normalizePersisted(profileWith(row.field, value)); + row.alsoProven(out[row.field], value); + } + }); + } +}); + +// ------------------------------------------------ driving the restore path + +// Everything above lands on Home. Home is not where this class of defect +// lives: all three of the false claims this file replaced were falsified by a +// RESTORE, through the unguarded restoreView() in src/popup/index.js. So a +// swept row's hostile values are driven onto EVERY restorable view, one boot +// each. +// +// This is what makes a LOOSE row falsifiable. A field that gains a structural +// dereference on any restorable view — `state.theme.toLowerCase()` in a view's +// show(), say — turns the row red here, instead of waiting for a reviewer to +// re-derive the claim by hand. + +// restoreWait() resumes from this, so it has to be a finite number and recent +// enough that the resumed deadline has not already passed — a wait that has +// outlived its deadline resolves on the first poll instead of staying on +// screen. Read once at module load, so every boot in one run shares it. +const BROADCAST_TIME = Date.now(); + +// A viewData well formed for every restorable branch at once, so the only +// thing a swept boot can fail on is the field the row corrupts. "the base +// profile the sweep corrupts" below proves this really does render each view +// rather than falling back — without that, a sweep could pass by never +// reaching a renderer at all. +const WELL_FORMED_DATA = { + hash: "0x1", + message: "boom", + to: ADDRESS, + decoded: { details: [{ address: TOKEN_ADDRESS }] }, + tx: { hash: "0x1", from: ADDRESS, to: ADDRESS, contractAddress: null }, + pendingTx: { + token: "ETH", + from: ADDRESS, + to: ADDRESS, + amount: "1", + balance: "2", + }, + pendingWait: { + hash: "0x1", + txInfo: { to: ADDRESS, amount: "1" }, + broadcastTime: BROADCAST_TIME, + }, +}; + +function restoringOnto(view, extra) { + return unversionedValidProfile({ + currentView: view, + selectedWallet: 0, + selectedAddress: 0, + selectedToken: TOKEN_ADDRESS, + viewStack: ["main"], + viewData: WELL_FORMED_DATA, + ...extra, + }); +} + +// A boot that RESTORED is healthy and landed on the view it stored, rather +// than falling back to Home — which a healthy boot also does, and which would +// let a sweep pass by never running the renderer it is aimed at. +async function restoredHealth(profile, view) { + const env = await bootPopup(profile); + return { + errors: env.pageErrors, + restored: env.visibleViews().includes(view), + }; +} + +const RESTORED = { errors: [], restored: true }; + +describe("the base profile the sweep corrupts", () => { + for (const view of RESTORABLE_VIEWS) { + test(`renders ${view} rather than falling back`, async () => { + await expect( + restoredHealth(restoringOnto(view), view), + ).resolves.toEqual(RESTORED); + }); + } +}); + +// A field the ROUTER itself reads — the two it gates on and the two +// hasValidAddress() indexes with. A hostile value in one of these legitimately +// changes which view renders, so each gets its own boot per view and is held +// only to "healthy", not to "restored onto the view it stored". +const routes = (row) => Boolean(row.routes); + +// Every routing row × every hostile value × every restorable view. Profiles +// are deduplicated because a hostile `currentView` REPLACES the view being +// restored onto, which would otherwise be the same boot eleven times. +describe("a hostile routing value restoring onto", () => { + for (const row of CONTRACT) { + if (!swept(row) || !routes(row)) continue; + const seen = new Set(); + for (const value of sweptValues(row)) { + for (const view of RESTORABLE_VIEWS) { + const profile = restoringOnto(view, { [row.field]: value }); + const key = JSON.stringify(profile); + if (seen.has(key)) continue; + seen.add(key); + test(`${view}: ${row.field} = ${JSON.stringify( + value, + )}`, async () => { + await expect(bootHealth(profile)).resolves.toEqual(HEALTHY); + }); + } + } + } +}); + +// Every OTHER swept field, corrupted at once, one boot per view per hostile +// slot: twelve fields on one boot rather than twelve boots. A field is only in +// here because it is not one the router reads — and that is ASSERTED, not +// argued, because the boot has to land on `view`. A field that does move the +// routing turns this red and has to declare `routes` and take the individual +// sweep above. +// +// Combining does hide one thing, and the last slot is what stops it. A hostile +// value is wrong-typed and therefore TRUTHY, so on a boot where every swept +// field is hostile, no `if (!state.x)` branch is entered — and a dereference +// inside such a branch would go unseen however loudly it throws. The last slot +// is the falsy one: every swept field that CAN be falsy is falsy on it, which +// is also the state an ordinary install boots in for three of them, while the +// fields that cannot be falsy stay hostile. Beyond that, a throw fails the boot +// whichever field threw, and a renderer that never ran is what `restored` +// forbids. When it does go red, the same view is re-booted one field at a time +// so the failure names the fields rather than leaving a reader to bisect twelve +// of them. +const UNROUTED = CONTRACT.filter((row) => swept(row) && !routes(row)); +const HOSTILE_SLOTS = Math.max( + ...UNROUTED.map((row) => sweptValues(row).length), +); + +function unroutedValues(slot) { + const fields = {}; + for (const row of UNROUTED) { + const values = sweptValues(row); + fields[row.field] = values[slot % values.length]; + } + return fields; +} + +describe("every field the router does not read, corrupted at once, onto", () => { + for (const view of RESTORABLE_VIEWS) { + for (let slot = 0; slot < HOSTILE_SLOTS; slot++) { + test(`${view}: hostile value ${slot + 1} in all ${ + UNROUTED.length + } of them`, async () => { + const fields = unroutedValues(slot); + const together = await restoredHealth( + restoringOnto(view, fields), + view, + ); + if (together.errors.length === 0 && together.restored) { + expect(together).toEqual(RESTORED); + return; + } + + const named = []; + for (const row of UNROUTED) { + const one = await restoredHealth( + restoringOnto(view, { + [row.field]: fields[row.field], + }), + view, + ); + if (one.errors.length === 0 && one.restored) continue; + named.push( + `${row.field}=${JSON.stringify(fields[row.field])}: ` + + (one.errors.join("; ") || `fell off ${view}`), + ); + } + expect({ view: view, fields: named }).toEqual({ + view: view, + fields: [], + }); + }); + } + } +}); + +// The values that only mean something on the restore path: a viewData that +// PASSES a branch's gate and then hands its renderer something dereferenced, +// and the index values whose route through hasValidAddress() differs from the +// row's own hostile set. +describe("a restore-only hostile value onto", () => { + for (const row of CONTRACT) { + for (const entry of row.hostileRestore || []) { + for (const view of entry.views || RESTORABLE_VIEWS) { + test(`${view}: ${row.field} = ${JSON.stringify( + entry.value, + )}`, async () => { + await expect( + bootHealth( + restoringOnto(view, { [row.field]: entry.value }), + ), + ).resolves.toEqual(HEALTHY); + }); + } + } + } +}); diff --git a/tests/popupElementIds.test.js b/tests/popupElementIds.test.js index c4a188f..2f93efc 100644 --- a/tests/popupElementIds.test.js +++ b/tests/popupElementIds.test.js @@ -35,6 +35,11 @@ const POPUP_HTML_PATH = path.join(POPUP_DIR, "index.html"); const RUNTIME_CREATED_IDS = new Set([ // Created by updateDebugBanner() in src/popup/views/helpers.js. "debug-banner", + // Created by showSaveFailureBanner() in the same file, on the first save + // that fails. Absent from the markup on purpose: a popup where nothing has + // failed must not have to carry an empty banner + // (https://git.eeqj.de/sneak/AutistMask/issues/362). + "save-failure-banner", ]); // Every id lookup the popup performs with a literal argument, as diff --git a/tests/stateRecovery.test.js b/tests/stateRecovery.test.js index a7e48b9..c76c7b7 100644 --- a/tests/stateRecovery.test.js +++ b/tests/stateRecovery.test.js @@ -13,61 +13,26 @@ // calls, is exactly the defect: what has to be true is that BOOTING the popup // on a bad blob lands on it. // -// The DOM stub is built FROM src/popup/index.html — every id in the markup, -// with the classes the markup gives it — so "which views are visible" is -// answered against the real element set, and a recovery screen with no markup -// behind it cannot pass. +// The boot harness and its DOM stub — built FROM src/popup/index.html, so +// "which views are visible" is answered against the real element set — live in +// tests/support/popupBoot.js, since tests/persistedEntryFloors.test.js needs +// the same boot. // // The fourth case is the upgrade one, and it is the case that must NOT reach // the recovery screen: every install in the field has a valid profile with no // version field, and showing those users a wipe prompt would be a worse defect // than the one being fixed. It is migrated in place and keeps working. -const fs = require("fs"); -const path = require("path"); - -const { makeStorageStub } = require("./support/storageStub"); - -const POPUP_HTML = fs.readFileSync( - path.join(__dirname, "..", "src", "popup", "index.html"), - "utf8", -); - -// Fixed address, never used for anything but these tests. -const ADDRESS = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; -// A fixed ERC-20 contract address, same rule. -const TOKEN_ADDRESS = "0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; +const { + bootPopup, + cleanupPopup, + unversionedValidProfile, + ADDRESS, + TOKEN_ADDRESS, +} = require("./support/popupBoot"); // ------------------------------------------------------------- fixtures -// A profile in the shape every install in the field has it: complete, valid, -// and carrying no version field, because no build ever wrote one. -function unversionedValidProfile() { - return { - hasWallet: true, - wallets: [ - { - type: "hd", - name: "Wallet 1", - xpub: "xpub-wallet-1", - encryptedSecret: "encrypted-secret-1", - nextIndex: 1, - addresses: [ - { address: ADDRESS, balance: "1.5", tokenBalances: [] }, - ], - }, - ], - activeAddress: ADDRESS, - networkId: "mainnet", - rpcUrl: "https://ethereum-rpc.publicnode.com", - blockscoutUrl: "https://eth.blockscout.com/api/v2", - allowedSites: { [ADDRESS]: ["dapp.example"] }, - deniedSites: {}, - trackedTokens: [], - theme: "system", - }; -} - // The three blobs from the issue, each with the error it produced. const CORRUPT_BLOBS = [ { @@ -103,235 +68,7 @@ const CORRUPT_BLOBS = [ }, ]; -// ------------------------------------------------------------- DOM stub - -function makeElement(id, className) { - const classes = new Set( - (className || "").split(/\s+/).filter((name) => name !== ""), - ); - const el = { - id, - tagName: "DIV", - textContent: "", - value: "", - innerHTML: "", - href: "", - download: "", - disabled: false, - style: {}, - dataset: {}, - listeners: {}, - clicked: 0, - classList: { - add: (...names) => names.forEach((n) => classes.add(n)), - remove: (...names) => names.forEach((n) => classes.delete(n)), - contains: (n) => classes.has(n), - toggle: (n, force) => { - const on = force === undefined ? !classes.has(n) : force; - if (on) classes.add(n); - else classes.delete(n); - return on; - }, - }, - addEventListener: (name, fn) => { - el.listeners[name] = el.listeners[name] || []; - el.listeners[name].push(fn); - }, - removeEventListener: () => {}, - appendChild: () => {}, - remove: () => {}, - focus: () => {}, - select: () => {}, - setAttribute: (name, value) => { - el[name] = value; - }, - querySelector: () => null, - querySelectorAll: () => [], - click: () => { - el.clicked += 1; - }, - }; - return el; -} - -// Every id in the markup, with the classes the markup gives it. A view the -// popup is supposed to reveal has to exist here, which means it has to exist -// in src/popup/index.html. -function idsFromHtml(html) { - const out = new Map(); - const tags = html.match(/<[a-zA-Z][^>]*>/g) || []; - for (const tag of tags) { - const id = /\bid="([^"]+)"/.exec(tag); - if (!id) continue; - const cls = /\bclass="([^"]*)"/.exec(tag); - out.set(id[1], cls ? cls[1] : ""); - } - return out; -} - -function makeDocument(html) { - const authored = idsFromHtml(html); - const els = new Map(); - for (const [id, className] of authored) { - els.set(id, makeElement(id, className)); - } - const created = []; - const doc = { - listeners: {}, - getElementById(id) { - // Created on demand by updateDebugBanner(); absent is the state a - // non-debug, non-testnet popup is in. - if (id === "debug-banner") return null; - if (!els.has(id)) els.set(id, makeElement(id, "")); - return els.get(id); - }, - createElement(tag) { - const el = makeElement("created-" + tag, ""); - el.tagName = String(tag).toUpperCase(); - created.push(el); - return el; - }, - addEventListener(name, fn) { - doc.listeners[name] = doc.listeners[name] || []; - doc.listeners[name].push(fn); - }, - querySelectorAll: () => [], - documentElement: makeElement("html", ""), - body: { - prepend: () => {}, - appendChild: () => {}, - removeChild: () => {}, - }, - elements: els, - authoredIds: authored, - created, - }; - return doc; -} - -// ------------------------------------------------------------- harness - -// Boot the real popup entry point over `stored`, exactly as the browser does: -// storage already holds the record, the page loads, DOMContentLoaded fires. -async function bootPopup(stored) { - jest.resetModules(); - - // The two modules that reach the network. Neither is on the path under - // test; both would make this suite hit the internet. - jest.doMock("../src/shared/prices", () => ({ - prices: {}, - refreshPrices: jest.fn(async () => {}), - clearPrices: jest.fn(), - getPrice: () => null, - formatUsd: () => "", - formatAddressTotal: () => "", - getAddressValue: () => ({ usd: null, partial: false }), - getWalletValue: () => ({ usd: null, partial: false }), - getTotalValue: () => ({ usd: null, partial: false }), - })); - jest.doMock("../src/shared/balances", () => ({ - fetchTokenBalances: jest.fn(async () => []), - refreshBalances: jest.fn(async () => {}), - lookupTokenInfo: jest.fn(async () => null), - getProvider: () => ({}), - scanForAddresses: jest.fn(async () => []), - })); - jest.doMock("../src/shared/transactions", () => ({ - fetchRecentTransactions: jest.fn(async () => []), - filterTransactions: () => [], - })); - - const storage = makeStorageStub( - stored === undefined ? {} : { autistmask: stored }, - ); - const document = makeDocument(POPUP_HTML); - const reloads = []; - - globalThis.chrome = { - storage: { local: storage.local }, - runtime: { - sendMessage: jest.fn(async () => ({})), - getURL: (p) => "chrome-extension://autistmask/" + p, - onMessage: { addListener: () => {} }, - }, - }; - globalThis.document = document; - globalThis.window = { - location: { - search: "", - href: "chrome-extension://autistmask/src/popup/index.html", - reload: () => reloads.push(Date.now()), - }, - matchMedia: () => ({ - matches: false, - addEventListener: () => {}, - removeEventListener: () => {}, - }), - addEventListener: () => {}, - }; - // The 10s refresh loop init() starts would outlive the test. - const realSetInterval = globalThis.setInterval; - globalThis.setInterval = () => 0; - - require("../src/popup/index"); - - const booted = []; - for (const fn of document.listeners.DOMContentLoaded || []) { - booted.push(fn()); - } - - // What the browser console would have shown. A throw out of init() is the - // blank popup this issue is about, so it is captured rather than thrown: - // the assertion that matters is what ended up on screen. - const pageErrors = []; - for (const p of booted) { - try { - await p; - } catch (e) { - pageErrors.push(String((e && e.message) || e)); - } - } - await settle(); - - globalThis.setInterval = realSetInterval; - - return { - storage, - document, - pageErrors, - reloaded: () => reloads.length, - node: (id) => document.getElementById(id), - text: (id) => document.getElementById(id).textContent, - value: (id) => document.getElementById(id).value, - hidden: (id) => - document.getElementById(id).classList.contains("hidden"), - click: async (id) => { - const el = document.getElementById(id); - const fns = el.listeners.click || []; - for (const fn of fns) await fn(); - await settle(); - }, - // The view ids whose section is not hidden, as the audit measured them. - visibleViews: () => { - const out = []; - for (const [id, el] of document.elements) { - if (!id.startsWith("view-")) continue; - if (!el.classList.contains("hidden")) out.push(id.slice(5)); - } - return out; - }, - }; -} - -async function settle() { - for (let i = 0; i < 50; i++) await Promise.resolve(); -} - -afterEach(() => { - delete globalThis.chrome; - delete globalThis.document; - delete globalThis.window; -}); +afterEach(cleanupPopup); // --------------------------------------------------------------- tests diff --git a/tests/support/popupBoot.js b/tests/support/popupBoot.js new file mode 100644 index 0000000..e8dcebb --- /dev/null +++ b/tests/support/popupBoot.js @@ -0,0 +1,347 @@ +// Boot the REAL popup entry point over a stored record, exactly as the browser +// does: storage already holds the record, the page loads, DOMContentLoaded +// fires. +// +// Written for https://git.eeqj.de/sneak/AutistMask/issues/311 inside +// tests/stateRecovery.test.js and lifted here unchanged in substance when +// https://git.eeqj.de/sneak/AutistMask/issues/362 needed the same boot for a +// second field. Assertions about a corrupt stored profile have to be made +// through the entry point rather than against a view module: a screen that +// renders perfectly when something calls it, and that nothing calls, IS the +// defect. +// +// The DOM stub is built FROM src/popup/index.html — every id in the markup, +// with the classes the markup gives it — so "which views are visible" is +// answered against the real element set, and a screen with no markup behind it +// cannot pass. + +const fs = require("fs"); +const path = require("path"); + +const { makeStorageStub } = require("./storageStub"); + +const POPUP_HTML = fs.readFileSync( + path.join(__dirname, "..", "..", "src", "popup", "index.html"), + "utf8", +); + +// Fixed addresses, never used for anything but these tests. +const ADDRESS = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; +const TOKEN_ADDRESS = "0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + +// A profile in the shape every install in the field has it: complete, valid, +// and carrying no version field, because no build ever wrote one. The starting +// point for "and now corrupt exactly one field of it". +function unversionedValidProfile(extra) { + return { + hasWallet: true, + wallets: [ + { + type: "hd", + name: "Wallet 1", + xpub: "xpub-wallet-1", + encryptedSecret: "encrypted-secret-1", + nextIndex: 1, + addresses: [ + { address: ADDRESS, balance: "1.5", tokenBalances: [] }, + ], + }, + ], + activeAddress: ADDRESS, + networkId: "mainnet", + rpcUrl: "https://ethereum-rpc.publicnode.com", + blockscoutUrl: "https://eth.blockscout.com/api/v2", + allowedSites: { [ADDRESS]: ["dapp.example"] }, + deniedSites: {}, + trackedTokens: [], + theme: "system", + ...(extra || {}), + }; +} + +function makeElement(id, className) { + const classes = new Set( + (className || "").split(/\s+/).filter((name) => name !== ""), + ); + const el = { + id, + tagName: "DIV", + textContent: "", + value: "", + innerHTML: "", + href: "", + download: "", + disabled: false, + style: {}, + dataset: {}, + listeners: {}, + clicked: 0, + classList: { + add: (...names) => names.forEach((n) => classes.add(n)), + remove: (...names) => names.forEach((n) => classes.delete(n)), + contains: (n) => classes.has(n), + toggle: (n, force) => { + const on = force === undefined ? !classes.has(n) : force; + if (on) classes.add(n); + else classes.delete(n); + return on; + }, + }, + addEventListener: (name, fn) => { + el.listeners[name] = el.listeners[name] || []; + el.listeners[name].push(fn); + }, + removeEventListener: () => {}, + appendChild: () => {}, + remove: () => {}, + focus: () => {}, + select: () => {}, + setAttribute: (name, value) => { + el[name] = value; + }, + querySelector: () => null, + querySelectorAll: () => [], + // The Receive view draws its QR onto #receive-qr through the qrcode + // package, which calls getContext("2d") and then createImageData/ + // putImageData on the result. Without this the render throws from + // inside a promise the view does not await, which takes the whole node + // process down rather than failing a test — so a suite that boots onto + // Receive could not report anything. + getContext: () => ({ + createImageData: (w, h) => ({ + width: w, + height: h, + data: new Uint8ClampedArray(w * h * 4), + }), + putImageData: () => {}, + clearRect: () => {}, + }), + click: () => { + el.clicked += 1; + }, + }; + // src/ reaches parentElement only to hide or unhide the wrapper a field + // sits in (txStatus.js renderSuccess(), transactionDetail.js render()). + // The stub is flat — it is built from the ids in the markup, not from its + // tree — so each element gets a wrapper of its own, made on demand so this + // does not recurse. It is never registered by id, so nothing can mistake + // it for a view. Without it, success-tx and transaction throw on the first + // line that touches a wrapper and cannot be booted onto at all. + let parent = null; + Object.defineProperty(el, "parentElement", { + get() { + if (!parent) parent = makeElement(id + "-parent", ""); + return parent; + }, + }); + return el; +} + +// Every id in the markup, with the classes the markup gives it. A view the +// popup is supposed to reveal has to exist here, which means it has to exist +// in src/popup/index.html. +function idsFromHtml(html) { + const out = new Map(); + const tags = html.match(/<[a-zA-Z][^>]*>/g) || []; + for (const tag of tags) { + const id = /\bid="([^"]+)"/.exec(tag); + if (!id) continue; + const cls = /\bclass="([^"]*)"/.exec(tag); + out.set(id[1], cls ? cls[1] : ""); + } + return out; +} + +// Ids the popup creates at runtime rather than authoring in the markup, and +// that must therefore read as ABSENT until something creates them. Answering +// with a fresh element instead would make "is the banner up?" always true. +const RUNTIME_IDS = new Set(["debug-banner", "save-failure-banner"]); + +function makeDocument(html) { + const authored = idsFromHtml(html); + const els = new Map(); + for (const [id, className] of authored) { + els.set(id, makeElement(id, className)); + } + const created = []; + const prepended = []; + const doc = { + listeners: {}, + getElementById(id) { + if (RUNTIME_IDS.has(id) && !els.has(id)) return null; + if (!els.has(id)) els.set(id, makeElement(id, "")); + return els.get(id); + }, + createElement(tag) { + const el = makeElement("created-" + tag, ""); + el.tagName = String(tag).toUpperCase(); + created.push(el); + return el; + }, + addEventListener(name, fn) { + doc.listeners[name] = doc.listeners[name] || []; + doc.listeners[name].push(fn); + }, + querySelectorAll: () => [], + documentElement: makeElement("html", ""), + body: { + // Recorded, and registered under its id: a banner the popup + // prepends is on the page from then on, and a test asking for it + // by id has to find it. + prepend: (el) => { + prepended.push(el); + if (el && el.id) els.set(el.id, el); + }, + appendChild: () => {}, + removeChild: () => {}, + }, + elements: els, + authoredIds: authored, + created, + prepended, + }; + return doc; +} + +async function settle() { + for (let i = 0; i < 50; i++) await Promise.resolve(); +} + +/** + * Boot the popup over `stored`. + * + * @param {*} stored the record storage holds, or undefined for a first run. + * @param {object} [options] + * @param {object} [options.storage] a storage stub from makeStorageStub(), for + * a test that needs to make writes fail or to watch the round trips. + * @returns {Promise} handles onto the booted page. + */ +async function bootPopup(stored, options) { + jest.resetModules(); + + // The three modules that reach the network. None is on the path under + // test; all would make the suite hit the internet. + jest.doMock("../../src/shared/prices", () => ({ + prices: {}, + refreshPrices: jest.fn(async () => {}), + clearPrices: jest.fn(), + getPrice: () => null, + formatUsd: () => "", + formatAddressTotal: () => "", + getAddressValue: () => ({ usd: null, partial: false }), + getWalletValue: () => ({ usd: null, partial: false }), + getTotalValue: () => ({ usd: null, partial: false }), + })); + jest.doMock("../../src/shared/balances", () => ({ + fetchTokenBalances: jest.fn(async () => []), + refreshBalances: jest.fn(async () => {}), + lookupTokenInfo: jest.fn(async () => null), + getProvider: () => ({}), + scanForAddresses: jest.fn(async () => []), + })); + jest.doMock("../../src/shared/transactions", () => ({ + fetchRecentTransactions: jest.fn(async () => []), + filterTransactions: () => [], + })); + + const storage = + (options && options.storage) || + makeStorageStub(stored === undefined ? {} : { autistmask: stored }); + const document = makeDocument(POPUP_HTML); + const reloads = []; + + globalThis.chrome = { + storage: { local: storage.local }, + runtime: { + sendMessage: jest.fn(async () => ({})), + getURL: (p) => "chrome-extension://autistmask/" + p, + onMessage: { addListener: () => {} }, + }, + }; + globalThis.document = document; + globalThis.window = { + location: { + search: "", + href: "chrome-extension://autistmask/src/popup/index.html", + reload: () => reloads.push(Date.now()), + }, + matchMedia: () => ({ + matches: false, + addEventListener: () => {}, + removeEventListener: () => {}, + }), + addEventListener: () => {}, + }; + // The 10s refresh loop init() starts would outlive the test. + const realSetInterval = globalThis.setInterval; + globalThis.setInterval = () => 0; + + require("../../src/popup/index"); + + const booted = []; + for (const fn of document.listeners.DOMContentLoaded || []) { + booted.push(fn()); + } + + // What the browser console would have shown. A throw out of init() is the + // blank popup issue 311 is about, so it is captured rather than thrown: + // the assertion that matters is what ended up on screen. + const pageErrors = []; + for (const p of booted) { + try { + await p; + } catch (e) { + pageErrors.push(String((e && e.message) || e)); + } + } + await settle(); + + globalThis.setInterval = realSetInterval; + + return { + storage, + document, + pageErrors, + reloaded: () => reloads.length, + node: (id) => document.getElementById(id), + text: (id) => { + const el = document.getElementById(id); + return el ? el.textContent : null; + }, + value: (id) => document.getElementById(id).value, + hidden: (id) => + document.getElementById(id).classList.contains("hidden"), + click: async (id) => { + const el = document.getElementById(id); + const fns = el.listeners.click || []; + for (const fn of fns) await fn(); + await settle(); + }, + settle, + // The view ids whose section is not hidden, as the audit measured them. + visibleViews: () => { + const out = []; + for (const [id, el] of document.elements) { + if (!id.startsWith("view-")) continue; + if (!el.classList.contains("hidden")) out.push(id.slice(5)); + } + return out; + }, + }; +} + +function cleanupPopup() { + delete globalThis.chrome; + delete globalThis.document; + delete globalThis.window; +} + +module.exports = { + bootPopup, + cleanupPopup, + settle, + unversionedValidProfile, + ADDRESS, + TOKEN_ADDRESS, + POPUP_HTML, +};