diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..49553d7 --- /dev/null +++ b/TRACE.md @@ -0,0 +1,24 @@ +# maxResponseSize trace + +## 1. `new Agent({ maxResponseSize })` → enforcement (HTTP/1.1, defaults, llhttp parser) + +1. `lib/dispatcher/agent.js:27` – the constructor puts `maxResponseSize` in `...options`, and line 48 deep-clones it into `this[kOptions]`. Line 95 passes `kOptions` to the factory when a new origin is first dispatched. +2. `lib/dispatcher/agent.js:19-21` – `defaultFactory` returns `new Pool(origin, opts)`. It returns a `Client` only when `connections === 1`. +3. `lib/dispatcher/pool.js:101` – the `Pool` constructor leaves `maxResponseSize` in `...options`, clones it into `this[kOptions]`, and `createClient` (`pool.js:41`) hands it to `new Client(url, opts)`. +4. `lib/dispatcher/client.js:130,215-216,323` – the `Client` constructor validates it (an integer >= -1) and stores it as `this[kMaxResponseSize]`, where -1 means unlimited. +5. `lib/dispatcher/client.js:557-559` – once the socket connects, ALPN picks `connectH2` or `connectH1`. Plain http gets `connectH1`. +6. `lib/dispatcher/client-h1.js:907` – `connectH1` builds `new Parser(...)`. The Milo `H1Parser` is built instead only when `useMilo` is set or `getMiloEnabled()` is true. Milo is off by default. +7. `lib/dispatcher/client-h1.js:261` – the `Parser` constructor copies `client[kMaxResponseSize]` to `this.maxResponseSize`. `bytesRead` starts at 0 on line 255 and resets per message on line 806. +8. `lib/dispatcher/client-h1.js:752,770` – `Parser.onBody(buf)` runs for every body chunk. It destroys the socket with `ResponseExceededMaxSizeError` when the limit is exceeded, otherwise it adds to `this.bytesRead` (line 775). + +Cause of the reported bug: line 770 compared only `buf.length > maxResponseSize`, which is the size of a single chunk. The running total in `bytesRead` was not used. A 1 MB limit therefore rejected one big chunk but let a body of any size through when it arrived in small chunks. The fix is `this.bytesRead + buf.length > maxResponseSize`. The other three enforcement points already use the cumulative form: `parser-h1.js:433,932` and `client-h2.js:1455`. + +## 2. Other ways of setting it + +| Dispatcher | Passed down? | Enforced on whole body? | Evidence | +|---|---|---|---| +| `BalancedPool` | Yes | Yes (after the fix) | `balanced-pool.js:52` puts it in `...opts`, and line 62 stores it in `kOptions`. Line 91 calls `this[kFactory](upstreamOrigin, this[kOptions])`. The default factory is `new Pool(origin, opts)` (line 48), so it then follows the Agent path from step 3. Enforcement is `client-h1.js:770`, or `client-h2.js:1455` over h2. | +| `RetryAgent` | No. The option is not accepted. | Only if the wrapped agent has it. | `retry-agent.js:9-27` does not read `maxResponseSize`. Its options go to `RetryHandler` as `retryOptions`, and `dispatch` just forwards to `this.#agent.dispatch`. Set the limit on the inner `Agent`. It is then enforced per attempt, because `bytesRead` is per message (`client-h1.js:806`) and h2 `state` is per stream (`client-h2.js:1137`). A retry starts counting from 0. | +| `ProxyAgent` | Partly. Tunnelled requests get it. Plain `http://` requests through an http proxy do not. | Tunnel: yes. Non-tunnel: no limit at all. | `proxy-agent.js:200-201` passes `...opts` to the inner `Agent`, so its `kOptions` contains the limit. `proxy-agent.js:189` is `return agentFactory(origin, options)`. That is reached only for tunnelled targets (https, or `proxyTunnel: true`) and does get the limit. For `http:` targets, `proxy-agent.js:180-187` builds an `Http1ProxyWrapper` that receives only `{headers, connect, factory, proxyServername}`. Its client is created with `factory(proxyUrl, { connect })` (`proxy-agent.js:59`), so `maxResponseSize` is dropped. Socks5 is dropped the same way: `proxy-agent.js:158-167` and `socks5-proxy-agent.js:235-238` build the `Pool` with only pipelining, connections and connect. I did not fix this. | +| `H2CClient` | Yes | Yes | `h2c-client.js:30,41-42` spreads `...opts` into `super(origin, ...)`, which is a `Client`. The limit is stored at `client.js:323`, copied to the stream state at `client-h2.js:1141`, and checked cumulatively at `client-h2.js:1455` (`state.bytesRead + chunk.length`). On breach it resets only that stream. Covered by `test/max-response-size.js`. | +| `Client` with `useMilo: true` | Yes | Yes | `client.js:340` stores `kUseMilo`, and `client-h1.js:907` picks `H1Parser`. `parser-h1.js:173` copies the limit. It is enforced cumulatively at `parser-h1.js:433` (`consumeDirectBody`) and `parser-h1.js:932` (`EVENT_DATA`). Milo never had the per-chunk bug. | 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..89018ef 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') @@ -197,6 +197,45 @@ describe('max response size', async (t) => { await t.completed }) + for (const useMilo of [false, true]) { + test(`should throw an error if a streamed response exceeds the limit in total (useMilo: ${useMilo})`, async (t) => { + t = tspl(t, { plan: 1 }) + + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('request', async (req, res) => { + res.writeHead(200) + // 20 chunks of 10 bytes, each below the 50 byte limit + for (let i = 0; i < 20; i++) { + if (!res.write('x'.repeat(10))) await once(res, 'drain') + await new Promise((resolve) => setTimeout(resolve, 5)) + if (res.destroyed) return + } + res.end() + }) + + server.listen(0) + await once(server, 'listening') + + const agent = new Agent({ maxResponseSize: 50, useMilo }) + after(() => agent.close()) + + try { + const { body } = await request(`http://localhost:${server.address().port}`, { dispatcher: agent }) + await body.text() + t.fail('expected ResponseExceededMaxSizeError') + } catch (err) { + t.strictEqual(err.code, new errors.ResponseExceededMaxSizeError().code) + } + + await t.completed + }) + } + test('invalid max response size should throw an error', async (t) => { t = tspl(t, { plan: 2 })