sindresorhus / got

🌐 Human-friendly and powerful HTTP request library for Node.js
MIT License
14.27k stars 935 forks source link

https option rejectUnauthorized is always set forcing node https module to ignore NODE_TLS_REJECT_UNAUTHORIZED #2149

Closed seal-hg closed 2 years ago

seal-hg commented 2 years ago

Describe the bug

In source/core/options.ts the https option rejectUnauthorized is always set. If not to true or false then at least to undefined. But even if it is only set to undefined it is deactivating the consideration of environment variable NODE_TLS_REJECT_UNAUTHORIZED in the https module of Node. You can easily avoid this by changing line https://github.com/sindresorhus/got/blob/73501b63fed981f7958e6f732d7c13791c050126/source/core/options.ts#L2442 to

            ...(https.rejectUnauthorized && {rejectUnauthorized: https.rejectUnauthorized}),

Actual behavior

...

Expected behavior

...

Code to reproduce

...

Checklist

szmarczak commented 2 years ago

Having multiple options to alter Node.js behavior is a bad idea. The only reasonable place to do so is the code.

seal-hg commented 2 years ago

Well, if having multiple options for doing something is a good or a bad idea is a matter of opinion. In this case following your opinion is breaking best practice of many years for test environments.

szmarczak commented 2 years ago

It's not best practice.

panva commented 1 year ago

It was certainly my expectation that NODE_TLS_REJECT_UNAUTHORIZED would work the same in v12 as well.

Before I could get by a test environment running self signed certs by just setting the environment variable in CI and it would work reliably even with my other dependencies, some of which may be got 11, that are based on the node:http(s) modules.

Now I need to add code that checks for the variable's value and changes got's option to false. That code is useless to my module's consumers in production and I can only keep my fingers crossed none of my other dependencies that execute in CI upgraded to got 12 and didn't write the same workaround.

I wish this change would be reconsidered, it is certainly unexpected.

fgreinacher commented 1 year ago

@szmarczak @sindresorhus This is causing some issues in semantic-release/gitlab. I understand your reasoning for not considering the env var, but it's really inconvenient for downstream tools, because all of us now need to provided dedicated configuration options for disabling the verification. Are you open to reconsider this or should we go ahead and solve it locally downstream?

Tri0L commented 1 year ago

@szmarczak Why got ignoring systems certificates? If I add my own CA certificate to /etc/ssl/certs/ca-certificates.crt, all system utilities like curl working fine and trust to certs of my domains. But not got.

All is working fine if I add NODE_EXTRA_CA_CERTS=/etc/ssl/certs/ca-certificates.crt env variable. Why? Where got gets list of trusted certs?