Repository navigation
Conversation
The hostname setter is spec-defined as the host state with state override
= hostname state, which returns without setting anything when it meets a
":" outside "[...]". WTF::URL::setHost only rejects a ":" when the value
does not start with "[", so a bracketed IPv6 literal followed by ":port"
reached the reparse and filled the port slot:
const u = new URL("http://a/p");
u.hostname = "[::1]:81";
u.href; // bun: "http://[::1]:81/p", spec/Node: "http://a/p"
Scan the value in URLDecomposition::setHostname, skipping ASCII
tab/newline and stopping at the host terminators WTF::URL::setHost
already truncates at, and return early on a ":" outside brackets.
|
Status: closed by the inactivity sweep on 2026-10-26, not merged. Not reopened. The bug still reproduces on
The fix belongs in Reproduce: create |
WalkthroughChangesThe hostname setter now scans host input for IPv6 bracket context and rejects out-of-bracket colons before calling Hostname setter behavior
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/url/url.test.ts`:
- Around line 204-211: Extend the URL test table around the existing HTAB and
backslash cases to cover CR, LF, and CRLF whitespace before the port delimiter,
plus backslash scanning when the URL uses a non-special scheme (special ==
false). Preserve the expected host/port parsing behavior and include explicit
CRLF input coverage.
- Around line 194-223: Replace the parameterized `it.each` suite for hostname
setter cases with `describe.each`, moving the shared test logic into a nested
regular `it` while preserving every case, title, and assertion. Verify the
updated test with the repository’s specified command.
🪄 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: be190c45-3706-470b-aa77-f832f2052182
📒 Files selected for processing (2)
src/jsc/bindings/URLDecomposition.cpptest/js/web/url/url.test.ts
|
No open issues found that this PR would fix. 🤖 Generated with Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 11:16 AM PT - Jul 27th, 2026
❌ @robobun, your commit 10f551c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36118That installs a local version of the PR into your bun-36118 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the scan loop is back at the branch tip (blob 7a07a0f91d2, commit 5f143d5), so my earlier concern is resolved.
What was reviewed:
- Cross-checked all 22 test expectations against Node — every one matches.
- Confirmed the fix is scoped to
setHostnameand leavessetHost(which should parse ports) untouched. - Verified
hasSpecialScheme()is already used elsewhere in this file, and the terminator set (/ ? # \\-on-special) plus tab/newline skipping matches the URL spec's host state.
Extended reasoning...
Overview
Two files: an 18-line pre-scan in URLDecomposition::setHostname (src/jsc/bindings/URLDecomposition.cpp) that returns early on a : outside [...] before forwarding to WTF URL::setHost, and a 22-row it.each table in test/js/web/url/url.test.ts covering the no-op cases, the terminator-before-colon cases, tab/CR/LF stripping, special vs non-special \\, and the still-accepted plain-bracket cases.
History on this PR
I previously flagged that commit 00d8d09 accidentally deleted the entire scan loop while trimming a comment, leaving the branch tip with tests but no implementation. Commit 5f143d5 restored it; git ls-tree HEAD now shows blob 7a07a0f91d2 for URLDecomposition.cpp, which is the intended post-fix content, and the net diff vs base is +18/+39 as expected.
Security risks
URL host parsing is SSRF-adjacent, but this change is strictly more restrictive: inputs like [::1]:81 that previously mutated both host and port now no-op, matching the WHATWG spec and Node/browsers. It cannot introduce a new host/port confusion — it removes one. I spot-checked seven representative rows against Node and all agree.
Level of scrutiny
Medium. It's a small, spec-cited change with a clear mechanism (the WTF setHost colon guard is bypassed by a leading [, so the caller pre-scans per the spec's host-state-with-override rule). The scan is placed at the correct layer — setHostname is the only setter with the "never touch port" invariant, and WTF URL::setHost is shared with the host setter which does parse ports, so patching there would be wrong.
Other factors
All review threads are resolved: comment-cop's length nag (comment reduced to a one-line spec reference matching the parsePort style in the same file), CodeRabbit's CR/LF and non-special-\\ coverage request (rows added in 3fcf5cd), and my dropped-implementation catch (restored in 5f143d5). The author declined the describe.each refactor, which is fine — it.each with a single assertion is idiomatic here. Fail-before/pass-after was verified (USE_SYSTEM_BUN=1: 11 fail; bun bd: 40 pass). The overlap with #32826 is noted in the PR body — this is the minimal targeted fix and #32826 would supersede it if it lands.
There was a problem hiding this comment.
LGTM — the scan loop is back at HEAD (blob 7a07a0f91d2, restored in 5f143d5), so my earlier concern is resolved.
What was reviewed:
- The
setHostnamebracket-tracking scan matches the WHATWG host-state rule (tab/newline skip, terminator break,:-outside-brackets return); the siblingsetHostis intentionally left alone since thehostsetter is allowed to write the port. - Test table (22 cases) covers special/non-special schemes, existing-port preservation, tab/CR/LF stripping, all four terminators, and the positive path (plain
[::1]still applies) — all cross-checked against Node. - The change strictly tightens behavior (no-op instead of port leak), so no new parse surface.
Extended reasoning...
Overview
Adds an 18-line pre-scan to URLDecomposition::setHostname (src/jsc/bindings/URLDecomposition.cpp) that returns early when the input contains a : outside an IPv6 [...] bracket pair, before delegating to WTF URL::setHost. This closes a hole in WTF's colon guard (contains(':') && !startsWith('[')), which let [::1]:81 reach the reparse and write the port. Adds a 22-row it.each table to test/js/web/url/url.test.ts with expected values verified against Node.
The branch went through some churn: 00d8d09 accidentally deleted the whole scan while trimming a comment (which I flagged in the previous review pass), and 5f143d5 restored it. I confirmed HEAD's blob for URLDecomposition.cpp is 7a07a0f91d2 — identical to what the PR diff shows — so the net change is now correct.
Security risks
URL parsing is security-adjacent (host/port confusion can enable SSRF or origin bypass), but this change moves behavior toward the spec by making the hostname setter refuse to touch the port when handed [v6]:port. It removes an unintended side-channel for port mutation rather than adding parse surface. The scan reads a bounded StringView character-by-character with no allocation, no arithmetic on untrusted lengths, and no memory-safety exposure.
Level of scrutiny
Medium-low. It is a targeted spec-compliance fix in a well-understood WebCore shim, with a single-line spec citation, a clear mechanical explanation of the WTF guard hole, and fail-before/pass-after verification (USE_SYSTEM_BUN=1 fails 11 of the new cases; bun bd passes 40/40). The loop mirrors the same shape already used in parsePort in the same file. The sibling URLDecomposition::setHost correctly does not get this guard because the host setter is spec-permitted to set the port.
Other factors
All review threads are resolved: the two comment-cop nags were addressed by trimming to a one-line spec reference, CodeRabbit's CR/LF and non-special-backslash coverage request landed in 3fcf5cd, and the it.each → describe.each suggestion was reasonably declined (matches file convention). PR #32826 is noted as a broader rewrite that would supersede this, but this minimal fix is independently landable.
|
Closing this PR because it has been inactive for more than 90 days. |
|
The inactivity sweep closed this PR. No review decision closed it. The bug is still present. On const u = new URL("http://a/p");
u.hostname = "[::1]:81";
u.href; // "http://[::1]:81/p". Node 26.3.0 gives "http://a/p".I will link the follow-up here when the fix is ready again. |
What
url.hostname = "[::1]:81"was writing both the host and the port. Thehostnamesetter is the one place spec-guaranteed never to touch the port, so this silently retargeted requests.Reproduces on any special scheme with a bracketed IPv6 literal followed by
:<anything>, as long as the URL has no existing port (an existing port made the recomposed string unparsable, which accidentally no-opped).Why
URLDecomposition::setHostnameforwards to WTFURL::setHost, whose colon guard is:A value that starts with
[bypasses the guard entirely, so[::1]:81reaches the reparse ofleft(hostStart) + "[::1]:81" + substring(m_hostEnd)and:81lands in the port slot.Per the URL Standard's host state with state override = hostname state, a
:outside the IPv6 brackets returns immediately without setting anything.Fix
Scan the value in
URLDecomposition::setHostnamethe way the spec state machine does: skip ASCII tab/newline, stop at the host terminators (/,?,#, and\\for special schemes) thatURL::setHostalready truncates at, and return early on the first:seen outside[...].Verification
18 new cases in
test/js/web/url/url.test.ts, all cross-checked against Node: the 8 that previously leaked a port now no-op, and the 10 that were already correct (plain[::1], terminator-before-colon, non-bracketedh:81) are kept as regression guards.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 2
evidence per changed file