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) =>