From 1c105de1fc172e225135475d2c7624ed3ca051ba Mon Sep 17 00:00:00 2001 From: Zaldaryon <273555259+Zaldaryon@users.noreply.github.com> Date: Thu, 27 Aug 2026 06:49:21 -0300 Subject: [PATCH 1/2] fix(account): preserve then rebuild an unreadable account store Fixes #259. readAccounts caught every read failure the same way, absent file or undecryptable one, and cached an empty map either way. saveAccountSecrets then wrote that empty map back over whatever was on disk. At single-account scale that cost the one account the old store held; since multi-account (#238) it costs every saved account's session in one shot, the instant any player logs back in. The sessions in an unreadable file are already gone the moment it stops decrypting: refusing the write recovers none of them, it only leaves the launcher unable to save any account ever again, with nothing in the app to clear the dead file. So this preserves the unreadable bytes once (preserveUnreadableStore, mirroring the existing pre-migration backup convention but under its own path so the two events can never collide) and rebuilds the store around the account logging in now, matching this codebase's own adoptRefreshedSession precedent: a storage problem must not block the player's own deliberate action. The one exception is when the bytes cannot even be copied aside (a permissions problem, most likely); proceeding there really would destroy something, so it throws AccountStoreUnreadableError instead. readStore now distinguishes an absent file (the ordinary no-accounts-yet case, unchanged) from a present-but-unreadable one via a new `unreadable` flag, which only saveAccountSecrets reads; every other caller still just wants the map. saveAccountSecrets returns a typed AccountSaveOutcome ("saved" or "saved-after-rebuild") instead of void. Wired into LOGIN: a rebuild flags the success result with `storeRebuilt: true` rather than a status of its own, since the login did succeed and a separate status would make every `status === "success"` check silently drop the account. The narrow copy-failure case gets its own AccountLoginResult status, `session-store-unreadable`, the same honesty this file already gives `unexpected-response`: the credentials were fine, only the local save failed. SessionButton shows a warning toast on a rebuild and a distinct error on the unreadable case, neither collapsing into "invalid email or password". adoptRefreshedSession (gameHandlers.ts) logs the rebuild but needed no behavior change, since it already treats a storage failure as non-fatal to the launch. 8 new tests in accountStore.test.ts cover the rebuild path: bytes preserved byte-for-byte, a dropped-entry file still treated as readable (not rebuilt), the first snapshot kept across repeated corruption, no collision with the pre-migration backup, and the copy-failure refusal. 2 new tests in accountHandlers.test.ts, 1 in accountLoginOutcome.test.ts, and a new sessionButtonStoreRebuilt.test.tsx cover the wire status and the UI. Verified the harness catches a real regression: reverting saveAccountSecrets to the old always-overwrite version turned 5 of the new accountStore tests red, then reverted cleanly. --- src/global.d.ts | 13 +- src/ipc/accountStore.ts | 139 +++++++++++++++--- src/ipc/handlers/accountHandlers.ts | 27 +++- src/ipc/handlers/accountLoginOutcome.ts | 5 + src/ipc/handlers/gameHandlers.ts | 7 +- .../src/components/ui/SessionButton.tsx | 2 + src/renderer/src/locales/en-US.json | 2 + tests/ipc/accountHandlers.test.ts | 42 +++++- tests/ipc/accountLoginOutcome.test.ts | 5 +- tests/ipc/accountStore.test.ts | 105 +++++++++++++ tests/ipc/configManager.test.ts | 4 +- tests/ipc/gameHandlers.test.ts | 2 +- .../sessionButtonStoreRebuilt.test.tsx | 79 ++++++++++ 13 files changed, 389 insertions(+), 43 deletions(-) create mode 100644 tests/renderer-dom/sessionButtonStoreRebuilt.test.tsx diff --git a/src/global.d.ts b/src/global.d.ts index 62af5aab..61365762 100644 --- a/src/global.d.ts +++ b/src/global.d.ts @@ -79,10 +79,19 @@ declare global { * back, so the player's password was never actually rejected. Collapsing * that case into `invalid-credentials` is exactly the bug this type exists * to make impossible again. + * + * `session-store-unreadable` is the same kind of honesty for a different + * failure: the credentials were accepted, but the local secret store could + * not be read or safely rebuilt, so nothing was saved. `storeRebuilt` on + * `success` is not a status of its own on purpose: the login still + * succeeded, and a separate status would make every `status === "success"` + * check silently drop the account. It flags that the store had to be + * rebuilt around this one login, so a previously unreadable file's other + * saved accounts are gone and will need to log in again. */ type AccountLoginResult = - | { status: "success"; account: AccountPublicType } - | { status: "invalid-credentials" | "requires-two-factor" | "wrong-two-factor" | "unexpected-response"; account?: undefined } + | { status: "success"; account: AccountPublicType; storeRebuilt?: boolean } + | { status: "invalid-credentials" | "requires-two-factor" | "wrong-two-factor" | "unexpected-response" | "session-store-unreadable"; account?: undefined } type GameVersionType = { version: string diff --git a/src/ipc/accountStore.ts b/src/ipc/accountStore.ts index 4eac0817..60b5d95a 100644 --- a/src/ipc/accountStore.ts +++ b/src/ipc/accountStore.ts @@ -4,6 +4,7 @@ import { join } from "node:path" import { parseStoredSecrets, parseStoredSecretsById, type AccountSecrets, type StoredAccountSecretsEntry } from "@domain/account/credentials" import { writeJsonAtomic } from "@src/ipc/atomicJsonFile" +import { isRecord } from "@src/ipc/validation" type EncryptedAccountFile = { version: 2 @@ -25,13 +26,15 @@ const ACCOUNT_STORE_VERSION = 2 const LEGACY_ACCOUNT_STORE_VERSION = 1 /** - * `undefined` means the file has not been read this process. A read that - * cannot be decrypted, or finds nothing there, caches an empty map rather - * than `undefined`, so a later call answers from memory instead of hitting - * the disk again. A miss on one account cannot poison a later hit on another, - * because the cache holds the whole file's contents, not one account's. + * `undefined` means the file has not been read this process. `unreadable` + * says whether the file existed but could not be trusted (wrong version, + * bad JSON, decrypt failure, a payload with no accounts list): a genuinely + * absent file is not unreadable, it is the ordinary no-accounts-yet case, + * and `readStore` never sets the flag for it. `saveAccountSecrets` is the + * only reader of the flag; every other caller only ever wants the map. */ -let cachedAccounts: Map | undefined +type StoreRead = { accounts: Map; unreadable: boolean } +let cachedRead: StoreRead | undefined function getAccountStorePath(): string { return join(app.getPath("userData"), "account-secrets.json") @@ -49,38 +52,96 @@ function getAccountStoreBackupPath(): string { return join(app.getPath("userData"), "account-secrets.pre-migration.bak.json") } +/** + * Where an unreadable store's bytes are kept before {@link saveAccountSecrets} + * rebuilds the file around a new login. A separate file from the + * pre-migration backup above on purpose: those are two different events (an + * old-format file being upgraded, versus a current-format file that stopped + * decrypting), and sharing one path would let whichever happens second + * silently erase the first one's snapshot. + */ +function getUnreadableStoreBackupPath(): string { + return join(app.getPath("userData"), "account-secrets.unreadable.bak.json") +} + +/** The unreadable store's bytes could not be copied aside, so nothing was written over it. */ +export class AccountStoreUnreadableError extends Error { + constructor(cause?: unknown) { + super("The account store is unreadable and its bytes could not be preserved", { cause }) + this.name = "AccountStoreUnreadableError" + } +} + +function isMissingFileError(error: unknown): boolean { + return isRecord(error) && error.code === "ENOENT" +} + function assertSecureStorage(): void { if (!safeStorage.isEncryptionAvailable()) throw new Error("Secure account storage is unavailable") if (process.platform === "linux" && safeStorage.getSelectedStorageBackend() === "basic_text") throw new Error("A system password store is required for account storage") } -async function readAccounts(): Promise> { - if (cachedAccounts !== undefined) return cachedAccounts +async function readStore(): Promise { + if (cachedRead !== undefined) return cachedRead try { assertSecureStorage() + } catch { + // Not the file's fault, and not cached: the keyring becoming available later must still + // reach the real file, and writeAccounts asserts the same thing before it could ever + // overwrite anything, so no save can destroy the store while this is true. + return { accounts: new Map(), unreadable: false } + } + + try { const stored = (await fse.readJSON(getAccountStorePath(), "utf8")) as Partial if (stored.version !== ACCOUNT_STORE_VERSION || typeof stored.ciphertext !== "string") throw new Error("Invalid account store") const decrypted = safeStorage.decryptString(Buffer.from(stored.ciphertext, "base64")) - cachedAccounts = parseStoredSecretsById(JSON.parse(decrypted)) - } catch { - cachedAccounts = new Map() + const payload: unknown = JSON.parse(decrypted) + // An entry parseStoredSecretsById itself drops is not corruption: a file holding one + // broken entry beside three good ones is still a store worth writing to. A payload with + // no accounts list at all is the file failing to be this store's shape in the first place. + if (!isRecord(payload) || !Array.isArray(payload.accounts)) throw new Error("Invalid account store payload") + cachedRead = { accounts: parseStoredSecretsById(payload), unreadable: false } + } catch (error) { + cachedRead = { accounts: new Map(), unreadable: !isMissingFileError(error) } } - return cachedAccounts + return cachedRead +} + +async function readAccounts(): Promise> { + return (await readStore()).accounts +} + +/** + * Copies the current, unreadable store file aside once, so a rebuild around a + * new login never destroys bytes that might still hold other accounts' + * sessions. `overwrite: false` keeps the first snapshot: the earliest + * unreadable file is the one most likely to still hold every account that + * was ever saved, and a later corruption event has nothing left to add. + * + * Throws {@link AccountStoreUnreadableError} when the copy itself fails (a + * permissions problem, most likely): that is the one case where proceeding + * to rebuild would destroy bytes rather than merely fail to read them, so + * the caller must not write anything. + */ +async function preserveUnreadableStore(): Promise { + try { + await fse.copy(getAccountStorePath(), getUnreadableStoreBackupPath(), { overwrite: false, errorOnExist: false }) + } catch (error) { + if (isMissingFileError(error)) return // Vanished since the read; nothing left to preserve. + throw new AccountStoreUnreadableError(error) + } } /** * Encrypts and writes the whole account map, atomically. * - * If the file on disk exists but cannot be decrypted, this still overwrites - * it: the old bytes are already unrecoverable, so refusing to write would - * only leave the launcher permanently unable to save any account. At - * multi-account scale this means one undecryptable file now costs every - * saved account's session instead of the one account the old single-account - * store held, which is a real change in blast radius worth this comment, - * even though it is not a new kind of risk. + * Callers decide what "the whole account map" should contain before calling + * this; it always overwrites the file with exactly what it is given. See + * {@link saveAccountSecrets} for the policy on when that is safe to do. */ async function writeAccounts(accounts: Map): Promise { assertSecureStorage() @@ -95,14 +156,39 @@ async function writeAccounts(accounts: Map): Promise undefined) - cachedAccounts = accounts + cachedRead = { accounts, unreadable: false } } -/** Saves or replaces one account's secrets. Logging into an already-saved account overwrites its entry: a session refresh, not a duplicate. */ -export async function saveAccountSecrets(accountId: string, secrets: AccountSecrets): Promise { - const accounts = new Map(await readAccounts()) +/** What {@link saveAccountSecrets} actually did: a plain save, or a save that first had to rebuild an unreadable store around it. */ +export type AccountSaveOutcome = "saved" | "saved-after-rebuild" + +/** + * Saves or replaces one account's secrets. Logging into an already-saved + * account overwrites its entry: a session refresh, not a duplicate. + * + * When the store is present but unreadable, the sessions it held are already + * gone the moment it stopped decrypting: refusing this write would not bring + * any of them back, only leave the launcher unable to save any account ever + * again, with nothing in the app to clear the dead file for it. So this + * preserves the unreadable bytes once (see {@link preserveUnreadableStore}) + * and rebuilds the store around just the account logging in now, the same + * "never destroy without a snapshot" rule the rest of this file already + * follows. The caller is told which happened, so a login that quietly wiped + * a housemate's session is never reported as an ordinary one. + */ +export async function saveAccountSecrets(accountId: string, secrets: AccountSecrets): Promise { + const store = await readStore() + const accounts = new Map(store.accounts) accounts.set(accountId, secrets) + + if (!store.unreadable) { + await writeAccounts(accounts) + return "saved" + } + + await preserveUnreadableStore() await writeAccounts(accounts) + return "saved-after-rebuild" } /** Reads one account's secrets, or null when nothing is stored for it. */ @@ -117,6 +203,11 @@ export async function getAccountSecrets(accountId: string): Promise { const accounts = new Map(await readAccounts()) @@ -125,7 +216,7 @@ export async function removeAccountSecrets(accountId: string): Promise try { if (accounts.size === 0) { await fse.remove(getAccountStorePath()) - cachedAccounts = new Map() + cachedRead = { accounts: new Map(), unreadable: false } } else { await writeAccounts(accounts) } diff --git a/src/ipc/handlers/accountHandlers.ts b/src/ipc/handlers/accountHandlers.ts index 9298bb89..370edee1 100644 --- a/src/ipc/handlers/accountHandlers.ts +++ b/src/ipc/handlers/accountHandlers.ts @@ -2,13 +2,14 @@ import { ipcMain } from "electron" import { interpretFirstPass, interpretSecondPass } from "@domain/account/login" import type { LoginVerdict } from "@domain/account/login" -import { badCredentialsResult, needsTwoFactorResult, twoFactorRejectedResult, unexpectedResponseOutcome } from "@src/ipc/handlers/accountLoginOutcome" +import { badCredentialsResult, needsTwoFactorResult, sessionStoreUnreadableResult, twoFactorRejectedResult, unexpectedResponseOutcome } from "@src/ipc/handlers/accountLoginOutcome" import { buildLoginRequestBody } from "@src/ipc/handlers/loginRequestBody" import { IPC_CHANNELS } from "@src/ipc/ipcChannels" import { assertTrustedIpcSender } from "@src/ipc/ipcSecurity" import { requestBoundedTextViaNode } from "@src/ipc/network" import { assertString, MAX_LOGIN_RESPONSE_BYTES } from "@src/ipc/validation" -import { removeAccountSecrets, saveAccountSecrets } from "@src/ipc/accountStore" +import { AccountStoreUnreadableError, removeAccountSecrets, saveAccountSecrets } from "@src/ipc/accountStore" +import type { AccountSaveOutcome } from "@src/ipc/accountStore" import { getErrorMessage, logMessage } from "@src/utils/logManager" const LOGIN_URL = new URL("https://auth3.vintagestory.at/v2/gamelogin") @@ -54,12 +55,28 @@ async function requestLoginPass(email: string, password: string, twoFactorCode?: */ async function settle(verdict: LoginVerdict): Promise { switch (verdict.status) { - case "success": + case "success": { // Keyed by the uid this same response just carried, so the store can never // disagree with what the renderer is told. Logging into an account already // saved overwrites its entry in place: a session refresh, not a duplicate. - await saveAccountSecrets(verdict.credentials.publicAccount.playerUid, verdict.credentials.secrets) - return { status: "success", account: verdict.credentials.publicAccount } + let outcome: AccountSaveOutcome + try { + outcome = await saveAccountSecrets(verdict.credentials.publicAccount.playerUid, verdict.credentials.secrets) + } catch (error) { + // Only the narrow "could not even copy the unreadable file aside" case gets its own + // wire status: anything else (no keyring, disk full) is a genuine storage failure and + // falls through to the caller's catch, same as before this file could rebuild a store. + if (!(error instanceof AccountStoreUnreadableError)) throw error + logMessage("error", "[back] [ipc] [accountHandlers.ts] [LOGIN] The account store is unreadable and could not be copied aside, so it was left untouched. The session was not saved.") + logMessage("debug", `[back] [ipc] [accountHandlers.ts] [LOGIN] ${getErrorMessage(error)}`) + return sessionStoreUnreadableResult() + } + + if (outcome === "saved-after-rebuild") + logMessage("warn", "[back] [ipc] [accountHandlers.ts] [LOGIN] The account store could not be read; it was copied aside and rebuilt around this login. Other saved accounts must log in again.") + + return { status: "success", account: verdict.credentials.publicAccount, ...(outcome === "saved-after-rebuild" ? { storeRebuilt: true } : {}) } + } case "needs-two-factor": return needsTwoFactorResult() case "two-factor-rejected": diff --git a/src/ipc/handlers/accountLoginOutcome.ts b/src/ipc/handlers/accountLoginOutcome.ts index 3be5a62e..5e4ab828 100644 --- a/src/ipc/handlers/accountLoginOutcome.ts +++ b/src/ipc/handlers/accountLoginOutcome.ts @@ -44,3 +44,8 @@ export function unexpectedResponseOutcome(verdict: UnreadableResponse): { result logMessage: `Login response claimed success but could not be read${verdict.diagnosis ? `: ${verdict.diagnosis}` : ""}.` } } + +/** The credentials were accepted, but the local secret store could not be read or safely rebuilt, so nothing was saved. */ +export function sessionStoreUnreadableResult(): AccountLoginResult { + return { status: "session-store-unreadable" } +} diff --git a/src/ipc/handlers/gameHandlers.ts b/src/ipc/handlers/gameHandlers.ts index 3a05e555..af58e508 100644 --- a/src/ipc/handlers/gameHandlers.ts +++ b/src/ipc/handlers/gameHandlers.ts @@ -89,7 +89,12 @@ function realJsonFile(): JsonFile { */ async function adoptRefreshedSession(accountId: string, secrets: AccountSecrets): Promise { try { - await saveAccountSecrets(accountId, secrets) + const outcome = await saveAccountSecrets(accountId, secrets) + if (outcome === "saved-after-rebuild") + logMessage( + "warn", + `[back] [ipc] [ipc/handlers/gameHandlers.ts] [EXECUTE_GAME] The account store could not be read; it was copied aside and rebuilt around this adoption. Other saved accounts must log in again.` + ) logMessage("info", `[back] [ipc] [ipc/handlers/gameHandlers.ts] [EXECUTE_GAME] The game had already refreshed this account's session. Adopted it instead of overwriting it.`) } catch (err) { logMessage("error", `[back] [ipc] [ipc/handlers/gameHandlers.ts] [EXECUTE_GAME] Could not store the session the game refreshed. Launching anyway.`) diff --git a/src/renderer/src/components/ui/SessionButton.tsx b/src/renderer/src/components/ui/SessionButton.tsx index ea700d65..5d6db5ed 100644 --- a/src/renderer/src/components/ui/SessionButton.tsx +++ b/src/renderer/src/components/ui/SessionButton.tsx @@ -61,8 +61,10 @@ function SessionButton(): JSX.Element { if (result.status === "invalid-credentials") return addNotification(t("features.config.invalidEmailPass"), "error") if (result.status === "requires-two-factor") return addNotification(t("features.config.wrongtwofa"), "error") if (result.status === "unexpected-response") return addNotification(t("features.config.unexpectedResponse"), "error") + if (result.status === "session-store-unreadable") return addNotification(t("features.config.sessionStoreUnreadable"), "error") if (result.status !== "success") return + if (result.storeRebuilt) addNotification(t("features.config.sessionStoreRebuilt"), "warning") await saveLogin(result.account) } catch { // A throw here means the request never produced a verdict (network down, diff --git a/src/renderer/src/locales/en-US.json b/src/renderer/src/locales/en-US.json index 7b3f4199..af500ee2 100644 --- a/src/renderer/src/locales/en-US.json +++ b/src/renderer/src/locales/en-US.json @@ -275,6 +275,8 @@ "invalidEmailPass": "Invalid email or password!", "loginUnreachable": "Couldn't reach the login service. Check your connection or firewall and try again.", "unexpectedResponse": "The account service answered in a way this launcher couldn't read. Your password was not rejected. Try again after an update.", + "sessionStoreUnreadable": "Your login worked, but the saved-accounts file can't be read and couldn't be backed up, so the launcher left it alone rather than overwrite it. Nothing was saved this time.", + "sessionStoreRebuilt": "The saved-accounts file couldn't be read. A copy was kept and the launcher started a fresh one, so any other accounts on this device need to log in again.", "loggedin": "Logged in as {{user}}!", "onlyIfEnabledTwoFA": "Fill this field only if you have 2FA enabled!", "uiScale": "UI Scale", diff --git a/tests/ipc/accountHandlers.test.ts b/tests/ipc/accountHandlers.test.ts index 92a45fd0..60a08190 100644 --- a/tests/ipc/accountHandlers.test.ts +++ b/tests/ipc/accountHandlers.test.ts @@ -10,7 +10,7 @@ import "./helpers/electronMock" import { createTrustedEvent, createUntrustedEvent, getIpcHandler, setElectronUserDataPath } from "./helpers/electronMock" import { IPC_CHANNELS } from "@src/ipc/ipcChannels" -import { removeAccountSecrets, saveAccountSecrets } from "@src/ipc/accountStore" +import { AccountStoreUnreadableError, removeAccountSecrets, saveAccountSecrets } from "@src/ipc/accountStore" import { requestBoundedTextViaNode } from "@src/ipc/network" /** @@ -40,10 +40,16 @@ vi.mock("@src/ipc/network", () => ({ requestBoundedTextViaNode: vi.fn() })) -vi.mock("@src/ipc/accountStore", () => ({ - saveAccountSecrets: vi.fn(async () => undefined), - removeAccountSecrets: vi.fn(async () => true) -})) +vi.mock("@src/ipc/accountStore", async (importOriginal) => { + // The real AccountStoreUnreadableError, not a stand-in: accountHandlers.ts's `instanceof` + // check on it has to see the same class the tests below throw. + const actual = await importOriginal() + return { + AccountStoreUnreadableError: actual.AccountStoreUnreadableError, + saveAccountSecrets: vi.fn(async () => "saved" as const), + removeAccountSecrets: vi.fn(async () => true) + } +}) import "@src/ipc/handlers/accountHandlers" @@ -94,7 +100,7 @@ beforeEach(async () => { userDataFolder = mkdtempSync(join(tmpdir(), "rift-account-handlers-test-")) setElectronUserDataPath(userDataFolder) vi.mocked(requestBoundedTextViaNode).mockReset() - vi.mocked(saveAccountSecrets).mockReset().mockResolvedValue(undefined) + vi.mocked(saveAccountSecrets).mockReset().mockResolvedValue("saved") vi.mocked(removeAccountSecrets).mockReset().mockResolvedValue(true) trustedEvent = await createTrustedEvent() }) @@ -273,6 +279,30 @@ describe("LOGIN", () => { // would leave a session that vanishes on the next start. await assert.rejects(loginHandler()(trustedEvent, EMAIL, PASSWORD), /Login failed/) }) + + it("flags a login that rebuilt the account store, rather than reporting an ordinary success", async () => { + transportAnswers(SUCCESS_BODY) + vi.mocked(saveAccountSecrets).mockResolvedValueOnce("saved-after-rebuild") + + const result = await loginHandler()(trustedEvent, EMAIL, PASSWORD) + + assert.deepEqual(result, { + status: "success", + account: { email: EMAIL, playerName: "Placeholder Player", playerUid: "placeholder-uid", playerEntitlements: "singleplayer", hostGameServer: false }, + storeRebuilt: true + }) + }) + + it("reports an unreadable, unpreservable store as its own status instead of a generic failure", async () => { + transportAnswers(SUCCESS_BODY) + vi.mocked(saveAccountSecrets).mockRejectedValueOnce(new AccountStoreUnreadableError(new Error("EACCES"))) + + // A login this cannot even preserve the old bytes for is neither an ordinary success (the + // session was not saved) nor "Login failed" (the credentials were fine): the player is told + // exactly what happened instead of either lie. + const result = await loginHandler()(trustedEvent, EMAIL, PASSWORD) + assert.deepEqual(result, { status: "session-store-unreadable" }) + }) }) describe("REMOVE_ACCOUNT", () => { diff --git a/tests/ipc/accountLoginOutcome.test.ts b/tests/ipc/accountLoginOutcome.test.ts index 65d9a111..fd2c40ce 100644 --- a/tests/ipc/accountLoginOutcome.test.ts +++ b/tests/ipc/accountLoginOutcome.test.ts @@ -1,13 +1,14 @@ import assert from "node:assert/strict" import { describe, it } from "vitest" -import { badCredentialsResult, needsTwoFactorResult, twoFactorRejectedResult, unexpectedResponseOutcome } from "../../src/ipc/handlers/accountLoginOutcome" +import { badCredentialsResult, needsTwoFactorResult, sessionStoreUnreadableResult, twoFactorRejectedResult, unexpectedResponseOutcome } from "../../src/ipc/handlers/accountLoginOutcome" -describe("badCredentialsResult / needsTwoFactorResult / twoFactorRejectedResult", () => { +describe("badCredentialsResult / needsTwoFactorResult / twoFactorRejectedResult / sessionStoreUnreadableResult", () => { it("carry the domain verdict onto the wire unchanged", () => { assert.deepEqual(badCredentialsResult(), { status: "invalid-credentials" }) assert.deepEqual(needsTwoFactorResult(), { status: "requires-two-factor" }) assert.deepEqual(twoFactorRejectedResult(), { status: "wrong-two-factor" }) + assert.deepEqual(sessionStoreUnreadableResult(), { status: "session-store-unreadable" }) }) }) diff --git a/tests/ipc/accountStore.test.ts b/tests/ipc/accountStore.test.ts index 3fd65ec4..fdc79bcc 100644 --- a/tests/ipc/accountStore.test.ts +++ b/tests/ipc/accountStore.test.ts @@ -76,6 +76,10 @@ function backupPath(): string { return join(mockState.userDataDir, "account-secrets.pre-migration.bak.json") } +function unreadableBackupPath(): string { + return join(mockState.userDataDir, "account-secrets.unreadable.bak.json") +} + /** Writes the store file directly, standing in for whatever left it in that state. */ function writeStoreFile(contents: unknown): void { writeFileSync(storePath(), typeof contents === "string" ? contents : JSON.stringify(contents)) @@ -187,6 +191,107 @@ describe("saveAccountSecrets", () => { }) }) +/** + * #259: a store present but unreadable used to be indistinguishable from an absent one, so the + * next `saveAccountSecrets` silently overwrote it, losing every other account's session in one + * shot instead of just the one the old single-account store would have held. These pin the fix: + * the bytes are preserved once, the write still lands so the player logging in is not blocked, + * and the caller is told a rebuild happened rather than an ordinary save. + */ +describe("saveAccountSecrets rebuilding an unreadable store", () => { + it("copies an undecryptable store aside before rebuilding it around the new login", async () => { + const original = JSON.stringify({ version: 2, ciphertext: Buffer.from("someone else's bytes", "utf8").toString("base64") }) + writeStoreFile(original) + const store = await loadStore() + + const outcome = await store.saveAccountSecrets("uid-a", ACCOUNT_A) + + assert.equal(outcome, "saved-after-rebuild") + assert.equal(readFileSync(unreadableBackupPath(), "utf8"), original, "the backup holds the exact bytes that were there, not a re-serialised guess at them") + + const reader = await loadStore() + assert.deepEqual(await reader.getAccountSecrets("uid-a"), ACCOUNT_A) + }) + + it("does the same for a file that is not JSON at all", async () => { + writeStoreFile("{ not json at all") + const store = await loadStore() + + assert.equal(await store.saveAccountSecrets("uid-a", ACCOUNT_A), "saved-after-rebuild") + assert.equal(existsSync(unreadableBackupPath()), true) + }) + + it("does the same for a store written by a version this build does not know", async () => { + writeStoreFile({ version: 3, ciphertext: Buffer.from("sealed:{}", "utf8").toString("base64") }) + const store = await loadStore() + + assert.equal(await store.saveAccountSecrets("uid-a", ACCOUNT_A), "saved-after-rebuild") + assert.equal(existsSync(unreadableBackupPath()), true) + }) + + it("treats a store whose entries it merely dropped as readable, not unreadable", async () => { + // Same fixture as the "drops one unreadable entry" case above: one bad entry beside one + // good one is a store worth writing to, not a store worth rebuilding around. + writeStoreFile({ + version: 2, + ciphertext: Buffer.from( + `sealed:${JSON.stringify({ + accounts: [ + { id: "uid-a", secrets: { sessionKey: "only-a-key" } }, + { id: "uid-b", secrets: ACCOUNT_B } + ] + })}`, + "utf8" + ).toString("base64") + }) + const store = await loadStore() + + assert.equal(await store.saveAccountSecrets("uid-a", ACCOUNT_A), "saved") + assert.equal(existsSync(unreadableBackupPath()), false) + + const reader = await loadStore() + assert.deepEqual(await reader.getAccountSecrets("uid-a"), ACCOUNT_A) + assert.deepEqual(await reader.getAccountSecrets("uid-b"), ACCOUNT_B, "the account the corrupt-entry check let through survives the save too") + }) + + it("keeps the first unreadable snapshot rather than overwriting it on a later corruption", async () => { + writeFileSync(unreadableBackupPath(), JSON.stringify({ sentinel: "already there" })) + writeStoreFile({ version: 2, ciphertext: Buffer.from("someone else's bytes", "utf8").toString("base64") }) + const store = await loadStore() + + await store.saveAccountSecrets("uid-a", ACCOUNT_A) + + assert.equal(readFileSync(unreadableBackupPath(), "utf8"), JSON.stringify({ sentinel: "already there" })) + }) + + it("never collides with the pre-migration backup file", async () => { + writeFileSync(backupPath(), JSON.stringify({ sentinel: "pre-migration snapshot" })) + writeStoreFile({ version: 2, ciphertext: Buffer.from("someone else's bytes", "utf8").toString("base64") }) + const store = await loadStore() + + await store.saveAccountSecrets("uid-a", ACCOUNT_A) + + assert.equal(readFileSync(backupPath(), "utf8"), JSON.stringify({ sentinel: "pre-migration snapshot" }), "the migration backup is a different event and a different file") + assert.equal(existsSync(unreadableBackupPath()), true) + }) + + it.skipIf(process.platform !== "linux" || process.getuid?.() === 0)("refuses to rebuild a store it cannot even copy aside", async () => { + const original = JSON.stringify({ version: 2, ciphertext: Buffer.from("someone else's bytes", "utf8").toString("base64") }) + writeStoreFile(original) + chmodSync(mockState.userDataDir, 0o500) // read the file, but fse.copy cannot create a new one here + const store = await loadStore() + + try { + await assert.rejects(store.saveAccountSecrets("uid-a", ACCOUNT_A), /could not be preserved/) + } finally { + chmodSync(mockState.userDataDir, 0o700) + } + + assert.equal(existsSync(unreadableBackupPath()), false) + assert.equal(readFileSync(storePath(), "utf8"), original, "nothing was written over a file this could not back up first") + }) +}) + describe("getAccountSecrets", () => { it("answers null when nothing was ever stored", async () => { const store = await loadStore() diff --git a/tests/ipc/configManager.test.ts b/tests/ipc/configManager.test.ts index 71fe7ee1..71d79af8 100644 --- a/tests/ipc/configManager.test.ts +++ b/tests/ipc/configManager.test.ts @@ -29,7 +29,7 @@ import { DEFAULT_COMPRESSION_LEVEL } from "@domain/config/defaults" * import, not after. */ vi.mock("@src/ipc/accountStore", () => ({ - saveAccountSecrets: vi.fn(async () => undefined), + saveAccountSecrets: vi.fn(async () => "saved" as const), adoptLegacySingleAccountSecrets: vi.fn(async () => false) })) @@ -60,7 +60,7 @@ beforeEach(() => { setElectronPath("appRoot", join(temporaryRoot, "app")) vi.mocked(saveAccountSecrets).mockReset() - vi.mocked(saveAccountSecrets).mockResolvedValue(undefined) + vi.mocked(saveAccountSecrets).mockResolvedValue("saved") vi.mocked(adoptLegacySingleAccountSecrets).mockReset() vi.mocked(adoptLegacySingleAccountSecrets).mockResolvedValue(false) }) diff --git a/tests/ipc/gameHandlers.test.ts b/tests/ipc/gameHandlers.test.ts index 5e290abf..f48bd579 100644 --- a/tests/ipc/gameHandlers.test.ts +++ b/tests/ipc/gameHandlers.test.ts @@ -39,7 +39,7 @@ import { writeJsonAtomic } from "@src/ipc/atomicJsonFile" */ vi.mock("@src/ipc/accountStore", () => ({ getAccountSecrets: vi.fn(async () => ({ mptoken: null, sessionKey: "session-key", sessionSignature: "session-signature" })), - saveAccountSecrets: vi.fn(async () => undefined), + saveAccountSecrets: vi.fn(async () => "saved" as const), adoptLegacySingleAccountSecrets: vi.fn(async () => false) })) diff --git a/tests/renderer-dom/sessionButtonStoreRebuilt.test.tsx b/tests/renderer-dom/sessionButtonStoreRebuilt.test.tsx new file mode 100644 index 00000000..c2ad0c97 --- /dev/null +++ b/tests/renderer-dom/sessionButtonStoreRebuilt.test.tsx @@ -0,0 +1,79 @@ +import { describe, expect, it, vi } from "vitest" +import { screen } from "@testing-library/react" +import userEvent from "@testing-library/user-event" + +import SessionButton from "@renderer/components/ui/SessionButton" +import NotificationsOverlay from "@renderer/components/layout/NotificationsOverlay" + +import { installMockWindowApi } from "./helpers/windowApi" +import { renderWithProviders } from "./helpers/render" + +const ACCOUNT = { email: "player@example.test", playerName: "Player", playerUid: "uid-a", playerEntitlements: null, hostGameServer: false } + +async function login(email: string, password: string): Promise { + const user = userEvent.setup() + await user.click(await screen.findByRole("button", { name: "Log in" })) + await user.type(screen.getByPlaceholderText("Email"), email) + await user.type(screen.getByPlaceholderText("Password"), password) + await user.click(screen.getByRole("button", { name: "Add" })) +} + +/** + * #259: a login whose credentials the service accepted, but whose secrets + * `saveAccountSecrets` could only save by rebuilding an unreadable store, or + * could not save at all. Neither is an ordinary success and neither is + * "invalid email or password": the player is told exactly what happened. + */ +describe("SessionButton on an account-store rebuild", () => { + it("logs the player in and warns that other saved accounts must log in again", async () => { + const loginFn = vi.fn(async () => ({ status: "success", account: ACCOUNT, storeRebuilt: true }) as AccountLoginResult) + installMockWindowApi({ accountManager: { login: loginFn } }) + + renderWithProviders( + <> + + + + ) + + await login("player@example.test", "correct-horse-battery-staple") + + expect(await screen.findByText(/logged in as player/i)).toBeTruthy() + expect(await screen.findByText(/couldn't be read/i)).toBeTruthy() + }) + + it("does not warn about a rebuild on an ordinary success", async () => { + const loginFn = vi.fn(async () => ({ status: "success", account: ACCOUNT }) as AccountLoginResult) + installMockWindowApi({ accountManager: { login: loginFn } }) + + renderWithProviders( + <> + + + + ) + + await login("player@example.test", "correct-horse-battery-staple") + + expect(await screen.findByText(/logged in as player/i)).toBeTruthy() + expect(screen.queryByText(/couldn't be read/i)).toBeNull() + }) + + it("tells the player their login worked but nothing could be saved, when the store could not even be backed up", async () => { + const loginFn = vi.fn(async () => ({ status: "session-store-unreadable" }) as AccountLoginResult) + installMockWindowApi({ accountManager: { login: loginFn } }) + + renderWithProviders( + <> + + + + ) + + await login("player@example.test", "correct-horse-battery-staple") + + expect(await screen.findByText(/your login worked/i)).toBeTruthy() + expect(screen.queryByText(/invalid email or password/i)).toBeNull() + expect(screen.queryByText(/logged in as/i)).toBeNull() + }) +}) From 1f2dd31858fed7aa7be7fafd7d92f7476c693230 Mon Sep 17 00:00:00 2001 From: Zaldaryon <273555259+Zaldaryon@users.noreply.github.com> Date: Thu, 27 Aug 2026 19:31:47 -0300 Subject: [PATCH 2/2] fix(account): refuse account removal when the store is unreadable removeAccountSecrets returned true when the account was absent, including when it was absent only because the store could not be read (a locked keyring, or bytes that stopped decrypting). The renderer took that as success, dropped the account from config, and told the player it was gone while its session sat on disk under a uid nothing named any more. It now returns readStore().unreadable => false in that case, so the renderer keeps the account and shows the store error instead. No rebuild: only saveAccountSecrets does that. Adds the locked-keyring regression test #261's review asked for: an intact v2 store plus an unavailable keyring must leave the file byte-for-byte, at its original mode, and must not create the one-shot unreadable snapshot. Reworks the preserveUnreadableStore comment to state the first-snapshot-wins trade is deliberate and accepted rather than resting it on a 'most likely'. PR #261 review. --- src/ipc/accountStore.ts | 40 +++++++++++++++++++++------------- tests/ipc/accountStore.test.ts | 35 +++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 15 deletions(-) diff --git a/src/ipc/accountStore.ts b/src/ipc/accountStore.ts index 60b5d95a..1ea7c29d 100644 --- a/src/ipc/accountStore.ts +++ b/src/ipc/accountStore.ts @@ -118,9 +118,14 @@ async function readAccounts(): Promise> { /** * Copies the current, unreadable store file aside once, so a rebuild around a * new login never destroys bytes that might still hold other accounts' - * sessions. `overwrite: false` keeps the first snapshot: the earliest - * unreadable file is the one most likely to still hold every account that - * was ever saved, and a later corruption event has nothing left to add. + * sessions. `overwrite: false` keeps the first snapshot and skips every later + * one. That is a deliberate one-shot: a second corruption event can land after + * the store has rebuilt and grown, so the skipped snapshot sometimes holds + * more than the kept one, which for a version-mismatch file (recoverable by + * the newer build that wrote it) is a real loss. The trade is accepted here + * because a single predictable recovery file beats an unbounded pile of them, + * and nothing in the app surfaces or clears these yet regardless (tracked as a + * follow-up on #259). * * Throws {@link AccountStoreUnreadableError} when the copy itself fails (a * permissions problem, most likely): that is the one case where proceeding @@ -197,21 +202,26 @@ export async function getAccountSecrets(accountId: string): Promise { - const accounts = new Map(await readAccounts()) - if (!accounts.delete(accountId)) return true + const store = await readStore() + const accounts = new Map(store.accounts) + if (!accounts.delete(accountId)) return !store.unreadable try { if (accounts.size === 0) { diff --git a/tests/ipc/accountStore.test.ts b/tests/ipc/accountStore.test.ts index fdc79bcc..e2e24d65 100644 --- a/tests/ipc/accountStore.test.ts +++ b/tests/ipc/accountStore.test.ts @@ -290,6 +290,28 @@ describe("saveAccountSecrets rebuilding an unreadable store", () => { assert.equal(existsSync(unreadableBackupPath()), false) assert.equal(readFileSync(storePath(), "utf8"), original, "nothing was written over a file this could not back up first") }) + + it("does not snapshot or touch an intact store when only the keyring is locked", async () => { + // A locked keyring reads as unreadable-adjacent, but the file is fine: readStore returns + // early with unreadable:false so a later unlock still reaches it, and writeAccounts throws + // before it could overwrite anything. Without that split, the first locked-keyring login + // would copy the intact store to the one-shot snapshot slot and then fail the login anyway, + // stranding a stale copy where a genuine corruption event would later need one (#261 review). + const writer = await loadStore() + await writer.saveAccountSecrets("uid-a", ACCOUNT_A) + await writer.saveAccountSecrets("uid-b", ACCOUNT_B) + const onDisk = readFileSync(storePath(), "utf8") + const mode = statSync(storePath()).mode & 0o777 + + mockState.encryptionAvailable = false + const store = await loadStore() + + await assert.rejects(store.saveAccountSecrets("uid-c", ACCOUNT_A), /Secure account storage is unavailable/) + + assert.equal(existsSync(unreadableBackupPath()), false, "a locked keyring is not a corruption event") + assert.equal(readFileSync(storePath(), "utf8"), onDisk, "the real store is left byte-for-byte") + assert.equal(statSync(storePath()).mode & 0o777, mode, "and at the mode it had") + }) }) describe("getAccountSecrets", () => { @@ -457,6 +479,19 @@ describe("removeAccountSecrets", () => { assert.equal(await store.removeAccountSecrets("uid-a"), true) }) + it("reports failure, not a false success, when the store cannot be read", async () => { + // A locked keyring, or a file that stopped decrypting. The account asked for could be in + // those very bytes, so "nothing to remove" is not the truth. A false success would have + // the renderer drop it from config and tell the player it is gone (#253 review). + const original = JSON.stringify({ version: 2, ciphertext: Buffer.from("someone else's bytes", "utf8").toString("base64") }) + writeFileSync(storePath(), original) + const store = await loadStore() + + assert.equal(await store.removeAccountSecrets("uid-a"), false) + assert.equal(readFileSync(storePath(), "utf8"), original, "the unreadable file is left exactly as it was") + assert.equal(existsSync(unreadableBackupPath()), false, "removing an account never snapshots or rebuilds the store") + }) + it.skipIf(process.platform !== "linux" || process.getuid?.() === 0)("reports failure when the file cannot be rewritten", async () => { const store = await loadStore() await store.saveAccountSecrets("uid-a", ACCOUNT_A)