From c209715421f29846d0c66b3bedadfb9c4ce503ff Mon Sep 17 00:00:00 2001 From: Daniel McCoy Stephenson Date: Thu, 8 Oct 2026 03:17:21 -0600 Subject: [PATCH] fix: bound maxDocumentBytes in bytes while the body is read The limit was applied as `text.length` after `response.text()` had already buffered the whole body, so it bounded neither memory nor bytes. The body is now read from its stream, counted in bytes as chunks arrive, and the transfer is cancelled and aborted as soon as the count passes the limit. A response with no stream still falls back to `text()`, but is measured in UTF-8 bytes so the option means one thing either way. Closes #16 Co-Authored-By: Claude Opus 5.5 --- js/README.md | 2 +- js/src/client.ts | 41 ++++++++++++++++++++++-- js/test/client.test.ts | 72 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 111 insertions(+), 4 deletions(-) diff --git a/js/README.md b/js/README.md index 0442fed..b2de4c3 100644 --- a/js/README.md +++ b/js/README.md @@ -128,7 +128,7 @@ docs.resolveLink('rules.md', 'handbook/getting-started.md'); | `transport` | `'api'` with a token, `'raw'` without | Contents API vs `raw.githubusercontent.com`. | | `documents` | *(unset)* | The catalogue. A list, or a comma-separated string. **Unset means the whole repository is fetchable; an empty list means nothing is.** | | `timeoutMs` | `5000` | A page renders its panel rather than making a visitor wait on someone else's outage. | -| `maxDocumentBytes` | `1048576` | Anything larger is not the document that was asked for. | +| `maxDocumentBytes` | `1048576` | Anything larger is not the document that was asked for. Counted in bytes of the body as it arrives, and the read stops there rather than buffering the rest. | | `apiBase` | `https://api.github.com` | For GitHub Enterprise. | | `fetchImpl` | global `fetch` | Injectable, for tests. | diff --git a/js/src/client.ts b/js/src/client.ts index 74a4053..62aa17f 100644 --- a/js/src/client.ts +++ b/js/src/client.ts @@ -126,6 +126,43 @@ export interface DocsClient { resolveLink(href: string, fromPath?: string): string; } +// The body, read no further than `maxBytes`, or null when it is larger. Counted +// in bytes as they arrive rather than as `string.length` after the fact: the +// point of the limit is that an oversized body is never held in memory, and a +// UTF-16 code-unit count would let a multi-byte document past it several times +// over. +const readBoundedText = async ( + response: Response, + maxBytes: number, + controller: AbortController +): Promise => { + if (!response.body) { + // No stream to bound — an empty body, or a minimal stand-in that only + // offers `text()`. Still measured in bytes, so the limit means one thing. + const text = await response.text(); + return new TextEncoder().encode(text).byteLength > maxBytes ? null : text; + } + const reader = response.body.getReader(); + const decoder = new TextDecoder(); + let received = 0; + let text = ''; + for (;;) { + const {done, value} = await reader.read(); + if (done) { + return text + decoder.decode(); + } + received += value.byteLength; + if (received > maxBytes) { + // Stop the transfer rather than draining it: the rest of the body + // is exactly what the limit exists to avoid reading. + void reader.cancel().catch(() => undefined); + controller.abort(); + return null; + } + text += decoder.decode(value, {stream: true}); + } +}; + // One fetch, bounded by a timeout and a size limit, with every failure // collapsed to `unavailable`. const fetchMarkdown = async ( @@ -145,8 +182,8 @@ const fetchMarkdown = async ( // intermediary might — must not be able to launder it through here. return {status: 'unavailable'}; } - const text = await response.text(); - if (text.length > maxBytes) { + const text = await readBoundedText(response, maxBytes, controller); + if (text === null) { return {status: 'unavailable'}; } return {status: 'ok', markdown: text}; diff --git a/js/test/client.test.ts b/js/test/client.test.ts index 6b488f9..af945e1 100644 --- a/js/test/client.test.ts +++ b/js/test/client.test.ts @@ -88,7 +88,7 @@ describe('failure is a value, not an exception', () => { }); } - it('refuses a document larger than the limit rather than holding it in memory', async () => { + it('refuses a document larger than the limit', async () => { const fetchImpl = vi.fn().mockResolvedValue(okResponse('x'.repeat(101))); const client = createDocsClient({ repo: REPO, @@ -110,6 +110,76 @@ describe('failure is a value, not an exception', () => { await expect(client.fetchMarkdown('rules')).resolves.toEqual({status: 'ok', markdown: 'x'.repeat(100)}); }); + it('stops reading an oversized body at the limit instead of draining it', async () => { + // A thousand 64-byte chunks against a 100-byte limit. Reading the whole + // body and then measuring it would pull every chunk; the limit is only + // worth having if it stops the read a chunk or two past the line. + let pulled = 0; + const body = new ReadableStream({ + pull(controller) { + pulled += 1; + if (pulled > 1000) { + controller.close(); + return; + } + controller.enqueue(new Uint8Array(64).fill(0x78)); + } + }); + let signal: AbortSignal | undefined; + const fetchImpl = vi.fn(async (_url: string, init?: RequestInit) => { + signal = init?.signal ?? undefined; + return new Response(body, {status: 200}); + }); + const client = createDocsClient({ + repo: REPO, + documents: DOCS, + maxDocumentBytes: 100, + fetchImpl: fetchImpl as never + }); + + await expect(client.fetchMarkdown('rules')).resolves.toEqual({status: 'unavailable'}); + expect(pulled).toBeLessThan(10); + expect(signal?.aborted).toBe(true); + }); + + it('measures the limit in bytes, not in string length', async () => { + // 51 two-byte characters: 102 bytes of UTF-8, but a `length` of 51. + const markdown = 'é'.repeat(51); + for (const response of [() => okResponse(markdown), () => new Response(markdown, {status: 200})]) { + const fetchImpl = vi.fn().mockResolvedValue(response()); + const client = createDocsClient({ + repo: REPO, + documents: DOCS, + maxDocumentBytes: 100, + fetchImpl: fetchImpl as never + }); + await expect(client.fetchMarkdown('rules')).resolves.toEqual({status: 'unavailable'}); + } + }); + + it('decodes a character split across chunks when the body is within the limit', async () => { + // 'é' is 0xC3 0xA9; delivering the two bytes in separate chunks is what + // a real network read does to a multi-byte character sooner or later. + const chunks = [new Uint8Array([0x23, 0x20, 0xc3]), new Uint8Array([0xa9, 0x0a])]; + const body = new ReadableStream({ + start(controller) { + for (const chunk of chunks) { + controller.enqueue(chunk); + } + controller.close(); + } + }); + const fetchImpl = vi.fn().mockResolvedValue(new Response(body, {status: 200})); + const client = createDocsClient({ + repo: REPO, + documents: DOCS, + maxDocumentBytes: 5, + fetchImpl: fetchImpl as never + }); + + await expect(client.fetchMarkdown('rules')).resolves.toEqual({status: 'ok', markdown: '# é\n'}); + }); + it('gives up after the timeout by aborting the request', async () => { const fetchImpl = vi.fn( (_url: string, init?: RequestInit) =>