http: compare URL scheme case-insensitively for proxy and redirect Location - #35144
Conversation
RFC 3986 section 3.1 defines the URL scheme as case-insensitive, and
both curl and undici's EnvHttpProxyAgent accept `HTTP://` in
`http_proxy`. Bun's `fetch(url, { proxy })` option accepts it because
the WHATWG URL parser lowercases the scheme, but the `http_proxy` /
`HTTPS_PROXY` environment variables are parsed by `bun_url::URL::parse`,
which borrows the scheme slice without normalizing it. The proxy
protocol check in `HTTPThread` and the `is_http`/`is_https` helpers
compared the borrowed bytes exactly, so `http_proxy=HTTP://host:port`
rejected every request (fetch and `bun install` alike) with
`UnsupportedProxyProtocol`.
Make `is_http`, `is_https`, `is_s3`, `is_file`, and
`has_http_like_protocol` compare ASCII case-insensitively, and route
the inline protocol checks in `HTTPThread` through
`has_http_like_protocol`. This also fixes `get_port_auto()` defaulting
an `HTTPS://proxy` URL to port 80 instead of 443.
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Reproduced: PR: #35144 CI (build 77893): the new tests in
Diff is ready for review. |
|
Updated 10:32 AM PT - Jul 22nd, 2026
❌ @robobun, your commit c50a91c has 3 failures in
🧪 To try this PR locally: bunx bun-pr 35144That installs a local version of the PR into your bun-35144 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Re #16182: that issue asks for full WHATWG normalization of the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The redirect follower slices the scheme out of the raw Location header bytes and checks it against "http"/"https" before WHATWG normalization runs, so `Location: HTTPS://host/...` was rejected with UnsupportedRedirectProtocol. Route those two comparisons through eql_case_insensitive_ascii to match the proxy-protocol fix.
robobun
changed the title
There was a problem hiding this comment.
LGTM — the follow-up commit addressed the two sibling Location-scheme comparisons I flagged, and the added redirect test covers them.
What was reviewed:
- Confirmed
eql_case_insensitive_ascii(.., true)length-checks first, so an empty protocol slice returns false without tripping the!a.is_empty()debug_assert. - Re-grepped
src/http/for== b"http"/b"https"— no remaining byte-exact scheme comparisons. - Checked
is_file()/is_s3()callers: thebun_url::URLones infetch.rsreceive WHATWG-normalized (already-lowercase) schemes, so widening is a no-op there; the other hits are unrelatedPath::is_file/Blob::is_s3methods. - Tests are hermetic (local
net.createServer, subprocess for env-var isolation), drain pipes concurrently, and include the negative case (socks5://still rejected).
Extended reasoning...
Overview
Swaps byte-exact scheme comparisons for ASCII-case-insensitive ones at every site that sees an un-normalized URL scheme: bun_url::URL::{is_http,is_https,is_s3,is_file,has_http_like_protocol}, the two proxy-protocol checks in HTTPThread::connect, and the two Location: scheme checks in the redirect follower. Adds subprocess-isolated tests for http_proxy=HTTP://… (accepted) / socks5:// (still rejected) and an in-process redirect test for Location: HTTP:// / Http:// / hTtP://.
Security risks
None identified. The change strictly widens which case-spellings of http/https/file/s3 are recognised, per RFC 3986 §3.1. For is_file() this is directionally safer (a FILE:// guard now also matches). The proxy path already treated an empty protocol as http-like; the negative test confirms unrecognised schemes remain rejected rather than silently going direct.
Level of scrutiny
Moderate — touches the HTTP client/proxy connect path and redirect follower, but the diff is a mechanical comparison swap through an existing in-tree helper with no control-flow, ownership, or lifetime changes. The URL helpers are shared, so I checked callers: fetch-side callers go through WHATWG normalization first (scheme already lowercase), so behaviour there is unchanged; the borrowing-parser paths (env-var proxy, raw Location: bytes) are exactly what the tests exercise.
Other factors
My prior review flagged the two src/http/lib.rs sibling sites; c50a91c fixes both and adds a covering test. I re-verified via grep that no == b"http"-shaped comparisons remain in src/http/. The eql_case_insensitive_ascii(.., true) helper short-circuits on length mismatch before its non-empty debug_asserts, so empty protocol slices (e.g. Location: ://foo) behave as before. Tests follow harness conventions (pipe draining via Promise.all, bunEnv spread with proxy env keys undefined, try/finally cleanup, port: 0).
2 participants
What
http_proxy=HTTP://host:port(or any scheme not spelled in lowercase) rejected every request throughfetchandbun installwithUnsupportedProxyProtocol, while the same string passed viafetch(url, { proxy: "HTTP://..." })worked. Similarly, a server responding withLocation: HTTPS://host/...failed the redirect withUnsupportedRedirectProtocol.Why
RFC 3986 section 3.1 defines the URL scheme as case-insensitive, and both curl and undici's
EnvHttpProxyAgentaccept the uppercase form. The{ proxy }option path goes through the WHATWG URL parser, which lowercases the scheme; thehttp_proxy/HTTPS_PROXYenvironment variables are parsed bybun_url::URL::parse, which is a borrowing parser and keepsprotocolas a raw slice of the input. The proxy protocol check inHTTPThreadand theis_http()/is_https()helpers compared those bytes exactly. The redirect follower slices the scheme out of the rawLocationheader bytes before WHATWG normalization runs and compared the same way.Fix
bun_url::URL::is_http,is_https,is_s3,is_file, andhas_http_like_protocolnow compare ASCII case-insensitively.HTTPThreadgo throughhas_http_like_protocol().Locationscheme comparisons in the redirect follower go throughstrings::eql_case_insensitive_ascii.This also fixes
get_port_auto()defaultingHTTPS://proxy(no explicit port) to 80 instead of 443, andHTTPClient::is_https()picking the plaintext context for anHTTPS://proxy.Verification
Full
proxy.test.ts(62 tests) andfetch-redirect.test.ts(15 tests) pass.Related: #16182 (this covers scheme case only; full WHATWG normalization of the proxy env URL is still open)
[review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file