diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..41fd2ea --- /dev/null +++ b/TRACE.md @@ -0,0 +1,24 @@ +# `maxResponseSize` trace + +## 1. `new Agent({ maxResponseSize })` → enforcement (plain HTTP/1.1, default settings) + +1. `lib/dispatcher/agent.js` — the constructor (L25) leaves `maxResponseSize` in the `...options` rest object and stores it in `this[kOptions]` (L48). On the first dispatch to an origin, `[kDispatch]` passes those options to the factory (L95-97). With default settings, `defaultFactory` (L18-22) builds a `Pool`, because `connections` is not 1. +2. `lib/dispatcher/pool.js` — the constructor (L57-71) leaves `maxResponseSize` in `...options` and stores it in `this[kOptions]` (L101). `createClient` (L41) passes those options to `defaultFactory`, which runs `new Client(origin, opts)` (L30-32). +3. `lib/dispatcher/client.js` — the constructor destructures `maxResponseSize` (L130), validates it (L215-216), and stores it as `this[kMaxResponseSize]`, with -1 meaning "no limit" (L323). Once a socket connects, `connectH1(client, socket)` is called for a non-h2 socket (L557-559). +4. `lib/dispatcher/client-h1.js` — `connectH1` picks the parser (L907-909). With default settings (`useMilo` not set and `UNDICI_USE_MILO` not set, see `lib/dispatcher/parser-h1.js` L70-71) it is the llhttp `Parser`. Its constructor copies `client[kMaxResponseSize]` into `this.maxResponseSize` (L261). The llhttp `wasm_on_body` callback (L184-187) calls `Parser.onBody`, which enforces the limit (L770) and destroys the socket with `ResponseExceededMaxSizeError`. **Bug (now fixed):** L770 compared only the current chunk (`buf.length > maxResponseSize`), not the running total. Each chunk below the limit was accepted, so a large body sent in small chunks got through. It now checks `this.bytesRead + buf.length`. `bytesRead` is increased at L775 and reset per message at L806. + +(`lib/dispatcher/dispatcher-base.js` and `lib/dispatcher/pool-base.js` also receive the options through `super(options)`, but they ignore `maxResponseSize`.) + +## 2. Other ways to set it + +| Setup | Passed down? | Whole body enforced? | Evidence | +|---|---|---|---| +| **BalancedPool** `new BalancedPool(upstreams, { maxResponseSize })` | Yes | Yes, after this fix (it goes through the same llhttp path as in section 1) | `lib/dispatcher/balanced-pool.js` L52 (rest `...opts`), L62 (`this[kOptions]`), L90 (`this[kFactory](upstreamOrigin, this[kOptions])`) → `defaultFactory` → `new Pool` (L47-48) → `lib/dispatcher/pool.js` L101/L41 → `lib/dispatcher/client.js` L323 → `lib/dispatcher/client-h1.js` L770 | +| **RetryAgent wrapping an Agent** `new RetryAgent(new Agent({ maxResponseSize }))` | Yes, through the inner Agent. RetryAgent only forwards `dispatch` to it (`lib/dispatcher/retry-agent.js` L18-26). | Per response attempt, yes, after this fix (same path as section 1). Caveats: (a) passing `maxResponseSize` to RetryAgent's own options does nothing. Those options become `retryOptions` (L13, L21) and go to `Dispatcher`'s `super(options)` (L11), which ignores them. (b) Each retry is a new response, so `bytesRead` restarts at 0 (`lib/dispatcher/client-h1.js` L806). If a retryable error (e.g. ECONNRESET) happens partway through and the transfer resumes with a `Range` request (`lib/handler/retry-handler.js` L386-471), the parts are counted separately. The total body seen by the caller can then exceed the limit. | `lib/dispatcher/retry-agent.js` L10-26 | +| **ProxyAgent** `new ProxyAgent({ uri, maxResponseSize })` | **Only for tunnelled requests.** The internal Agent gets `...opts` (`lib/dispatcher/proxy-agent.js` L200-202). For tunnelled targets (`https:`, or `proxyTunnel: true`, see L41-43), the custom factory calls `agentFactory(origin, options)` (L188). That builds a Pool/Client with the limit (L34-39) and reaches `client-h1.js` L770 (or the h2 path if h2 is negotiated). **For plain `http:` targets with the default `proxyTunnel`**, the factory returns `Http1ProxyWrapper` (L170-187). The wrapper builds its proxy client with only `{ connect }`: `factory(proxyUrl, { connect })` / `new Client(proxyUrl, { connect })` (L58-62). So `maxResponseSize` is dropped and **not enforced at all**. The CONNECT client to the proxy (`clientFactory(url, { connect })`, L197) also has no limit, but it only carries the CONNECT handshake. | Tunnelled: yes, after this fix. Non-tunnelled http: no. | `lib/dispatcher/proxy-agent.js` L41-43, L49-62, L152-188, L197-202 | +| **H2CClient** `new H2CClient(origin, { maxResponseSize })` | Yes | Yes, already before this fix. The h2 path checks the running total. | `lib/dispatcher/h2c-client.js` L18-19 (rest `...opts`), L41-47 (`super(origin, { ...opts, allowH2: true, useH2c: true })`) → `lib/dispatcher/client.js` L323, L557-558 (`connectH2`) → `lib/dispatcher/client-h2.js` L1141 (`maxResponseSize: client[kMaxResponseSize]` in per-stream state, with `bytesRead: 0` at L1136) and L1455 (`state.bytesRead + chunk.length > maxResponseSize`, then the stream is reset with `ResponseExceededMaxSizeError`) | +| **Client with Milo** `new Client(origin, { maxResponseSize, useMilo: true })` (or `UNDICI_USE_MILO=1`) | Yes | Yes, already before this fix. Both Milo body paths check the running total. | `lib/dispatcher/client.js` L140, L340 (`this[kUseMilo]`), L323 → `lib/dispatcher/client-h1.js` L907-908 (`new H1Parser`) → `lib/dispatcher/parser-h1.js` L173 (`this.maxResponseSize = client[kMaxResponseSize]`), L433 (direct-body fast path: `this.bytesRead + len > maxResponseSize`), L932 (`EVENT_DATA` path: same check), reset per message at L648 | + +Summary: before the fix, only the llhttp HTTP/1.1 parser (the default) checked each chunk instead of the total. That parser is used by Agent, Pool, BalancedPool, RetryAgent, a plain Client and tunnelled ProxyAgent requests. The h2 path and the Milo parser were already correct. The fix is in `lib/dispatcher/client-h1.js` L770. + +One separate gap is not fixed here: for non-tunnelled ProxyAgent requests to `http:` targets, the limit is never passed down at all. diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js index 295facf..a26b9c0 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -767,7 +767,7 @@ class Parser { assert(statusCode >= 200) - if (maxResponseSize > -1 && buf.length > maxResponseSize) { + if (maxResponseSize > -1 && this.bytesRead + buf.length > maxResponseSize) { util.destroy(socket, new ResponseExceededMaxSizeError()) return -1 } diff --git a/test/max-response-size.js b/test/max-response-size.js index cb5d120..21874a7 100644 --- a/test/max-response-size.js +++ b/test/max-response-size.js @@ -3,7 +3,7 @@ const { tspl } = require('@matteo.collina/tspl') const { test, after, describe } = require('node:test') const { once } = require('node:events') -const { Client, H2CClient, errors } = require('..') +const { Client, H2CClient, Agent, request, errors } = require('..') const { createServer } = require('node:http') const { createServer: createH2CServer } = require('node:http2') @@ -197,6 +197,57 @@ describe('max response size', async (t) => { await t.completed }) + for (const useMilo of [false, true]) { + test(`should enforce the limit on the whole body when it arrives in many small chunks (useMilo: ${useMilo})`, async (t) => { + t = tspl(t, { plan: 2 }) + + const chunk = Buffer.alloc(64, 'a') + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + // 64 chunks of 64 bytes, each written on its own timer so that every + // chunk reaches the client in a separate read, each below the limit. + server.on('request', (req, res) => { + let sent = 0 + const write = () => { + if (res.destroyed) return + if (sent++ === 64) { + res.end() + return + } + res.write(chunk) + setTimeout(write, 1) + } + write() + }) + + server.listen(0) + await once(server, 'listening') + + const dispatcher = new Agent({ maxResponseSize: 1024, useMilo }) + after(() => dispatcher.destroy()) + + let received = 0 + let error = null + try { + const { body } = await request(`http://localhost:${server.address().port}`, { dispatcher }) + for await (const buf of body) { + received += buf.length + } + } catch (err) { + error = err + } + + t.ok(error instanceof errors.ResponseExceededMaxSizeError) + t.ok(received <= 1024, `received ${received} bytes`) + + await t.completed + }) + } + test('invalid max response size should throw an error', async (t) => { t = tspl(t, { plan: 2 })