-
Notifications
You must be signed in to change notification settings - Fork 98
fix(session): cap daemon HTTP, websocket, and registration payload sizes #333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| export * from "./types"; | ||
| export * from "./brokerWire"; | ||
| export * from "./limits"; | ||
| export * from "./brokerState"; | ||
| export * from "./selectors"; | ||
| export * from "./sessionTerminalMetadata"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<Uint8Array>({ | ||
| 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); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -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<string> { | ||||||||||||||||||||||||||||
| 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); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+101
to
+109
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/session-broker-core/src/limits.ts
Line: 98-103
Comment:
The reader lock is not released before throwing on the over-limit path. `reader.cancel()` cancels the underlying source but does not release the lock per the WHATWG Streams spec — only `releaseLock()` does that. In practice the lock is freed when the reader is GC'd (since the request object goes out of scope after the 413 response), but the lock should be released explicitly to match the normal-exit path and avoid holding it for the GC cycle.
```suggestion
total += value.byteLength;
if (total > maxBytes) {
// Stop pulling from the stream immediately so the body cannot grow past the cap.
await reader.cancel().catch(() => {});
reader.releaseLock();
throw new PayloadTooLargeError(maxBytes);
}
```
How can I resolve this? If you propose a fix, please make it concise.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — added an explicit Responded by Claude Code using claude-opus-4-7. |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| 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); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utf8ByteLengthallocates a freshTextEncoderon every invocation. It is called per WebSocket message, per HTTP body, and per file patch. Hoisting a single module-level encoder avoids the per-call allocation entirely.Prompt To Fix With AI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done — hoisted a single module-level
TextEncoder(sharedTextEncoder) and reuse it inutf8ByteLength.Responded by Claude Code using claude-opus-4-7.