Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 27 days. After that, they cost $0.25 per reviewed file. Or wait 23 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 9:29 PM PT - Aug 24th, 2026
✅ @robobun, your commit b3921981d93c0d464aa1f8d60175d980dcf466be passed in 🧪 To try this PR locally: bunx bun-pr 40067That installs a local version of the PR into your bun-40067 --bun |
|
Status: the change is ready. On the latest run (build 103449, ea26bf1, rebased onto main with #40062 and #40070) the new S3 test passes on every lane that ran it, and every red item is unrelated to this diff: the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — reorders parse_s3_endpoint to consult WTF::URL first so an endpoint with a scheme means exactly what new URL() says.
What was reviewed:
- Traced the new control flow against the old for scheme-less inputs (
localhost:9000,[::1]:9000,127.0.0.1:9000/prefix/), scheme-relative//host, ands3://bucket— all take the same path as before; only the four newly-tested spellings change. S3Endpoint::from_whatwgis a straight extraction of the previous inline WTF::URL→endpoint block plus an empty-host guard;whatwg::Parsedstill frees viaDropand theOwnedStringpattern is unchanged.- Both callers (
credentials_jsc.rs,env_loader.rs) only readhost_with_path/is_http, so no downstream adjustment needed.
Extended reasoning...
Overview
The PR touches one function, URL::parse_s3_endpoint in src/url/lib.rs, and its test file. It extracts the WTF::URL → S3Endpoint conversion into S3Endpoint::from_whatwg (adding an empty-host guard) and reorders the parse: try the WHATWG parser on the raw input first, and only fall back to Bun's slice-based URL::parse + https:// prepend when WTF::URL yields no host. Four new test cases (http:/…, http:…, http:///…, whitespace-padded) exercise the fix; four scheme-less cases and one no-host rejection are pinned to prevent regression.
Security risks
None material. The function decides which host a user-supplied S3 endpoint string names. The change makes that interpretation converge on new URL() semantics rather than diverge from it, so the set of hosts an input can name shrinks toward the standard rather than expanding. No new untrusted input surface, no credential handling touched, and both call sites (S3Credentials option parsing and S3_ENDPOINT/AWS_ENDPOINT env reading) already routed through this function.
Level of scrutiny
Medium. It is a small refactor of a single pure function with no unsafe, no FFI signature changes, and no memory-ownership changes — the same whatwg::Parsed RAII handle and OwnedString wrappers are used in the same order as before, just moved into a helper. The PR description is unusually thorough: it names each edge case (localhost:9000 reads as scheme localhost, http://:9/x must not be re-prefixed) and why the guard for it is shaped as it is. I walked each of those cases through both the old and new code and they line up.
Other factors
The one behavioral difference beyond the four fixed spellings is that inputs WTF::URL parses with an empty host (e.g. a data://x-shaped endpoint) now fall back to URL::parse instead of producing a host-less S3Endpoint; that is strictly less broken and not a plausible real endpoint. The extracted helper deduplicates what would otherwise be two copies of the hostname/pathname stitching, satisfying the "second time a block appears, extract it" rule. Tests are hermetic (presign is local, no network) and assert exact strings.
22bf34d to
ea26bf1
Compare
There was a problem hiding this comment.
LGTM — reorders parse_s3_endpoint to consult WTF::URL first so a schemed endpoint means exactly what new URL() says.
What was reviewed:
- Traced the three fall-through paths: WTF::URL-with-host wins; scheme-less inputs (
localhost:9000,[::1]:9000) still gethttps://prepended because WTF::URL yields an empty host;//hoststill returnsNoneviaURL::parse's emptyhost_with_path. - Checked the extracted
S3Endpoint::from_whatwgcopies everything into owned memory beforeParseddrops the C++WTF::URL; the empty-host guard is new but only replaces a previously-unusable empty result with theas_writtenfallback. - Four new test cases fail on main and pass here; scheme-less and no-host cases are pinned alongside.
Extended reasoning...
Overview
The PR reorders URL::parse_s3_endpoint in src/url/lib.rs so that the raw endpoint string is handed to WTF::URL (the WHATWG parser behind new URL()) first. If it parses with a non-empty host, that result is used directly. Only when WTF::URL yields no host does the function fall back to the existing URL::parse + optional https:// prepend. The duplicated host/pathname/protocol extraction is factored into S3Endpoint::from_whatwg. Tests add four schemed spellings that new URL() accepts (http:/…, http:…, http:///…, whitespace-padded) plus pin the scheme-less and no-host behaviours.
Security risks
parse_s3_endpoint decides the host that signed S3 requests are sent to, so a misparse is security-relevant. The change moves parsing closer to the WHATWG reference: previously http:/127.0.0.1:9 was misread as host http, which would send a signed request (and credentials) to whatever http resolves to. Now it goes to 127.0.0.1:9 as new URL() says. The //host rejection and the scheme-less default-to-https path are preserved and pinned by tests. No loosening of validation.
Level of scrutiny
Medium. The function is ~20 lines and the refactor is a reordering plus an extracted helper; every branch in the PR description is explained with a concrete input and the before/after presigned URL. I traced each case (localhost:9000 → WTF::URL scheme=localhost, empty host → falls through; //127.0.0.1:1 → WTF::URL fails, URL::parse empty host → None; http://:9/x → WTF::URL fails, URL::parse sees a protocol → no double-prepend, uses as_written). Memory: whatwg::Parsed is the RAII wrapper with Drop, and from_whatwg builds owned Vec<u8>/Box<[u8]> before the borrow ends — same pattern as the code it replaces. The one behaviour delta beyond the headline fix is that a WTF::URL result with an empty host now falls back to as_written instead of producing an empty host_with_path; that is strictly an improvement.
Other factors
The mechgate evidence shows the four new cases fail on main (ASAN debug) and pass with the fix, on both debug and release. The full test/js/bun/s3/ suite was run. No prior human review comments to address. The PR description's Notes section pre-emptively answers the two non-obvious design questions (why gate on "has a host" rather than "parses", and why keep the protocol.is_empty() guard on the prepend).
ea26bf1 to
938edec
Compare
There was a problem hiding this comment.
LGTM — reorders parse_s3_endpoint to consult WTF::URL first so schemed endpoints match new URL() exactly, with the scheme-less fallback unchanged.
What was reviewed:
- Traced the new WTF::URL-first path against each existing input class (
localhost:9000,s3://bucket, bare host,//host, IPv6) — all fall through to the same branch as before; only the four newly-tested spellings change. S3Endpoint::from_whatwgis a byte-for-byte lift of the previous inline extraction (samehostname()/pathname()calls, same trailing-slash strip), so no ownership or encoding change.- Checked that the empty-host guard on the WTF result is what keeps
localhost:9000(parsed as schemelocalhost, no host) on thehttps://default — the PR notes explain this and the newdefaults %j to httpscases pin it.
Extended reasoning...
Overview
The PR touches one function, URL::parse_s3_endpoint in src/url/lib.rs, and its test file. It reorders the two-parser flow: instead of asking the in-tree URL::parse slicer whether a scheme exists and only then handing to WTF::URL, it hands the input to WTF::URL first. If WTF::URL parses with a non-empty host, that result is authoritative (matching new URL()). Otherwise the old path — URL::parse, prepend https:// when scheme-less, fall back to the sliced host — runs unchanged. The extraction of host[:port][/prefix] and is_http from a whatwg::URL is factored into S3Endpoint::from_whatwg, which is the identical sequence of calls the removed inline code made.
Security risks
The endpoint decides where signed S3 requests (and their credentials) go, so misparsing is security-adjacent. This change tightens alignment with the WHATWG parser rather than loosening it: the four newly-accepted spellings (http:/host, http:host, http:///host, whitespace-padded) now go where new URL() says instead of to a host literally named http or being rejected. No new input is accepted that new URL() would reject, and the //host no-scheme case is still rejected (pinned by a new test). I don't see an injection or host-confusion vector introduced here.
Level of scrutiny
Low-to-medium. ~30 lines of Rust net, no unsafe, no new FFI, no allocation-lifetime changes — the helper returns owned String values via existing getters and the Parsed handle drops as before. The logic is a reorder plus an extract-method refactor. The PR description is unusually thorough, walks each edge case (why "parses with a host" not "parses", why the https:// prepend stays gated on URL::parse's protocol check), and shows fails-on-main / passes-on-PR evidence for the four new cases.
Other factors
Tests use presign() so nothing dials a port. The new it.each for scheme-less defaults and the explicit //127.0.0.1:1 rejection test pin the fallback path against future regressions from this reorder. The single caller in env_loader.rs is already covered by the existing S3_ENDPOINT child-process test. No prior review comments to address; CI status per robobun is green on the touched test across lanes.
parse_s3_endpoint asked URL::parse first whether the endpoint had a scheme, and only then handed it to WTF::URL. URL::parse recognizes a scheme only as "scheme://", so http:/host:9 and http:host:9 were read as the host "http", and " http://host:9 " was rejected as having no host. new URL() accepts all three and names host:9. Give the string to WTF::URL first. When it parses with a host, that is the endpoint. Otherwise it is host[:port][/prefix] with https as the default, and URL::parse decides whether there is a host, as before.
938edec to
b392198
Compare
There was a problem hiding this comment.
LGTM — focused reorder of parse_s3_endpoint so WTF::URL (the new URL() parser) gets first say when the endpoint has a scheme.
What was reviewed
- Traced the new WTF::URL-first path against scheme-less inputs (
localhost:9000,[::1]:9000,s3.example.com) — all yield an empty host in WTF::URL and fall through to the unchangedhttps://default, now pinned by tests. - Confirmed
S3Endpoint::from_whatwgis a byte-for-byte extraction of the old inline conversion plus the empty-host guard;is_httpand path-suffix trimming are unchanged. - Checked the fallback branch when
URL::parsesees a protocol: WTF::URL was already tried at the top, so skipping thehttps://prepend there matches the old behavior for inputs likehttp://:9/x. //127.0.0.1:1still returnsNone(host_with_path empty) — pinned by the new rejection test.
Extended reasoning...
Overview
This PR reorders URL::parse_s3_endpoint in src/url/lib.rs so the input is handed to WTF::URL (the WHATWG parser behind new URL()) first, and only falls back to Bun's slice-based URL::parse when WTF::URL yields no host. The WTF::URL → S3Endpoint conversion is extracted into S3Endpoint::from_whatwg, which is now called from both the direct-parse and the https://-prepend paths. Net diff is ~30 lines of Rust plus 22 lines of test additions in test/js/bun/s3/s3-list-encode-overflow.test.ts.
Security risks
None identified. The S3 endpoint is user-supplied configuration (via the endpoint option or S3_ENDPOINT/AWS_ENDPOINT env), not untrusted network input, so there is no SSRF surface change — the user already controls where requests go. The change makes parsing more consistent with new URL(), which is what a user reading the docs would expect. Inputs that were previously rejected (//host) remain rejected; inputs that were previously misparsed as host http now resolve to the intended host, which is strictly a correctness improvement.
Level of scrutiny
Medium. parse_s3_endpoint decides which host signed S3 requests connect to, so a mis-parse could send credentials to the wrong place. However the change is a small, mechanical reorder of an existing two-parser flow, the extracted helper is a faithful copy of the prior inline code (verified line-by-line), and the PR description walks through each edge case (localhost:9000 as opaque scheme, http://:9/x fallback, s3://bucket, IPv6) with before/after probes. The new tests pin both the fixed cases and the unchanged scheme-less/rejection cases, and the gate evidence shows the four new cases fail on main and pass with the fix.
Other factors
- No prior human or bot review comments to address; CI status per robobun is green with only unrelated flakes.
- The refactor deduplicates the WTF::URL result handling into one helper rather than two inline copies, satisfying the "same block twice → extract" rule.
- The one behavioral subtlety — why the
https://prepend is still gated onURL::parseseeing no protocol rather than always applied — is explained in the PR notes and I verified it: prepending ontohttp://:9/xwould make WTF::URL readhttpas the host, so keeping the gate preserves today's fallback. - No CODEOWNERS entry covers
src/url/.
|
This also fixes a silent fallback in the env path. With this change WTF::URL strips the whitespace and the env value is used. The option form ( |
This branch has not been deployed
2 participants
Problem
new URL()accepts still go elsewhere.http:/127.0.0.1:9andhttp:127.0.0.1:9are read as the hosthttpand fail withDNSResolveFailedafter a resolver query.http://127.0.0.1:9is rejected withERR_INVALID_ARG_TYPE.new URL()reads all three as127.0.0.1:9. Same on 1.4.0.URL::parse_s3_endpoint(src/url/lib.rs) asksURL::parsefirst whether the endpoint has a scheme, and hands the string to WTF::URL only after that.URL::parserecognizes a scheme only asscheme://. With one slash or none it takeshttp:for the host, sohttps://is prepended and WTF::URL readshttpas the host. Leading whitespace makes it find no host at all.Fix
host[:port][/prefix]withhttpsas the default, andURL::parsedecides whether there is a host, as before.new URL(), so an endpoint with a scheme now means exactly whatnew URL(endpoint)says. A scheme-less endpoint (localhost:9000,s3.example.com,[::1]:9000) has no host to WTF::URL and takes the old path unchanged. An endpoint without a host (//127.0.0.1:9) is still rejected.test/js/bun/s3/s3-list-encode-overflow.test.ts, four new cases fail on main and on 1.4.0. The scheme-less and no-host cases are pinned as well.test/js/bun/s3/: 163 pass, 2 pre-existing flakes (notes).Background
S3Credentials.endpointstoreshost[:port][/prefix].sign_requestsigns the part before the first/as the host and connects to it, so whatparse_s3_endpointstores decides where signed requests go.new URL().bun_url::URL::parseslices an href that is already normalized, and it is the only one that accepts a scheme-lesshost:port.must be of type string. Received type string. The codeERR_INVALID_ARG_TYPEis pinned by two tests ins3.test.ts, so this change leaves it alone.Notes
Probe on main (
d95bc353ee) and on release 1.4.0, presigned URL without the query:With the change the first four give
http://127.0.0.1:1/b/k, the last one still throws.localhost:9000,s3.example.com,bucket.test.r2.cloudflarestorage.com,[::1]:9000,127.0.0.1:9000/prefix/ands3://bucketproduce the same presigned URL before and after.Why "parses with a host" and not "parses": WTF::URL reads
localhost:9000as the schemelocalhostwith the opaque path9000, andmy-host:9000/xthe same way. Both have an empty host, so they fall through to thehttpsdefault as before.Why
https://is still prepended only whenURL::parsesees no scheme: an input such ashttp://:9/xfails in WTF::URL and has the schemehttptoURL::parse. Prepending would turn it intohttps://http://:9/x, which WTF::URL reads as hosthttp. Leaving the prepend out keeps the fallback at whatURL::parsereads, as today.S3_ENDPOINTandAWS_ENDPOINTgo through the same function (src/dotenv/env_loader.rs) and are covered by the existing child-process test.The two failures in the S3 suite run are not related:
s3-list-objects.test.ts"Should fall back to NoSuchKey" hits its 5 s budget next to an 18 s test in the samedescribe.concurrentblock under ASAN (passes alone), ands3.test.ts"uploads a fetch response body via the native ByteStream" lost a part's byte count toreceived += (await ...)in one of four runs (#37205 fixes that test).Rebased over #40238 (
bun_core::Stringowns its WTF ref) and #40374 (eq_ascii):S3Endpoint::from_whatwgcalls thewhatwg::URLgetters directly and compares the scheme witheq_ascii, the same changes main made to the inline code this PR moves into the helper. No other conflict.[review] gate passed · iteration 4 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 4
evidence per changed file