Fix url.format() only replacing first colon in auth credentials by robobun · Pull Request #28752 · oven-sh/bun

robobun · 2026-04-01T18:07:47Z

Repro:

const url = require('url');
const user = encodeURIComponent('us:er');
const password = encodeURIComponent('pass:word');
const uri = 'http://' + user + ':' + password + '@localhost/';
const formatted = url.format(url.parse(uri));
// Bun:  http://us:er%3Apass%3Aword@localhost/
// Node: http://us:er:pass:word@localhost/

Cause: In Url.prototype.format, the auth field is run through encodeURIComponent and then colons are restored with .replace(/%3A/i, ':'). The regex was missing the g flag, so only the first %3A was replaced — any additional colons stayed percent-encoded.

Fix: Add the g flag: /%3A/gi.

Verification:

USE_SYSTEM_BUN=1 bun test test/regression/issue/28751.test.ts → fails (bug present)
bun bd test test/regression/issue/28751.test.ts → passes (fix works)

claude

⚠️ Code review skipped — your organization's overage spend limit has been reached.

Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.

Once credits are available, push a new commit or reopen this pull request to trigger a review.

robobun · 2026-04-01T18:08:00Z

^{Updated 1:42 PM PT - Apr 1st, 2026}

❌ @robobun, your commit aeadc9e has 6 failures in Build #43126 (All Failures):

test/js/bun/webview/webview.test.ts - timeout on 🍎 14 aarch64
test/js/bun/webview/webview.test.ts - timeout on 🍎 13 aarch64
test/js/bun/webview/webview.test.ts - timeout on 🍎 13 x64
test/js/bun/webview/webview.test.ts - timeout on 🍎 14 x64
test/regression/issue/28632.test.ts - code 1 on 🐧 13 x64-asan
test/js/third_party/@azure/service-bus/azure-service-bus.test.ts - SIGILL on 🐧 13 x64-asan
test/js/node/worker_threads/worker_threads.test.ts - pid 19600 segmentation fault on 🐧 25.04 x64-baseline
test/js/bun/jsc-stress/jsc-stress.test.ts - pid 2474 error: addresssanitizer on 🐧 13 x64-asan
test/js/bun/webview/webview-chrome.test.ts - code 1 on 🍎 14 aarch64

🧪 To try this PR locally:

bunx bun-pr 28752

That installs a local version of the PR into your bun-28752 executable, so you can run:

bun-28752 --bun

coderabbitai · 2026-04-01T18:09:47Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 24be2137-a266-44b7-ab92-297f3760789d

📥 Commits

Reviewing files that changed from the base of the PR and between 5646fa1 and aeadc9e.

📒 Files selected for processing (2)

src/js/node/url.ts
test/regression/issue/28751.test.ts

Walkthrough

Auth normalization in Url.prototype.format now unescapes all case-insensitive %3A sequences and query # characters are percent-encoded globally. A regression test was added to verify decoded colons in formatted auth and that search # characters become %23.

Changes

Cohort / File(s)	Summary
URL formatting logic `src/js/node/url.ts`	In `Url.prototype.format`, replace all case-insensitive `%3A` occurrences using a global regex (`/%3A/gi`) when normalizing `auth`; encode all `#` characters in `search` globally (`search.replace(/#/g, "%23")`).
Regression test `test/regression/issue/28751.test.ts`	Added tests: one verifies `url.parse()` decodes percent-encoded colons in `auth` and `url.format()` outputs decoded `user:pass` in authority; another verifies `Url` with `search` containing `#` returns query with `#` percent-encoded as `%23`.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly describes the main bug fix—replacing only the first colon in auth credentials—which matches the primary change in the code.
Description check	✅ Passed	The description includes reproduction steps, root cause analysis, the fix applied, and verification instructions, aligning with the template structure.
Linked Issues check	✅ Passed	The code changes directly address issue `#28751`: fixing the regex in url.format() to use the global flag (/%3A/gi) and percent-encoding all # characters in search, with regression tests validating both fixes.
Out of Scope Changes check	✅ Passed	All changes are scoped to the linked issue: fixing the colon-replacement regex in auth normalization and encoding # in search strings, with regression tests covering only these specific edge cases.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

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/28751.test.ts`:
- Line 4: Replace the in-test dynamic require of the URL module with a
module-scope import: remove the line that declares const url = require("url")
inside the test and add a top-level import for the URL module (e.g., import * as
url from "url" or appropriate named/default import) so the test uses a static,
module-scope import; ensure all references to the symbol url in the test
continue to work after the replacement.

🪄 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: 9ded39e8-e2b4-4e95-b0e9-d5d9d5c9b028

📥 Commits

Reviewing files that changed from the base of the PR and between 3ed4186 and bc5a39e.

📒 Files selected for processing (2)

src/js/node/url.ts
test/regression/issue/28751.test.ts

test/regression/issue/28751.test.ts

km-anthropic · 2026-04-01T18:45:50Z

@claude review

claude

LGTM — the g flag addition is correct and the regression test covers the fixed behavior.

Extended reasoning...

Overview

This PR modifies one line in src/js/node/url.ts (Url.prototype.format): the regex /%3A/i becomes /%3A/gi, ensuring all percent-encoded colons are decoded back when formatting the auth component. A matching regression test is added in test/regression/issue/28751.test.ts.

Security risks

None. The change affects URL formatting output only, making it more faithfully match Node.js behavior. There is no auth bypass or data exposure risk.

Level of scrutiny

This is a minimal, mechanical fix — a single character addition to a regex flag. The intent is unambiguous, the Node.js reference behavior is well-defined, and the test directly exercises the multi-colon case. Low scrutiny is appropriate.

Other factors

The bug hunter identified a pre-existing, analogous missing g flag on search.replace("#", "%23") at line 541 (not introduced by this PR). That is a separate issue for a follow-up. The CI failures shown in the build comment are unrelated (libuv/tinycc clang warnings on aarch64 and a build-cpp failure), not caused by these changes.

src/js/node/url.ts

coderabbitai

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/28751.test.ts`:
- Around line 1-2: Add a single-line GitHub issue URL comment at the top of the
test file referencing the issue from the filename (use //
https://github.com/oven-sh/bun/issues/28751); place it before the existing
imports (before the lines importing expect/test and format/parse/Url) so the
regression test follows the repo convention.

🪄 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: 1b7072ef-6f71-40cf-83ab-f0c4bd7cadc7

📥 Commits

Reviewing files that changed from the base of the PR and between e799d72 and 5646fa1.

📒 Files selected for processing (2)

src/js/node/url.ts
test/regression/issue/28751.test.ts

coderabbitai · 2026-04-01T19:29:34Z

test/regression/issue/28751.test.ts

+import { expect, test } from "bun:test";
+import { format, parse, Url } from "node:url";


🧹 Nitpick | 🔵 Trivial

Consider adding GitHub issue URL comment.

Per repo convention, regression test files typically include a single-line URL comment linking to the issue:

import { expect, test } from "bun:test"; import { format, parse, Url } from "node:url"; + +// https://github.com/oven-sh/bun/issues/28751

Based on learnings: "In oven-sh/bun regression test files under test/regression/issue/, a single-line URL comment linking to the GitHub issue (e.g., // https://github.com/oven-sh/bun/issues/NNNNN) IS the standard pattern."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

import { expect, test } from "bun:test";

import { format, parse, Url } from "node:url";

import { expect, test } from "bun:test";

import { format, parse, Url } from "node:url";

// https://github.com/oven-sh/bun/issues/28751

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/28751.test.ts` around lines 1 - 2, Add a single-line GitHub issue URL comment at the top of the test file referencing the issue from the filename (use // https://github.com/oven-sh/bun/issues/28751); place it before the existing imports (before the lines importing expect/test and format/parse/Url) so the regression test follows the repo convention.

claude

LGTM — both the auth colon and search hash fixes are correct and the regression tests cover both cases.

Extended reasoning...

Overview

Two one-line regex fixes in Url.prototype.format in src/js/node/url.ts: adding the g flag to /%3A/i (auth colon restoration) and replacing the string-based search.replace("#", "%23") with search.replace(/#/g, "%23"). A new regression test file covers both behaviors.

Security risks

None. Both changes only affect URL serialization output fidelity; there is no auth bypass, injection surface, or data exposure introduced.

Level of scrutiny

Low. Both changes are minimal, surgical, and mechanically correct — adding a missing g flag is a well-understood JavaScript regex fix with no side effects beyond replacing all matches instead of just the first.

Other factors

All prior review feedback (coderabbit’s module-scope import suggestion and my own pre-existing # encoding bug report) was addressed in follow-up commits. No bugs were found by the automated bug hunting system. The test cases are clean and directly validate the fixed behavior.

In Url.prototype.format, two replace calls were missing the global flag: 1. auth.replace(/%3A/i, ':') only restored the first percent-encoded colon. Add the g flag so all colons are preserved through the parse/format roundtrip. 2. search.replace('#', '%23') only encoded the first hash character. Switch to a regex with the g flag so all hashes in the search component are percent-encoded. Closes #28751

claude

LGTM — both missing g flags addressed and covered by regression tests.

Extended reasoning...

Overview

This PR touches two lines in src/js/node/url.ts (Url.prototype.format) and adds a new regression test file. Both changes are minimal, mechanical fixes that add missing g flags to .replace() calls.

Security risks

None. These are pure string-encoding corrections with no auth, crypto, or permission implications.

Level of scrutiny

Low. The root cause is well-understood (string .replace() without a regex g flag only replaces the first match), the fix is a one-character addition, and the behavior is directly verifiable against Node.js output. The test cases cover both the auth colon restoration and the search hash encoding.

Other factors

My prior review flagged the search.replace("#", "%23") companion bug (missing g flag, same issue class). The author fixed it in e799d72 and added a second test case. All reviewer feedback has been addressed and the PR is complete.

robobun mentioned this pull request Apr 1, 2026

url.format() in Bun does not decode auth credentials correctly after url.parse() #28751

Open

claude bot reviewed Apr 1, 2026

View reviewed changes

github-actions bot added the claude label Apr 1, 2026

coderabbitai bot reviewed Apr 1, 2026

View reviewed changes

test/regression/issue/28751.test.ts Outdated Show resolved Hide resolved

robobun force-pushed the farm/5903c497/fix-url-format-auth-colons branch from a214a91 to ff32881 Compare April 1, 2026 18:17

claude bot reviewed Apr 1, 2026

View reviewed changes

src/js/node/url.ts Show resolved Hide resolved

robobun force-pushed the farm/5903c497/fix-url-format-auth-colons branch from e799d72 to 5646fa1 Compare April 1, 2026 19:26

coderabbitai bot reviewed Apr 1, 2026

View reviewed changes

claude bot reviewed Apr 1, 2026

View reviewed changes

robobun force-pushed the farm/5903c497/fix-url-format-auth-colons branch from 5646fa1 to aeadc9e Compare April 1, 2026 19:49

claude bot reviewed Apr 1, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix url.format() only replacing first colon in auth credentials#28752

Fix url.format() only replacing first colon in auth credentials#28752
robobun wants to merge 1 commit intomainfrom
farm/5903c497/fix-url-format-auth-colons

robobun commented Apr 1, 2026

Uh oh!

claude bot left a comment

Uh oh!

robobun commented Apr 1, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Apr 1, 2026 •

edited

Loading

Reviews paused

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

km-anthropic commented Apr 1, 2026

Uh oh!

claude bot left a comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Apr 1, 2026

Uh oh!

claude bot left a comment

Uh oh!

claude bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		import { expect, test } from "bun:test";
		import { format, parse, Url } from "node:url";

Conversation

robobun commented Apr 1, 2026

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Uh oh!

robobun commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

km-anthropic commented Apr 1, 2026

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Apr 1, 2026 •

edited

Loading

coderabbitai bot commented Apr 1, 2026 •

edited

Loading