diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..1a8f352 --- /dev/null +++ b/TRACE.md @@ -0,0 +1,31 @@ +# How `maxResponseSize` travels to the code that enforces it + +## 1. `new Agent({ maxResponseSize })` → `request()`, plain HTTP/1.1, default settings + +Default settings means no `connections` option, the llhttp parser (Milo off) and an `http:` origin. + +| # | File | What it does with `maxResponseSize` | +|---|------|--------------------------------------| +| 1 | `lib/dispatcher/agent.js` | The constructor (L25) leaves it in `...options`, deep-clones it into `this[kOptions]` (L48), and on the first dispatch to an origin passes `this[kOptions]` to `defaultFactory` (L95). That builds a `Pool` because `connections !== 1` (L18-21). | +| 2 | `lib/dispatcher/pool.js` | The constructor (L57-70) leaves it in `...options` and stores it in `this[kOptions]` (L101). `createClient` (L41) passes those options to `defaultFactory`, which runs `new Client(origin, opts)` (L30-31). | +| 3 | `lib/dispatcher/client.js` | Reads it (L130), checks it is an integer ≥ -1 (L215-216), stores it as `this[kMaxResponseSize]` (L323, where `null`/unset becomes `-1` = unlimited), and on connect calls `connectH1` (L558-559). | +| 4 | `lib/dispatcher/client-h1.js` | `connectH1` creates the llhttp `Parser` (L907-909), whose constructor copies `client[kMaxResponseSize]` to `this.maxResponseSize` (L261). The WASM `wasm_on_body` callback (L184-187) calls `Parser.onBody` for each body chunk. **`onBody` (L770) enforces the limit**: it destroys the socket with `ResponseExceededMaxSizeError`. `bytesRead` is added to at L775 and reset per message at L806. | + +(The call reaches the Agent like this: `request()` → `lib/api/api-request.js` (L233 `this.dispatch`) → `lib/dispatcher/dispatcher-base.js` → `Agent[kDispatch]` (`agent.js` L78). These files only carry the request options; the size option is not involved.) + +**Bug:** before the fix, L770 compared only the current chunk, `buf.length > maxResponseSize`, and ignored the running `bytesRead`. A 1 MB+ body that arrived in one chunk was rejected. A body streamed in many chunks under 1 MB each was never rejected, whatever its total size. It now reads `this.bytesRead + buf.length > maxResponseSize`, the same check the H2 and Milo paths use. + +## 2. Other ways people set it + +| Setup | Passed down? | Enforced on the whole body? | Evidence | +|---|---|---|---| +| **BalancedPool** `new BalancedPool(upstreams, { maxResponseSize })` | Yes | Yes (after this fix; before it, the same per-chunk bug applied) | `lib/dispatcher/balanced-pool.js` L52 keeps it in `...opts`, L62 `this[kOptions]`, L91 `this[kFactory](upstreamOrigin, this[kOptions])`, L47-48 `new Pool(origin, opts)` → then the same path as §1 (`pool.js` → `client.js` → `client-h1.js` L770). | +| **RetryAgent wrapping an Agent** `new RetryAgent(new Agent({ maxResponseSize }))` | Yes, through the wrapped Agent | Yes per attempt (after this fix) | `lib/dispatcher/retry-agent.js` L26 just calls `this.#agent.dispatch(opts, retry)`, so the Agent path in §1 applies. A size error is not retried: `UND_ERR_RES_EXCEEDED_MAX_SIZE` is not in the default `errorCodes` (`lib/handler/retry-handler.js` L135-144). **Caveat:** setting it on the RetryAgent itself (`new RetryAgent(agent, { maxResponseSize })`) does nothing. That second argument is only retry options (`retry-agent.js` L10-13, L21), and `DispatcherBase` reads only `webSocket`/`eventSource` from it (`lib/dispatcher/dispatcher-base.js` L34-37). Checked by running it: 5000 bytes went through a 1024 limit. | +| **ProxyAgent** `new ProxyAgent({ uri, maxResponseSize })` | **Only for tunnelled requests** | Tunnelled: yes (after this fix). Forwarded `http:` targets and SOCKS: **no limit at all** | `lib/dispatcher/proxy-agent.js` L200-201 spreads `...opts` into an inner `Agent`. For tunnelled requests (`https:` targets, or `proxyTunnel: true`, see `shouldProxyTunnel` L41-43), `factory` returns `agentFactory(origin, options)` (L189), so the option reaches `Pool`/`Client`. But for an `http:` target with the default `proxyTunnel`, `factory` returns `new Http1ProxyWrapper(...)` (L180-187) with no size option, and the wrapper builds its client as `factory(proxyUrl, { connect })` / `new Client(proxyUrl, { connect })` (L58-61). That drops `maxResponseSize`. The SOCKS branch (L157-167 → `lib/dispatcher/socks5-proxy-agent.js` L235-238 `new Pool(origin, { pipelining, connections, connect })`) drops it the same way. Checked by running it: a default ProxyAgent to an `http:` origin returned 5000 bytes with a 1024 limit, while `proxyTunnel: true` rejected it. **Not fixed here**; this is a separate bug. | +| **H2CClient** `new H2CClient(origin, { maxResponseSize })` | Yes | Yes (cumulative; this path was already correct) | `lib/dispatcher/h2c-client.js` L18 keeps it in `...opts`, L41-42 `super(origin, { ...opts, ... })` → `client.js` L323 → `lib/dispatcher/client-h2.js` L1141 copies it into the per-stream `state`. L1455 checks `state.bytesRead + chunk.length > maxResponseSize` and resets only that stream; L1463 adds to `bytesRead`. | +| **Client with Milo** `new Client(origin, { maxResponseSize, useMilo: true })` | Yes | Yes (cumulative; this path was already correct) | `client.js` L140/L340 store `kUseMilo`, and `client-h1.js` L907-908 then creates `H1Parser`. `lib/dispatcher/parser-h1.js` L173 copies `client[kMaxResponseSize]`. Both body paths check the running total: the Content-Length fast path `consumeDirectBody` (L433 `this.bytesRead + len > maxResponseSize`, L437 adds) and the `EVENT_DATA` handler (L932, L940 adds). `bytesRead` is reset per message at L648. | + +## 3. Fix and test + +- Fix: `lib/dispatcher/client-h1.js` L770, which now checks `this.bytesRead + buf.length > maxResponseSize`. +- Regression test: `test/max-response-size.js`, test "should count every chunk toward the limit when the body is streamed". It uses `new Agent({ maxResponseSize: 1024 })` with `request()` against a server that writes 10 × 200-byte chunks 5 ms apart, and runs once with llhttp and once with Milo. Without the fix the llhttp case fails: the full 2000 bytes are received and no error is raised. 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..6310756 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,50 @@ describe('max response size', async (t) => { await t.completed }) + for (const useMilo of [false, true]) { + test(`should count every chunk toward the limit when the body is streamed (useMilo: ${useMilo})`, async (t) => { + const server = createServer({ joinDuplicateHeaders: true }) + t.after(() => { + server.closeAllConnections?.() + server.close() + }) + const p = tspl(t, { plan: 2 }) + + // Every chunk is smaller than the limit, but together they exceed it. + const chunk = Buffer.alloc(200, 'a') + server.on('request', async (req, res) => { + res.on('error', () => {}) + for (let i = 0; i < 10 && !res.destroyed; i++) { + res.write(chunk) + await new Promise((resolve) => setTimeout(resolve, 5)) + } + res.end() + }) + + server.listen(0) + await once(server, 'listening') + + const agent = new Agent({ maxResponseSize: 1024, useMilo }) + t.after(() => agent.close()) + + 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 + } + + p.ok(error instanceof errors.ResponseExceededMaxSizeError) + p.ok(received <= 1024, `received ${received} bytes`) + + await p.completed + }) + } + test('should throw an error if the response is too big over h2c', async (t) => { t = tspl(t, { plan: 1 })