-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG] Config environment variable forwarding breaks with false config values #2284
Comments
What happens with npm v7.0.15? |
I wasn't able to reproduce this with npm 7, only with npm 6. |
npm If your bug is preproducible on If your issue was a feature request, please consider opening a new RRFC or RFC. If your issue was a question or other idea that was not CLI-specific, consider opening a discussion on our feedback repo |
Current Behavior:
I have an install script that needs to run the
npm
command to install a platform-specific package. I'm currently doing this by forwarding the install script's environment variables to thenpm
command. This automatically picks up most custom configuration from users including project-local.npmrc
files. However, it doesn't work for config values that are exactly equal tofalse
.The problem appears to be that when the install script is run, the
npm-lifecycle
package sets allfalse
config values in environment variables to an empty string instead, and then thenpm/cli
package ignores environment variables that are empty strings when reading environment variables back in.Specifically, users that set
strict-ssl=false
in their.npmrc
files will have that setting ignored becausenpm-lifecycle
sets thenpm_config_strict_ssl
environment variable to an empty string, and thennpm/cli
ignores that and setsstrict-ssl
totrue
.Here is where
false
becomes""
:https://github.com/npm/npm-lifecycle/blob/bfb6f73853a222a3f35a1a6651b0756816e2ed87/index.js#L464
Here is where
""
is ignored, and becomestrue
:cli/lib/config/core.js
Line 273 in 48753bb
There is more info here: evanw/esbuild#565.
Expected Behavior:
There are three possible scenarios, and I'm not sure which one is the case here:
This could be considered a problem with
npm-lifecycle
. If so, I would expect that package to set environment variables forfalse
config values to"false"
instead of""
.This could be considered a problem with
npm/cli
. If so, I would expect that package to interpret environment variables that are present but empty strings asfalse
values instead of ignoring them.This behavior could be by design, or could be too risky to change. Then I would expect for my install script to have to handle this somehow.
Regardless of which case it is, I'd like to know if I should adapt my install script to translate
npm_config_*
environment variables that are empty strings to the stringfalse
. Even if this is fixed in npm itself, it would be great if my install script could be made to work with older npm versions.I'm asking because I want to make sure there aren't going to be unintended consequences from doing this. It seems prudent to ask about this here since setting the
strict-ssl
option tofalse
potentially has security consequences.Steps To Reproduce:
Reproducing this is somewhat painful since it involves a custom registry. I am unfamiliar with custom registries (I'm just trying to solve a problem for a user of mine, and I don't have this problem myself) but I was able to reproduce this using https://verdaccio.org/.
package.json
file containing{}
.npmrc
withregistry=(your local HTTPS registry URL)
andstrict-ssl=false
esbuild
packageWhen the issue happens, I see the
errno DEPTH_ZERO_SELF_SIGNED_CERT
error. This error only happens when the install script forwards environment variables to a nestednpm
command. It does not happen when manually I run the nestednpm
command myself.The
esbuild
package may still install successfully because the install script is somewhat resilient and will fall back to a raw HTTP request to the npm registry, but that's not a solution for my user because their CI environment doesn't have direct internet access.Environment:
The text was updated successfully, but these errors were encountered: