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

Feature disable tls certs #1511

Merged
merged 3 commits into from
Nov 21, 2019
Merged

Conversation

commenthol
Copy link
Contributor

Tackles Issue #926 by adding a new method disableTLSCerts to node client to allow bypassing TLS certificate checks.

@niftylettuce
Copy link
Collaborator

Could you add documentation to this in the README/API?

Do not reject expired or invalid TLS certs.
Sets `rejectUnauthorized=true`.
Be warned that this allows MITM attacks.
Need upgrade to pass linter errors:
The 'Object.getOwnPropertyDescriptors' is not supported until
Node.js 7.0.0. The configured version range is '>= 6.4.0'
node/no-unsupported-features/es-builtins
@commenthol commenthol force-pushed the feat-disableTLSCerts branch from b107e33 to ba08767 Compare October 14, 2019 19:59
@commenthol
Copy link
Contributor Author

@niftylettuce new method is documented in docs/index.md. I hope that this was meant with README/API. If there are other places which need documentation update, please let me know.

@Andro999b
Copy link

@niftylettuce Caould you please merge it if all ok?

@B0B234
Copy link

B0B234 commented Nov 21, 2019

PLEASE MERGE THIS FEATURE
It would make any developer happy.
give us the freedom to choose we know the consequences.

@JaneX8
Copy link

JaneX8 commented Nov 21, 2019

If @niftylettuce is responsible or anyone else who can merge at visionmedia. Please merge!

I've just been reading about 4 issues regarding this issue and saw there are a ton more about this issue. It is unbelievable this is not currently available. I totally understand the principle standpoint of wanting HTTP-only or HTTPS correct-only (referring to @kornelski) but that is just not the real world! Therefore you completely make superagent unusable for basically any use case where the requested URL is unknown or unmanaged by the developer or dynamic and changing. This way you force developers to do something even more insecure which is disabling it for the entire node.js process and all other request (libraries) used. I really cannot believe this.

If developers decide to misuse that, it is their problem. Document it well and/or warn the shit out of developers in log files and in documentation. It is their responsibility and you're ignorantly taking the freedom of developers away, as a matter of fact it is so common to have this basic option that with "product/library selection" it is not even a requirement exactly because it is such a basic functionality. We've chosen to use superagent because of other cool easy-to-use features such as throtteling plugin and I was quite happy about it, but this is just unbelievable. Not having this options makes the library completely useless for our and many other use cases. In our case, setting back development for at least 3 months if this PR is not merged.

Again, please merge to the new version as soon as possible! And to @commenthol finally someone who took action. Thanks for developing and documenting this PR.

@niftylettuce niftylettuce merged commit fbfd7f7 into ladjs:master Nov 21, 2019
@niftylettuce
Copy link
Collaborator

this feature has been released to npm as of superagent@5.1.1

npm i superagent@latest or yarn add superagent@latest

thanks for your patience everyone and to @commenthol for the hard work 🙏

see documentation for this feature at https://github.com/visionmedia/superagent/blob/master/docs/index.md#tls-options or simply append disableTSLCerts() to your usage

if you guys need anything else ping me or email niftylettuce@gmail.com if I don't catch something the first time around

@B0B234
Copy link

B0B234 commented Nov 21, 2019

Thank you

@JaneX8
Copy link

JaneX8 commented Nov 22, 2019

Thanks a lot @niftylettuce and @commenthol. You made my day!

@commenthol
Copy link
Contributor Author

@niftylettuce Thanks for the merge and for maintaining this great project. 👏

@commenthol commenthol deleted the feat-disableTLSCerts branch November 22, 2019 16:10
@josh18
Copy link

josh18 commented Dec 18, 2019

Unfortunately it seems like this has broken the use of process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';? I can confirm that it worked in 5.1.0 but not 5.1.1.

Also I think this probably should have been a minor version bump? I think it may have been overlooked as there isn't any info for this feature (or any changes) in the 5.1.1 release.

@fbruffaert
Copy link

I can confirm @josh18 message after struggling the whole afternoon with the same exact issue.

@niftylettuce
Copy link
Collaborator

Will fix. One sec.

@niftylettuce
Copy link
Collaborator

niftylettuce commented Dec 20, 2019

Released in v5.1.3.

npm install superagent@latest

or with yarn:

yarn add superagent@latest

Please consider sponsoring my work at https://github.com/sponsors/niftylettuce.


According to Node docs, the use of that environment variable is "strongly discouraged" just so you all know.

Ref: https://nodejs.org/api/cli.html#cli_node_tls_reject_unauthorized_value

@josh18
Copy link

josh18 commented Dec 20, 2019

Thanks! Yeah currently we use process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; as a workaround when running tests locally.

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

Successfully merging this pull request may close these issues.

7 participants