From 762c57140811833fe1ab089ffa8f903447ce076c Mon Sep 17 00:00:00 2001 From: JF Date: Fri, 21 Aug 2026 23:42:55 -0400 Subject: [PATCH] hardening(dap): bound DapFrameDecoder frames and make accumulation linear MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The decoder accepted any finite positive Content-Length (a hostile or buggy peer could OOM the proxy worker / mirror connection), buffered header-less garbage without bound, and Buffer.concat'ed per chunk — O(N^2) for large frames arriving in small pieces (issue #402). - maxContentLength option, default 64 MB (DAP_MAX_FRAME_BYTES override); over-cap advertisements report a new 'overflow' error context and discard the buffer, same recovery contract as 'header' - header-search accumulation bounded at 16 KB - body bytes accumulate as a chunk list concatenated once per frame - MinimalDapClient warns and continues on overflow; DapMirrorServer closes the mirror connection (as it does for corrupt headers) Co-Authored-By: Claude Fable 5 --- docs/development/setup-guide.md | 1 + src/proxy/dap-framing.ts | 117 ++++++++++++++---- src/proxy/dap-mirror-server.ts | 6 +- src/proxy/minimal-dap.ts | 5 +- tests/unit/proxy/dap-framing.property.test.ts | 62 ++++++++++ tests/unit/proxy/minimal-dap.test.ts | 12 +- 6 files changed, 168 insertions(+), 35 deletions(-) diff --git a/docs/development/setup-guide.md b/docs/development/setup-guide.md index 42bc1b89..8b173998 100644 --- a/docs/development/setup-guide.md +++ b/docs/development/setup-guide.md @@ -315,6 +315,7 @@ TEST_TIMEOUT=30000 | `DAP_TRACE` | Set to `1` to trace every DAP frame to a per-session `dap-trace-.ndjson` (capped at 50 MB) | Not set | | `DAP_TRACE_FILE` | Explicit DAP trace file path (implies tracing on) | Not set | | `MCP_SKIP_ORPHAN_REAPERS` | Set to `1` to skip the startup orphan-process scans (e.g. PID-namespaced containers where orphans are impossible) | Not set | +| `DAP_MAX_FRAME_BYTES` | Upper bound for a single DAP frame body accepted by the frame decoder | 64 MB | ## Troubleshooting Setup Issues diff --git a/src/proxy/dap-framing.ts b/src/proxy/dap-framing.ts index 825c5581..2aaea849 100644 --- a/src/proxy/dap-framing.ts +++ b/src/proxy/dap-framing.ts @@ -20,62 +20,117 @@ export function encodeDapMessage(message: DebugProtocol.ProtocolMessage): Buffer return Buffer.from(`Content-Length: ${Buffer.byteLength(json, 'utf8')}${TWO_CRLF}${json}`, 'utf8'); } -export type DapFrameDecoderErrorContext = 'header' | 'json'; +export type DapFrameDecoderErrorContext = 'header' | 'json' | 'overflow'; + +/** + * Default frame-body cap (issue #402). Generous — real DAP messages are KBs; + * the largest legitimate payloads (huge variable dumps, base64 blobs) stay + * far under this. Matches the repo's 64MB maxBuffer precedent. Override per + * decoder via options.maxContentLength or globally via DAP_MAX_FRAME_BYTES. + */ +const DEFAULT_MAX_CONTENT_LENGTH = 64 * 1024 * 1024; + +/** Headers are a handful of short lines; anything past this with no separator is garbage. */ +const MAX_HEADER_BYTES = 16 * 1024; export interface DapFrameDecoderOptions { /** * Invoked on malformed input. 'header' means an invalid/absent * Content-Length header was seen and the buffered payload was discarded; - * 'json' means a complete frame failed to parse and was skipped. + * 'json' means a complete frame failed to parse and was skipped; + * 'overflow' means the peer advertised a frame above maxContentLength (or + * streamed header bytes past the header allowance) and the buffer was + * discarded — same recovery contract as 'header'. */ onError?: (error: Error, context: DapFrameDecoderErrorContext) => void; + /** Upper bound for a single frame body (issue #402); default 64 MB or DAP_MAX_FRAME_BYTES. */ + maxContentLength?: number; +} + +function defaultMaxContentLength(): number { + const env = Number(process.env.DAP_MAX_FRAME_BYTES); + return Number.isFinite(env) && env > 0 ? env : DEFAULT_MAX_CONTENT_LENGTH; } /** * Incremental Content-Length frame decoder. Feed it raw socket chunks via * push(); it returns every complete protocol message contained so far and * buffers any trailing partial frame for the next call. + * + * Accumulation is linear (issue #402): header bytes live in a small bounded + * buffer, body bytes in a chunk list concatenated once per completed frame — + * never a per-chunk Buffer.concat of the whole backlog. */ export class DapFrameDecoder { - private rawData = Buffer.alloc(0); + /** Bytes in the header-search phase; bounded by MAX_HEADER_BYTES + one chunk. */ + private headerData: Buffer = Buffer.alloc(0); + /** Body bytes of the frame in progress, concatenated once when complete. */ + private bodyChunks: Buffer[] = []; + private bodyBytes = 0; private contentLength = -1; private readonly onError?: DapFrameDecoderOptions['onError']; + private readonly maxContentLength: number; constructor(options?: DapFrameDecoderOptions) { this.onError = options?.onError; + this.maxContentLength = options?.maxContentLength ?? defaultMaxContentLength(); } push(data: Buffer): DebugProtocol.ProtocolMessage[] { - this.rawData = Buffer.concat([this.rawData, data]); const messages: DebugProtocol.ProtocolMessage[] = []; + let input: Buffer | null = data; while (true) { if (this.contentLength >= 0) { - // We have a content length, check if we have the full message - if (this.rawData.length >= this.contentLength) { - const message = this.rawData.toString('utf8', 0, this.contentLength); - this.rawData = this.rawData.slice(this.contentLength); - this.contentLength = -1; - - if (message.length > 0) { - try { - messages.push(JSON.parse(message) as DebugProtocol.ProtocolMessage); - } catch (e) { - this.onError?.(e instanceof Error ? e : new Error(String(e)), 'json'); - } + // Body phase: collect chunks until the advertised length is buffered + if (input && input.length > 0) { + this.bodyChunks.push(input); + this.bodyBytes += input.length; + } + input = null; + if (this.bodyBytes < this.contentLength) { + break; + } + const full = this.bodyChunks.length === 1 + ? this.bodyChunks[0] + : Buffer.concat(this.bodyChunks, this.bodyBytes); + const message = full.toString('utf8', 0, this.contentLength); + // Bytes past the frame belong to the next header + input = full.subarray(this.contentLength); + this.bodyChunks = []; + this.bodyBytes = 0; + this.contentLength = -1; + + if (message.length > 0) { + try { + messages.push(JSON.parse(message) as DebugProtocol.ProtocolMessage); + } catch (e) { + this.onError?.(e instanceof Error ? e : new Error(String(e)), 'json'); } - continue; } + continue; + } + + // Header phase + if (input && input.length > 0) { + this.headerData = this.headerData.length === 0 ? input : Buffer.concat([this.headerData, input]); } + input = null; - // Look for the header - const idx = this.rawData.indexOf(TWO_CRLF); + const idx = this.headerData.indexOf(TWO_CRLF); if (idx === -1) { + if (this.headerData.length > MAX_HEADER_BYTES) { + this.onError?.( + new Error(`No header separator within ${MAX_HEADER_BYTES} bytes; discarding payload`), + 'overflow' + ); + this.reset(); + } // No complete header yet break; } - const header = this.rawData.toString('utf8', 0, idx); + const header = this.headerData.toString('utf8', 0, idx); const lines = header.split('\r\n'); let parsedLength: number | null = null; @@ -90,27 +145,39 @@ export class DapFrameDecoder { } } - // Remove header from buffer - this.rawData = this.rawData.slice(idx + TWO_CRLF.length); + // Remove header from buffer; the remainder starts the body (or next header) + const remainder = this.headerData.subarray(idx + TWO_CRLF.length); + this.headerData = Buffer.alloc(0); if (parsedLength === null || parsedLength <= 0 || !Number.isFinite(parsedLength)) { this.onError?.( new Error('Invalid Content-Length header encountered; discarding payload'), 'header' ); - this.contentLength = -1; - this.rawData = Buffer.alloc(0); + this.reset(); + continue; + } + + if (parsedLength > this.maxContentLength) { + this.onError?.( + new Error(`Content-Length ${parsedLength} exceeds cap ${this.maxContentLength}; discarding payload`), + 'overflow' + ); + this.reset(); continue; } this.contentLength = parsedLength; + input = remainder; } return messages; } reset(): void { - this.rawData = Buffer.alloc(0); + this.headerData = Buffer.alloc(0); + this.bodyChunks = []; + this.bodyBytes = 0; this.contentLength = -1; } } diff --git a/src/proxy/dap-mirror-server.ts b/src/proxy/dap-mirror-server.ts index 74f1edde..b9194a91 100644 --- a/src/proxy/dap-mirror-server.ts +++ b/src/proxy/dap-mirror-server.ts @@ -186,8 +186,10 @@ export class MirrorClientConnection { this.decoder = new DapFrameDecoder({ onError: (error, context) => { this.logger.warn(`[DapMirror] Malformed frame from mirror client (${context}): ${error.message}`); - if (context === 'header') { - // Framing is unrecoverable once the byte stream is corrupt. + if (context === 'header' || context === 'overflow') { + // Framing is unrecoverable once the byte stream is corrupt, and a + // client advertising an over-cap frame is equally untrustworthy + // (issue #402). this.close(); } } diff --git a/src/proxy/minimal-dap.ts b/src/proxy/minimal-dap.ts index 34ad1b2b..2d869cac 100644 --- a/src/proxy/minimal-dap.ts +++ b/src/proxy/minimal-dap.ts @@ -40,8 +40,9 @@ export class MinimalDapClient extends EventEmitter { private socket: Socket | null = null; private decoder = new DapFrameDecoder({ onError: (error, context) => { - if (context === 'header') { - logger.warn('[MinimalDapClient] Invalid Content-Length header encountered; discarding payload'); + if (context === 'header' || context === 'overflow') { + // Same recovery contract: the decoder discarded the buffer (issue #402) + logger.warn(`[MinimalDapClient] ${error.message}`); } else { logger.error('[MinimalDapClient] Error parsing message:', error); } diff --git a/tests/unit/proxy/dap-framing.property.test.ts b/tests/unit/proxy/dap-framing.property.test.ts index 96289bb3..0eacc793 100644 --- a/tests/unit/proxy/dap-framing.property.test.ts +++ b/tests/unit/proxy/dap-framing.property.test.ts @@ -153,6 +153,68 @@ describe('DapFrameDecoder malformed-input recovery', () => { expect(errors).toEqual(['json']); }); + it('rejects a Content-Length above the cap with an overflow error and recovers (issue #402)', () => { + const errors: string[] = []; + const decoder = new DapFrameDecoder({ + onError: (_err, context) => errors.push(context), + maxContentLength: 1024 + }); + + // A hostile/buggy peer advertises a frame the decoder must never buffer + const evil = Buffer.from('Content-Length: 999999999\r\n\r\npartial body...', 'utf8'); + expect(decoder.push(evil)).toEqual([]); + expect(errors).toEqual(['overflow']); + + // The overflow discarded the buffer; a fresh valid frame decodes normally + const msg = { seq: 4, type: 'event', event: 'output', body: { text: 'ok' } }; + expect(decoder.push(frame(msg))).toEqual([msg]); + }); + + it('accepts a frame exactly at the cap boundary (issue #402)', () => { + const bodyText = '{"type":"event"}'; + const decoder = new DapFrameDecoder({ + maxContentLength: Buffer.byteLength(bodyText, 'utf8') + }); + + const exact = Buffer.from( + `Content-Length: ${Buffer.byteLength(bodyText, 'utf8')}\r\n\r\n${bodyText}`, + 'utf8' + ); + expect(decoder.push(exact)).toEqual([{ type: 'event' }]); + }); + + it('bounds header-search accumulation when no header separator ever arrives (issue #402)', () => { + const errors: string[] = []; + const decoder = new DapFrameDecoder({ + onError: (_err, context) => errors.push(context) + }); + + // A garbage stream with no \r\n\r\n used to buffer without bound + const garbage = Buffer.alloc(20 * 1024, 0x78); // 20 KB of 'x' + expect(decoder.push(garbage)).toEqual([]); + expect(errors).toEqual(['overflow']); + + // Recovery after the discard + const msg = { seq: 5, type: 'event', event: 'output', body: {} }; + expect(decoder.push(frame(msg))).toEqual([msg]); + }); + + it('reassembles a large frame delivered in many small chunks (issue #402)', () => { + // The old implementation Buffer.concat'ed per chunk — O(N^2) on exactly + // this shape. This pins correctness; the linear accumulation is the fix. + const big = { seq: 6, type: 'event', event: 'output', body: { text: 'y'.repeat(256 * 1024) } }; + const encoded = frame(big); + const decoder = new DapFrameDecoder(); + + const received: unknown[] = []; + const CHUNK = 1024; + for (let offset = 0; offset < encoded.length; offset += CHUNK) { + received.push(...decoder.push(encoded.subarray(offset, offset + CHUNK))); + } + + expect(received).toEqual([big]); + }); + it('reset() drops any partial frame in progress', () => { const decoder = new DapFrameDecoder(); const msg = { seq: 3, type: 'event', event: 'continued', body: {} }; diff --git a/tests/unit/proxy/minimal-dap.test.ts b/tests/unit/proxy/minimal-dap.test.ts index 0576f869..641159a2 100644 --- a/tests/unit/proxy/minimal-dap.test.ts +++ b/tests/unit/proxy/minimal-dap.test.ts @@ -299,9 +299,9 @@ describe('MinimalDapClient', () => { '[MinimalDapClient] Invalid Content-Length header encountered; discarding payload' ); expect(protocolSpy).not.toHaveBeenCalled(); - expect( - (client as unknown as { decoder: { rawData: Buffer } }).decoder.rawData.length - ).toBe(0); + const decoder = (client as unknown as { decoder: { headerData: Buffer; bodyBytes: number } }).decoder; + expect(decoder.headerData.length).toBe(0); + expect(decoder.bodyBytes).toBe(0); protocolSpy.mockRestore(); }); @@ -325,9 +325,9 @@ describe('MinimalDapClient', () => { 2, '[MinimalDapClient] Invalid Content-Length header encountered; discarding payload' ); - expect( - (client as unknown as { decoder: { rawData: Buffer } }).decoder.rawData.length - ).toBe(0); + const decoder = (client as unknown as { decoder: { headerData: Buffer; bodyBytes: number } }).decoder; + expect(decoder.headerData.length).toBe(0); + expect(decoder.bodyBytes).toBe(0); }); it('should handle incomplete message body', async () => {