diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..64bd815 --- /dev/null +++ b/TRACE.md @@ -0,0 +1,36 @@ +# maxResponseSize trace + +Findings come from reading the code. Only the Agent/HTTP/1.1 llhttp case (and the fix) was run. + +## 1. `new Agent({ maxResponseSize })` → enforcement (HTTP/1.1, defaults) + +1. `lib/dispatcher/agent.js:25,48`: the constructor puts `maxResponseSize` in `...options` and stores a deep clone in `this[kOptions]`. +2. `lib/dispatcher/agent.js:95`: on the first dispatch to an origin, `this[kFactory](opts.origin, this[kOptions])` is called. `defaultFactory` (`agent.js:19-23`) returns a `Pool` when `connections` is not 1. +3. `lib/dispatcher/pool.js:70,101`: the Pool keeps the option in `...options` and in `this[kOptions]`. +4. `lib/dispatcher/pool.js:40-41`: `createClient()` calls `pool[kFactory](pool[kUrl], pool[kOptions])`, and the default factory (`pool.js:30-31`) builds a `Client`. +5. `lib/dispatcher/client.js:130,215-216,323`: the Client validates the option and stores it as `this[kMaxResponseSize]` (`-1` means unlimited). +6. `lib/dispatcher/client-h1.js:907-909`: when a socket connects, `connectH1` builds the parser. It is `H1Parser` if Milo is on, otherwise llhttp `Parser`. Default is llhttp. +7. `lib/dispatcher/client-h1.js:261`: the llhttp `Parser` copies it from the client, `this.maxResponseSize = client[kMaxResponseSize]`. +8. `lib/dispatcher/client-h1.js:752-777` (`onBody`): the check runs on every body chunk. This is where the bug was (see section 3). After the fix it is `this.bytesRead + buf.length > maxResponseSize`, which destroys the socket with `ResponseExceededMaxSizeError`. `bytesRead` is reset per message at `client-h1.js:806`. + +## 2. Other ways of setting it + +| Setting | Passed down? | Enforced on the whole body? | Evidence | +|---|---|---|---| +| **BalancedPool** | Yes | Yes (llhttp path fixed) | `balanced-pool.js:52,62` keeps it in `opts` / `kOptions`. `balanced-pool.js:91` passes `kOptions` to the factory. The default factory (`balanced-pool.js:47-48`) builds a `Pool`, then it follows the Agent path from `pool.js:41` on. | +| **RetryAgent wrapping an Agent** | The limit lives in the wrapped Agent, so it is not a RetryAgent option | Yes, per attempt | `retry-agent.js:18-26` only wraps the handler and calls `this.#agent.dispatch`. `bytesRead` resets per response, so the cap applies to each attempt, not to the total across retries. Passing `maxResponseSize` to `RetryAgent` itself does nothing. | +| **ProxyAgent**, tunnelled (`https:` target, or `proxyTunnel: true`) | Yes | Yes | `proxy-agent.js:200-201` builds the inner `Agent({...opts, factory})`. `proxy-agent.js:189` returns `agentFactory(origin, options)` with the Agent's `kOptions`, so a Pool/Client gets the limit. | +| **ProxyAgent**, plain `http:` target without `proxyTunnel` (the default) | **No** | **No limit at all** | `proxy-agent.js:170-188` returns `new Http1ProxyWrapper(...)` without `options`. `proxy-agent.js:49,59,61` builds the real client as `factory(proxyUrl, { connect })`, so `maxResponseSize` is dropped. I did not change this. | +| **ProxyAgent**, SOCKS5 | **No** | **No limit** | `proxy-agent.js:157-167` builds `Socks5ProxyAgent` without `options`. | +| **H2CClient** | Yes | Yes | `h2c-client.js:18,41-47` spreads `...opts` into `super()`, which is `Client` (`client.js:323`). `client-h2.js:1141` puts it in the stream state. `client-h2.js:1455` checks `state.bytesRead + chunk.length` against it, and `client-h2.js:1463` accumulates. Covered by the existing h2c tests. | +| **Client with Milo** (`useMilo: true`) | Yes | Yes (never had the bug) | `client.js:140,340` stores `useMilo`. `client-h1.js:907-908` picks `H1Parser`. `parser-h1.js:173` copies the limit. It checks the cumulative `this.bytesRead + len` at `parser-h1.js:433` (direct body, which throws and is turned into a socket destroy) and at `parser-h1.js:932` (`EVENT_DATA`). `bytesRead` is reset at `parser-h1.js:648`. | + +## 3. Cause of the bug + +`lib/dispatcher/client-h1.js` `onBody` (the llhttp parser, used by default) compared only the current chunk's length to the limit: `buf.length > maxResponseSize`. A body split into many chunks, each at or under the limit, was never rejected. A single large chunk was rejected, which matches the report. + +The Milo parser and the H2 path already compared `bytesRead + len`. + +Fix: `this.bytesRead + buf.length > maxResponseSize` (`client-h1.js:770`). + +Regression test: `test/max-response-size.js`, "should enforce the limit on the total of a body streamed in small chunks via Agent". It uses `new Agent({ maxResponseSize: 50 })` and `request()`, and the server writes twenty 10-byte chunks. It fails without the fix and passes with it. 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..e1d1807 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,43 @@ describe('max response size', async (t) => { await t.completed }) + test('should enforce the limit on the total of a body streamed in small chunks via Agent', async (t) => { + t = tspl(t, { plan: 1 }) + + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('request', async (req, res) => { + res.writeHead(200) + for (let i = 0; i < 20; i++) { + if (!res.write('0123456789')) await once(res, 'drain') + 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('should throw an error if the response is too big over h2c', async (t) => { t = tspl(t, { plan: 1 })