Repository navigation
Conversation
When a registry's OTP web login response returns a doneUrl on a different origin than the registry itself, bun publish was silently abandoning the web flow and falling back to the classic "Enter OTP:" prompt with no explanation. The guard existed because the doneUrl poll reused construct_publish_headers, which attaches the registry's Authorization header and a registry-derived Host header. Sending those to an arbitrary origin would leak the token, so the flow was aborted instead. Match npm's behavior (npm-registry-fetch scopes credentials by registry): keep running the web flow and poll the cross-origin doneUrl, but only attach the Authorization header (and the registry-derived Host override) when doneUrl is same-origin with the registry. Same-origin polling is unchanged and keeps the Bearer/Basic header.
|
Status: diff is green. Repro: mock registry returns a |
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
There was a problem hiding this comment.
I didn't find any bugs, but this changes when/where the registry Authorization header is sent during the OTP web-login flow — security-sensitive enough to warrant a human look.
Checked: the count/append passes in construct_publish_headers are gated symmetrically on include_auth; the same-origin comparison uses is_https() after the http(s)-only guard so an https→http downgrade correctly withholds credentials; all other call sites pass include_auth: true; the rewritten cross-origin test asserts authorization: null on the foreign poll and Bearer <token> on both registry PUTs.
Extended reasoning...
Overview
Modifies get_otp in src/runtime/cli/publish_command.rs so a cross-origin doneUrl from an OTP web-login response is polled instead of falling through to the classic Enter OTP: prompt. Adds an include_auth: bool parameter to construct_publish_headers that gates the Authorization and explicit Host headers; the doneUrl poll passes include_auth = done_url_same_origin, all other callers pass true. test/cli/install/bun-publish.test.ts inverts the existing cross-origin test to assert the new behavior and strengthens the same-origin test to assert credentials are still attached.
Security risks
This is the load-bearing part of the change. Previously bun refused to poll a cross-origin doneUrl at all; now it will issue GET requests to any http(s) URL the registry's 401 response supplies, stripping the registry token. That matches npm-registry-fetch's credential-scoping, and the token is correctly withheld (test asserts authorization: null). The same-origin predicate compares is_https() / hostname / get_port_auto() after an http(s)-only guard, so an https-registry → http-doneUrl downgrade withholds credentials (fails closed). Hostname comparison is byte-exact, so any case/normalization mismatch also fails closed. I don't see a leak, but relaxing a same-origin guard on a credential-carrying request path is exactly the kind of change a human should sign off on.
Level of scrutiny
High — this is auth-header routing in bun publish. The mechanical parts (new bool parameter threaded through, symmetric count/append gating) are straightforward; the policy change (poll arbitrary registry-supplied origins) is the part that needs a maintainer's judgment.
Other factors
The two-pass HeaderBuilder structure means count and append must agree — verified both blocks gate authorization and Host identically on include_auth. The cross-origin test was deliberately rewritten (title and assertions inverted) to match the new intended behavior, and the same-origin test now positively asserts Bearer <token> is still sent, so both directions are covered. No prior reviews from me on this PR.
|
Updated 8:16 AM PT - Jul 31st, 2026
✅ @robobun, your commit c3b1bce587ffedd61c3a93b2a7cc98b894e0c130 passed in 🧪 To try this PR locally: bunx bun-pr 36556That installs a local version of the PR into your bun-36556 --bun |
There was a problem hiding this comment.
No bugs found, but this loosens a credential-related guard in bun publish (previously refused to poll cross-origin doneUrl; now polls it with auth stripped). The shape looks right and the tests assert no Authorization header reaches the foreign origin, but auth-header handling on outbound requests is worth a human look.
What was reviewed:
construct_publish_headers: count/append phases stay symmetric under the newinclude_authgate; both PUT call sites passtrue, only the doneUrl poll passes the same-origin check.- Same-origin check via
is_https()equality + hostname + port — correctly treats an http doneUrl against an https registry as cross-origin (no credential downgrade). - Test asserts the foreign server sees
authorization: nulland the correct derivedHost, and both registry PUTs still carryBearer <token>; the same-origin test now asserts the poll keeps credentials.
Extended reasoning...
Overview
Changes get_otp in src/runtime/cli/publish_command.rs so a cross-origin doneUrl from the registry's OTP web-login response is polled instead of silently falling back to the classic Enter OTP: prompt. Adds an include_auth: bool parameter to construct_publish_headers that gates the Authorization and explicit Host headers; the two PUT calls pass true, the doneUrl poll passes the result of a scheme/host/port same-origin check. test/cli/install/bun-publish.test.ts updates one test to assert same-origin polls keep credentials and rewrites the cross-origin test to assert the foreign server is polled with no Authorization and a URL-derived Host.
Security risks
This is credential-handling code. The pre-existing guard was there specifically to avoid leaking the registry token to an arbitrary origin. The fix keeps that property by stripping Authorization (and the registry-derived Host override) when the doneUrl origin differs, matching how npm-registry-fetch scopes credentials. I checked that the count and append phases of HeaderBuilder are gated identically so they stay in sync, that the same-origin comparison uses is_https() equality (so http-vs-https on the same host is treated as cross-origin, preventing an https→http credential downgrade), and that the cross-origin test asserts authorization: null on the foreign request. I did not find a leak path, but loosening a guard whose stated purpose was token protection is exactly the kind of change a maintainer should sign off on.
Level of scrutiny
Medium-high. The mechanical change is small (one new bool threaded through, two if wrappers duplicated across the count/append halves), but it sits in the outbound-auth path of bun publish and reverses a previously deliberate refusal. The tests are strong — fail-before/pass-after per the description, exact-array assertions on headers received by both servers, stdin: "ignore" so a regression to the prompt would hang/fail rather than pass.
Other factors
The comment-cop bot flagged a long justification comment which was removed in c3b1bce (thread resolved). No prior reviews from me. The behavior change is user-visible (web login now works where it previously fell back), so it's also an intentional UX/compat change with npm, not purely a bugfix — another reason for a human to confirm the direction.
When a registry's OTP web login response returns a
doneUrlon a different origin than the registry,bun publishwas silently abandoning the web flow and falling back to theEnter OTP:prompt with no message explaining why.Cause
The same-origin guard in
get_otp(src/runtime/cli/publish_command.rs) didbreak 'try_webon any scheme/host/port mismatch. The guard existed because the poll reusedconstruct_publish_headers, which attaches the registry'sAuthorizationheader (and a registry-derivedHostoverride). Sending those to an arbitrary origin would leak the token, so web login was aborted instead.Fix
Match npm (
npm-registry-fetchscopes credentials by registry): always run the web flow, but only attachAuthorizationand the explicitHostheader whendoneUrlis same-origin with the registry. A cross-origindoneUrlis polled with no credentials (the HTTP client derivesHostfrom the request URL). A non-http(s)doneUrlstill falls back to the classic prompt.Verification
test/cli/install/bun-publish.test.ts:done url on a different origin is polled without credentials: registry onlocalhost:AreturnsdoneUrlat127.0.0.1:B; asserts the second server is polled withAuthorization: nulland correctHost, while both registry PUTs still carryBearer <token>.non-http auth url ... done url pollingnow also asserts the same-origin poll keepsBearer <token>attached.Fail-before (src/ stashed): falls through to
Enter OTP:and never printsAuthenticate your account at, test fails. Pass-after: all 38 tests inbun-publish.test.tspass.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-publish.test.ts