-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Secret callback revisited #480
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
Changes from 9 commits
96cb28b
798b033
8d96f89
ddb1736
7d60544
6199e60
7a39153
fdfb2ef
e67f5d3
2e27e84
f83432f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
[*] | ||
indent_style = space | ||
indent_size = 2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
||
|
@@ -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) { | ||
// fetch secret or public key | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. jwt.verify |
||
console.log(decoded.foo) // bar | ||
}); | ||
|
||
``` | ||
|
||
### jwt.decode(token [, options]) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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')); | ||
|
@@ -67,6 +69,90 @@ describe('verify', function() { | |
}); | ||
}); | ||
|
||
describe('secret or token as callback', function () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to make this a thing with #486 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just saw that even when you are passing the |
||
}; | ||
|
||
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'; | ||
|
There was a problem hiding this comment.
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:
getKey(header, issuer, callback)
, and think if we want to check the issuer, if passed in options, before even calling thegetSecret
function.getKey
function (to see if the caller expects only header and callback or header, issuer and callback)What do you think?
There was a problem hiding this comment.
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 ;-)