Skip to content
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

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

Closed
2 tasks done
seal-hg opened this issue Sep 19, 2022 · 6 comments

Comments

@seal-hg
Copy link

seal-hg commented Sep 19, 2022

Describe the bug

  • Node.js version: 16
  • OS & version: Ubuntu

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

rejectUnauthorized: https.rejectUnauthorized,

to

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

Actual behavior

...

Expected behavior

...

Code to reproduce

...

Checklist

  • I have read the documentation.
  • I have tried my code with the latest version of Node.js and Got.
@szmarczak
Copy link
Collaborator

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

@seal-hg
Copy link
Author

seal-hg commented Sep 20, 2022

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
Copy link
Collaborator

It's not best practice.

@panva
Copy link

panva commented Nov 30, 2022

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
Copy link

@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
Copy link

Tri0L commented Mar 23, 2023

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants