diff --git a/TODO.md b/TODO.md index e39ebf3..29a1c3f 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,12 @@ Tag v1.0.0. # Completed Steps +- 2026-09-23: Pinned three guards reviewers found untested (issue 89). The + download idle deadline's timer is unref'd, so it can never keep the process + alive, and a test checks no timer is left after a download completes or fails. + A test covers the rejection of `#` in a request path. The EXIF scan compares + the `Exif` header only in an APP1 segment of length 8 or more, so it never + reads the next segment's bytes, with a test for a short one. - 2026-09-23: Made the download deadline an idle deadline (issue 24). `downloadTimeoutMs` now aborts a file or thumbnail download only after no bytes have arrived for that long, default 60 seconds, instead of bounding the diff --git a/src/api/client.ts b/src/api/client.ts index 4d7d104..316f9cc 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -49,8 +49,8 @@ export interface StreamOptions { // An abort signal that fires once `ms` pass without a call to `restart`. It // aborts with a `TimeoutError`, the same reason `AbortSignal.timeout()` gives, // so the retry classifier treats an idle download exactly as it treats any -// other deadline. `stop` must be called when the download ends, or the timer -// keeps the process alive until it fires. +// other deadline. `stop` must be called when the download ends. The timer is +// unref'd, so even one left running never keeps the process alive. const idleDeadline = (ms: number) => { const controller = new AbortController(); let timer: ReturnType | undefined; @@ -65,6 +65,7 @@ const idleDeadline = (ms: number) => { ), ); }, ms); + timer.unref(); }; restart(); return { signal: controller.signal, restart, stop }; diff --git a/src/metadata-backup.ts b/src/metadata-backup.ts index b022189..8497e2d 100644 --- a/src/metadata-backup.ts +++ b/src/metadata-backup.ts @@ -45,8 +45,11 @@ export const extractExifFromJpeg = ( error: `segment length ${len} at byte ${offset} runs past the end of the file`, }; if (marker === 0xe1) { - // APP1 — check for "Exif\0\0" header + // APP1 — check for "Exif\0\0" header. A length under 8 cannot hold + // the six-byte header, so the segment is not EXIF; below 6 the + // bytes compared would also lie past the segment. if ( + len >= 8 && buf[offset + 4] === 0x45 && buf[offset + 5] === 0x78 && buf[offset + 6] === 0x69 && diff --git a/test/api/client.test.ts b/test/api/client.test.ts index 4f201cb..68ab7cf 100644 --- a/test/api/client.test.ts +++ b/test/api/client.test.ts @@ -474,6 +474,16 @@ describe("ApiClient request URLs", () => { ); expect(calls).toHaveLength(0); }); + + it("rejects a path that carries a fragment", async () => { + const { fetch, calls } = recordingFetch(); + const client = new ApiClient({ fetch }); + + await expect(client.getJSON("/diff#top")).rejects.toThrow( + /must not contain "\?" or "#"/, + ); + expect(calls).toHaveLength(0); + }); }); describe("ApiError", () => { @@ -980,6 +990,52 @@ describe("ApiClient timeouts", () => { } expect(joined).toEqual(payload); }); + + it("leaves no timer pending after a download completes or fails", async () => { + vi.useFakeTimers(); + try { + const { fetch } = scriptedFetch( + streamResponse(new Uint8Array([1, 2, 3])), + textResponse("gone", 404), + ); + const client = new ApiClient({ + fetch, + retry: { ...noWait, attempts: 1 }, + }); + + expect(await readAll(await client.getFileStream(1))).toBe(3); + expect(vi.getTimerCount()).toBe(0); + + await expect(client.getFileStream(2)).rejects.toBeInstanceOf( + ApiError, + ); + expect(vi.getTimerCount()).toBe(0); + } finally { + vi.useRealTimers(); + } + }); + + it("never lets the download timer keep the process alive", async () => { + const spy = vi.spyOn(globalThis, "setTimeout"); + try { + const { fetch } = scriptedFetch( + streamResponse(new Uint8Array([1])), + ); + const client = new ApiClient({ + fetch, + downloadTimeoutMs: 12_345, + retry: noWait, + }); + + const stream = await client.getFileStream(1); + const i = spy.mock.calls.findIndex((call) => call[1] === 12_345); + const timer = spy.mock.results[i]!.value as NodeJS.Timeout; + expect(timer.hasRef()).toBe(false); + await stream.cancel(); + } finally { + spy.mockRestore(); + } + }); }); describe("ApiClient error typing", () => { diff --git a/test/cli/metadata-exif.test.ts b/test/cli/metadata-exif.test.ts index fdaafa2..4a8cf55 100644 --- a/test/cli/metadata-exif.test.ts +++ b/test/cli/metadata-exif.test.ts @@ -51,6 +51,20 @@ describe("extractExifFromJpeg", () => { expect(extractExifFromJpeg(bytes(SOI, app0, SOS))).toEqual({}); }); + it("ignores an APP1 segment too short to hold the Exif header", () => { + // A length under 8 cannot hold the six-byte "Exif\0\0" header, so the + // segment is not EXIF. This one has length 7 and holds only "Exif\0", + // which the old code, lacking the length check, returned as EXIF. + const short = app1(EXIF_HEADER.slice(0, 5)); + expect(extractExifFromJpeg(bytes(SOI, short, SOS))).toEqual({}); + }); + + it("accepts an APP1 segment of length 8 holding just the Exif header", () => { + const scan = extractExifFromJpeg(bytes(SOI, app1(EXIF_HEADER), SOS)); + expect(scan.error).toBeUndefined(); + expect([...scan.exif!]).toEqual(EXIF_HEADER); + }); + it("reports a JPEG truncated inside a segment header", () => { const scan = extractExifFromJpeg(bytes(SOI, [0xff, 0xe1, 0x00])); expect(scan.exif).toBeUndefined();