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 @@ -122,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 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
86 changes: 86 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,90 @@ 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) {
assert.deepEqual(header, { alg: 'HS256', typ: 'JWT' });

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