diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..63d918a --- /dev/null +++ b/TRACE.md @@ -0,0 +1,24 @@ +# How `maxResponseSize` gets from the caller to the code that enforces it + +Line numbers are from the fixed tree. + +## 1. `new Agent({ maxResponseSize })` → enforcement (plain HTTP/1.1, default settings) + +1. `lib/dispatcher/agent.js`: the constructor takes `maxResponseSize` as part of `...options` (line 25) and stores a copy in `this[kOptions]` (line 48). On the first request to an origin, `[kDispatch]` (line 95) passes `this[kOptions]` to `defaultFactory` (lines 18-22), which builds a `Pool` because `connections` is not 1. +2. `lib/dispatcher/pool.js`: the constructor stores the options in `this[kOptions]` (line 101). `[kGetDispatcher]` calls `createClient` (lines 199/221), which runs `pool[kFactory](pool[kUrl], pool[kOptions])` (line 41). The default factory there is `new Client(origin, opts)` (line 31). +3. `lib/dispatcher/client.js`: the constructor destructures `maxResponseSize` (line 130), checks it (lines 215-216) and saves it as `this[kMaxResponseSize]`, with -1 meaning "no limit" (line 323). When the socket connects, `connectH1(client, socket)` is called (line 559) because there is no `h2` ALPN. +4. `lib/dispatcher/client-h1.js`: `connectH1` (line 885) creates the llhttp `Parser`, since Milo is off by default (lines 907-909). The `Parser` constructor copies `client[kMaxResponseSize]` into `this.maxResponseSize` (line 261). **`onBody` enforces the limit** (line 770): if the bytes counted so far plus the new chunk go over the limit, it destroys the socket with `ResponseExceededMaxSizeError`. `this.bytesRead` grows at line 775 and is reset for each response at line 806. + +(`request()` itself, in `lib/api/api-request.js`, only calls `dispatcher.dispatch()`. It never reads the option.) + +**The bug.** Line 770 used to read `buf.length > maxResponseSize`. That compared each chunk on its own to the limit, so a body sent as many small chunks never failed the check. It now reads `this.bytesRead + buf.length > maxResponseSize`, which is the same check the Milo parser and the HTTP/2 code already use. + +## 2. Other ways to set it + +| Setup | Passed down? | Applies to the whole body? | Evidence | +|---|---|---|---| +| **BalancedPool** | Yes | Yes (HTTP/1.1 had the same chunk bug as Agent; fixed now) | `lib/dispatcher/balanced-pool.js:52` takes it in `...opts`, `:62` stores `this[kOptions]`, `:91` passes them to the factory, `:47-48` default factory is `new Pool(origin, opts)`. From there the path is the same as in section 1 (`pool.js:41` → `client.js:323` → `client-h1.js:261`, `:770`). | +| **RetryAgent wrapping an Agent** | The RetryAgent never sees the option. It lives on the wrapped Agent and applies to each attempt | Per attempt, not across attempts | `lib/dispatcher/retry-agent.js:10-13`: the second argument is retry options only. `:26` sends every attempt to the wrapped agent, which enforces the limit as in section 1. `ResponseExceededMaxSizeError` (`UND_ERR_RES_EXCEEDED_MAX_SIZE`) is not in the default `errorCodes` (`lib/handler/retry-handler.js:135-145`), so going over the limit is not retried. But if the connection drops partway (for example `ECONNRESET`) and the request resumes with a `Range` request, the new 206 response starts counting from 0 (`client-h1.js:806`). The total delivered across attempts can then go above `maxResponseSize`. | +| **ProxyAgent** | **Only when the request is tunnelled** (`https:` target, or `proxyTunnel: true`) | Tunnelled: yes. Forwarded `http:` target with default settings: **no limit at all** | `lib/dispatcher/proxy-agent.js:200-201` does `new Agent({ ...opts, factory })`. For tunnelled origins the factory calls `agentFactory(origin, options)` (`:189`), so the option reaches `Pool`/`Client`. For an `http:` target, `proxyTunnel` defaults to `undefined` (`:121`, `:132`), so `shouldProxyTunnel` (`:41-43`) returns false. The request then goes through `Http1ProxyWrapper` (`:170-186`), which builds its client with `factory(proxyUrl, { connect })` or `new Client(proxyUrl, { connect })` (`:58-62`). That drops `maxResponseSize`, so the client ends up with -1 (`client.js:323`). The SOCKS5 branch (`:157-167`) also leaves it out. **This is not fixed.** | +| **H2CClient** | Yes | Yes | `lib/dispatcher/h2c-client.js:18` keeps it in `...opts`, `:41-46` passes it to `Client` with `useH2c: true`. `client.js:323` stores it. `lib/core/connect.js:135-136` sets `socket.alpnProtocol = 'h2'`, so `client.js:557-558` uses `connectH2`. `lib/dispatcher/client-h2.js:1141` copies it into the per-stream state, and `:1455` checks the running total `state.bytesRead + chunk.length` (summed at `:1463`). Going over resets only that stream. | +| **Client with Milo** (`useMilo: true` or `UNDICI_USE_MILO=1`) | Yes | Yes | `client.js:140`, `:340` store `kUseMilo`. `client-h1.js:907-908` creates `H1Parser`. `lib/dispatcher/parser-h1.js:173` copies the limit. Both body paths check the running total: the content-length fast path `consumeDirectBody` (`:433`, summed at `:437`) and the `EVENT_DATA` path for chunked or other bodies (`:932`, summed at `:940`). `bytesRead` is reset per response 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..7130e19 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,59 @@ describe('max response size', async (t) => { await t.completed }) + for (const useMilo of [false, true]) { + for (const framing of ['chunked', 'content-length']) { + test(`should count the whole body when it arrives in small chunks (useMilo: ${useMilo}, ${framing})`, async (t) => { + t = tspl(t, { plan: 2 }) + + const chunk = Buffer.alloc(64, 'a') + const chunks = 64 // 4096 bytes in total, each write well under the limit + + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('request', async (req, res) => { + res.on('error', () => {}) + if (framing === 'content-length') { + res.setHeader('content-length', chunk.length * chunks) + } + res.flushHeaders() + for (let i = 0; i < chunks && !res.destroyed; i++) { + res.write(chunk) + // Give each write its own TCP read on the client side. + await new Promise((resolve) => setTimeout(resolve, 2)) + } + res.end() + }) + + server.listen(0) + await once(server, 'listening') + + const agent = new Agent({ maxResponseSize: 1024, useMilo }) + after(() => agent.destroy()) + + let received = 0 + let error = null + try { + const { body } = await request(`http://localhost:${server.address().port}`, { dispatcher: agent }) + for await (const buf of body) { + received += buf.length + } + } catch (err) { + error = err + } + + t.ok(error instanceof errors.ResponseExceededMaxSizeError) + t.ok(received <= 1024, `received ${received} bytes`) + + await t.completed + }) + } + } + test('should throw an error if the response is too big over h2c', async (t) => { t = tspl(t, { plan: 1 })