Repository navigation
fix(gaxios): enforce maxContentLength on stream responses in Bun - #9777
danieljbruce wants to merge 4 commits into
Conversation
![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 introduces lazy conversion of the Web ReadableStream response body into a Node.js Readable stream, wrapping it in a byte-counting Transform stream when maxSize is specified to enforce maxContentLength. A corresponding test was also added. The review feedback highlights a potential crash when handling null or undefined response bodies (such as 204 or 304 responses) and suggests adding a guard. Additionally, it points out redundant stream destruction logic inside the transform method that can be simplified.
| 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; | ||
| }; | ||
| const readBuffer = async () => { | ||
| nodeStream ||= Readable.fromWeb(rawBody); | ||
| const chunks: Buffer[] = []; | ||
| 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); | ||
| for await (const chunk of getNodeStream()) { | ||
| chunks.push( | ||
| Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk as Uint8Array), | ||
| ); | ||
| } | ||
| return Buffer.concat(chunks); | ||
| }; |
There was a problem hiding this comment.
1. Potential Crash on Null/Undefined Response Body
If the response body is null (e.g., for 204 No Content or 304 Not Modified responses), Readable.fromWeb(rawBody) will throw a TypeError because rawBody is null. We should add a guard to return null from getNodeStream() when rawBody is null, and handle this in readBuffer() to prevent runtime crashes.
2. Redundant Stream Destruction
Calling webReadable.destroy() directly inside the transform method is redundant because the limiter.once('close', ...) listener already handles destroying webReadable when the limiter stream closes. Removing it simplifies the logic and avoids potential race conditions.
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 (!rawBody) {
return null;
}
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) {
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;
};
const readBuffer = async () => {
const chunks: Buffer[] = [];
const stream = getNodeStream();
if (stream) {
for await (const chunk of stream) {
chunks.push(
Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk as Uint8Array)
);
}
}
return Buffer.concat(chunks);
};References
- Avoid overriding
_destroyon instantiated Node.js streams for cleanup. Instead, use the standard public APIstream.once('close', ...)for stream cleanup, as Node.js emits'close'after everydestroy()(whether due to early break, error, or normal end).
There was a problem hiding this comment.
Addressed in dd7cbab: added a null guard (if (!rawBody) return null;) in getNodeStream(), guarded if (stream) in readBuffer(), removed the redundant webReadable.destroy() call inside transform(), and switched to pipeline(webReadable, limiter, () => {}) for stream error propagation and cleanup.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors how response streams are handled in gaxios by introducing a lazy getNodeStream helper that wraps the Web ReadableStream in a byte-counting Transform stream when maxSize is specified. This ensures that maxContentLength is enforced for both buffered and streamed responses, and a corresponding test case is added. The review feedback points out a potential TypeError when rawBody is null (such as in 204 No Content or HEAD requests) and suggests using the pipeline utility from the stream module to simplify error propagation and stream cleanup.
| 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; | ||
| }; | ||
| const readBuffer = async () => { | ||
| nodeStream ||= Readable.fromWeb(rawBody); | ||
| const chunks: Buffer[] = []; | ||
| 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); | ||
| for await (const chunk of getNodeStream()) { | ||
| chunks.push( | ||
| Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk as Uint8Array), | ||
| ); | ||
| } | ||
| return Buffer.concat(chunks); | ||
| }; | ||
| Object.defineProperty(res, 'body', { | ||
| get() { | ||
| nodeStream ||= Readable.fromWeb(rawBody); | ||
| return nodeStream; | ||
| return getNodeStream(); | ||
| }, | ||
| configurable: true, | ||
| enumerable: true, |
There was a problem hiding this comment.
If the response has no body (e.g., for a 204 No Content or HEAD request), rawBody will be null. Calling Readable.fromWeb(rawBody) directly will throw a TypeError under Bun, crashing the request.
Additionally, managing manual event listeners for error propagation and stream destruction can be error-prone and lead to resource leaks if one stream is destroyed but the other is not. We can simplify this and make it extremely robust by using pipeline from the stream module, which automatically handles bidirectional error propagation and stream cleanup.
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 (!rawBody) {
return null;
}
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) {
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);
},
});
nodeStream = pipeline(webReadable, limiter, () => {});
} else {
nodeStream = webReadable;
}
}
return nodeStream;
};
const readBuffer = async () => {
const chunks: Buffer[] = [];
const stream = getNodeStream();
if (stream) {
for await (const chunk of stream) {
chunks.push(
Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk as Uint8Array)
);
}
}
return Buffer.concat(chunks);
};
Object.defineProperty(res, 'body', {
get() {
return getNodeStream();
},
configurable: true,
enumerable: true,There was a problem hiding this comment.
Addressed in dd7cbab: added if (!rawBody) { return null; } in getNodeStream(), guarded if (stream) in readBuffer(), and replaced the manual stream listeners with nodeStream = pipeline(webReadable, limiter, () => {});.
…or byte limiter stream
| } 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 Node's built-in stream module so getNodeStream() can construct a byte-counting transform stream when maxContentLength (maxSize) is configured.
| 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 (!rawBody) { | ||
| return null; | ||
| } | ||
| 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) { | ||
| 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); | ||
| }, | ||
| }); | ||
| nodeStream = pipeline(webReadable, limiter, () => {}); | ||
| } else { | ||
| nodeStream = webReadable; | ||
| } | ||
| } | ||
| return nodeStream; | ||
| }; |
There was a problem hiding this comment.
[Generated] Define getNodeStream() to lazily convert the Web ReadableStream (rawBody) into a Node.js Readable stream (returning null if rawBody is null/empty) and pipe it through a byte-counting Transform stream via pipeline(webReadable, limiter, () => {}) when maxSize > 0. This ensures both res.body stream consumers (responseType: 'stream') and buffered body readers enforce maxContentLength with automatic stream cleanup and error propagation.
| return nodeStream; | ||
| }; | ||
| const readBuffer = async () => { | ||
| nodeStream ||= Readable.fromWeb(rawBody); |
There was a problem hiding this comment.
[Generated] Remove direct Readable.fromWeb(rawBody) initialization from readBuffer() since stream creation and maxContentLength enforcement are now centralized in getNodeStream().
| const stream = getNodeStream(); | ||
| if (stream) { | ||
| for await (const chunk of stream) { | ||
| chunks.push( | ||
| Buffer.isBuffer(chunk) | ||
| ? chunk | ||
| : Buffer.from(chunk as Uint8Array), |
There was a problem hiding this comment.
[Generated] Simplify readBuffer() to consume the stream returned by getNodeStream() (when non-null), delegating maxContentLength byte-limit checking to getNodeStream()'s Transform stream rather than duplicating byte counting in readBuffer().
| : Buffer.from(chunk as Uint8Array), | ||
| ); | ||
| } | ||
| chunks.push(buf); |
There was a problem hiding this comment.
[Generated] Consolidate chunk buffering inside the for await (const chunk of stream) loop now that byte-limit enforcement is handled upstream by getNodeStream().
| get() { | ||
| nodeStream ||= Readable.fromWeb(rawBody); | ||
| return nodeStream; | ||
| return getNodeStream(); |
There was a problem hiding this comment.
[Generated] Route the res.body getter through getNodeStream() so responseType: 'stream' callers receive the maxContentLength-enforcing Node.js Readable stream under Bun.
| ); | ||
|
|
||
| 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 a unit test for responseType: 'stream' with maxContentLength: 2 on the Bun fetch code path (ReadableStream response body), verifying that iterating streamMaxRes.data rejects with over limit: 2 when the streamed payload exceeds maxContentLength.
…9773) ## Description Enforces `maxContentLength` on headerless buffered responses under Bun. ## Impact Prevents oversized buffered responses from bypassing `maxContentLength` limits on Bun. ## Changes - Enforce `maxSize` (`rawInit.size` / `rawInit.maxContentLength`) inside `readBuffer()` in `Gaxios.#createBunFetch()` so buffered (`json`, `text`, `arraybuffer`, `blob`) responses reject with `content size at <url> over limit: <maxSize>` when exceeded even without a `Content-Length` header. ### Stack 1. **#9773** — `fix(gaxios): enforce maxContentLength on headerless responses in Bun` (`fix/gaxios-bun-max-content-length` → `main`) *(this PR)* 2. #9777 — `fix(gaxios): enforce maxContentLength on stream responses in Bun` (`fix/gaxios-bun-max-content-length-stream` → `fix/gaxios-bun-max-content-length`) 3. #9778 — `fix(gaxios): honor maxContentLength and maxRedirects instance defaults` (`fix/gaxios-instance-defaults` → `fix/gaxios-bun-max-content-length-stream`) 4. #9774 — `fix(gaxios): forward https.Agent TLS options and agent factories in Bun` (`fix/gaxios-bun-agent-tls` → `fix/gaxios-instance-defaults`) ## Testing - Added unit tests in `core/packages/gaxios/test/test.getch.ts` covering `maxContentLength` rejection on headerless responses across `json`, `text`, `arraybuffer`, and `blob` response types. - **Steps to Reproduce (in terminal)**: Run `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 - Checking `maxContentLength` only when a `Content-Length` header is present in `getResponseData()` was rejected because chunked/headerless responses silently bypass response size limits under Bun. - Not merging this PR was rejected because `node-fetch` enforces `size` on headerless responses on Node.js.
Executive SummaryWhen
Runtime Behavior Matrix (
|
| Runtime | Before | After |
|---|---|---|
Bun (Gaxios.#createBunFetch over globalThis.fetch) |
Buffered (json, text, arraybuffer, blob): Enforced maxContentLength via the upfront Content-Length check and per-chunk counting in readBuffer().Streaming ( responseType: 'stream'): res.body returned Readable.fromWeb(rawBody) directly with no byte counter; headerless/chunked streams exceeding maxContentLength were not rejected during stream consumption. |
Buffered & Streaming: getNodeStream() lazily wraps Readable.fromWeb(rawBody) in a byte-counting Transform via pipeline() whenever maxSize > 0 (and returns Readable.fromWeb(rawBody) directly when maxSize <= 0). Both res.body (responseType: 'stream') and readBuffer() consume getNodeStream(), rejecting with Error('content size at <url> over limit: <maxSize>') (type: 'max-size') as soon as cumulative bytes exceed maxContentLength. |
Node V8 (node-fetch via Gaxios.#getFetch) |
Buffered & Streaming: Gaxios.#getFetch() selects node-fetch (since 'Bun' in globalThis is false), which natively tracks cumulative response bytes against opts.size (maxContentLength) across both buffered reads and res.body streams, emitting a max-size error when the limit is exceeded. |
Buffered & Streaming: Unchanged. Node V8 continues to use node-fetch with native opts.size enforcement on both buffered and streaming responses; Bun's res.body stream behavior now matches Node V8 parity. |
Description
Enforces
maxContentLengthon streaming (responseType: 'stream') responses under Bun.Impact
Immediately aborts oversized response streams (
res.body) under Bun whenmaxContentLengthis exceeded.Changes
getNodeStream()inGaxios.#createBunFetch()to lazily convert the WebReadableStreaminto a Node.jsReadableand pipe it through a byte-countingTransformwhenmaxSize > 0.getNodeStream()from theres.bodygetter andreadBuffer()so bothresponseType: 'stream'and buffered readers share the byte-limiting stream.Stack
fix(gaxios): enforce maxContentLength on headerless responses in Bun(fix/gaxios-bun-max-content-length→main)fix(gaxios): enforce maxContentLength on stream responses in Bun(fix/gaxios-bun-max-content-length-stream→fix/gaxios-bun-max-content-length) (this PR)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
responseType: 'stream'test incore/packages/gaxios/test/test.getch.tsverifying that consumingres.datarejects withover limit: 2whenmaxContentLengthis exceeded.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 "Web ReadableStream response body in Bun fetch path".Alternatives
maxContentLengthonly insidereadBuffer()(fix(gaxios): enforce maxContentLength on headerless responses in Bun #9773) was considered as the standalone baseline, but leavesresponseType: 'stream'callers consumingres.bodydirectly without byte-limit enforcement.node-fetchenforcessizeonres.bodystreams on Node.js.