Pin three untested guards: download timer, URL fragment, short APP1 (closes #89)
check / check (push) Successful in 15s
check / check (push) Successful in 15s
The download idle deadline's timer is unref'd so it never holds the process open, 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 when the APP1 length is at least 8, so it never reads the next segment's bytes. Model: opus-5-5
This commit is contained in:
@@ -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
|
||||
|
||||
+3
-2
@@ -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<typeof setTimeout> | undefined;
|
||||
@@ -65,6 +65,7 @@ const idleDeadline = (ms: number) => {
|
||||
),
|
||||
);
|
||||
}, ms);
|
||||
timer.unref();
|
||||
};
|
||||
restart();
|
||||
return { signal: controller.signal, restart, stop };
|
||||
|
||||
@@ -45,8 +45,10 @@ 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 has no
|
||||
// room for it, and the bytes compared would be the next segment's.
|
||||
if (
|
||||
len >= 8 &&
|
||||
buf[offset + 4] === 0x45 &&
|
||||
buf[offset + 5] === 0x78 &&
|
||||
buf[offset + 6] === 0x69 &&
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -51,6 +51,13 @@ describe("extractExifFromJpeg", () => {
|
||||
expect(extractExifFromJpeg(bytes(SOI, app0, SOS))).toEqual({});
|
||||
});
|
||||
|
||||
it("ignores an APP1 segment too short to hold the Exif header", () => {
|
||||
// Length 7 leaves room for "Exif\0" only. Without the length check
|
||||
// the scan compared the header against bytes past the segment.
|
||||
const short = app1(EXIF_HEADER.slice(0, 5));
|
||||
expect(extractExifFromJpeg(bytes(SOI, short, SOS))).toEqual({});
|
||||
});
|
||||
|
||||
it("reports a JPEG truncated inside a segment header", () => {
|
||||
const scan = extractExifFromJpeg(bytes(SOI, [0xff, 0xe1, 0x00]));
|
||||
expect(scan.exif).toBeUndefined();
|
||||
|
||||
Reference in New Issue
Block a user