diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..84b96eb --- /dev/null +++ b/TRACE.md @@ -0,0 +1,152 @@ +# `maxResponseSize`: where it travels and where it is enforced + +All paths are relative to the repo root. Line numbers refer to the tree after +the fix in part 3 (the fix adds three comment lines to +`lib/dispatcher/client-h1.js`, so numbers in that file are shifted by +3 +relative to the original fixture). + +## 1. Plain HTTP/1.1 request with default settings + +Scenario: `const agent = new Agent({ maxResponseSize })` used as the +`dispatcher` of `request()`. `request()` (`lib/api/api-request.js`) never +looks at the option; it only calls `dispatcher.dispatch()`. The option itself +flows through the constructors below, in this order. + +1. `lib/dispatcher/agent.js` + The constructor (line 25) destructures only `factory`, `maxOrigins`, + `connect` and `tls`; `maxResponseSize` stays in `...options` and is + deep-cloned into `this[kOptions]` (line 48). On the first dispatch for an + origin, `[kDispatch]` calls `this[kFactory](opts.origin, this[kOptions])` + (lines 95-97). With default settings `connections` is undefined, so + `defaultFactory` (lines 18-22) returns `new Pool(origin, opts)`. + +2. `lib/dispatcher/pool.js` + The constructor (lines 57-71) pulls out the connection-related options and + leaves `maxResponseSize` in `...options`, stored in `this[kOptions]` + (line 101). When a request needs a connection, `createClient` (lines 40-41) + calls `pool[kFactory](pool[kUrl], pool[kOptions])`, whose default + (lines 30-32) is `new Client(origin, opts)`. (`pool-base.js` only forwards + the options to `DispatcherBase` for the WebSocket/EventSource settings and + does not touch `maxResponseSize`.) + +3. `lib/dispatcher/client.js` + The constructor destructures `maxResponseSize` (line 130), validates that + it is an integer >= -1 (lines 215-217) and stores + `this[kMaxResponseSize] = maxResponseSize > -1 ? maxResponseSize : -1` + (line 323). When the socket connects, the client picks the HTTP/1.1 + implementation because `socket.alpnProtocol !== 'h2'` (lines 557-559). + +4. `lib/dispatcher/client-h1.js` (enforcement) + `connectH1` builds the llhttp-backed `Parser` (lines 910-912; Milo is off + by default). The `Parser` constructor copies `client[kMaxResponseSize]` + into `this.maxResponseSize` (line 261) and starts `this.bytesRead` at 0 + (line 255). Each body chunk goes through `onBody` (lines 751-785): line 773 + compares `this.bytesRead + buf.length` with the limit and, when exceeded, + destroys the socket with `ResponseExceededMaxSizeError` and returns -1; + otherwise line 778 adds the chunk to `bytesRead`. `onMessageComplete` + resets `bytesRead` to 0 for the next pipelined response (line 809), so the + limit is per response body. + +## 2. Other ways of setting it + +### BalancedPool: passed down and enforced + +- `lib/dispatcher/balanced-pool.js:52` keeps `maxResponseSize` in `...opts`, + line 62 stores it in `this[kOptions]`, and `addUpstream` (line 91) calls + `this[kFactory](upstreamOrigin, this[kOptions])`, whose default + (lines 47-49) is `new Pool(origin, opts)`. +- From there it is the same path as part 1: `lib/dispatcher/pool.js:101` -> + `lib/dispatcher/client.js:323` -> `lib/dispatcher/client-h1.js:773` + (or the h2 / Milo sites listed below). Enforced on the whole body per + response. + +### RetryAgent wrapping an Agent: enforced only if set on the inner Agent + +- `lib/dispatcher/retry-agent.js:10-16`: the constructor is + `(agent, options)`. `options` are retry options; they are stored at line 13 + and handed to `RetryHandler` as `retryOptions` (line 21). `super(options)` + reaches `lib/dispatcher/dispatcher.js`, a bare `EventEmitter` subclass with + no constructor, so nothing is read from them. `maxResponseSize` passed in + the RetryAgent options is silently ignored (it is also absent from + `types/retry-agent.d.ts`). +- `new RetryAgent(new Agent({ maxResponseSize }))`: `dispatch` delegates to + the wrapped agent (line 26) and retries go through the same agent + (line 23), so the inner Agent's Client enforces it via the part 1 path. +- Caveat on "whole body": the counter lives in the parser and is reset per + HTTP response (`client-h1.js:809`, `parser-h1.js:648`). If `RetryHandler` + resumes a partially consumed body with a `Range` request + (`lib/handler/retry-handler.js:560`), the second response gets a fresh + counter, so the bytes delivered across retries are capped per response, + not in aggregate. `ResponseExceededMaxSizeError` itself + (`UND_ERR_RES_EXCEEDED_MAX_SIZE`) is not in the default retry `errorCodes` + (`retry-handler.js:135-144`), so an exceeded response is not retried. + +### ProxyAgent: depends on the proxy mode + +`lib/dispatcher/proxy-agent.js:200-202` builds the inner +`new Agent({ ...opts, factory, connect })`, so the Agent's `kOptions` +contains `maxResponseSize`. What the custom `factory` (lines 153-190) does +with those options decides whether it reaches a Client: + +- Tunnel mode (`https:` targets, or `proxyTunnel: true`; see + `shouldProxyTunnel`, lines 41-43): line 189 calls + `agentFactory(origin, options)`, default `defaultAgentFactory` + (lines 34-39) -> `new Pool(origin, opts)` / `new Client(origin, opts)`. + Passed down and enforced via part 1. Verified with a local CONNECT proxy + after the fix: a 256-byte streamed body with a 100-byte limit fails with + `ResponseExceededMaxSizeError`. +- Forward mode (the default for `http:` targets): lines 180-187 construct + `Http1ProxyWrapper(this[kProxy].uri, { headers, connect, factory, + proxyServername })` and drop `options`. The wrapper constructor + (lines 49-63) creates `factory(proxyUrl, { connect })` or + `new Client(proxyUrl, { connect })` with only `connect`, so + `kMaxResponseSize` is -1. NOT enforced. Verified after the fix: the same + 256-byte body passes through with a 100-byte limit. +- SOCKS5 proxies (lines 157-168): `Socks5ProxyAgent` builds + `new Pool(origin, { pipelining, connections, connect })` + (`lib/dispatcher/socks5-proxy-agent.js:235-238`), so the limit is not + passed down either (from reading the code; not exercised). +- The CONNECT client created at line 197 (`clientFactory(url, { connect })`) + has no limit, but it only reads the `CONNECT` response headers. + +### H2CClient: passed down and enforced + +- `lib/dispatcher/h2c-client.js:18` keeps `maxResponseSize` in `...opts` and + line 41-47 calls `super(origin, { ...opts, allowH2: true, useH2c: true })`. +- `lib/dispatcher/client.js:323` stores it. With `useH2c`, the connector + marks the socket `alpnProtocol = 'h2'` (`lib/core/connect.js:135-136`), so + `client.js:557-558` selects `connectH2`. +- `lib/dispatcher/client-h2.js:1141` copies `client[kMaxResponseSize]` into + the per-stream `state.maxResponseSize` (with `bytesRead: 0`, line 1137). + `onData` (lines 1436-1468) checks the running total at line 1455 + (`state.bytesRead + chunk.length > maxResponseSize`) and resets only the + offending stream with `ResponseExceededMaxSizeError` (line 1459), keeping + the session alive. Enforced on the whole body. Covered by the existing + h2c tests in `test/max-response-size.js`. + +### Client with the Milo HTTP/1 parser: passed down and enforced + +- `lib/dispatcher/client.js:140` destructures `useMilo` and line 340 stores + it in `kUseMilo`; `maxResponseSize` is stored at line 323 as usual. +- `lib/dispatcher/client-h1.js:910-911`: when `client[kUseMilo]` (or the + `UNDICI_USE_MILO` env flag, `parser-h1.js:71`) is set, `connectH1` creates + `H1Parser` from `lib/dispatcher/parser-h1.js` instead of the llhttp parser. +- `lib/dispatcher/parser-h1.js:173` copies the limit into + `this.maxResponseSize` (line 169 starts `bytesRead` at 0). Both body paths + check the running total: `consumeDirectBody` at line 433 (accumulates at + 437) and the `EVENT_DATA` case at line 932 (accumulates at 940). Both + destroy the socket with `ResponseExceededMaxSizeError`. `bytesRead` is + reset per message at line 648. Enforced on the whole body. The new + `(milo)` regression tests in `test/max-response-size.js` pass both before + and after the fix in part 3. + +## 3. The reported bug + +Only the default llhttp `Parser` in `lib/dispatcher/client-h1.js` was wrong: +`onBody` compared just the current chunk (`buf.length > maxResponseSize`) +against the limit, while every other enforcement site compares +`bytesRead + len`. A body delivered in one chunk larger than the limit was +rejected, but a body streamed in chunks that are each below the limit was +accepted in full. The fix changes the comparison to +`this.bytesRead + buf.length > maxResponseSize` (line 773). Regression tests +for both parsers were added to `test/max-response-size.js`. diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js index 295facf..f303aa7 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -767,7 +767,10 @@ class Parser { assert(statusCode >= 200) - if (maxResponseSize > -1 && buf.length > maxResponseSize) { + // Compare the running total, not just this chunk: a body that arrives in + // many small pieces must be capped the same as one delivered in a single + // chunk. + 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..52af33f 100644 --- a/test/max-response-size.js +++ b/test/max-response-size.js @@ -3,11 +3,103 @@ const { tspl } = require('@matteo.collina/tspl') const { test, after, describe } = require('node:test') const { once } = require('node:events') -const { Client, H2CClient, errors } = require('..') +const { Client, Agent, H2CClient, request, errors } = require('..') const { createServer } = require('node:http') const { createServer: createH2CServer } = require('node:http2') +// Streams `chunks` writes of `chunkSize` bytes each, one per event-loop turn, +// so the client parser sees the body arrive in many small pieces. +function streamBody (res, { chunks, chunkSize }) { + let sent = 0 + const tick = () => { + if (sent++ < chunks) { + res.write(Buffer.alloc(chunkSize, 'x')) + setImmediate(tick) + } else { + res.end() + } + } + tick() +} + describe('max response size', async (t) => { + for (const useMilo of [false, true]) { + const parser = useMilo ? 'milo' : 'llhttp' + + test(`should reject a body streamed in many small chunks whose total exceeds the limit (${parser})`, async (t) => { + t = tspl(t, { plan: 2 }) + + const server = createServer({ joinDuplicateHeaders: true }) + + // 16 chunks of 16 bytes: every chunk is well under the limit, the + // total (256 bytes) is not. + server.on('request', (req, res) => { + res.writeHead(200, { 'content-type': 'application/octet-stream' }) + streamBody(res, { chunks: 16, chunkSize: 16 }) + }) + + server.listen(0) + await once(server, 'listening') + + const agent = new Agent({ maxResponseSize: 100, useMilo }) + + let error = null + let received = 0 + try { + const { body } = await request(`http://localhost:${server.address().port}/`, { dispatcher: agent }) + for await (const chunk of body) { + received += chunk.length + } + } catch (err) { + error = err + } finally { + // Tear down before asserting so a failed assertion cannot leave the + // keep-alive socket and the server holding the process open. + await agent.close() + server.closeAllConnections?.() + server.close() + } + + t.ok(error instanceof errors.ResponseExceededMaxSizeError, `expected ResponseExceededMaxSizeError, got ${error} after ${received} bytes`) + t.ok(received <= 100, `delivered ${received} bytes, more than the 100 byte limit`) + + await t.completed + }) + + test(`should allow a body streamed in many small chunks whose total equals the limit (${parser})`, async (t) => { + t = tspl(t, { plan: 2 }) + + const server = createServer({ joinDuplicateHeaders: true }) + + server.on('request', (req, res) => { + res.writeHead(200, { 'content-type': 'application/octet-stream' }) + streamBody(res, { chunks: 16, chunkSize: 16 }) + }) + + server.listen(0) + await once(server, 'listening') + + const agent = new Agent({ maxResponseSize: 256, useMilo }) + + let statusCode + let length + try { + const res = await request(`http://localhost:${server.address().port}/`, { dispatcher: agent }) + statusCode = res.statusCode + length = (await res.body.arrayBuffer()).byteLength + } finally { + await agent.close() + server.closeAllConnections?.() + server.close() + } + + t.strictEqual(statusCode, 200) + t.strictEqual(length, 256) + + await t.completed + }) + } + test('default max default size should allow all responses', async (t) => { t = tspl(t, { plan: 3 })