install: use uppercase %2F for scoped package manifest URLs - #33784
install: use uppercase %2F for scoped package manifest URLs#33784robobun wants to merge 5 commits into
Conversation
Match the npm CLI and RFC 3986 §2.1 canonical form. GitLab's npm registry returns 404 for lowercase %2f but accepts %2F. AWS CodeArtifact and Azure Artifacts accept both. Port of the fix from #30312 to the Rust codebase: - src/install/NetworkTask.rs (bun install manifest fetch) - src/bun_core/fmt.rs (DependencyUrlFormatter for bun pm view/publish) Adds regression tests and updates install test assertions. Fixes #30311 Fixes #26241
Remove the dead 404 branches (lowercase requests fell through to the default 404 anyway), assert the exact request paths with toEqual, and move the bun pm view test next to the other pm-view coverage in bun-info.test.ts where it does not have to work around the Verdaccio scaffolding.
|
Reproduced and verified locally. Fail-before (released bun): Pass-after ( Commits carried from #33774; second commit applies review cleanups (removes dead 404 branches in the mock, asserts exact request paths, moves the CI status: build #70755 finished with 285/286 jobs passing. All install/pm-view lanes are green; both new regression tests pass on every platform. The single failure is Ready to merge. |
WalkthroughThis PR changes percent-encoding of the slash in scoped package names (e.g. ChangesScoped package URL encoding fix
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:35 PM PT - Jul 8th, 2026
❌ @alii, your commit e8ff570 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 33784That installs a local version of the PR into your bun-33784 --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 — two-character casing fix (%2f → %2F) to match npm CLI and RFC 3986 canonical form, with regression tests.
Extended reasoning...
Overview
The production change is exactly two characters across two files: src/install/NetworkTask.rs (scoped manifest URL for bun install) and src/bun_core/fmt.rs (DependencyUrlFormatter for bun pm view/bun publish), each changing %2f → %2F. The remaining ten files are test updates: existing assertions on the request URL updated to expect uppercase, mock-registry path normalization extended to accept both casings, and two new regression tests that assert the exact bytes reaching a local Bun.serve registry.
I confirmed the remaining lowercase %2f occurrences in src/ (BunObject.cpp, package_json.rs, bindings.cpp) are all in validation/rejection paths that check for either casing — not URL construction — so they are correctly left unchanged.
Security risks
None. This changes the hex-digit casing of a percent-encoded slash in an outgoing manifest request URL. It does not touch auth, credentials, TLS, input validation, or any trust boundary. RFC 3986 §2.1 defines uppercase as the canonical form, and the npm CLI already uses %2F; per the linked issues, npmjs.org, AWS CodeArtifact, Azure Artifacts, and GitLab all accept %2F while GitLab rejects %2f. There is no known registry that accepts lowercase but rejects uppercase, so this is strictly a compatibility improvement.
Level of scrutiny
Low. The production diff is a trivial, mechanical string-literal casing change with a well-documented rationale and two linked user-reported issues. The test updates are equally mechanical (%2f → %2F in assertions, .replaceAll("%2F", "/") added alongside the existing lowercase normalizer). The two new regression tests follow repo conventions (using server = Bun.serve({port: 0}), exact-path assertions, exit-code asserted last, issue URL comment) and were verified fail-before/pass-after per the robobun comment.
Other factors
No CODEOWNERS cover the touched paths. No outstanding human reviewer comments. The bug hunter found nothing. This supersedes #33774 with the same commits plus test cleanups already applied.
There was a problem hiding this comment.
LGTM — two-character production change (%2f → %2F) with matching test updates and regression coverage.
Extended reasoning...
Overview
Production change is two single-character edits: src/install/NetworkTask.rs:428 and src/bun_core/fmt.rs:330 change the percent-encoding of / in scoped package manifest URLs from lowercase %2f to uppercase %2F, matching the npm CLI and RFC 3986 §2.1 canonical form. The remaining ten files are test-suite updates: existing assertions on request URLs/error messages flipped from %2f to %2F, the mock registry (dummy.registry.ts) now normalizes both casings, and two new regression tests assert the exact request path bytes that reach a local Bun.serve mock registry for both bun install and bun pm view.
Security risks
None. The change does not touch auth, credential handling, or the origin-matching logic in for_tarball. It only alters the ASCII case of two hex digits in an outgoing URL path segment. The mock-registry normalization change is additive (.replaceAll("%2F", "/") alongside the existing lowercase replace), so no test loses coverage.
Level of scrutiny
Low. This is a mechanical case-normalization fix with a clear real-world motivation (GitLab's npm registry rejects lowercase %2f), verified fail-before/pass-after, and no known registry that accepts %2f but rejects %2F. I grepped src/ for remaining lowercase %2f — the only hits are unrelated path-validation checks that already handle both casings.
Other factors
No CODEOWNERS cover the touched paths. No outstanding reviewer comments. The bug-hunting system found nothing. The two new regression tests are hermetic (local Bun.serve, port: 0, using for cleanup) and assert exact request paths, so they fail for the right reason on the old build. The mock-registry keeps the lowercase replace for back-compat, so no other test can silently pass on the wrong encoding.
There was a problem hiding this comment.
LGTM — two-character casing fix (%2f → %2F) with matching test-assertion updates and regression coverage.
Extended reasoning...
Overview
Production change is exactly two characters across two files: src/install/NetworkTask.rs (manifest URL for bun install) and src/bun_core/fmt.rs (DependencyUrlFormatter used by bun pm view/bun publish) switch the percent-encoding of / in scoped package names from lowercase %2f to uppercase %2F. Everything else is test updates: existing assertions in bun-add/bun-create/bun-install/bun-update/bunx tests changed to expect %2F, two new regression tests asserting exact request-path bytes against a local Bun.serve mock, and the dummy-registry helpers now normalize both %2f and %2F when routing.
Security risks
None. This changes the casing of a percent-encoding literal in an outbound URL that bun already constructs; it does not touch auth, credentials, path validation, or any user-controlled input parsing. RFC 3986 §2.1 defines uppercase hex as canonical and mandates case-insensitive equivalence, so this is a strict compatibility improvement (npm CLI already sends %2F; GitLab rejects %2f). I checked for other lowercase %2f producers in src/ — the remaining hits are all input-validation checks (fileURLToPath, exports resolution) that already handle both casings and are unrelated.
Level of scrutiny
Low. The production diff is a two-character literal substitution with a well-documented rationale (RFC canonical form, npm CLI parity, real-world GitLab 404s). The test changes are mechanical string updates plus two focused regression tests that verify the exact bytes on the wire. The mock-registry .replaceAll("%2f", "/").replaceAll("%2F", "/") change is defensive and keeps the helper working regardless of which casing the client sends.
Other factors
Bug hunting found nothing. CI is green on all install/pm-view lanes; the remaining red is unrelated infra (mysql/postgres container health on specific runners, reproduced on other PRs). The PR supersedes #33774 with review cleanups already applied. No CODEOWNERS-sensitive paths, no API surface change, no design decisions.
|
Closing this — I dug into the premise and it doesn't hold up. Sorry, I know that's annoying after the rebase work. The npm CLI sends lowercase
// scoped packages in couch must have slash url-encoded, e.g. @foo%2Fbar
this.escapedName = name.replace('/', '%2f')Note the comment says Our GitLab treats both casings identically. Against live All three decode to the same name. And for a namespace that exists, both casings return a byte-identical So this change can't affect GitLab. It also can't break anything ( The tests don't cover the reported bug. The mock registry does Neither linked issue is an encoding bug:
Leaving both issues open; the real fix looks like auth-token scoping by registry path. Thanks for chasing this down regardless — the repro detail in #30311 is what made it tractable to run down. |
3 participants
Fixes #30311
Fixes #26241
Supersedes #33774 (same commits, rebased onto a repo branch so CI can run, with test cleanups)
What does this PR do?
Since #5716, bun percent-encodes the
/between scope and name in private-registry manifest requests as lowercase%2f:GitLab's npm registry returns 404 for this URL; it does not treat
%2fand%2Fas equivalent. bun 1.2.23 sent the unencoded slash, which GitLab accepted. The npm CLI uses uppercase%2F, and RFC 3986 §2.1 says uppercase is the canonical form. npmjs.org, AWS CodeArtifact, Azure Artifacts and GitLab all accept%2F; no known registry accepts%2fbut rejects%2F.Production changes (two characters):
src/install/NetworkTask.rs: scoped manifest URL forbun install(%2f→%2F)src/bun_core/fmt.rs:DependencyUrlFormatter, used bybun pm view/bun publish(%2f→%2F)Also updates install-suite assertions and the mock-registry path normalization to expect/accept uppercase
%2F.How did you verify your code works?
Two regression tests, each asserting the exact bytes that reach a local
Bun.servemock registry:test/cli/install/bun-install-registry.test.tsscoped package manifest url uses uppercase %2F(coversNetworkTask.rs)test/cli/install/bun-info.test.tsshould use uppercase %2F in the manifest URL for scoped names(coversDependencyUrlFormatterviabun pm view)Both fail on the previous build with
and pass on this branch.
Co-authored-by: Erik erik.balfe@protonmail.com
no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-add.test.ts test/cli/install/bun-install-registry.test.ts test/cli/install/bun-install.test.ts test/cli/install/bunx.test.ts