diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..1947323 --- /dev/null +++ b/TRACE.md @@ -0,0 +1,59 @@ +# `maxResponseSize`: where it goes and where it is enforced + +Paths are from the repo root. Line numbers are for the tree with the fix in +`lib/dispatcher/client-h1.js` applied. + +## 1. `new Agent({ maxResponseSize })` to enforcement (plain HTTP/1.1, defaults) + +The option is never looked at per request. It is copied down through +constructors when the Agent first creates a dispatcher for an origin, and ends +up as a field on the socket's parser. + +| # | File | What it does with the option | +|---|------|------------------------------| +| 1 | `lib/dispatcher/agent.js:25,48` | The constructor collects it in `...options` and stores a clone in `this[kOptions]`. No validation here. | +| 2 | `lib/dispatcher/agent.js:95-97` (factory at `18-22`) | On the first dispatch to an origin, passes `this[kOptions]` to the factory: `new Pool(origin, opts)`, or `new Client(origin, opts)` when `connections === 1`. | +| 3 | `lib/dispatcher/pool.js:57-71,101` | The Pool constructor leaves it in `...options` and stores it in `this[kOptions]`. | +| 4 | `lib/dispatcher/pool.js:41` (factory at `30-32`) | `createClient` passes `pool[kOptions]` to `new Client(origin, opts)` for each connection it opens. | +| 5 | `lib/dispatcher/client.js:130,215-217,323` | Destructures it, validates it (integer >= -1), and stores it as `this[kMaxResponseSize]` (`-1` means no limit; `null`/`undefined` become `-1`). The symbol is defined in `lib/core/symbols.js:60`. | +| 6 | `lib/dispatcher/client.js:557-559` | After connecting, a socket without ALPN `h2` gets `connectH1(client, socket)`. | +| 7 | `lib/dispatcher/client-h1.js:909-911` | `connectH1` creates the socket's parser. With defaults (`useMilo` unset, `UNDICI_USE_MILO` unset) that is the llhttp `Parser`. | +| 8 | `lib/dispatcher/client-h1.js:261` | The `Parser` constructor copies `client[kMaxResponseSize]` to `this.maxResponseSize`. | +| 9 | `lib/dispatcher/client-h1.js:751-777` | **Enforcement.** `onBody` (called from llhttp's `wasm_on_body`, line `184`) runs once per body chunk; line `772` destroys the socket with `ResponseExceededMaxSizeError` when `bytesRead + buf.length > maxResponseSize`. `bytesRead` is advanced at `777` and reset per response at `808`. | + +The request itself takes the same route at run time, without carrying the +option: `index.js:108-114` (`request()` calls `dispatcher.request`) -> +`lib/api/api-request.js:233` -> `lib/dispatcher/dispatcher-base.js:185` -> +`lib/dispatcher/agent.js:78` -> `lib/dispatcher/pool-base.js:243-250` -> +`lib/dispatcher/client.js:408`. + +## 2. Other ways of setting it + +"Enforced on the whole body" means the check compares the running total for the +response, not one chunk, against the limit. Each row was also checked by +running a 5000-byte body sent as 50 chunks of 100 bytes against a limit of +1000, with both chunked and `content-length` framing (h2c: one framing). + +| Configuration | Passed down? | Enforced on whole body? | Evidence | +|---|---|---|---| +| `new BalancedPool(upstreams, { maxResponseSize })` | Yes | Yes (after the fix; it uses the same llhttp check as section 1) | `lib/dispatcher/balanced-pool.js:52,62` keeps it in `this[kOptions]`; `:91` (factory at `:47-49`) passes that to `new Pool(...)` for each upstream; from there steps 3-9 above. | +| `new RetryAgent(new Agent({ maxResponseSize }))` | Yes, the wrapped Agent keeps its own setting | Yes (after the fix) | `lib/dispatcher/retry-agent.js:26` dispatches on the wrapped agent, so section 1 applies unchanged. The error is not retried: `UND_ERR_RES_EXCEEDED_MAX_SIZE` is not in the default `errorCodes` at `lib/handler/retry-handler.js:135-145` (server saw one request). | +| `new RetryAgent(agent, { maxResponseSize })` | **No** | **No** | `lib/dispatcher/retry-agent.js:10-13,19-22`: the second argument is only used as `retryOptions`; `lib/handler/retry-handler.js:103-116` does not read `maxResponseSize`. Full 5000 bytes received. Set the limit on the wrapped agent instead. | +| `new ProxyAgent({ uri, maxResponseSize })`, tunnelled request (`https:` target, or `proxyTunnel: true`) | Yes | Yes (after the fix for an h1 target; h2 targets use the h2 check below) | `lib/dispatcher/proxy-agent.js:200-202` spreads `opts` into the inner `Agent`; `:189` calls `agentFactory(origin, options)` with the Agent's options (`:34-39` -> `Pool`/`Client`). Verified with `proxyTunnel: true`; the `https:` target case was read, not run. | +| `new ProxyAgent({ uri, maxResponseSize })`, `http:` target with default settings (no tunnel) | **No** | **No** | `lib/dispatcher/proxy-agent.js:42,170` picks the non-tunnel path for `http:` targets; `:180-187` builds `Http1ProxyWrapper` without the Agent's options, and `:59,61` create its client with `{ connect }` only. Full 5000 bytes received. Not changed by this fix. | +| `new ProxyAgent({ uri: 'socks5://...', maxResponseSize })` | **No** (by reading; not run) | **No** | `lib/dispatcher/proxy-agent.js:157-167` builds `Socks5ProxyAgent` without it, and `lib/dispatcher/socks5-proxy-agent.js:235-237` creates its `Pool` with `pipelining`, `connections` and `connect` only. | +| `new H2CClient(origin, { maxResponseSize })` | Yes | Yes (already correct before the fix) | `lib/dispatcher/h2c-client.js:18,41-47` passes `...opts` to `Client`; `lib/dispatcher/client.js:323` stores it; `lib/dispatcher/client-h2.js:1141` copies it into the per-stream state; `:1455-1463` aborts the stream when `state.bytesRead + chunk.length > maxResponseSize`. | +| `new Client(origin, { maxResponseSize, useMilo: true })` | Yes | Yes (already correct before the fix) | `lib/dispatcher/client.js:323,340`; `lib/dispatcher/client-h1.js:909-911` picks `H1Parser`; `lib/dispatcher/parser-h1.js:173` copies the limit; the two body paths both check the running total: `:433-437` (direct `content-length` body) and `:932-940` (parser `EVENT_DATA`); reset per response at `:648`. | + +## 3. Cause of the reported bug + +`lib/dispatcher/client-h1.js`, `Parser.onBody` (llhttp path), compared the +size of the current chunk to the limit: + +```js +if (maxResponseSize > -1 && buf.length > maxResponseSize) { +``` + +so a body was only rejected when a single chunk was larger than the limit. It +now compares the running total (`this.bytesRead + buf.length`), as the Milo and +HTTP/2 paths already did. diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js index 295facf..a30ceef 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -767,7 +767,9 @@ class Parser { assert(statusCode >= 200) - if (maxResponseSize > -1 && buf.length > maxResponseSize) { + // The limit applies to the whole body, not to each chunk: count the bytes + // already delivered for this response as well. + 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..0afb921 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,104 @@ describe('max response size', async (t) => { await t.completed }) + // The limit applies to the whole body. A body that arrives in many chunks, + // each smaller than the limit, must still be rejected once the total passes it. + for (const framing of ['chunked', 'content-length']) { + test(`should throw an error if a ${framing} response streamed in small chunks is too big (Agent)`, async (t) => { + t = tspl(t, { plan: 3 }) + + const chunk = Buffer.alloc(100, 'a') + const chunks = 50 + const maxResponseSize = 1000 + + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('request', (req, res) => { + if (framing === 'content-length') { + res.setHeader('content-length', chunk.length * chunks) + } + res.on('error', () => {}) + res.flushHeaders() + + let sent = 0 + const timer = setInterval(() => { + if (res.destroyed) { + clearInterval(timer) + return + } + res.write(chunk) + if (++sent === chunks) { + clearInterval(timer) + res.end() + } + }, 2) + res.on('close', () => clearInterval(timer)) + }) + + server.listen(0) + await once(server, 'listening') + + const dispatcher = new Agent({ maxResponseSize }) + after(() => dispatcher.destroy()) + + let received = 0 + let error = null + try { + const { body } = await request(`http://localhost:${server.address().port}`, { dispatcher }) + for await (const buf of body) { + received += buf.length + } + } catch (err) { + error = err + } + + t.ok(error instanceof errors.ResponseExceededMaxSizeError) + t.strictEqual(error?.code, 'UND_ERR_RES_EXCEEDED_MAX_SIZE') + t.ok(received <= maxResponseSize, `received ${received} bytes, limit is ${maxResponseSize}`) + + await t.completed + }) + } + + test('max response size is counted per response, not per connection', async (t) => { + t = tspl(t, { plan: 4 }) + + let connections = 0 + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('connection', () => { + connections++ + }) + server.on('request', (req, res) => { + res.write('hel') + setTimeout(() => res.end('lo'), 10) + }) + + server.listen(0) + await once(server, 'listening') + + // Each body is 5 bytes: exactly at the limit, and over it if the count + // were carried from one response to the next on the reused socket. + const client = new Client(`http://localhost:${server.address().port}`, { maxResponseSize: 5 }) + after(() => client.destroy()) + + for (let i = 0; i < 3; i++) { + const { body } = await client.request({ path: '/', method: 'GET' }) + t.strictEqual(await body.text(), 'hello') + } + t.strictEqual(connections, 1) + + await t.completed + }) + test('invalid max response size should throw an error', async (t) => { t = tspl(t, { plan: 2 })