Conversation
URL::get_port_auto() turns a port that fails u16 parsing (":107688",
":99999", ":9000abc") into the scheme default, so consumers that skip
the WHATWG parser silently sent traffic to port 80/443 of the host:
- S3Client endpoint: the SigV4-signed request went to port 80/443 of
the endpoint host, and presign() emitted the bogus port verbatim
- HTTP_PROXY / HTTPS_PROXY: every proxied fetch went to port 80/443 of
the proxy host
HTTPThread::connect now rejects these before dialing (InvalidPort /
InvalidProxyPort), S3 signing rejects them with ERR_S3_INVALID_ENDPOINT,
and the S3Client endpoint option throws ERR_INVALID_ARG_VALUE at
construction.
|
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 (8)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this changes behavior in the HTTP client's single connect chokepoint (HTTPThread::connect) and in S3 signing — every network request and every SigV4 sign goes through these paths. Worth a human look at the new validation guards and the custom host_has_invalid_port port-splitting in s3_signing/credentials.rs.
What was reviewed:
has_invalid_port()and the connect-path guards — placed after the unix-socket branch, before anyget_port_auto()call; covers direct, proxy, and custom-TLS-context paths.- The
credentials_jsc.rsearly return —str.deref()is called andutf8: ZigStringSlicedrops on return, matching the existing invalid-endpoint branch a few lines below. - New
http::Errorvariants —name()and the FetchTasklet message match are updated; no other exhaustive matches onhttp::Errorfound.
Extended reasoning...
Overview
The PR closes a hole where a URL with an unparseable :port (out-of-u16-range or trailing garbage) silently fell back to the scheme-default port 80/443 via URL::get_port_auto(). It adds URL::has_invalid_port() and rejects at three layers: the HTTP client's HTTPThread::connect (request URL and proxy URL), S3 credentials JS-option parsing (constructor / per-file endpoint), and S3 SigV4 signing (host_has_invalid_port on the endpoint host, covering env-var endpoints and presign()). Two new http::Error variants get names and fetch-facing messages. Tests cover env-proxy, S3Client constructor, per-file endpoint, valid-port passthrough, and the S3_ENDPOINT env-var presign path.
Security risks
This is a security-hardening fix (prevents SigV4 Authorization headers from being delivered to whatever listens on port 80/443 of a mistyped endpoint host, and prevents proxied traffic from going to port 80/443 of the proxy host). No new attack surface is introduced. The change is fail-closed: a previously-silently-misdirected request now errors.
Level of scrutiny
Medium-high. The validation logic itself is small and readable, but the placement is in Bun's single TCP-connect chokepoint — every fetch, every S3 op, every bun install network task funnels through HTTPThread::connect. The guard is placed after the unix-socket early return (correct — unix sockets ignore ports) and before every get_port_auto() call, and I traced all three connect branches (default context, custom-SSL cache hit, custom-SSL cache miss) — all read the port after the guard. But given this file's blast radius, a maintainer should confirm the placement.
The host_has_invalid_port helper in s3_signing/credentials.rs is bespoke port-splitting: it handles [ipv6]:port vs host:port by looking for a leading [. At this call site host is self.endpoint up to the first /, which for JS-supplied endpoints came from URL::parse().host_with_path(), but for env-supplied endpoints the normalization path may differ. A bare unbracketed IPv6 literal would false-positive here (first : treated as port separator), though that's unlikely to have worked before either. Worth a maintainer glance.
Other factors
- The
credentials_jsc.rsearly-return cleanup mirrors the existing!endpoint.is_empty()invalid-endpoint branch below it:str.deref()explicit,utf8drops viaZigStringSlice'sDrop. - I checked for other exhaustive matches on
http::Errorthat would need updating — none found beyond thename()match (updated) and the FetchTasklet message match (updated, has a fallback arm). - Tests are well-structured (subprocess isolation for env vars, concurrent, drain all pipes, assert exit code last, exercise the four bad-port shapes plus a valid-port control). The four unrelated s3.test.ts fixtures now clear ambient
HTTP_PROXY— a hermeticity fix, but it means those tests' behavior changes in environments that set one. - This is a behavior change: configs that previously "worked" by accident (endpoint on 80/443 with a garbage
:port) will now error. That's correct and matches curl/Node, but it's the kind of change a human should sign off on.
|
Verified the IPv6 concern from the review:
|
|
Updated 6:26 PM PT - Aug 5th, 2026
✅ @robobun, your commit 4b7f963a433c58dac2e8adb27d290bd450d4ec44 passed in 🧪 To try this PR locally: bunx bun-pr 37003That installs a local version of the PR into your bun-37003 --bun |
### Problem
- `new Bun.S3Client({ endpoint: "http://127.0.0.1:A#@127.0.0.1:B" })`
signs for `127.0.0.1:B` and sends `PUT /bkt/k`, `Authorization` and the
body there. `new URL(endpoint).host` is `127.0.0.1:A`. `http://A\@B`
behaves the same. A service that checks a tenant's endpoint with `new
URL()` sends signed requests to a host it did not approve.
- Cause: `credentials_jsc.rs:119` and `env_loader.rs:281` give the raw
string to `bun_url::URL::parse`, whose credential and host scanning stop
only at `/` and `?`, and store its `host_with_path()`. Dot segments and
`127.1` are sent as written for the same reason.
### Fix
- `URL::parse_s3_endpoint` (`src/url/lib.rs`) keeps the existing `parse`
host check, so the accepted set does not change. It then parses the
string with WTF::URL (`https://` prepended when there is no scheme) and
stores its scheme, host with port, and path. When WTF::URL rejects the
input, it stores what `parse` reads, as today. Both stores call it.
- WTF::URL ends the authority at `#`, `\` and the last `@`, resolves dot
segments, canonicalizes the host and never puts credentials in it. The
stored form is unchanged, so `guess_region`, inspect output and
path-prefix endpoints still work. The notes list the visible side
effects.
- Verified: `test/js/bun/s3/s3-list-encode-overflow.test.ts` (the
network-free presign tests), 8 of the 12 new cases fail on 1.4.0. Also
`test/js/bun/s3/`: 156 pass, 1 timeout (notes).
### Background
- `S3Credentials.endpoint` stores `host[:port][/prefix]` without a
scheme. `sign_request` (`src/s3_signing/credentials.rs:384`) signs the
part before the first `/` as the host and sends it as `Host`. The stored
bytes are both signed for and connected to.
- Bun has two URL parsers. WTF::URL (WebKit) is the WHATWG parser behind
`new URL()`. `bun_url::URL::parse` slices an href that is already
normalized. `URL::from_string` and `OwnedURL` bridge the two.
- #38043 changes `URL::parse` itself for `user@host:port` and keeps `#`
in the authority on purpose. The two changes are independent. #16183 is
the umbrella issue for that parser.
<details><summary>Notes</summary>
Repro on release 1.4.0 with two `Bun.serve` listeners A and B on
127.0.0.1 (run without `HTTP_PROXY` set, since S3 ignores `NO_PROXY`,
#32045):
```
endpoint http://127.0.0.1:A#@127.0.0.1:B new URL().host = 127.0.0.1:A
A: (nothing)
B: PUT /bkt/k host=127.0.0.1:B x-amz-security-token=SESSIONTOKENEXAMPLE
endpoint http://127.0.0.1:A\@127.0.0.1:B same
```
presign() on 1.4.0 for the spellings in the test:
```
http://127.0.0.1:A#@127.0.0.1:B -> http://127.0.0.1:B/bkt/k?...
http://127.0.0.1:A\@127.0.0.1:B -> http://127.0.0.1:B/bkt/k?...
http://127.0.0.1:A?@127.0.0.1:B -> http://A/bkt/k?... (the port text becomes the host)
http://u:p@ss@127.0.0.1:A -> http://ss@127.0.0.1:A/bkt/k?... (a request resolves ss@127.0.0.1)
http://127.0.0.1:A/x/../prefix/ -> http://127.0.0.1:A/x/../prefix/bkt/k?...
http://127.1:A -> http://127.1:A/bkt/k?...
```
With the change every one of them starts with `http://127.0.0.1:A/`.
`\@` becomes the path prefix `/@127.0.0.1:B/`, which is what `new URL()`
reports for it. `S3_ENDPOINT` and `AWS_ENDPOINT` (`env_loader.rs`) had
the same problem and get the same treatment; the test covers
`S3_ENDPOINT` in a child process.
The test compares the presigned URL as text. `new URL(presigned)` would
itself strip a leaked `ss@`, resolve `/x/../` and canonicalize `127.1`,
and so hide the credential, dot-segment and `127.1` cases.
Why the `parse` host check is kept: `s3.test.ts` expects `endpoint:
"🙂.🥯"` and `"..asd.@%&&&%%"` to throw `ERR_INVALID_ARG_TYPE`, and
WTF::URL accepts the first as an IDN host. With the old check in front,
this change only alters what is stored for endpoints that were already
accepted.
Visible side effects besides the fix: a default port in the endpoint
(`http://h:80`) is now left out of the `Host` header, and a mixed-case
host is lowercased. Both come from the WTF::URL serialization.
Why the raw fallback: WTF::URL rejects a port such as `:99999`, which
today is accepted and connects to 80/443. #37003 turns that into an
error. This change leaves it alone so the two do not overlap.
Also unchanged: `guess_bucket`, the scheme-less R2 and
`s3.us-west-1.amazonaws.com` endpoints in `s3.test.ts` (stored byte for
byte as before), and a Supabase style `https://host/storage/v1/s3`
prefix (covered by the new test).
The S3 suite was run with the proxy variables of this environment unset.
The one timeout is `s3-list-objects.test.ts` "Should fall back to
NoSuchKey ...". Its `describe.concurrent` block runs it next to "Should
work with big responses", which takes about 18 s in this debug ASAN
build, so it exceeds its 5 s budget. It passes when run alone with this
build, and the stored endpoint for `server.url.href` is byte-identical
with and without the change.
`bun_s3_signing` does not depend on `bun_url`, and `bun_dotenv` must not
name `bun_s3_signing` types, so the helper lives in `bun_url` next to
`host_with_path()`, whose only callers were these two stores.
An earlier revision normalized the string with WTF::URL and sliced the
resulting href with `URL::parse` again. Review caught that this
inherited the `user@host:port` reading of `URL::parse` (#16181):
WTF::URL serializes `http://user:@h:1` as `http://user@h:1/`, so that
endpoint would have stored `user@h:1` where it stores `h:1` today. The
helper now reads scheme, host with port and path from the WTF::URL
components directly, and the `user:@` and `user@` spellings are in the
test. The second one is wrong on 1.4.0 as well.
</details>
<!-- robobun:evidence:begin -->
---
**[review]** gate passed · iteration 0 · 4 files touched
<details><summary>fails on main (without fix)</summary>
```console
ASAN without fix: 8 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/bun/s3/s3-list-encode-overflow.test.ts"
bun test v1.4.0 (6e906e4)
test/js/bun/s3/s3-list-encode-overflow.test.ts:
(pass) S3Client.list() option encoding > should not panic when prefix is longer than 1024 bytes when encoded [25.53ms]
(pass) S3Client.list() option encoding > should not panic when delimiter is longer than 1024 bytes when encoded [2.12ms]
(pass) S3Client.list() option encoding > should not panic when continuationToken is longer than 1024 bytes when encoded [1.78ms]
(pass) S3Client.list() option encoding > should not panic when startAfter is longer than 1024 bytes when encoded [2.24ms]
(pass) S3 object keys containing '?' or '#' > includes the full object key in the presigned URL path [9.33ms]
(pass) S3Client region option > rejects the region us-east-1/other.example.com because it is not a valid host name component [5.83ms]
(pass) S3Client region option > rejects the region us-east-1?x because it is not a valid host name component [1.85ms]
(pass) S3Client region option > rejects the region us-east-1#x because i
... (truncated)
release without fix: 8 FAILED
bun test v1.4.0-canary.1 (58d38cf)
test/js/bun/s3/s3-list-encode-overflow.test.ts:
(pass) S3Client.list() option encoding > should not panic when prefix is longer than 1024 bytes when encoded [0.15ms]
(pass) S3Client.list() option encoding > should not panic when delimiter is longer than 1024 bytes when encoded [0.03ms]
(pass) S3Client.list() option encoding > should not panic when continuationToken is longer than 1024 bytes when encoded [0.02ms]
(pass) S3Client.list() option encoding > should not panic when startAfter is longer than 1024 bytes when encoded [0.01ms]
(pass) S3 object keys containing '?' or '#' > includes the full object key in the presigned URL path [0.14ms]
(pass) S3Client region option > rejects the region us-east-1/other.example.com because it is not a valid host name component [0.07ms]
(pass) S3Client region option > rejects the region us-east-1?x because it is not a valid host name component [0.01ms]
(pass) S3Client region option > rejects the region us-east-1#x because it is not a valid host name component
(pass) S3Client region option > rejects the region us east 1 because it is not a valid host name component
(pass) S3Client region option
... (truncated)
```
</details>
<details><summary>passes on PR (with fix)</summary>
```console
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/bun/s3/s3-list-encode-overflow.test.ts"
bun test v1.4.0 (6e906e4)
test/js/bun/s3/s3-list-encode-overflow.test.ts:
(pass) S3Client.list() option encoding > should not panic when prefix is longer than 1024 bytes when encoded [10.88ms]
(pass) S3Client.list() option encoding > should not panic when delimiter is longer than 1024 bytes when encoded [28.39ms]
(pass) S3Client.list() option encoding > should not panic when continuationToken is longer than 1024 bytes when encoded [3.34ms]
(pass) S3Client.list() option encoding > should not panic when startAfter is longer than 1024 bytes when encoded [2.50ms]
(pass) S3 object keys containing '?' or '#' > includes the full object key in the presigned URL path [14.80ms]
(pass) S3Client region option > rejects the region us-east-1/other.example.com because it is not a valid host name component [8.69ms]
(pass) S3Client region option > rejects the region us-east-1?x because it is not a valid host name component [2.69ms]
(pass) S3Client region option > rejects the region us-east-1#x because
... (truncated)
release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 770ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/6] gen generated_host_exports.rs
generated_host_exports.rs: 92 exports (host=3, lazy=10, generic=79, rust=0); 240 extern-C blocks audited
[1/6] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)
nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)
�[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys)
�[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety)
�[1m�[92m Compiling�[0m bun_base64 v0.0.0 (/workspace/bun/src/base64)
�[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys)
�[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys)
�[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd)
�[1m�[92m Compiling�[0m bun_paths v0.0.0
... (truncated)
```
</details>
<details><summary>diff hotspot</summary>
```
src/dotenv/env_loader.rs | 8 ++--
src/runtime/webcore/s3/credentials_jsc.rs | 9 ++--
src/url/lib.rs | 62 +++++++++++++++++++++++++-
test/js/bun/s3/s3-list-encode-overflow.test.ts | 57 +++++++++++++++++++++++
4 files changed, 125 insertions(+), 11 deletions(-)
```
</details>
**gate history** · 1 passed · 0 rejected · iteration 0
<details><summary>evidence per changed file</summary>
```
file reads edits tests
src/dotenv/env_loader.rs 3 2 0
src/runtime/webcore/s3/credentials_jsc.rs 2 2 0
src/url/lib.rs 6 11 0
test/js/bun/s3/s3-list-encode-overflow.test.ts 2 4 0
```
</details>
<!-- robobun:evidence:end -->
This branch has not been deployed
2 participants
Problem
A URL port that fails to parse as a u16 (
:107688,:4295009448,:99999,:9000abc) silently collapses into the scheme default:URL::get_port_auto()(src/url/lib.rs) maps "present but unparseable" toNoneand then substitutes 80/443. Everything that goes through the WHATWG parser first (fetch(url),fetch(url, { proxy }),new WebSocket) rejects these strings, but two public surfaces skip it:S3Clientendpoint: the SigV4-signed request is sent to port 80/443 of the endpoint host (theAuthorizationheader is delivered to whatever listens there), andpresign()emits a URL carrying the bogus port verbatim.HTTP_PROXY/HTTPS_PROXY: every proxied fetch is sent to port 80/443 of the proxy host.curl errors on these ("Port number was not a decimal number between 0 and 65535"), and Node with
NODE_USE_ENV_PROXY=1throwsERR_PROXY_INVALID_CONFIG.Fix
bun_url::URL::has_invalid_port(): true when the port text is non-empty and does not parse as a u16.HTTPThread::connectrejects before dialing:InvalidPortfor the request URL,InvalidProxyPortfor the proxy. This is the single choke point for every TCP connect (direct, proxied, custom TLS contexts, per redirect hop), so it also covers env-proxy re-resolution after a cross-scheme redirect and the install/registry paths. The unix-socket path is unaffected.S3Credentials::sign) rejects an endpoint host with an unparseable port withERR_S3_INVALID_ENDPOINT. This covers endpoints fromS3_ENDPOINT/AWS_ENDPOINTenv vars andpresign(), which never reaches the connect path.S3Clientendpointoption now throwsERR_INVALID_ARG_VALUE("endpoint port must be a number between 0 and 65535") at construction, matching howfetch(url, { proxy })rejects invalid proxy URLs.Fetch rejections get explicit messages: "Invalid port number in [proxy ]URL. Ports must be a number between 0 and 65535."
Tests
test/js/bun/s3/s3.test.ts: constructor and per-fileendpointoptions throw for all four port shapes, valid ports still construct and presign with the port intact, andS3_ENDPOINTenv var makespresign()throwERR_S3_INVALID_ENDPOINTinstead of emitting the URL.test/js/bun/http/proxy.test.ts:HTTP_PROXY/http_proxy/HTTPS_PROXYwith each bad-port shape rejects the fetch withInvalidProxyPortbefore any connection is attempted.All fail on the unfixed build (the proxy ones fail with
ConnectionRefusedfrom the silent port-80 dial; presign emits the URL) and pass with the fix. Also patched the four existing loopback-S3 fixtures in s3.test.ts to stop inheriting ambientHTTP_PROXYfrom the environment, which broke them in containers that set one.bun install --registry http://host:99999/already failed URL joining before this change; the connect-layer check adds depth there rather than changing behavior.[review] gate passed · iteration 0 · 8 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