Repository navigation
fix(gaxios): enforce maxContentLength on stream responses in Bun - #9777
danieljbruce wants to merge 2 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.
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.