From 8e89a078a7aaebd51e0b0c9bc6384b5663f12974 Mon Sep 17 00:00:00 2001 From: saopig1 <4x7sw862st@gmail.com> Date: Sat, 4 Jul 2026 11:31:00 +0800 Subject: [PATCH] fix(spotify): atomic OAuth token store write so a crash during rotating refresh can't corrupt/lose it [corner-case R3-5] Co-Authored-By: Claude Opus 4.8 (1M context) --- src/music/spotify/spotify-oauth.test.ts | 111 +++++++++++++++++++++++- src/music/spotify/spotify-oauth.ts | 26 +++++- 2 files changed, 134 insertions(+), 3 deletions(-) diff --git a/src/music/spotify/spotify-oauth.test.ts b/src/music/spotify/spotify-oauth.test.ts index e4e59e6..406f39c 100644 --- a/src/music/spotify/spotify-oauth.test.ts +++ b/src/music/spotify/spotify-oauth.test.ts @@ -1,5 +1,12 @@ -import { describe, it, expect, vi } from "vitest"; -import { mkdtempSync, rmSync } from "node:fs"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { + mkdtempSync, + rmSync, + readdirSync, + readFileSync, + renameSync, + statSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -12,6 +19,20 @@ import { type OAuthTokenStore, } from "./spotify-oauth.js"; +// Wrap the fs functions the token store uses in call-through spies so the +// atomic-write path (temp file + rename) can be observed/forced. Everything else +// (mkdtemp, rmSync, readdir, …) is the real implementation via `...actual`, so +// all other tests keep 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, + writeFileSync: vi.fn(actual.writeFileSync), + renameSync: vi.fn(actual.renameSync), + }; +}); + // Correction C3.2: the control OAuth REQUIRES a user-provided client_id (their // own Spotify Developer app) + caller-supplied loopback redirect. There is NO // librespot public-client / :5588 default. @@ -478,3 +499,89 @@ describe("createFileOAuthTokenStore", () => { } }); }); + +// R3-5: the token store save() must be atomic (same-dir temp file + rename), so +// a crash / power loss / ENOSPC while persisting a ROTATED refresh token (Spotify +// invalidates the OLD one the instant it responds — the new one lives only in +// memory until this write lands) can never truncate the file and silently +// de-authenticate the operator. Mirrors the hardened config.ts saveConfig. +describe("createFileOAuthTokenStore atomic write (R3-5)", () => { + const dirs: string[] = []; + function makeTmpDir(): string { + const dir = mkdtempSync(join(tmpdir(), "sp-oauth-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; + }); + + const TOK: OAuthTokens = { + accessToken: "a", + refreshToken: "r", + expiresAt: 123, + scope: "s", + }; + + it("round-trips save/load and leaves NO .tmp file behind", () => { + const dir = makeTmpDir(); + const file = join(dir, "tokens.json"); + const store = createFileOAuthTokenStore(file); + + store.save(TOK); + + expect(store.load()).toEqual(TOK); + // 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 file = join(dir, "tokens.json"); + + createFileOAuthTokenStore(file).save(TOK); + + expect(vi.mocked(renameSync)).toHaveBeenCalled(); + const [from, to] = vi.mocked(renameSync).mock.calls[0] as [string, string]; + expect(to).toBe(file); // renamed ONTO the real path + expect(String(from)).not.toBe(file); // ...from a distinct temp file + expect(join(String(from), "..")).toBe(join(file, "..")); // ...in the SAME dir + }); + + it("persists the token file with 0600 permissions (POSIX)", () => { + if (process.platform === "win32") return; // mode bits aren't meaningful on Windows + const dir = makeTmpDir(); + const file = join(dir, "tokens.json"); + createFileOAuthTokenStore(file).save(TOK); + expect(statSync(file).mode & 0o777).toBe(0o600); + }); + + it("does NOT truncate/corrupt a pre-existing valid token file when the write fails mid-way", () => { + const dir = makeTmpDir(); + const file = join(dir, "tokens.json"); + const store = createFileOAuthTokenStore(file); + + // A valid, previously-persisted token file (the live refresh token on disk). + store.save(TOK); + const before = readFileSync(file, "utf-8"); + + // Simulate a crash / ENOSPC at the atomic-replace step while persisting a + // ROTATED refresh token. + const rotated: OAuthTokens = { ...TOK, accessToken: "a2", refreshToken: "r2" }; + vi.mocked(renameSync).mockImplementationOnce(() => { + throw new Error("rename boom"); + }); + + expect(() => store.save(rotated)).toThrow(/rename boom/); + + // The original file is untouched: present, byte-identical, still parseable. + expect(readFileSync(file, "utf-8")).toBe(before); + expect(store.load()).toEqual(TOK); + // ...and the failed write left no temp file lying around. + expect(readdirSync(dir).filter((f) => f.includes(".tmp"))).toEqual([]); + }); +}); diff --git a/src/music/spotify/spotify-oauth.ts b/src/music/spotify/spotify-oauth.ts index 3ac79ca..fbbc537 100644 --- a/src/music/spotify/spotify-oauth.ts +++ b/src/music/spotify/spotify-oauth.ts @@ -4,6 +4,7 @@ import { existsSync, mkdirSync, readFileSync, + renameSync, rmSync, writeFileSync, } from "node:fs"; @@ -81,7 +82,30 @@ export function createFileOAuthTokenStore(filePath: string): OAuthTokenStore { }, save(t: OAuthTokens) { mkdirSync(dirname(filePath), { recursive: true }); - writeFileSync(filePath, JSON.stringify(t, null, 2), { mode: 0o600 }); + // 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 the token file truncated — a reader always sees either the previous + // file or the fully-written new one, never a partial. This matters because + // refresh() persists a ROTATED refresh token here: Spotify invalidates the + // OLD one the instant it responds, so a torn write would silently + // de-authenticate the operator (next load() JSON.parse-fails -> null -> + // full PKCE re-login). Mirrors config.ts saveConfig. The temp lives in the + // same dir so the rename stays on one filesystem; pid + timestamp keep + // concurrent writers from colliding on the temp name. 0600 is preserved. + const tmp = `${filePath}.${process.pid}.${Date.now()}.tmp`; + try { + writeFileSync(tmp, JSON.stringify(t, null, 2), { mode: 0o600 }); + renameSync(tmp, filePath); + } catch (err) { + // Never leave a partial temp file behind on failure. + try { + rmSync(tmp, { force: true }); + } catch { + /* best-effort cleanup */ + } + throw err; + } }, clear() { try {