Apply the response head size cap to the parsed head, not only to short reads

Closed
RO
1mo ago
+214-10

Problem

  • fetch() caps the response head at 1 MiB (ResponseHeadersTooLarge), only in the picohttp ShortRead arm of handle_on_data_headers (src/http/lib.rs:3738). A read that crosses 1 MiB and delivers the blank line skips the check.
  • new WebSocket() has the same gap in buffer_and_parse_head (src/http_jsc/websocket_client/WebSocketUpgradeClient.rs:723) for its 16 KB cap. The cap is also on raw bytes: Node opens a 16400 byte 101 head that Bun rejects when a read ends past 16384.

Fix

  • fetch: after a successful parse, reject when parsed.bytes_read exceeds the cap. WebSocket: both arms compare the head with max_http_header_size() + 128 * 4 + 64, the raw bound bun-uws uses for request heads. An over-size head fails with Response headers exceed --max-http-header-size.
  • Behavior change: a 101 head, a proxy CONNECT reply, or a tunnelled 101 head over the bound now always fails. Before, it opened when no read ended inside it. Node rejects such a head with HPE_HEADER_OVERFLOW.
  • Verified: test/js/web/fetch/fetch.test.ts (3 cases), test/js/web/websocket/websocket-client-short-read.test.ts (4 new, 1 updated). Stock bun fails 2 and 4 of them.
  • Self-reviewed: 4 concerns raised, 3 addressed. Open: a maintainer decision on the behavior change.

Background

  • picohttp parses the head from the accumulated bytes. ShortRead means no blank line yet, so the client buffers and parses again on the next read.
  • on_data delivers what recv() returned, up to 512 KiB. The peer's writes and timing decide where a read ends.
  • max_http_header_size is --max-http-header-size. Node applies it to the bytes llhttp reports (status text, field names, values), not to framing (": ", "\r\n").

Replaces #41569. Refs #20488.

Notes

What changes for real traffic. The fetch() half only affects heads over 1 MiB. The WebSocket half is the one with reach. On main, a head over the cap opens whenever no read ends inside the incomplete head with more than 16384 bytes buffered. Examples: a head of up to 512 KiB that arrives in one read, and a 21000 byte head whose first read is 16000 bytes. With this PR every head over max_http_header_size() + 576 fails, at any segmentation. A head at or under that bound opens at any segmentation. On main a 16400 byte head fails when a read ends past 16384, so the window between 16384 and the bound is now more lenient than main. buffer_and_parse_head has three callers, so the bound also applies to the proxy CONNECT reply and to the 101 head inside a TLS tunnel. --max-http-header-size raises the bound. With --max-http-header-size=32768 a 20000 byte head opens.

Node comparison (v26.3.0, http.request upgrade, one write from a raw TCP server, same head layout as the tests): a head of 16416 raw bytes or less opens. 16417 and above fail with HPE_HEADER_OVERFLOW. Node's TrackHeader adds the lengths of the status text, the field names and the values, and fails at 16384. For this head the framing is 33 bytes, which gives the 16417 threshold. This branch opens up to 16960 raw bytes and rejects 16961 and above. So it is never stricter than Node for up to 128 fields, and at most 576 bytes more lenient.

Why a raw bound with slack. packages/bun-uws/src/HttpParser.h does the same for request heads: MAX_HEADER_FRAMING_SLACK = UWS_HTTP_MAX_HEADERS_COUNT * 4 + 64, with the comment that raw bounds get the framing as slack "so we don't reject requests Node accepts". The result stays a function of the raw head length alone, which keeps it independent of where reads end. If the field limit of this client grows (#43243 proposes 2000), the slack must follow it.

History. #31175 added the cap to the accumulator, before the parse, once buffering had begun. #32394 moved it into the ShortRead arm so that frames pipelined after the head are not counted. That left a complete head unchecked. This PR checks head_len, so pipelined frames are still not counted.

Intentionally not changed. handle_proxy_response rejects a CONNECT reply early when the first bytes are not HTTP/1.1 200 or HTTP/1.0 200 . That check looks at the status line prefix, not at the head size. It has its own dependence on read boundaries: a 407 reply reports Proxy connection failed in one read and Proxy authentication required when the first read has 13 bytes or fewer. That is a separate bug and this PR leaves it alone. Moving the cap into picohttp::Response::parse_parts is a possible follow-up.

Open question for a maintainer. No maintainer has said that a complete head over the bound must be rejected. Node (through llhttp) and the bun-uws request parser reject it. If the preferred policy for the WebSocket client is a growth bound only, the Ok arm check can be dropped and the rest of the PR still stands.

Fail-before. Stock bun (1.4.3-canary.1+b52d51348):

  • fetch.test.ts: accepts a head of 1 MiB + 1 byte in one write and in 4 KiB writes. A head of cap + 1 bytes can never hit the old short-read check, because no incomplete prefix is longer than the cap. The exact 1 MiB case is a control.
  • websocket-client-short-read.test.ts: opens a 16961 byte head, opens a 21000 byte head whose first read is 16000 bytes, rejects a 16400 byte head whose first read is 16390 bytes, and reports Invalid response for an incomplete 20 KB head. The 16960 byte case is a control.

Also ran (debug build with ASAN): websocket-client.test.ts, websocket-upgrade.test.ts, websocket-handshake-event.test.ts, websocket-custom-headers.test.ts, websocket-accept-header-validation.test.ts, websocket-proxy.test.ts, error-event.test.ts, test/js/bun/http/proxy-stress-protocol.test.ts. All pass. websocket-client-short-read.test.ts passed 5 runs in a row.

The full fetch.test.ts run under the ASAN debug build also fails a set of tests that need public internet (the container proxy denies it), root-sensitive permission tests, and a few (with gc) tests that time out at 5 s. These fail without this change too. should allow very long redirect URLS needs 5.6 s on the debug build (200 forced GCs) and passes with a larger timeout.

Trial merges of this branch with #43243 and with #43181 are clean.

no test proof · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/fetch.test.ts