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

Secret callback revisited #480

Merged
merged 11 commits into from Jun 11, 2018
3 changes: 3 additions & 0 deletions .editorconfig
@@ -0,0 +1,3 @@
[*]
indent_style = space
indent_size = 2
12 changes: 12 additions & 0 deletions README.md
Expand Up @@ -29,6 +29,7 @@ $ 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
Copy link
Contributor

Choose a reason for hiding this comment

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

This is in wrong place, right? it should be under jwt.verify section, where we should indicate secretOrPublicKey can be a function too.


`options`:

Expand Down Expand Up @@ -197,6 +198,17 @@ jwt.verify(token, cert, { algorithms: ['RS256'] }, function (err, payload) {
// if token alg != RS256, err == invalid signature
});

// Verify using getKey callback
function getKey(header, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to use this function for JWK + openid cases we would need to add the issuer as well.

The options are:

  • We add it here like getKey(header, issuer, callback), and think if we want to check the issuer, if passed in options, before even calling the getSecret function.
  • Leave it as is in this PR. Give support for that in a different one by checking the length of getKey function (to see if the caller expects only header and callback or header, issuer and callback)

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the sake of progress, i would like to keep it as is. Let's make an separate issue for it and i will pick it up after this one ;-)

// fetch secret or public key

Choose a reason for hiding this comment

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

I'd add some comment encouraging the users to cache the key with the kid so they avoid fetching it multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add https://github.com/auth0/node-jwks-rsa as an example to load the keys, that should make it more clear.

const key = ...;
callback(null, key);
}

verify(token, getKey, options, function(err, decoded) {

Choose a reason for hiding this comment

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

jwt.verify

console.log(decoded.foo) // bar
});

```

### jwt.decode(token [, options])
Expand Down
3 changes: 1 addition & 2 deletions package.json
Expand Up @@ -27,8 +27,7 @@
"lodash.isplainobject": "^4.0.6",
"lodash.isstring": "^4.0.1",
"lodash.once": "^4.0.0",
"ms": "^2.1.1",
"xtend": "^4.0.1"
"ms": "^2.1.1"
},
"devDependencies": {
"atob": "^1.1.2",
Expand Down
5 changes: 2 additions & 3 deletions sign.js
@@ -1,5 +1,4 @@
var timespan = require('./lib/timespan');
var xtend = require('xtend');
var jws = require('jws');
var includes = require('lodash.includes');
var isBoolean = require('lodash.isboolean');
Expand Down Expand Up @@ -85,7 +84,7 @@ module.exports = function (payload, secretOrPrivateKey, options, callback) {
var isObjectPayload = typeof payload === 'object' &&
!Buffer.isBuffer(payload);

var header = xtend({
var header = Object.assign({
alg: options.algorithm || 'HS256',
typ: isObjectPayload ? 'JWT' : undefined,
kid: options.keyid
Expand All @@ -112,7 +111,7 @@ module.exports = function (payload, secretOrPrivateKey, options, callback) {
return failure(error);
}
if (!options.mutatePayload) {
payload = xtend(payload);
payload = Object.assign({},payload);
}
} else {
var invalid_options = options_for_objects.filter(function (opt) {
Expand Down
84 changes: 84 additions & 0 deletions test/verify.tests.js
Expand Up @@ -3,8 +3,10 @@ var jws = require('jws');
var fs = require('fs');
var path = require('path');
var sinon = require('sinon');
var JsonWebTokenError = require('../lib/JsonWebTokenError');

var assert = require('chai').assert;
var expect = require('chai').expect;

describe('verify', function() {
var pub = fs.readFileSync(path.join(__dirname, 'pub.pem'));
Expand Down Expand Up @@ -67,6 +69,88 @@ describe('verify', function() {
});
});

describe('secret or token as callback', function () {
Copy link
Member

Choose a reason for hiding this comment

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

This function is not properly indented with the rest of the file and doesn't follow the newly introduced .editorconfig

Copy link
Contributor Author

@JacoKoster JacoKoster Jun 8, 2018

Choose a reason for hiding this comment

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

I'd like to make this a thing with #486

Choose a reason for hiding this comment

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

I agree with @jfromaniello on this one. Since the bad indentation is introduced in this PR there's no need to wait for a different one to fix it.

var token = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJmb28iOiJiYXIiLCJpYXQiOjE0MzcwMTg1ODIsImV4cCI6MTQzNzAxODU5Mn0.3aR3vocmgRpG05rsI9MpR6z2T_BGtMQaPq2YR6QaroU';
var key = 'key';

var payload = { foo: 'bar', iat: 1437018582, exp: 1437018592 };
var options = {algorithms: ['HS256'], ignoreExpiration: true};

it('without callback', function (done) {
jwt.verify(token, key, options, function (err, p) {
assert.isNull(err);
assert.deepEqual(p, payload);
done();
});
});

it('simple callback', function (done) {
var keyFunc = function(header, callback) {
callback(undefined, key);
Copy link
Contributor

Choose a reason for hiding this comment

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

I just saw that even when you are passing the decodedToken the tests pass. Can you assert here you get just the content of the header from the token?

};

jwt.verify(token, keyFunc, options, function (err, p) {
assert.isNull(err);
assert.deepEqual(p, payload);
done();
});
});

it('should error if called synchronously', function (done) {
var keyFunc = function(header, callback) {
callback(undefined, key);
};

expect(function () {
jwt.verify(token, keyFunc, options);
}).to.throw(JsonWebTokenError, /verify must be called asynchronous if secret or public key is provided as a callback/);

done();
});

it('simple error', function (done) {
var keyFunc = function(header, callback) {
callback(new Error('key not found'));
};

jwt.verify(token, keyFunc, options, function (err, p) {
assert.equal(err.name, 'JsonWebTokenError');
assert.match(err.message, /error in secret or public key callback/);
assert.isUndefined(p);
done();
});
});

it('delayed callback', function (done) {
var keyFunc = function(header, callback) {
setTimeout(function() {
callback(undefined, key);
}, 25);
};

jwt.verify(token, keyFunc, options, function (err, p) {
assert.isNull(err);
assert.deepEqual(p, payload);
done();
});
});

it('delayed error', function (done) {
var keyFunc = function(header, callback) {
setTimeout(function() {
callback(new Error('key not found'));
}, 25);
};

jwt.verify(token, keyFunc, options, function (err, p) {
assert.equal(err.name, 'JsonWebTokenError');
assert.match(err.message, /error in secret or public key callback/);
assert.isUndefined(p);
done();
});
});
});

describe('expiration', function () {
// { foo: 'bar', iat: 1437018582, exp: 1437018592 }
var token = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJmb28iOiJiYXIiLCJpYXQiOjE0MzcwMTg1ODIsImV4cCI6MTQzNzAxODU5Mn0.3aR3vocmgRpG05rsI9MpR6z2T_BGtMQaPq2YR6QaroU';
Expand Down