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

examples: fix pbkdf2 invocation #3207

Closed
wants to merge 1 commit into from
Closed

Conversation

addaleax
Copy link

Calling crypto.pbkdf2() without a digest has been deprecated in Node and is scheduled to be broken in Node 8.

Fix this by actually passing a digest.

ref: nodejs/node#11305

Note: npm test still fails with Node master for me, but I’m on it :S

Calling `crypto.pbkdf2()` without a digest has been deprecated in Node
and is scheduled to be broken in Node 8.

Fix this by actually passing a digest.

ref: nodejs/node#11305
@dougwilson
Copy link
Contributor

I think Node.js master is broken also from #3174 , though I haven't herd back on my question in that PR since it seems to break existing uses with the change.

@dougwilson
Copy link
Contributor

P.S. it looks like the change here somehow breaks Node.js 0.10 usage. Does this mean it may no longer be possible to have code for this method that works in Node.js 0.10 and Node.js 8 ?

@addaleax
Copy link
Author

I think Node.js master is broken also from #3174

Right, thanks for looking that up and saving me from bisecting Node. :S

Does this mean it may no longer be possible to have code for this method that works in Node.js 0.10 and Node.js 8 ?

That might just be the case. :/

@jasnell just fyi…

@jasnell
Copy link

jasnell commented Feb 15, 2017

Hmm... it may be that we just need to back this back out and give it more time as a runtime deprecation. We can take this back to the @nodejs/ctc for discussion

@jasnell
Copy link

jasnell commented Feb 15, 2017

The challenge here is that the change was made for a very good security reason (avoiding silently defaulting to SHA1). We really do want this change to stick. Perhaps there's a way to shim it when running in 0.10 but obviously that has draw backs also

@dougwilson
Copy link
Contributor

Thanks for the input @addaleax and @jasnell . Of course this is just example code, so even if this does land in Node.js 8 we'll get something fixed in Express, perhaps simplifying this example in general :) I don't think this example code in Express should force a different decision, though.

I should have taken a closer look at the build logs, as the deprecation warning is in the Express build logs on Travis for 6.x 🙃

@dougwilson
Copy link
Contributor

So thinking a bit about this today, and mulling about either removing the hashing altogether, using a simple hash, or a module, I think I'm leaning towards just placing in pbkdf2-password for now to keep the mostly-good example for the time being to hold over until the examples end up breaking out into another repository. That module checks process.version to make the determination (probably the only good solution left).

@dougwilson
Copy link
Contributor

Technically we could also just add the detection in here, but I'd rather punt that to something else :) Plus removing that pass.js file is kina nice, to keep the focus on the express example.

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

Successfully merging this pull request may close these issues.

3 participants