Repository navigation
http: don't inject default :80/:443 in proxy request-line - #29378
Conversation
When sending a request to an HTTP proxy, the absolute-form request URI (RFC 7230 §5.3.2) should omit the default port. Bun was unconditionally writing `:<port>` — falling back to "80"/"443" when the URL had no explicit port — which turned `http://example.com/path` into `POST http://example.com:80/path HTTP/1.1` on the wire. That breaks proxies that do strict Host/authority matching (Charles, mitmproxy, corporate middleboxes) and diverges from curl and Node's `http.request`, both of which omit the default port. Fix: only emit `:<port>` when the original URL had an explicit port. Fixes #29371
|
Updated 9:08 PM PT - Apr 16th, 2026
❌ @robobun, your commit 9684a51 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29378That installs a local version of the PR into your bun-29378 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChanged proxy request formatting so the absolute-form URI written by Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29371.test.ts`:
- Around line 2-8: The file header contains a multi-line bug-context block that
should be trimmed to the project standard: replace the long explanatory prose at
the top of test/regression/issue/29371.test.ts with a single-line GitHub issue
URL comment (keeping the existing issue link), removing the multi-line
paragraph; ensure the file retains the single issue URL comment pattern used
across test/regression/issue/* so only the one-line comment remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: af0df62b-3a44-402b-adda-119897f913d9
📒 Files selected for processing (2)
src/http.zigtest/regression/issue/29371.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the Zig change is a clean, minimal fix for a well-documented RFC violation.
Extended reasoning...
Overview
The PR modifies two files: src/http.zig (5 lines removed, 4 added in writeProxyRequest) and a new regression test test/regression/issue/29371.test.ts. The core change removes the unconditional default-port injection in the absolute-form proxy request-URI.
Security risks
None. The change only affects how the port component is formatted in outgoing proxy request-lines. No auth, crypto, or permission-sensitive code is touched.
Level of scrutiny
Low. The fix is a single conditional expression change with clear RFC justification (RFC 7230 §5.3.2). The behavior difference (omitting vs. including the redundant default port) is well-understood, and the fix matches curl and Node.js behavior. The regression test covers all three relevant cases.
Other factors
The only reported issue is a nit in test code (socket.on('error', () => {}) should call socket.destroy() to avoid a theoretical hang in proxy.close()). This is test-code only and an extremely low-probability race on localhost. The inline comment already captures this. The core production change is unambiguously correct.
An empty error handler suppresses the throw but leaves the socket tracked by the server, so server.close() can't emit 'close'. Call socket.destroy() on error so proxy.close() always completes.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/regression/issue/29371.test.ts (1)
2-8:⚠️ Potential issue | 🟡 MinorKeep only the single issue URL header comment.
Please remove the multi-line bug-context prose at the top and keep just the single URL line to match regression test file convention.
Based on learnings: in `test/regression/issue/*`, keep the single-line GitHub issue URL comment and avoid multi-line bug-context prose.✂️ Suggested edit
// https://github.com/oven-sh/bun/issues/29371 -// -// Bun was unconditionally inserting `:80` (or `:443`) into the absolute-form -// request-URI sent to an HTTP proxy, even when the target URL had no explicit -// port. That turned e.g. `http://example.com/path` into -// `POST http://example.com:80/path HTTP/1.1`, which breaks proxies that do -// strict Host/authority matching. Per RFC 7230 §5.3.2 the default port should -// be omitted; curl and Node's `http.request` both do this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/29371.test.ts` around lines 2 - 8, The top of the regression test file test/regression/issue/29371.test.ts contains a multi-line bug-context comment block; replace that block with a single-line GitHub issue URL comment only (remove all explanatory prose), ensuring the file follows the test/regression/issue/* convention of keeping just the issue URL comment at the top.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/regression/issue/29371.test.ts`:
- Around line 2-8: The top of the regression test file
test/regression/issue/29371.test.ts contains a multi-line bug-context comment
block; replace that block with a single-line GitHub issue URL comment only
(remove all explanatory prose), ensuring the file follows the
test/regression/issue/* convention of keeping just the issue URL comment at the
top.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 86b8d560-ca66-4c5a-aa92-ceb97e3d2ef6
📒 Files selected for processing (1)
test/regression/issue/29371.test.ts
There was a problem hiding this comment.
LGTM — the socket.destroy() fix addressed my prior feedback and the RFC 7230 §5.3.2 fix in http.zig is correct.
Extended reasoning...
Overview
Two files changed: src/http.zig removes the unconditional default-port injection in writeProxyRequest(), and test/regression/issue/29371.test.ts adds three regression test cases (no explicit port, explicit non-default port, WHATWG-normalized-away port).
Security risks
None. This is a bug fix to HTTP request formatting; it only affects the shape of the absolute-form URI in the proxy request-line.
Level of scrutiny
The zig change is a 9-line diff: removes a dead port-fallback variable, adds a conditional write that mirrors what getPort() already exposes. It precisely implements RFC 7230 §5.3.2 and matches curl/Node behavior. Low risk, well-scoped.
Other factors
My earlier inline comment about the missing socket.destroy() call in the error handler has been resolved — the current diff already contains socket.on("error", () => socket.destroy()), and the inline comment is marked resolved. No bugs were found by the automated system. CI build failures visible in the timeline are from an intermediate autofix.ci commit; the most recent commits on main include both the http.zig fix and the test helper fix.
|
CI failures on 2bdcdde are pre-existing on main, not caused by this change:
This PR only modifies |
Main's `Un-revert mimalloc upgrade` bumped the mimalloc pin to a29368ef but left two tests referencing the previous state: - process.versions.mimalloc expected 9a5e1f52 (parent of a29368ef). Fails deterministically on windows/linux/alpine/debian/ubuntu lanes that actually link mimalloc. - static-initializers expected 3/4 initializers assuming MI_OSX_ZONE=ON, but scripts/build/deps/mimalloc.ts sets ZONE=OFF on darwin (zone override breaks NAPI addons), so the zone-swap constructor doesn't run. Actual count is 2 on arm64 / 3 on x64. Pre-existing on main; included here to unblock CI.
|
Added a second commit (9684a51) to sync two tests that
Pre-existing on main; folded in here to unblock CI. Happy to split out to a separate PR if preferred. |
There was a problem hiding this comment.
LGTM — previous feedback addressed, fix is correct.
Extended reasoning...
Overview
The PR touches four files: the core HTTP client in src/http.zig, a new regression test in test/regression/issue/29371.test.ts, and two test expectation syncs for mimalloc (process.test.js, static-initializers.test.ts).
Core fix
The change in writeProxyRequest() removes unconditional :port injection, writing it only when client.url.getPort() returns non-null. This is a one-logical-step fix that aligns with RFC 7230 §5.3.2 and matches curl/Node behavior.
My prior feedback
My previous inline comment flagged that socket.on('error', () => {}) in the test helper left sockets untracked after errors, potentially preventing server.close() from emitting 'close'. Commit 2bdcdde addressed this with socket.on('error', () => socket.destroy()); both inline comments are marked resolved.
Security risks
None — this is a request formatting change in client code with no auth, crypto, or permission implications.
Level of scrutiny
Low. The change is minimal, correct, well-tested with three targeted regression cases, and consistent with existing HTTP client patterns.
|
Latest CI failure is |
) Fixes oven-sh#29371. ## Repro ```js // proxy-spy.mjs — logs raw request lines import net from "node:net"; net.createServer(s => s.on("data", d => { console.log(d.toString().split("\r\n")[0]); s.end("HTTP/1.1 200 OK\r\nConnection: close\r\n\r\nok\n"); })).listen(9999); ``` ```console $ HTTP_PROXY=http://localhost:9999 bun -e 'await fetch("http://example.com/test", {method:"POST", body:""})' POST http://example.com:80/test HTTP/1.1 ← Bun POST http://example.com/test HTTP/1.1 ← curl, Node, expected ``` ## Cause `writeProxyRequest` in `src/http.zig` unconditionally wrote `:<port>` to the absolute-form request-URI, falling back to `"80"` / `"443"` when the URL had no explicit port. Per RFC 7230 §5.3.2 the default port should be omitted, and strict proxies (Charles, mitmproxy, corporate middleboxes) refuse to accept `:80` as equivalent to no port when matching authorities. ## Fix Only emit `:<port>` when `client.url.getPort()` returns a value — i.e. the URL had an explicit port. `getPort()` returns `null` for empty `port`, so no explicit port = no `:port` in the request-line. Explicit non-default ports (e.g. `http://host:8080/`) still get written through. ## Verification Regression test at `test/regression/issue/29371.test.ts` spins up a TCP listener acting as an HTTP proxy and asserts the first line of the forwarded request: - no explicit port (`http://example.com/test`) → `POST http://example.com/test HTTP/1.1` ✓ - explicit non-default port (`http://example.com:8080/test`) → `GET http://example.com:8080/test HTTP/1.1` ✓ - WHATWG-normalized-away port (`http://example.com:80/test` → fetch strips `:80`) → `GET http://example.com/test HTTP/1.1` ✓ Fail-before / pass-after: the two no-explicit-port tests fail on `main`, all three pass with this change. Existing `test/js/bun/http/proxy.test.ts` has the same pre-existing failures with and without this patch. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
) Fixes oven-sh#29371. ## Repro ```js // proxy-spy.mjs — logs raw request lines import net from "node:net"; net.createServer(s => s.on("data", d => { console.log(d.toString().split("\r\n")[0]); s.end("HTTP/1.1 200 OK\r\nConnection: close\r\n\r\nok\n"); })).listen(9999); ``` ```console $ HTTP_PROXY=http://localhost:9999 bun -e 'await fetch("http://example.com/test", {method:"POST", body:""})' POST http://example.com:80/test HTTP/1.1 ← Bun POST http://example.com/test HTTP/1.1 ← curl, Node, expected ``` ## Cause `writeProxyRequest` in `src/http.zig` unconditionally wrote `:<port>` to the absolute-form request-URI, falling back to `"80"` / `"443"` when the URL had no explicit port. Per RFC 7230 §5.3.2 the default port should be omitted, and strict proxies (Charles, mitmproxy, corporate middleboxes) refuse to accept `:80` as equivalent to no port when matching authorities. ## Fix Only emit `:<port>` when `client.url.getPort()` returns a value — i.e. the URL had an explicit port. `getPort()` returns `null` for empty `port`, so no explicit port = no `:port` in the request-line. Explicit non-default ports (e.g. `http://host:8080/`) still get written through. ## Verification Regression test at `test/regression/issue/29371.test.ts` spins up a TCP listener acting as an HTTP proxy and asserts the first line of the forwarded request: - no explicit port (`http://example.com/test`) → `POST http://example.com/test HTTP/1.1` ✓ - explicit non-default port (`http://example.com:8080/test`) → `GET http://example.com:8080/test HTTP/1.1` ✓ - WHATWG-normalized-away port (`http://example.com:80/test` → fetch strips `:80`) → `GET http://example.com/test HTTP/1.1` ✓ Fail-before / pass-after: the two no-explicit-port tests fail on `main`, all three pass with this change. Existing `test/js/bun/http/proxy.test.ts` has the same pre-existing failures with and without this patch. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
Fixes #29371.
Repro
Cause
writeProxyRequestinsrc/http.zigunconditionally wrote:<port>to the absolute-form request-URI, falling back to"80"/"443"when the URL had no explicit port. Per RFC 7230 §5.3.2 the default port should be omitted, and strict proxies (Charles, mitmproxy, corporate middleboxes) refuse to accept:80as equivalent to no port when matching authorities.Fix
Only emit
:<port>whenclient.url.getPort()returns a value — i.e. the URL had an explicit port.getPort()returnsnullfor emptyport, so no explicit port = no:portin the request-line. Explicit non-default ports (e.g.http://host:8080/) still get written through.Verification
Regression test at
test/regression/issue/29371.test.tsspins up a TCP listener acting as an HTTP proxy and asserts the first line of the forwarded request:http://example.com/test) →POST http://example.com/test HTTP/1.1✓http://example.com:8080/test) →GET http://example.com:8080/test HTTP/1.1✓http://example.com:80/test→ fetch strips:80) →GET http://example.com/test HTTP/1.1✓Fail-before / pass-after: the two no-explicit-port tests fail on
main, all three pass with this change. Existingtest/js/bun/http/proxy.test.tshas the same pre-existing failures with and without this patch.