From 0c5da4de07c3d1b7bb910c3ddf8152f054c82d86 Mon Sep 17 00:00:00 2001 From: Priya Raman Date: Wed, 26 Aug 2026 21:28:27 +0000 Subject: [PATCH] refactor(adapter-utils): carry the http2 bridge forward response body as bytes The host held two copies of every forwarded body on the HTTP/2 bridge path: readBridgeForwardResponseBody decoded the response to a UTF-8 string, then the caller encoded it back to bytes for the sandbox stream. This change carries the body as one Buffer end to end. - readBridgeForwardResponseBody now returns Promise. - forwardBridgeRequest accepts a string or Uint8Array request body and returns a Buffer response body. Every error path returns a Buffer. - The HTTP/2 forward handler passes and returns raw bytes with no toString("utf8") call. - The file bridge call site converts the body to a string, so the file bridge response file format stays unchanged. - postedIssueCommentLogMarker now accepts a Buffer or a string. It decodes at most 4,096 bytes, and it logs a comment id only when the parsed value is a canonical lowercase UUID. Co-authored-by: Paperclip --- .../src/execution-target-sandbox.test.ts | 196 ++++++++++++++++-- .../adapter-utils/src/execution-target.ts | 93 ++++++--- 2 files changed, 244 insertions(+), 45 deletions(-) diff --git a/packages/adapter-utils/src/execution-target-sandbox.test.ts b/packages/adapter-utils/src/execution-target-sandbox.test.ts index 3a26fbc439f..d173cdd6534 100644 --- a/packages/adapter-utils/src/execution-target-sandbox.test.ts +++ b/packages/adapter-utils/src/execution-target-sandbox.test.ts @@ -25,6 +25,7 @@ import { formatAdapterExecutionTimeoutStartLogLine, parseAdapterExecutionTarget, postedIssueCommentLogMarker, + POSTED_COMMENT_MARKER_DECODE_LIMIT_BYTES, resolveAdapterExecutionTargetTimeout, resolveAdapterExecutionTargetTimeoutSec, runAdapterExecutionTargetProcess, @@ -119,10 +120,46 @@ describe("sandbox adapter execution targets", () => { const cleanupDirs: string[] = []; it("records successful issue comment ids for attribution recovery", () => { - expect(postedIssueCommentLogMarker("POST", "/api/issues/issue-1/comments", 201, '{"id":"comment-1"}')) - .toBe("comment id: comment-1\n"); - expect(postedIssueCommentLogMarker("POST", "/api/issues/issue-1/comments", 401, '{"id":"comment-1"}')) - .toBeNull(); + const uuid = "550e8400-e29b-41d4-a716-446655440000"; + expect(postedIssueCommentLogMarker("POST", "/api/issues/issue-1/comments", 201, `{"id":"${uuid}"}`)).toBe( + `comment id: ${uuid}\n`, + ); + expect(postedIssueCommentLogMarker("POST", "/api/issues/issue-1/comments", 401, `{"id":"${uuid}"}`)).toBeNull(); + // The function accepts a `Buffer` body, matching the bytes the http2 path + // hands it after it drops the string round trip. + expect( + postedIssueCommentLogMarker( + "POST", + "/api/issues/issue-1/comments", + 201, + Buffer.from(`{"id":"${uuid}"}`, "utf8"), + ), + ).toBe(`comment id: ${uuid}\n`); + }); + + it("the comment marker returns null for an identifier that is not a canonical UUID", () => { + expect( + postedIssueCommentLogMarker("POST", "/api/issues/issue-1/comments", 201, '{"id":"comment-1"}'), + ).toBeNull(); + // Uppercase hex is a valid UUID, but not a canonical *lowercase* UUID. + expect( + postedIssueCommentLogMarker( + "POST", + "/api/issues/issue-1/comments", + 201, + '{"id":"550E8400-E29B-41D4-A716-446655440000"}', + ), + ).toBeNull(); + }); + + it("the comment marker returns null for a body past the decode limit", () => { + const uuid = "550e8400-e29b-41d4-a716-446655440000"; + const oversized = Buffer.from( + JSON.stringify({ id: uuid, padding: "x".repeat(POSTED_COMMENT_MARKER_DECODE_LIMIT_BYTES) }), + "utf8", + ); + expect(oversized.length).toBeGreaterThan(POSTED_COMMENT_MARKER_DECODE_LIMIT_BYTES); + expect(postedIssueCommentLogMarker("POST", "/api/issues/issue-1/comments", 201, oversized)).toBeNull(); }); afterEach(async () => { @@ -3264,10 +3301,10 @@ describe("sandbox adapter execution targets", () => { */ function http2TestRequest( session: http2.ClientHttp2Session, - request: { method: string; path: string; headers?: Record; body?: string }, - ): Promise<{ status: number; headers: Record; body: string }> { + request: { method: string; path: string; headers?: Record; body?: string | Buffer }, + ): Promise<{ status: number; headers: Record; body: string; bodyBytes: Buffer }> { return new Promise((resolve, reject) => { - const body = Buffer.from(request.body ?? "", "utf8"); + const body = Buffer.isBuffer(request.body) ? request.body : Buffer.from(request.body ?? "", "utf8"); const stream = session.request( { ":method": request.method, ":path": request.path, ...request.headers }, { endStream: body.length === 0 }, @@ -3284,7 +3321,10 @@ describe("sandbox adapter execution targets", () => { } }); stream.on("data", (chunk: Buffer) => chunks.push(chunk)); - stream.once("end", () => resolve({ status, headers, body: Buffer.concat(chunks).toString("utf8") })); + stream.once("end", () => { + const bodyBytes = Buffer.concat(chunks); + resolve({ status, headers, body: bodyBytes.toString("utf8"), bodyBytes }); + }); stream.once("error", (error) => reject(error)); if (body.length > 0) stream.end(body); else if (!stream.writableEnded) stream.end(); @@ -3294,7 +3334,10 @@ describe("sandbox adapter execution targets", () => { // Start a host API server that records each forwarded request, so a test can // assert the real token and the run id reach the host, or that a rejected // request never forwards. - async function startRecordingApiServer(): Promise<{ + async function startRecordingApiServer(options?: { + /** Overrides the default `{ ok: true }` JSON reply with these raw bytes. */ + responseBody?: Buffer; + }): Promise<{ origin: string; requests: Array<{ method: string; @@ -3302,6 +3345,7 @@ describe("sandbox adapter execution targets", () => { auth: string | null; runId: string | null; headers: Record; + body: Buffer; }>; close: () => Promise; }> { @@ -3311,21 +3355,28 @@ describe("sandbox adapter execution targets", () => { auth: string | null; runId: string | null; headers: Record; + body: Buffer; }> = []; + const responseBody = options?.responseBody ?? Buffer.from(JSON.stringify({ ok: true }), "utf8"); const server = createServer((req, res) => { - const headers: Record = {}; - for (const [key, value] of Object.entries(req.headers)) { - if (typeof value === "string") headers[key] = value; - } - requests.push({ - method: req.method ?? "GET", - url: req.url ?? "/", - auth: req.headers.authorization ?? null, - runId: typeof req.headers["x-paperclip-run-id"] === "string" ? req.headers["x-paperclip-run-id"] : null, - headers, + const bodyChunks: Buffer[] = []; + req.on("data", (chunk: Buffer) => bodyChunks.push(chunk)); + req.on("end", () => { + const headers: Record = {}; + for (const [key, value] of Object.entries(req.headers)) { + if (typeof value === "string") headers[key] = value; + } + requests.push({ + method: req.method ?? "GET", + url: req.url ?? "/", + auth: req.headers.authorization ?? null, + runId: typeof req.headers["x-paperclip-run-id"] === "string" ? req.headers["x-paperclip-run-id"] : null, + headers, + body: Buffer.concat(bodyChunks), + }); + res.writeHead(200, { "content-type": "application/json" }); + res.end(responseBody); }); - res.writeHead(200, { "content-type": "application/json" }); - res.end(JSON.stringify({ ok: true })); }); await new Promise((resolve, reject) => { server.once("error", reject); @@ -3460,6 +3511,109 @@ describe("sandbox adapter execution targets", () => { expect(control.stopCount).toBeGreaterThanOrEqual(1); }, 20000); + it("the HTTP/2 path forwards malformed UTF-8 request bytes to the host API unchanged", async () => { + // A lone continuation byte and a truncated multi-byte sequence: bytes no + // UTF-8 decode-then-encode round trip preserves. + const malformed = Buffer.from([0x80, 0xc3, 0x28, 0xff, 0xfe, 0x00]); + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-http2-req-bytes-")); + cleanupDirs.push(rootDir); + const remoteCwd = path.join(rootDir, "workspace"); + await mkdir(remoteCwd, { recursive: true }); + const api = await startRecordingApiServer(); + const sessionRef: { current: http2.ClientHttp2Session | null } = { current: null }; + let bridgeToken = ""; + const { runner } = makeHttp2SelectionRunner((ctx) => { + bridgeToken = ctx.bridgeToken; + ctx.emitReady(); + sessionRef.current = ctx.connectHttp2(); + }); + const target: AdapterSandboxExecutionTarget = { + kind: "remote", + transport: "sandbox", + providerKey: "daytona", + remoteCwd, + timeoutMs: 30_000, + runner, + effectiveCapabilities: duplexCapabilities(true), + }; + + const bridge = await startAdapterExecutionTargetPaperclipBridge({ + runId: "run-req-bytes", + target, + runtimeRootDir: path.join(remoteCwd, ".paperclip-runtime", "codex"), + adapterKey: "codex", + hostApiToken: "real-run-jwt", + hostApiUrl: api.origin, + enableSandboxDuplexBridge: true, + }); + try { + expect(bridge?.env.PAPERCLIP_API_BRIDGE_MODE).toBe("http2_v1"); + await waitForCondition(() => sessionRef.current !== null, "the http2 client session to open", 4000); + await http2TestRequest(sessionRef.current!, { + method: "POST", + path: "/api/issues/issue-1/comments", + headers: { authorization: `Bearer ${bridgeToken}`, "content-type": "application/octet-stream" }, + body: malformed, + }); + await waitForCondition(() => api.requests.length >= 1, "the host to forward the http2 request", 4000); + expect(api.requests[0].body.equals(malformed)).toBe(true); + } finally { + sessionRef.current?.close(); + await bridge?.stop(); + await api.close(); + } + }, 20000); + + it("the HTTP/2 path returns malformed UTF-8 host response bytes to the sandbox unchanged", async () => { + const malformed = Buffer.from([0xff, 0xfe, 0x80, 0x81, 0x00, 0x01]); + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-http2-resp-bytes-")); + cleanupDirs.push(rootDir); + const remoteCwd = path.join(rootDir, "workspace"); + await mkdir(remoteCwd, { recursive: true }); + const api = await startRecordingApiServer({ responseBody: malformed }); + const sessionRef: { current: http2.ClientHttp2Session | null } = { current: null }; + let bridgeToken = ""; + const { runner } = makeHttp2SelectionRunner((ctx) => { + bridgeToken = ctx.bridgeToken; + ctx.emitReady(); + sessionRef.current = ctx.connectHttp2(); + }); + const target: AdapterSandboxExecutionTarget = { + kind: "remote", + transport: "sandbox", + providerKey: "daytona", + remoteCwd, + timeoutMs: 30_000, + runner, + effectiveCapabilities: duplexCapabilities(true), + }; + + const bridge = await startAdapterExecutionTargetPaperclipBridge({ + runId: "run-resp-bytes", + target, + runtimeRootDir: path.join(remoteCwd, ".paperclip-runtime", "codex"), + adapterKey: "codex", + hostApiToken: "real-run-jwt", + hostApiUrl: api.origin, + enableSandboxDuplexBridge: true, + }); + try { + expect(bridge?.env.PAPERCLIP_API_BRIDGE_MODE).toBe("http2_v1"); + await waitForCondition(() => sessionRef.current !== null, "the http2 client session to open", 4000); + const response = await http2TestRequest(sessionRef.current!, { + method: "GET", + path: "/api/agents/me", + headers: { authorization: `Bearer ${bridgeToken}` }, + }); + expect(response.status).toBe(200); + expect(response.bodyBytes.equals(malformed)).toBe(true); + } finally { + sessionRef.current?.close(); + await bridge?.stop(); + await api.close(); + } + }, 20000); + it("falls back to the file bridge when a post-READY pre-bind flood exceeds the aggregate ceiling", async () => { // The gateway sends a valid READY, then floods the channel before the broker // binds. The pre-READY buffer cap does not bound the post-READY replay buffer, diff --git a/packages/adapter-utils/src/execution-target.ts b/packages/adapter-utils/src/execution-target.ts index 2a083f8a016..f147cf61948 100644 --- a/packages/adapter-utils/src/execution-target.ts +++ b/packages/adapter-utils/src/execution-target.ts @@ -88,13 +88,36 @@ import type { LocalProcessSandboxOptions } from "./local-process-sandbox.js"; export type { RuntimeProgressSink } from "./runtime-progress.js"; -export function postedIssueCommentLogMarker(method: string, requestPath: string, status: number, body: string) { +/** + * The decode limit for {@link postedIssueCommentLogMarker}. The response body + * is untrusted output, so the host never decodes more than this many bytes of + * it to build one log line, and it never allocates a second full-size string + * for a body over this limit. + */ +export const POSTED_COMMENT_MARKER_DECODE_LIMIT_BYTES = 4096; + +const CANONICAL_LOWERCASE_UUID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/; + +export function postedIssueCommentLogMarker( + method: string, + requestPath: string, + status: number, + body: Buffer | string, +) { if (method !== "POST" || !/^\/api\/issues\/[^/]+\/comments$/.test(requestPath) || status < 200 || status >= 300) { return null; } + const bodyBytes = typeof body === "string" ? Buffer.byteLength(body, "utf8") : body.length; + if (bodyBytes > POSTED_COMMENT_MARKER_DECODE_LIMIT_BYTES) { + return null; + } + const decoded = + typeof body === "string" ? body : body.toString("utf8", 0, POSTED_COMMENT_MARKER_DECODE_LIMIT_BYTES); try { - const parsed = JSON.parse(body) as { id?: unknown }; - return typeof parsed.id === "string" && parsed.id.length > 0 ? `comment id: ${parsed.id}\n` : null; + const parsed = JSON.parse(decoded) as { id?: unknown }; + return typeof parsed.id === "string" && CANONICAL_LOWERCASE_UUID_PATTERN.test(parsed.id) + ? `comment id: ${parsed.id}\n` + : null; } catch { return null; } @@ -1505,7 +1528,8 @@ function bridgeResponseBodyLimitError(maxBodyBytes: number): Error { } /** - * Read the forward response body into a string. The reader bounds the body with + * Read the forward response body into one `Buffer`. The reader carries the raw + * bytes end to end and holds no separate string copy. It bounds the body with * two controls. The per-request `maxBodyBytes` limit rejects a body larger than * the configured per-request ceiling. The optional host aggregate byte ledger * bounds the retained bytes across all live routes. @@ -1524,7 +1548,7 @@ async function readBridgeForwardResponseBody( response: Response, maxBodyBytes: number, ledger?: DuplexAggregateByteLedger | null, -): Promise { +): Promise { const rawContentLength = response.headers.get("content-length"); if (rawContentLength) { const contentLength = Number.parseInt(rawContentLength, 10); @@ -1534,7 +1558,7 @@ async function readBridgeForwardResponseBody( } if (!response.body) { - return ""; + return Buffer.alloc(0); } const reader = response.body.getReader(); @@ -1579,7 +1603,7 @@ async function readBridgeForwardResponseBody( } tokens.push(concatToken); } - return Buffer.concat(chunks, totalBytes).toString("utf8"); + return Buffer.concat(chunks, totalBytes); } finally { if (ledger) { for (const token of tokens) { @@ -4225,14 +4249,17 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { path: string; query: string; headers: Record; - /** The file bridge passes the whole request body here as one string. */ - body?: string; + /** + * The file bridge passes the whole request body here as one string. The + * http2 path passes the raw request bytes with no string round trip. + */ + body?: string | Uint8Array; }, signal?: AbortSignal, options?: { suppressDebugLog?: boolean; }, - ): Promise<{ status: number; headers: Record; body: string }> => { + ): Promise<{ status: number; headers: Record; body: Buffer }> => { const method = request.method.trim().toUpperCase() || "GET"; // The per-request debug log prints the method, the path, and the query. The // duplex path suppresses it, so no route or query rides a log line there. The @@ -4257,14 +4284,22 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { const timeoutSignal = AbortSignal.timeout(forwardTimeoutMs); const forwardSignal = signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal; // Build the request-body init. A GET or a HEAD carries no body. The file - // bridge passes the whole body as one string. + // bridge passes the whole body as one string; the http2 path passes the + // raw request bytes. const forwardInit: RequestInit = { method, headers, signal: forwardSignal, }; - if (method !== "GET" && method !== "HEAD" && typeof request.body === "string") { - forwardInit.body = request.body; + if ( + method !== "GET" && + method !== "HEAD" && + (typeof request.body === "string" || request.body instanceof Uint8Array) + ) { + // `fetch` accepts a `Uint8Array` body at runtime. The DOM `BodyInit` type + // pins the typed-array generic to a concrete `ArrayBuffer`, so a + // `Buffer`'s wider `ArrayBufferLike` generic needs this assertion. + forwardInit.body = request.body as BodyInit; } const response = await fetch(buildBridgeForwardUrl(hostApiUrl, request), forwardInit); if (emitDebugLog) { @@ -4285,7 +4320,7 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { // non-retryable 504 and marks the outcome indeterminate, exactly like an // aborted in-flight forward. The in-sandbox server maps the indeterminate 504 // to a non-retryable 409 for both the file bridge and the duplex broker. - let responseBody: string; + let responseBody: Buffer; try { responseBody = await readBridgeForwardResponseBody( response, @@ -4300,9 +4335,11 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { return { status: 502, headers: { "content-type": "application/json" }, - body: JSON.stringify({ - error: error instanceof Error ? error.message : String(error), - }), + body: Buffer.from( + JSON.stringify({ + error: error instanceof Error ? error.message : String(error), + }), + ), }; } return { @@ -4311,11 +4348,13 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { "content-type": "application/json", "x-paperclip-bridge-outcome": "indeterminate", }, - body: JSON.stringify({ - error: error instanceof Error ? error.message : String(error), - outcome: "indeterminate", - retryable: false, - }), + body: Buffer.from( + JSON.stringify({ + error: error instanceof Error ? error.message : String(error), + outcome: "indeterminate", + retryable: false, + }), + ), }; } const commentMarker = postedIssueCommentLogMarker(method, request.path, response.status, responseBody); @@ -4546,7 +4585,7 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { path: request.pathname, query: request.query, headers: request.headers, - body: request.body.toString("utf8"), + body: request.body, }, undefined, { suppressDebugLog: true }, @@ -4669,7 +4708,13 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { maxBodyBytes, getRuntimeParentContext: input.getRuntimeParentContext, runtimeSpan: input.runtimeSpan, - handleRequest: async (request, options) => forwardBridgeRequest(request, options?.signal), + // The file bridge response file holds the body as a JSON string field, so + // this call site converts the forwarded bytes to a string here. It is the + // one place on the file bridge path that creates a string copy. + handleRequest: async (request, options) => { + const result = await forwardBridgeRequest(request, options?.signal); + return { status: result.status, headers: result.headers, body: result.body.toString("utf8") }; + }, }); server = await startSandboxCallbackBridgeServer({ runner,