diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..44cbd2b --- /dev/null +++ b/TRACE.md @@ -0,0 +1,91 @@ +# How `maxResponseSize` travels from `new Agent({ maxResponseSize })` to enforcement + +All paths are relative to the repo root. Line numbers refer to the tree after +the fix in part 3 (`lib/dispatcher/client-h1.js` gained three comment lines +above `onBody`'s size check; nothing else moved). + +## 1. Plain HTTP/1.1 request, default settings + +Scenario: `const agent = new Agent({ maxResponseSize: 1024 * 1024 })`, then +`request(url, { dispatcher: agent })` against an `http://` origin, no +`useMilo`, no `UNDICI_USE_MILO`, no `connections: 1`. + +| # | File | What it does with the option | +|---|------|------------------------------| +| 1 | `index.js:74-121` | `request` is `makeDispatcher(api.request)`. Line 108 pulls `dispatcher` (our Agent) out of the request opts and line 114 calls `api.request` with `this` bound to that Agent. `maxResponseSize` is not a per-request option here; it already lives inside the Agent. | +| 2 | `lib/api/api-request.js:221-233` | Builds a `RequestHandler` and calls `this.dispatch(opts, handler)` on the Agent. Does not look at the limit. | +| 3 | `lib/dispatcher/dispatcher-base.js:163-185` | `Agent.dispatch` (inherited). Validates handler/opts and forwards to the Agent's `[kDispatch]`. The constructor at lines 34-38 only reads `webSocket` and `eventSource` from the options object; `maxResponseSize` passes through untouched. | +| 4 | `lib/dispatcher/agent.js:25,48,95-97,18-22` | Constructor destructures `factory`, `maxOrigins`, `connect`, `tls`; everything else (including `maxResponseSize`) stays in `options` and is deep-cloned into `this[kOptions]` (line 48). On first dispatch for an origin, `[kDispatch]` passes `this[kOptions]` to the factory (lines 95-97); `defaultFactory` (lines 18-22) builds a `Pool` because `connections !== 1`. | +| 5 | `lib/dispatcher/pool.js:57-71,101,41,30-32` | `Pool` constructor destructures its own knobs (`connections`, `connect`, `allowH2`, ...) and keeps the rest, including `maxResponseSize`, in `this[kOptions]` (line 101). `createClient` (line 41) calls the factory with `pool[kOptions]`; `defaultFactory` (lines 30-32) does `new Client(origin, opts)`. | +| 6 | `lib/dispatcher/pool-base.js:243-250` | `Pool[kDispatch]` picks or creates a `Client` via `[kGetDispatcher]` (`pool.js:180-201`) and calls `client.dispatch(opts, handler)`. Nothing here touches the limit. | +| 7 | `lib/dispatcher/client.js:130,215-217,323` | `Client` constructor destructures `maxResponseSize` (line 130), validates it as an integer `>= -1` (lines 215-217), and stores it on the instance as `this[kMaxResponseSize]` (line 323), normalising anything not `> -1` to `-1` (disabled). `kMaxResponseSize` is defined in `lib/core/symbols.js:60`. | +| 8 | `lib/dispatcher/client.js:557-559` | When the socket connects, `connect()` chooses `connectH1` because `socket.alpnProtocol !== 'h2'` for plain TCP. | +| 9 | `lib/dispatcher/client-h1.js:907-909` | `connectH1` creates the parser. With `useMilo` unset and `UNDICI_USE_MILO` unset it picks the llhttp `Parser` (line 909), not Milo's `H1Parser`. | +| 10 | `lib/dispatcher/client-h1.js:261` | `Parser` constructor copies the limit from the client: `this.maxResponseSize = client[kMaxResponseSize]`. `this.bytesRead` starts at 0 (line 255). | +| 11 | `lib/dispatcher/client-h1.js:184-188` | llhttp's `wasm_on_body` callback wraps each body segment in a `FastBuffer` and calls `Parser#onBody`. | +| 12 | `lib/dispatcher/client-h1.js:751-785` | **Enforcement point.** `onBody` checks the limit (line 773, see part 3), adds the chunk to `this.bytesRead` (line 778), then hands the chunk to `request.onResponseData`. On violation it destroys the socket with `ResponseExceededMaxSizeError` (`lib/core/errors.js:344-349`, code `UND_ERR_RES_EXCEEDED_MAX_SIZE`) and returns `-1` to stop llhttp. `onMessageComplete` resets `bytesRead` to 0 for the next pipelined response (line 809). | + +## 2. Other ways of setting it + +For every entry below the final enforcement is one of three places: + +* llhttp HTTP/1.1: `lib/dispatcher/client-h1.js:773` (`this.bytesRead + buf.length > maxResponseSize`, cumulative after the part 3 fix). +* Milo HTTP/1.1: `lib/dispatcher/parser-h1.js:932` (`EVENT_DATA`) and `lib/dispatcher/parser-h1.js:433` (`consumeDirectBody`, the fast path for `Content-Length` bodies). Both are cumulative via `this.bytesRead`; it is reset per message at `parser-h1.js:648`. +* HTTP/2: `lib/dispatcher/client-h2.js:1455` in `onData`, cumulative via `state.bytesRead` (`client-h2.js:1463`). It aborts the single stream rather than the socket. + +### BalancedPool + +**Passed down and enforced on the whole body: yes.** + +* `lib/dispatcher/balanced-pool.js:52` destructures only `factory`, `connect`, `tls`; the rest (`maxResponseSize` included) is deep-cloned into `this[kOptions]` at line 62. +* `addUpstream` passes `this[kOptions]` to the factory at line 91; `defaultFactory` (lines 47-49) does `new Pool(origin, opts)`. +* From there it is the same as rows 5-12 above: `pool.js:101` → `client.js:323` → `client-h1.js:261,773` (or the Milo / h2 equivalents depending on the connection). + +### RetryAgent wrapping an Agent + +**Passed down and enforced: yes, provided the limit is set on the inner Agent.** The RetryAgent itself never sees or forwards it. + +* `lib/dispatcher/retry-agent.js:10-16`: the constructor stores the options object as retry options only. `super(options)` goes to `lib/dispatcher/dispatcher.js:5` (a bare `EventEmitter` subclass with no constructor logic), so a `maxResponseSize` passed to `new RetryAgent(agent, { maxResponseSize })` is silently ignored. +* `lib/dispatcher/retry-agent.js:18-27`: `dispatch` wraps the handler in a `RetryHandler` and forwards to `this.#agent.dispatch`, so the inner Agent's own `kMaxResponseSize` (via `agent.js:48` → `pool.js:101` → `client.js:323`) is what gets enforced at `client-h1.js:773` / `parser-h1.js:932,433` / `client-h2.js:1455`. +* `ResponseExceededMaxSizeError` (`UND_ERR_RES_EXCEEDED_MAX_SIZE`) is not in the default retry `errorCodes` list (`lib/handler/retry-handler.js:135-145`), so the error surfaces to the caller instead of triggering a retry. + +### ProxyAgent + +**Passed down and enforced for tunnelled (CONNECT) targets; dropped for plain `http://` targets going through an HTTP(S) proxy, and dropped for SOCKS5 proxies.** + +* `lib/dispatcher/proxy-agent.js:200-202`: `new Agent({ ...opts, factory, connect })` spreads the ProxyAgent options, so `maxResponseSize` reaches the inner Agent's `kOptions` (`agent.js:48`) and is handed to the custom `factory` on every new origin (`agent.js:95-97`). +* Tunnel case (`https://` targets, or `proxyTunnel: true`): `proxy-agent.js:189` calls `agentFactory(origin, options)` → `defaultAgentFactory` (`proxy-agent.js:34-39`) → `Pool`/`Client` with the full options → `client.js:323` → enforced at `client-h1.js:773` (or Milo / h2). **Enforced on the whole body.** +* Non-tunnel case (`http://` target, default `proxyTunnel`; `shouldProxyTunnel` at `proxy-agent.js:41-43` returns false): `proxy-agent.js:180-187` builds an `Http1ProxyWrapper` with only `{ headers, connect, factory, proxyServername }`. Its constructor (`proxy-agent.js:58-62`) then does `factory(proxyUrl, { connect })` or `new Client(proxyUrl, { connect })`. The options object passed to the inner Client contains only `connect`, so **`maxResponseSize` is lost and the response body is not limited**. +* SOCKS5 case: `proxy-agent.js:157-168` builds a `Socks5ProxyAgent` without forwarding `maxResponseSize`; `lib/dispatcher/socks5-proxy-agent.js:235-238` creates `new Pool(origin, { pipelining, connections, connect })`, so **the limit is dropped here too**. +* The `clientFactory(url, { connect })` at `proxy-agent.js:197` is the client used only for the `CONNECT` handshake; it never reads a response body, so the missing limit there is harmless. + +### H2CClient + +**Passed down and enforced on the whole body: yes.** + +* `lib/dispatcher/h2c-client.js:18-19` destructures `maxConcurrentStreams` and `pipelining`; `...opts` keeps `maxResponseSize`. +* `h2c-client.js:41-47` calls `super(origin, { ...opts, allowH2: true, useH2c: true })` → `client.js:130,215-217,323`. +* `useH2c: true` makes the connector tag the socket with `alpnProtocol = 'h2'` (`lib/core/connect.js:135-137`), so `client.js:557-558` chooses `connectH2`. +* `lib/dispatcher/client-h2.js:1141` copies `client[kMaxResponseSize]` into the per-stream `state`; `client-h2.js:1444-1463` `onData` checks `state.bytesRead + chunk.length > maxResponseSize` for every DATA frame and aborts the stream with `ResponseExceededMaxSizeError`. Covered by the existing h2c tests in `test/max-response-size.js`. + +### Client with the Milo HTTP/1 parser + +**Passed down and enforced on the whole body: yes.** + +* `new Client(origin, { maxResponseSize, useMilo: true })`: `client.js:130,140` destructure both; `client.js:323` stores the limit, `client.js:340` stores `useMilo` as `this[kUseMilo]`. +* `lib/dispatcher/client-h1.js:907-908`: `connectH1` picks `new H1Parser(client, socket)` when `client[kUseMilo]` is true (or `UNDICI_USE_MILO=1`, see `parser-h1.js:71,1061`). +* `lib/dispatcher/parser-h1.js:173` copies `client[kMaxResponseSize]` onto the parser; `bytesRead` starts at 0 (line 169) and resets per message (line 648). +* Chunked / parser-driven body data: `parser-h1.js:913-940` (`EVENT_DATA`), check at line 932 is `this.bytesRead + len > maxResponseSize`. +* `Content-Length` fast path: `parser-h1.js:418-437` (`consumeDirectBody`), check at line 433 is also cumulative. +* Both branches were already correct; the new Milo variants in `test/max-response-size.js` confirm it. + +## 3. Reported bug + +Cause: `lib/dispatcher/client-h1.js` `Parser#onBody` compared only the current +chunk against the limit (`buf.length > maxResponseSize`) instead of the running +total, so any body delivered in pieces that were each smaller than the limit +slipped through. The Milo and HTTP/2 paths already used the cumulative +`bytesRead`. Fixed to `this.bytesRead + buf.length > maxResponseSize` +(`client-h1.js:773`), with regression tests in `test/max-response-size.js` +for both llhttp and Milo (chunked body over the limit is rejected; chunked +body exactly at the limit is allowed). diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js index 295facf..c544611 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -767,7 +767,10 @@ class Parser { assert(statusCode >= 200) - if (maxResponseSize > -1 && buf.length > maxResponseSize) { + // Compare against the running total, not just this chunk: a body that + // arrives in many small pieces (chunked encoding, slow peer) must be + // rejected once the cumulative size passes the limit. + 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..115c198 100644 --- a/test/max-response-size.js +++ b/test/max-response-size.js @@ -118,6 +118,98 @@ describe('max response size', async (t) => { await t.completed }) + for (const useMilo of [false, true]) { + const parser = useMilo ? 'milo' : 'llhttp' + + test(`should throw an error if the response is streamed in chunks that each fit the limit (${parser})`, async (t) => { + t = tspl(t, { plan: 2 }) + + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + // 4 chunks of 5 bytes each: every chunk is under the 10 byte limit but + // the whole body (20 bytes) is not. + server.on('request', (req, res) => { + let remaining = 4 + const writeChunk = () => { + res.write('hello') + if (--remaining > 0) { + setImmediate(writeChunk) + } else { + res.end() + } + } + writeChunk() + }) + + server.listen(0, () => { + const client = new Client(`http://localhost:${server.address().port}`, { + maxResponseSize: 10, + useMilo + }) + + after(() => client.destroy()) + + client.request({ path: '/', method: 'GET' }, (err, { body }) => { + t.ifError(err) + body.on('data', () => {}) + body.on('error', (err) => { + t.ok(err instanceof errors.ResponseExceededMaxSizeError) + }) + body.on('end', () => { + t.fail('body should not complete') + }) + }) + }) + + await t.completed + }) + + test(`should allow a chunked response whose total size equals the limit (${parser})`, async (t) => { + t = tspl(t, { plan: 3 }) + + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('request', (req, res) => { + res.write('hello') + setImmediate(() => { + res.write('world') + res.end() + }) + }) + + server.listen(0, () => { + const client = new Client(`http://localhost:${server.address().port}`, { + maxResponseSize: 10, + useMilo + }) + + after(() => client.close()) + + client.request({ path: '/', method: 'GET' }, (err, { statusCode, body }) => { + t.ifError(err) + t.strictEqual(statusCode, 200) + const bufs = [] + body.on('data', (buf) => { + bufs.push(buf) + }) + body.on('end', () => { + t.strictEqual('helloworld', Buffer.concat(bufs).toString('utf8')) + }) + }) + }) + + await t.completed + }) + } + test('should throw an error if the response is too big over h2c', async (t) => { t = tspl(t, { plan: 1 })