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

Remove rejectUnauthorized:false from download flags #3086

Closed
wants to merge 3 commits into from

Conversation

AWare
Copy link

@AWare AWare commented Apr 13, 2021

This should resolve #3067 rejectUnauthorized: false was added in #567

It would seem more important to resolve the CVE-2020-24025 than to support misconfigured enterprise proxies.

@nschonni
Copy link
Contributor

I'd be OK with dropping this, but I think we need some sort of documentation or environment variable opt-in.
I'm not sure if https://stackoverflow.com/questions/35633829/node-js-error-process-env-node-tls-reject-unauthorized-what-does-this-mean covers off the same behaviour, or if a documented NODE_SASS_REJECT_UNAUTHORIZED makes more sense

@AWare
Copy link
Author

AWare commented Apr 13, 2021

Thanks for looking at this.

If the libsass opinion is to have it on a switch, I would like to respectfully disagree. HTTP support is absolutely the expected behaviour by end users, and this CVE has prompted us to reevaluate our use of node-sass.

I would personally be in favour of suggesting users download libsass themselves if it cannot be done safely over https- I don't think the documentation would benefit from detail here. I would be more than happy to add something to this effect to TROUBLESHOOTING.md.

@saper
Copy link
Member

saper commented Apr 16, 2021

How to set one's own custom trusted root certificate instead for poor folks behind SSL intercepting corporate proxies?

@AWare
Copy link
Author

AWare commented Apr 20, 2021

As described, they can provide the binary themselves.

@xzyfer
Copy link
Contributor

xzyfer commented Apr 21, 2021

The linked SO post suggest NODE_TLS_REJECT_UNAUTHORIZED="0" npm install node-sass would have the current behaviour. If this can be confirmed then we can ship this with that tip to the troubleshooting guide.

@AWare
Copy link
Author

AWare commented Apr 21, 2021

image

I would prefer if that message were provided by the team, as I don't feel comfortable recommending that users disable HTTPS.

@saper
Copy link
Member

saper commented Apr 21, 2021

I would prefer if that message were provided by the team, as I don't feel comfortable recommending that users disable HTTPS.

What exactly happens with this pull request if going through, say, SSL-intercepting proxy? What message the users will get? We have to work from there.

@AWare
Copy link
Author

AWare commented Apr 26, 2021

Sorry but I have no way of testing this. The balance of importance given to fixing a CVE vs convenience of users with ssl interception is somewhat confounding.

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.

Security Vulnerability Issue [CVE-2020-24025]
4 participants