diff --git a/packages/adapter-utils/src/paperclip-api-launcher.test.ts b/packages/adapter-utils/src/paperclip-api-launcher.test.ts index 7db5694d87a..c68664fef3a 100644 --- a/packages/adapter-utils/src/paperclip-api-launcher.test.ts +++ b/packages/adapter-utils/src/paperclip-api-launcher.test.ts @@ -1,6 +1,7 @@ import { execFile } from "node:child_process"; import { createServer, type IncomingMessage, type Server } from "node:http"; -import { mkdtemp, writeFile, rm } from "node:fs/promises"; +import { mkdtemp, writeFile, readFile, rm } from "node:fs/promises"; +import { randomBytes } from "node:crypto"; import os from "node:os"; import path from "node:path"; import { promisify } from "node:util"; @@ -10,7 +11,19 @@ import { paperclipApiHelperSource } from "./paperclip-api-launcher.js"; const exec = promisify(execFile); const SENTINEL_BEARER = "sentinel-bearer-must-never-leave-the-configured-origin"; -type RecordedRequest = { method: string; url: string; headers: IncomingMessage["headers"]; body: string }; +type RecordedRequest = { method: string; url: string; headers: IncomingMessage["headers"]; body: string; rawBody: Buffer }; + +function countOccurrences(buffer: Buffer, marker: Buffer): number { + let count = 0; + let from = 0; + for (;;) { + const index = buffer.indexOf(marker, from); + if (index === -1) break; + count++; + from = index + marker.length; + } + return count; +} async function startRecordingServer( respond: (req: IncomingMessage) => { status: number; body: string }, @@ -20,7 +33,8 @@ async function startRecordingServer( const chunks: Buffer[] = []; req.on("data", (chunk) => chunks.push(chunk)); req.on("end", () => { - requests.push({ method: req.method ?? "", url: req.url ?? "", headers: req.headers, body: Buffer.concat(chunks).toString("utf8") }); + const rawBody = Buffer.concat(chunks); + requests.push({ method: req.method ?? "", url: req.url ?? "", headers: req.headers, body: rawBody.toString("utf8"), rawBody }); const { status, body } = respond(req); res.writeHead(status, { "content-type": "application/json" }); res.end(body); @@ -114,7 +128,7 @@ describe("paperclip-api helper", () => { it("does not follow a redirect to another origin", async () => { await new Promise((resolve) => configured.server.close(() => resolve())); configured.server = createServer((req, res) => { - configured.requests.push({ method: req.method ?? "", url: req.url ?? "", headers: req.headers, body: "" }); + configured.requests.push({ method: req.method ?? "", url: req.url ?? "", headers: req.headers, body: "", rawBody: Buffer.alloc(0) }); res.writeHead(302, { location: `http://127.0.0.1:${nonconfigured.port}/api/x` }); res.end(); }); @@ -248,4 +262,254 @@ describe("paperclip-api helper", () => { 30_000, ); }); + + function execWithStdin(args: string[], env: NodeJS.ProcessEnv, input: Buffer) { + const result = exec(bin, args, { env, maxBuffer: 64 * 1024 * 1024 }); + result.child.stdin!.end(input); + return result; + } + + describe("a request body from a file or from standard input", () => { + it("sends a body piped through standard input, byte for byte", async () => { + const payload = randomBytes(4096); + const result = await execWithStdin(["POST", "/api/issues/PAP-1/comments", "-d", "@-"], runEnv(), payload); + expect(JSON.parse(result.stdout)).toEqual({ ok: true }); + expect(configured.requests[0]!.rawBody.equals(payload)).toBe(true); + }); + + it("sends a body read from a named file, byte for byte", async () => { + const payload = randomBytes(4096); + const filePath = path.join(root, "body.bin"); + await writeFile(filePath, payload); + const result = await exec(bin, ["POST", "/api/issues/PAP-1/comments", "-d", `@${filePath}`], { env: runEnv() }); + expect(JSON.parse(result.stdout)).toEqual({ ok: true }); + expect(configured.requests[0]!.rawBody.equals(payload)).toBe(true); + }); + + it("keeps sending a literal -d argument as a body", async () => { + await exec(bin, ["POST", "/api/issues/PAP-1/comments", "-d", '{"body":"literal"}'], { env: runEnv() }); + expect(configured.requests[0]!.body).toBe('{"body":"literal"}'); + }); + + it("fails without making a request when the named body file does not exist", async () => { + const filePath = path.join(root, "missing.bin"); + await expect(exec(bin, ["POST", "/api/issues/PAP-1/comments", "-d", `@${filePath}`], { env: runEnv() })).rejects.toMatchObject({ + code: 2, + }); + expect(configured.requests).toHaveLength(0); + }); + }); + + describe("a multipart file upload", () => { + function multipartBoundary(request: RecordedRequest): string { + const match = /boundary=(.+)$/.exec(request.headers["content-type"] ?? ""); + expect(match).toBeTruthy(); + return match![1]!; + } + + it("uploads one file part, with the file content unchanged, and Content-Type set once", async () => { + const payload = randomBytes(2048); + const filePath = path.join(root, "output.webm"); + await writeFile(filePath, payload); + const result = await exec( + bin, + ["POST", "/api/issues/PAP-1/attachments", "-F", `file=@${filePath};type=video/webm`], + { env: runEnv() }, + ); + expect(JSON.parse(result.stdout)).toEqual({ ok: true }); + const request = configured.requests[0]!; + expect(request.headers["content-type"]).toMatch(/^multipart\/form-data; boundary=/); + const boundary = multipartBoundary(request); + expect(countOccurrences(request.rawBody, Buffer.from(`--${boundary}`, "utf8"))).toBe(2); + expect(countOccurrences(request.rawBody, Buffer.from("Content-Disposition: form-data", "utf8"))).toBe(1); + const headEnd = request.rawBody.indexOf("\r\n\r\n") + 4; + const tailStart = request.rawBody.lastIndexOf(`\r\n--${boundary}--`); + expect(request.rawBody.subarray(headEnd, tailStart).equals(payload)).toBe(true); + }); + + it("rejects a caller-supplied Content-Type header in multipart mode", async () => { + const filePath = path.join(root, "output.webm"); + await writeFile(filePath, "content"); + await expect( + exec( + bin, + ["POST", "/api/issues/PAP-1/attachments", "-F", `file=@${filePath};type=video/webm`, "-H", "Content-Type: text/plain"], + { env: runEnv() }, + ), + ).rejects.toMatchObject({ code: 2 }); + expect(configured.requests).toHaveLength(0); + }); + + it("rejects more than one -F file part", async () => { + const filePath = path.join(root, "output.webm"); + await writeFile(filePath, "content"); + await expect( + exec( + bin, + [ + "POST", + "/api/issues/PAP-1/attachments", + "-F", + `file=@${filePath};type=video/webm`, + "-F", + `second=@${filePath};type=video/webm`, + ], + { env: runEnv() }, + ), + ).rejects.toMatchObject({ code: 2 }); + expect(configured.requests).toHaveLength(0); + }); + + it("fails without making a request when the multipart file does not exist", async () => { + const filePath = path.join(root, "missing.webm"); + await expect( + exec(bin, ["POST", "/api/issues/PAP-1/attachments", "-F", `file=@${filePath};type=video/webm`], { env: runEnv() }), + ).rejects.toMatchObject({ code: 2 }); + expect(configured.requests).toHaveLength(0); + }); + + // This character set holds a carriage return, a line feed, a double + // quote, and a backslash. Each of these can inject a header, end a body + // early, or collide with the boundary. The set also holds the + // letters, digits, and hyphen the boundary itself uses. + const MULTIPART_TEST_CHARSET = 'PaperclipFormBoundary0123456789abcdefABCDEF-_\r\n"\\'; + const HAS_FORBIDDEN_MULTIPART_CHAR = /[\r\n"\\]/; + const FIXED_UPLOAD_CONTENT = Buffer.from("sample file content, unchanged"); + + function randomMultipartTestString(minLength: number, maxLength: number): string { + const length = minLength + Math.floor(Math.random() * (maxLength - minLength + 1)); + let value = ""; + for (let i = 0; i < length; i++) { + value += MULTIPART_TEST_CHARSET[Math.floor(Math.random() * MULTIPART_TEST_CHARSET.length)]; + } + return value; + } + + async function runMultipartCase(fieldName: string, fileNameSeed: string, contentType: string) { + const filePath = path.join(root, `upload-${fileNameSeed}`); + await writeFile(filePath, FIXED_UPLOAD_CONTENT); + try { + const outcome = await exec( + bin, + ["POST", "/api/issues/PAP-1/attachments", "-F", `${fieldName}=@${filePath};type=${contentType}`], + { env: runEnv() }, + ); + return { accepted: true as const, result: outcome }; + } catch (error) { + return { accepted: false as const, error: error as { code?: number } }; + } + } + + // This is the one invariant the program's boundary handling must keep. + // Either the program rejects the caller's field name, file name, or + // content type. Or it sends a request with the boundary in exactly the + // two places the multipart format needs, and with the original file + // bytes and nothing else. This same check also catches an injected + // header line: an injected carriage return or line feed moves the "end + // of headers" marker, so the extracted content no longer matches the + // original bytes. + function assertRequestHasOneSafePart(request: RecordedRequest) { + const boundary = multipartBoundary(request); + expect(countOccurrences(request.rawBody, Buffer.from(`--${boundary}`, "utf8"))).toBe(2); + const headersEnd = request.rawBody.indexOf("\r\n\r\n"); + const tailStart = request.rawBody.lastIndexOf(`\r\n--${boundary}--`); + expect(headersEnd).toBeGreaterThan(-1); + expect(tailStart).toBeGreaterThan(headersEnd); + const content = request.rawBody.subarray(headersEnd + 4, tailStart); + expect(content.equals(FIXED_UPLOAD_CONTENT)).toBe(true); + } + + const NAMED_MULTIPART_CASES: Array<{ name: string; fieldName: string; fileName: string; contentType: string }> = [ + { name: "a carriage return in the field name", fieldName: "file\rname", fileName: "plain", contentType: "text/plain" }, + { name: "a line feed in the file name", fieldName: "file", fileName: "plain\nname", contentType: "text/plain" }, + { name: "a double quote in the content type", fieldName: "file", fileName: "plain", contentType: 'text/plain"x' }, + { name: "a backslash in the field name", fieldName: "fi\\le", fileName: "plain", contentType: "text/plain" }, + { name: "a header-ending double CRLF in the field name", fieldName: "file\r\n\r\ninjected", fileName: "plain", contentType: "text/plain" }, + { name: "the boundary's own characters in the content type (accepted, still one safe part)", fieldName: "file", fileName: "plain", contentType: "PaperclipFormBoundaryabc123" }, + ]; + + it.each(NAMED_MULTIPART_CASES)("$name", async ({ fieldName, fileName, contentType }) => { + configured.requests.length = 0; + const outcome = await runMultipartCase(fieldName, fileName, contentType); + const dangerous = HAS_FORBIDDEN_MULTIPART_CHAR.test(fieldName) || HAS_FORBIDDEN_MULTIPART_CHAR.test(fileName) || HAS_FORBIDDEN_MULTIPART_CHAR.test(contentType); + if (dangerous) { + expect(outcome.accepted).toBe(false); + expect(!outcome.accepted && outcome.error.code).toBe(2); + expect(configured.requests).toHaveLength(0); + return; + } + expect(outcome.accepted).toBe(true); + assertRequestHasOneSafePart(configured.requests[0]!); + }); + + it( + "either rejects a dangerous field/file/content-type, or sends exactly one safe part, for 200 generated inputs (property test)", + async () => { + for (let trial = 0; trial < 200; trial++) { + configured.requests.length = 0; + const fieldName = randomMultipartTestString(1, 12); + const fileNameSeed = `${trial}-${randomMultipartTestString(1, 12)}`; + const contentType = randomMultipartTestString(1, 24); + const outcome = await runMultipartCase(fieldName, fileNameSeed, contentType); + const dangerous = + HAS_FORBIDDEN_MULTIPART_CHAR.test(fieldName) + || HAS_FORBIDDEN_MULTIPART_CHAR.test(fileNameSeed) + || HAS_FORBIDDEN_MULTIPART_CHAR.test(contentType); + if (dangerous) { + expect(outcome.accepted, `trial ${trial} accepted a dangerous field/file/content-type`).toBe(false); + expect(!outcome.accepted && outcome.error.code, `trial ${trial} rejected with an unexpected exit code`).toBe(2); + expect(configured.requests, `trial ${trial} sent a request despite rejecting`).toHaveLength(0); + continue; + } + expect(outcome.accepted, `trial ${trial} rejected a safe field/file/content-type`).toBe(true); + assertRequestHasOneSafePart(configured.requests[0]!); + } + }, + 60_000, + ); + }); + + describe("the response status code and output file", () => { + it("writes the numeric status code and nothing else for a 200 response", async () => { + const result = await exec(bin, ["GET", "/api/agents/me", "--status"], { env: runEnv() }); + expect(result.stdout).toBe("200"); + }); + + it("writes the numeric status code and nothing else for a 404 response", async () => { + await new Promise((resolve) => configured.server.close(() => resolve())); + configured.server = createServer((req, res) => { + res.writeHead(404, { "content-type": "application/json" }); + res.end('{"error":"not found"}'); + }); + await new Promise((resolve) => configured.server.listen(configured.port, "127.0.0.1", resolve)); + const result = await exec(bin, ["GET", "/api/agents/me", "--status"], { env: runEnv() }); + expect(result.stdout).toBe("404"); + }); + + it("does not fail the process on a non-2xx status when --status is given", async () => { + await new Promise((resolve) => configured.server.close(() => resolve())); + configured.server = createServer((req, res) => { + res.writeHead(404, { "content-type": "application/json" }); + res.end('{"error":"not found"}'); + }); + await new Promise((resolve) => configured.server.listen(configured.port, "127.0.0.1", resolve)); + await expect(exec(bin, ["GET", "/api/agents/me", "--status"], { env: runEnv() })).resolves.toMatchObject({ stdout: "404" }); + }); + + it("writes the response body to -o instead of standard output", async () => { + const outputPath = path.join(root, "response.json"); + const result = await exec(bin, ["GET", "/api/agents/me", "-o", outputPath], { env: runEnv() }); + expect(result.stdout).toBe(""); + const written = await readFile(outputPath, "utf8"); + expect(JSON.parse(written)).toEqual({ ok: true }); + }); + + it("combines -o and --status the way a status-then-body curl call would", async () => { + const outputPath = path.join(root, "response.json"); + const result = await exec(bin, ["GET", "/api/agents/me", "-o", outputPath, "--status"], { env: runEnv() }); + expect(result.stdout).toBe("200"); + const written = await readFile(outputPath, "utf8"); + expect(JSON.parse(written)).toEqual({ ok: true }); + }); + }); }); diff --git a/packages/adapter-utils/src/paperclip-api-launcher.ts b/packages/adapter-utils/src/paperclip-api-launcher.ts index 55cf1a3694a..c9c265e1a6d 100644 --- a/packages/adapter-utils/src/paperclip-api-launcher.ts +++ b/packages/adapter-utils/src/paperclip-api-launcher.ts @@ -4,6 +4,13 @@ export function paperclipApiHelperSource(): string { // paperclip-api: call the Paperclip API without putting the bearer token on a // command line. The token stays in the process environment; this program // reads it there and adds it to the request itself. +const fsPromises = require('node:fs/promises'); +const pathModule = require('node:path'); +const cryptoModule = require('node:crypto'); + +const USAGE = 'usage: paperclip-api [-H "Name: Value"]... ' + + '[-d |-d @-|-d @] [-F \'field=@;type=\'] [-o ] [--status]'; + const FORBIDDEN_HEADER_NAMES = new Set([ 'authorization', 'host', 'proxy-authorization', 'proxy-connection', 'forwarded', 'x-forwarded-for', 'x-forwarded-host', 'x-forwarded-proto', 'x-forwarded-port', 'x-real-ip', 'via', @@ -14,13 +21,37 @@ function usageError(message) { process.exit(2); } +function hasHeaderNamed(headers, name) { + return Object.keys(headers).some((key) => key.toLowerCase() === name.toLowerCase()); +} + +// Parse "=@;type=", the one multipart shape +// this program accepts. The field name and the content type must not carry +// the literal text "=@" or ";type=". Keep these two character sequences out +// of any generated test data for this parser. +function parseMultipartArg(raw) { + const marker = raw.indexOf('=@'); + if (marker < 0) usageError('-F needs a "field=@path;type=mime" argument'); + const fieldName = raw.slice(0, marker); + const rest = raw.slice(marker + 2); + const typeMarker = rest.indexOf(';type='); + if (typeMarker < 0) usageError('-F needs a ";type=" suffix'); + const filePath = rest.slice(0, typeMarker); + const contentType = rest.slice(typeMarker + ';type='.length); + if (!fieldName || !filePath || !contentType) usageError('-F needs a non-empty field name, file path, and content type'); + return { fieldName, filePath, contentType }; +} + function parseArgs(argv) { const method = argv[0]; const relativePath = argv[1]; - if (!method || !relativePath) usageError('usage: paperclip-api [-H "Name: Value"]... [-d ]'); + if (!method || !relativePath) usageError(USAGE); if (!/^[A-Z]+$/.test(method)) usageError('the method must be an uppercase HTTP verb'); const headers = {}; - let body; + let bodySpec; + let multipartSpec; + let outputPath; + let wantStatus = false; for (let i = 2; i < argv.length; i++) { const arg = argv[i]; if (arg === '-H') { @@ -32,13 +63,24 @@ function parseArgs(argv) { if (FORBIDDEN_HEADER_NAMES.has(name.toLowerCase())) usageError('this program sets the "' + name + '" header itself; the caller must not set it'); headers[name] = value; } else if (arg === '-d') { - body = argv[++i]; - if (body === undefined) usageError('-d needs a body argument'); + bodySpec = argv[++i]; + if (bodySpec === undefined) usageError('-d needs a body argument'); + } else if (arg === '-F') { + const raw = argv[++i]; + if (raw === undefined) usageError('-F needs a "field=@path;type=mime" argument'); + if (multipartSpec) usageError('this program accepts at most one -F file part'); + multipartSpec = parseMultipartArg(raw); + } else if (arg === '-o') { + outputPath = argv[++i]; + if (outputPath === undefined) usageError('-o needs a file path argument'); + } else if (arg === '--status') { + wantStatus = true; } else { usageError('unknown argument "' + arg + '"'); } } - return { method, relativePath, headers, body }; + if (bodySpec !== undefined && multipartSpec) usageError('-d and -F cannot be used together'); + return { method, relativePath, headers, bodySpec, multipartSpec, outputPath, wantStatus }; } // Accept only a path relative to the configured Paperclip API. Reject an @@ -81,17 +123,107 @@ function buildRequestUrl(base, relativePath) { return url; } +function readStdinBytes() { + return new Promise((resolve, reject) => { + const chunks = []; + process.stdin.on('data', (chunk) => chunks.push(chunk)); + process.stdin.on('end', () => resolve(Buffer.concat(chunks))); + process.stdin.on('error', reject); + }); +} + +// Resolve the -d argument to a byte body. "@-" reads standard input. +// "@" reads the named file. Anything else keeps its current meaning: +// a literal body. Read the file or the stream as bytes. Do not assume it is +// text. This way a binary attachment or a large comment body survives +// unchanged. +async function resolveBodySpec(bodySpec) { + if (bodySpec === '@-') return readStdinBytes(); + if (bodySpec.startsWith('@')) { + const filePath = bodySpec.slice(1); + try { return await fsPromises.readFile(filePath); } + catch { usageError('could not read the body file "' + filePath + '"'); } + } + return bodySpec; +} + +function countOccurrences(buffer, marker) { + let count = 0; + let from = 0; + for (;;) { + const index = buffer.indexOf(marker, from); + if (index === -1) break; + count++; + from = index + marker.length; + } + return count; +} + +// A caller controls the field name, the file name, and the content type of a +// multipart upload. Each value enters the assembled request as a header +// line. Reject a carriage return, a line feed, a double quote, and a +// backslash. This stops a caller from adding a header, changing a header, or +// ending the body early. +function assertSafeMultipartValue(value, label) { + if (/[\r\n"\\]/.test(value)) usageError('a multipart ' + label + ' must not contain a carriage return, a line feed, a double quote, or a backslash'); +} + +// Build one multipart/form-data body holding exactly one file part. The +// boundary comes from a cryptographic random source. It never derives from +// caller input, so a caller cannot predict it. After assembly, count the +// boundary marker in the finished body. It must appear exactly twice: once +// opening the part, once closing it. A third occurrence means a +// caller-controlled value, most plausibly the file content, collided with +// the boundary. Do not send that request. +async function buildMultipartBody(spec) { + const { fieldName, filePath, contentType } = spec; + const fileName = pathModule.basename(filePath); + assertSafeMultipartValue(fieldName, 'field name'); + assertSafeMultipartValue(fileName, 'file name'); + assertSafeMultipartValue(contentType, 'content type'); + let fileBytes; + try { fileBytes = await fsPromises.readFile(filePath); } + catch { usageError('could not read the multipart file "' + filePath + '"'); } + const boundary = 'PaperclipFormBoundary' + cryptoModule.randomBytes(24).toString('hex'); + const head = Buffer.from( + '--' + boundary + '\r\n' + + 'Content-Disposition: form-data; name="' + fieldName + '"; filename="' + fileName + '"\r\n' + + 'Content-Type: ' + contentType + '\r\n\r\n', + 'utf8', + ); + const tail = Buffer.from('\r\n--' + boundary + '--\r\n', 'utf8'); + const assembled = Buffer.concat([head, fileBytes, tail]); + const occurrences = countOccurrences(assembled, Buffer.from('--' + boundary, 'utf8')); + if (occurrences !== 2) usageError('a multipart file part could not be assembled safely'); + return { body: assembled, contentType: 'multipart/form-data; boundary=' + boundary }; +} + async function main() { - const { method, relativePath, headers, body } = parseArgs(process.argv.slice(2)); + const parsed = parseArgs(process.argv.slice(2)); + const { method, relativePath, headers, outputPath, wantStatus } = parsed; assertSafeRelativeApiPath(relativePath); const base = readConfiguredBase(); const url = buildRequestUrl(base, relativePath); const bearer = process.env.PAPERCLIP_API_KEY; if (!bearer) usageError('PAPERCLIP_API_KEY is not set'); - if (body !== undefined && !('Content-Type' in headers)) headers['Content-Type'] = 'application/json'; + + let body; + if (parsed.multipartSpec) { + // The program owns the multipart Content-Type header, the same way it + // owns Authorization. This header carries the boundary. A caller value + // here can smuggle in a different boundary. + if (hasHeaderNamed(headers, 'content-type')) usageError('this program sets the multipart Content-Type header itself; the caller must not set it'); + const multipart = await buildMultipartBody(parsed.multipartSpec); + body = multipart.body; + headers['Content-Type'] = multipart.contentType; + } else if (parsed.bodySpec !== undefined) { + body = await resolveBodySpec(parsed.bodySpec); + if (!('Content-Type' in headers)) headers['Content-Type'] = 'application/json'; + } if (process.env.PAPERCLIP_RUN_ID && !('X-Paperclip-Run-Id' in headers)) headers['X-Paperclip-Run-Id'] = process.env.PAPERCLIP_RUN_ID; // Set once, here. Nothing above this line can add or change this header. headers['Authorization'] = 'Bearer ' + bearer; + let response; try { response = await fetch(url, { method, headers, body, redirect: 'error', signal: AbortSignal.timeout(30000) }); @@ -100,9 +232,21 @@ async function main() { process.exitCode = 1; return; } - const text = await response.text(); - process.stdout.write(text); - if (!response.ok) process.exitCode = 1; + const responseBytes = Buffer.from(await response.arrayBuffer()); + if (outputPath) { + await fsPromises.writeFile(outputPath, responseBytes); + } else if (!wantStatus) { + process.stdout.write(responseBytes); + } + if (wantStatus) { + // The caller reads the numeric status and decides what to do next, the + // same way it already reads a response body written to -o. Do not fail + // the process here on a non-2xx status. That decision belongs to the + // caller, not to this program. + process.stdout.write(String(response.status)); + } else if (!response.ok) { + process.exitCode = 1; + } } main().catch(() => { process.stderr.write('paperclip-api: an unexpected error stopped the request.\n'); process.exitCode = 1; }); `;