diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..fec2acd --- /dev/null +++ b/TRACE.md @@ -0,0 +1,37 @@ +# How `maxResponseSize` reaches enforcement + +## 1. `new Agent({ maxResponseSize })`, plain HTTP/1.1, default settings + +1. `lib/dispatcher/agent.js:25` – the constructor destructures `factory`, `maxOrigins`, `connect` and `tls`. Everything else, including `maxResponseSize`, stays in `...options`. Line 48 stores it in `this[kOptions]`. +2. `lib/dispatcher/agent.js:95-97` – on the first dispatch to an origin, `this[kFactory](origin, this[kOptions])` is called. `defaultFactory` (lines 18-22) returns `new Pool(origin, opts)`, or a `Client` if `connections === 1`. +3. `lib/dispatcher/pool.js:59-70,101` – the Pool keeps `maxResponseSize` in `...options` and copies it into `this[kOptions]`. `lib/dispatcher/pool.js:41` then calls `pool[kFactory](pool[kUrl], pool[kOptions])`. `defaultFactory` (lines 30-32) is `new Client(origin, opts)`. +4. `lib/dispatcher/client.js:130,215-216,323` – the Client destructures the option, validates it (an integer >= -1), and stores it as `this[kMaxResponseSize]` (`-1` means unlimited). +5. `lib/dispatcher/client-h1.js:907-909` – when a socket connects, the default build picks the llhttp `Parser`. The Milo `H1Parser` is chosen only if `kUseMilo` or `UNDICI_USE_MILO` is set. +6. `lib/dispatcher/client-h1.js:255,261` – the llhttp `Parser` constructor sets `this.bytesRead = 0` and copies `client[kMaxResponseSize]` into `this.maxResponseSize`. +7. `lib/dispatcher/client-h1.js:751-782` – `Parser.onBody` runs once per body chunk, and this is where the limit is enforced. It destroys the socket with `ResponseExceededMaxSizeError` when the limit is exceeded, otherwise it adds to `this.bytesRead`. `bytesRead` is reset per message at line 806. + +## 2. Other ways of setting it + +| Setup | Passed down? | Enforced on the whole body? | Evidence | +|---|---|---|---| +| `BalancedPool` | Yes | Yes (once the fix is applied) | `lib/dispatcher/balanced-pool.js:52,62,91` keeps the option in `opts`, copies it into `kOptions`, and gives it to `new Pool(...)`. From there the path is the same as in section 1. | +| `RetryAgent` | There is nothing to pass down | Yes, per attempt, using the wrapped Agent's limit | `lib/dispatcher/retry-agent.js:10-26` takes an already-built dispatcher. Its own `options` are only retry options, and `maxResponseSize` on them does nothing. The limit comes from the wrapped Agent. A retry or resume is a new response on a new parser message, so `bytesRead` resets (`client-h1.js:806`). The limit applies to each attempt, not to the total across retries. | +| `ProxyAgent` | Only on the tunnel path | Tunnel path: yes. Plain `http:` forwarding without a tunnel: **no limit** | `lib/dispatcher/proxy-agent.js:200-202` builds the inner `Agent({...opts, factory, ...})`, which carries `maxResponseSize` into `agentFactory(origin, options)` at line 189. That is the tunnel (`https:` or `proxyTunnel`) path. For non-tunnel `http:` targets the factory returns `Http1ProxyWrapper` (lines 180-187). That wrapper calls `factory(proxyUrl, { connect })` or `new Client(proxyUrl, { connect })` (`proxy-agent.js:49-62`), so `maxResponseSize` is dropped. The SOCKS5 branch (lines 157-167) also passes no size option. I did not change this. | +| `H2CClient` | Yes | Yes | `lib/dispatcher/h2c-client.js:18,41-47` forwards `...opts` to `Client`. `client-h2.js:1141` puts `client[kMaxResponseSize]` into the stream state. `client-h2.js:1455` compares `state.bytesRead + chunk.length` against the limit, and line 1463 accumulates it. The existing h2c tests cover this. | +| `Client` with Milo (`useMilo: true`) | Yes | Yes | `client.js:140,340` stores `kUseMilo`. `client-h1.js:907` selects `H1Parser`. `parser-h1.js:173` copies the limit. `parser-h1.js:932` (`EVENT_DATA`) and `parser-h1.js:433` (`consumeDirectBody`) both compare `this.bytesRead + len` and accumulate at lines 940 and 437. | + +## 3. Cause of the reported bug + +`Parser.onBody` in the llhttp parser (`lib/dispatcher/client-h1.js`, around line 770) compared only the current chunk against the limit: + +```js +if (maxResponseSize > -1 && buf.length > maxResponseSize) { +``` + +`this.bytesRead` was updated on the next line but never included in the comparison. A body arriving in one chunk bigger than the limit was rejected. A body arriving in many small chunks never tripped the check, however large the total. The Milo parser (`parser-h1.js:433,932`) and HTTP/2 (`client-h2.js:1455`) already used `bytesRead + len`. + +The fix is `this.bytesRead + buf.length > maxResponseSize`. + +The regression test is `should throw an error if a streamed response exceeds the limit in total` in `test/max-response-size.js`. It sends 20 chunks of 10 bytes through an `Agent({ maxResponseSize: 50 })` using `request()`. It fails without the fix and passes with it. + +The unfixed gap is the `ProxyAgent` non-tunnel `http:` path, described in the table above. 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..48b50ae 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') @@ -197,6 +197,43 @@ describe('max response size', async (t) => { await t.completed }) + test('should throw an error if a streamed response exceeds the limit in total', async (t) => { + t = tspl(t, { plan: 1 }) + + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('request', async (req, res) => { + // 20 chunks of 10 bytes: each chunk is under the limit, the sum is not + for (let i = 0; i < 20 && !res.destroyed; i++) { + res.write('0123456789') + await new Promise((resolve) => setTimeout(resolve, 5)) + } + res.end() + }) + + server.listen(0) + await once(server, 'listening') + + const agent = new Agent({ maxResponseSize: 50 }) + after(() => agent.close()) + + let error = null + try { + const { body } = await request(`http://localhost:${server.address().port}`, { dispatcher: agent }) + await body.text() + } catch (err) { + error = err + } + + t.ok(error instanceof errors.ResponseExceededMaxSizeError) + + await t.completed + }) + test('invalid max response size should throw an error', async (t) => { t = tspl(t, { plan: 2 })