diff --git a/TRACE.md b/TRACE.md new file mode 100644 index 0000000..99c913e --- /dev/null +++ b/TRACE.md @@ -0,0 +1,64 @@ +# `maxResponseSize` trace + +Paths are from the repo root. Line numbers are for the tree with the fix in +`lib/dispatcher/client-h1.js` applied (the fix changes one line in place, so +no line numbers moved). + +## 1. `new Agent({ maxResponseSize })` to enforcement (plain HTTP/1.1, defaults) + +| # | File | What it does with the option | +|---|------|------------------------------| +| 1 | `index.js:108`, `index.js:205` | `request()` takes `opts.dispatcher` (the Agent) and calls `api.request` on it; the option is not touched here. | +| 2 | `lib/api/api-request.js:233` | Calls `this.dispatch(opts, handler)` on the Agent; the option is not in the request opts, it lives on the Agent. | +| 3 | `lib/dispatcher/agent.js:25`, `:48` | The constructor collects it in the `...options` rest and stores a JSON deep clone in `this[kOptions]`. (`DispatcherBase`, `lib/dispatcher/dispatcher-base.js:34-38`, receives the same object but only reads `webSocket`/`eventSource`.) | +| 4 | `lib/dispatcher/agent.js:95-97`, `:18-22` | On the first request to an origin, `kDispatch` calls the factory with `this[kOptions]`; the default factory builds `new Pool(origin, opts)` (or `new Client` when `connections === 1`). | +| 5 | `lib/dispatcher/pool.js:58-72`, `:101` | The Pool constructor collects it in its own `...options` rest and deep-clones it into `this[kOptions]`. | +| 6 | `lib/dispatcher/pool-base.js:243-250`, `lib/dispatcher/pool.js:180-199`, `:41`, `:31-33` | Dispatch asks for a client; `createClient` calls the factory with `pool[kOptions]`, and the default factory builds `new Client(origin, opts)`. | +| 7 | `lib/dispatcher/client.js:130`, `:215-217`, `:323` | Destructures `maxResponseSize`, validates it (integer >= -1), and stores it as `this[kMaxResponseSize]` (`-1` = no limit). The symbol is defined in `lib/core/symbols.js:60`. | +| 8 | `lib/dispatcher/client.js:557-559` | After the socket connects without ALPN `h2`, calls `connectH1(client, socket)`. | +| 9 | `lib/dispatcher/client-h1.js:907-909` | `connectH1` picks the parser; with defaults (`useMilo` unset, `UNDICI_USE_MILO` unset) it is the llhttp `Parser`. | +| 10 | `lib/dispatcher/client-h1.js:261` | The `Parser` constructor copies `client[kMaxResponseSize]` to `this.maxResponseSize`. | +| 11 | `lib/dispatcher/client-h1.js:184-188` | The llhttp `wasm_on_body` callback hands each body fragment to `parser.onBody(buf)`. | +| 12 | `lib/dispatcher/client-h1.js:751-782` | **Enforcement.** `onBody` compares against the limit at `:770`, destroys the socket with `ResponseExceededMaxSizeError` at `:771` (class in `lib/core/errors.js:344`), and otherwise adds to the per-response counter `this.bytesRead` at `:775` (reset per response at `:806`). | + +### The bug (step 12) + +`lib/dispatcher/client-h1.js:770` was: + +```js +if (maxResponseSize > -1 && buf.length > maxResponseSize) { +``` + +It compared only the current fragment with the limit, so a body was rejected +only if a single fragment was larger than the whole limit. Any body delivered +in fragments each at or under the limit passed, whatever its total. It now +reads: + +```js +if (maxResponseSize > -1 && this.bytesRead + buf.length > maxResponseSize) { +``` + +which is the same running-total check the Milo parser and the HTTP/2 client +already used. + +## 2. Other ways of setting it + +"Checked" means I ran it: a server streaming 4000 bytes as 40 chunks of 100 +bytes against a limit of 1000, after the fix. Before the fix, every row that +ends in the llhttp parser let the whole chunked body through. + +| How it is set | Passed down? | Enforced on the whole body? | Evidence | +|---------------|--------------|-----------------------------|----------| +| `new BalancedPool(upstreams, { maxResponseSize })` | Yes | Yes (checked) | `lib/dispatcher/balanced-pool.js:52` (`...opts` rest), `:62` (clone into `kOptions`), `:91` and `:48-50` (`new Pool(origin, this[kOptions])`), then steps 5-12 above. | +| `new RetryAgent(new Agent({ maxResponseSize }))` | Yes, it is the inner Agent's own option | Yes for each response (checked); **no across a resumed download** (checked) | `lib/dispatcher/retry-agent.js:18-26` forwards to the wrapped agent, so steps 3-12 apply. The size error is not retried: its code is not in `lib/handler/retry-handler.js:135-145` and is refused at `:326`. But when a retryable error (e.g. a reset connection) interrupts the body, the handler re-requests the rest with a `Range` header (`lib/handler/retry-handler.js:557-560`), and each attempt is a new response with its counter back at 0 (`lib/dispatcher/client-h1.js:806`). 600 + 600 bytes over two attempts got through a limit of 1000. | +| `new RetryAgent(agent, { maxResponseSize })` | **No** | **No** (checked: 4000 bytes got through) | `lib/dispatcher/retry-agent.js:10-13` keeps the second argument only as retry options; `lib/handler/retry-handler.js:103-116` never reads `maxResponseSize`. It has to be set on the wrapped agent. | +| `new ProxyAgent({ uri, maxResponseSize })`, request tunnelled with CONNECT (every `https:` target, or `proxyTunnel: true`) | Yes | Yes (checked with `proxyTunnel: true` and an `http:` target) | `lib/dispatcher/proxy-agent.js:200-202` spreads `...opts` into the inner `Agent`; its factory returns `agentFactory(origin, options)` at `:189`, which builds a `Pool`/`Client` with those options (`:34-39`). Then steps 5-12. | +| `new ProxyAgent({ uri, maxResponseSize })`, `http:` target with defaults (no tunnel) | **No** | **No** (checked: 4000 bytes got through) | `lib/dispatcher/proxy-agent.js:42-44` and `:170` choose the non-tunnel path for `http:` targets; `:180-187` builds `Http1ProxyWrapper` without the `options` argument, and the wrapper creates its client with `{ connect }` only (`:59`, `:61`). The limit never reaches the `Client` that reads the response. | +| `new ProxyAgent({ uri: 'socks5://...', maxResponseSize })` | **No** (by reading the code, not run) | No | `lib/dispatcher/proxy-agent.js:158-167` builds `Socks5ProxyAgent` without it, and `lib/dispatcher/socks5-proxy-agent.js:235-237` builds its `Pool` with only `pipelining`, `connections` and `connect`. | +| `new H2CClient(origin, { maxResponseSize })` | Yes | Yes (checked) | `lib/dispatcher/h2c-client.js:18` and `:41-47` pass `...opts` to `Client`; `lib/dispatcher/client.js:323` stores it; `lib/dispatcher/client-h2.js:1141` copies it into the per-stream state; `:1455-1460` checks `state.bytesRead + chunk.length` and aborts only that stream; `:1463` adds to the counter. | +| `new Client(origin, { maxResponseSize, useMilo: true })` (or `UNDICI_USE_MILO=1`) | Yes | Yes (checked) | `lib/dispatcher/client.js:140`, `:340` store `useMilo`; `lib/dispatcher/client-h1.js:907-908` picks `H1Parser` (env switch at `lib/dispatcher/parser-h1.js:71`); `lib/dispatcher/parser-h1.js:173` copies the limit; both body paths check the running total: `:433-437` (direct content-length body) and `:932-940` (`EVENT_DATA`). | + +### Not fixed here + +The ProxyAgent non-tunnel and SOCKS5 gaps, and the RetryAgent resume case, are +separate from the reported bug and were left as they are. 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..fc6533b 100644 --- a/test/max-response-size.js +++ b/test/max-response-size.js @@ -3,7 +3,8 @@ const { tspl } = require('@matteo.collina/tspl') const { test, after, describe } = require('node:test') const { once } = require('node:events') -const { Client, H2CClient, errors } = require('..') +const { setTimeout: sleep } = require('node:timers/promises') +const { Agent, Client, H2CClient, request, errors } = require('..') const { createServer } = require('node:http') const { createServer: createH2CServer } = require('node:http2') @@ -118,6 +119,55 @@ describe('max response size', async (t) => { await t.completed }) + test('should count the whole body when it arrives in chunks smaller than the limit', async (t) => { + const chunkSize = 100 + const chunks = 20 + const maxResponseSize = 250 + + const server = createServer({ joinDuplicateHeaders: true }) + const dispatcher = new Agent({ maxResponseSize }) + // Test-scoped cleanup, so a failure here cannot leave open handles behind. + t.after(async () => { + await dispatcher.destroy() + server.closeAllConnections?.() + server.close() + }) + + t = tspl(t, { plan: 3 }) + + server.on('request', async (req, res) => { + res.on('error', () => {}) + // No single chunk is over the limit, only their sum is. + res.flushHeaders() + for (let i = 0; i < chunks && !res.destroyed; i++) { + res.write(Buffer.alloc(chunkSize, 'a')) + await sleep(5) + } + res.end() + }) + + server.listen(0) + await once(server, 'listening') + + const { statusCode, body } = await request(`http://localhost:${server.address().port}`, { dispatcher }) + t.strictEqual(statusCode, 200) + + let received = 0 + let error = null + try { + for await (const chunk of body) { + received += chunk.length + } + } catch (err) { + error = err + } + + t.ok(error instanceof errors.ResponseExceededMaxSizeError) + t.ok(received <= maxResponseSize, `received ${received} bytes with a limit of ${maxResponseSize}`) + + await t.completed + }) + test('should throw an error if the response is too big over h2c', async (t) => { t = tspl(t, { plan: 1 })