diff --git a/src/data/config.test.ts b/src/data/config.test.ts index 287981d..afac5ba 100644 --- a/src/data/config.test.ts +++ b/src/data/config.test.ts @@ -411,4 +411,43 @@ describe("loadConfig error handling", () => { // The corrupt original is preserved verbatim (recoverable, never deleted). expect(readFileSync(join(dir, backups[0]), "utf-8")).toBe(garbage); }); + + // (d)/(e) Valid JSON that is NOT a non-null object (null / [] / 42 / "str") passes + // JSON.parse but would throw a raw TypeError in the per-field sanitize block + // (property access on a non-object), bypassing the corrupt-backup path. It must be + // treated EXACTLY like corrupt JSON: back up to *.corrupt-* (original preserved), + // return defaults — NOT a thrown TypeError, and NOT a silent defaults-with-no-backup. + it("(d) a `null` config is treated as corrupt: defaults + *.corrupt-* backup (original preserved)", () => { + const dir = makeTmpDir(); + const path = join(dir, "config.json"); + writeFileSync(path, "null", "utf-8"); + + let loaded: ReturnType; + expect(() => { + loaded = loadConfig(path); + }).not.toThrow(); + + expect(loaded!).toEqual(getDefaultConfig()); + const backups = readdirSync(dir).filter((f) => f.includes(".corrupt-")); + expect(backups.length).toBeGreaterThan(0); + expect(readFileSync(join(dir, backups[0]), "utf-8")).toBe("null"); + }); + + it("(e) a non-object config (`[]` / `42`) is backed up + defaults, not a thrown TypeError", () => { + for (const content of ["[]", "42"]) { + const dir = makeTmpDir(); + const path = join(dir, "config.json"); + writeFileSync(path, content, "utf-8"); + + let loaded: ReturnType; + expect(() => { + loaded = loadConfig(path); + }).not.toThrow(); + + expect(loaded!).toEqual(getDefaultConfig()); + const backups = readdirSync(dir).filter((f) => f.includes(".corrupt-")); + expect(backups.length).toBeGreaterThan(0); + expect(readFileSync(join(dir, backups[0]), "utf-8")).toBe(content); + } + }); }); diff --git a/src/data/config.ts b/src/data/config.ts index 61b79a2..ed8ff83 100755 --- a/src/data/config.ts +++ b/src/data/config.ts @@ -98,6 +98,26 @@ export function getDefaultConfig(): BotConfig { }; } +/** + * Move an unusable config aside to a timestamped `*.corrupt-*` backup so the data + * stays recoverable (it is NEVER deleted), for both the corrupt-JSON case and the + * parses-but-not-an-object case. 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. + */ +function backupCorruptConfig(path: string): void { + const backup = `${path}.corrupt-${Date.now()}`; + try { + renameSync(path, backup); + } catch { + try { + copyFileSync(path, backup); + } catch (backupErr) { + throw backupErr; + } + } +} + export function loadConfig(path: string): BotConfig { const defaults = getDefaultConfig(); @@ -120,27 +140,26 @@ export function loadConfig(path: string): BotConfig { throw err; // (b) transient/permission error on an existing file — do not clobber it } - let partial: Partial; + let parsed: unknown; try { - partial = JSON.parse(raw) as Partial; + parsed = JSON.parse(raw); } 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; - } - } + // so the data stays recoverable, then fall back to defaults. + backupCorruptConfig(path); return defaults; } + // (d) Parses cleanly but is NOT a non-null object (e.g. `null`, `42`, `"str"`, + // `[]`). The per-field sanitize below assumes an object and would throw a raw + // TypeError (or silently spread junk), bypassing the corrupt-backup path. Treat + // it EXACTLY like corrupt JSON: back it up (never delete), then return defaults. + if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { + backupCorruptConfig(path); + return defaults; + } + const partial = parsed as Partial; + { // 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 diff --git a/src/music/spotify/webapi.test.ts b/src/music/spotify/webapi.test.ts index bbc8068..0058a22 100644 --- a/src/music/spotify/webapi.test.ts +++ b/src/music/spotify/webapi.test.ts @@ -509,6 +509,39 @@ describe("getAlbumTracks pagination (R2-6)", () => { expect(out[50].id).toBe("a50"); // page 2 appended in order expect(out[52].id).toBe("a52"); }); + + // Robustness: a malformed/proxy response of { items: [], next: } never + // grows songs.length nor nulls `next`, so a while-loop bounded ONLY by + // songs.length >= cap spins forever. getAlbumTracks must have a hard offset/page + // bound (mirroring the playlist loop) so it TERMINATES regardless of items/next. + it("terminates on a { items: [], next: } response (bounded call count, no hang)", async () => { + const auth = { + post: vi.fn().mockResolvedValue({ data: { access_token: "t", expires_in: 3600 } }), + } as any; + const http = { + get: vi.fn().mockImplementation((path: string) => { + if (path === "/v1/albums/alb1") { + // Even the embedded first page is malformed: empty items, non-null next. + return Promise.resolve({ + data: { + name: "Broken", + images: [{ url: "https://i.scdn.co/broken.jpg" }], + tracks: { items: [], next: "http://x/next" }, + }, + }); + } + // Every /albums/{id}/tracks page keeps advertising a further page forever. + return Promise.resolve({ data: { items: [], next: "http://x/next" } }); + }), + } as any; + const api = new SpotifyWebApi(() => ({ clientId: "a", clientSecret: "b" }), { http, auth }); + const out = await api.getAlbumTracks("alb1"); + + // Returns what it has (nothing) rather than hanging. + expect(out).toEqual([]); + // Bounded: 1 album GET + at most MAX_ALBUM_TRACKS/ALBUM_PAGE_SIZE (=10) page fetches. + expect(http.get.mock.calls.length).toBeLessThanOrEqual(12); + }); }); // R2-7: search() must null-filter tracks.items like its album/playlist siblings so diff --git a/src/music/spotify/webapi.ts b/src/music/spotify/webapi.ts index 1c954b9..828a5df 100644 --- a/src/music/spotify/webapi.ts +++ b/src/music/spotify/webapi.ts @@ -185,7 +185,11 @@ export class SpotifyWebApi { // Page 1 (up to 50 tracks) is embedded in the album payload; the embedded // paging object caps at 50, so follow its `next` via the dedicated - // /albums/{id}/tracks endpoint until exhausted or the cap is reached. + // /albums/{id}/tracks endpoint until exhausted or the cap is reached. The + // offset bound is a HARD termination guarantee (mirrors the playlist loop): + // a malformed/proxy response of { items: [], next: } never grows + // songs.length nor nulls `next`, so a loop bounded only by songs.length would + // spin forever — the offset cap stops it regardless of items/next. const songs: Song[] = []; let page = album?.tracks; let offset = ALBUM_PAGE_SIZE; @@ -193,7 +197,7 @@ export class SpotifyWebApi { for (const t of page.items.filter(Boolean)) { songs.push({ ...mapSpotifyTrack(t), album: albumName, coverUrl: cover }); } - if (!page.next || songs.length >= MAX_ALBUM_TRACKS) break; + if (!page.next || songs.length >= MAX_ALBUM_TRACKS || offset >= MAX_ALBUM_TRACKS) break; page = await this.get(`/v1/albums/${albumId}/tracks`, { limit: ALBUM_PAGE_SIZE, offset,