S3: read the endpoint the way new URL() reads it - #39933
Conversation
The S3 endpoint option and the S3_ENDPOINT / AWS_ENDPOINT variables went straight to bun_url::URL::parse, whose authority does not end at "#", "\" or an unencoded "@" in the credentials. An endpoint such as http://a:1#@b:2 therefore signed for and connected to b:2, while new URL(endpoint).host says a:1. Dot segments and IPv4 shorthand in the endpoint were sent as written. Add URL::from_s3_endpoint, which keeps the existing host check and then normalizes the endpoint with WTF::URL (https when no scheme is given, the input as written when WTF::URL rejects it), and use it at both places that store an endpoint.
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 2 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (4)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Slicing the normalized href with URL::parse again inherited its user@host:port reading, so http://user:@host:1 (which WTF::URL serializes as http://user@host:1/) stored user@host:1 where it used to store host:1. Read the scheme, host-with-port and path from the parsed WTF::URL instead, which never puts credentials in the host.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 4:12 AM PT - Aug 21st, 2026
✅ @robobun, your commit af03ddac2751a1c688b2eb9bacdf65ffec835974 passed in 🧪 To try this PR locally: bunx bun-pr 39933That installs a local version of the PR into your bun-39933 --bun |
There was a problem hiding this comment.
I re-reviewed after 1d52512/af03dda and didn't find further issues — the user:@ / user@ regression is fixed by reading host+port and path straight from WTF::URL, and both spellings are in the test matrix. Because this changes which host receives signed S3 requests and has documented side effects on the Host header (default-port stripping, host lowercasing), a human sign-off is still worthwhile.
What was reviewed:
parse_s3_endpoint: gate viaURL::parseunchanged, WTF::URL components read directly (no href round-trip), fallback path preserves today's behaviour for inputs WTF rejects.whatwg::ParsedRAII wrapper —Dropdeletes the heapWTF::URLonce;OwnedStringwraps each getter result so the returnedBunStrings arederef()ed.- Both call sites (
credentials_jsc.rs,env_loader.rs) and theS3_ENDPOINTenv path covered by the new tests.
Extended reasoning...
Overview
The PR routes S3 endpoint parsing through WTF::URL (the WHATWG parser behind new URL()) so the stored host[:port][/prefix] matches what new URL(endpoint) reports. It adds URL::parse_s3_endpoint and an RAII whatwg::Parsed handle in src/url/lib.rs, wires both endpoint stores (credentials_jsc.rs for the JS option, env_loader.rs for S3_ENDPOINT/AWS_ENDPOINT) through it, and adds a presign-based test matrix in s3-list-encode-overflow.test.ts.
Security risks
This is a security fix for a parser-differential issue: today bun_url::URL::parse and new URL() disagree on #@, \\@, multi-@ userinfo, dot segments and non-canonical IPv4, so a service that validates a tenant endpoint with new URL() can have Bun send signed requests (with Authorization and x-amz-security-token) to a different host. The change decides which host receives those credentials, which is exactly why it warrants human eyes. The earlier revision's user:@host:port regression (my prior comment) is fixed — the helper no longer re-parses the WTF href with URL::parse; it reads protocol(), hostname() (host+port) and pathname() directly and both user:@ and user@ are asserted in the test.
Level of scrutiny
High. Beyond the security surface, the PR documents visible behaviour changes to the SigV4 Host header (default port omitted, host lowercased) and adds a public whatwg::Parsed FFI wrapper with unsafe Deref/Drop. Those are reasonable but a maintainer should confirm the Host-header changes are acceptable for known S3-compatible backends.
Other factors
I checked memory ownership on the new path: whatwg::Parsed deletes the heap WTF::URL exactly once in Drop, and each url.protocol()/hostname()/pathname() result is wrapped in OwnedString whose Drop calls .deref(), so no BunString leaks. The URL::parse gate is kept unchanged so previously-rejected endpoints (the s3.test.ts emoji/garbage cases) still throw, and the raw fallback preserves today's behaviour for inputs WTF::URL rejects (e.g. :99999, deferred to #37003). All prior review threads on this PR are resolved.
2 participants
Problem
new Bun.S3Client({ endpoint: "http://127.0.0.1:A#@127.0.0.1:B" })signs for127.0.0.1:Band sendsPUT /bkt/k,Authorizationand the body there.new URL(endpoint).hostis127.0.0.1:A.http://A\@Bbehaves the same. A service that checks a tenant's endpoint withnew URL()sends signed requests to a host it did not approve.credentials_jsc.rs:119andenv_loader.rs:281give the raw string tobun_url::URL::parse, whose credential and host scanning stop only at/and?, and store itshost_with_path(). Dot segments and127.1are sent as written for the same reason.Fix
URL::parse_s3_endpoint(src/url/lib.rs) keeps the existingparsehost 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 whatparsereads, as today. Both stores call it.#,\and the last@, resolves dot segments, canonicalizes the host and never puts credentials in it. The stored form is unchanged, soguess_region, inspect output and path-prefix endpoints still work. The notes list the visible side effects.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. Alsotest/js/bun/s3/: 156 pass, 1 timeout (notes).Background
S3Credentials.endpointstoreshost[: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 asHost. The stored bytes are both signed for and connected to.new URL().bun_url::URL::parseslices an href that is already normalized.URL::from_stringandOwnedURLbridge the two.URL::parseitself foruser@host:portand keeps#in the authority on purpose. The two changes are independent. Abouturl.zig#16183 is the umbrella issue for that parser.Notes
Repro on release 1.4.0 with two
Bun.servelisteners A and B on 127.0.0.1 (run withoutHTTP_PROXYset, since S3 ignoresNO_PROXY, #32045):presign() on 1.4.0 for the spellings in the test:
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 whatnew URL()reports for it.S3_ENDPOINTandAWS_ENDPOINT(env_loader.rs) had the same problem and get the same treatment; the test coversS3_ENDPOINTin a child process.The test compares the presigned URL as text.
new URL(presigned)would itself strip a leakedss@, resolve/x/../and canonicalize127.1, and so hide the credential, dot-segment and127.1cases.Why the
parsehost check is kept:s3.test.tsexpectsendpoint: "🙂.🥯"and"..asd.@%&&&%%"to throwERR_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 theHostheader, 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 ands3.us-west-1.amazonaws.comendpoints ins3.test.ts(stored byte for byte as before), and a Supabase stylehttps://host/storage/v1/s3prefix (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 ...". Itsdescribe.concurrentblock 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 forserver.url.hrefis byte-identical with and without the change.bun_s3_signingdoes not depend onbun_url, andbun_dotenvmust not namebun_s3_signingtypes, so the helper lives inbun_urlnext tohost_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::parseagain. Review caught that this inherited theuser@host:portreading ofURL::parse(#16181): WTF::URL serializeshttp://user:@h:1ashttp://user@h:1/, so that endpoint would have storeduser@h:1where it storesh:1today. The helper now reads scheme, host with port and path from the WTF::URL components directly, and theuser:@anduser@spellings are in the test. The second one is wrong on 1.4.0 as well.[review] gate passed · iteration 0 · 4 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