diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..6ed4f12 --- /dev/null +++ b/TRACE.md @@ -0,0 +1,27 @@ +# Trace of `maxResponseSize` + +## 1. `new Agent({ maxResponseSize })` to enforcement (HTTP/1.1, defaults) + +1. `lib/dispatcher/agent.js:25,48` – the constructor puts `maxResponseSize` into `...options` and stores it in `this[kOptions]`. Agent validates nothing about it. +2. `lib/dispatcher/agent.js:95-97` – on first dispatch to an origin, `defaultFactory` (`:18-22`) is called with `this[kOptions]`. It creates a `Pool`, or a `Client` if `connections === 1`. +3. `lib/dispatcher/pool.js:70,101` – Pool keeps the option in `...options` and in `this[kOptions]`. `pool.js:30-31,41` then calls `new Client(origin, pool[kOptions])` for each connection. +4. `lib/dispatcher/client.js:130,215-216,323` – Client destructures the option, validates it (integer, >= -1), and stores it as `this[kMaxResponseSize]` (`-1` means unlimited). The symbol is defined in `lib/core/symbols.js:60`. +5. `lib/dispatcher/client-h1.js:907-909` – on connect, `connectH1` picks the parser. This is llhttp's `Parser` unless `client[kUseMilo] ?? getMiloEnabled()` is true. +6. `lib/dispatcher/client-h1.js:261` – the llhttp `Parser` constructor copies `client[kMaxResponseSize]` to `this.maxResponseSize`. +7. `lib/dispatcher/client-h1.js:752-777` – `Parser.onBody` enforces the limit. It destroys the socket with `ResponseExceededMaxSizeError`, otherwise adds to `this.bytesRead` and calls `request.onResponseData`. `bytesRead` is reset only at message complete (`:806`). + +### Cause of the bug + +The check at `client-h1.js:770` was `buf.length > maxResponseSize`. That compares one chunk against the limit, not the running total. Many small chunks never trip it. The other three enforcement points (`parser-h1.js:433` (`consumeDirectBody`) and `:932` (`EVENT_DATA`), `client-h2.js:1455`) already used `bytesRead + len`. The fix is `this.bytesRead + buf.length > maxResponseSize`. Regression tests are in `test/max-response-size.js`. + +## 2. Other ways to set it + +| Setup | Passed down? | Enforced on whole body? | Evidence | +|---|---|---|---| +| `BalancedPool` | Yes | Yes (after the fix) | `balanced-pool.js:52,62` keep `...opts` in `kOptions`. `:91` passes them to `new Pool(...)`, which follows the chain in section 1 (`pool.js:41`). | +| `RetryAgent(agent)` | N/A. The option belongs on the wrapped agent. RetryAgent's own `options` are retry options and are not forwarded as client options. | Yes, on the wrapped agent | `retry-agent.js:10-26`: `dispatch` calls `this.#agent.dispatch(opts, retry)`. The wrapped Agent, Pool or Client enforces it as in section 1. A `maxResponseSize` passed to `RetryAgent` itself does nothing. Each retry attempt is a new response, so I would expect the limit to apply per attempt, not across attempts. I did not test that. | +| `ProxyAgent`, CONNECT tunnel (https, or `proxyTunnel: true`) | Yes | Yes | `proxy-agent.js:200-201` spreads `...opts` into the inner `Agent`. `:189` calls `agentFactory(origin, options)` with those options. Section 1 then applies. | +| `ProxyAgent`, plain `http:` origin without tunnel (`Http1ProxyWrapper`) | **No, dropped** | **No** | `proxy-agent.js:170-181` builds `Http1ProxyWrapper` without the option. Its constructor (`:47-62`) calls `factory(proxyUrl, { connect })` or `new Client(proxyUrl, { connect })`, so the Client gets the default `-1`. | +| `ProxyAgent` via `socks5:`/`socks:` | **No, dropped** | **No** | `proxy-agent.js:158-167` does not pass it to `Socks5ProxyAgent`. `socks5-proxy-agent.js:235-237` builds each `Pool` with only `pipelining`, `connections` and `connect`. | +| `H2CClient` | Yes | Yes | `h2c-client.js:18,41-47` spread `...opts` into `super(origin, {...})`, so `Client` stores it (`client.js:323`). HTTP/2 enforcement: `client-h2.js:1141` copies it into stream state, and `:1455` checks `state.bytesRead + chunk.length`. It aborts the stream and leaves the session usable (tested in `test/max-response-size.js`). | +| `Client` with `useMilo: true` | Yes | Yes | `client-h1.js:907-908` selects `H1Parser`. `parser-h1.js:173` copies the limit. `:433` (`consumeDirectBody`) and `:932` (`EVENT_DATA`) both check `this.bytesRead + len`. `bytesRead` is reset per 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..21c47dd 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,7 +118,42 @@ describe('max response size', async (t) => { await t.completed }) - test('should throw an error if the response is too big over h2c', async (t) => { + for (const useMilo of [false, true]) { + test(`should enforce the limit across many small chunks (useMilo: ${useMilo})`, async (t) => { + t = tspl(t, { plan: 2 }) + + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('request', (req, res) => { + res.write('x'.repeat(10)) + const timer = setInterval(() => { + if (res.destroyed || res.writableEnded) { + clearInterval(timer) + return + } + res.write('x'.repeat(10)) + }, 5) + res.on('close', () => clearInterval(timer)) + }) + + await once(server.listen(0), 'listening') + + const agent = new Agent({ maxResponseSize: 50, useMilo }) + after(() => agent.close()) + + const { body } = await request(`http://localhost:${server.address().port}`, { dispatcher: agent }) + await t.rejects(body.text(), (err) => { + t.ok(err instanceof errors.ResponseExceededMaxSizeError) + return true + }) + }) + } + + test('should throw an error if the response is too big over h2c',async (t) => { t = tspl(t, { plan: 1 }) const server = createH2CServer()