publish: accept Basic credentials from .npmrc and bunfig.toml - #38782
publish: accept Basic credentials from .npmrc and bunfig.toml#38782robobun wants to merge 1 commit into
Conversation
`bun publish` refused to run with "missing authentication" unless the registry had a token or the registry URL itself carried userinfo, even though the Basic credential built from `//host/:username` + `//host/:_password`, `//host/:_auth`, or a bunfig `username`/`password` pair is what `bun install` already sends and what the publish request headers are built from. Count that credential as authentication too. The two lifecycle-order publish tests spawn six child bun processes each and exceed the default timeout on debug builds, which kills the shared Verdaccio for the rest of the file; give them their own timeout.
|
Warning Review limit reached
Next review available in: 4 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 |
|
Status: closed as a duplicate of #38776, which was opened a few minutes earlier with the same one-line fix and landed first. Suggestions from this branch (scoped-registry and tarball test cases, and the lifecycle test timeout that keeps the rest of |
|
Duplicate of #38776 (same fix, opened first). Closing this one; the extra test cases from this branch are suggested over there. |
There was a problem hiding this comment.
LGTM — one-line widening of the pre-flight auth check to accept scope.auth, which construct_publish_headers already sends as Authorization: Basic.
What was reviewed:
- Verified
construct_publish_headersandcheck_package_version_existsin the same file already branch on!registry.auth.is_empty(), so the check was the only gap. - Tests cover the variant matrix (
.npmrcusername+_password,_auth, bunfig scoped, tarball, and the no-credentials negative), assert the exactAuthorizationheader, and isolate HOME/USERPROFILE so a runner's~/.npmrccan't satisfy the test. - The 30s timeout on the two lifecycle-script tests is justified by the comment (six child bun spawns on a debug build; a timeout there tears down the shared Verdaccio).
Extended reasoning...
Overview
The source change is a single added conjunct in the pre-flight authentication check in publish() (src/runtime/cli/publish_command.rs): it now also accepts a non-empty registry.auth (the base64 user:password Basic credential) alongside the existing registry.token and URL-embedded userinfo. construct_publish_headers and check_package_version_exists in the same file already send Authorization: Basic <auth> when token is empty and auth is not, so this check was the only piece rejecting Basic-auth configs. The test file gains a describe("basic auth") block with five test.concurrent cases against an in-process Bun.serve mock registry, plus a 30s timeout on two existing lifecycle-script tests.
Security risks
None introduced. The change loosens a pre-flight rejection to match what the header-construction code already does with the same credential — it does not add a new credential source, change how credentials are read from disk, or alter what is sent on the wire. The negative test ("no credentials is still rejected before anything is uploaded") confirms the check still fails closed when no credential is present.
Level of scrutiny
Low-to-medium. The Rust change is a one-line boolean widening whose correctness is verifiable by reading the two sibling functions in the same file that already consume registry.auth. It is a CLI pre-flight check, not a hot path or memory-owning code. The tests are the larger part of the diff and follow the repo's harness conventions (tempDir, using, bunEnv spread, port: 0, test.concurrent, stdout asserted before exit code, exact-value assertions on the recorded Authorization header).
Other factors
The tests isolate HOME/USERPROFILE/XDG_CONFIG_HOME so a real ~/.npmrc on the CI runner cannot provide a token that would make the positive tests pass without the fix — addressing the "prove the test fails for the right reason" concern. The 30s timeout bump on the two should run in order tests is commented with the reason (six child-bun spawns on a debug build; a timeout there kills the file-shared Verdaccio and cascades), which is the shape REVIEW.md asks for when a timeout is unavoidable rather than a workload that can be shrunk. No CODEOWNERS entries cover either changed file. No prior human review comments to address.
Fixes #17531
Problem
bun publishexits with "error: missing authentication (run bunx npm login)" and uploads nothing whenever the registry is configured with Basic credentials:.npmrc//host/:username=+//host/:_password=,.npmrc//host/:_auth=, or a bunfigregistry/[install.scopes]entry withusername+password.bun installworks with the same config, and so doesnpm publish. This is the shape used with registries that authenticate with Basic (Nexus, Azure DevOps feeds, Verdaccio), and it is the Basic-auth half of Cannot publish to custom registry, npm can with the same config #18670 (thepublishConfig.registryhalf is publish: honor publishConfig.registry and publishConfig["@scope:registry"] #38322).publish()(src/runtime/cli/publish_command.rs:870) only acceptsscope.tokenor userinfo embedded in the registry URL. The Basic credential that the config loaders put inscope.authis ignored, even thoughconstruct_publish_headers(same file) already sendsAuthorization: Basic <scope.auth>when there is no token, so the rest of the publish path supports it.Fix
scope.auth. Nothing else changes: a registry with neither a token nor Basic credentials still fails withmissing authenticationbefore any request is made.test/cli/install/bun-publish.test.ts,describe("basic auth"), against an in-process registry that records thePUTand itsAuthorizationheader:.npmrcusername +_password,.npmrc_auth, bunfig scoped registry with username + password (the Bun Publish doesn't support token based login (Azure Devops) #17531 config),bun publish <tarball>, and the no-credentials rejection. The four positive cases fail on the current canary (missing authentication, no request) and pass with this change; all ofbun-publish.test.ts(44 tests) andnpmrc.test.tspass with the debug build.lifecycle scripts > should run in ordertests in the same file spawn six child bun processes each and take about 5s on a debug build, and their timeout kills the Verdaccio shared by the rest of the file. They now declare a 30s timeout; without it the file cannot pass under a debug build here..npmrccredential resolution more broadly; this PR only carries the publish check so the RC gets it independently of that work.Background
Scope(src/install/npm.rs) is bun's resolved registry: the URL plus one credential, eithertoken(sent asAuthorization: Bearer) orauth, the base64user:passwordstring (sent asAuthorization: Basic).Scope::from_apibuildsauthfrom a username + password pair, and the.npmrcloader decodes_authinto that pair, so every Basic configuration ends up inscope.auth.scope_for_package_namepicks the default scope or the@scope:registryone for the package being published;publish()runs the credential check on that scope and thenconstruct_publish_headersturns it into the header, which is why the header side already handled Basic while the check did not.Repro matrix on 1.4.0-canary.1 (b7a0431), mock registry recording the PUT
With this change every publish row sends
PUT /<name>withAuthorization: Basic <base64>and exits 0.