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 redundant code (for Node.js 0.9.4 and below) and dependency #2885

Merged
merged 7 commits into from
May 15, 2018

Conversation

ChALkeR
Copy link
Contributor

@ChALkeR ChALkeR commented Mar 3, 2018

PR Checklist:

  • I have run npm test locally and all tests are passing.
  • I have added/updated tests for any new behavior.
  • If this is a significant change, an issue has already been created where the problem / solution was discussed: [N/A, or add link to issue here]

PR Description

That code was needed for Node.js 0.9.4 and below, which is not supported anymore.

The tests don't pass, but the exact same tests that fail were already failing on master prior to this change.

/cc @mikeal

@jeradg
Copy link

jeradg commented May 13, 2018

May I ask what the status of this PR is? The Hackerone report of stringstream's vulnerability was disclosed today (https://hackerone.com/reports/321670). I'd love to get a patched version so Snyk will stop complaining!

@ChALkeR
Copy link
Contributor Author

ChALkeR commented May 15, 2018

/cc @simov, perhaps?

@simov
Copy link
Member

simov commented May 15, 2018

I wanted to make tests green again, first. There was an effort on that front, if you merge master you'll see something like this https://travis-ci.org/request/request/builds/373888724

Besides the warning messages, there is an error with the certificates on node v10:

Error: error:140AB18F:SSL routines:SSL_CTX_use_certificate:ee key too small

I've tried generating new certificates numerous times with various options, but it never stopped complaining. @ChALkeR can you help me with that?

@ChALkeR
Copy link
Contributor Author

ChALkeR commented May 15, 2018

@simov E.g. this works for me in the tests/ssl/ca dir:

openssl genrsa 4096 > server.key
openssl req -new -sha256 -key server.key -config server.cnf -out server.csr
openssl x509 -req -sha256 -in server.csr -CA ca.crt -CAkey ca.key -out server.crt

Not sure what exactly do you want to test, so better verify that it does what you want it to do.

Your ca.key password seems to be password, you will have to type that in.

That one was needed for Node.js 0.9.4 and below,
which is not supported anymore.
@ChALkeR
Copy link
Contributor Author

ChALkeR commented May 15, 2018

Hope that helps! I also rebased this PR against current master.

@simov simov mentioned this pull request May 15, 2018
@simov simov merged commit 76a6e5b into request:master May 15, 2018
@ChALkeR
Copy link
Contributor Author

ChALkeR commented May 15, 2018

🎉

@ChALkeR
Copy link
Contributor Author

ChALkeR commented May 15, 2018

For posterity, the only commit in this PR was 81f8cb5, all the other changes were introduced while landing and come from #2942, not sure how they ended up here.

@simov
Copy link
Member

simov commented May 15, 2018

It's my fault.

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.

3 participants