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

crypto: move pbkdf2 without digest to EOL #31166

Open
wants to merge 2 commits into
base: master
from

Conversation

@jasnell
Copy link
Member

jasnell commented Jan 2, 2020

API has been being incrementally deprecated since 6.0.0

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
API has been being incrementally deprecated since 6.0.0
@jasnell jasnell force-pushed the jasnell:eol-DEP0009 branch from 0525dbc to a6d77ac Jan 2, 2020
lib/internal/crypto/pbkdf2.js Outdated Show resolved Hide resolved
@@ -61,7 +56,7 @@ function ondone(err, key) {

// Error path should not leak memory (check with valgrind).

This comment has been minimized.

Copy link
@Trott

Trott Jan 2, 2020

Member

Is this comment accurate for the test immediately below it? It's not obvious to me.

@@ -136,7 +131,7 @@ assert.throws(
{
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError',
message: 'The "digest" argument must be of type string or null. ' +
message: 'The "digest" argument must be of type string. ' +
'Received undefined'
});

This comment has been minimized.

Copy link
@Trott

Trott Jan 2, 2020

Member

Needs a test for the new behavior (throwing on null).

Suggested change
assert.throws(
() => crypto.pbkdf2Sync('password', 'salt', 8, 8, null),
{
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError',
message: 'The "digest" argument must be of type string. ' +
'Received undefined'
});
@lpinca
lpinca approved these changes Jan 4, 2020
Copy link
Member

lpinca left a comment

LGTM with @Trott's comment addressed.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Jan 6, 2020

Ping @jasnell

@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented Jan 7, 2020

Yep, haven't forgotten. This is on my list to get updated later this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.