fix(api): gate every tool endpoint and stop ZIP streams failing quietly (#646)

Three defects from #645, all of which let the server report something
that was not true.

Tool access was enforced per route, so it drifted. createToolRoute calls
requireToolAccess and the factory tools were fine, but all 45
hand-written routes had to remember the same call and none of them did.
A role without tools:use could run image-to-pdf, svg-to-raster,
erase-object, favicon, qr-generate, upscale, sign-pdf and the rest. The
issue described this as affecting two routes; it was every one of them.

The check now lives in a single preHandler keyed off the tool the router
matched, so it covers sub-paths (/batch, /info, /preview, /analyze,
/inspect) and any route added later without that route opting in. Ids no
tool claims stay unresolved, which keeps an unknown or misfiled tool a
404 rather than telling an unauthorized caller which ids exist.

Resolution reads request.routeOptions.url, the pattern the app itself
registered, rather than parsing request.url a second time. find-my-way
decodes before matching, so an independent parse disagrees with the
router and the router wins: `/api/v1/tools/image/%66avicon` ran favicon
while the gate saw no tool at all. Absolute-form request targets slipped
it the same way. Taking the router's own answer removes the disagreement.

A ZIP stream that failed after the 200 headers were out called
reply.raw.end(). On a chunked response that is indistinguishable from
success, so a client kept an archive with no central directory believing
it whole. Worse, a source stream that errored had no listener: the
request hung until it timed out and the error surfaced as unhandled. A
poisoned-storage probe reproduced both. The socket is destroyed instead,
and every source stream is listened to. svg-to-raster additionally ran
its append loop past the hijack with no try/catch, where a throw leaves
Fastify logging and walking away with the socket neither ended nor
destroyed. pdf-to-image is fixed alongside the other two: it shipped in
#643 with the destroy half but not the listener, so it hung the same way.

A zero-byte upload was dropped during parsing. The client pairs results
with its own file list by index, so every later result shifted onto the
wrong file: one document's output was presented as another's, under
another's name, while the file that actually converted was marked "not
found in batch results". Empty parts now keep their slot and fail in
place with a reason.

Two tests in adversarial-extended.test.ts asserted the old zero-byte
behavior, including a comment that batch "silently skips zero-byte
parts". They now pin the replacement: still rejected, nothing processed,
but the caller is told which files were empty instead of being told it
sent none.

A guard walks the whole catalog and fails if any of the 241 tools answers
anything but 403 for a role without tools:use, so a tool cannot escape
the gate by being registered in a shape nobody thought to sample.

Fixes #645
This commit is contained in:
SnapOtter
2026-07-26 09:57:44 +08:00
committed by GitHub
parent 0058fc610f
commit 2d8b57c57f
12 changed files with 718 additions and 75 deletions
@@ -726,8 +726,11 @@ describe("Batch edge cases — extended", () => {
expect(json.errors.length).toBe(2);
});
it("handles batch with zero-byte files (skipped as empty)", async () => {
// Batch processing silently skips zero-byte parts (buffer.length === 0)
it("rejects a zero-byte file and names it (issue #645)", async () => {
// Zero-byte parts used to be dropped during parsing, which answered "No
// files provided" for a request that did provide one and, in a mixed
// batch, shifted every later result onto the wrong file. They now keep
// their slot and fail in place with a reason.
const res = await postBatch("resize", [
{
name: "file",
@@ -738,10 +741,12 @@ describe("Batch edge cases — extended", () => {
{ name: "settings", content: JSON.stringify({ width: 50 }) },
]);
// The zero-byte file is skipped, resulting in 0 valid files
expect(res.statusCode).toBe(400);
expect(res.statusCode).toBe(422);
const json = JSON.parse(res.body);
expect(json.error).toMatch(/no file/i);
expect(json.error).toMatch(/all files failed/i);
expect(json.errors).toHaveLength(1);
expect(json.errors[0].filename).toBe("empty.png");
expect(json.errors[0].error).toMatch(/empty/i);
});
});
@@ -1273,7 +1278,7 @@ describe("Settings boundary values", () => {
// ═══════════════════════════════════════════════════════════════════════════
// MULTI-FORMAT ZERO-BYTE BATCH
// ═══════════════════════════════════════════════════════════════════════════
describe("Batch with only zero-byte files", () => {
describe("Batch where every file is zero-byte", () => {
it("rejects batch where all files are zero-byte", async () => {
const res = await postBatch("resize", [
{
@@ -1291,10 +1296,15 @@ describe("Batch with only zero-byte files", () => {
{ name: "settings", content: JSON.stringify({ width: 50 }) },
]);
// Zero-byte files are skipped during parsing, so 0 valid files
expect(res.statusCode).toBe(400);
// Still rejected outright, nothing processed, but the caller is now told
// which files were empty instead of being told it sent none (issue #645).
expect(res.statusCode).toBe(422);
const json = JSON.parse(res.body);
expect(json.error).toMatch(/no file/i);
expect(json.error).toMatch(/all files failed/i);
expect(json.errors.map((e: { filename: string }) => e.filename)).toEqual([
"empty1.png",
"empty2.png",
]);
});
});