diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..d52ef9d --- /dev/null +++ b/TRACE.md @@ -0,0 +1,22 @@ +# How `maxResponseSize` travels through the code + +## 1. `new Agent({ maxResponseSize })` used as the dispatcher for `request()` (plain HTTP/1.1, default settings) + +`request()` never reads the option. It only calls `dispatcher.dispatch()` (`lib/api/api-request.js:233`). The limit lives on the dispatcher chain: + +1. `lib/dispatcher/agent.js`: the constructor collects `maxResponseSize` into `...options` (line 25) and stores a copy in `this[kOptions]` (line 48). On the first dispatch to an origin, `[kDispatch]` (line 78) calls `this[kFactory](origin, this[kOptions])` (line 95). The default factory then builds `new Pool(origin, opts)` (line 21), or a `Client` if `connections === 1`. +2. `lib/dispatcher/pool.js`: the constructor stores the options, including `maxResponseSize`, in `this[kOptions]` (line 101). `createClient` (line 41) passes them to the factory, which runs `new Client(origin, opts)` (line 31). Dispatch reaches it through `lib/dispatcher/pool-base.js` `[kDispatch]` (line 243) and `[kGetDispatcher]` (pool.js:180/199). +3. `lib/dispatcher/client.js`: the constructor destructures `maxResponseSize` (line 130), checks it (lines 215-216) and stores it as `this[kMaxResponseSize]` (line 323; `null`/`undefined` become `-1`, meaning no limit). Once the socket connects, `connect()` calls `connectH1(client, socket)` (line 559), because a plain `http:` socket has no `h2` ALPN. +4. `lib/dispatcher/client-h1.js`: `connectH1` creates the llhttp `Parser` (line 909; Milo is off by default). The `Parser` constructor copies `client[kMaxResponseSize]` into `this.maxResponseSize` (line 261). `Parser.onBody()` enforces it (line 770) and destroys the socket with `ResponseExceededMaxSizeError`. `this.bytesRead` adds up the bytes (line 775) and is reset for each response in `onMessageComplete` (line 806). + + **This is where the reported bug was.** Line 770 checked `buf.length > maxResponseSize`, so it compared each chunk against the limit on its own, not the running total. A body sent in chunks that were each under the limit was never rejected. It now checks `this.bytesRead + buf.length > maxResponseSize`, the same way the Milo and H2 paths already did. + +## 2. Other ways to set it + +| Setup | Passed down? | Limit applies to the whole body? | Evidence | +|---|---|---|---| +| **BalancedPool** | Yes | Yes (after the fix; before it, the same per-chunk bug as Agent) | `lib/dispatcher/balanced-pool.js:52` (`...opts`) → `:62` (`this[kOptions]`) → `:91` (`this[kFactory](upstreamOrigin, this[kOptions])`) → `:48` `new Pool(origin, opts)` → `lib/dispatcher/pool.js:101`, `:41`, `:31` → `lib/dispatcher/client.js:323` → `lib/dispatcher/client-h1.js:261`, `:770`. | +| **RetryAgent wrapping an Agent** | Yes, but only from the wrapped Agent. RetryAgent's own options are retry options: they go to `super(options)` and to `retryOptions` (`lib/dispatcher/retry-agent.js:10-13`, `:19-21`) and never reach a Client. | Yes for each attempt (after the fix). Every attempt goes through `this.#agent.dispatch` (`retry-agent.js:23`, `:26`), so it follows the Agent path in section 1. `UND_ERR_RES_EXCEEDED_MAX_SIZE` is not in the default `errorCodes` (`lib/handler/retry-handler.js:135-145`), so a size error is not retried. Caveat: if a body is cut off by a retryable error (e.g. `ECONNRESET`) and resumed with a `Range` request, the new response's parser starts counting from 0 (`client-h1.js:806`). The limit then applies to each attempt, not to the total delivered across attempts. | `lib/dispatcher/retry-agent.js:10-26` | +| **ProxyAgent** | **Only for tunneled requests.** `opts` is spread into the inner `Agent` (`lib/dispatcher/proxy-agent.js:200-201`). For tunneled requests (`https:` targets, or `proxyTunnel: true`), `factory` returns `agentFactory(origin, options)` (`:189`), and `options` contains `maxResponseSize`. For **`http:` targets without `proxyTunnel: true`** (`shouldProxyTunnel`, `:41-43`), `factory` returns an `Http1ProxyWrapper` (`:180-187`) that is never given the option. It builds its Client with only `{ connect }` (`:59` / `:61`). | Tunneled: yes (after the fix), through `client.js:323` → `client-h1.js:770`. Forwarded `http:` targets: **not enforced at all**. This is a separate gap that this change does not fix. | `lib/dispatcher/proxy-agent.js:41-43`, `:59-61`, `:180-189`, `:200-201` | +| **H2CClient** | Yes | Yes. It already counted the whole body before the fix. | `lib/dispatcher/h2c-client.js:18` (`...opts`) → `:41-46` `super(origin, { ...opts, useH2c: true })` → `lib/dispatcher/client.js:323`. `lib/core/connect.js:136` sets `socket.alpnProtocol = 'h2'`, so `client.js:558` calls `connectH2`. Then `lib/dispatcher/client-h2.js:1141` (copied into the per-stream state) and `:1455` (`state.bytesRead + chunk.length > maxResponseSize`, adds up per stream at `:1463`). | +| **Client with Milo (`useMilo: true` or `UNDICI_USE_MILO=1`)** | Yes | Yes. It already counted the whole body before the fix (the new regression test passed for Milo on the old code). | `lib/dispatcher/client.js:140`, `:340` (`kUseMilo`) → `lib/dispatcher/client-h1.js:907-908` `new H1Parser(client, socket)` → `lib/dispatcher/parser-h1.js:173` (copied in). Enforced in `consumeDirectBody` at `:433` and on `EVENT_DATA` at `:932`, both as `this.bytesRead + len > maxResponseSize`. Reset for each message at `:648`. | 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..9675b82 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 { Agent, Client, H2CClient, errors, request } = require('..') const { createServer } = require('node:http') const { createServer: createH2CServer } = require('node:http2') @@ -118,6 +118,50 @@ describe('max response size', async (t) => { await t.completed }) + for (const useMilo of [false, true]) { + test(`should count the whole body when it arrives in small chunks (useMilo: ${useMilo})`, async (t) => { + t = tspl(t, { plan: 2 }) + + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + // 20 chunks of 10 bytes, each below the limit but 200 bytes in total + server.on('request', async (req, res) => { + res.on('error', () => {}) + for (let i = 0; i < 20 && !res.destroyed; i++) { + res.write('x'.repeat(10)) + await new Promise((resolve) => setTimeout(resolve, 5)) + } + res.end() + }) + + server.listen(0) + await once(server, 'listening') + + const agent = new Agent({ maxResponseSize: 50, useMilo }) + after(() => agent.close()) + + let received = 0 + let error = null + try { + const { body } = await request(`http://localhost:${server.address().port}`, { dispatcher: agent }) + for await (const chunk of body) { + received += chunk.length + } + } catch (err) { + error = err + } + + t.ok(error instanceof errors.ResponseExceededMaxSizeError) + t.ok(received <= 50, `received ${received} bytes`) + + await t.completed + }) + } + test('should throw an error if the response is too big over h2c', async (t) => { t = tspl(t, { plan: 1 })