diff --git a/CHANGELOG.md b/CHANGELOG.md index 36229582..c2bab5cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ All notable user-visible changes to Hunk are documented in this file. - Hardened the local session daemon against browser-originated requests by validating Host and Origin headers and requiring JSON content types for API posts. - Disabled the generic broker HTTP API by default so Hunk's supported session API is the only app-daemon command surface. +- Bounded session daemon memory by capping HTTP request body and websocket message sizes and rejecting session registrations with oversized file, hunk, patch, comment, or note payloads. ## [0.13.0] - 2026-05-18 diff --git a/packages/session-broker-bun/src/serve.ts b/packages/session-broker-bun/src/serve.ts index 40a850dd..67cdd920 100644 --- a/packages/session-broker-bun/src/serve.ts +++ b/packages/session-broker-bun/src/serve.ts @@ -1,4 +1,8 @@ -import type { SessionServerMessage } from "@hunk/session-broker-core"; +import { + MAX_WS_MESSAGE_BYTES, + utf8ByteLength, + type SessionServerMessage, +} from "@hunk/session-broker-core"; import type { SessionBrokerDaemon } from "@hunk/session-broker"; export interface ServeSessionBrokerDaemonOptions< @@ -88,11 +92,20 @@ export function serveSessionBrokerDaemon< return (await options.notFound?.(request)) ?? defaultNotFound(); }, websocket: { + // Let Bun reject oversized frames at the protocol layer before they are ever buffered. + maxPayloadLength: MAX_WS_MESSAGE_BYTES, message: (socket, message) => { if (typeof message !== "string") { return; } + // Defense in depth: Bun's maxPayloadLength already bounds raw frames, but guard the + // decoded string too so a registration payload cannot be parsed unbounded here. + if (utf8ByteLength(message) > MAX_WS_MESSAGE_BYTES) { + socket.close(1009, "Message exceeds the session broker size limit."); + return; + } + options.daemon.handleConnectionMessage(socket, message); }, close: (socket) => { diff --git a/packages/session-broker-core/src/index.ts b/packages/session-broker-core/src/index.ts index 46c5960a..c75d7464 100644 --- a/packages/session-broker-core/src/index.ts +++ b/packages/session-broker-core/src/index.ts @@ -1,5 +1,6 @@ export * from "./types"; export * from "./brokerWire"; +export * from "./limits"; export * from "./brokerState"; export * from "./selectors"; export * from "./sessionTerminalMetadata"; diff --git a/packages/session-broker-core/src/limits.test.ts b/packages/session-broker-core/src/limits.test.ts new file mode 100644 index 00000000..ce9d38a1 --- /dev/null +++ b/packages/session-broker-core/src/limits.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, test } from "bun:test"; +import { PayloadTooLargeError, readRequestTextWithLimit, utf8ByteLength } from "./limits"; + +/** Build a streaming request body so the read path runs without a Content-Length header. */ +function streamingRequest(byteLength: number, chunkSize = 64 * 1024) { + const stream = new ReadableStream({ + pull(controller) { + const remaining = byteLength - sent; + if (remaining <= 0) { + controller.close(); + return; + } + + const size = Math.min(chunkSize, remaining); + controller.enqueue(new Uint8Array(size).fill(120)); + sent += size; + }, + }); + let sent = 0; + + return new Request("http://broker.test/api", { + method: "POST", + body: stream, + // Bun requires half-duplex opt-in for streamed request bodies. + duplex: "half", + } as RequestInit); +} + +describe("readRequestTextWithLimit", () => { + test("rejects an oversized declared Content-Length before reading the body", async () => { + const request = new Request("http://broker.test/api", { + method: "POST", + headers: { "content-type": "application/json", "content-length": String(10 * 1024 * 1024) }, + body: "ignored", + }); + + await expect(readRequestTextWithLimit(request, 1024)).rejects.toBeInstanceOf( + PayloadTooLargeError, + ); + }); + + test("aborts the stream when a missing Content-Length hides an oversized body", async () => { + const request = streamingRequest(2 * 1024 * 1024); + + await expect(readRequestTextWithLimit(request, 256 * 1024)).rejects.toBeInstanceOf( + PayloadTooLargeError, + ); + }); + + test("returns the decoded body when it stays under the limit", async () => { + const request = new Request("http://broker.test/api", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ action: "list" }), + }); + + await expect(readRequestTextWithLimit(request, 1024 * 1024)).resolves.toBe( + JSON.stringify({ action: "list" }), + ); + }); + + test("treats a missing body as an empty string", async () => { + const request = new Request("http://broker.test/api", { method: "GET" }); + + await expect(readRequestTextWithLimit(request, 1024)).resolves.toBe(""); + }); +}); + +describe("utf8ByteLength", () => { + test("counts multi-byte characters by their encoded size", () => { + expect(utf8ByteLength("abc")).toBe(3); + expect(utf8ByteLength("é")).toBe(2); + expect(utf8ByteLength("😀")).toBe(4); + }); +}); diff --git a/packages/session-broker-core/src/limits.ts b/packages/session-broker-core/src/limits.ts new file mode 100644 index 00000000..47781063 --- /dev/null +++ b/packages/session-broker-core/src/limits.ts @@ -0,0 +1,124 @@ +/** + * Hard size ceilings for everything the session broker parses or stores from the network. + * + * The broker is loopback-only by default, but a hostile or buggy local process (and any remote + * peer when HUNK_MCP_UNSAFE_ALLOW_REMOTE=1) can otherwise stream unbounded HTTP bodies or + * websocket frames, or register a changeset with an unbounded number of files, hunks, comments, + * or patch bytes. These caps keep memory bounded while staying far above any realistic review. + */ + +/** Maximum decoded byte length accepted for one HTTP API request body. */ +export const MAX_HTTP_BODY_BYTES = 4 * 1024 * 1024; + +/** Maximum byte length accepted for one inbound websocket message. */ +export const MAX_WS_MESSAGE_BYTES = 8 * 1024 * 1024; + +/** Maximum number of files accepted in one session registration payload. */ +export const MAX_REGISTRATION_FILES = 5_000; + +/** Maximum number of hunks accepted per registered file. */ +export const MAX_REGISTRATION_HUNKS_PER_FILE = 10_000; + +/** Maximum byte length accepted for one registered file's patch text. */ +export const MAX_REGISTRATION_PATCH_BYTES = 2 * 1024 * 1024; + +/** Maximum number of live comments accepted in one session snapshot. */ +export const MAX_SNAPSHOT_LIVE_COMMENTS = 10_000; + +/** Maximum number of review notes accepted in one session snapshot. */ +export const MAX_SNAPSHOT_REVIEW_NOTES = 10_000; + +/** Raised when an inbound payload exceeds its configured byte budget. */ +export class PayloadTooLargeError extends Error { + constructor(readonly limitBytes: number) { + super(`Payload exceeds the ${limitBytes}-byte session broker limit.`); + this.name = "PayloadTooLargeError"; + } +} + +// Reused across every websocket message, HTTP body, and patch check to avoid a per-call alloc. +const sharedTextEncoder = new TextEncoder(); + +/** UTF-8 byte length of a string without allocating a Buffer in non-Node runtimes. */ +export function utf8ByteLength(value: string): number { + return sharedTextEncoder.encode(value).length; +} + +/** + * Read one request body as text while enforcing a hard byte ceiling. + * + * The Content-Length header is rejected early when it already declares an oversized body, and the + * stream is aborted mid-read so a missing or lying Content-Length cannot force the daemon to + * buffer an unbounded body before the cap is noticed. + */ +export async function readRequestTextWithLimit( + request: Request, + maxBytes: number, +): Promise { + const declared = request.headers.get("content-length"); + if (declared) { + const length = Number.parseInt(declared, 10); + if (Number.isInteger(length) && length > maxBytes) { + throw new PayloadTooLargeError(maxBytes); + } + } + + const body = request.body; + if (!body) { + // Some runtimes do not expose a streaming body; the Content-Length guard above still bounds + // well-behaved clients, and the post-read check bounds the rest. + const text = await request.text(); + if (utf8ByteLength(text) > maxBytes) { + throw new PayloadTooLargeError(maxBytes); + } + + return text; + } + + const reader = body.getReader(); + const chunks: Uint8Array[] = []; + let total = 0; + for (;;) { + let done: boolean; + let value: Uint8Array | undefined; + try { + const result = await reader.read(); + done = result.done; + value = result.value; + } catch (error) { + reader.releaseLock(); + throw error; + } + + if (done) { + break; + } + + if (!value) { + continue; + } + + total += value.byteLength; + if (total > maxBytes) { + // Stop pulling from the stream immediately so the body cannot grow past the cap. + await reader.cancel().catch(() => {}); + // cancel() does not release the lock per the Streams spec; release it explicitly so the + // over-limit path matches the normal-exit path instead of waiting for GC. + reader.releaseLock(); + throw new PayloadTooLargeError(maxBytes); + } + + chunks.push(value); + } + + reader.releaseLock(); + + const merged = new Uint8Array(total); + let offset = 0; + for (const chunk of chunks) { + merged.set(chunk, offset); + offset += chunk.byteLength; + } + + return new TextDecoder().decode(merged); +} diff --git a/packages/session-broker/src/daemon.test.ts b/packages/session-broker/src/daemon.test.ts index 152b84cb..bbee4e8c 100644 --- a/packages/session-broker/src/daemon.test.ts +++ b/packages/session-broker/src/daemon.test.ts @@ -199,6 +199,29 @@ describe("session broker daemon", () => { daemon.shutdown(); }); + test("rejects raw broker API bodies that exceed the size limit", async () => { + const daemon = createSessionBrokerDaemon({ + broker: createBroker(), + capabilities: { version: 1 }, + exposeHttpApi: true, + }); + + const oversized = JSON.stringify({ action: "list", filler: "x".repeat(5 * 1024 * 1024) }); + const response = await daemon.handleRequest( + new Request("http://broker.test/broker", { + method: "POST", + headers: { "content-type": "application/json" }, + body: oversized, + }), + ); + + expect(response?.status).toBe(413); + await expect(response?.json()).resolves.toMatchObject({ + error: expect.stringContaining("session broker limit"), + }); + daemon.shutdown(); + }); + test("dispatches one raw command through the broker API", async () => { const daemon = createSessionBrokerDaemon({ broker: createBroker(), diff --git a/packages/session-broker/src/daemon.ts b/packages/session-broker/src/daemon.ts index 1dc2d3c5..3e1e38d1 100644 --- a/packages/session-broker/src/daemon.ts +++ b/packages/session-broker/src/daemon.ts @@ -1,4 +1,10 @@ -import type { SessionServerMessage, SessionTargetSelector } from "@hunk/session-broker-core"; +import { + MAX_HTTP_BODY_BYTES, + PayloadTooLargeError, + readRequestTextWithLimit, + type SessionServerMessage, + type SessionTargetSelector, +} from "@hunk/session-broker-core"; import type { SessionBrokerController, SessionBrokerPeer } from "./broker"; import { DEFAULT_SESSION_BROKER_API_PATH, @@ -64,8 +70,9 @@ function hasJsonContentType(request: Request) { async function parseJsonRequest( request: Request, ) { + const text = await readRequestTextWithLimit(request, MAX_HTTP_BODY_BYTES); try { - return (await request.json()) as SessionBrokerDaemonRequest; + return JSON.parse(text) as SessionBrokerDaemonRequest; } catch { throw new Error("Expected one JSON request body."); } @@ -369,6 +376,10 @@ export class SessionBrokerDaemon< return Response.json(response); } catch (error) { + if (error instanceof PayloadTooLargeError) { + return jsonError(error.message, 413); + } + return jsonError(error instanceof Error ? error.message : "Unknown broker API error."); } } diff --git a/src/hunk-session/wire.test.ts b/src/hunk-session/wire.test.ts index 13418a97..489843ae 100644 --- a/src/hunk-session/wire.test.ts +++ b/src/hunk-session/wire.test.ts @@ -1,7 +1,36 @@ import { describe, expect, test } from "bun:test"; -import { SESSION_BROKER_REGISTRATION_VERSION } from "@hunk/session-broker-core"; +import { + MAX_REGISTRATION_FILES, + MAX_REGISTRATION_HUNKS_PER_FILE, + MAX_REGISTRATION_PATCH_BYTES, + MAX_SNAPSHOT_LIVE_COMMENTS, + MAX_SNAPSHOT_REVIEW_NOTES, + SESSION_BROKER_REGISTRATION_VERSION, +} from "@hunk/session-broker-core"; import { parseSessionRegistration, parseSessionSnapshot } from "./wire"; +function createRegistration(files: unknown[]) { + return { + registrationVersion: SESSION_BROKER_REGISTRATION_VERSION, + sessionId: "session-1", + pid: 123, + cwd: "/repo", + launchedAt: "2026-03-22T00:00:00.000Z", + info: { inputKind: "vcs", title: "repo working tree", sourceLabel: "/repo", files }, + }; +} + +function createFile(overrides: Record = {}) { + return { + id: "file-1", + path: "src/example.ts", + additions: 1, + deletions: 0, + hunks: [{ index: 0, header: "@@ -1 +1 @@" }], + ...overrides, + }; +} + function createValidComment(overrides: Record = {}) { return { commentId: "comment-1", @@ -62,4 +91,57 @@ describe("hunk session wire parsing", () => { files: [], }); }); + + test("rejects registrations with more files than the cap", () => { + const files = Array.from({ length: MAX_REGISTRATION_FILES + 1 }, (_, index) => + createFile({ id: `file-${index}`, path: `src/file-${index}.ts` }), + ); + + expect(parseSessionRegistration(createRegistration(files))).toBeNull(); + }); + + test("rejects files with more hunks than the per-file cap", () => { + const hunks = Array.from({ length: MAX_REGISTRATION_HUNKS_PER_FILE + 1 }, (_, index) => ({ + index, + header: `@@ hunk ${index} @@`, + })); + + expect(parseSessionRegistration(createRegistration([createFile({ hunks })]))).toBeNull(); + }); + + test("rejects files whose patch exceeds the byte cap", () => { + const patch = "x".repeat(MAX_REGISTRATION_PATCH_BYTES + 1); + + expect(parseSessionRegistration(createRegistration([createFile({ patch })]))).toBeNull(); + }); + + test("rejects snapshots with more live comments than the cap", () => { + const liveComments = Array.from({ length: MAX_SNAPSHOT_LIVE_COMMENTS + 1 }, (_, index) => + createValidComment({ commentId: `comment-${index}` }), + ); + + const snapshot = parseSessionSnapshot({ + updatedAt: "2026-03-22T00:00:00.000Z", + state: { selectedHunkIndex: 0, showAgentNotes: true, liveComments }, + }); + + expect(snapshot).toBeNull(); + }); + + test("rejects snapshots with more review notes than the cap", () => { + const reviewNotes = Array.from({ length: MAX_SNAPSHOT_REVIEW_NOTES + 1 }, (_, index) => ({ + noteId: `note-${index}`, + source: "user", + filePath: "src/example.ts", + body: "Looks good", + createdAt: "2026-03-22T00:00:00.000Z", + })); + + const snapshot = parseSessionSnapshot({ + updatedAt: "2026-03-22T00:00:00.000Z", + state: { selectedHunkIndex: 0, showAgentNotes: true, liveComments: [], reviewNotes }, + }); + + expect(snapshot).toBeNull(); + }); }); diff --git a/src/hunk-session/wire.ts b/src/hunk-session/wire.ts index e25f20d8..c03c92fd 100644 --- a/src/hunk-session/wire.ts +++ b/src/hunk-session/wire.ts @@ -1,8 +1,14 @@ import type { CliInput } from "../core/types"; import { + MAX_REGISTRATION_FILES, + MAX_REGISTRATION_HUNKS_PER_FILE, + MAX_REGISTRATION_PATCH_BYTES, + MAX_SNAPSHOT_LIVE_COMMENTS, + MAX_SNAPSHOT_REVIEW_NOTES, brokerWireParsers, parseSessionRegistrationEnvelope, parseSessionSnapshotEnvelope, + utf8ByteLength, } from "@hunk/session-broker-core"; import type { HunkSessionRegistration, HunkSessionSnapshot } from "./types"; import type { @@ -70,7 +76,7 @@ function parseSessionReviewFile(value: unknown): SessionReviewFile | null { return null; } - if (!Array.isArray(record.hunks)) { + if (!Array.isArray(record.hunks) || record.hunks.length > MAX_REGISTRATION_HUNKS_PER_FILE) { return null; } @@ -79,6 +85,13 @@ function parseSessionReviewFile(value: unknown): SessionReviewFile | null { return null; } + // Reject files whose patch text alone would blow the per-file memory budget instead of + // silently dropping it, so an oversized registration fails loudly rather than half-loading. + const patch = brokerWireParsers.parseOptionalString(record.patch); + if (patch !== undefined && utf8ByteLength(patch) > MAX_REGISTRATION_PATCH_BYTES) { + return null; + } + return { id, path, @@ -86,7 +99,7 @@ function parseSessionReviewFile(value: unknown): SessionReviewFile | null { additions, deletions, hunkCount: (hunks as SessionReviewHunk[]).length, - patch: brokerWireParsers.parseOptionalString(record.patch), + patch, hunks: hunks as SessionReviewHunk[], }; } @@ -183,7 +196,7 @@ function parseSessionReviewNoteSummary(value: unknown): SessionReviewNoteSummary /** Parse the app-owned registration info embedded inside one broker registration envelope. */ function parseHunkSessionInfo(value: unknown): HunkSessionInfo | null { const record = brokerWireParsers.asRecord(value); - if (!record || !Array.isArray(record.files)) { + if (!record || !Array.isArray(record.files) || record.files.length > MAX_REGISTRATION_FILES) { return null; } @@ -210,7 +223,12 @@ function parseHunkSessionInfo(value: unknown): HunkSessionInfo | null { /** Parse the app-owned snapshot state embedded inside one broker snapshot envelope. */ function parseHunkSessionState(value: unknown): HunkSessionState | null { const record = brokerWireParsers.asRecord(value); - if (!record || !Array.isArray(record.liveComments)) { + if ( + !record || + !Array.isArray(record.liveComments) || + record.liveComments.length > MAX_SNAPSHOT_LIVE_COMMENTS || + (Array.isArray(record.reviewNotes) && record.reviewNotes.length > MAX_SNAPSHOT_REVIEW_NOTES) + ) { return null; } diff --git a/src/session-broker/brokerServer.test.ts b/src/session-broker/brokerServer.test.ts index 54e1047b..42ddf044 100644 --- a/src/session-broker/brokerServer.test.ts +++ b/src/session-broker/brokerServer.test.ts @@ -399,6 +399,29 @@ describe("Hunk session daemon server", () => { } }); + test("rejects session API bodies that exceed the size limit", async () => { + const port = await reserveLoopbackPort(); + process.env.HUNK_MCP_HOST = "127.0.0.1"; + process.env.HUNK_MCP_PORT = String(port); + + const server = serveSessionBrokerDaemon(); + + try { + const response = await fetch(`http://127.0.0.1:${port}/session-api`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ action: "list", filler: "x".repeat(5 * 1024 * 1024) }), + }); + + expect(response.status).toBe(413); + await expect(response.json()).resolves.toMatchObject({ + error: expect.stringContaining("session broker limit"), + }); + } finally { + server.stop(true); + } + }); + test("closes snapshots for missing sessions with a specific not-registered reason", async () => { // Bun's Windows WebSocket client does not reliably surface this immediate server close. // The daemon-core test covers the close code/reason without the flaky transport layer. diff --git a/src/session-broker/brokerServer.ts b/src/session-broker/brokerServer.ts index 4093038e..5103a234 100644 --- a/src/session-broker/brokerServer.ts +++ b/src/session-broker/brokerServer.ts @@ -24,6 +24,11 @@ import type { ReloadedSessionResult, RemovedCommentResult, } from "../hunk-session/types"; +import { + MAX_HTTP_BODY_BYTES, + PayloadTooLargeError, + readRequestTextWithLimit, +} from "@hunk/session-broker-core"; import { listHunkSessionNotes } from "../hunk-session/projections"; import { HUNK_SESSION_API_PATH, @@ -195,8 +200,9 @@ function validateOriginHeader(request: Request, expectedPort: number, allowRemot } async function parseJsonRequest(request: Request) { + const text = await readRequestTextWithLimit(request, MAX_HTTP_BODY_BYTES); try { - return (await request.json()) as SessionDaemonRequest; + return JSON.parse(text) as SessionDaemonRequest; } catch { throw new Error("Expected one JSON request body."); } @@ -362,6 +368,10 @@ async function handleSessionApiRequest(state: HunkSessionBrokerState, request: R return Response.json(response); } catch (error) { + if (error instanceof PayloadTooLargeError) { + return jsonError(error.message, 413); + } + return jsonError(error instanceof Error ? error.message : "Unknown session API error."); } }