diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..f2fcb42 --- /dev/null +++ b/TRACE.md @@ -0,0 +1,115 @@ +# How `maxResponseSize` reaches the code that enforces it + +Line numbers refer to the tree after the fix in `lib/dispatcher/client-h1.js` +(the fix changes one line in place, so the numbers are the same as before). + +## 1. `new Agent({ maxResponseSize })` → enforcement (plain HTTP/1.1, default settings) + +Default settings means: `http://` origin, `connections` not set (so a Pool is +used), `useMilo` not set and `UNDICI_USE_MILO` not set (so llhttp is used). + +1. `lib/dispatcher/agent.js` – the constructor (l.25) collects + `maxResponseSize` into `...options` and stores a copy in `this[kOptions]` + (l.48). On the first dispatch to an origin, `[kDispatch]` (l.78) calls the + factory with those options (l.95-97). `defaultFactory` (l.18-22) builds a + `Pool`, or a `Client` if `connections === 1`. +2. `lib/dispatcher/pool.js` – the constructor keeps the options in + `this[kOptions]` (l.101). `createClient()` (l.40-41) passes them to + `defaultFactory` (l.30-31) → `new Client(origin, opts)`. + `lib/dispatcher/pool-base.js` `[kDispatch]` (l.243-244) picks/creates that + Client through `[kGetDispatcher]`. +3. `lib/dispatcher/client.js` – the constructor destructures `maxResponseSize` + (l.130), checks it (l.215-216) and stores it as + `this[kMaxResponseSize]` (l.323, `-1` = unlimited). Once the socket + connects it calls `connectH1(client, socket)`, because a non-TLS socket has + no `alpnProtocol === 'h2'` (l.557-559). +4. `lib/dispatcher/client-h1.js` – `connectH1` creates the llhttp `Parser` + (l.907-909: Milo is off, so `new Parser(...)`). The `Parser` constructor + copies `client[kMaxResponseSize]` to `this.maxResponseSize` (l.261). The + wasm callback `wasm_on_body` (l.184-187) calls `Parser.onBody(buf)` for each + body segment. **`onBody` enforces the limit** (l.770-773): it destroys the + socket with `ResponseExceededMaxSizeError` and returns -1. The running + count `this.bytesRead` goes up at l.775 and is reset per response in + `onMessageComplete` (l.806). + + The bug: l.770 used to be `buf.length > maxResponseSize`, so each segment + was compared to the limit separately and `bytesRead` was never used. A + body sent in many small segments (a chunked body, or a Content-Length body + arriving over several TCP reads) was never rejected. It now reads + `this.bytesRead + buf.length > maxResponseSize`, the same check the Milo + and HTTP/2 paths use. + +The `request()` side (`lib/api/api-request.js` l.233 → `Dispatcher.dispatch`) +does not touch the option. Only the dispatcher's constructor options matter. + +## 2. Other ways to set it + +| Setup | Passed down? | Enforced on the whole body? | +|---|---|---| +| `BalancedPool` | Yes | Yes (after this fix; HTTP/1 + llhttp had the same bug) | +| `RetryAgent(new Agent({ maxResponseSize }))` | Yes, from the wrapped Agent (not from RetryAgent's own options) | Yes per response (after this fix) | +| `ProxyAgent` | Only for tunnelled requests (`https:` targets, or `proxyTunnel: true`). **Not** for plain `http:` targets forwarded in absolute-form, or through SOCKS proxies | Yes when passed (after this fix); not at all on the forwarded path | +| `H2CClient` | Yes | Yes (was already right) | +| `Client` with `useMilo: true` | Yes | Yes (was already right) | + +### BalancedPool +- `lib/dispatcher/balanced-pool.js` l.52: `...opts` captures it. l.62 stores + `this[kOptions]`. l.91 `this[kFactory](upstreamOrigin, this[kOptions])` uses + `defaultFactory` (l.47-48) → `new Pool(origin, opts)`. +- From there it is the same as steps 2-4 above (`pool.js` l.101/l.41 → + `client.js` l.323 → `client-h1.js` l.261 / l.770). + +### RetryAgent wrapping an Agent +- `lib/dispatcher/retry-agent.js` l.10-13: RetryAgent's own `options` are + **retry options only** (`this.#options` becomes `retryOptions`, l.21). Passing + `maxResponseSize` to RetryAgent does nothing. +- l.23-26: every attempt is dispatched through the wrapped Agent, so the + Agent's `maxResponseSize` applies through the path in part 1. +- The limit applies to each attempt's response (`bytesRead` resets per message, + `client-h1.js` l.806). `ResponseExceededMaxSizeError` + (`UND_ERR_RES_EXCEEDED_MAX_SIZE`) is not in the default retry `errorCodes` + (`lib/handler/retry-handler.js` l.135-145), so an oversized response is not + retried. However, if a response is cut off by a retryable error and resumed + with `Range`, each partial response is checked separately, not the total + delivered to the caller. + +### ProxyAgent +- `lib/dispatcher/proxy-agent.js` l.200-202: `this[kAgent] = new Agent({ ...opts, factory, ... })`. + The inner Agent gets `maxResponseSize`, and calls `factory(origin, options)` + (l.153) with its `kOptions`, so the value is in `options`. +- **Tunnel path** (`https:` target or `proxyTunnel: true`, l.41-43 / l.170): + `return agentFactory(origin, options)` (l.189) → `defaultAgentFactory` + (l.34-39) → `Pool`/`Client` with the options, so it **is enforced** + (`client.js` l.323 → `client-h1.js` l.770, or `client-h2.js` if h2 is + negotiated). +- **Forwarding path** (plain `http:` target, the default): l.180-187 builds + `new Http1ProxyWrapper(proxyUri, { headers, connect, factory, proxyServername })`. + `options` is not passed on, and the wrapper creates its client with only + `{ connect }` (l.58-62). So `maxResponseSize` is **dropped**, `kMaxResponseSize` + becomes `-1`, and the limit is **not enforced at all** for HTTP targets + through a ProxyAgent. +- **SOCKS path** (l.157-167): `new Socks5ProxyAgent(...)` is not given the + options either, so the limit is dropped there too. +- (`clientFactory(url, { connect })` at l.197 is the client used for the + CONNECT request to the proxy. It also gets no `maxResponseSize`, but it only + carries the tunnel handshake.) + +### H2CClient +- `lib/dispatcher/h2c-client.js` l.18 keeps it in `...opts`. l.41-47 + `super(origin, { ...opts, allowH2: true, useH2c: true })` → `Client` + (`client.js` l.323). +- `lib/dispatcher/client-h2.js` l.1141 copies `client[kMaxResponseSize]` into + the per-stream `state`. `onData` checks + `state.bytesRead + chunk.length > maxResponseSize` (l.1455) and resets only + that stream. `bytesRead` goes up at l.1463, so the **whole body** is + counted. Covered by `test/max-response-size.js` ("…over h2c"). + +### Client with the Milo parser (`useMilo: true` or `UNDICI_USE_MILO=1`) +- `lib/dispatcher/client.js` l.140 / l.340 stores `kUseMilo`. l.323 stores + `kMaxResponseSize`. +- `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 count the total: the fast path `consumeDirectBody` (l.433 check, + l.437 count) and the `EVENT_DATA` path (l.932 check, l.940 count). So the + **whole body** is enforced. The new regression test runs with + `useMilo: true` as well as `false`. 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..cc3f230 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,49 @@ describe('max response size', async (t) => { await t.completed }) + for (const useMilo of [false, true]) { + test(`should throw an error if a response streamed in small chunks is too big (useMilo: ${useMilo})`, async (t) => { + t = tspl(t, { plan: 2 }) + + const chunk = Buffer.alloc(1024, 'a') + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('request', async (req, res) => { + res.on('error', () => {}) + // Every chunk is well below the limit; only their sum exceeds it. + for (let i = 0; i < 64 && !res.destroyed; i++) { + if (!res.write(chunk)) { + await once(res, 'drain') + } + } + res.end() + }) + + server.listen(0) + await once(server, 'listening') + + const dispatcher = new Agent({ maxResponseSize: 16 * 1024, useMilo }) + after(() => dispatcher.destroy()) + + let error = null + try { + const { statusCode, body } = await request(`http://localhost:${server.address().port}`, { dispatcher }) + t.strictEqual(statusCode, 200) + await body.arrayBuffer() + } 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 })