Repository navigation
http: don't inject default :80/:443 in proxy request-line #29378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
4138e79
http: don't inject default :80/:443 in proxy request-line
robobun 36410f6
[autofix.ci] apply automated fixes
autofix-ci[bot] 2bdcdde
test(29371): destroy errored sockets in proxy test helper
robobun 9684a51
test: sync post-mimalloc-upgrade expected values
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| // 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. | ||
|
|
||
| import { expect, test } from "bun:test"; | ||
| import { once } from "node:events"; | ||
| import net from "node:net"; | ||
|
|
||
| function createCapturingProxy() { | ||
| const requests: string[] = []; | ||
| const server = net.createServer((socket: net.Socket) => { | ||
| let buf = ""; | ||
| socket.on("data", chunk => { | ||
| buf += chunk.toString("utf8"); | ||
| // Capture the request-line + headers on the first request we see. | ||
| const headerEnd = buf.indexOf("\r\n\r\n"); | ||
| if (headerEnd !== -1) { | ||
| requests.push(buf.slice(0, headerEnd)); | ||
| socket.end("HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: close\r\n\r\nok"); | ||
| buf = ""; | ||
| } | ||
| }); | ||
|
robobun marked this conversation as resolved.
|
||
| // If the socket errors (e.g. client RSTs mid-write), destroy it so | ||
| // server.close() can drop its tracking and emit 'close'. A bare | ||
| // empty handler leaves the socket tracked forever. | ||
| socket.on("error", () => socket.destroy()); | ||
| }); | ||
| return { | ||
| server, | ||
| requests, | ||
| async listen() { | ||
| server.listen(0); | ||
| await once(server, "listening"); | ||
| return (server.address() as net.AddressInfo).port; | ||
| }, | ||
| async close() { | ||
| server.close(); | ||
| await once(server, "close"); | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| test("proxy request-line omits default :80 for http:// without explicit port", async () => { | ||
| const proxy = createCapturingProxy(); | ||
| const port = await proxy.listen(); | ||
| try { | ||
| const res = await fetch("http://example.com/test", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ hello: "world" }), | ||
| proxy: `http://localhost:${port}`, | ||
| keepalive: false, | ||
| }); | ||
| expect(res.status).toBe(200); | ||
| await res.text(); | ||
|
|
||
| expect(proxy.requests.length).toBeGreaterThanOrEqual(1); | ||
| const requestLine = proxy.requests[0].split("\r\n")[0]; | ||
| // Must NOT contain the injected default port. | ||
| expect(requestLine).toBe("POST http://example.com/test HTTP/1.1"); | ||
| expect(requestLine).not.toContain(":80"); | ||
| } finally { | ||
| await proxy.close(); | ||
| } | ||
| }); | ||
|
|
||
| test("proxy request-line keeps explicit non-default port for http://host:PORT/", async () => { | ||
| const proxy = createCapturingProxy(); | ||
| const port = await proxy.listen(); | ||
| try { | ||
| const res = await fetch("http://example.com:8080/test", { | ||
| method: "GET", | ||
| proxy: `http://localhost:${port}`, | ||
| keepalive: false, | ||
| }); | ||
| expect(res.status).toBe(200); | ||
| await res.text(); | ||
|
|
||
| expect(proxy.requests.length).toBeGreaterThanOrEqual(1); | ||
| const requestLine = proxy.requests[0].split("\r\n")[0]; | ||
| // Explicit non-default port must be preserved. | ||
| expect(requestLine).toBe("GET http://example.com:8080/test HTTP/1.1"); | ||
| } finally { | ||
| await proxy.close(); | ||
| } | ||
| }); | ||
|
|
||
| test("proxy request-line strips explicit :80 that fetch normalized away", async () => { | ||
| // Even if the user writes `:80`, WHATWG URL normalization in fetch() drops | ||
| // it before the URL reaches the HTTP client. The request-line must match | ||
| // that normalized form — no phantom `:80` reappearing on the wire. | ||
| const proxy = createCapturingProxy(); | ||
| const port = await proxy.listen(); | ||
| try { | ||
| const res = await fetch("http://example.com:80/test", { | ||
| method: "GET", | ||
| proxy: `http://localhost:${port}`, | ||
| keepalive: false, | ||
| }); | ||
| expect(res.status).toBe(200); | ||
| await res.text(); | ||
|
|
||
| expect(proxy.requests.length).toBeGreaterThanOrEqual(1); | ||
| const requestLine = proxy.requests[0].split("\r\n")[0]; | ||
| expect(requestLine).toBe("GET http://example.com/test HTTP/1.1"); | ||
| } finally { | ||
| await proxy.close(); | ||
| } | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.