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.

Sign up for GitHub

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

Jump to bottom

Secret callback revisited #480

Merged
merged 11 commits into from Jun 11, 2018
Merged

Secret callback revisited #480

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
96cb28b
Introduction of the secret callback
JacoKoster Jun 3, 2018
798b033
Removed some spaces...
JacoKoster Jun 3, 2018
8d96f89
Removed xtend as a dependency, as the native Object.Assign can do thi…
JacoKoster Jun 3, 2018
ddb1736
Removed xtend as a dependency, as the native Object.Assign can do thi…
JacoKoster Jun 3, 2018
7d60544
Merge branch 'add-secret-for-callback' of https://github.com/JacoKost…
JacoKoster Jun 3, 2018
6199e60
Resolve feedback from review
JacoKoster Jun 8, 2018
7a39153
Added extra test and fixed the associated bug
JacoKoster Jun 8, 2018
fdfb2ef
The return of the header
JacoKoster Jun 8, 2018
e67f5d3
Forgot to change this one as well... Sorry bout that
JacoKoster Jun 8, 2018
2e27e84
Updated the readme and made the if-statements consistent
JacoKoster Jun 8, 2018
f83432f
Space; The final frontier
JacoKoster Jun 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Expand Up @@ -29,7 +29,6 @@ $ npm install jsonwebtoken

`secretOrPrivateKey` is a string, buffer, or object containing either the secret for HMAC algorithms or the PEM
encoded private key for RSA and ECDSA. In case of a private key with passphrase an object `{ key, passphrase }` can be used (based on [crypto documentation](https://nodejs.org/api/crypto.html#crypto_sign_sign_private_key_output_format)), in this case be sure you pass the `algorithm` option.
If `jwt.verify` is called asynchronous, `secretOrPublicKey` can be a function that should fetch the secret or public key. See below for a detailed example

`options`:

Expand Down Expand Up @@ -123,6 +122,7 @@ jwt.sign({

`secretOrPublicKey` is a string or buffer containing either the secret for HMAC algorithms, or the PEM
encoded public key for RSA and ECDSA.
If `jwt.verify` is called asynchronous, `secretOrPublicKey` can be a function that should fetch the secret or public key. See below for a detailed example

As mentioned in [this comment](https://github.com/auth0/node-jsonwebtoken/issues/208#issuecomment-231861138), there are other libraries that expect base64 encoded secrets (random bytes encoded using base64), if that is your case you can pass `Buffer.from(secret, 'base64')`, by doing this the secret will be decoded using base64 and the token verification will use the original random bytes.

Expand Down
50 changes: 21 additions & 29 deletions verify.js
View file
Open in desktop
Expand Up @@ -43,22 +43,6 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) {
return done(new JsonWebTokenError('jwt must be a string'));
}

var parts = jwtString.split('.');

if (parts.length !== 3){
return done(new JsonWebTokenError('jwt malformed'));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be kept here, it refers to the token format, not secret related, there is no need to potentially call the getSecret function if the jwt is a malformed


var hasSignature = parts[2].trim() !== '';

if (!hasSignature && secretOrPublicKey){
return done(new JsonWebTokenError('jwt signature is required'));
}

if (hasSignature && !secretOrPublicKey) {
return done(new JsonWebTokenError('secret or public key must be provided'));
}

var decodedToken;
try {
decodedToken = decode(jwtString, { complete: true });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this {complete:true} option wasn't being passed before. How does that affect the output @ziluvatar ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mostly the same, however, our decode implementation tries to parse always the JSON (if string), the jws.decode only does it when the type is JWT. With this and other change afterwards we reduce the amount of parsing to half.

Expand All @@ -70,8 +54,6 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) {
return done(new JsonWebTokenError('invalid token'));
}

var header = decodedToken.header;

var getSecret;

if(typeof secretOrPublicKey === 'function') {
Expand All @@ -82,16 +64,32 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) {
getSecret = secretOrPublicKey;
}
else {
getSecret = function(header, secretCallback) {
getSecret = function(decodedToken, secretCallback) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even if it is not used, can you rename it back to header as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JacoKoster I saw your last commit after this comment, but that commit does not address this (not sure if you realized about it)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, i forgot this one.

return secretCallback(null, secretOrPublicKey);
};
}

return getSecret(header, function(err, secretOrPublicKey) {
return getSecret(decodedToken, function(err, secretOrPublicKey) {
Copy link
Contributor

@ziluvatar ziluvatar Jun 8, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 why do you pass all the decoded token now?, I think it was fine passing just the decodedToken.header, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we will be using both the header and the payload of the token now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, wait, yes. that is wrong.

if(err) {
return done(new JsonWebTokenError('error in secret or public key callback: ' + err.message));
}

var parts = jwtString.split('.');

if (parts.length !== 3){
return done(new JsonWebTokenError('jwt malformed'));
}

var hasSignature = parts[2].trim() !== '';

if (!hasSignature && secretOrPublicKey){
return done(new JsonWebTokenError('jwt signature is required'));
}

if (hasSignature && !secretOrPublicKey) {
return done(new JsonWebTokenError('secret or public key must be provided'));
}

if (!hasSignature && !options.algorithms) {
options.algorithms = ['none'];
}
Expand All @@ -106,28 +104,22 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) {

}

if (!~options.algorithms.indexOf(header.alg)) {
if (!~options.algorithms.indexOf(decodedToken.header.alg)) {
return done(new JsonWebTokenError('invalid algorithm'));
}

var valid;

try {
valid = jws.verify(jwtString, header.alg, secretOrPublicKey);
valid = jws.verify(jwtString, decodedToken.header.alg, secretOrPublicKey);
} catch (e) {
return done(e);
}

if (!valid)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please be consistent on the braces usage. A few lines above this one, another one liner if clause uses them.

Copy link
Contributor Author

@JacoKoster JacoKoster Jun 8, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed this as well, but because it was already on master this way. I was planning to add eslint and fixing it in one go.

return done(new JsonWebTokenError('invalid signature'));

var payload;

try {
payload = decode(jwtString);
} catch (err) {
return done(err);
}
var payload = decodedToken.payload;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️


if (typeof payload.nbf !== 'undefined' && !options.ignoreNotBefore) {
if (typeof payload.nbf !== 'number') {
Expand Down