diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..9d618ab --- /dev/null +++ b/TRACE.md @@ -0,0 +1,29 @@ +# How `maxResponseSize` travels to the code that enforces it + +Line numbers match the tree after the fix in `lib/dispatcher/client-h1.js`. + +## 1. `new Agent({ maxResponseSize })` + `request()`, plain HTTP/1.1, default settings + +1. `index.js:108,114`: `request()` (built by `makeDispatcher`, `index.js:205`) takes `opts.dispatcher` (the Agent) and calls `api.request` on it. `request()` itself never reads `maxResponseSize`. +2. `lib/api/api-request.js:221,233`: `request()` wraps the call in a `RequestHandler` and calls `this.dispatch(opts, handler)` on the Agent. The handler later receives body chunks (`:163`). +3. `lib/dispatcher/agent.js:25,48`: the constructor collects `maxResponseSize` into `...options` and stores a deep clone in `this[kOptions]`. (`super(options)` at `:38` only reads the webSocket/eventSource options, see `lib/dispatcher/dispatcher-base.js:34-38`.) +4. `lib/dispatcher/agent.js:95-97` → `:18-21`: on the first request to an origin, `kDispatch` calls the factory with `this[kOptions]`. The default factory creates a `Pool`, or a `Client` if `connections === 1`. +5. `lib/dispatcher/pool.js:57-70,101`: the Pool constructor collects `maxResponseSize` into `...options` and stores it in `this[kOptions]`. +6. `lib/dispatcher/pool.js:40` → `:30-31`: `createClient` calls `new Client(origin, pool[kOptions])`. +7. `lib/dispatcher/client.js:130,215-216,323`: the Client destructures `maxResponseSize`, checks it (an integer ≥ -1), and stores it as `this[kMaxResponseSize]` (-1 means no limit). The symbol is defined in `lib/core/symbols.js`. +8. `lib/dispatcher/client.js:557-559`: once the socket connects and ALPN is not `h2`, it calls `connectH1(client, socket)`. +9. `lib/dispatcher/client-h1.js:907-909`: `connectH1` creates the llhttp `Parser`, since Milo is off by default (`lib/dispatcher/parser-h1.js:70-71`, env `UNDICI_USE_MILO`). +10. `lib/dispatcher/client-h1.js:261`: the `Parser` constructor copies `client[kMaxResponseSize]` into `this.maxResponseSize`. +11. `lib/dispatcher/client-h1.js:751-775`: **enforcement**. `onBody` (called from the wasm `wasm_on_body` callback, `:184-187`) destroys the socket with `ResponseExceededMaxSizeError` when the body goes over the limit. It then adds the chunk to `this.bytesRead`, which `onMessageComplete` resets for each response (`:806`). + +**The bug:** line 770 used to be `buf.length > maxResponseSize`, which compares each chunk on its own against the limit. Any body sent in chunks that are each under the limit got through, however large the total. It is now `this.bytesRead + buf.length > maxResponseSize`, so the check covers the whole body. This matches the Milo and HTTP/2 paths. + +## 2. Other ways of setting it + +| Setup | Passed down? | Enforced on the whole body? | Evidence | +|---|---|---|---| +| `new BalancedPool(upstreams, { maxResponseSize })` | Yes | Yes for HTTP/1.1 with llhttp only after this fix (before it, the check was per chunk) | `lib/dispatcher/balanced-pool.js:52,62` stores `...opts` in `kOptions`; `:91` → `:47-48` `new Pool(upstream, kOptions)`; then same as steps 5–11 above | +| `new RetryAgent(new Agent({ maxResponseSize }))` | Yes, through the wrapped Agent | Yes for each attempt (after this fix for llhttp) | `lib/dispatcher/retry-agent.js:18-26` just calls `this.#agent.dispatch(...)`, so the Agent path above applies. `ResponseExceededMaxSizeError` (`UND_ERR_RES_EXCEEDED_MAX_SIZE`) is not in the default `errorCodes` (`lib/handler/retry-handler.js:135-144`, checked at `:326`), so going over the limit is not retried. Caveats: `maxResponseSize` given to the **RetryAgent's own** options does nothing, because those options become `retryOptions` (`retry-agent.js:13,21`). Also, `bytesRead` resets per response, so if a download is resumed with Range after some other retryable error, the total across attempts can go over the limit | +| `new ProxyAgent({ uri, maxResponseSize })` | **It depends on the target and proxy type** | Tunneled requests (https targets, or `proxyTunnel: true`): yes (after this fix). Plain `http:` targets without `proxyTunnel` (the default for `http:`) and SOCKS proxies: **no, the limit is dropped** | `lib/dispatcher/proxy-agent.js:200-201` spreads `...opts` into an internal `Agent`; its factory (`:153`) passes `options` on only in the tunnel branch (`:189` `agentFactory(origin, options)` → `:34-38` Pool/Client). Non-tunneled `http:` (`:41-43`, `:170`) creates `Http1ProxyWrapper`, which builds `new Client(proxyUrl, { connect })` (`:58-61`) without `maxResponseSize`. SOCKS (`:157-167`) builds `Socks5ProxyAgent` without it, and that builds `new Pool(origin, { pipelining, connections, connect })` (`lib/dispatcher/socks5-proxy-agent.js:235-238`) | +| `new H2CClient(origin, { maxResponseSize })` | Yes | Yes (was already counted across the whole body) | `lib/dispatcher/h2c-client.js:18,41-42` spreads `...opts` into `Client` → `lib/dispatcher/client.js:323`; `connectH2` (`client.js:558`) → `lib/dispatcher/client-h2.js:1141` copies it into the stream state; `:1455` checks `state.bytesRead + chunk.length > maxResponseSize`, `:1463` adds to the total | +| `new Client(origin, { maxResponseSize, useMilo: true })` | Yes | Yes (was already counted across the whole body) | `lib/dispatcher/client.js:140,340` stores `kUseMilo`; `lib/dispatcher/client-h1.js:907-908` creates `H1Parser`; `lib/dispatcher/parser-h1.js:173` copies the limit; both body paths count the total: `:433-437` (`consumeDirectBody`) and `:932-940` (`EVENT_DATA`); `:648` resets `bytesRead` for each message | 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..d946780 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,79 @@ describe('max response size', async (t) => { await t.completed }) + for (const useMilo of [false, true]) { + test(`should enforce the limit on the whole body when it arrives in many small chunks (useMilo: ${useMilo})`, async (t) => { + t = tspl(t, { plan: 2 }) + + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + // 20 chunks of 2 bytes each: every chunk is under the limit, the total is not. + server.on('request', async (req, res) => { + res.on('error', () => {}) + for (let i = 0; i < 20 && !res.destroyed; i++) { + res.write('ab') + await new Promise((resolve) => setTimeout(resolve, 5)) + } + res.end() + }) + + server.listen(0) + await once(server, 'listening') + + const agent = new Agent({ maxResponseSize: 10, useMilo }) + after(() => agent.close()) + + const { body } = await request(`http://localhost:${server.address().port}`, { dispatcher: agent }) + let received = 0 + let error = null + try { + for await (const chunk of body) { + received += chunk.length + } + } catch (err) { + error = err + } + + t.ok(error instanceof errors.ResponseExceededMaxSizeError) + t.ok(received <= 10, `received ${received} bytes`) + + await t.completed + }) + } + + test('should allow a streamed response body exactly at the limit', async (t) => { + t = tspl(t, { plan: 1 }) + + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('request', async (req, res) => { + for (let i = 0; i < 5; i++) { + res.write('ab') + await new Promise((resolve) => setTimeout(resolve, 5)) + } + res.end() + }) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`, { maxResponseSize: 10 }) + after(() => client.close()) + + const { body } = await client.request({ path: '/', method: 'GET' }) + t.strictEqual(await body.text(), 'ababababab') + + await t.completed + }) + test('should throw an error if the response is too big over h2c', async (t) => { t = tspl(t, { plan: 1 })