diff --git a/apps/api/src/index.ts b/apps/api/src/index.ts index 97d170f1..ee88d75e 100644 --- a/apps/api/src/index.ts +++ b/apps/api/src/index.ts @@ -53,6 +53,7 @@ import { registerMfa } from "./plugins/mfa.js"; import { oidcRoutes } from "./plugins/oidc.js"; import { registerSaml } from "./plugins/saml.js"; import { registerStatic } from "./plugins/static.js"; +import { toolAccessMiddleware } from "./plugins/tool-access.js"; import { registerUpload } from "./plugins/upload.js"; import { adminOpsRoutes } from "./routes/admin-ops.js"; import { analyticsRoutes } from "./routes/analytics.js"; @@ -456,6 +457,11 @@ await preferencesRoutes(app); // Auth middleware (must be registered before routes it protects) await authMiddleware(app); +// Tool access gate (after auth so request.user is populated). Covers every +// per-tool endpoint including the hand-written routes, which each used to +// have to remember the check themselves. +await toolAccessMiddleware(app); + // Per-user rate limiting (after auth so request.user is populated) await registerPerUserRateLimit(app); diff --git a/apps/api/src/plugins/tool-access.ts b/apps/api/src/plugins/tool-access.ts new file mode 100644 index 00000000..b045c23a --- /dev/null +++ b/apps/api/src/plugins/tool-access.ts @@ -0,0 +1,63 @@ +import { TOOLS, toolSection } from "@snapotter/shared"; +import type { FastifyInstance } from "fastify"; +import { requireToolAccess } from "../permissions.js"; + +const TOOLS_PREFIX = "/api/v1/tools/"; + +/** toolId -> section, so a route resolves without scanning the catalog. */ +const SECTION_BY_TOOL_ID = new Map(TOOLS.map((tool) => [tool.id, toolSection(tool)])); + +/** + * The tool a request targets, read from the route the router actually matched + * rather than from the raw URL. + * + * This distinction is the whole point. find-my-way percent-decodes before + * matching, so `/api/v1/tools/image/%66avicon` runs the favicon handler while + * a second, independent parse of the raw string sees no tool at all and waves + * it through. Taking the router's own answer means the gate and the router + * cannot disagree. `routeUrl` is the registered pattern, so it is already + * canonical for static routes; the one parametric tool route + * (`:section/:toolId/batch`) fills in from params, which Fastify has likewise + * already decoded. + * + * Null means "not a per-tool endpoint": the catalog listings, and ids no tool + * claims. Leaving those alone keeps an unknown or misfiled tool a 404 rather + * than telling an unauthorized caller which ids exist. + */ +export function toolIdFromRoute( + routeUrl: string | undefined, + params?: { section?: string; toolId?: string }, +): string | null { + if (!routeUrl?.startsWith(TOOLS_PREFIX)) return null; + const [patternSection, patternToolId] = routeUrl.slice(TOOLS_PREFIX.length).split("/"); + if (!patternSection || !patternToolId) return null; + const section = patternSection === ":section" ? params?.section : patternSection; + const toolId = patternToolId === ":toolId" ? params?.toolId : patternToolId; + if (!section || !toolId) return null; + return SECTION_BY_TOOL_ID.get(toolId) === section ? toolId : null; +} + +/** + * Enforce tool access for every per-tool endpoint from one place. + * + * createToolRoute gates the tools it builds, but the 45 hand-written routes + * each had to remember the same call and none of them did, so a role without + * `tools:use` could run image-to-pdf, svg-to-raster, erase-object, favicon and + * the rest (issue #645). Per-route calls are exactly what drifted, so the + * check keys off the matched route and therefore covers sub-paths (/batch, + * /info, /preview, /analyze, /inspect) and any route added later without it + * having to opt in. + * + * Must be registered after the auth middleware, which populates request.user. + * The remaining per-route calls stay as defense in depth. + */ +export async function toolAccessMiddleware(app: FastifyInstance): Promise { + app.addHook("preHandler", async (request, reply) => { + const toolId = toolIdFromRoute( + request.routeOptions?.url, + request.params as { section?: string; toolId?: string } | undefined, + ); + if (!toolId) return; + if (!(await requireToolAccess(request, reply, toolId))) return reply; + }); +} diff --git a/apps/api/src/routes/batch.ts b/apps/api/src/routes/batch.ts index 5b9a3723..6a00644f 100644 --- a/apps/api/src/routes/batch.ts +++ b/apps/api/src/routes/batch.ts @@ -138,7 +138,11 @@ export async function registerBatchRoutes(app: FastifyInstance): Promise { 413, ); } - if (file.size > 0) files.push({ kind: "path", ...file }); + // Empty parts keep their slot: the client maps results back + // onto its own file list by index, so dropping one here + // would label a converted file with a different file's name + // (issue #645). They fail in place in the loop below. + files.push({ kind: "path", ...file }); } else { const chunks: Buffer[] = []; for await (const chunk of part.file) { @@ -154,14 +158,12 @@ export async function registerBatchRoutes(app: FastifyInstance): Promise { chunks.push(chunk); } const buffer = Buffer.concat(chunks); - if (buffer.length > 0) { - files.push({ - kind: "buffer", - buffer, - filename: sanitizeFilename(part.filename ?? "file"), - size: buffer.length, - }); - } + files.push({ + kind: "buffer", + buffer, + filename: sanitizeFilename(part.filename ?? "file"), + size: buffer.length, + }); } filePartIndex++; } else if (part.fieldname === "settings") { @@ -285,6 +287,18 @@ export async function registerBatchRoutes(app: FastifyInstance): Promise { const childId = `${parentId}-f${flowChildIndex}`; let key = `uploads/${childId}/${processFilename}`; + // Reported against the slot it arrived in, so every later result + // stays paired with the file it came from. + if (file.size === 0) { + preFailures.push({ + originalIndex: i, + filename: file.filename, + error: "File is empty", + }); + if (file.kind === "path") await rm(file.path, { force: true }).catch(() => {}); + continue; + } + if (file.kind === "path") { try { await storeValidatedOcrPdf(file, key, { @@ -573,12 +587,20 @@ export async function registerBatchRoutes(app: FastifyInstance): Promise { const archive = archiver("zip", { zlib: { level: 5 } }); - archive.on("error", (err) => { - request.log.error({ err }, "Archiver error during batch processing"); - if (!reply.raw.writableEnded) { - reply.raw.end(); - } - }); + // The 200 headers are already out, so nothing here can change the + // status. Destroy the socket rather than end() it: a clean end on a + // chunked response is indistinguishable from success, so the client + // would keep a ZIP with no central directory believing it complete. + let streamFailed = false; + const failStream = (err: Error, msg: string) => { + if (streamFailed) return; + streamFailed = true; + request.log.error({ err, jobId: parentId }, msg); + archive.abort(); + reply.raw.destroy(err); + }; + + archive.on("error", (err) => failStream(err, "Archiver error during batch processing")); archive.pipe(reply.raw); @@ -587,16 +609,22 @@ export async function registerBatchRoutes(app: FastifyInstance): Promise { for (const entry of successEntries) { if (!entry.outputRef) continue; const stream = await getObjectStream(entry.outputRef); + // A backend that resolves the stream and only then fails (local + // createReadStream emitting ENOENT) never rejects here, so the + // catch below cannot see it. Without this listener that error is + // unhandled and the request hangs instead of terminating. + stream.on("error", (err: Error) => + failStream(err, "Object stream error during batch processing"), + ); archive.append(stream, { name: entry.filename }); } await archive.finalize(); } catch (err) { - request.log.error({ err }, "Failed to stream ZIP entries during batch processing"); - archive.abort(); - if (!reply.raw.writableEnded) { - reply.raw.end(); - } + failStream( + err instanceof Error ? err : new Error(String(err)), + "Failed to stream ZIP entries during batch processing", + ); } } finally { request.raw.removeListener("aborted", abortIngress); diff --git a/apps/api/src/routes/tools/pdf-to-image.ts b/apps/api/src/routes/tools/pdf-to-image.ts index 462d7ff4..a9851d4c 100644 --- a/apps/api/src/routes/tools/pdf-to-image.ts +++ b/apps/api/src/routes/tools/pdf-to-image.ts @@ -591,7 +591,10 @@ export function registerPdfToImageRoute( // Destroy the socket rather than end() it: a clean end on a chunked // response is indistinguishable from success, and the client would keep // a truncated ZIP believing it complete. + let streamFailed = false; const failStream = (err: Error, msg: string) => { + if (streamFailed) return; + streamFailed = true; request.log.error({ err, jobId }, msg); archive.abort(); reply.raw.destroy(err); @@ -603,7 +606,14 @@ export function registerPdfToImageRoute( try { for (const entry of results) { if (!entry) continue; - archive.append(await getObjectStream(entry.key), { name: entry.filename }); + const stream = await getObjectStream(entry.key); + // The local backend resolves the stream and only then emits ENOENT, + // so the catch below never sees it. Without this listener the error + // is unhandled and the request hangs instead of terminating. + stream.on("error", (err: Error) => + failStream(err, "Object stream error during PDF batch processing"), + ); + archive.append(stream, { name: entry.filename }); } await archive.finalize(); } catch (err) { diff --git a/apps/api/src/routes/tools/svg-to-raster.ts b/apps/api/src/routes/tools/svg-to-raster.ts index 0837fb72..d2e0e489 100644 --- a/apps/api/src/routes/tools/svg-to-raster.ts +++ b/apps/api/src/routes/tools/svg-to-raster.ts @@ -130,13 +130,14 @@ export function registerSvgToRasterRoute( for await (const chunk of part.file) { chunks.push(chunk); } - const buf = Buffer.concat(chunks); - if (buf.length > 0) { - files.push({ - buffer: buf, - filename: sanitizeFilename(part.filename ?? "output"), - }); - } + // Empty parts keep their slot: the client maps results back onto its + // own file list by index, so dropping one would label a converted + // file with a different file's name (issue #645). The per-file loop + // below fails it in place. + files.push({ + buffer: Buffer.concat(chunks), + filename: sanitizeFilename(part.filename ?? "output"), + }); } else if (part.fieldname === "settings") { settingsRaw = part.value as string; } else if (part.fieldname === "clientJobId") { @@ -165,7 +166,13 @@ export function registerSvgToRasterRoute( if (opts.accept) { const accept = opts.accept; - const invalid = files.find((file) => !matchesAccept(file.filename, accept)); + // Empty parts are exempt: they now keep their slot rather than being + // dropped (issue #645), and a nameless one would otherwise fail this + // whole-request check and take every valid file down with it. They fail + // on their own below, where the reason reaches the file it belongs to. + const invalid = files.find( + (file) => file.buffer.length > 0 && !matchesAccept(file.filename, accept), + ); if (invalid) { return reply.status(400).send({ error: "File is not a valid SVG. This tool only accepts SVG files.", @@ -337,22 +344,41 @@ export function registerSvgToRasterRoute( const archive = archiver("zip", { zlib: { level: 5 } }); - archive.on("error", (err) => { - request.log.error({ err }, "Archiver error during SVG batch processing"); - if (!reply.raw.writableEnded) { - reply.raw.end(); - } - }); + // Headers are already out, so this cannot change the status. Destroying + // the socket is the only way to tell the client the archive is incomplete: + // ending it cleanly reads as success and hands back a ZIP with no central + // directory. Entries here are in-memory buffers, so unlike the other batch + // routes there is no storage read to fail, but an archiver fault still has + // to reach the client. + let streamFailed = false; + const failStream = (err: Error, msg: string) => { + if (streamFailed) return; + streamFailed = true; + request.log.error({ err, jobId }, msg); + archive.abort(); + reply.raw.destroy(err); + }; + + archive.on("error", (err) => failStream(err, "Archiver error during SVG batch processing")); archive.pipe(reply.raw); - for (const result of results) { - if (result) { - archive.append(result.buffer, { name: result.filename }); + // A throw past this point cannot be answered: the reply is hijacked, so + // Fastify logs and walks away, leaving the socket neither ended nor + // destroyed and the client waiting forever. + try { + for (const result of results) { + if (result) { + archive.append(result.buffer, { name: result.filename }); + } } + await archive.finalize(); + } catch (err) { + failStream( + err instanceof Error ? err : new Error(String(err)), + "Failed to finalize ZIP during SVG batch processing", + ); } - - await archive.finalize(); }); // --- Single-file endpoint --- diff --git a/tests/integration/platform/batch.test.ts b/tests/integration/platform/batch.test.ts index 9beff248..5899e161 100644 --- a/tests/integration/platform/batch.test.ts +++ b/tests/integration/platform/batch.test.ts @@ -214,6 +214,32 @@ describe("Filename deduplication in batch", () => { // ── X-File-Results header ─────────────────────────────────────── describe("X-File-Results header", () => { + it("keeps index alignment when an upload is empty (issue #645)", async () => { + // The client maps results back onto its own file list by index, so + // dropping a zero-byte part would shift every later index and label a + // converted file with a different file's name. + const { body, contentType } = createMultipartPayload([ + { name: "file", filename: "empty.png", contentType: "image/png", content: Buffer.alloc(0) }, + { name: "file", filename: "real.png", contentType: "image/png", content: PNG }, + { name: "settings", content: JSON.stringify({ width: 80 }) }, + ]); + + const res = await app.inject({ + method: "POST", + url: "/api/v1/tools/image/resize/batch", + headers: { "content-type": contentType, authorization: `Bearer ${adminToken}` }, + body, + }); + + expect(res.statusCode).toBe(200); + const fileResults = JSON.parse(decodeURIComponent(res.headers["x-file-results"] as string)); + expect(fileResults["0"]).toBeUndefined(); + expect(fileResults["1"]).toMatch(/real/); + + const zip = new AdmZip(res.rawPayload); + expect(zip.getEntries()).toHaveLength(1); + }); + it("maps file indices to output filenames", async () => { const { body, contentType } = createMultipartPayload([ { name: "file", filename: "first.png", contentType: "image/png", content: PNG }, diff --git a/tests/integration/platform/tool-access-gate.test.ts b/tests/integration/platform/tool-access-gate.test.ts new file mode 100644 index 00000000..20400d43 --- /dev/null +++ b/tests/integration/platform/tool-access-gate.test.ts @@ -0,0 +1,191 @@ +/** + * Every tool endpoint enforces tool access, not just the factory-built ones + * (issue #645). + * + * createToolRoute calls requireToolAccess, so factory tools were gated. The + * 45 hand-written routes each called nothing, so a role without `tools:use` + * could run erase-object, favicon, qr-generate, image-to-pdf, svg-to-raster + * and the rest. Per-route calls are what drifted, so the gate now lives in + * one preHandler keyed off the tool id in the URL. + */ +import { apiToolPath, TOOLS } from "@snapotter/shared"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { fixtures, readFixture } from "../../fixtures/index.js"; +import { + buildTestApp, + createMultipartPayload, + createUserAndLogin, + loginAsAdmin, + type TestApp, +} from "../test-server.js"; + +const PNG = readFixture(fixtures.image.base.png200); +const SVG = readFixture(fixtures.image.base.svg100); +const PDF = readFixture(fixtures.document.pdf3); + +let testApp: TestApp; +let app: TestApp["app"]; +let adminToken: string; +/** Session for a role that deliberately lacks tools:use. */ +let noToolsToken: string; + +beforeAll(async () => { + testApp = await buildTestApp(); + app = testApp.app; + adminToken = await loginAsAdmin(app); + + await app.inject({ + method: "POST", + url: "/api/v1/roles", + headers: { authorization: `Bearer ${adminToken}` }, + payload: { + name: "notools645", + description: "Can browse but not run tools", + permissions: ["files:own"], + }, + }); + noToolsToken = (await createUserAndLogin(app, "notools645user", "notools645")).token; +}, 30_000); + +afterAll(async () => { + await testApp.cleanup(); +}, 10_000); + +function post( + url: string, + token: string | null, + file: { name: string; type: string; buf: Buffer }, +) { + const { body, contentType } = createMultipartPayload([ + { name: "file", filename: file.name, contentType: file.type, content: file.buf }, + { name: "settings", content: JSON.stringify({}) }, + ]); + return app.inject({ + method: "POST", + url, + body, + headers: { + "content-type": contentType, + ...(token ? { authorization: `Bearer ${token}` } : {}), + }, + }); +} + +const IMG = { name: "a.png", type: "image/png", buf: PNG }; +const VEC = { name: "a.svg", type: "image/svg+xml", buf: SVG }; +const DOC = { name: "a.pdf", type: "application/pdf", buf: PDF }; + +/** + * A spread of endpoints across every registration style: the factory, the + * three custom ZIP routes and their presets, plain hand-written routes, an AI + * route, and the generic batch route. + */ +const GATED: Array<[string, string, typeof IMG]> = [ + ["factory tool", "/api/v1/tools/image/resize", IMG], + ["generic batch", "/api/v1/tools/image/resize/batch", IMG], + ["image-to-pdf base", "/api/v1/tools/image/image-to-pdf", IMG], + ["image-to-pdf preset", "/api/v1/tools/image/jpg-to-pdf", IMG], + ["svg-to-raster base", "/api/v1/tools/image/svg-to-raster", VEC], + ["svg-to-raster batch", "/api/v1/tools/image/svg-to-raster/batch", VEC], + ["svg-to-raster preset", "/api/v1/tools/image/svg-to-png", VEC], + ["pdf-to-image preset", "/api/v1/tools/pdf/pdf-to-jpg", DOC], + ["pdf-to-image info", "/api/v1/tools/pdf/pdf-to-jpg/info", DOC], + ["hand-written: favicon", "/api/v1/tools/image/favicon", IMG], + ["hand-written: qr-generate", "/api/v1/tools/image/qr-generate", IMG], + ["hand-written: collage", "/api/v1/tools/image/collage", IMG], + ["hand-written: split", "/api/v1/tools/image/split", IMG], + ["hand-written: stitch", "/api/v1/tools/image/stitch", IMG], + ["hand-written: image-to-base64", "/api/v1/tools/image/image-to-base64", IMG], + ["hand-written: info", "/api/v1/tools/image/info", IMG], + ["hand-written: compare", "/api/v1/tools/image/compare", IMG], + ["hand-written: vectorize", "/api/v1/tools/image/vectorize", IMG], + ["hand-written: strip-metadata inspect", "/api/v1/tools/image/strip-metadata/inspect", IMG], + ["ai route: upscale", "/api/v1/tools/image/upscale", IMG], + ["ai route: erase-object", "/api/v1/tools/image/erase-object", IMG], + ["ai route: remove-background", "/api/v1/tools/image/remove-background", IMG], + ["sign-pdf", "/api/v1/tools/pdf/sign-pdf", DOC], +]; + +describe("tool access gate (issue #645)", () => { + it.each(GATED)("%s returns 403 for a role without tools:use", async (_label, url, file) => { + const res = await post(url, noToolsToken, file); + expect(res.statusCode, `${url} -> ${res.body.slice(0, 200)}`).toBe(403); + }); + + it.each(GATED)("%s returns 401 with no session", async (_label, url, file) => { + const res = await post(url, null, file); + expect(res.statusCode, `${url} -> ${res.body.slice(0, 200)}`).toBe(401); + }); + + it.each(GATED)("%s does not 403 an admin", async (_label, url, file) => { + const res = await post(url, adminToken, file); + expect(res.statusCode, `${url} -> ${res.body.slice(0, 200)}`).not.toBe(403); + }); + + /** + * The sampled list above is readable but partial. This walks the whole + * catalog so a tool cannot escape the gate by being registered in a shape + * nobody thought to sample, for instance inside an encapsulated plugin, + * which is the way a global hook would silently stop applying. + */ + it("refuses every tool in the catalog for a role without tools:use", async () => { + const reachable: string[] = []; + for (const tool of TOOLS) { + const res = await post(apiToolPath(tool.id), noToolsToken, IMG); + if (res.statusCode !== 403) reachable.push(`${tool.id} -> ${res.statusCode}`); + } + expect(reachable, `tools not gated: ${reachable.join(", ")}`).toEqual([]); + }, 120_000); + + it("still lets a tools:use-less role read the popular-tools listing", async () => { + // A listing under the same prefix is not a tool run. `popular` has one + // path segment, so it must not be mistaken for a section/toolId pair. + const res = await app.inject({ + method: "GET", + url: "/api/v1/tools/popular", + headers: { authorization: `Bearer ${noToolsToken}` }, + }); + expect(res.statusCode).toBe(200); + }); + + it("leaves an unknown tool id as a 404, not a 403", async () => { + const res = await post("/api/v1/tools/image/not-a-real-tool", noToolsToken, IMG); + expect(res.statusCode).toBe(404); + }); + + it("leaves a real tool under the wrong section as a 404", async () => { + const res = await post("/api/v1/tools/video/resize", noToolsToken, IMG); + expect(res.statusCode).toBe(404); + }); + + /** + * The gate reads the tool id out of the URL, so any URL form the router + * still resolves to a tool handler but the gate reads differently would be + * a way straight past it. Each of these must be refused or unrouted; the + * one answer that must never appear is a 2xx. + */ + it.each([ + // Probe hand-written routes specifically. A factory tool like resize + // keeps its own requireToolAccess call, so it answers 403 whether or not + // the gate resolved the URL, and would hide a gate that failed open. + ["percent-encoded hand-written id", "/api/v1/tools/image/%66avicon"], + ["percent-encoded ai route", "/api/v1/tools/image/%75pscale"], + ["fully percent-encoded hand-written id", "/api/v1/tools/image/%73%70%6c%69%74"], + ["percent-encoded tool id", "/api/v1/tools/image/%72esize"], + ["percent-encoded section", "/api/v1/tools/%69mage/resize"], + ["fully percent-encoded id", "/api/v1/tools/image/%72%65%73%69%7a%65"], + ["encoded separator", "/api/v1/tools/image%2fresize"], + ["uppercase tool id", "/api/v1/tools/image/RESIZE"], + ["double slash before section", "/api/v1/tools//image/resize"], + ["dot segment", "/api/v1/tools/image/./resize"], + ["parent segment", "/api/v1/tools/pdf/../image/resize"], + ["trailing dot", "/api/v1/tools/image/resize."], + ["query smuggling", "/api/v1/tools/image/resize?x=/api/v1/tools/image/other"], + ])("never processes a tool via %s", async (_label, url) => { + const res = await post(url, noToolsToken, IMG); + expect( + res.statusCode, + `${url} reached a handler with ${res.statusCode}: ${res.body.slice(0, 200)}`, + ).toBeGreaterThanOrEqual(400); + }); +}); diff --git a/tests/integration/platform/zip-stream-failure.test.ts b/tests/integration/platform/zip-stream-failure.test.ts new file mode 100644 index 00000000..f0096991 --- /dev/null +++ b/tests/integration/platform/zip-stream-failure.test.ts @@ -0,0 +1,185 @@ +/** + * What every ZIP-streaming route does when object storage fails AFTER the 200 + * headers are already on the wire (issue #645). + * + * The response is hijacked and chunked by then, so the status cannot change. + * Ending the response cleanly is therefore indistinguishable from success and + * hands the client a ZIP with no central directory; worse, a source stream + * that errors with no listener leaves the request hanging and raises an + * unhandled error. The routes must instead destroy the connection so the + * client sees a transport failure it can act on. + */ +import AdmZip from "adm-zip"; +import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; +import { fixtures, readFixture } from "../../fixtures/index.js"; +import { + buildTestApp, + createMultipartPayload, + loginAsAdmin, + type TestApp, +} from "../test-server.js"; + +/** Prefixes whose getObjectStream should fail once a consumer starts reading. */ +const storageMock = vi.hoisted(() => ({ poison: new Set() })); + +vi.mock("../../../apps/api/src/lib/object-storage.js", async (importOriginal) => { + const actual = + await importOriginal(); + const { Readable } = await import("node:stream"); + return { + ...actual, + getObjectStream: async (key: string) => { + for (const prefix of storageMock.poison) { + if (key.startsWith(prefix)) { + // Fail on read, not on open: that is how a real backend behaves + // (createReadStream resolves, then emits ENOENT), and it is the case + // the route's try/catch cannot see. + return new Readable({ + read() { + this.destroy(new Error(`test poison: stream error for ${key}`)); + }, + }); + } + } + return actual.getObjectStream(key); + }, + }; +}); + +const PNG = readFixture(fixtures.image.base.png200); +const JPG = readFixture(fixtures.image.base.jpg100); +const SVG = readFixture(fixtures.image.base.svg100); +const PDF_3PAGE = readFixture(fixtures.document.pdf3); +const PDF_2PAGE = readFixture(fixtures.document.pdf2); + +let testApp: TestApp; +let token: string; + +beforeAll(async () => { + testApp = await buildTestApp(); + token = await loginAsAdmin(testApp.app); +}, 30_000); + +afterAll(async () => { + await testApp.cleanup(); +}, 10_000); + +/** + * Run a request whose ZIP entries all come from poisoned storage and report + * how it ended. A route that handles this correctly must terminate promptly + * and must not hand back something that parses as a complete archive. + */ +async function runWithPoisonedOutputs( + url: string, + parts: Array<{ name: string; filename?: string; contentType?: string; content: Buffer | string }>, +): Promise<{ settledWithin: boolean; deliveredCompleteZip: boolean }> { + const { body, contentType } = createMultipartPayload(parts); + storageMock.poison.add("outputs/"); + + // Race a real deadline rather than leaning on the test timeout: a hang is + // the failure mode being tested, and "the suite timed out" is a much weaker + // signal than a named assertion. + const HANG_BUDGET_MS = 10_000; + let timer: NodeJS.Timeout | undefined; + const deadline = new Promise<"timeout">((resolve) => { + timer = setTimeout(() => resolve("timeout"), HANG_BUDGET_MS); + }); + + try { + const request = testApp.app + .inject({ + method: "POST", + url, + headers: { authorization: `Bearer ${token}`, "content-type": contentType }, + body, + }) + // A destroyed connection surfaces as a rejected request, which is + // exactly the signal the client should get. + .then( + (res) => res, + () => null, + ); + + const outcome = await Promise.race([request, deadline]); + if (outcome === "timeout") return { settledWithin: false, deliveredCompleteZip: false }; + if (outcome === null) return { settledWithin: true, deliveredCompleteZip: false }; + + try { + new AdmZip(outcome.rawPayload).getEntries(); + return { settledWithin: true, deliveredCompleteZip: true }; + } catch { + return { settledWithin: true, deliveredCompleteZip: false }; + } + } finally { + if (timer) clearTimeout(timer); + storageMock.poison.clear(); + } +} + +describe("ZIP streaming failure after headers are sent (issue #645)", () => { + // Healthy path first: a hung request from a later case would otherwise + // starve it and make an unrelated timeout look like a regression. + it("returns a complete ZIP when storage is healthy", async () => { + const { body, contentType } = createMultipartPayload([ + { name: "file", filename: "a.png", contentType: "image/png", content: PNG }, + { name: "file", filename: "b.jpg", contentType: "image/jpeg", content: JPG }, + { name: "settings", content: JSON.stringify({ width: 50 }) }, + ]); + const res = await testApp.app.inject({ + method: "POST", + url: "/api/v1/tools/image/resize/batch", + headers: { authorization: `Bearer ${token}`, "content-type": contentType }, + body, + }); + + expect(res.statusCode).toBe(200); + expect(new AdmZip(res.rawPayload).getEntries()).toHaveLength(2); + }, 60_000); + + it("svg-to-raster still returns a complete ZIP (it never reads storage mid-stream)", async () => { + // Kept as a guard rather than a failure case: svg-to-raster appends + // in-memory buffers, so it has no post-header storage read to poison. + const { body, contentType } = createMultipartPayload([ + { name: "file", filename: "a.svg", contentType: "image/svg+xml", content: SVG }, + { name: "file", filename: "b.svg", contentType: "image/svg+xml", content: SVG }, + { name: "settings", content: JSON.stringify({ outputFormat: "png" }) }, + ]); + storageMock.poison.add("outputs/"); + try { + const res = await testApp.app.inject({ + method: "POST", + url: "/api/v1/tools/image/svg-to-raster/batch", + headers: { authorization: `Bearer ${token}`, "content-type": contentType }, + body, + }); + expect(res.statusCode).toBe(200); + expect(new AdmZip(res.rawPayload).getEntries()).toHaveLength(2); + } finally { + storageMock.poison.clear(); + } + }, 60_000); + + it("generic /batch terminates instead of hanging, and delivers no complete ZIP", async () => { + const result = await runWithPoisonedOutputs("/api/v1/tools/image/resize/batch", [ + { name: "file", filename: "a.png", contentType: "image/png", content: PNG }, + { name: "file", filename: "b.jpg", contentType: "image/jpeg", content: JPG }, + { name: "settings", content: JSON.stringify({ width: 50 }) }, + { name: "clientJobId", content: "zipfail-batch" }, + ]); + + expect(result.settledWithin, "request hung instead of terminating").toBe(true); + expect(result.deliveredCompleteZip).toBe(false); + }, 60_000); + + it("pdf-to-image /batch terminates instead of hanging", async () => { + const result = await runWithPoisonedOutputs("/api/v1/tools/pdf/pdf-to-jpg/batch", [ + { name: "file", filename: "a.pdf", contentType: "application/pdf", content: PDF_3PAGE }, + { name: "file", filename: "b.pdf", contentType: "application/pdf", content: PDF_2PAGE }, + { name: "settings", content: JSON.stringify({ dpi: 72 }) }, + { name: "clientJobId", content: "zipfail-pdf" }, + ]); + + expect(result.settledWithin, "request hung instead of terminating").toBe(true); + expect(result.deliveredCompleteZip).toBe(false); + }, 60_000); +}); diff --git a/tests/integration/security/adversarial-extended.test.ts b/tests/integration/security/adversarial-extended.test.ts index 3a687de6..7a1b5b5e 100644 --- a/tests/integration/security/adversarial-extended.test.ts +++ b/tests/integration/security/adversarial-extended.test.ts @@ -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", + ]); }); }); diff --git a/tests/integration/test-server.ts b/tests/integration/test-server.ts index c60c3121..4ab50091 100644 --- a/tests/integration/test-server.ts +++ b/tests/integration/test-server.ts @@ -47,6 +47,7 @@ import { registerMfa } from "../../apps/api/src/plugins/mfa.js"; import { oidcRoutes } from "../../apps/api/src/plugins/oidc.js"; import { registerPerUserRateLimit } from "../../apps/api/src/plugins/per-user-rate-limit.js"; import { registerSaml } from "../../apps/api/src/plugins/saml.js"; +import { toolAccessMiddleware } from "../../apps/api/src/plugins/tool-access.js"; import { registerUpload } from "../../apps/api/src/plugins/upload.js"; import { adminOpsRoutes } from "../../apps/api/src/routes/admin-ops.js"; import { analyticsRoutes } from "../../apps/api/src/routes/analytics.js"; @@ -155,6 +156,9 @@ export async function buildTestApp(): Promise { // Auth middleware (must be registered before routes) await authMiddleware(app); + // Tool access gate (mirrors index.ts: after auth, before routes) + await toolAccessMiddleware(app); + // Per-user rate limiting (after auth so request.user is populated) try { await registerPerUserRateLimit(app); diff --git a/tests/integration/tools/image/svg-to-raster.test.ts b/tests/integration/tools/image/svg-to-raster.test.ts index e54912e9..fe3bc65f 100644 --- a/tests/integration/tools/image/svg-to-raster.test.ts +++ b/tests/integration/tools/image/svg-to-raster.test.ts @@ -129,34 +129,33 @@ describe("svg-to-raster", () => { expect(meta.height).toBeLessThanOrEqual(50); }); - it.each([ - "png", - "jpg", - "webp", - ] as const)("converts to output format: %s", async (outputFormat) => { - const { body, contentType } = createMultipartPayload([ - { name: "file", filename: "test.svg", contentType: "image/svg+xml", content: SVG }, - { name: "settings", content: JSON.stringify({ outputFormat }) }, - ]); + it.each(["png", "jpg", "webp"] as const)( + "converts to output format: %s", + async (outputFormat) => { + const { body, contentType } = createMultipartPayload([ + { name: "file", filename: "test.svg", contentType: "image/svg+xml", content: SVG }, + { name: "settings", content: JSON.stringify({ outputFormat }) }, + ]); - const res = await app.inject({ - method: "POST", - url: "/api/v1/tools/image/svg-to-raster", - headers: { authorization: `Bearer ${adminToken}`, "content-type": contentType }, - body, - }); + const res = await app.inject({ + method: "POST", + url: "/api/v1/tools/image/svg-to-raster", + headers: { authorization: `Bearer ${adminToken}`, "content-type": contentType }, + body, + }); - expect(res.statusCode).toBe(200); - const json = JSON.parse(res.body); - const dlRes = await app.inject({ - method: "GET", - url: json.downloadUrl, - headers: { authorization: `Bearer ${adminToken}` }, - }); - const meta = await sharp(Buffer.from(dlRes.rawPayload)).metadata(); - const expectedFormat = outputFormat === "jpg" ? "jpeg" : outputFormat; - expect(meta.format).toBe(expectedFormat); - }); + expect(res.statusCode).toBe(200); + const json = JSON.parse(res.body); + const dlRes = await app.inject({ + method: "GET", + url: json.downloadUrl, + headers: { authorization: `Bearer ${adminToken}` }, + }); + const meta = await sharp(Buffer.from(dlRes.rawPayload)).metadata(); + const expectedFormat = outputFormat === "jpg" ? "jpeg" : outputFormat; + expect(meta.format).toBe(expectedFormat); + }, + ); it("respects DPI setting", async () => { const { body, contentType } = createMultipartPayload([ @@ -572,6 +571,33 @@ describe("svg-to-raster", () => { // ── Batch endpoint coverage ──────────────────────────────────────── describe("batch", () => { + it("keeps index alignment when an upload is empty (issue #645)", async () => { + // The client pairs results with its own file list by index, so a + // dropped zero-byte part would pin this SVG's output to the empty slot. + const { body, contentType } = createMultipartPayload([ + { + name: "file", + filename: "empty.svg", + contentType: "image/svg+xml", + content: Buffer.alloc(0), + }, + { name: "file", filename: "real.svg", contentType: "image/svg+xml", content: SVG }, + { name: "settings", content: JSON.stringify({ outputFormat: "png" }) }, + ]); + + const res = await app.inject({ + method: "POST", + url: "/api/v1/tools/image/svg-to-raster/batch", + headers: { authorization: `Bearer ${adminToken}`, "content-type": contentType }, + body, + }); + + expect(res.statusCode).toBe(200); + const fileResults = JSON.parse(decodeURIComponent(res.headers["x-file-results"] as string)); + expect(fileResults["0"]).toBeUndefined(); + expect(fileResults["1"]).toMatch(/real/); + }); + it("converts multiple SVGs in a single batch request", async () => { const { body, contentType } = createMultipartPayload([ { name: "file", filename: "a.svg", contentType: "image/svg+xml", content: SVG }, diff --git a/tests/unit/api/tool-access-url.test.ts b/tests/unit/api/tool-access-url.test.ts new file mode 100644 index 00000000..7a5da3a3 --- /dev/null +++ b/tests/unit/api/tool-access-url.test.ts @@ -0,0 +1,68 @@ +/** + * Route resolution for the tool access gate (issue #645). + * + * The gate protects whatever tool this resolves, so getting it wrong either + * opens a tool up or 403s something that was never a tool run. It reads the + * matched route pattern rather than the raw URL on purpose: find-my-way + * decodes before matching, so a second parse of the raw string disagrees with + * the router and fails open on `/api/v1/tools/image/%66avicon`. + */ +import { apiToolPath, TOOLS, toolSection } from "@snapotter/shared"; +import { describe, expect, it } from "vitest"; +import { toolIdFromRoute } from "../../../apps/api/src/plugins/tool-access.js"; + +const BATCH_PATTERN = "/api/v1/tools/:section/:toolId/batch"; + +describe("toolIdFromRoute", () => { + it("resolves every tool in the catalog from its own registered path", () => { + const unresolved = TOOLS.filter( + (tool) => toolIdFromRoute(apiToolPath(tool.id)) !== tool.id, + ).map((tool) => tool.id); + expect(unresolved, `tools whose own path does not resolve: ${unresolved.join(", ")}`).toEqual( + [], + ); + }); + + it.each(["batch", "info", "preview", "analyze", "inspect", "generate", "effects"])( + "resolves the /%s sub-path to its parent tool", + (suffix) => { + expect(toolIdFromRoute(`/api/v1/tools/image/resize/${suffix}`)).toBe("resize"); + }, + ); + + it("resolves the parametric batch route from the router's decoded params", () => { + expect(toolIdFromRoute(BATCH_PATTERN, { section: "image", toolId: "resize" })).toBe("resize"); + }); + + it("returns null when the parametric route names an unknown tool", () => { + expect(toolIdFromRoute(BATCH_PATTERN, { section: "image", toolId: "nope" })).toBeNull(); + }); + + it("returns null when the parametric route's section does not match the tool", () => { + expect(toolIdFromRoute(BATCH_PATTERN, { section: "video", toolId: "resize" })).toBeNull(); + }); + + it("returns null when the parametric route has no params", () => { + expect(toolIdFromRoute(BATCH_PATTERN)).toBeNull(); + }); + + it.each([ + ["no matched route", undefined], + ["the catalog root", "/api/v1/tools/"], + ["a single-segment listing", "/api/v1/tools/popular"], + ["an unrelated route", "/api/v1/jobs/:jobId/progress"], + ["a path that merely looks similar", "/api/v1/toolsomething/image/resize"], + ])("returns null for %s", (_label, routeUrl) => { + expect(toolIdFromRoute(routeUrl)).toBeNull(); + }); + + it("returns null for an unknown tool id so the route can still 404", () => { + expect(toolIdFromRoute("/api/v1/tools/image/not-a-real-tool")).toBeNull(); + }); + + it("returns null when a real tool is registered under the wrong section", () => { + const image = TOOLS.find((t) => toolSection(t) === "image"); + if (!image) throw new Error("no image-section tool in the catalog"); + expect(toolIdFromRoute(`/api/v1/tools/video/${image.id}`)).toBeNull(); + }); +});