diff --git a/README.md b/README.md index cce64ea..3cbf3d7 100644 --- a/README.md +++ b/README.md @@ -985,6 +985,55 @@ tokens with fewer than 1,000 holders" setting governs the transaction history and the send-screen token selector, not this list. Tracked tokens with a zero balance are listed as well while "Show tracked tokens with zero balance" is on. +#### Stored state and its version + +The whole profile lives under a single extension-storage key, `autistmask`, and +carries a `schemaVersion` — `STATE_SCHEMA_VERSION` in +`src/shared/stateSchema.js`, currently `1`. Every write stamps it: the popup's +`saveState()` and the background's `updateState()` both do, so whichever context +wrote last, the record says which build's shape it is in. + +Version 1 is the shape that shipped before versions existed, so a stored record +with no `schemaVersion` is version 1 rather than a defect: it loads normally and +is migrated in place by being stamped on the first write. An upgrade never shows +an existing user a warning about a profile that is perfectly good. The version +is bumped only when the MEANING of a stored field changes — a new field with a +sensible absent value is handled by `normalizePersisted()` and is not a bump, +because bumping for one would send every older install to StateRecovery for +nothing. + +Every read of the record goes through `assertStateUsable()` first, on the raw +bytes, before normalization: `loadState()` for the popup and `getState()` for +the background. It refuses a record that is not an object, a `schemaVersion` +this build does not understand (a newer one included), a `wallets` that is not a +list of wallet records with address records in them, and a `networkId` that is +not a network in `src/shared/networks.js`. Refusing is the whole point — a +record the wallet cannot vouch for is never normalized, never written back, and +never half-loaded. The popup shows StateRecovery; a dApp gets a specific error +(`-32007`, an EIP-1474 server-error code the spec leaves unassigned) saying the +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. + +The `networkId` check is not cosmetic: that value is an object KEY into +`state.networkEndpoints`, so an unvalidated `"__proto__"` would set the map's +prototype instead of an own key and the user's endpoint would silently not be +recorded. Every key test in the gate is an own-property test for that reason, +and `networkById()` throws on an id it does not know rather than quietly +answering mainnet. + #### Navigation The main view shows all addresses grouped by wallet, with ETH balances inline. @@ -1011,10 +1060,12 @@ Three elements sit outside the screens and are present on all of them: the title bar ("AutistMask by @sneak" plus the Settings gear), the flash message line under it, and the red banner at the very top that appears on a debug build, when runtime debug mode is on, or when the active network is a testnet. They are not -repeated in the element lists below. +repeated in the element lists below. StateRecovery is the one screen they are +not all present on: it hides the Settings gear, because it is shown precisely +when there is no profile for the screens behind that gear to render from. Closing and reopening the popup returns to the screen the user was last on only -for the views listed in `RESTORABLE_VIEWS` (`src/popup/restorableViews.js`). +for the views listed in `RESTORABLE_VIEWS` (`src/shared/restorableViews.js`). Every other screen falls back to Home. The screens that display a secret — ExportPrivKey and ShowRecoveryPhrase — are deliberately absent from that list, so the popup can never reopen onto one of them with no password prompt in front @@ -1684,6 +1735,48 @@ view would leave a wallet one click from deletion. - Popup window closed without answering → the request is rejected with EIP-1193 code 4001 +#### StateRecovery (`state-recovery`) + +- **When**: `loadState()` refused the stored profile, so the popup has no + profile at all. It is the only screen reached without one, and the only one + that never appears during ordinary use. +- **Why it exists**: a record the wallet cannot read used to render nothing — no + view, no message, no control — while every dApp call answered a generic + internal error, and no reset or wipe control existed anywhere in the product. + The only escape was clearing extension storage through browser internals + ([#311](https://git.eeqj.de/sneak/AutistMask/issues/311)). +- **Elements**: + - "Saved Data Cannot Be Read" heading, and a statement that nothing has been + changed or erased and nothing can be signed or sent + - The problem, in one sentence naming what is wrong with the record + - "Export Saved Data" button, and the read-only text box it fills + - What erasing does and does not do, in bold: every wallet stored in this + browser is deleted; nothing on chain changes and no money is moved + - A text input asking for `ERASE MY WALLET` to be typed back + - Error line + - "Erase Saved Data" button +- **Transitions**: + - "Export Saved Data" → the raw stored record, verbatim, in the text box on + the screen, and a downloaded `autistmask-saved-data.json` where the + browser allows one. The box is filled first and never depends on the + download: an export that can fail is not an export. + - "Erase Saved Data" (phrase typed) → the stored record is removed and the + popup reloads into **Welcome** + - "Erase Saved Data" (phrase not typed) → "Type ERASE MY WALLET to confirm. + Nothing was erased." on the error line +- **No other control is reachable.** The Settings gear is hidden while this + screen is up, because every screen behind it renders from the profile that + could not be read, and `showView()` is not used to raise it for the same + reason — it reads and writes the state singleton. +- **Both controls are required.** An export with no reset leaves the user + looking at a broken profile with no way to use the wallet again; a reset with + no export destroys the only copy of a record that may hold recoverable key + material. The typed phrase is the same barrier DeleteWalletLostPassword uses, + and for the same reason: there is no password to gate this with, since there + is no profile to check one against. +- Not in `RESTORABLE_VIEWS`: it is never persisted as the current view, because + nothing on this path writes state at all. + ### External Services AutistMask is not a fully self-contained offline tool. It necessarily diff --git a/TODO.md b/TODO.md index 2409815..f985496 100644 --- a/TODO.md +++ b/TODO.md @@ -57,6 +57,46 @@ but the review is broader than any of them. `Token Out` now reads `Unknown (not named in the calldata)` and `Min. received` falls to the base-unit refusal from [#340](https://git.eeqj.de/sneak/AutistMask/issues/340). + +- 2026-08-23: The stored profile carries a schema version, and a record the + wallet cannot read produces a screen instead of a blank popup + ([#311](https://git.eeqj.de/sneak/AutistMask/issues/311)). `saveState()` and + `updateState()` both stamp `STATE_SCHEMA_VERSION` + (`src/shared/stateSchema.js`), and every read goes through + `assertStateUsable()` on the raw bytes before normalization gets a chance to + paper over them. Version 1 is the shape that shipped unversioned, so the + profile every existing install holds loads normally and is migrated in place + by being stamped on the first write — an upgrade shows nobody a wipe prompt + for a wallet that is fine. A record this build cannot vouch for is refused + instead: not normalized, not written back, not half-loaded. The popup shows + the new StateRecovery screen, which names the problem, exports the raw record + verbatim into the page (and downloads it where the browser allows), and offers + an erase behind a typed `ERASE MY WALLET` — both controls, because an export + with no reset leaves the user stuck and a reset with no export destroys the + only copy of possibly recoverable key material. The background refuses the + same record and answers dApps `-32007` — a code EIP-1474 leaves unassigned, + unlike `-32000`..`-32006` — with a message saying the saved data cannot be + read and that nothing was signed or sent, rather than the generic `-32603` + that every request used to get. Fields the gate deliberately does not check + produced the same blank popup on their own: `trackedTokens` and + `activeAddress` were floored on truthiness rather than on type, and + `trackedTokens`' ENTRIES and each address's `tokenBalances` were not floored + at all — `[1, 2]` is a list, and the dereference is `t.address.toLowerCase()` + one level below the container. All of them are type-checked now, entries + included, and the header of `src/shared/stateSchema.js` lists which fields of + the record get a type check and which get a `saved.x || default` or a verbatim + passthrough, rather than asserting a rule the module does not follow. + `networkById()` now throws on an id it does not know instead of quietly + answering mainnet, and the gate's key tests are all own-property tests: + `networkId` is an object key into `networkEndpoints`, so an unvalidated + `"__proto__"` used to set that map's prototype and drop the user's endpoint + silently. The three corrupt blobs from the issue drive the real popup entry + point in `tests/stateRecovery.test.js` and the real worker in + `tests/stateUnusableRpc.test.js`; each rendered nothing at all and answered + `-32603` before this. `src/popup/restorableViews.js` moved to + `src/shared/restorableViews.js`, since `persistedState.js` requires it and + that module is in the background bundle. + - 2026-08-23: The background no longer reads or writes the shared `state` singleton ([#324](https://git.eeqj.de/sneak/AutistMask/issues/324)), which also closes the cold-worker wrong-chain send diff --git a/src/background/index.js b/src/background/index.js index 18af9a2..e893024 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -16,6 +16,7 @@ const { applyChainSwitchFields } = require("../shared/chainSwitchFields"); // script/lib/forbiddenBundleInputs.js). The ESLint rule of the same name is // the same prohibition reported early, not the guarantee. const { getState, updateState } = require("./state"); +const { StateUnusableError } = require("../shared/stateSchema"); const { refreshBalances, getProvider } = require("../shared/balances"); const { debugFetch, log } = require("../shared/log"); const { @@ -179,6 +180,45 @@ const INTERNAL_ERROR_CODE = -32603; const INTERNAL_ERROR_MESSAGE = "AutistMask could not complete this request because of an internal error."; +// What the page is told when the wallet's own stored profile cannot be read. +// +// This used to be the generic answer above: getActiveAddress() dereferenced +// the stored wallet list on nearly every method, so a corrupt or +// newer-than-this-build record turned EVERY request from EVERY page into +// "internal error", which is also what a failed signing attempt answers. The +// page cannot tell those apart, and the user is told nothing about the one +// thing that is actually wrong or where to fix it +// (https://git.eeqj.de/sneak/AutistMask/issues/311). +// +// Its own code rather than -32603, because the condition is specific, +// diagnosable and has a user action attached — none of which "internal error" +// conveys. +// +// -32007 specifically: EIP-1474 sets aside -32000..-32099 for +// implementation-defined server errors, but it ASSIGNS meanings to -32000 +// through -32006 (Invalid input, Resource not found, Resource unavailable, +// Transaction rejected, Method not supported, Limit exceeded, JSON-RPC version +// not supported). -32007..-32099 are the unassigned ones, and this condition +// is not any of the seven. Nothing above it is free to be overloaded either: +// this wallet already answers EIP-1474's -32002 "Resource unavailable" for a +// pending approval, the conventional way, so a page is entitled to read these +// codes by that table. +const STATE_UNUSABLE_CODE = -32007; +const STATE_UNUSABLE_MESSAGE = + "AutistMask cannot read its saved data, so nothing was signed or sent." + + " Open the AutistMask extension to export or reset it."; + +// The EIP-1193 error for a handler that threw, by cause. Everything that +// consults the profile goes through getState(), so this one mapping covers +// every method rather than each one having to know about the condition. +function failureError(err) { + if (err instanceof StateUnusableError) { + log.errorf("state is unusable:", err.problem); + return { code: STATE_UNUSABLE_CODE, message: STATE_UNUSABLE_MESSAGE }; + } + return { code: INTERNAL_ERROR_CODE, message: INTERNAL_ERROR_MESSAGE }; +} + // The active address of a profile snapshot. Pure, and taking the snapshot as // an argument rather than reading storage itself: a handler that has already // read state must not answer "which account is this" from a SECOND, later read @@ -883,6 +923,11 @@ async function handleRpc(method, params, origin) { const result = await proxyRpc(method, params); return { result }; } catch (e) { + // A node that answered with an error is reported as itself. The + // wallet being unable to read its own profile is not that, and it + // must not be flattened into a message with no code: it goes back + // to the dispatcher, which has the one answer for it. + if (e instanceof StateUnusableError) throw e; return { error: { message: e.message } }; } } @@ -1142,7 +1187,14 @@ async function backgroundRefresh() { // module-level state does not outlive it. Alarms are held by the browser and // wake the worker to deliver them. registerAlarmHandlers({ - [BALANCE_REFRESH_ALARM]: backgroundRefresh, + // Caught here rather than left to the alarm dispatcher, which does not + // await what it calls: a profile this build cannot read makes every + // refresh throw, and an unhandled rejection per alarm tick says less than + // one logged line per tick does. + [BALANCE_REFRESH_ALARM]: () => + backgroundRefresh().catch((e) => { + log.errorf("background balance refresh failed:", e); + }), }); // Everything the background context needs re-established on start. This runs @@ -1241,12 +1293,7 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { // "it does not throw today" is not a property anyone is // maintaining. log.errorf("RPC request failed:", msg.method, err); - sendResponse({ - error: { - code: INTERNAL_ERROR_CODE, - message: INTERNAL_ERROR_MESSAGE, - }, - }); + sendResponse({ error: failureError(err) }); }); return true; } @@ -1481,18 +1528,10 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { // the popup nor the page is ever answered. Settle both, through // the same chokepoint as every other retirement. log.errorf("transaction approval response failed:", e); - settleApproval( - msg.id, - { - error: { - code: INTERNAL_ERROR_CODE, - message: INTERNAL_ERROR_MESSAGE, - }, - }, - { holdsClaim: true }, - ); + const failure = failureError(e); + settleApproval(msg.id, { error: failure }, { holdsClaim: true }); sendResponse({ - error: INTERNAL_ERROR_MESSAGE, + error: failure.message, retryable: false, stage: lastResortStage, }); @@ -1584,18 +1623,10 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { // Same shape as the transaction path: a throw out of the catch // block above would leave the popup and the page both waiting. log.errorf("sign approval response failed:", e); - settleApproval( - msg.id, - { - error: { - code: INTERNAL_ERROR_CODE, - message: INTERNAL_ERROR_MESSAGE, - }, - }, - { holdsClaim: true }, - ); + const failure = failureError(e); + settleApproval(msg.id, { error: failure }, { holdsClaim: true }); sendResponse({ - error: INTERNAL_ERROR_MESSAGE, + error: failure.message, retryable: false, }); }); diff --git a/src/background/state.js b/src/background/state.js index cd66837..45f9f81 100644 --- a/src/background/state.js +++ b/src/background/state.js @@ -28,14 +28,26 @@ const { storageGet, storageSet } = require("../shared/browserApi"); const { normalizePersisted } = require("../shared/persistedState"); +const { + STATE_SCHEMA_VERSION, + assertStateUsable, +} = require("../shared/stateSchema"); // A fresh, fully-normalized, detached copy of the persisted profile. // // Normalized rather than raw: a legacy or malformed record is self-healed the // same way loadState() heals it for the popup, so the background is never the // one context reasoning about a shape the rest of the extension repairs. +// +// Throws StateUnusableError for a record this build cannot make sense of, +// before normalization gets a chance to paper over it — the same gate, in the +// same place, as the popup's loadState(). Every handler that consults the +// profile comes through here, so a dApp call against such a record is answered +// with the specific error the dispatcher maps that to (src/background/index.js) +// rather than dereferencing its way into a generic -32603. async function getState() { const result = await storageGet("autistmask"); + assertStateUsable(result.autistmask); return normalizePersisted(result.autistmask); } @@ -48,6 +60,10 @@ async function updateStateOnce(mutate) { const s = await getState(); await mutate(s); s.hasWallet = Boolean(s.wallets && s.wallets.length > 0); + // Stamped on every write, exactly as the popup's saveState() stamps it: + // whichever context writes last, the record in storage is in this build's + // shape and says so. + s.schemaVersion = STATE_SCHEMA_VERSION; await storageSet({ autistmask: s }); return s; } diff --git a/src/popup/index.html b/src/popup/index.html index 74d11fa..ed99df5 100644 --- a/src/popup/index.html +++ b/src/popup/index.html @@ -1761,6 +1761,75 @@ + + + + diff --git a/src/popup/index.js b/src/popup/index.js index 671c12d..1a7e01e 100644 --- a/src/popup/index.js +++ b/src/popup/index.js @@ -2,6 +2,7 @@ // Loads state, initializes views, triggers first render. const { state, saveState, loadState } = require("../shared/state"); +const { StateUnusableError } = require("../shared/stateSchema"); const { setRuntimeDebug } = require("../shared/log"); const { refreshPrices } = require("../shared/prices"); const { refreshBalances } = require("../shared/balances"); @@ -16,7 +17,7 @@ const { const { applyTheme } = require("./theme"); // Renders a view the popup lands on without having navigated to it forward: // on restore here, and on Back. Only the views that can be fully re-rendered -// from persisted state (RESTORABLE_VIEWS, src/popup/restorableViews.js) go +// from persisted state (RESTORABLE_VIEWS, src/shared/restorableViews.js) go // through it; anything else falls back to the nearest restorable parent. const { renderView, makeBackRenderer } = require("./viewRouter"); @@ -35,6 +36,7 @@ const settings = require("./views/settings"); const settingsAddToken = require("./views/settingsAddToken"); const deleteAddress = require("./views/deleteAddress"); const approval = require("./views/approval"); +const stateRecovery = require("./views/stateRecovery"); function renderWalletList() { home.render(ctx); @@ -134,7 +136,22 @@ function fallbackView() { } async function init() { - await loadState(); + try { + await loadState(); + } catch (e) { + // A profile this build cannot read is the one failure that must not + // fall through to the rest of init(). It used to: the load "succeeded" + // on a record nothing had validated, and the first dereference below + // threw, leaving a popup with no view, no message and no control on + // it, and no way out of the wallet from inside the product + // (https://git.eeqj.de/sneak/AutistMask/issues/311). Now the load + // refuses, and this is the screen that says so. + if (e instanceof StateUnusableError) { + stateRecovery.show(e); + return; + } + throw e; + } applyTheme(state.theme); // Sync runtime debug flag from persisted state before first render diff --git a/src/popup/viewRouter.js b/src/popup/viewRouter.js index b37d53f..bcfbf35 100644 --- a/src/popup/viewRouter.js +++ b/src/popup/viewRouter.js @@ -12,7 +12,7 @@ // dispatch and its data guards can be tested directly; src/popup/index.js // cannot be required outside a browser. -const { RESTORABLE_VIEWS } = require("./restorableViews"); +const { RESTORABLE_VIEWS } = require("../shared/restorableViews"); // The views this page load has rendered. // diff --git a/src/popup/views/helpers.js b/src/popup/views/helpers.js index a959204..87bc357 100644 --- a/src/popup/views/helpers.js +++ b/src/popup/views/helpers.js @@ -45,6 +45,11 @@ const VIEWS = [ "approve-sign", "export-privkey", "show-phrase", + // Shown by src/popup/views/stateRecovery.js when the stored profile + // cannot be read. It is never reached through showView() — by then the + // state singleton this file writes on every navigation refuses to be read + // — but it is listed so that every view-hiding loop covers it. + "state-recovery", ]; // Cleanup callbacks for views that hold a secret in the DOM. The view diff --git a/src/popup/views/stateRecovery.js b/src/popup/views/stateRecovery.js new file mode 100644 index 0000000..b32f26f --- /dev/null +++ b/src/popup/views/stateRecovery.js @@ -0,0 +1,197 @@ +// The screen the popup shows when it cannot read the stored profile. +// +// Everything else in the popup assumes a loaded profile: showView() reads and +// writes the state singleton, every view renders from it, and the Settings +// gear leads to a screen that does both. None of that is available here — by +// the time this runs, loadState() has REFUSED, deliberately, and reading the +// singleton throws (https://git.eeqj.de/sneak/AutistMask/issues/311). +// +// So this module talks to the DOM directly and touches no state at all. It is +// the one screen that must work when nothing else can, which is also why it +// takes no ctx and needs no init(): whatever the rest of the popup did or did +// not manage to wire up, this shows. +// +// Two controls, and both are required. An export with no reset leaves the user +// looking at their broken profile with no way to use the wallet again; a reset +// with no export destroys the only copy of a record that may hold key material +// a later build could read. So the export is offered first, in the page where +// it cannot fail, and the reset is behind a typed confirmation. + +// $ and VIEWS only: nothing else in helpers is safe here, since showView() and +// everything under it read the state singleton. $ is taken from there rather +// than written again locally so that tests/popupElementIds.test.js sees these +// lookups and holds every id below against the markup. +const { $, VIEWS } = require("./helpers"); +const { storageGet, storageRemove } = require("../../shared/browserApi"); +const { log } = require("../../shared/log"); + +// Typed in full before anything is erased, in the same spirit as the wallet +// name on DeleteWalletLostPassword: this button destroys key material and +// there is no password in front of it, because there is no profile to check a +// password against. Compared case-insensitively — the phrase is the barrier, +// not the shift key. +const RESET_PHRASE = "ERASE MY WALLET"; + +let wired = false; + +function setFlash(message) { + const node = $("state-recovery-flash"); + node.textContent = message; + node.style.visibility = message ? "visible" : "hidden"; +} + +// The stored record exactly as storage hands it back, however malformed, with +// no normalization, no defaulting and no repair on it: this is evidence, and +// the point of the export is that a later build (or a human) sees what is +// actually there. It is not the raw bytes — storage deserializes, and +// exportRecord() re-serializes with JSON.stringify — so a value JSON cannot +// represent is the one thing that does not survive the trip. See there. +async function rawRecord() { + const result = await storageGet("autistmask"); + return result.autistmask; +} + +// Best effort, and never the only route. A download from an extension popup +// depends on the browser, the popup staying open long enough, and the +// extension's content security policy; the textarea below depends on none of +// those, and is filled first. +function offerDownload(text) { + try { + if ( + typeof Blob !== "function" || + typeof URL === "undefined" || + typeof URL.createObjectURL !== "function" + ) { + return false; + } + const url = URL.createObjectURL( + new Blob([text], { type: "application/json" }), + ); + const link = document.createElement("a"); + link.href = url; + link.download = "autistmask-saved-data.json"; + link.click(); + // Revoked in a later task, not in this one: the download is started + // from the click and revoking the URL in the same turn can cancel it + // before it has been read. If the popup closes first the URL dies with + // the document anyway. + if (typeof URL.revokeObjectURL === "function") { + setTimeout(() => URL.revokeObjectURL(url), 0); + } + return true; + } catch (e) { + log.errorf("state recovery: download failed:", e); + return false; + } +} + +// Residual, stated rather than left to be discovered: structured-clone storage +// holds values JSON does not have, and no build here writes one, but the export +// is a funds-recovery path and what it cannot carry has to be written down. +// +// Loud: JSON.stringify THROWS on a reference cycle or a BigInt. That lands in +// the catch below, so the export fails entirely and erase is the only control +// left on the screen. +// +// Silent, and the worse of the two, because the box then looks complete: +// a Date becomes its ISO string, a Map or a Set becomes {}, a property whose +// value is undefined is dropped from the output entirely, and NaN and +// ±Infinity become null. Nothing here can serialize any of it faithfully; +// recovering such a record needs the browser's own storage inspector. +async function exportRecord() { + let text; + try { + const record = await rawRecord(); + text = JSON.stringify(record === undefined ? null : record, null, 2); + } catch (e) { + log.errorf("state recovery: export failed:", e); + setFlash( + "The saved data could not be read out of storage. Nothing has" + + " been changed.", + ); + return; + } + // JSON.stringify answers undefined for a value it cannot represent, and + // an empty box would read as "there was nothing there". + if (typeof text !== "string") text = String(text); + + const box = $("state-recovery-blob"); + box.value = text; + box.classList.remove("hidden"); + const downloaded = offerDownload(text); + setFlash( + downloaded + ? "Saved data downloaded, and shown below. Keep a copy before" + + " erasing anything." + : "Saved data shown below. Copy it and keep it before erasing" + + " anything.", + ); +} + +async function resetProfile() { + const typed = $("state-recovery-reset-input").value || ""; + if (typed.trim().toUpperCase() !== RESET_PHRASE) { + setFlash("Type " + RESET_PHRASE + " to confirm. Nothing was erased."); + return; + } + try { + await storageRemove("autistmask"); + } catch (e) { + log.errorf("state recovery: reset failed:", e); + setFlash("The saved data could not be erased. Nothing was changed."); + return; + } + setFlash("Saved data erased. AutistMask is starting fresh."); + // Back to a first run, which is what the wallet now is. A popup that + // cannot reload says so rather than sitting on a screen describing a + // profile that no longer exists. + if ( + typeof window !== "undefined" && + window.location && + typeof window.location.reload === "function" + ) { + window.location.reload(); + return; + } + setFlash("Saved data erased. Close and reopen AutistMask."); +} + +function wire() { + if (wired) return; + wired = true; + $("btn-state-recovery-export").addEventListener("click", exportRecord); + $("btn-state-recovery-reset").addEventListener("click", resetProfile); +} + +/** + * Show the recovery screen, naming `problem`. + * + * @param {Error|string} problem the StateUnusableError from the read that + * refused, or its sentence. + */ +function show(problem) { + const sentence = + (problem && (problem.problem || problem.message)) || String(problem); + + // Not showView(): that reads and writes the singleton this screen exists + // because nothing could load. + for (const view of VIEWS) { + const node = document.getElementById("view-" + view); + if (node) node.classList.add("hidden"); + } + // The one global control, and it leads to a screen that renders from the + // profile. There is nowhere to go from here but out. + const gear = $("btn-settings"); + if (gear) gear.classList.add("hidden"); + + $("state-recovery-problem").textContent = sentence; + $("state-recovery-blob").value = ""; + $("state-recovery-blob").classList.add("hidden"); + $("state-recovery-reset-input").value = ""; + setFlash(""); + wire(); + $("view-state-recovery").classList.remove("hidden"); + log.errorf("state is unusable, showing the recovery screen:", sentence); +} + +module.exports = { show, RESET_PHRASE }; diff --git a/src/shared/browserApi.js b/src/shared/browserApi.js index 339ccfc..c512286 100644 --- a/src/shared/browserApi.js +++ b/src/shared/browserApi.js @@ -194,6 +194,23 @@ function storageSet(items) { return Promise.resolve(storage.set(items)); } +/** + * Erase stored keys. The one caller is the destructive reset on the recovery + * screen (src/popup/views/stateRecovery.js), which is the only way out of a + * profile no build can read; it rejects rather than defaulting for the same + * reason the two above do — a reset that silently did nothing would leave the + * user in the dead end they were promised an exit from. + * + * @param {string|string[]} keys + * @returns {Promise} + * @throws rejects where `storage.local` is absent. + */ +function storageRemove(keys) { + const storage = storageLocal(); + if (!storage) return storageUnavailable("remove"); + return Promise.resolve(storage.remove(keys)); +} + /** * @param {Object} queryInfo * @returns {Promise} the matching tabs. @@ -251,6 +268,7 @@ module.exports = { sendMessage, storageGet, storageLocal, + storageRemove, storageSet, tabsApi, tabsQuery, diff --git a/src/shared/networks.js b/src/shared/networks.js index 4550590..fcc486f 100644 --- a/src/shared/networks.js +++ b/src/shared/networks.js @@ -31,8 +31,42 @@ const SUPPORTED_CHAIN_IDS = new Set( Object.values(NETWORKS).map((n) => n.chainId), ); +// Thrown rather than defaulted. An id this build does not know used to answer +// with MAINNET, so a stored `{networkId:"base"}` rendered the selector as +// Ethereum Mainnet with no banner and answered eth_chainId 0x1, while rpcUrl +// still pointed at Base — the wallet telling the user and the page one chain +// while transacting on another. Nothing in this codebase has an unknown id to +// offer: stored state is validated against this table before it is loaded +// (src/shared/stateSchema.js), and every other caller passes an id it took +// from here. So an unknown id is a defect, and it says so, the same way +// getProvider() (src/shared/balances.js) already refuses one. +class UnknownNetworkError extends Error { + constructor(id) { + super( + "AutistMask does not know the network " + + JSON.stringify(id) + + "; it supports " + + Object.keys(NETWORKS).join(", "), + ); + this.name = "UnknownNetworkError"; + this.networkId = id; + } +} + +// Own properties only: NETWORKS inherits from Object.prototype, so +// NETWORKS["constructor"] and NETWORKS["__proto__"] both answer with something +// truthy that is not a network. A stored id is untrusted input, and this is +// the test the validator uses to decide whether it may be adopted at all. +function isKnownNetworkId(id) { + return ( + typeof id === "string" && + Object.prototype.hasOwnProperty.call(NETWORKS, id) + ); +} + function networkById(id) { - return NETWORKS[id] || NETWORKS.mainnet; + if (!isKnownNetworkId(id)) throw new UnknownNetworkError(id); + return NETWORKS[id]; } function networkByChainId(chainId) { @@ -51,6 +85,8 @@ function explorerLink(network, type, value) { module.exports = { NETWORKS, SUPPORTED_CHAIN_IDS, + UnknownNetworkError, + isKnownNetworkId, networkById, networkByChainId, explorerLink, diff --git a/src/shared/persistedState.js b/src/shared/persistedState.js index 36a6d27..414cbb1 100644 --- a/src/shared/persistedState.js +++ b/src/shared/persistedState.js @@ -10,8 +10,14 @@ // reaching it anyway and being served DEFAULT_STATE. const { DEFAULT_RPC_URL, DEFAULT_BLOCKSCOUT_URL } = require("./constants"); -// Dependency-free constant module; safe to pull into a background bundle. -const { RESTORABLE_VIEWS } = require("../popup/restorableViews"); +const { isKnownNetworkId } = require("./networks"); +const { STATE_SCHEMA_VERSION } = require("./stateSchema"); +// Dependency-free constant module. It lives under src/shared/ rather than +// src/popup/ precisely because this module is in the background bundle: a +// popup-path module reached from the worker is the shape the prohibition in +// script/lib/forbiddenBundleInputs.js exists to keep out, even when the +// particular module is harmless. +const { RESTORABLE_VIEWS } = require("./restorableViews"); const DEFAULT_STATE = { hasWallet: false, @@ -46,7 +52,10 @@ const DEFAULT_STATE = { // Every field written to and read from the single "autistmask" storage key. // hasWallet is deliberately excluded from the diffing/merge logic in // state.js — like loadState() does, it is always derived from `wallets`, -// never carried as an independent value. +// never carried as an independent value. schemaVersion is excluded for the +// same reason and is absent from DEFAULT_STATE for it: it describes the +// record rather than being part of it, and every write stamps the current +// value rather than diffing whatever was read. const PERSISTED_FIELDS = Object.keys(DEFAULT_STATE) .filter((key) => key !== "hasWallet") .concat([ @@ -58,6 +67,31 @@ const PERSISTED_FIELDS = Object.keys(DEFAULT_STATE) "viewStack", ]); +function isRecord(value) { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +// A list of token references, as everything downstream dereferences them: +// `t.address.toLowerCase()`, with no guard of its own (src/shared/balances.js, +// src/popup/views/helpers.js, and every view that shows a balance line). +// +// Both the container AND the entries, because they are separate defects. A +// container check alone leaves a well-formed list of malformed entries walking +// through to a dereference one level below the check, which is the same blank +// popup: `[1, 2]` and `[{}]` are lists. +// +// A malformed entry is DROPPED rather than repaired: a token reference with no +// address identifies nothing, so there is no value to repair it to, and the +// alternative — refusing the whole record — sends a user whose wallets are +// perfectly readable to an export-or-erase screen over a token list. An entry +// that is a record with a text address is kept verbatim, extra fields and all. +function tokenRefs(value) { + if (!Array.isArray(value)) return []; + return value.filter( + (entry) => isRecord(entry) && typeof entry.address === "string", + ); +} + // 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 @@ -107,11 +141,52 @@ function restorableStack(stored, currentView) { function normalizePersisted(saved) { saved = saved || {}; const out = {}; + // Every write goes out at the current version. That IS the migration for + // the unversioned records every install in the field holds: version 1 is + // the shape that shipped unversioned, so a record that validated is + // carried forward simply by being stamped. A record this build does NOT + // understand never reaches here — assertStateUsable() refuses it on the + // read path first (src/shared/stateSchema.js). + out.schemaVersion = STATE_SCHEMA_VERSION; out.wallets = structuredClone(saved.wallets || []); // Derived, never trusted verbatim off storage — see loadState(). out.hasWallet = out.wallets.length > 0; - out.trackedTokens = structuredClone(saved.trackedTokens || []); - out.networkId = saved.networkId || DEFAULT_STATE.networkId; + // Each address's token holdings, floored to a list of token records on the + // detached copy above. Every reader iterates it behind a `|| []` that only + // covers an ABSENT value, and then dereferences `t.address.toLowerCase()` + // and `t.balance` — so a stored string iterates as characters, a number + // throws on the iterator, and a null entry throws on the field. + // + // This field specifically, because refreshBalances() writes it WHOLESALE + // rather than merging into it: a write that only partly lands is the live + // cause https://git.eeqj.de/sneak/AutistMask/issues/311 names, and this is + // where it lands. The wallet list itself is the gate's (stateSchema.js); + // what is below an address record is not, and gets floored here. + if (Array.isArray(out.wallets)) { + for (const wallet of out.wallets) { + if (!isRecord(wallet) || !Array.isArray(wallet.addresses)) continue; + for (const addr of wallet.addresses) { + if (!isRecord(addr)) continue; + addr.tokenBalances = tokenRefs(addr.tokenBalances); + } + } + } + // An actual list of token records is required, not merely a truthy value + // and not merely a list: everything downstream iterates this and + // dereferences `token.address`, so a stored string or object walks through + // a `|| []`, and a list of numbers walks through an Array.isArray(), and + // both throw on the first read — the blank popup from the issue, for a + // profile whose wallets are perfectly fine. An empty list is a legitimate + // value and survives. + out.trackedTokens = structuredClone(tokenRefs(saved.trackedTokens)); + // The loud refusal for an unknown id is assertStateUsable(); this is the + // floor under it. networkId is an object KEY into networkEndpoints below, + // so a value that is not a network in networks.js must never get that far + // — "__proto__" would set the map's prototype instead of an own key, and + // the user's endpoint would silently not be recorded. + out.networkId = isKnownNetworkId(saved.networkId) + ? saved.networkId + : DEFAULT_STATE.networkId; out.rpcUrl = saved.rpcUrl || DEFAULT_STATE.rpcUrl; out.blockscoutUrl = saved.blockscoutUrl || DEFAULT_STATE.blockscoutUrl; // An actual object is required, not merely a truthy non-array: the code @@ -127,7 +202,19 @@ function normalizePersisted(saved) { : {}; out.networkEndpoints = {}; for (const netId of Object.keys(rawEndpoints)) { - out.networkEndpoints[netId] = { ...rawEndpoints[netId] }; + // 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, + }); } // A profile written before this map existed carries exactly one pair of // endpoints, belonging to whatever network it was last on. Adopt it as @@ -140,7 +227,18 @@ function normalizePersisted(saved) { }; } out.lastBalanceRefresh = saved.lastBalanceRefresh || 0; - out.activeAddress = saved.activeAddress || null; + // A non-empty address, or null, never anything else: this is passed to + // address.slice() and compared against stored addresses, so a stored + // number or object walks through a `|| null` and throws on the first + // render. The empty string is text but it is not an address, and it must + // become null rather than survive: init() auto-selects the first address + // only on a STRICT null, so a stored "" would leave the popup with no + // address ever selected. Nothing in src/ writes one, and this keeps the + // behaviour the `|| null` this check replaced already had. + out.activeAddress = + typeof saved.activeAddress === "string" && saved.activeAddress !== "" + ? saved.activeAddress + : null; out.allowedSites = saved.allowedSites && !Array.isArray(saved.allowedSites) ? structuredClone(saved.allowedSites) diff --git a/src/popup/restorableViews.js b/src/shared/restorableViews.js similarity index 79% rename from src/popup/restorableViews.js rename to src/shared/restorableViews.js index f6053ad..6cf031a 100644 --- a/src/popup/restorableViews.js +++ b/src/shared/restorableViews.js @@ -17,7 +17,13 @@ // // Kept in its own module, with no dependencies, so tests can assert the // exclusion directly rather than trusting a reading of the popup entry -// point, which cannot be required outside a browser. +// point. +// +// It sits under src/shared/ rather than src/popup/ because +// src/shared/persistedState.js needs it and that module is in the BACKGROUND +// bundle: a popup-path module reached from the worker is the shape +// script/lib/forbiddenBundleInputs.js exists to keep out, whether or not the +// particular module is harmless. const RESTORABLE_VIEWS = new Set([ "main", "address", diff --git a/src/shared/state.js b/src/shared/state.js index 1651c26..08442c6 100644 --- a/src/shared/state.js +++ b/src/shared/state.js @@ -29,6 +29,12 @@ const { normalizePersisted, } = require("./persistedState"); +const { + STATE_SCHEMA_VERSION, + assertStateUsable, + migrationNeeded, +} = require("./stateSchema"); + const { storageGet, storageSet } = require("./browserApi"); const { log } = require("./log"); @@ -446,6 +452,14 @@ function mergeNetworkEndpoints(base, ours, theirs) { async function saveStateOnce() { const current = snapshotPersisted(); const result = await storageGet("autistmask"); + // The record in storage right now is about to be merged into and written + // back, so it is validated exactly like a load validates it. Without this, + // a page whose own load succeeded would normalize a record it does not + // understand — one a NEWER build wrote in the meantime, say — and write + // the result back over it, destroying the only copy of whatever that + // record held. Refusing is louder than that and loses nothing: the live + // state is untouched and the next save retries. + assertStateUsable(result.autistmask); // Normalized, not raw: a field this page did not change still has to // come from storage in its loaded (self-healed) shape. See // normalizePersisted() in persistedState.js. @@ -481,6 +495,10 @@ async function saveStateOnce() { } } merged.hasWallet = Boolean(merged.wallets && merged.wallets.length > 0); + // Stamped on every write, never merged or diffed: the record that goes to + // storage is in THIS build's shape whatever shape it was read in, which is + // what migrates the unversioned records every install in the field holds. + merged.schemaVersion = STATE_SCHEMA_VERSION; await storageSet({ autistmask: merged }); @@ -512,8 +530,24 @@ function saveState() { return turn; } +// Rejects with StateUnusableError for a stored record this build cannot make +// sense of. Nothing is assigned and `loaded` stays false in that case, so a +// caller that ignores the rejection gets StateNotLoadedError on the first +// read rather than a half-populated profile. The caller that does NOT ignore +// it is the popup entry point, which shows the recovery screen +// (src/popup/views/stateRecovery.js) instead of proceeding. async function loadState() { const result = await storageGet("autistmask"); + // Before normalization, on the raw bytes: normalizing first would paper + // over the very shapes this refuses, which is how a corrupt record used to + // reach the popup and blank it (issue #311). + assertStateUsable(result.autistmask); + if (migrationNeeded(result.autistmask)) { + log.infof( + "state: migrating an unversioned profile to schema version", + STATE_SCHEMA_VERSION, + ); + } if (result.autistmask) { Object.assign(rawState, normalizePersisted(result.autistmask)); } diff --git a/src/shared/stateSchema.js b/src/shared/stateSchema.js new file mode 100644 index 0000000..fb12cbc --- /dev/null +++ b/src/shared/stateSchema.js @@ -0,0 +1,265 @@ +// The version stamped on the stored profile, and the shape check every read +// of one goes through. +// +// Storage is the one input to this extension that nobody validated. A profile +// carried no version at all, so there was no way to tell a record this build +// understands from one a later build wrote, and loadState() coerced scalars +// while trusting the structure — so a `wallets` that was a string, or an array +// of nulls, or a later schema's wallet records, reached the popup and threw on +// the first dereference. The popup rendered NOTHING: no view, no message, no +// control, and no way out from inside the product +// (https://git.eeqj.de/sneak/AutistMask/issues/311). +// +// Two separate jobs, deliberately not merged: +// +// stateProblem() / assertStateUsable() refuse a record this build cannot +// safely reason about, loudly, naming the problem in a +// sentence that goes on screen. This is the gate. +// normalizePersisted() (persistedState.js) self-heal a record that IS +// usable: absent fields, legacy shapes, out-of-range flags. +// +// The gate runs FIRST, on the raw stored bytes, before normalization has a +// chance to paper over a record whose meaning nobody can vouch for. A blob +// that fails it is left in storage untouched — it is the user's only copy of +// whatever it holds, and the recovery screen exports it before offering to +// erase it. +// +// 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: +// +// 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(). +// 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. +// +// 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. + +const { isKnownNetworkId } = require("./networks"); + +// Bump this when the MEANING of a stored field changes, and add the migration +// that carries the older version forward. Adding a field with a defaulted +// absent value is not a bump: normalizePersisted() already handles that, and +// bumping for it would send every older install to the recovery screen for no +// reason. +// +// Version 1 is the shape that shipped unversioned. An unversioned record is +// therefore version 1, not a defect — see migrationNeeded() below. +const STATE_SCHEMA_VERSION = 1; + +// Thrown by every read path that finds a record it cannot use. `problem` is +// the sentence shown to the user; `message` carries the same text so a log +// line or a rethrow is not empty. +class StateUnusableError extends Error { + constructor(problem) { + super(problem); + this.name = "StateUnusableError"; + this.problem = problem; + } +} + +function isPlainObject(value) { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +// Own properties only, everywhere in this file. `saved` comes from storage as +// parsed JSON, so `saved.constructor` and `saved.__proto__` answer from the +// prototype chain for a record that carries neither — a check written as a +// plain truthiness test can be satisfied by Object.prototype rather than by +// anything the user's profile actually contains. +function has(obj, key) { + return Object.prototype.hasOwnProperty.call(obj, key); +} + +function ordinal(index) { + return String(index + 1); +} + +function describeType(value) { + if (value === null) return "null"; + if (Array.isArray(value)) return "a list"; + return "a " + typeof value; +} + +// One address record, as every screen dereferences it. +function addressProblem(addr, walletIndex, addrIndex) { + const where = + "address " + + ordinal(addrIndex) + + " of wallet " + + ordinal(walletIndex) + + " in the saved data"; + if (!isPlainObject(addr)) { + return "The " + where + " is " + describeType(addr) + ", not a record."; + } + if (typeof addr.address !== "string" || addr.address === "") { + return "The " + where + " has no address."; + } + return null; +} + +function walletProblem(wallet, index) { + const where = "Wallet " + ordinal(index) + " in the saved data"; + if (!isPlainObject(wallet)) { + return where + " is " + describeType(wallet) + ", not a wallet record."; + } + if (!Array.isArray(wallet.addresses)) { + return where + " has no list of addresses."; + } + if (has(wallet, "name") && typeof wallet.name !== "string") { + return where + " has a name that is not text."; + } + for (let i = 0; i < wallet.addresses.length; i++) { + const problem = addressProblem(wallet.addresses[i], index, i); + if (problem) return problem; + } + return null; +} + +function versionProblem(saved) { + // No version field at all is the shape every install in the field has: + // no build ever wrote one. It is version 1, and it is migrated in place. + if (!has(saved, "schemaVersion")) return null; + const version = saved.schemaVersion; + if ( + typeof version !== "number" || + !Number.isInteger(version) || + version < 1 + ) { + return ( + "The saved data carries a schema version AutistMask does not" + + " recognize (" + + JSON.stringify(version) + + ")." + ); + } + if (version > STATE_SCHEMA_VERSION) { + return ( + "The saved data was written by a newer version of AutistMask" + + " (schema version " + + version + + "; this build understands version " + + STATE_SCHEMA_VERSION + + ")." + ); + } + return null; +} + +/** + * The reason this build cannot use `saved`, as a sentence for the user, or + * null when it can. + * + * @param {*} saved the raw record from storage, or undefined for a fresh + * install. + * @returns {string|null} + */ +function stateProblem(saved) { + // Nothing stored is a first run, not a defect. + if (saved === undefined || saved === null) return null; + if (!isPlainObject(saved)) { + return ( + "The saved data is " + + describeType(saved) + + ", not the record AutistMask stores." + ); + } + + const version = versionProblem(saved); + if (version) return version; + + // Read once, from an OWN property or not at all, so that a polluted + // prototype cannot decide whether a profile is refused. Note that + // normalizePersisted() reads the same field plainly, and so WOULD consult + // the prototype chain: the two halves agree only because a record arriving + // from storage has been through structuredClone and always carries + // Object.prototype. Nothing reachable from storage can put them at odds, + // but a caller that hands either one a hand-built object with an unusual + // prototype is not covered by that. + const wallets = + has(saved, "wallets") && saved.wallets !== undefined + ? saved.wallets + : []; + if (!Array.isArray(wallets)) { + return ( + "The list of wallets in the saved data is " + + describeType(wallets) + + ", not a list." + ); + } + for (let i = 0; i < wallets.length; i++) { + const problem = walletProblem(wallets[i], i); + if (problem) return problem; + } + + // networkId is not merely displayed: it is an object KEY into + // state.networkEndpoints. A corrupt "__proto__" would set that map's + // prototype instead of an own key, so the user's endpoint would silently + // not be recorded and a switch away and back would return the public + // default. isKnownNetworkId() is an own-property test against the network + // table for exactly that reason. + if ( + has(saved, "networkId") && + saved.networkId !== undefined && + !isKnownNetworkId(saved.networkId) + ) { + return ( + "The saved data selects a network AutistMask does not know (" + + JSON.stringify(saved.networkId) + + ")." + ); + } + + return null; +} + +/** + * Refuse a record this build cannot use. + * + * @param {*} saved the raw record from storage. + * @throws {StateUnusableError} + */ +function assertStateUsable(saved) { + const problem = stateProblem(saved); + if (problem) throw new StateUnusableError(problem); +} + +/** + * Whether `saved` is a usable record written before versions existed, and so + * gets the current version stamped on it the next time anything writes. Purely + * informational — the migration itself is that stamp, since version 1 IS the + * unversioned shape. + * + * @param {*} saved + * @returns {boolean} + */ +function migrationNeeded(saved) { + return ( + isPlainObject(saved) && + !has(saved, "schemaVersion") && + stateProblem(saved) === null + ); +} + +module.exports = { + STATE_SCHEMA_VERSION, + StateUnusableError, + assertStateUsable, + migrationNeeded, + stateProblem, +}; diff --git a/tests/alarms.test.js b/tests/alarms.test.js index 759e961..dfdc61f 100644 --- a/tests/alarms.test.js +++ b/tests/alarms.test.js @@ -397,8 +397,23 @@ describe("balance refresh steady-state cadence", () => { return { autistmask: { hasWallet: true, + // A whole wallet record, not a bare address: a stored profile + // is validated against the schema on every read now + // (src/shared/stateSchema.js), and a wallet with no address + // list is one of the shapes that refuses to load. wallets: [ - { address: "0x0000000000000000000000000000000000000001" }, + { + name: "Wallet 1", + type: "hd", + addresses: [ + { + address: + "0x0000000000000000000000000000000000000001", + balance: "0", + tokenBalances: [], + }, + ], + }, ], lastBalanceRefresh: 0, }, diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index b67ba79..09f8b7e 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -175,8 +175,22 @@ function loadBackground(options) { } const persisted = { + // Address RECORDS, not bare address strings: a stored profile is + // validated against the schema on every read now + // (src/shared/stateSchema.js), and a bare string where a record + // belongs is one of the shapes that refuses to load. wallets: [ - { name: "Wallet 1", type: "hd", addresses: [signer.address] }, + { + name: "Wallet 1", + type: "hd", + addresses: [ + { + address: signer.address, + balance: "0", + tokenBalances: [], + }, + ], + }, ], networkId: "mainnet", rpcUrl: "https://rpc.invalid", diff --git a/tests/deleteWalletLostPassword.test.js b/tests/deleteWalletLostPassword.test.js index 8fa3d8e..88cc05f 100644 --- a/tests/deleteWalletLostPassword.test.js +++ b/tests/deleteWalletLostPassword.test.js @@ -35,7 +35,7 @@ jest.mock("../src/shared/vault", () => ({ decryptWithPassword: jest.fn(), })); -const { RESTORABLE_VIEWS } = require("../src/popup/restorableViews"); +const { RESTORABLE_VIEWS } = require("../src/shared/restorableViews"); const { makeStorageStub } = require("./support/storageStub"); const VIEW = "delete-wallet-lost-password"; diff --git a/tests/exportPrivkey.test.js b/tests/exportPrivkey.test.js index 516026b..06fef69 100644 --- a/tests/exportPrivkey.test.js +++ b/tests/exportPrivkey.test.js @@ -21,7 +21,7 @@ jest.mock("../src/shared/wallet", () => ({ getSignerForAddress: jest.fn(() => ({ privateKey: mockPrivateKey })), })); -const { RESTORABLE_VIEWS } = require("../src/popup/restorableViews"); +const { RESTORABLE_VIEWS } = require("../src/shared/restorableViews"); const VIEW = "export-privkey"; const PASSWORD = "correct horse battery"; diff --git a/tests/showPhrase.test.js b/tests/showPhrase.test.js index d7cf2dd..4a76499 100644 --- a/tests/showPhrase.test.js +++ b/tests/showPhrase.test.js @@ -12,7 +12,7 @@ const fs = require("fs"); const path = require("path"); const { walletHasRecoveryPhrase } = require("../src/shared/wallet"); -const { RESTORABLE_VIEWS } = require("../src/popup/restorableViews"); +const { RESTORABLE_VIEWS } = require("../src/shared/restorableViews"); const SHOW_PHRASE_VIEW = "show-phrase"; diff --git a/tests/state.test.js b/tests/state.test.js index 61c8ad4..878182f 100644 --- a/tests/state.test.js +++ b/tests/state.test.js @@ -1,7 +1,17 @@ const ADDRESS = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; +// Address RECORDS, not bare address strings. A stored profile is validated +// against the schema on every read now (src/shared/stateSchema.js), and a bare +// string where an address record belongs is one of the shapes that refuses to +// load — as it should, since every screen dereferences addr.address. function oneWallet() { - return [{ name: "Wallet 1", type: "hd", addresses: [ADDRESS] }]; + return [ + { + name: "Wallet 1", + type: "hd", + addresses: [{ address: ADDRESS, balance: "0", tokenBalances: [] }], + }, + ]; } const { makeStorageStub } = require("./support/storageStub"); diff --git a/tests/stateRecovery.test.js b/tests/stateRecovery.test.js new file mode 100644 index 0000000..a7e48b9 --- /dev/null +++ b/tests/stateRecovery.test.js @@ -0,0 +1,630 @@ +// A stored profile the popup cannot read must produce a SCREEN, not a blank +// popup (https://git.eeqj.de/sneak/AutistMask/issues/311). +// +// The three corrupt blobs below are the ones the pre-1.0 audit wrote into +// storage. Against the build this file was added to, each one rendered nothing +// at all — no view, no message, no control — because init() dereferenced +// `state.wallets[0].addresses` on a record nothing had validated and threw +// before the first showView(). +// +// So the assertions here are deliberately made through the REAL popup entry +// point rather than against the recovery view module directly. A recovery +// screen that renders perfectly when something calls it, and that nothing +// 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 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"; + +// ------------------------------------------------------------- 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 = [ + { + name: "wallets is a string", + blob: { hasWallet: true, wallets: ADDRESS, activeAddress: ADDRESS }, + }, + { + name: "wallets is an array of garbage", + blob: { + hasWallet: true, + wallets: [null, 42, "wallet"], + activeAddress: ADDRESS, + }, + }, + { + name: "future-schema blob (unknown fields, no version)", + blob: { + hasWallet: true, + // A later schema that renamed the field and moved the key + // material, written by a build this one knows nothing about, and + // stamped with no version because this build never wrote one. + wallets: [ + { + id: "wallet-1", + label: "Wallet 1", + accounts: [{ addr: ADDRESS, wei: "0x0" }], + keyring: { kind: "hd", vault: "…" }, + }, + ], + profileFormat: "am-2", + activeAccount: ADDRESS, + }, + }, +]; + +// ------------------------------------------------------------- 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; +}); + +// --------------------------------------------------------------- tests + +describe("a stored profile the popup cannot read", () => { + for (const { name, blob } of CORRUPT_BLOBS) { + test(`${name}: the recovery screen, not a blank popup`, async () => { + const env = await bootPopup(blob); + + // Asserted together, and in the audit's own shape: a failure here + // prints both what was on screen and what the console said, which + // is the pair that identifies this defect. + expect({ + visibleViews: env.visibleViews(), + errors: env.pageErrors, + }).toEqual({ visibleViews: ["state-recovery"], errors: [] }); + + // The screen has to NAME the problem. A blank recovery screen is + // the same dead end with a border around it. + expect(env.text("state-recovery-problem").length).toBeGreaterThan( + 10, + ); + }); + } + + test("the Settings gear is hidden, since every screen behind it reads the profile", async () => { + const env = await bootPopup(CORRUPT_BLOBS[0].blob); + + expect(env.hidden("btn-settings")).toBe(true); + }); + + test("it does not write over the record it could not read", async () => { + // The blob is evidence, and possibly the only copy of key material in + // a shape a later build could recover. A boot that normalized it back + // into storage would destroy exactly that. + const env = await bootPopup(CORRUPT_BLOBS[2].blob); + + expect(env.storage.read("autistmask")).toEqual(CORRUPT_BLOBS[2].blob); + expect(env.storage.set).not.toHaveBeenCalled(); + }); +}); + +describe("the export on the recovery screen", () => { + test("hands back the raw stored record verbatim", async () => { + const env = await bootPopup(CORRUPT_BLOBS[2].blob); + + await env.click("btn-state-recovery-export"); + + // Shown in the page, which always works, whatever the browser does + // with a download from an extension popup. + expect(env.hidden("state-recovery-blob")).toBe(false); + expect(JSON.parse(env.value("state-recovery-blob"))).toEqual( + CORRUPT_BLOBS[2].blob, + ); + }); +}); + +describe("the destructive reset on the recovery screen", () => { + test("erases nothing without the typed confirmation", async () => { + const env = await bootPopup(CORRUPT_BLOBS[0].blob); + + env.node("state-recovery-reset-input").value = "yes"; + await env.click("btn-state-recovery-reset"); + + expect(env.storage.read("autistmask")).toEqual(CORRUPT_BLOBS[0].blob); + expect(env.text("state-recovery-flash").length).toBeGreaterThan(10); + expect(env.reloaded()).toBe(0); + }); + + test("erases the stored profile once the phrase is typed", async () => { + const env = await bootPopup(CORRUPT_BLOBS[0].blob); + + env.node("state-recovery-reset-input").value = "erase my wallet"; + await env.click("btn-state-recovery-reset"); + + expect(env.storage.read("autistmask")).toBeUndefined(); + expect(env.reloaded()).toBe(1); + }); +}); + +describe("an unversioned profile that is perfectly valid", () => { + // The upgrade case. Every install in the field is in this state, and the + // popup must load it, not offer to wipe it. + test("boots to the wallet list, not the recovery screen", async () => { + const env = await bootPopup(unversionedValidProfile()); + + expect(env.visibleViews()).toEqual(["main"]); + expect(env.pageErrors).toEqual([]); + }); + + test("is migrated in place: the version is stamped, the wallet survives", async () => { + const env = await bootPopup(unversionedValidProfile()); + + const stored = env.storage.read("autistmask"); + expect(stored.schemaVersion).toBe(1); + expect(stored.wallets).toHaveLength(1); + expect(stored.wallets[0].encryptedSecret).toBe("encrypted-secret-1"); + expect(stored.wallets[0].addresses[0].address).toBe(ADDRESS); + expect(stored.activeAddress).toBe(ADDRESS); + expect(stored.allowedSites).toEqual({ [ADDRESS]: ["dapp.example"] }); + }); +}); + +describe("a first run with nothing in storage", () => { + test("boots to Welcome", async () => { + const env = await bootPopup(undefined); + + expect(env.visibleViews()).toEqual(["welcome"]); + expect(env.pageErrors).toEqual([]); + }); +}); + +describe("a garbage value in a field the gate does not check", () => { + // The gate refuses only what nothing can floor: the wallet list, the + // version, the network key. Everything else is normalizePersisted()'s job, + // and where that job was written as `saved.x || default` rather than a + // type check, a TRUTHY value of the wrong type walked straight through and + // threw on the first dereference — the same blank popup this issue is + // about, measured the same way. Every row below did, at the head named + // against it; none has ever been removed from this list. + // + // These belong on the floor rather than in the gate: none of these values + // carries key material, all have a sane default, and sending a user whose + // wallets are perfectly readable to an export-or-erase screen over a + // broken token list would destroy more than it saves. + // + // The CONTAINER and its ELEMENTS are separate defects. Round 2 floored the + // containers with Array.isArray(), which left every row whose container is + // a well-formed list of malformed entries still blanking the popup: the + // dereference is `t.address.toLowerCase()`, one level below the check. + const CORRUPT_FIELDS = [ + // Container shapes. Observed at 2e2ecf9, before the round-2 floor: + // trackedTokens: "nope" -> views=[] "Cannot read properties of + // undefined (reading 'toLowerCase')" + // trackedTokens: 42 -> views=[] "trackedTokens is not iterable" + // trackedTokens: {a:1} -> views=[] "trackedTokens is not iterable" + // activeAddress: 42 -> views=[] "address.slice is not a + // function" + // activeAddress: {a:1} -> views=[] "address.slice is not a + // function" + { name: "trackedTokens is a string", patch: { trackedTokens: "nope" } }, + { name: "trackedTokens is a number", patch: { trackedTokens: 42 } }, + { + name: "trackedTokens is an object", + patch: { trackedTokens: { a: 1 } }, + }, + { name: "activeAddress is a number", patch: { activeAddress: 42 } }, + { + name: "activeAddress is an object", + patch: { activeAddress: { a: 1 } }, + }, + // Element shapes: a list, holding entries that are not token records. + // Observed at a10a984, AFTER the container floor: + // [1,2] -> views=[] "Cannot read properties of undefined + // (reading 'toLowerCase')" + // [null] -> views=[] "Cannot read properties of null + // (reading 'address')" + // [{}] -> views=[] "Cannot read properties of undefined + // (reading 'toLowerCase')" + // [{address:42}] -> views=[] "t.address.toLowerCase is not a + // function" + // ["0xAA…"] -> views=[] "Cannot read properties of undefined + // (reading 'toLowerCase')" + { + name: "trackedTokens holds numbers", + patch: { trackedTokens: [1, 2] }, + }, + { + name: "trackedTokens holds null", + patch: { trackedTokens: [null] }, + }, + { + name: "trackedTokens holds a record with no address", + patch: { trackedTokens: [{}] }, + }, + { + name: "trackedTokens holds a record whose address is a number", + patch: { trackedTokens: [{ address: 42 }] }, + }, + { + name: "trackedTokens holds bare address strings", + patch: { trackedTokens: [TOKEN_ADDRESS] }, + }, + ]; + + // The same defect one level deeper, inside a wallet the gate accepted. + // tokenBalances is written WHOLESALE by refreshBalances(), so the partial + // write https://git.eeqj.de/sneak/AutistMask/issues/311 names as the live + // cause of a corrupt record lands exactly here. Observed at a10a984: + // "x" -> views=[] "Cannot read properties of undefined (reading + // 'toLowerCase')" (a string iterates as characters) + // [null] -> views=[] "Cannot read properties of null (reading + // 'balance')" + // [42] -> views=[] "Cannot read properties of undefined (reading + // 'toLowerCase')" + // 42 -> views=[] "number 42 is not iterable" + const CORRUPT_TOKEN_BALANCES = [ + { name: "a string", value: "x" }, + { name: "a list holding null", value: [null] }, + { name: "a list of numbers", value: [42] }, + { name: "a number", value: 42 }, + ]; + + function profileWithTokenBalances(value) { + const profile = unversionedValidProfile(); + profile.wallets[0].addresses[0].tokenBalances = value; + return profile; + } + + for (const { name, patch } of CORRUPT_FIELDS) { + test(`${name}: a working popup, not a blank one`, async () => { + const env = await bootPopup( + Object.assign(unversionedValidProfile(), patch), + ); + + expect({ + visibleViews: env.visibleViews(), + errors: env.pageErrors, + }).toEqual({ visibleViews: ["main"], errors: [] }); + }); + } + + for (const { name, value } of CORRUPT_TOKEN_BALANCES) { + test(`an address whose tokenBalances is ${name}: a working popup, not a blank one`, async () => { + const env = await bootPopup(profileWithTokenBalances(value)); + + expect({ + visibleViews: env.visibleViews(), + errors: env.pageErrors, + }).toEqual({ visibleViews: ["main"], errors: [] }); + }); + } + + test("the wallet is intact afterwards, and the bad value is gone", async () => { + const env = await bootPopup( + Object.assign(unversionedValidProfile(), { + trackedTokens: "nope", + activeAddress: 42, + }), + ); + + const stored = env.storage.read("autistmask"); + expect(stored.wallets[0].encryptedSecret).toBe("encrypted-secret-1"); + expect(stored.trackedTokens).toEqual([]); + // Floored to null, then filled in by init()'s auto-default. + expect(stored.activeAddress).toBe(ADDRESS); + }); + + test("an empty activeAddress does not leave the popup with none selected", async () => { + // "" is text, so a type check alone lets it through — and init() + // auto-selects the first address only on a STRICT null, so it has to + // be floored to null rather than kept. + const env = await bootPopup( + Object.assign(unversionedValidProfile(), { activeAddress: "" }), + ); + + expect(env.visibleViews()).toEqual(["main"]); + expect(env.storage.read("autistmask").activeAddress).toBe(ADDRESS); + }); + + test("a malformed token entry is dropped, and the well-formed ones beside it survive", async () => { + const env = await bootPopup( + Object.assign(unversionedValidProfile(), { + trackedTokens: [ + 1, + null, + {}, + { address: 42 }, + TOKEN_ADDRESS, + { address: TOKEN_ADDRESS, symbol: "AAA", decimals: 18 }, + ], + }), + ); + + const stored = env.storage.read("autistmask"); + expect(stored.trackedTokens).toEqual([ + { address: TOKEN_ADDRESS, symbol: "AAA", decimals: 18 }, + ]); + }); + + test("a malformed tokenBalances entry is dropped, and the wallet and its address survive", async () => { + const env = await bootPopup( + profileWithTokenBalances([ + null, + 42, + { address: TOKEN_ADDRESS, symbol: "AAA", balance: "2.0" }, + ]), + ); + + const stored = env.storage.read("autistmask"); + expect(stored.wallets[0].encryptedSecret).toBe("encrypted-secret-1"); + expect(stored.wallets[0].addresses[0].address).toBe(ADDRESS); + expect(stored.wallets[0].addresses[0].tokenBalances).toEqual([ + { address: TOKEN_ADDRESS, symbol: "AAA", balance: "2.0" }, + ]); + }); +}); diff --git a/tests/stateSchema.test.js b/tests/stateSchema.test.js new file mode 100644 index 0000000..d0d375d --- /dev/null +++ b/tests/stateSchema.test.js @@ -0,0 +1,423 @@ +// The stored-profile version stamp and the shape gate in front of it +// (https://git.eeqj.de/sneak/AutistMask/issues/311). +// +// tests/stateRecovery.test.js covers what the USER sees when a record is +// refused. This file covers what is refused and what is not, which is the +// half that decides whether an upgrade brick or a false alarm ever happens: +// a gate that refuses too much sends a perfectly good wallet to a wipe prompt, +// and one that refuses too little is the blank popup again. + +const fs = require("fs"); +const path = require("path"); + +const { + STATE_SCHEMA_VERSION, + StateUnusableError, + assertStateUsable, + migrationNeeded, + stateProblem, +} = require("../src/shared/stateSchema"); +const { + NETWORKS, + UnknownNetworkError, + isKnownNetworkId, + networkById, +} = require("../src/shared/networks"); +const { normalizePersisted } = require("../src/shared/persistedState"); +const { RESET_PHRASE } = require("../src/popup/views/stateRecovery"); +const { makeStorageStub } = require("./support/storageStub"); + +const ADDRESS = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; + +function validProfile(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", + ...(extra || {}), + }; +} + +afterEach(() => { + delete global.chrome; +}); + +describe("what the gate accepts", () => { + test("nothing stored at all is a first run, not a defect", () => { + expect(stateProblem(undefined)).toBeNull(); + expect(stateProblem(null)).toBeNull(); + expect(stateProblem({})).toBeNull(); + }); + + test("a valid profile with no version field is accepted and migrated", () => { + const saved = validProfile(); + + expect(stateProblem(saved)).toBeNull(); + expect(migrationNeeded(saved)).toBe(true); + // The migration IS the stamp: version 1 is the shape that shipped + // unversioned, so nothing about the record has to change. + expect(normalizePersisted(saved).schemaVersion).toBe( + STATE_SCHEMA_VERSION, + ); + expect(normalizePersisted(saved).wallets).toEqual(saved.wallets); + }); + + test("a profile already at the current version needs no migration", () => { + const saved = validProfile({ schemaVersion: STATE_SCHEMA_VERSION }); + + expect(stateProblem(saved)).toBeNull(); + expect(migrationNeeded(saved)).toBe(false); + }); + + test("an empty wallet list is fine", () => { + expect(stateProblem({ hasWallet: false, wallets: [] })).toBeNull(); + }); + + test("unknown extra fields alone are not a defect", () => { + // Only a change in the MEANING of a stored field is a version bump, so + // a field this build does not know must not be a refusal on its own — + // otherwise a downgrade would wipe a working wallet. + expect(stateProblem(validProfile({ somethingNew: 42 }))).toBeNull(); + }); +}); + +describe("what the gate refuses", () => { + test("a record that is not the record AutistMask stores", () => { + expect(stateProblem("wallet")).toMatch(/not the record/); + expect(stateProblem([1, 2])).toMatch(/not the record/); + }); + + test("a version from a newer build, naming both versions", () => { + const problem = stateProblem( + validProfile({ schemaVersion: STATE_SCHEMA_VERSION + 1 }), + ); + + expect(problem).toMatch(/newer version/); + expect(problem).toContain(String(STATE_SCHEMA_VERSION + 1)); + expect(problem).toContain(String(STATE_SCHEMA_VERSION)); + }); + + test("a version that is not a version at all", () => { + for (const version of ["1", 1.5, 0, -1, null, {}]) { + expect( + stateProblem(validProfile({ schemaVersion: version })), + ).toMatch(/schema version/); + } + }); + + test("wallets that is not a list", () => { + expect(stateProblem({ wallets: ADDRESS })).toMatch(/not a list/); + expect(stateProblem({ wallets: { 0: {} } })).toMatch(/not a list/); + }); + + test("a wallet that is not a wallet record", () => { + expect(stateProblem({ wallets: [null] })).toMatch(/wallet record/); + expect(stateProblem({ wallets: [42] })).toMatch(/wallet record/); + }); + + test("a wallet whose addresses are missing or not records", () => { + expect(stateProblem({ wallets: [{ name: "Wallet 1" }] })).toMatch( + /no list of addresses/, + ); + expect(stateProblem({ wallets: [{ addresses: [ADDRESS] }] })).toMatch( + /not a record/, + ); + expect(stateProblem({ wallets: [{ addresses: [{}] }] })).toMatch( + /no address/, + ); + }); + + test("the problem names WHICH wallet, counting from one", () => { + const problem = stateProblem({ + wallets: [validProfile().wallets[0], { name: "Wallet 2" }], + }); + + expect(problem).toContain("Wallet 2"); + }); + + test("assertStateUsable throws the sentence, not a generic error", () => { + let thrown = null; + try { + assertStateUsable({ wallets: ADDRESS }); + } catch (e) { + thrown = e; + } + + expect(thrown).toBeInstanceOf(StateUnusableError); + expect(thrown.problem).toMatch(/not a list/); + expect(thrown.message).toBe(thrown.problem); + }); +}); + +describe("networkId, which is used as an object key", () => { + // https://git.eeqj.de/sneak/AutistMask/issues/311#issuecomment-67478: + // state.networkId keys state.networkEndpoints, so a corrupt "__proto__" + // sets that map's prototype instead of an own key and the user's endpoint + // is silently not recorded. + test("a network this build does not know is refused", () => { + expect(stateProblem(validProfile({ networkId: "base" }))).toMatch( + /does not know/, + ); + }); + + test('"__proto__" and "constructor" are refused, not resolved', () => { + for (const id of ["__proto__", "constructor", "toString"]) { + expect(stateProblem(validProfile({ networkId: id }))).toMatch( + /does not know/, + ); + expect(isKnownNetworkId(id)).toBe(false); + } + }); + + test("the gate reads own properties only", () => { + // A record whose PROTOTYPE carries the fields must not be read as + // though it carried them itself: that is how a polluted prototype + // would decide whether a profile is refused. + const inherited = Object.create({ + schemaVersion: STATE_SCHEMA_VERSION + 99, + networkId: "base", + wallets: "not a list", + }); + + expect(stateProblem(inherited)).toBeNull(); + }); + + test("normalizing never turns a stored key into a prototype", () => { + // JSON can carry an own "__proto__" key, and plain assignment would + // treat it as the prototype setter rather than storing an entry. + const saved = JSON.parse( + '{"networkId":"mainnet","networkEndpoints":' + + '{"__proto__":{"rpcUrl":"https://evil.invalid"}}}', + ); + + const out = normalizePersisted(saved); + + expect(Object.prototype.hasOwnProperty.call(out, "rpcUrl")).toBe(true); + expect(out.rpcUrl).not.toBe("https://evil.invalid"); + expect(Object.getPrototypeOf(out.networkEndpoints)).toBe( + Object.prototype, + ); + expect({}.rpcUrl).toBeUndefined(); + }); + + test("an unknown stored networkId never reaches the endpoint map", () => { + // The floor under the gate: normalization alone must not adopt it. + const out = normalizePersisted({ networkId: "__proto__" }); + + expect(out.networkId).toBe("mainnet"); + expect(Object.keys(out.networkEndpoints)).toEqual(["mainnet"]); + }); +}); + +describe("the floors under the gate, for fields the gate does not check", () => { + // The gate's scope is what nothing can floor. Everything it lets through + // is normalizePersisted()'s to make safe, and a floor written as + // `saved.x || default` is not one: a truthy value of the wrong type walks + // through it and throws on the first dereference, which produced the blank + // popup from the issue. Nor is a container check on its own: [1, 2] is a + // list, and the dereference is `t.address.toLowerCase()` one level below + // it. Container AND entries, therefore — an empty list still survives. + const TOKEN = "0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + + test("trackedTokens that is not a list becomes an empty list", () => { + for (const bad of ["nope", 42, true, { a: 1 }]) { + expect( + normalizePersisted({ trackedTokens: bad }).trackedTokens, + ).toEqual([]); + } + }); + + test("a trackedTokens entry that is not a token record is dropped", () => { + for (const bad of [1, null, {}, { address: 42 }, TOKEN, [], true]) { + expect( + normalizePersisted({ trackedTokens: [bad] }).trackedTokens, + ).toEqual([]); + } + }); + + test("a real trackedTokens list survives, copied not shared", () => { + const saved = { trackedTokens: [{ address: ADDRESS, symbol: "AM" }] }; + + const out = normalizePersisted(saved); + + expect(out.trackedTokens).toEqual(saved.trackedTokens); + expect(out.trackedTokens).not.toBe(saved.trackedTokens); + expect(normalizePersisted({ trackedTokens: [] }).trackedTokens).toEqual( + [], + ); + }); + + test("a good trackedTokens entry beside a malformed one survives", () => { + const good = { address: TOKEN, symbol: "AM", decimals: 18 }; + + expect( + normalizePersisted({ trackedTokens: [1, null, good, {}] }) + .trackedTokens, + ).toEqual([good]); + }); + + // Below an address record, which the gate walks but does not descend into. + // refreshBalances() writes tokenBalances WHOLESALE, so a write that only + // partly lands leaves exactly this field malformed. + function walletWith(tokenBalances) { + return { + wallets: [ + { + name: "Wallet 1", + addresses: [{ address: ADDRESS, tokenBalances }], + }, + ], + }; + } + + function balancesOf(out) { + return out.wallets[0].addresses[0].tokenBalances; + } + + test("an address's tokenBalances that is not a list becomes an empty list", () => { + for (const bad of ["x", 42, true, { a: 1 }, undefined]) { + expect(balancesOf(normalizePersisted(walletWith(bad)))).toEqual([]); + } + }); + + test("a tokenBalances entry that is not a token record is dropped", () => { + for (const bad of [null, 42, "x", {}, { address: 42 }]) { + expect(balancesOf(normalizePersisted(walletWith([bad])))).toEqual( + [], + ); + } + }); + + test("a real tokenBalances entry survives, copied not shared", () => { + const held = { address: TOKEN, symbol: "AM", balance: "2.0" }; + const saved = walletWith([held]); + + const out = normalizePersisted(saved); + + expect(balancesOf(out)).toEqual([held]); + expect(balancesOf(out)[0]).not.toBe(held); + }); + + test("activeAddress that is not text becomes null", () => { + for (const bad of [42, true, { a: 1 }, [ADDRESS]]) { + expect( + normalizePersisted({ activeAddress: bad }).activeAddress, + ).toBeNull(); + } + }); + + test("a real activeAddress survives; the empty string becomes null", () => { + expect( + normalizePersisted({ activeAddress: ADDRESS }).activeAddress, + ).toBe(ADDRESS); + // "" is text but it is not an address, and src/popup/index.js + // auto-selects the first address only on a STRICT null — so keeping + // the empty string would leave the popup with none ever selected. + expect( + normalizePersisted({ activeAddress: "" }).activeAddress, + ).toBeNull(); + }); +}); + +describe("networkById on an unknown id", () => { + test("throws instead of quietly answering mainnet", () => { + expect(() => networkById("base")).toThrow(UnknownNetworkError); + expect(() => networkById(undefined)).toThrow(UnknownNetworkError); + // Both of these used to answer with something truthy off the + // prototype chain rather than with a network. + expect(() => networkById("constructor")).toThrow(UnknownNetworkError); + expect(() => networkById("__proto__")).toThrow(UnknownNetworkError); + }); + + test("still answers every network it does know", () => { + for (const id of Object.keys(NETWORKS)) { + expect(networkById(id).id).toBe(id); + } + }); +}); + +describe("the version stamp on the way out", () => { + function loadStateModule(persisted) { + jest.resetModules(); + const storage = makeStorageStub( + persisted ? { autistmask: persisted } : {}, + ); + global.chrome = { storage }; + return { storage, mod: require("../src/shared/state") }; + } + + test("the popup stamps it on a profile that had none", async () => { + const { storage, mod } = loadStateModule(validProfile()); + + await mod.loadState(); + await mod.saveState(); + + expect(storage.read("autistmask").schemaVersion).toBe( + STATE_SCHEMA_VERSION, + ); + }); + + test("the background stamps it on a profile that had none", async () => { + jest.resetModules(); + const storage = makeStorageStub({ autistmask: validProfile() }); + global.chrome = { storage }; + const { updateState } = require("../src/background/state"); + + await updateState((s) => { + s.lastBalanceRefresh = 1; + }); + + expect(storage.read("autistmask").schemaVersion).toBe( + STATE_SCHEMA_VERSION, + ); + }); + + test("a save refuses to write over a record it cannot read", async () => { + // Another context — a newer build, or something else entirely — wrote + // a record this one does not understand while this page was open. That + // record is the only copy of whatever it holds, and normalizing it + // back into storage would destroy it. + const { storage, mod } = loadStateModule(validProfile()); + await mod.loadState(); + + const hostile = { schemaVersion: STATE_SCHEMA_VERSION + 1 }; + storage.write("autistmask", hostile); + + // By name rather than by constructor: jest.resetModules() above gives + // the module under test its own copy of the error class, so instanceof + // across that boundary would be comparing two identical classes. + const thrown = await mod.saveState().then( + () => null, + (e) => e, + ); + expect(thrown && thrown.name).toBe("StateUnusableError"); + expect(thrown.problem).toMatch(/newer version/); + expect(storage.read("autistmask")).toEqual(hostile); + }); +}); + +describe("the typed confirmation phrase", () => { + test("the markup asks for the phrase the code checks", () => { + // The button is behind a phrase typed by hand. A screen that asks for + // one phrase while the code compares another is an exit that cannot be + // taken, on the one screen that exists to be an exit. + const html = fs.readFileSync( + path.join(__dirname, "..", "src", "popup", "index.html"), + "utf8", + ); + + expect(html).toContain(RESET_PHRASE); + }); +}); diff --git a/tests/stateUnusableRpc.test.js b/tests/stateUnusableRpc.test.js new file mode 100644 index 0000000..66fd70a --- /dev/null +++ b/tests/stateUnusableRpc.test.js @@ -0,0 +1,203 @@ +// What a dApp is told when the wallet's stored profile cannot be read +// (https://git.eeqj.de/sneak/AutistMask/issues/311). +// +// The popup is not the only casualty of a bad blob. getActiveAddress() +// dereferences the stored wallet list on nearly every method, so against the +// build this file was added to, EVERY request from EVERY page came back as +// -32603 "AutistMask could not complete this request because of an internal +// error" — the code the wallet also answers when a signing attempt blows up, +// with nothing in it to tell the page or the user what is actually wrong or +// what to do about it. +// +// So what is pinned here is that the answer is SPECIFIC: its own code, and a +// message that says the saved data cannot be read, that nothing was signed or +// sent, and where to go to fix it. +// +// Same cold-worker shape as tests/coldWorkerChainId.test.js: the real state +// modules, over a storage stub, with no loadState() of the test's own — the +// handler has to reach storage by itself, as a worker revived by the page's +// own message does. + +const CONNECTED_ORIGIN = "https://dapp.example"; +const ADDRESS = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; + +// The generic answer, quoted rather than imported: this file's whole point is +// that the state-unusable path stopped using it. +const GENERIC_INTERNAL_ERROR_CODE = -32603; + +// EIP-1474's assigned non-standard codes, verbatim. The spec sets aside +// -32000..-32099 for implementation-defined server errors but hands out +// meanings for the first seven, so those are exactly the codes this condition +// may NOT take: a page reading -32001 is entitled to read "Resource not +// found". -32002 is in this table AND in use here, for a pending approval. +const EIP_1474_ASSIGNED = { + "-32000": "Invalid input", + "-32001": "Resource not found", + "-32002": "Resource unavailable", + "-32003": "Transaction rejected", + "-32004": "Method not supported", + "-32005": "Limit exceeded", + "-32006": "JSON-RPC version not supported", +}; + +// The three blobs from the issue. +const CORRUPT_BLOBS = [ + { + name: "wallets is a string", + blob: { hasWallet: true, wallets: ADDRESS }, + }, + { + name: "wallets is an array of garbage", + blob: { hasWallet: true, wallets: [null, 42, "wallet"] }, + }, + { + name: "future-schema blob (unknown fields, no version)", + blob: { + hasWallet: true, + wallets: [{ id: "wallet-1", accounts: [{ addr: ADDRESS }] }], + profileFormat: "am-2", + }, + }, +]; + +async function settle() { + for (let i = 0; i < 50; i++) await Promise.resolve(); +} + +afterEach(() => { + delete global.chrome; +}); + +function loadColdWorker(stored) { + jest.resetModules(); + + jest.doMock("../src/shared/balances", () => ({ + getProvider: () => ({}), + refreshBalances: jest.fn(async () => {}), + })); + jest.doMock("../src/shared/phishingDomains", () => ({ + isPhishingDomain: () => false, + })); + jest.doMock("../src/shared/alarms", () => ({ + BALANCE_REFRESH_ALARM: "balance", + BALANCE_REFRESH_PERIOD_MINUTES: 1, + ensureRecurringAlarms: jest.fn(async () => {}), + registerAlarmHandlers: jest.fn(), + })); + + const store = { autistmask: structuredClone(stored) }; + let messageListener = null; + const set = jest.fn(async (items) => { + store.autistmask = structuredClone(items.autistmask); + }); + + global.chrome = { + storage: { + local: { + get: jest.fn(async () => structuredClone(store)), + set, + }, + }, + runtime: { + getURL: (p) => "chrome-extension://autistmask/" + p, + onMessage: { + addListener: (fn) => { + messageListener = fn; + }, + }, + onConnect: { addListener: () => {} }, + lastError: null, + }, + windows: { + getLastFocused: (cb) => cb(null), + create: (options, cb) => cb({ id: 1 }), + remove: (id, cb) => { + if (cb) cb(); + }, + onRemoved: { addListener: () => {} }, + }, + tabs: { + query: (queryInfo, cb) => cb([{ id: 1 }]), + sendMessage: (tabId, message, cb) => { + if (cb) cb(); + }, + }, + action: { setPopup: () => {} }, + }; + + require("../src/background/index"); + + async function rpc(method, params) { + let result = null; + messageListener( + { type: "AUTISTMASK_RPC", method, params: params || [] }, + { origin: CONNECTED_ORIGIN }, + (r) => { + result = r; + }, + ); + await settle(); + return result; + } + + return { rpc, persisted: () => store.autistmask, storageSet: set }; +} + +// Every method a page can reach that has to consult the profile. +const METHODS = [ + "eth_accounts", + "eth_requestAccounts", + "eth_chainId", + "personal_sign", + "eth_sendTransaction", +]; + +describe("a dApp call against a profile the wallet cannot read", () => { + for (const { name, blob } of CORRUPT_BLOBS) { + test(`${name}: a specific error, not the generic internal one`, async () => { + const bg = loadColdWorker(blob); + + const answer = await bg.rpc("eth_accounts"); + + expect(answer.error).toBeDefined(); + expect(answer.error.code).not.toBe(GENERIC_INTERNAL_ERROR_CODE); + // The message has to say what is wrong, that nothing was sent, + // and where to go. "Internal error" says none of the three. + expect(answer.error.message).toMatch(/saved data/i); + expect(answer.error.message).toMatch(/nothing was/i); + expect(answer.error.message).toMatch(/AutistMask/); + }); + } + + test("every method that consults the profile answers the same way", async () => { + const bg = loadColdWorker(CORRUPT_BLOBS[0].blob); + + const codes = new Set(); + for (const method of METHODS) { + const answer = await bg.rpc(method, ["0x00", ADDRESS]); + expect(answer.error).toBeDefined(); + codes.add(answer.error.code); + } + + // One code for the condition, whatever the method was. + expect(codes.size).toBe(1); + expect(codes.has(GENERIC_INTERNAL_ERROR_CODE)).toBe(false); + + // And it is a code EIP-1474 has not already given a meaning to, so a + // page reading it by the spec's table is not told something false. + const code = [...codes][0]; + expect(EIP_1474_ASSIGNED[String(code)]).toBeUndefined(); + expect(code).toBeLessThanOrEqual(-32007); + expect(code).toBeGreaterThanOrEqual(-32099); + }); + + test("it does not write over the record it could not read", async () => { + const bg = loadColdWorker(CORRUPT_BLOBS[1].blob); + + await bg.rpc("eth_accounts"); + await bg.rpc("eth_chainId"); + + expect(bg.storageSet).not.toHaveBeenCalled(); + expect(bg.persisted()).toEqual(CORRUPT_BLOBS[1].blob); + }); +}); diff --git a/tests/txStatus.test.js b/tests/txStatus.test.js index 2f92947..267aabe 100644 --- a/tests/txStatus.test.js +++ b/tests/txStatus.test.js @@ -107,7 +107,7 @@ global.chrome = { storage }; const txStatus = require("../src/popup/views/txStatus"); const { state } = require("../src/shared/state"); -const { RESTORABLE_VIEWS } = require("../src/popup/restorableViews"); +const { RESTORABLE_VIEWS } = require("../src/shared/restorableViews"); const TX_HASH = "0x85215772ed26ea8b39c2b3b18779030487efbe0b5fd7e882592b2f62b837be84";