fix: version the stored profile, and give a record that cannot be read a way out (closes #311)

The stored profile carried no version, so nothing could tell a record this build wrote from one a later build did, and loadState() coerced scalars while trusting the structure. A wallets that was a string, an array of nulls, or a later schema's wallet records reached the popup and threw on the first dereference: no view, no message, no control, and every dApp call answering a generic -32603 because getActiveAddress() dereferenced the same record. There was no reset or wipe control anywhere in the product, so the only escape was clearing extension storage through browser internals.

saveState() and updateState() now both stamp STATE_SCHEMA_VERSION, and every read goes through assertStateUsable() on the raw bytes before normalization can 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, and refused all the way: not normalized, not written back, not half-loaded, and not overwritten by a save either.

The gate covers what nothing downstream can floor. Everything else is normalizePersisted()'s job, and two of those floors were written on truthiness rather than on type, so a truthy value of the wrong type walked straight through and threw on the first dereference — the same blank popup, by a longer route. trackedTokens: "nope" rendered nothing with "Cannot read properties of undefined (reading 'toLowerCase')", and activeAddress: 42 rendered nothing with "address.slice is not a function", for profiles whose wallets were perfectly readable. Both are type checks now, matching what networkEndpoints and viewStack already did in the same file, and an empty list or an empty string still survives.

The popup shows a new StateRecovery screen. It names the problem in a sentence, exports the stored record into a text box on the page with no normalization or repair on it (and downloads it where the browser allows), and offers an erase behind a typed ERASE MY WALLET. Both controls are required: 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 Settings gear is hidden while it is up, and showView() is not used to raise it, because both read the state singleton that by then refuses to be read. The export is JSON.stringify of the deserialized record, so a value JSON cannot represent — a cycle, or a BigInt, which Firefox's storage can hold — fails the export entirely and leaves erase as the only control; that is now stated where the export is written.

The background refuses the same record and answers dApps -32007 with a message saying the saved data cannot be read and that nothing was signed or sent, rather than the -32603 it also answers when a signing attempt breaks. EIP-1474 sets aside -32000..-32099 for implementation-defined server errors but assigns meanings to -32000 through -32006, including -32001 "Resource not found" and the -32002 "Resource unavailable" this wallet already uses for a pending approval; -32007..-32099 are the unassigned ones, and a test pins the code against that table.

networkById() now throws on an id it does not know instead of quietly answering mainnet, which also stops NETWORKS["constructor"] resolving off the prototype chain. Every key test in the gate is an own-property test, because networkId is an object key into networkEndpoints and an unvalidated "__proto__" set that map's prototype instead of an own key, dropping the user's endpoint silently; normalizePersisted() copies endpoint entries with defineProperty for the same reason. That own-property discipline is the gate's alone — normalizePersisted() reads the same fields plainly, and the two agree only because a record from storage has been through structuredClone and carries Object.prototype.

The three corrupt blobs from the issue drive the real popup entry point and the real worker in tests; each rendered nothing at all and answered -32603 before this, and the unversioned-but-valid case is tested too. Three test files used fixture wallets the product cannot produce (a bare address string where an address record belongs, a wallet with no address list) and now use whole records. src/popup/restorableViews.js moved to src/shared/restorableViews.js, since persistedState.js requires it and that module is in the background bundle.

README.md

@@ -1010,8 +1010,15 @@ 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
-(`-32001`) saying the saved data cannot be read and that nothing was signed or
+(`-32007`, an EIP-1474 server-error code the spec leaves unassigned) saying the
-sent, rather than the generic `-32603` every request used to answer.
+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, and that floor is a type check: a truthy value of the wrong type walks
+through a `saved.x || default` and throws on the first dereference, which is the
+blank popup again by a longer route. Adding a field means giving it a floor
+there or a check in the gate.
 
 The `networkId` check is not cosmetic: that value is an object KEY into
 `state.networkEndpoints`, so an unvalidated `"__proto__"` would set the map's

TODO.md

@@ -74,11 +74,15 @@ but the review is broader than any of them.
   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 `-32001` with a message saying the saved data
+  same record and answers dApps `-32007` — a code EIP-1474 leaves unassigned,
-  cannot be read and that nothing was signed or sent, rather than the generic
+  unlike `-32000`..`-32006` — with a message saying the saved data cannot be
-  `-32603` that every request used to get. `networkById()` now throws on an id
+  read and that nothing was signed or sent, rather than the generic `-32603`
-  it does not know instead of quietly answering mainnet, and the gate's key
+  that every request used to get. Two fields the gate deliberately does not
-  tests are all own-property tests: `networkId` is an object key into
+  check, `trackedTokens` and `activeAddress`, were floored on truthiness rather
+  than on type and so produced the same blank popup for a truthy value of the
+  wrong type; both are type-checked now. `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`

src/background/index.js

@@ -190,11 +190,20 @@ const INTERNAL_ERROR_MESSAGE =
 // thing that is actually wrong or where to fix it
 // (https://git.eeqj.de/sneak/AutistMask/issues/311).
 //
-// -32001 rather than -32603: EIP-1474 reserves -32000..-32099 for
+// Its own code rather than -32603, because the condition is specific,
-// implementation-defined server errors, this wallet uses no other code in that
+// diagnosable and has a user action attached — none of which "internal error"
-// range except -32002 for a pending approval, and the condition is specific,
+// conveys.
-// diagnosable and has a user action attached — none of which -32603 conveys.
+//
-const STATE_UNUSABLE_CODE = -32001;
+// -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.";

src/popup/views/stateRecovery.js

@@ -40,9 +40,12 @@ function setFlash(message) {
     node.style.visibility = message ? "visible" : "hidden";
 }
 
-// The raw record, as bytes, however malformed. Never normalized and never
+// The stored record exactly as storage hands it back, however malformed, with
-// re-serialized from a parsed copy of itself: this is evidence, and the point
+// no normalization, no defaulting and no repair on it: this is evidence, and
-// of the export is that a later build (or a human) sees exactly what is there.
+// 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;
@@ -82,6 +85,12 @@ function offerDownload(text) {
     }
 }
 
+// Residual, stated rather than left to be discovered: JSON.stringify THROWS on
+// a reference cycle or a BigInt, and Firefox's structured-clone storage can
+// hold both even though no build here writes one. That lands in the catch
+// below, so the export fails entirely and erase is the only control left on
+// the screen. Nothing here can serialize such a record; recovering one needs
+// the browser's own storage inspector.
 async function exportRecord() {
     let text;
     try {

src/shared/persistedState.js

@@ -126,7 +126,14 @@ function normalizePersisted(saved) {
     out.wallets = structuredClone(saved.wallets || []);
     // Derived, never trusted verbatim off storage — see loadState().
     out.hasWallet = out.wallets.length > 0;
-    out.trackedTokens = structuredClone(saved.trackedTokens || []);
+    // An actual list is required, not merely a truthy value: everything
+    // downstream iterates this and dereferences `token.address`, so a stored
+    // string or object walks through a `|| []` and throws 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 = Array.isArray(saved.trackedTokens)
+        ? structuredClone(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
@@ -175,7 +182,13 @@ function normalizePersisted(saved) {
         };
     }
     out.lastBalanceRefresh = saved.lastBalanceRefresh || 0;
-    out.activeAddress = saved.activeAddress || null;
+    // Text 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 and is kept as stored; every reader treats it as "none
+    // selected", which is what it is.
+    out.activeAddress =
+        typeof saved.activeAddress === "string" ? saved.activeAddress : null;
     out.allowedSites =
         saved.allowedSites && !Array.isArray(saved.allowedSites)
             ? structuredClone(saved.allowedSites)

src/shared/stateSchema.js

@@ -24,9 +24,13 @@
 // whatever it holds, and the recovery screen exports it before offering to
 // erase it.
 //
-// What is checked here is what the rest of the code dereferences without a
+// What is checked HERE is what nothing downstream can floor: the wallet list,
-// floor of its own. Everything else has one in normalizePersisted() and does
+// the version, and the network id that keys an object. Every other field is
-// not need a second.
+// normalizePersisted()'s to make safe, and that obligation is a TYPE CHECK,
+// not a `saved.x || default` — a truthy value of the wrong type walks through
+// truthiness and throws on the first dereference, which is the same blank
+// popup, reached the long way round. Adding a field to the record means giving
+// it a floor there or a check here; do not assume a default covers it.
 
 const { isKnownNetworkId } = require("./networks");
 
@@ -161,10 +165,14 @@ function stateProblem(saved) {
     const version = versionProblem(saved);
     if (version) return version;
 
-    // Read once, from an OWN property or not at all. Reading `saved.wallets`
+    // Read once, from an OWN property or not at all, so that a polluted
-    // again below would consult the prototype chain for a record that carries
+    // prototype cannot decide whether a profile is refused. Note that
-    // no wallets of its own, so what gets validated would not be what gets
+    // normalizePersisted() reads the same field plainly, and so WOULD consult
-    // loaded.
+    // 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

tests/stateRecovery.test.js

@@ -440,3 +440,62 @@ describe("a first run with nothing in storage", () => {
         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. Both of these did, at 2e2ecf9:
+    //
+    //   trackedTokens: "nope"  ->  views=[] "Cannot read properties of
+    //                              undefined (reading 'toLowerCase')"
+    //   activeAddress: 42      ->  views=[] "address.slice is not a function"
+    //
+    // These belong on the floor rather than in the gate: neither value carries
+    // key material, both 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.
+    const CORRUPT_FIELDS = [
+        { 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 } },
+        },
+    ];
+
+    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: [] });
+        });
+    }
+
+    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);
+    });
+});

tests/stateSchema.test.js

@@ -221,6 +221,53 @@ describe("networkId, which is used as an object key", () => {
     });
 });
 
+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. These two did, and
+    // produced the blank popup from the issue. Type checks, not truthiness —
+    // an empty list and an empty string are legitimate values and survive.
+    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 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("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, including an empty string", () => {
+        expect(
+            normalizePersisted({ activeAddress: ADDRESS }).activeAddress,
+        ).toBe(ADDRESS);
+        // Not a useful address, but it is text and it is what was stored;
+        // rewriting it to null would be normalization inventing a change.
+        expect(normalizePersisted({ activeAddress: "" }).activeAddress).toBe(
+            "",
+        );
+    });
+});
+
 describe("networkById on an unknown id", () => {
     test("throws instead of quietly answering mainnet", () => {
         expect(() => networkById("base")).toThrow(UnknownNetworkError);

tests/stateUnusableRpc.test.js

@@ -25,6 +25,21 @@ const ADDRESS = "0x66133E8ea0f5D1d612D2502a968757D1048c214a";
 // 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 = [
     {
@@ -167,6 +182,13 @@ describe("a dApp call against a profile the wallet cannot read", () => {
         // 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 () => {