WebSocket: parse the proxy CONNECT reply before judging its status

Closed
RO
1mo ago
+117-17

Problem

  • new WebSocket(url, { proxy }) reports a different result for the same CONNECT reply, depending on where the first read ends. A 407 in one read gives Proxy connection failed. With a first read of 13 bytes or fewer it gives Proxy authentication required.
  • HTTP/1.1 200\r\n\r\n (no reason phrase) fails in one read and opens the tunnel after a short first read. fetch accepts it.
  • Cause: the prefix check in handle_proxy_response (src/http_jsc/websocket_client/WebSocketUpgradeClient.rs:835). A first read of more than 13 bytes that does not start with HTTP/1.1 200 or HTTP/1.0 200 fails with ProxyConnectFailed before the parser runs.

Fix

  • Remove the prefix check. The parsed status decides: 200 opens the tunnel, 407 gives Proxy authentication required, any other status gives Proxy connection failed.
  • A reply that is not HTTP now always gives Invalid response (1002). A long first read gave Proxy connection failed (1006) before.
  • #36272 deleted the same check from the 101 path. Only that deletion is the precedent: this PR adds no ws event. Three tests expected Proxy connection failed for a 407, because #40624 recorded the one-read text.
  • Verified: test/js/web/websocket/websocket-proxy.test.ts (5 replies, each in one read and in two). Stock bun fails 5 tests there. Self-reviewed: 3 concerns raised, all are follow-ups (Notes).

Background

  • With a proxy option, the client first sends CONNECT host:port to the proxy. A 200 reply means that the proxy now relays bytes.
  • buffer_and_parse_head keeps an incomplete head and parses again on the next read.
  • ErrorCode::ProxyAuthenticationRequired exists only for a 407. WebSocket.cpp maps it to that text.
Notes

Repro. A raw TCP proxy answers CONNECT with HTTP/1.1 407 Proxy Authentication Required\r\nProxy-Authenticate: Basic realm="proxy"\r\nContent-Length: 0\r\n\r\n. Clear NO_PROXY first, because it also applies to an explicit proxy for a loopback target. On bun 1.4.3-canary.1+367d939d9:

one write:            failed: Proxy connection failed
split after 10 bytes: failed: Proxy authentication required
split after 13 bytes: failed: Proxy authentication required
split after 14 bytes: failed: Proxy connection failed

The same check, other replies (one read / first read of 10 bytes, before this change):

CONNECT replyone readshort first read
HTTP/1.1 200\r\n\r\nProxy connection failedopens
SSH-2.0-OpenSSH_9.6\r\n1006 Proxy connection failed1002 Invalid response
HTTP/1.1 502 Bad Gateway\r\n\r\nProxy connection failedProxy connection failed

fetch("https://...", { proxy }) through a proxy that answers HTTP/1.1 200\r\n\r\n returns the origin response. picohttp accepts a status line with no reason phrase.

Which text for a 407. #25614 added the prefix check, the 407 branch and error code 34 together. The check is a copy of the "fail early if we receive a non-101 status code" check of the upgrade path, and it made the 407 branch unreachable for a reply that arrives in one read. The tests asserted only "an error fired" until #40624 recorded the observed text. Node's ws with an HttpsProxyAgent reports Unexpected server response: 407 for this reply, so the status is visible there too.

Replies whose result changes, all for a first read of more than 13 bytes. Each one already had the new result for a short first read.

  • 407: Proxy connection failed to Proxy authentication required.
  • 200 with no reason phrase: Proxy connection failed to an open tunnel.
  • Not HTTP: 1006 Proxy connection failed to 1002 Invalid response.
  • A status line, then FIN with no blank line: Proxy connection failed to Connection ended.
  • A non-200 head with more than 128 header fields: Proxy connection failed to Invalid response. picohttp parses into a 128-entry buffer.
  • A non-200 status line, then a stall with the socket open: Proxy connection failed at once becomes Timeout after the handshake timeout (BUN_CONFIG_WS_HANDSHAKE_TIMEOUT, default 120 s). With the check, a stall after a 200 status line, after a first read of 10 bytes, or with no reply at all already waited for that timeout (measured: 8 s for each with the timeout set to 1). The max_http_header_size cap also applies. The 101 path has waited for the complete head since #36272.

The split in the test. Nothing tells a server that the client consumed the first bytes, so the proxy writes the first 10 bytes, awaits two chained setImmediate callbacks, then writes the rest. auto_tick runs immediates and then polls sockets, so one poll falls between the two callbacks, and the client reads the first part in it. A probe on stock bun (where a 407 reports Proxy authentication required only after a short first read) saw a real split in 100 of 100 runs with two callbacks, and in 0 of 100 with one. On the debug build the handleProxyResponse log line appears twice for each split case. After this change the result does not depend on the split, so a reply that arrives whole repeats the one-read case.

Fail before. USE_SYSTEM_BUN=1 bun test test/js/web/websocket/websocket-proxy.test.ts: 5 fail (proxy auth failure returns error, the NO_PROXY not matching case, and the one-read rows for 407, 200 with no reason phrase, and not HTTP). Every two-read row passes there, which shows that the split is real. bun bd test: 49 pass, 4 skip (Docker).

Other suites, debug build. test/js/first_party/ws/ws-proxy.test.ts (19 pass), websocket-client-short-read, websocket-handshake-event, websocket-upgrade, websocket-proxy-close-reentrancy, test-ws-bidir-proxy. websocket-syscall-fault.test.ts has 5 s timeouts under the concurrent debug run, a different test each run, and each passes alone.

Not in this PR. The self-review raised these. None of them changes this diff.

  • The ws client emits no unexpected-response event for a refused CONNECT. Node's ws does, because https-proxy-agent replays the proxy reply as the upgrade response.
  • The WebSocket error drops the status line and the headers of the proxy reply. fetch keeps the proxy reply on its ProxyConnectFailed error (src/http/lib.rs). To carry the status here, the parser must see every reply first, which is this change.
  • The failure texts are one switch in WebSocket.cpp. There is no table per ErrorCode.
  • Found on the way and handed off: a 2xx reply other than 200 (201, 204) still fails with Proxy connection failed, and fetch accepts any 2xx (RFC 9110 section 9.3.6).

Other open PRs. The branch merges cleanly with #41573, #43243 and #37638 (git merge-tree).