From 03690952cbc7052df819e943912e6de62c40208c Mon Sep 17 00:00:00 2001 From: saopig1 <4x7sw862st@gmail.com> Date: Fri, 3 Jul 2026 12:03:01 +0800 Subject: [PATCH] fix(config): atomic saveConfig + never overwrite a real config on transient/corrupt read [corner-case R2-1,R2-2] Co-Authored-By: Claude Opus 4.8 (1M context) --- src/data/config.test.ts | 153 +++++++++++++++++++++++++++++++++++++++- src/data/config.ts | 79 +++++++++++++++++++-- 2 files changed, 223 insertions(+), 9 deletions(-) diff --git a/src/data/config.test.ts b/src/data/config.test.ts index 3b30d60..287981d 100644 --- a/src/data/config.test.ts +++ b/src/data/config.test.ts @@ -1,9 +1,32 @@ -import { describe, it, expect, afterEach } from "vitest"; +import { describe, it, expect, afterEach, beforeEach, vi } from "vitest"; import { join } from "node:path"; -import { mkdtempSync, rmSync, writeFileSync, existsSync, readFileSync } from "node:fs"; +import { + mkdtempSync, + rmSync, + writeFileSync, + existsSync, + readFileSync, + readdirSync, + renameSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { getDefaultConfig, loadConfig, saveConfig, migrateLegacyConfig } from "./config.js"; +// Wrap the fs functions config.ts uses in call-through spies so the atomic-write +// and transient-read-error paths can be observed/forced. Everything else (mkdtemp, +// rmSync, existsSync, …) is the real implementation via `...actual`, so all other +// tests keep their real filesystem behavior. `vi.spyOn` can't be used here because +// the node:fs ESM namespace is non-configurable in this setup. +vi.mock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + readFileSync: vi.fn(actual.readFileSync), + writeFileSync: vi.fn(actual.writeFileSync), + renameSync: vi.fn(actual.renameSync), + }; +}); + describe("config", () => { const dirs: string[] = []; @@ -263,3 +286,129 @@ describe("spotify config", () => { }); }); }); + +// --- R2-1: saveConfig must write atomically (temp file + rename), never truncate --- + +describe("saveConfig atomic write", () => { + const dirs: string[] = []; + function makeTmpDir(): string { + const dir = mkdtempSync(join(tmpdir(), "tsmb-atomic-")); + dirs.push(dir); + return dir; + } + beforeEach(() => { + vi.clearAllMocks(); // reset call history, keep the call-through implementations + }); + afterEach(() => { + for (const d of dirs) rmSync(d, { recursive: true, force: true }); + dirs.length = 0; + }); + + it("round-trips (save then load equals) and leaves NO .tmp file behind", () => { + const dir = makeTmpDir(); + const path = join(dir, "config.json"); + const config = { ...getDefaultConfig(), webPort: 4567, adminPassword: "pw" }; + + saveConfig(path, config); + + expect(loadConfig(path)).toEqual(config); + // No temp remnants in the target directory. + expect(readdirSync(dir).filter((f) => f.includes(".tmp"))).toEqual([]); + }); + + it("writes via a same-dir temp file then renameSync onto the final path", () => { + const dir = makeTmpDir(); + const path = join(dir, "config.json"); + + saveConfig(path, getDefaultConfig()); + + expect(vi.mocked(renameSync)).toHaveBeenCalled(); + const [from, to] = vi.mocked(renameSync).mock.calls[0] as [string, string]; + expect(to).toBe(path); // renamed ONTO the real path + expect(String(from)).not.toBe(path); // ...from a distinct temp file + expect(join(String(from), "..")).toBe(join(path, "..")); // ...in the SAME directory + }); + + it("does not corrupt a pre-existing valid config when saving over it", () => { + const dir = makeTmpDir(); + const path = join(dir, "config.json"); + saveConfig(path, { ...getDefaultConfig(), adminPassword: "first", webPort: 1234 }); + + // Overwrite with a different, fully-formed config. + saveConfig(path, { ...getDefaultConfig(), adminPassword: "second", webPort: 9999 }); + + const loaded = loadConfig(path); + expect(loaded.adminPassword).toBe("second"); + expect(loaded.webPort).toBe(9999); + // The on-disk file is a single complete JSON document (no partial/truncated write). + expect(() => JSON.parse(readFileSync(path, "utf-8"))).not.toThrow(); + expect(readdirSync(dir).filter((f) => f.includes(".tmp"))).toEqual([]); + }); + + it("cleans up the temp file (no .tmp remnant) when the rename fails", () => { + const dir = makeTmpDir(); + const path = join(dir, "config.json"); + vi.mocked(renameSync).mockImplementationOnce(() => { + throw new Error("rename boom"); + }); + + expect(() => saveConfig(path, getDefaultConfig())).toThrow(/rename boom/); + // The failed write left no temp file lying around. + expect(readdirSync(dir).filter((f) => f.includes(".tmp"))).toEqual([]); + }); +}); + +// --- R2-2: loadConfig must not treat a transient/corrupt read as "missing" --- + +describe("loadConfig error handling", () => { + const dirs: string[] = []; + function makeTmpDir(): string { + const dir = mkdtempSync(join(tmpdir(), "tsmb-load-")); + dirs.push(dir); + return dir; + } + beforeEach(() => { + vi.clearAllMocks(); + }); + afterEach(() => { + for (const d of dirs) rmSync(d, { recursive: true, force: true }); + dirs.length = 0; + }); + + it("(a) ENOENT (missing file) returns defaults — unchanged first-run behavior", () => { + const dir = makeTmpDir(); + expect(loadConfig(join(dir, "config.json"))).toEqual(getDefaultConfig()); + }); + + it("(b) a non-ENOENT read error (EBUSY) rethrows instead of returning defaults", () => { + const dir = makeTmpDir(); + const path = join(dir, "config.json"); + // A REAL config exists on disk; a transient lock must NOT collapse to defaults + // (the caller would otherwise overwrite this real config with defaults). + saveConfig(path, { ...getDefaultConfig(), adminPassword: "keep-me" }); + vi.mocked(readFileSync).mockImplementationOnce(() => { + const err = new Error("EBUSY: resource busy or locked") as NodeJS.ErrnoException; + err.code = "EBUSY"; + throw err; + }); + + expect(() => loadConfig(path)).toThrow(/EBUSY/); + // The on-disk config is untouched and still readable once the lock clears. + expect(loadConfig(path).adminPassword).toBe("keep-me"); + }); + + it("(c) corrupt JSON returns defaults AND backs up the original to *.corrupt-*", () => { + const dir = makeTmpDir(); + const path = join(dir, "config.json"); + const garbage = "{ not: valid json, "; + writeFileSync(path, garbage, "utf-8"); + + const loaded = loadConfig(path); + + expect(loaded).toEqual(getDefaultConfig()); + const backups = readdirSync(dir).filter((f) => f.includes(".corrupt-")); + expect(backups.length).toBeGreaterThan(0); + // The corrupt original is preserved verbatim (recoverable, never deleted). + expect(readFileSync(join(dir, backups[0]), "utf-8")).toBe(garbage); + }); +}); diff --git a/src/data/config.ts b/src/data/config.ts index 3b708d6..61b79a2 100755 --- a/src/data/config.ts +++ b/src/data/config.ts @@ -1,4 +1,12 @@ -import { readFileSync, writeFileSync, mkdirSync, existsSync, copyFileSync, rmSync } from "node:fs"; +import { + readFileSync, + writeFileSync, + mkdirSync, + existsSync, + copyFileSync, + rmSync, + renameSync, +} from "node:fs"; import { dirname } from "node:path"; import type { BotAccess, GuestPermissions } from "./permissions.js"; import { GUEST_PERMISSION_FLAGS } from "./permissions.js"; @@ -92,10 +100,48 @@ export function getDefaultConfig(): BotConfig { export function loadConfig(path: string): BotConfig { const defaults = getDefaultConfig(); - try { - const raw = readFileSync(path, "utf-8"); - const partial = JSON.parse(raw) as Partial; + // Distinguish the three failure modes so a *real* on-disk config is NEVER + // silently replaced with defaults (the caller saveConfig()s right after load, + // which would otherwise erase spotify creds / adminPassword / adminGroups / + // guestMode permanently): + // (a) file ABSENT (ENOENT) — normal first run → defaults. + // (b) any OTHER read error (EBUSY/EACCES/EPERM/EISDIR/…) on an existing file — + // rethrow (fail-fast at boot). A loud crash beats silent credential loss. + // (c) file readable but JSON.parse fails (corrupt) — back the file up first + // (never delete it), THEN return defaults so boot can proceed. + let raw: string; + try { + raw = readFileSync(path, "utf-8"); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "ENOENT") { + return defaults; // (a) missing file — first run + } + throw err; // (b) transient/permission error on an existing file — do not clobber it + } + + let partial: Partial; + try { + partial = JSON.parse(raw) as Partial; + } catch { + // (c) Corrupt content: move the unreadable file aside to a timestamped backup + // so the data stays recoverable, then fall back to defaults. Prefer an atomic + // same-dir rename; if that fails, copy instead. If it can't be preserved at + // all, rethrow rather than let the caller overwrite unrecoverable data. + const backup = `${path}.corrupt-${Date.now()}`; + try { + renameSync(path, backup); + } catch { + try { + copyFileSync(path, backup); + } catch (backupErr) { + throw backupErr; + } + } + return defaults; + } + + { // Normalize/sanitize guestMode on load. The WRITE path (POST /api/bot/settings) // sanitizes too, but a hand-edited/legacy/corrupt config.json reaches the gate // directly — so coerce it here as well, mirroring that write-path logic. @@ -163,14 +209,33 @@ export function loadConfig(path: string): BotConfig { guestMode: gm, spotify, }; - } catch { - return defaults; } } export function saveConfig(path: string, config: BotConfig): void { mkdirSync(dirname(path), { recursive: true }); - writeFileSync(path, JSON.stringify(config, null, 2), "utf-8"); + const json = JSON.stringify(config, null, 2); + + // Atomic write: serialize to a sibling temp file in the SAME directory, then + // rename it onto the final path. rename is an atomic replace on POSIX and modern + // Windows, so a crash / power loss / ENOSPC mid-write can never leave config.json + // truncated — a reader always sees either the previous file or the fully-written + // new one, never a partial. The temp lives in the same dir so the rename stays on + // one filesystem (a cross-device rename would fail); pid + timestamp keep + // concurrent writers from colliding on the temp name. + const tmp = `${path}.${process.pid}.${Date.now()}.tmp`; + try { + writeFileSync(tmp, json, "utf-8"); + renameSync(tmp, path); + } catch (err) { + // Never leave a partial temp file behind on failure. + try { + rmSync(tmp, { force: true }); + } catch { + /* best-effort cleanup */ + } + throw err; + } } /**