diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..bb928e7 --- /dev/null +++ b/TRACE.md @@ -0,0 +1,100 @@ +# `maxResponseSize` trace + +Paths are from the repo root. Line numbers are for the tree with the fix applied +(the fix adds two lines at `lib/dispatcher/client-h1.js:770`). + +Every "enforced / not enforced" verdict below was checked by running it, not +only by reading: a server streamed a 64 KiB body in 1 KiB chunks against a +16 KiB limit, through each dispatcher. + +## 1. `new Agent({ maxResponseSize })` to enforcement (plain HTTP/1.1, defaults) + +| # | File | What it does with the option | +|---|------|------------------------------| +| 1 | `index.js:108-114`, `index.js:205` | `request()` only picks the dispatcher (`opts.dispatcher`) and calls `api.request` on it; it never sees `maxResponseSize`. | +| 2 | `lib/dispatcher/agent.js:25`, `:48` | The constructor leaves it in `...options` and stores it in `this[kOptions]` (a JSON deep clone). | +| 3 | `lib/dispatcher/agent.js:18-22`, `:95-97` | On the first request to an origin, `this[kFactory](opts.origin, this[kOptions])` builds a `Pool` (or a `Client` when `connections === 1`) with those options. | +| 4 | `lib/dispatcher/pool.js:57-71`, `:101` | The `Pool` constructor leaves it in `...options` and stores it in `this[kOptions]`. | +| 5 | `lib/dispatcher/pool.js:30-32`, `:41` | `createClient` calls `pool[kFactory](pool[kUrl], pool[kOptions])`, i.e. `new Client(origin, opts)`. | +| 6 | `lib/dispatcher/client.js:130`, `:215-217`, `:323` | `Client` validates it (integer, >= -1) and stores it as `this[kMaxResponseSize]` (`-1` = no limit). The symbol is defined in `lib/core/symbols.js:60`. | +| 7 | `lib/dispatcher/client.js:557-559` | After connecting, a non-`h2` socket is handed to `connectH1(client, socket)`. | +| 8 | `lib/dispatcher/client-h1.js:909-911` | `connectH1` picks the parser. By default (`useMilo` unset, `UNDICI_USE_MILO` unset, see `lib/dispatcher/parser-h1.js:70-71`) it is the llhttp `Parser`. | +| 9 | `lib/dispatcher/client-h1.js:261` | The `Parser` constructor copies `client[kMaxResponseSize]` to `this.maxResponseSize`. | +| 10 | `lib/dispatcher/client-h1.js:184-187`, `:751-784` | llhttp calls `wasm_on_body` -> `Parser.onBody(buf)` for each body fragment. `:772` is the check; on violation the socket is destroyed with `ResponseExceededMaxSizeError`. `:777` adds the fragment to `this.bytesRead`, which `:808` resets when the message completes. | + +The reported bug was in step 10. See section 3. + +## 2. Other ways to set it + +| Configuration | Passed down? | Enforced on the whole body? | +|---|---|---| +| `BalancedPool` | Yes | Yes (after the fix) | +| `RetryAgent` wrapping an `Agent`, limit on the `Agent` | Yes | Per attempt only. Not on a body that is resumed after a connection failure | +| `RetryAgent` wrapping an `Agent`, limit in the `RetryAgent` options | No | No | +| `ProxyAgent`, tunnelled (any `https:` target, or `proxyTunnel: true`) | Yes | Yes (after the fix) | +| `ProxyAgent`, plain `http:` target (default, no tunnel) | **No** | **No** | +| `H2CClient` | Yes | Yes | +| `Client` with Milo (`useMilo: true` or `UNDICI_USE_MILO=1`) | Yes | Yes | + +"After the fix" means the path ends in the llhttp parser that had the bug, so it +was also broken before this change. + +### BalancedPool - passed down, enforced + +- `lib/dispatcher/balanced-pool.js:52`, `:62`: the option stays in `...opts` and is stored in `this[kOptions]`. +- `lib/dispatcher/balanced-pool.js:47-49`, `:91`: each upstream is `new Pool(upstreamOrigin, this[kOptions])`. +- From there it is steps 4-10 above (`lib/dispatcher/pool.js:41`, `lib/dispatcher/client.js:323`, `lib/dispatcher/client-h1.js:772`). + +### RetryAgent wrapping an Agent - depends on where it is set and on resumes + +- Limit on the wrapped `Agent`: `lib/dispatcher/retry-agent.js:26` dispatches on the wrapped agent, so the limit travels as in section 1 and is enforced for each attempt. The error is not retried: its code `UND_ERR_RES_EXCEEDED_MAX_SIZE` is not in the default retryable codes (`lib/handler/retry-handler.js:135-145`, checked at `:326`). +- **Gap, resumed bodies.** If an attempt dies mid-body with a retryable error (for example `UND_ERR_SOCKET`), `lib/handler/retry-handler.js:556-566` sends a new request with `Range: bytes=-`, and `:522-524` keeps appending to the same downstream body. Each attempt is a new HTTP message, so the parser's count starts again from zero (`lib/dispatcher/client-h1.js:808`, or a new parser on a new socket, `:255`). The limit therefore applies to each attempt, not to the body the caller receives. Confirmed: a 64 KiB body delivered as 12 KiB per attempt passed a 16 KiB limit in full. +- **Gap, limit given to the RetryAgent itself.** `new RetryAgent(agent, { maxResponseSize })` does nothing: `lib/dispatcher/retry-agent.js:10-13` keeps the options only as `retryOptions` (`:19-22`), and `lib/handler/retry-handler.js:103-116` does not read `maxResponseSize`. Nothing forwards it to the wrapped agent. Confirmed: 64 KiB passed a 16 KiB limit. + +### ProxyAgent - enforced when tunnelling, dropped for plain `http:` targets + +- `lib/dispatcher/proxy-agent.js:200-202`: the inner `Agent` gets `...opts`, so it holds `maxResponseSize` and passes it to the `factory` at `:153`. +- Tunnelled (`https:` target or `proxyTunnel: true`, decided at `:41-43`, `:170`): `:189` returns `agentFactory(origin, options)`, which is `new Pool`/`new Client` with the options (`:34-39`). Then steps 4-10 above (or `lib/dispatcher/client-h2.js:1455` if the target negotiates h2). Enforced. +- **Gap, plain `http:` target without a tunnel (the default for `http:` URLs):** `:180-187` builds an `Http1ProxyWrapper` and does not pass `options` to it. The wrapper creates its client with `{ connect }` only (`:58-62`), so that `Client` has `kMaxResponseSize = -1` (`lib/dispatcher/client.js:323`). Confirmed: 64 KiB passed a 16 KiB limit. +- Same pattern, by reading only (not run): a SOCKS5 proxy. `lib/dispatcher/proxy-agent.js:157-167` does not pass `options`, and `lib/dispatcher/socks5-proxy-agent.js:235-237` builds its `Pool` with only `pipelining`, `connections` and `connect`. + +### H2CClient - passed down, enforced + +- `lib/dispatcher/h2c-client.js:18-19`, `:41-47`: the option stays in `...opts` and goes to `Client` via `super(origin, { ...opts, ... })`. +- `lib/dispatcher/client.js:323`: stored as `kMaxResponseSize`. `:557-558`: the socket goes to `connectH2`. +- `lib/dispatcher/client-h2.js:1137`, `:1141`: each request gets its own state with `bytesRead: 0` and `maxResponseSize`. +- `lib/dispatcher/client-h2.js:1455-1463`: `onData` checks `state.bytesRead + chunk.length > maxResponseSize`, resets only that stream, then adds to `bytesRead`. This is a running total, so the whole body is covered. + +### Client with the Milo HTTP/1 parser - passed down, enforced + +- `lib/dispatcher/client.js:140`, `:340`: `useMilo` is stored as `kUseMilo`. `lib/dispatcher/client-h1.js:909-910` then creates `H1Parser` instead of the llhttp `Parser` (confirmed at runtime: the socket's parser was `H1Parser`). +- `lib/dispatcher/parser-h1.js:173`: copies `client[kMaxResponseSize]`. +- `lib/dispatcher/parser-h1.js:932-940`: the `EVENT_DATA` path (chunked bodies and others) checks `this.bytesRead + len > maxResponseSize`, then adds to `bytesRead`. +- `lib/dispatcher/parser-h1.js:433-437`: the direct `Content-Length` body path (`consumeDirectBody`) does the same. +- `lib/dispatcher/parser-h1.js:648`: `bytesRead` resets per message. Both paths use a running total, so Milo never had the reported bug. Confirmed for a chunked body and a `Content-Length` body. + +## 3. The reported bug + +Cause: `lib/dispatcher/client-h1.js`, `Parser.onBody`. The check was + +```js +if (maxResponseSize > -1 && buf.length > maxResponseSize) { +``` + +It compared the size of the current fragment with the limit, not the running +total. A body that arrived as one fragment larger than the limit was rejected; +a body of any size sent in fragments each smaller than the limit passed. + +Fix (`lib/dispatcher/client-h1.js:772`): compare `this.bytesRead + buf.length`, +the same form the Milo and HTTP/2 paths already use. + +This one check sits at the end of the `Agent`, `Pool`, `Client`, `BalancedPool` +and tunnelled `ProxyAgent` paths for HTTP/1.1, so the fix covers all of them. + +Regression tests: `test/max-response-size.js` ("body streamed in small chunks", +chunked and `Content-Length` variants through `new Agent({ maxResponseSize })` +and `request()`, plus a body of exactly the limit that must still pass). + +Not fixed here (separate defects, listed in section 2): `ProxyAgent` dropping the +option for plain `http:` targets and SOCKS5, and `RetryAgent` (option ignored +when given to it; limit applied per attempt on resumed bodies). diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js index 295facf..b071582 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -767,7 +767,9 @@ class Parser { assert(statusCode >= 200) - if (maxResponseSize > -1 && buf.length > maxResponseSize) { + // The limit applies to the whole body, not to a single chunk: count what + // was already delivered for this response (bytesRead resets per message). + 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..76d95bc 100644 --- a/test/max-response-size.js +++ b/test/max-response-size.js @@ -1,9 +1,10 @@ 'use strict' +const assert = require('node:assert') 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 +119,101 @@ describe('max response size', async (t) => { await t.completed }) + // The limit covers the whole body: a response delivered in many chunks that + // are each below the limit must still be rejected once their sum exceeds it. + for (const [name, headers] of [ + ['chunked', {}], + ['content-length', { 'content-length': 64 * 1024 }] + ]) { + test(`should throw an error if a body streamed in small chunks is too big (Agent, ${name})`, async (t) => { + const chunk = Buffer.alloc(1024, 'a') + const chunks = 64 + const maxResponseSize = 16 * 1024 + + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('request', (req, res) => { + res.writeHead(200, headers) + let sent = 0 + const write = () => { + if (res.destroyed) { + return + } + if (sent++ === chunks) { + res.end() + return + } + res.write(chunk) + setTimeout(write, 1) + } + write() + }) + + server.listen(0) + await once(server, 'listening') + + const agent = new Agent({ maxResponseSize }) + after(() => agent.destroy()) + + const { statusCode, body } = await request(`http://localhost:${server.address().port}`, { + dispatcher: agent + }) + assert.strictEqual(statusCode, 200) + + let received = 0 + let error = null + try { + for await (const buf of body) { + received += buf.length + } + } catch (err) { + error = err + } + + assert.ok( + error instanceof errors.ResponseExceededMaxSizeError, + `expected ResponseExceededMaxSizeError, got ${error} after ${received} bytes with a limit of ${maxResponseSize}` + ) + assert.ok(received <= maxResponseSize, `received ${received} bytes with a limit of ${maxResponseSize}`) + }) + } + + test('should allow a body streamed in small chunks that is exactly the limit', async (t) => { + const server = createServer({ joinDuplicateHeaders: true }) + after(() => { + server.closeAllConnections?.() + server.close() + }) + + server.on('request', (req, res) => { + let sent = 0 + const write = () => { + if (sent++ === 8) { + res.end() + return + } + res.write(Buffer.alloc(1024, 'a')) + setTimeout(write, 1) + } + write() + }) + + server.listen(0) + await once(server, 'listening') + + const agent = new Agent({ maxResponseSize: 8 * 1024 }) + after(() => agent.destroy()) + + const { body } = await request(`http://localhost:${server.address().port}`, { + dispatcher: agent + }) + assert.strictEqual((await body.arrayBuffer()).byteLength, 8 * 1024) + }) + test('should throw an error if the response is too big over h2c', async (t) => { t = tspl(t, { plan: 1 })