diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..b46436c --- /dev/null +++ b/TRACE.md @@ -0,0 +1,38 @@ +# How `maxResponseSize` travels + +Line numbers refer to the tree with the fix applied. + +## 1. `new Agent({ maxResponseSize })` → enforcement (plain HTTP/1.1, default settings) + +With default settings, `connections` is unset, so the Agent builds a `Pool`. Milo is off unless +`UNDICI_USE_MILO` is set, so the llhttp WASM parser is used. + +| # | File | What it does with `maxResponseSize` | +|---|------|-------------------------------------| +| 1 | `lib/dispatcher/agent.js` | The constructor collects it in `...options` (l.25) and stores it in `this[kOptions]` through `util.deepClone` (l.48). `[kDispatch]` passes `this[kOptions]` to the factory (l.95-97). `defaultFactory` makes a `Pool` because `connections !== 1` (l.18-22). | +| 2 | `lib/core/util.js` | `deepClone` is a JSON round-trip (l.294-296). A finite integer passes through unchanged. | +| 3 | `lib/dispatcher/pool.js` | The constructor collects it in `...options` (l.70) and stores it in `this[kOptions]` (l.101). `createClient` passes `pool[kOptions]` to the factory (l.41), and `defaultFactory` makes `new Client(origin, opts)` (l.30-32). | +| 4 | `lib/dispatcher/client.js` | The constructor destructures it (l.130), validates it as an integer ≥ -1 (l.215-217), and stores it as `this[kMaxResponseSize]`, with -1 meaning "no limit" (l.323). On connect, a non-h2 ALPN result calls `connectH1(client, socket)` (l.557-559). | +| 5 | `lib/dispatcher/client-h1.js` | `connectH1` creates the llhttp `Parser` when Milo is off (l.907-909). The `Parser` constructor copies `client[kMaxResponseSize]` into `this.maxResponseSize` (l.261). The WASM `wasm_on_body` callback (l.184-187) calls `Parser.onBody(buf)` for each body piece. `onBody` enforces the limit (l.770): if it would be exceeded, it destroys the socket with `ResponseExceededMaxSizeError`. `bytesRead` is reset per response in `onMessageComplete` (l.806). | + +The request side: `request()` (`index.js` l.205, `makeDispatcher` l.74-108) picks the +`dispatcher` from the options. It calls `lib/api/api-request.js` `request` (l.221), which calls +`this.dispatch` (l.233). `DispatcherBase.dispatch` then calls `Agent[kDispatch]`. The option +itself never travels with the request. It lives on the `Client` and reaches the parser through +`client[kMaxResponseSize]`. + +**The reported bug** was at `lib/dispatcher/client-h1.js:770`. The check was +`buf.length > maxResponseSize`, so it compared each body piece against the limit and ignored the +running total `this.bytesRead`. A body that arrives in one piece is caught. A body streamed in +many pieces that are each under the limit got through, however big it was. The fixed check is +`this.bytesRead + buf.length > maxResponseSize`, which matches the Milo and HTTP/2 paths. + +## 2. Other ways of setting it + +| Setup | Passed down? | Enforced on the whole body? | Evidence | +|-------|--------------|-----------------------------|----------| +| **BalancedPool** `new BalancedPool(upstreams, { maxResponseSize })` | Yes | Yes, after this fix (llhttp path). Before the fix, only per piece. | `lib/dispatcher/balanced-pool.js` l.52 (`...opts`), l.62 (`this[kOptions] = { ...util.deepClone(opts), … }`), l.91 (`this[kFactory](upstreamOrigin, this[kOptions])`), `defaultFactory` → `new Pool` l.47-49. From there it follows the Pool → Client → `client-h1.js` path in section 1. | +| **RetryAgent wrapping an Agent** `new RetryAgent(new Agent({ maxResponseSize }))` | Yes, through the wrapped Agent | Yes, after this fix | `lib/dispatcher/retry-agent.js` l.18-27: `dispatch` forwards to `this.#agent.dispatch`, so the inner Agent's Clients enforce the limit as in section 1. RetryAgent's own `options` are only retry options (l.13, l.21). `new RetryAgent(agent, { maxResponseSize })` is silently ignored. `ResponseExceededMaxSizeError` (`UND_ERR_RES_EXCEEDED_MAX_SIZE`, `lib/core/errors.js` l.349) is not in the default retryable `errorCodes` (`lib/handler/retry-handler.js` l.135-144), so the error is not retried away. | +| **ProxyAgent** `new ProxyAgent({ uri, maxResponseSize })` | **Only for tunnelled requests** (https targets, or `proxyTunnel: true`) | Tunnelled: yes, after this fix. Plain `http://` targets without `proxyTunnel`, and SOCKS proxies: **no limit at all**. | `lib/dispatcher/proxy-agent.js` l.200-202 spreads `...opts` into the inner `Agent`, and the tunnel case calls `agentFactory(origin, options)` with those options (l.189), giving Pool → Client. For non-tunnelled http targets the factory returns `Http1ProxyWrapper` (l.170-188), which builds its Client with only `{ connect }` (l.59 / l.61), so `maxResponseSize` is dropped. SOCKS: `Socks5ProxyAgent` gets no `maxResponseSize` (l.158-167), and `lib/dispatcher/socks5-proxy-agent.js` l.235-237 creates `new Pool(origin, { pipelining, connections, connect })`. | +| **H2CClient** `new H2CClient(origin, { maxResponseSize })` | Yes | Yes (cumulative) | `lib/dispatcher/h2c-client.js` l.18 (`...opts`) and l.41-47 (`super(origin, { ...opts, … })`). `lib/dispatcher/client.js` l.323 stores it. `lib/dispatcher/client-h2.js` l.1141 copies `client[kMaxResponseSize]` into the per-stream state. l.1455 checks `state.bytesRead + chunk.length > maxResponseSize` and resets only that stream, and l.1463 adds to `bytesRead`. | +| **Client with Milo** `new Client(origin, { maxResponseSize, useMilo: true })` (or `UNDICI_USE_MILO=1`) | Yes | Yes (cumulative), and it was already correct before this fix | `lib/dispatcher/client.js` l.340 stores `kUseMilo`. `lib/dispatcher/client-h1.js` l.907-908 picks `new H1Parser(client, socket)`. `lib/dispatcher/parser-h1.js` l.173 copies `client[kMaxResponseSize]`. Both body paths check the running total: `EVENT_DATA` at l.932 (`this.bytesRead + len > maxResponseSize`) and the direct-body fast path `consumeDirectBody` at l.433. `bytesRead` is reset per response at l.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..bdc8f7c 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,87 @@ describe('max response size', async (t) => { await t.completed }) + for (const useMilo of [false, true]) { + const parser = useMilo ? 'milo' : 'llhttp' + + // Each write becomes its own chunk of a chunked response, so the parser + // hands the body over in many pieces that are each below the limit. + async function streamChunks (res, chunks, size) { + res.writeHead(200, { 'content-type': 'application/octet-stream' }) + for (let i = 0; i < chunks; i++) { + if (!res.write(Buffer.alloc(size, 'a'))) { + await once(res, 'drain') + } + await new Promise((resolve) => setImmediate(resolve)) + } + res.end() + } + + test(`should throw an error if a streamed response exceeds the limit in total (${parser})`, async (t) => { + t = tspl(t, { plan: 2 }) + + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + res.on('error', () => {}) + streamChunks(res, 64, 64).catch(() => {}) + }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.listen(0) + await once(server, 'listening') + + const agent = new Agent({ maxResponseSize: 1024, useMilo }) + after(() => agent.destroy()) + + let error = null + let received = 0 + 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 <= 1024, `received ${received} bytes`) + + await t.completed + }) + + test(`should allow a streamed response exactly at the limit (${parser})`, async (t) => { + t = tspl(t, { plan: 1 }) + + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + streamChunks(res, 16, 64).catch(() => {}) + }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.listen(0) + await once(server, 'listening') + + const agent = new Agent({ maxResponseSize: 1024, useMilo }) + after(() => agent.close()) + + const origin = `http://localhost:${server.address().port}` + // Two responses on the same connection: the count must reset per response. + let total = 0 + for (let i = 0; i < 2; i++) { + const { body } = await request(origin, { dispatcher: agent }) + total += (await body.arrayBuffer()).byteLength + } + t.strictEqual(total, 2048) + + await t.completed + }) + } + test('should throw an error if the response is too big over h2c', async (t) => { t = tspl(t, { plan: 1 })