fix(publish) Support split npmrc auth for publish - #26178
gavinhailey wants to merge 3 commits into
Conversation
WalkthroughBroadened the authentication condition in PublishCommand.publish to require missing token, auth, and user (plus incomplete URL-embedded credentials). Added tests covering split npmrc authentication formats (username/password and _authToken) for scoped and unscoped registries against Verdaccio. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/cli/publish_command.zig`:
- Around line 537-539: The auth gate in publish_command.zig incorrectly requires
both registry.auth and registry.user to be set, causing false NeedAuth errors
when legacy _auth populates only registry.auth; update the condition so it
accepts any of: registry.token non-empty, registry.auth non-empty OR
registry.user non-empty, or URL-embedded credentials
(registry.url.username/registry.url.password), i.e., check registry.auth.len > 0
|| registry.user.len > 0 rather than requiring both; ensure this logic aligns
with constructPublishHeaders which already uses registry.auth.
In `@test/cli/install/bun-publish.test.ts`:
- Around line 1014-1018: Reorder the assertions in the test so stdout/stderr
expectations run before checking the process exit code: move the
expect(out).toContain(`+ ${format.packageName}@1.0.0`) and
expect(err).not.toContain("missing authentication") to precede
expect(exitCode).toBe(0) in the test that calls publish(env, packageDir)
(variables: out, err, exitCode, and helper publish).
|
Are there any other blockers preventing this from being merged? This has been a very annoying issue for a very long time... |
8c95bf0 to
2f8f31a
Compare
2f8f31a to
a2aba29
Compare
|
Closing as stale: this PR predates the Rust rewrite. Every If the underlying change is still wanted, it will need to be redone against the current Rust/C++ tree. Apologies for the churn, and thank you for the contribution. |
What does this PR do?
Fixes
bun publishfailing with "error: missing authentication" despite valid.npmrccredentials, whilebun installandnpm publishboth work fine with the same configuration. Updates the auth check to accept theregistry.authandregistry.usercombination that gets set.This specifically affects split username/password authentication like:
Related Issues
How did you verify your code works?
New auth tests in
test/cli/install/bun-publish.test.ts, four new tests verify different.npmrcauthentication formats:_authTokenwith default registry (already worked, but expanded coverage)_authTokenwith scoped registry (already worked, but expanded coverage)Manual testing with debug release:
Tested publishing to a private JFrog Artifactory registry using split username/password in
.npmrc:Before fix: ❌
error: missing authenticationAfter fix: ✅
+ @scope/abcefg@0.1.0-rc.0