diff --git a/README.md b/README.md index c824dbf..45cde14 100644 --- a/README.md +++ b/README.md @@ -1114,16 +1114,18 @@ 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. +bytes, before normalization: `loadState()` and every `saveState()` 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, +whether it finds the record unreadable when it opens or at a save while it is +open; 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, and the floor is not the same for every field. Some are type-checked as a @@ -1167,8 +1169,9 @@ now also reported rather than swallowed: `onSaveFailure()` in `src/shared/state.js` is called for every failed save, awaited or not, and the popup puts up a persistent "NOT SAVED" banner (`showSaveFailureBanner()` in `src/popup/views/helpers.js`). Storage can still fail for reasons no floor -covers — a quota, a revoked permission, a record a newer build wrote — and the -wallet must never look healthy while that is true. +covers — a quota, a revoked permission — and the wallet must never look healthy +while that is true. A save that fails because the stored record fails the gate, +such as one a newer build wrote, gets StateRecovery instead of the banner. The `networkId` check is not cosmetic: that value is an object KEY into `state.networkEndpoints`, so an unvalidated `"__proto__"` would set the map's @@ -1978,9 +1981,20 @@ view would leave a wallet one click from deletion. #### 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. +- **When**: the stored profile fails `assertStateUsable()`. At open, that is + `loadState()` refusing it, so the popup has no profile at all. While the popup + is open, on any screen, it is a save refusing it: every `saveState()` reads + the stored record and runs the same check before writing, so the popup finds + it at the next navigation, or at the next ten-second balance refresh that + reaches the network ([#373](https://git.eeqj.de/sneak/AutistMask/issues/373)). + A save that fails for any other reason, such as a storage read or write that + errors, gets the "NOT SAVED" banner instead and leaves the screen as it is. + The screen it replaces is left as any navigation leaves it, so a revealed + phrase or key, or a typed password, is wiped. Once up, the screen stays until + the popup closes or the record is erased: work still running in the popup, + such as a transaction wait, cannot replace it, and nothing in AutistMask + writes over a record that fails the check, so it cannot become readable again + underneath. It is the only screen 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. @@ -2007,16 +2021,20 @@ view would leave a wallet one click from deletion. 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. + could not be read. At open `showView()` is not used to raise it for the same + reason — it reads and writes the state singleton. Under an open popup it is + also passed to `showView()`, which runs the replaced screen's cleanup and + records it as the current view, and `showView()` shows nothing else after + that. - **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. +- Not in `RESTORABLE_VIEWS`, and not persisted as the current view: at open + nothing on this path writes state at all, and under an open popup the check + that raised the screen refuses the save that would record it. ### External Services diff --git a/TODO.md b/TODO.md index 6e3158f..702dedc 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,18 @@ but the review is broader than any of them. # Completed Steps +- 2026-10-04: A popup that is already open when the stored profile becomes + unreadable moves to the recovery screen + ([#373](https://git.eeqj.de/sneak/AutistMask/issues/373)). It used to stay on + the last good profile, with the "NOT SAVED" banner at most, until reopened. + Every save already ran the check the popup runs at open; a save that fails + that check now raises the recovery screen and stops the ten-second refresh, so + the popup finds the record at the next navigation or refresh. The screen it + replaces is left as any navigation leaves it, so a revealed phrase or key or a + typed password is wiped. Once up, nothing else can replace it, and a later + save or a transaction wait that ends does not clear an export or a typed + confirmation. Any other failed save still gets the banner and leaves the + screen alone. - 2026-10-04: The signature screen shows a personal message as the bytes that are signed ([#403](https://git.eeqj.de/sneak/AutistMask/issues/403)). It showed only the decoded text, with bidirectional and zero-width characters diff --git a/src/popup/index.js b/src/popup/index.js index a1c2112..14ad18a 100644 --- a/src/popup/index.js +++ b/src/popup/index.js @@ -50,6 +50,10 @@ function renderWalletList() { let refreshInFlight = false; +// The ten-second refresh init() starts, stopped when the popup moves to the +// recovery screen: there is no profile left to refresh. +let refreshTimer = null; + async function doRefreshAndRender() { if (refreshInFlight) return; refreshInFlight = true; @@ -155,7 +159,28 @@ async function init() { // reported rather than being swallowed by the save queue // (https://git.eeqj.de/sneak/AutistMask/issues/362). Registered ahead of // the approval-window branch below too, since that window saves as well. - onSaveFailure(showSaveFailureBanner); + // + // Every save first reads the stored record and refuses it with the same + // check loadState() runs below. So a record that becomes unreadable while + // the popup is open is found by the next save, a navigation or the + // ten-second refresh, and gets the screen it would get at open + // (https://git.eeqj.de/sneak/AutistMask/issues/373). The recovery screen + // is then also passed to showView(), which runs the leave cleanup of the + // screen it replaces, so a phrase, key or password on it is wiped, and + // records it as the current view, after which showView() shows nothing + // else. The save showView() fires fails too and comes back here, where + // both calls see the screen already up and do nothing. Any other failed + // save is a read or write that failed, and gets the banner without + // changing the screen. + onSaveFailure((e) => { + if (e instanceof StateUnusableError) { + clearInterval(refreshTimer); + stateRecovery.show(e); + showView("state-recovery"); + } else { + showSaveFailureBanner(e); + } + }); try { await loadState(); } catch (e) { @@ -244,7 +269,7 @@ async function init() { renderWalletList(); restoreView(); doRefreshAndRender(); - setInterval(doRefreshAndRender, 10000); + refreshTimer = setInterval(doRefreshAndRender, 10000); } } diff --git a/src/popup/views/helpers.js b/src/popup/views/helpers.js index 8a01449..dc4360d 100644 --- a/src/popup/views/helpers.js +++ b/src/popup/views/helpers.js @@ -52,9 +52,10 @@ const VIEWS = [ "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. + // cannot be read. At open it is not reached through showView(), since the + // state singleton refuses to be read; under an open popup, + // src/popup/index.js also passes it to showView() so the screen it + // replaces is left like any other. "state-recovery", ]; @@ -86,6 +87,11 @@ function hideError(id) { } function showView(name) { + // The recovery screen, once up, is never replaced: work still running + // when it went up, such as a transaction wait, must not take the user off + // it or clear what they exported or typed there + // (https://git.eeqj.de/sneak/AutistMask/issues/373). + if (state.currentView === "state-recovery") return; const leaving = state.currentView; if (leaving && leaving !== name) { const onLeave = viewLeaveHandlers.get(leaving); diff --git a/src/popup/views/stateRecovery.js b/src/popup/views/stateRecovery.js index b32f26f..9c933a2 100644 --- a/src/popup/views/stateRecovery.js +++ b/src/popup/views/stateRecovery.js @@ -3,8 +3,11 @@ // 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). +// the time this runs, the stored record has been REFUSED, deliberately: at +// open loadState() refused it and reading the singleton throws +// (https://git.eeqj.de/sneak/AutistMask/issues/311), and under an open popup +// a save refused it, so every further save fails too +// (https://git.eeqj.de/sneak/AutistMask/issues/373). // // 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 @@ -170,6 +173,11 @@ function wire() { * refused, or its sentence. */ function show(problem) { + // Already up: a later save that trips over the same record, such as a + // refresh that was in flight when the screen went up, must not clear what + // the user has exported or typed here. + if (!$("view-state-recovery").classList.contains("hidden")) return; + const sentence = (problem && (problem.problem || problem.message)) || String(problem); diff --git a/tests/stateRecovery.test.js b/tests/stateRecovery.test.js index 2da6146..a10833e 100644 --- a/tests/stateRecovery.test.js +++ b/tests/stateRecovery.test.js @@ -148,6 +148,150 @@ describe("the destructive reset on the recovery screen", () => { }); }); +describe("a popup already open when the stored profile becomes unreadable", () => { + // https://git.eeqj.de/sneak/AutistMask/issues/373. The popup used to stay + // on the wallet list with the last good balances, and only a reopen + // reached the recovery screen. + test("moves to the recovery screen at its next refresh", async () => { + const env = await bootPopup(unversionedValidProfile()); + expect(env.visibleViews()).toEqual(["main"]); + + env.storage.write("autistmask", CORRUPT_BLOBS[0].blob); + await env.tick(); + + expect(env.visibleViews()).toEqual(["state-recovery"]); + expect(env.text("state-recovery-problem").length).toBeGreaterThan(10); + expect(env.hidden("btn-settings")).toBe(true); + expect(env.storage.read("autistmask")).toEqual(CORRUPT_BLOBS[0].blob); + }); + + test("stops the ten-second refresh", async () => { + const env = await bootPopup(unversionedValidProfile()); + env.storage.write("autistmask", CORRUPT_BLOBS[0].blob); + await env.tick(); + const { refreshBalances } = require("../src/shared/balances"); + const calls = refreshBalances.mock.calls.length; + + await env.tick(); + + expect(refreshBalances).toHaveBeenCalledTimes(calls); + }); + + test("a later save does not clear what the user exported or typed", async () => { + const env = await bootPopup(unversionedValidProfile()); + env.storage.write("autistmask", CORRUPT_BLOBS[2].blob); + await env.tick(); + + await env.click("btn-state-recovery-export"); + env.node("state-recovery-reset-input").value = "erase my"; + // Such as the save of a refresh already in flight when the screen + // went up. + const { saveState } = require("../src/shared/state"); + await expect(saveState()).rejects.toThrow(); + await env.settle(); + + expect(env.visibleViews()).toEqual(["state-recovery"]); + expect(env.hidden("state-recovery-blob")).toBe(false); + expect(env.value("state-recovery-reset-input")).toBe("erase my"); + }); + + test("a transaction wait that ends under it does not replace it", async () => { + const env = await bootPopup( + unversionedValidProfile({ + currentView: "wait-tx", + viewData: { + pendingWait: { + hash: "0x1", + txInfo: { to: ADDRESS, amount: "1", token: "ETH" }, + broadcastTime: Date.now(), + }, + }, + }), + ); + expect(env.visibleViews()).toEqual(["wait-tx"]); + env.storage.write("autistmask", CORRUPT_BLOBS[2].blob); + await env.tick(); + await env.click("btn-state-recovery-export"); + env.node("state-recovery-reset-input").value = "erase my"; + + // The test provider answers no receipt lookup, and six that fail in + // a row end the wait with an error. + for (let i = 0; i < 6; i++) await env.tick(); + + expect(env.text("error-tx-message")).toMatch(/could not be reached/); + expect(env.visibleViews()).toEqual(["state-recovery"]); + expect(env.hidden("state-recovery-blob")).toBe(false); + expect(env.value("state-recovery-reset-input")).toBe("erase my"); + }); + + test("a storage read that fails once leaves the wallet list up", async () => { + const env = await bootPopup(unversionedValidProfile()); + + env.storage.local.get.mockRejectedValueOnce( + new Error("IO error: storage busy"), + ); + await env.tick(); + + // Reported as a failed save, not mistaken for an unreadable profile. + expect(env.visibleViews()).toEqual(["main"]); + expect(env.node("save-failure-banner")).not.toBeNull(); + + await env.tick(); + expect(env.visibleViews()).toEqual(["main"]); + }); + + // The screen it replaces is left as any navigation leaves it: the rules + // at the top of src/popup/views/showPhrase.js and exportPrivkey.js hold + // for this way off them too. + describe("from a screen holding a secret", () => { + const PHRASE = + "abandon abandon abandon abandon abandon abandon abandon" + + " abandon abandon abandon abandon about"; + + afterEach(() => jest.dontMock("../src/shared/vault")); + + test("a recovery phrase on screen is wiped", async () => { + jest.doMock("../src/shared/vault", () => ({ + decryptWithPassword: async () => PHRASE, + })); + const env = await bootPopup(unversionedValidProfile()); + require("../src/popup/views/showPhrase").show(0); + env.node("show-phrase-password").value = "password"; + await env.click("btn-show-phrase-reveal"); + expect(env.text("show-phrase-value")).toBe(PHRASE); + + env.storage.write("autistmask", CORRUPT_BLOBS[0].blob); + await env.tick(); + + expect(env.visibleViews()).toEqual(["state-recovery"]); + expect(env.text("show-phrase-value")).toBe(""); + }); + + test("a private key still being decrypted is never written", async () => { + let answer; + jest.doMock("../src/shared/vault", () => ({ + decryptWithPassword: () => + new Promise((resolve) => { + answer = resolve; + }), + })); + const env = await bootPopup(unversionedValidProfile()); + require("../src/popup/views/exportPrivkey").show(0, 0); + env.node("export-privkey-password").value = "password"; + const revealing = env.click("btn-export-privkey-confirm"); + + env.storage.write("autistmask", CORRUPT_BLOBS[0].blob); + await env.tick(); + expect(env.visibleViews()).toEqual(["state-recovery"]); + expect(env.value("export-privkey-password")).toBe(""); + + answer(PHRASE); + await revealing; + expect(env.text("export-privkey-value")).toBe(""); + }); + }); +}); + 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. diff --git a/tests/support/popupBoot.js b/tests/support/popupBoot.js index f389f0a..20d4a76 100644 --- a/tests/support/popupBoot.js +++ b/tests/support/popupBoot.js @@ -40,6 +40,10 @@ jest.doMock("libsodium-wrappers-sumo", () => sodium); jest.doMock("qrcode", () => QRCode); jest.doMock("ethereum-blockies-base64", () => makeBlockie); +// Taken before any boot replaces them; see bootPopup(). +const realSetInterval = globalThis.setInterval; +const realClearInterval = globalThis.clearInterval; + const POPUP_HTML = fs.readFileSync( path.join(__dirname, "..", "..", "src", "popup", "index.html"), "utf8", @@ -292,9 +296,20 @@ async function bootPopup(stored, options) { }), addEventListener: () => {}, }; - // The 10s refresh loop init() starts would outlive the test. - const realSetInterval = globalThis.setInterval; - globalThis.setInterval = () => 0; + // The ten-second refresh init() starts, and a transaction wait's timers, + // would outlive the test. So every interval is recorded rather than + // started, clearInterval() removes it as a browser would, and tick() below + // runs the ones still set. Put back by cleanupPopup(). + const intervals = new Map(); + let lastId = 0; + globalThis.setInterval = (fn) => { + lastId += 1; + intervals.set(lastId, fn); + return lastId; + }; + globalThis.clearInterval = (id) => { + intervals.delete(id); + }; require("../../src/popup/index"); @@ -316,8 +331,6 @@ async function bootPopup(stored, options) { } await settle(); - globalThis.setInterval = realSetInterval; - return { storage, document, @@ -337,6 +350,12 @@ async function bootPopup(stored, options) { for (const fn of fns) await fn(); await settle(); }, + // Every interval still set runs once: the ten-second refresh, and a + // transaction wait's receipt poll and elapsed counter while one runs. + tick: async () => { + for (const fn of intervals.values()) await fn(); + await settle(); + }, settle, // The view ids whose section is not hidden, as the audit measured them. visibleViews: () => { @@ -354,6 +373,8 @@ function cleanupPopup() { delete globalThis.chrome; delete globalThis.document; delete globalThis.window; + globalThis.setInterval = realSetInterval; + globalThis.clearInterval = realClearInterval; } module.exports = {