diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..8f3ce46 --- /dev/null +++ b/TRACE.md @@ -0,0 +1,44 @@ +# How `maxResponseSize` reaches the code that enforces it + +## 1. `new Agent({ maxResponseSize })` → `request()`, plain HTTP/1.1, default settings + +1. `lib/dispatcher/agent.js`: the constructor (line 25) leaves `maxResponseSize` in `...options`, + and line 48 stores it in `this[kOptions]`. On the first `dispatch` for an origin (`[kDispatch]`, + line 78), lines 95-97 call `this[kFactory](origin, this[kOptions])`. The default factory + (lines 18-22) builds a `Pool`, or a `Client` when `connections === 1`. +2. `lib/api/api-request.js`: `request()` (line 221) only calls `dispatcher.dispatch(opts, handler)` + (line 233). It never reads the option. It just sends the request into the Agent above. +3. `lib/dispatcher/pool.js`: the constructor (lines 57-70) leaves it in `...options`, and line 101 + copies it into `this[kOptions]`. `createClient()` (line 41) calls + `pool[kFactory](pool[kUrl], pool[kOptions])`, and `defaultFactory` (lines 30-32) does + `new Client(origin, opts)`. Dispatch goes through `lib/dispatcher/pool-base.js:243-250`. +4. `lib/dispatcher/client.js`: destructures `maxResponseSize` (line 130), validates it (lines 215-216), + and stores it as `this[kMaxResponseSize]`, with `-1` meaning unlimited (line 323). After the + socket connects, lines 557-559 pick `connectH1` because a plain `http:` socket has no `h2` + ALPN. +5. `lib/dispatcher/client-h1.js`: `connectH1` builds the parser at lines 907-909. By default this + is the llhttp `Parser`, because `useMilo` is unset and Milo is disabled. The `Parser` + constructor copies `client[kMaxResponseSize]` into `this.maxResponseSize` (line 261). + **Enforcement:** `onBody()` (lines 751-782) runs for every body chunk llhttp produces. Line 770 + checks the limit and destroys the socket with `ResponseExceededMaxSizeError`. `this.bytesRead` + holds the running total; it is incremented at line 775 and reset per response in + `onMessageComplete` (line 806). + + *Bug (now fixed):* line 770 used to test `buf.length > maxResponseSize`, which compares a + single chunk to the limit instead of the running total. A body that arrives in one read + was rejected, but a bigger body streamed in chunks that were each under the limit was never + rejected. The check is now `this.bytesRead + buf.length > maxResponseSize`, which matches the + Milo and HTTP/2 paths. + +## 2. Other ways of setting it + +These results were checked against the code and also by running each setup against a +streaming server. + +| Setup | Passed down? | Enforced on the whole body? | Where | +|---|---|---|---| +| `new BalancedPool(upstreams, { maxResponseSize })` | Yes | Yes (after the fix; it uses the same llhttp path as §1) | `lib/dispatcher/balanced-pool.js:52` keeps it in `...opts`, `:62` stores it in `this[kOptions]`, `:91` calls `this[kFactory](upstreamOrigin, this[kOptions])`, and `:47-49` `defaultFactory` does `new Pool(origin, opts)`. From there the path is §1 steps 3-5: `pool.js:101,41,30-32` → `client.js:323` → `client-h1.js:261,770` | +| `new RetryAgent(new Agent({ maxResponseSize }))` | Yes, through the wrapped Agent | Yes (after the fix) | `lib/dispatcher/retry-agent.js:10-15` keeps the wrapped agent, and `:18-27` forwards every dispatch to `this.#agent.dispatch`, so the path is §1 unchanged. Each retry is a new response, so each attempt gets its own budget (`client-h1.js:806` resets `bytesRead`). **But** `new RetryAgent(agent, { maxResponseSize })` does nothing: those options are only used as `retryOptions` for `RetryHandler` (`retry-agent.js:13,19-22`) and never reach a Client. | +| `new ProxyAgent({ uri, maxResponseSize })` | **Only when tunnelling** | Tunnel: yes. Forward-proxy: **no** | `lib/dispatcher/proxy-agent.js:200-201` passes `...opts` (which includes `maxResponseSize`) to an inner `Agent`. For tunnelled requests (`https:` targets, or `proxyTunnel: true`; see `shouldProxyTunnel` at `:41-43`), the factory returns `agentFactory(origin, options)` (`:189`), which builds a Pool/Client from those options (`:34-39`), so the limit applies through `client-h1.js:770`. For plain `http:` targets with the default `proxyTunnel`, the factory returns an `Http1ProxyWrapper` (`:170-187`), and that wrapper creates its own `new Client(proxyUrl, { connect })` / `factory(proxyUrl, { connect })` (`:58-62`). That drops `maxResponseSize`, so **no limit applies**. The SOCKS5 branch (`:157-167`) does not forward it either. Not fixed here. | +| `new H2CClient(origin, { maxResponseSize })` | Yes | Yes (this path never had the bug) | `lib/dispatcher/h2c-client.js:18` keeps it in `...opts`, and `:41-47` passes it to `Client` with `useH2c: true`. `client.js:323` stores it. `lib/core/connect.js:135-137` sets `socket.alpnProtocol = 'h2'`, so `client.js:557-558` picks `connectH2`. `lib/dispatcher/client-h2.js:1141` copies it into per-stream state, and `onData` checks the running total at `:1455` (`state.bytesRead + chunk.length > maxResponseSize`), resetting the stream with `ResponseExceededMaxSizeError`. | +| `new Client(origin, { maxResponseSize, useMilo: true })` | Yes | Yes (this path never had the bug) | `client.js:140,340` stores `useMilo`, and `client-h1.js:907-908` then builds `H1Parser` from `lib/dispatcher/parser-h1.js`, which copies the limit at `:173`. Both body paths check the running total: `EVENT_DATA` at `:932` (`this.bytesRead + len > maxResponseSize`) and the direct-body fast path `consumeDirectBody` at `:433`. `bytesRead` resets per message 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..7e4f04f 100644 --- a/test/max-response-size.js +++ b/test/max-response-size.js @@ -1,9 +1,10 @@ 'use strict' const { tspl } = require('@matteo.collina/tspl') +const assert = require('node:assert') 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') @@ -197,6 +198,79 @@ 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 exceeds the limit in total (useMilo: ${useMilo})`, async (t) => { + const chunk = Buffer.alloc(100, 'a') + const chunks = 20 // 2000 bytes in total, each chunk well below the limit + + const server = createServer({ joinDuplicateHeaders: true }) + t.after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('request', async (req, res) => { + res.on('error', () => {}) + res.writeHead(200) + for (let i = 0; i < chunks && !res.destroyed; i++) { + res.write(chunk) + // Let each chunk reach the client in its own read. + await new Promise((resolve) => setTimeout(resolve, 5)) + } + res.end() + }) + + server.listen(0) + await once(server, 'listening') + + const agent = new Agent({ maxResponseSize: 1000, useMilo }) + t.after(() => agent.close()) + + let error = null + let received = 0 + 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 + } + + assert.ok(error instanceof errors.ResponseExceededMaxSizeError, `got ${error} after ${received} bytes`) + assert.ok(received <= 1000, `received ${received} bytes`) + }) + + test(`should count the limit per response on a reused connection (useMilo: ${useMilo})`, async (t) => { + const server = createServer({ joinDuplicateHeaders: true }) + t.after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('request', (req, res) => { + res.end(Buffer.alloc(600, 'a')) + }) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`, { maxResponseSize: 1000, useMilo }) + t.after(() => client.close()) + + let connections = 0 + client.on('connect', () => { + connections++ + }) + + for (let i = 0; i < 2; i++) { + const { body } = await client.request({ path: '/', method: 'GET' }) + assert.strictEqual((await body.arrayBuffer()).byteLength, 600) + } + assert.strictEqual(connections, 1) + }) + } + test('invalid max response size should throw an error', async (t) => { t = tspl(t, { plan: 2 })