Repository navigation
fix(gaxios): enforce maxContentLength on headerless responses in Bun - #9773
danieljbruce wants to merge 3 commits into
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a stream-limiting mechanism in Gaxios to enforce maxContentLength on responses that lack a Content-Length header. It introduces a Transform stream limiter that tracks received bytes and destroys the stream if the limit is exceeded. Additionally, it fixes a bug where instance options for maxContentLength and maxRedirects were not correctly mapped. The review feedback highlights that the newly added tests might not actually exercise the stream-limiting logic for headerless responses because nock automatically includes a Content-Length header by default. It is recommended to explicitly set the transfer-encoding header to chunked in the test mocks to ensure the stream limiter is properly tested.
| 'arraybuffer', | ||
| 'blob', | ||
| ] as const) { | ||
| const scope = nock(url).get('/').reply(200, body); |
There was a problem hiding this comment.
By default, nock automatically calculates and includes a Content-Length header when replying with a body. This means the test might be passing due to the pre-existing Content-Length validation rather than actually exercising the new stream-limiting logic for headerless responses.
To ensure the response is truly headerless and the stream limiter is exercised, explicitly set the transfer-encoding header to chunked in the nock reply.
const scope = nock(url)
.get('/')
.reply(200, body, {'transfer-encoding': 'chunked'});| scope.done(); | ||
| } | ||
|
|
||
| const defaultScope = nock(url).get('/').reply(200, body); |
There was a problem hiding this comment.
Similar to the test cases above, nock will automatically include a Content-Length header here. To ensure this test case actually exercises the stream limiter for instance defaults on headerless responses, set the transfer-encoding header to chunked.
const defaultScope = nock(url)
.get('/')
.reply(200, body, {'transfer-encoding': 'chunked'});
There was a problem hiding this comment.
Code Review
This pull request implements enforcement of maxContentLength on responses without a Content-Length header by wrapping the response stream in a Transform stream that tracks chunk sizes and destroys the stream if the limit is exceeded. It also corrects the mapping of maxContentLength and maxRedirects from options to opts. The feedback suggests optimizing the readBuffer function by using a zero-copy Buffer view over Uint8Array chunks to avoid unnecessary memory allocations and copying overhead.
| for await (const chunk of getNodeStream()) { | ||
| chunks.push( | ||
| Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk as Uint8Array), | ||
| ); |
There was a problem hiding this comment.
In Bun, the fetch response stream yields Uint8Array chunks rather than Node.js Buffer instances. Using Buffer.from(chunk as Uint8Array) copies the underlying memory of each chunk into a new Buffer allocation.
To avoid this unnecessary memory allocation and copying overhead, you can use Buffer.from(chunk.buffer, chunk.byteOffset, chunk.byteLength) to create a zero-copy Buffer view over the same underlying ArrayBuffer.
for await (const chunk of getNodeStream()) {
chunks.push(
Buffer.isBuffer(chunk)
? chunk
: Buffer.from(chunk.buffer, chunk.byteOffset, chunk.byteLength),
);
}
| } from './common.js'; | ||
| import {getRetryConfig} from './retry.js'; | ||
| import {PassThrough, Readable, pipeline} from 'stream'; | ||
| import {PassThrough, Readable, Transform, pipeline} from 'stream'; |
There was a problem hiding this comment.
[Generated] Import Transform from stream so #createBunFetch() can wrap the response stream in a byte-counting transform when maxContentLength (size) is configured.
| if (typeof opts.maxContentLength === 'number') { | ||
| opts.size = opts.maxContentLength; |
There was a problem hiding this comment.
[Generated] Read opts.maxContentLength from the merged opts object instead of raw options so new Gaxios({maxContentLength}) instance defaults populate opts.size.
There was a problem hiding this comment.
I like this. For reference, opts just adds the defaults to options so I can't see any problem with this unless we would want to deliberately have opts.size be undefined for cases that it wasn't passed into the function.
| if (typeof opts.maxRedirects === 'number') { | ||
| opts.follow = opts.maxRedirects; |
There was a problem hiding this comment.
[Generated] Read opts.maxRedirects from the merged opts object instead of raw options so new Gaxios({maxRedirects}) instance defaults populate opts.follow.
| size?: number; | ||
| maxContentLength?: number; |
There was a problem hiding this comment.
[Generated] Declare size and maxContentLength on rawInit in #createBunFetch() so the Bun fetch wrapper can read the configured byte limit.
| } | ||
| throw err; | ||
| } | ||
| const maxSize = rawInit?.size ?? rawInit?.maxContentLength ?? 0; |
There was a problem hiding this comment.
[Generated] Compute maxSize from rawInit.size or rawInit.maxContentLength (defaulting to 0 when unset) to determine whether response stream byte-limiting is needed.
| const getNodeStream = () => { | ||
| // Lazily converts the Web ReadableStream response body into a Node.js | ||
| // Readable stream and wraps it in a byte-counting Transform when | ||
| // maxSize is set so both res.body and buffered body readers enforce | ||
| // maxContentLength. | ||
| if (!nodeStream) { | ||
| const webReadable = Readable.fromWeb(rawBody); | ||
| if (maxSize > 0) { | ||
| let totalBytes = 0; | ||
| const limiter = new Transform({ | ||
| transform(chunk: Buffer | Uint8Array, _encoding, callback) { | ||
| totalBytes += chunk.byteLength; | ||
| if (totalBytes > maxSize) { | ||
| webReadable.destroy(); | ||
| callback( | ||
| Object.assign( | ||
| new Error( | ||
| `content size at ${String((input as {url?: unknown})?.url ?? input)} over limit: ${maxSize}`, | ||
| ), | ||
| {type: 'max-size'}, | ||
| ), | ||
| ); | ||
| return; | ||
| } | ||
| callback(null, chunk); | ||
| }, | ||
| }); | ||
| webReadable.on('error', err => limiter.destroy(err)); | ||
| limiter.once('close', () => { | ||
| if (!webReadable.destroyed) webReadable.destroy(); | ||
| }); | ||
| nodeStream = webReadable.pipe(limiter); | ||
| } else { | ||
| nodeStream = webReadable; | ||
| } | ||
| } | ||
| return nodeStream; | ||
| }; |
There was a problem hiding this comment.
[Generated] Define getNodeStream() to lazily convert the Web ReadableStream into a Node.js Readable and pipe it through a byte-counting Transform when maxSize > 0, immediately destroying the underlying stream and emitting content size at <url> over limit: <maxSize> (type: 'max-size') if exceeded.
There was a problem hiding this comment.
Is this really necessary to solve the issue in question or can we stack this change on top?
There was a problem hiding this comment.
This is also out of date. We removed getNodeStream.
| const readBuffer = async () => { | ||
| const chunks: Buffer[] = []; | ||
| for await (const chunk of nodeStream!) { | ||
| for await (const chunk of getNodeStream()) { |
There was a problem hiding this comment.
[Generated] Consume getNodeStream() inside readBuffer() so buffered response readers (text(), json(), arrayBuffer(), blob()) pass through the maxSize stream limiter.
| get() { | ||
| nodeStream ||= Readable.fromWeb(rawBody); | ||
| return nodeStream; | ||
| return getNodeStream(); |
There was a problem hiding this comment.
[Generated] Return getNodeStream() from the res.body getter so responseType: 'stream' callers also receive the byte-limited Node.js Readable stream.
| }); | ||
| res.text = async () => { | ||
| if (!nodeStream) return origText(); | ||
| if (!nodeStream && maxSize <= 0) return origText(); |
There was a problem hiding this comment.
[Generated] Route res.text() through readBuffer() whenever maxSize > 0 so headerless/chunked text responses enforce maxContentLength instead of bypassing the stream limiter via origText().
| }; | ||
| res.json = async () => { | ||
| if (!nodeStream) return origJson(); | ||
| if (!nodeStream && maxSize <= 0) return origJson(); |
There was a problem hiding this comment.
[Generated] Route res.json() through res.text() / readBuffer() whenever maxSize > 0 so headerless/chunked JSON responses enforce maxContentLength.
| }; | ||
| res.arrayBuffer = async () => { | ||
| if (!nodeStream) return origArrayBuffer(); | ||
| if (!nodeStream && maxSize <= 0) return origArrayBuffer(); |
There was a problem hiding this comment.
[Generated] Route res.arrayBuffer() through readBuffer() whenever maxSize > 0 so headerless/chunked binary responses enforce maxContentLength.
| }; | ||
| res.blob = async () => { | ||
| if (!nodeStream) return origBlob(); | ||
| if (!nodeStream && maxSize <= 0) return origBlob(); |
There was a problem hiding this comment.
[Generated] Route res.blob() through readBuffer() whenever maxSize > 0 so headerless/chunked Blob responses enforce maxContentLength.
| it('should enforce maxContentLength on responses without a Content-Length header and from instance defaults', async () => { | ||
| const body = {hello: 'world'}; | ||
| for (const responseType of [ | ||
| 'json', | ||
| 'text', | ||
| 'arraybuffer', | ||
| 'blob', | ||
| ] as const) { | ||
| const scope = nock(url).get('/').reply(200, body); | ||
| await assert.rejects( | ||
| request({url, maxContentLength: 5, responseType}), | ||
| (err: Error) => err instanceof GaxiosError && /limit/.test(err.message), | ||
| ); | ||
| scope.done(); | ||
| } | ||
|
|
||
| const defaultScope = nock(url).get('/').reply(200, body); | ||
| const client = new Gaxios({maxContentLength: 5}); | ||
| await assert.rejects( | ||
| client.request({url}), | ||
| (err: Error) => err instanceof GaxiosError && /limit/.test(err.message), | ||
| ); | ||
| defaultScope.done(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[Generated] Add unit test verifying that maxContentLength rejects responses without a Content-Length header across json, text, arraybuffer, and blob response types, as well as when configured via new Gaxios({maxContentLength}) instance defaults.
| await assert.rejects( | ||
| request({url, maxContentLength: 2}), | ||
| (err: Error) => | ||
| err instanceof GaxiosError && /over limit: 2/.test(err.message), | ||
| ); | ||
|
|
||
| const streamMaxRes = await request<Readable>({ | ||
| url, | ||
| maxContentLength: 2, | ||
| responseType: 'stream', | ||
| }); | ||
| await assert.rejects( | ||
| (async () => { | ||
| for await (const chunk of streamMaxRes.data) { | ||
| assert.ok(chunk); | ||
| } | ||
| })(), | ||
| (err: Error) => /over limit: 2/.test(err.message), | ||
| ); | ||
|
|
There was a problem hiding this comment.
[Generated] Add unit assertions in the Bun fetch test verifying that maxContentLength rejects oversized responses with over limit: 2 for both buffered JSON and responseType: 'stream'.
678200a to
a2900c6
Compare
| const origArrayBuffer = res.arrayBuffer.bind(res); | ||
| const origBlob = res.blob.bind(res); | ||
| const readBuffer = async () => { | ||
| nodeStream ||= Readable.fromWeb(rawBody); |
There was a problem hiding this comment.
[Generated] Lazily initialize nodeStream from Readable.fromWeb(rawBody) inside readBuffer() so buffered readers (text(), json(), arrayBuffer(), blob()) can stream and measure the body even when res.body has not been accessed yet.
| let totalBytes = 0; | ||
| for await (const chunk of nodeStream) { | ||
| const buf = Buffer.isBuffer(chunk) | ||
| ? chunk | ||
| : Buffer.from(chunk as Uint8Array); | ||
| totalBytes += buf.byteLength; | ||
| if (maxSize > 0 && totalBytes > maxSize) { | ||
| throw Object.assign( | ||
| new Error( | ||
| `content size at ${String((input as {url?: unknown})?.url ?? input)} over limit: ${maxSize}`, | ||
| ), | ||
| {type: 'max-size'}, | ||
| ); | ||
| } | ||
| chunks.push(buf); |
There was a problem hiding this comment.
[Generated] Track totalBytes while reading chunks in readBuffer() and throw content size at <url> over limit: <maxSize> (type: 'max-size') as soon as totalBytes > maxSize.
There was a problem hiding this comment.
[Generated] Intent: Enforce maxContentLength (opts.size) on headerless/chunked buffered responses (json, text, arraybuffer, blob) in Bun so that both Node.js (V8) and Bun throw a max-size GaxiosError as soon as totalBytes > maxSize.
2x2 Matrix (totalBytes > maxContentLength without Content-Length header)
| Without this change | With this change | |
|---|---|---|
| Node.js (V8) | ❌ Throws error (GaxiosError: content size at <url> over limit: <maxSize>) |
❌ Throws error (GaxiosError: content size at <url> over limit: <maxSize>) |
| Bun | ✅ No error thrown (resolves 200 OK with the oversized payload) |
❌ Throws error (GaxiosError: content size at <url> over limit: <maxSize>) |
What happens if we don't throw the error for Bun?
- Silent limit bypass: When a response omits the
Content-Lengthheader (e.g.,Transfer-Encoding: chunked),Gaxios.getResponseData()'s upfront header check does not run. Because Bun's nativeglobalThis.fetchignores thenode-fetchsizeoption,for await (const chunk of nodeStream)inreadBuffer()continues reading every chunk to completion and resolves successfully with the oversizedjson/text/arraybuffer/blobdata instead of rejecting. - Unbounded memory buffering (OOM risk): All incoming chunks are accumulated in
chunks: Buffer[]and concatenated viaBuffer.concat(chunks)with no upper bound, allowing a large or runaway response stream to exhaust memory even when the caller explicitly configuredmaxContentLength. - Runtime divergence & test failures: Callers expecting
maxContentLengthto reject oversized responses get aGaxiosError(type: 'max-size') on Node.js (V8) but a resolved response on Bun, causing the headerlessmaxContentLengthtests incore/packages/gaxios/test/test.getch.tsto fail under Bun.
There was a problem hiding this comment.
[Generated] Good catch — in the list above, #1 was phrased as the mechanism (how the check gets bypassed when Content-Length is omitted), while #2 and #3 were the main consequences.
That said, even when an oversized response fits comfortably in memory (so #2 OOM doesn't happen) and setting aside #3 (Node vs. Bun divergence), resolving 200 OK with the oversized json / text / arraybuffer / blob payload instead of rejecting causes two concrete issues for callers:
- Unnecessary synchronous CPU / event-loop blocking (especially for
json): InGaxios.getResponseData(),case 'json'awaitsres.text()and then runsJSON.parse(data)synchronously. If a caller setsmaxContentLength: 10_000(10 KB) and a server streams a 50 MB chunked JSON response, resolvingreadBuffer()means the runtime UTF-8 decodes and synchronouslyJSON.parse()s all 50 MB on the main thread instead of aborting as soon as the 10 KB threshold is crossed. - Broken size invariant for downstream consumers: Callers use
maxContentLengthas a boundary guardrail sores.datais guaranteed to be<= maxContentLengthbefore passing it to downstream systems with strict payload caps (e.g., database columns, Redis/cache entries, Pub/Sub or Cloud Tasks messages) or when fetching from untrusted endpoints that useTransfer-Encoding: chunked. Resolving successfully hands oversized data to application code that assumed the size limit was enforced at the HTTP layer.
| it('should enforce maxContentLength on responses without a Content-Length header', async () => { | ||
| const body = {hello: 'world'}; | ||
| for (const responseType of [ | ||
| 'json', | ||
| 'text', | ||
| 'arraybuffer', | ||
| 'blob', | ||
| ] as const) { | ||
| const scope = nock(url).get('/').reply(200, body); | ||
| await assert.rejects( | ||
| request({url, maxContentLength: 5, responseType}), | ||
| (err: Error) => err instanceof GaxiosError && /limit/.test(err.message), | ||
| ); | ||
| scope.done(); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
[Generated] Add unit test verifying that maxContentLength rejects responses without a Content-Length header across json, text, arraybuffer, and blob response types.
| const origArrayBuffer = res.arrayBuffer.bind(res); | ||
| const origBlob = res.blob.bind(res); | ||
| const readBuffer = async () => { | ||
| nodeStream ||= Readable.fromWeb(rawBody); |
There was a problem hiding this comment.
Seems like a genuine bug. I notice that nodeStream wasn't even assigned before!
|
[Generated] Intent: Enforce 2x2 Matrix (
|
| Without this change | With this change | |
|---|---|---|
| Node.js (V8) | ❌ Throws error (GaxiosError: content size at <url> over limit: <maxSize>) |
❌ Throws error (GaxiosError: content size at <url> over limit: <maxSize>) |
| Bun | ✅ No error thrown (resolves 200 OK with the oversized payload) |
❌ Throws error (GaxiosError: content size at <url> over limit: <maxSize>) |
What happens if we don't throw the error for Bun?
- Silent limit bypass: When a response omits the
Content-Lengthheader (e.g.,Transfer-Encoding: chunked),Gaxios.getResponseData()'s upfront header check does not run. Because Bun's nativeglobalThis.fetchignores thenode-fetchsizeoption,for await (const chunk of nodeStream)inreadBuffer()continues reading every chunk to completion and resolves successfully with the oversizedjson/text/arraybuffer/blobdata instead of rejecting. - Unbounded memory buffering (OOM risk): All incoming chunks are accumulated in
chunks: Buffer[]and concatenated viaBuffer.concat(chunks)with no upper bound, allowing a large or runaway response stream to exhaust memory even when the caller explicitly configuredmaxContentLength. - Runtime divergence & test failures: Callers expecting
maxContentLengthto reject oversized responses get aGaxiosError(type: 'max-size') on Node.js (V8) but a resolved response on Bun, causing the headerlessmaxContentLengthtests incore/packages/gaxios/test/test.getch.tsto fail under Bun.
| const origArrayBuffer = res.arrayBuffer.bind(res); | ||
| const origBlob = res.blob.bind(res); | ||
| const readBuffer = async () => { | ||
| nodeStream ||= Readable.fromWeb(rawBody); |
There was a problem hiding this comment.
Re: your comment on why nodeStream wasn't assigned in readBuffer() previously: in main, nodeStream was only initialized inside the res.body getter because res.text(), res.json(), res.arrayBuffer(), and res.blob() all started with if (!nodeStream) return origText(). That meant readBuffer() was previously only reachable if the caller had already accessed res.body (which locked rawBody via Readable.fromWeb(rawBody)).
Now that those guards check if (!nodeStream && maxSize <= 0), readBuffer() runs whenever maxSize > 0 even if res.body was never touched, so lazily initializing nodeStream ||= Readable.fromWeb(rawBody) here is required.
There was a problem hiding this comment.
This is good to know. Thank you!
| const buf = Buffer.isBuffer(chunk) | ||
| ? chunk | ||
| : Buffer.from(chunk as Uint8Array); | ||
| totalBytes += buf.byteLength; | ||
| if (maxSize > 0 && totalBytes > maxSize) { | ||
| throw Object.assign( | ||
| new Error( | ||
| `content size at ${String((input as {url?: unknown})?.url ?? input)} over limit: ${maxSize}`, | ||
| ), | ||
| {type: 'max-size'}, | ||
| ); | ||
| } | ||
| chunks.push(buf); |
There was a problem hiding this comment.
Nit (non-blocking, and I see the byte-counting part of this loop gets replaced upstack in #9777 by getNodeStream()'s Transform limiter): Buffer.from(chunk as Uint8Array) allocates and copies the Uint8Array's memory before checking totalBytes > maxSize (and before Buffer.concat copies all chunks into the final buffer).
Checking chunk.byteLength before converting avoids copying an oversized chunk that is about to throw, and in both Node.js and Bun Buffer.concat(chunks) natively accepts ReadonlyArray<Uint8Array> (or you can wrap Uint8Array chunks with a zero-copy view Buffer.from(chunk.buffer, chunk.byteOffset, chunk.byteLength)) to avoid the extra copy per chunk.
There was a problem hiding this comment.
Sounds good. Probably worth addressing here since it seems to be a small change and improves the code immediately. We might only merge the rest of the stack if it is highest in priority.
| 'arraybuffer', | ||
| 'blob', | ||
| ] as const) { | ||
| const scope = nock(url).get('/').reply(200, body); |
There was a problem hiding this comment.
Note on the gemini-code-assist comment here: nock does not add a Content-Length header unless .replyContentLength() or an explicit 'content-length' header is specified (by default .reply(200, body) only sets {'content-type': 'application/json'}), so this test already exercises the headerless response path as written.
There was a problem hiding this comment.
[Generated] Thank you for pointing that out! To make it unambiguous in the test itself that the headerless stream byte-counting path is exercised rather than the upfront Content-Length header check ("Response's Content-Length is over the limit."), I tightened the assertion in eb5273f to match /over limit: 5/.test(err.message) and added a brief comment noting nock's default .reply(200, body) behavior.
022a425 to
eb5273f
Compare
Description
Enforces
maxContentLengthon headerless buffered responses under Bun.Impact
Prevents oversized buffered responses from bypassing
maxContentLengthlimits on Bun.Changes
maxSize(rawInit.size/rawInit.maxContentLength) insidereadBuffer()inGaxios.#createBunFetch()so buffered (json,text,arraybuffer,blob) responses reject withcontent size at <url> over limit: <maxSize>when exceeded even without aContent-Lengthheader.Stack
fix(gaxios): enforce maxContentLength on headerless responses in Bun(fix/gaxios-bun-max-content-length→main) (this PR)fix(gaxios): enforce maxContentLength on stream responses in Bun(fix/gaxios-bun-max-content-length-stream→fix/gaxios-bun-max-content-length)fix(gaxios): honor maxContentLength and maxRedirects instance defaults(fix/gaxios-instance-defaults→fix/gaxios-bun-max-content-length-stream)fix(gaxios): forward https.Agent TLS options and agent factories in Bun(fix/gaxios-bun-agent-tls→fix/gaxios-instance-defaults)Testing
core/packages/gaxios/test/test.getch.tscoveringmaxContentLengthrejection on headerless responses acrossjson,text,arraybuffer, andblobresponse types.cd core/packages/gaxios && pnpm run compile && JS_RUNTIME=bun MOCHA_PARALLEL=false bun --bun ../../../bin/run-test.cjs --no-c8 build/esm/test/test.getch.js -g "maxContentLength".Alternatives
maxContentLengthonly when aContent-Lengthheader is present ingetResponseData()was rejected because chunked/headerless responses silently bypass response size limits under Bun.node-fetchenforcessizeon headerless responses on Node.js.