Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.3k
fix: unsupported env fallbacks in .npmrc when publishing#10255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
zkochan commentedDec 3, 2025
could we just stop copying the |
KSXGitHub commentedDec 3, 2025
I'm not sure how it's going to work with keys like |
| "@pnpm/plugin-commands-publishing":patch | ||
| --- | ||
| pnpm uses npm under the hood when publishing, and npm doesn't support the`:-fallback` syntax, resulting in npm not reading any envs specified with a fallback. For example, if the npmrc contains`//registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN-fallback}`, all publish commands fails even if the NODE_AUTH_TOKEN env is set. The fallbacks are now stripped before publishing, i.e. the line becomes`//registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN}`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this misses a: here.
| pnpm uses npm under the hood when publishing, and npm doesn't support the`:-fallback` syntax, resulting in npm not reading any envs specified with a fallback. For example, if the npmrc contains`//registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN-fallback}`, all publish commands fails even if the NODE_AUTH_TOKEN env is set. The fallbacks are now stripped before publishing, i.e. the line becomes`//registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN}`. | |
| pnpm uses npm under the hood when publishing, and npm doesn't support the`:-fallback` syntax, resulting in npm not reading any envs specified with a fallback. For example, if the npmrc contains`//registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN:-fallback}`, all publish commands fails even if the NODE_AUTH_TOKEN env is set. The fallbacks are now stripped before publishing, i.e. the line becomes`//registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN}`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Stripping could cause inconsistent behavior betweenpnpm publish and other commands.
I wonder if we can replace the whole${NODE_AUTH_TOKEN:-fallback} with its actual value. What do you think,@zkochan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This change only strips the fallbacks in the temporary file that is copied to the tmp directory that is used to publish from, not touching the original .npmrc file, so it shouldn't affect any other commands.
As mentioned in the PR description, I'd personally advise against actually placing the secret in the file and writing it to disk. What if therunNpm throws and rimraf isn't called? What if the code is changed later on, without the dev being attentive to the fact that a file which normally doesn't contain sensitive values now does? It seems like a risky approach to me.
| constENV_EXPR=/(?<!\\)(\\*)\$\{([^${}]+)\}/g | ||
| constENV_VALUE=/([^:-]+)(:?)-(.+)/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't like complex RegExps. I prefer JavaScript functions that call.startsWith,.endsWith,.includes,.split, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Neither of these are introduced by this PR. As mentioned in the description and in the code comment, I'm considering this PR a WIP untilpnpm/components#25 is either merged, or a different approach is agreed upon. Both of the RegExp's are copied from@pnpm/config.env-replace :)
WIP untilpnpm/components#25 is merged.
As mentioned in the changeset,
pnpm publishdoesn't work if any of the .npmrc settings that are required to publish, uses an env fallback, since the syntax isn't supported by npm.Any use of the
:-fallbacksyntax in .npmrc files is now stripped before publishing, using the newstripEnvFallbackfunction from@pnpm/config.env-replace.I opted to create
stripEnvFallbacksince using the existingenvReplacewould place the actual env value in the .npmrc file, which is not desirable for security reasons, even if pnpm attempts to delete the temp npmrc after publishing.