Repository navigation
Conversation
|
Updated 2:37 AM PT - May 6th, 2026
❌ @robobun, your commit 91bdf27 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 28662That installs a local version of the PR into your bun-28662 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDelegated host/port parsing from manual logic to Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28661.test.ts`:
- Around line 1-38: Add the standard single-line GitHub issue URL comment at the
very top of this test file (before the import and the test declaration) to match
the repo pattern; update the file containing the test named "URL.host setter
replaces port when set to default port" by inserting a one-line comment like "//
https://github.com/oven-sh/bun/issues/<issue-number>" so the regression test is
traceable to the original issue.
🪄 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: 5c7352f2-8152-4a9e-b17f-40700885e82c
📥 Commits
Reviewing files that changed from the base of the PR and between 17616ae and cf2b0711ab44a13d933e16b28f59e4b1702de0da.
📒 Files selected for processing (2)
src/bun.js/bindings/URLDecomposition.cpptest/regression/issue/28661.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean simplification that replaces buggy custom port-parsing logic with the upstream WebKit setHostAndPort call, with a solid regression test.
Extended reasoning...
Overview\nFixes URL.host setter not clearing an existing non-default port when the new value specifies a default port (e.g. some-domain:80 on an HTTP URL). Two files changed: the C++ binding and a new regression test.\n\n### Security Risks\nNone. This is a web-standards compliance fix for URL parsing with no auth, crypto, or permission implications.\n\n### Level of Scrutiny\nLow. The fix is a net deletion of ~35 lines of custom, subtly-buggy port-parsing logic replaced by a single call to URL::setHostAndPort(value), which is the upstream WebKit implementation and already handles IPv6, default-port stripping, and other edge cases. The regression test covers the reported case, non-default ports, host-only assignment, and FTP.\n\n### Other Factors\nThe removal of the hasOpaquePath() guard from setHostname is a minor collateral change; WebKit's URL::setHost handles opaque paths gracefully internally, so the guard is redundant. No outstanding reviewer comments. Build triggered.
There was a problem hiding this comment.
LGTM — clean fix that removes custom port-parsing logic in favor of delegating to WebKit's setHostAndPort, which correctly handles the default-port case.
Extended reasoning...
Overview
Touches two files: src/bun.js/bindings/URLDecomposition.cpp (removes ~34 lines of custom port-parsing logic, replaces with a single fullURL.setHostAndPort(value) delegation) and a new regression test test/regression/issue/28661.test.ts.
Security risks
None. This is pure URL parsing behavior — no auth, crypto, or permission code is touched.
Level of scrutiny
Low. The change is a simplification: removing a divergent re-implementation of port handling and deferring to the upstream WebKit URL::setHostAndPort method, which already handles default-port stripping, IPv6, and edge cases through the URL parser. The root cause (calling setHostAndPort(hostname_only) on default ports left the old port intact) is clearly documented and the fix is minimal.
Other factors
The regression test covers all key scenarios: HTTP port 80, HTTPS port 443, FTP port 21 (default ports stripped), non-default ports preserved, and hostname-only assignment preserving the existing port. coderabbitai's nitpick about the issue URL comment was already addressed in commit 0432473. No outstanding unresolved comments.
There was a problem hiding this comment.
LGTM — clean simplification that fixes the bug by delegating to WebKit's correct setHostAndPort implementation.
Extended reasoning...
Overview
This PR modifies src/bun.js/bindings/URLDecomposition.cpp to fix URL.host setter behavior when assigned a value containing a default port (e.g., some-domain:80 for HTTP). It removes ~30 lines of custom port-parsing logic and replaces it with a single call to fullURL.setHostAndPort(value), aligning with upstream WebKit. A regression test is added at test/regression/issue/28661.test.ts.
Security risks
None. This is a URL string parsing fix with no auth, crypto, or permissions involvement.
Level of scrutiny
Low. The change is a net code deletion — removing a bespoke implementation in favor of the battle-tested WebKit upstream. The bug being fixed (old port surviving a host setter call) is clearly demonstrated and the fix is obviously correct. The removal of the hasOpaquePath() early-return in setHostname is safe since URL::setHost handles that case internally in WebKit.
Other factors
All CodeRabbit feedback was addressed. No outstanding reviewer comments. The regression test is comprehensive, covering HTTP/HTTPS/FTP default ports, non-default ports, and hostname-only assignment.
18866bf to
1d6a109
Compare
There was a problem hiding this comment.
LGTM — clean fix that delegates to instead of reimplementing default-port-stripping logic.
Extended reasoning...
Overview
The PR modifies and adds a regression test in . The C++ change removes a custom helper and ~30 lines of manual port-parsing logic in , replacing them with a single delegation to . It also removes the redundant early-return guard in , as upstream WebKit's handles that internally.
Security risks
None. This is a URL parsing fix with no auth, crypto, or permission implications.
Level of scrutiny
Low. The fix is a straightforward simplification — removing divergent custom logic in favor of the well-tested upstream WebKit implementation. The before/after behavior difference is narrow and precisely targeted at the default-port-stripping bug. The regression test covers HTTP, HTTPS, FTP, non-default ports, and hostname-only assignments.
Other factors
CI failures (webview timeout on macOS aarch64, v8-heap-snapshot SIGKILL on Ubuntu) are pre-existing flakes unrelated to URL parsing. No bugs were found by the automated bug hunter. The CodeRabbit nitpick (add issue URL comment) was addressed by the author.
1d6a109 to
7c0e657
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28661.test.ts`:
- Around line 4-39: Add a single bracketed IPv6 case to the test to cover the
parser edge for `]`/colon handling: create a new URL (e.g., variable like u6)
from "http://[::1]:3000/foo", set its host to "[::1]:80", and assert that href
becomes "http://[::1]/foo", host becomes "[::1]" and port becomes "" so the
default-port stripping exercised by setHostAndPort() is validated for IPv6
bracketed addresses.
🪄 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: d1831573-be16-4aca-82cb-801cbb9d89c9
📥 Commits
Reviewing files that changed from the base of the PR and between 1d6a109f4830608ae9c055c91663f3c23579d009 and 7c0e6575a6d98c6dd87d69cda791fc4a454e4b9f.
📒 Files selected for processing (2)
src/bun.js/bindings/URLDecomposition.cpptest/regression/issue/28661.test.ts
There was a problem hiding this comment.
LGTM — clean simplification that delegates to WebKit's URL parser, fixing the default-port stripping bug correctly.
Extended reasoning...
Overview
The PR modifies src/bun.js/bindings/URLDecomposition.cpp and adds test/regression/issue/28661.test.ts. It removes ~35 lines of custom host/port parsing from URLDecomposition::setHost (including the countASCIIDigits helper) and replaces the entire block with a single fullURL.setHostAndPort(value) call. It also removes the hasOpaquePath() early-return guard from setHostname that was duplicating logic already handled by the upstream WebKit URL parser.
Security risks
None. This is a pure correctness fix on the URL decomposition API. It delegates to a well-tested WebKit primitive rather than re-implementing port parsing, which actually reduces the attack surface compared to the bespoke code it replaces.
Level of scrutiny
Low. The change is a mechanical simplification — delete custom logic, call the upstream function that was always intended to handle this. The regression test covers HTTP/HTTPS/FTP default ports, non-default ports, and hostname-only assignments. The fix is already in the main branch (commit 7c0e657).
Other factors
No bugs were flagged by the automated bug-hunting system. CodeRabbit's only comment (adding the standard issue URL comment) was addressed before merge. The change is self-contained with no CODEOWNER-sensitive paths.
7c0e657 to
00fb9db
Compare
There was a problem hiding this comment.
LGTM — clean delegation to upstream WebKit's setHostAndPort fixes the default-port bug with no new logic introduced.
Extended reasoning...
Overview
Two files changed: URLDecomposition.cpp (bug fix) and a new regression test. The fix removes ~35 lines of custom port-parsing logic in URLDecomposition::setHost and replaces it with a single call to fullURL.setHostAndPort(value). Additionally removes an opaque-path early-return from setHostname to align with upstream WebKit.
Root Cause Correctness
The old code, when the parsed port was a default port, stripped the port and called setHostAndPort(hostname_only) — this correctly changed the hostname but left the previous URL's port untouched because no port was passed. The fix delegates the entire value to setHostAndPort, which runs through WebKit's URL parser and handles default-port stripping, IPv6 brackets, and all edge cases.
Security Risks
None. This is URL parsing logic with no auth, crypto, or permission implications.
Level of Scrutiny
Low — the change is a simplification that removes custom logic in favor of a well-tested upstream implementation. The behavioral difference is exactly the bug being fixed. The opaque-path guard removal in setHostname is a minor alignment with upstream.
Other Factors
No bugs found by the automated system. The regression test is comprehensive (HTTP/HTTPS/FTP default ports, non-default ports, hostname-only, IPv6). CodeRabbit suggestions (issue URL comment, IPv6 case) were all addressed by the author. The PR is self-contained and the fix is obviously correct.
Setting URL.host to a value with a default port (e.g. 'some-domain:80' for HTTP, 'some-domain:443' for HTTPS) incorrectly preserved the old port instead of replacing it. Align with upstream WebKit by delegating directly to URL::setHostAndPort(value), which already handles default port stripping, IPv6, and all edge cases internally via the URL parser. Closes #28661
00fb9db to
91bdf27
Compare
There was a problem hiding this comment.
LGTM — aligns setHost/setHostname with upstream WebKit and adds solid regression coverage.
Extended reasoning...
Overview
This PR fixes #28661 by replacing Bun's custom port-parsing logic in URLDecomposition::setHost with a direct call to WTF::URL::setHostAndPort(), and removes a redundant opaque-path early-return from setHostname. It also removes the now-unused countASCIIDigits helper and StringToIntegerConversion.h include. A regression test covers HTTP/HTTPS/FTP default ports, non-default ports, hostname-only assignment, and bracketed IPv6.
Verification against upstream
I diffed the new implementations against current upstream WebKit (Source/WebCore/html/URLDecomposition.cpp) and they match line-for-line. The removed hasOpaquePath() checks are safe to drop because WTF::URL::setHostAndPort and WTF::URL::setHost both early-return on opaque paths internally (verified in Source/WTF/wtf/URL.cpp).
Security risks
None. This removes custom string-parsing logic in favor of the battle-tested WTF URL parser, which reduces the surface for parsing inconsistencies rather than adding any.
Level of scrutiny
Low-to-moderate. URL parsing is spec-sensitive, but this change removes divergent custom code and converges on upstream WebKit behavior, which is the source of truth for WHATWG URL compliance. The regression test assertions match Node.js / WHATWG spec behavior (including the u4 case where setting host without a port preserves the existing port).
Other factors
All CodeRabbit suggestions (issue URL comment, IPv6 test case) were addressed and resolved. The bug-hunting system found no issues. The PR description includes before/after verification with system Bun vs. patched Bun.
|
CI failures are pre-existing flaky tests on main, not caused by this PR:
This PR only modifies |
|
Superseded by #32826, which covers this case plus four sibling divergences in the same setters (out-of-range port handling, file host state, the port setter's tab/newline handling, and the hostname state's colon rule). Delegating to |
Bug
Setting
URL.hostto a value with a default port (e.g."some-domain:80"for HTTP,"some-domain:443"for HTTPS) incorrectly preserved the old port instead of replacing it.Root Cause
Bun's
URLDecomposition::setHosthad custom port-parsing logic that diverged from upstream WebKit. When the port was a default port, it stripped the port string and calledsetHostAndPort(hostname_only), which only updated the hostname and left the old port intact.Fix
Align with upstream WebKit by delegating directly to
URL::setHostAndPort(value), which already handles default port stripping, IPv6, and all edge cases internally via the URL parser.Verification
USE_SYSTEM_BUN=1 bun test test/regression/issue/28661.test.ts→ FAIL (bug present)bun bd test test/regression/issue/28661.test.ts→ PASS (fix works)Closes #28661