-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix(verify)!: Remove default none
support verify
methods, and require it to be explicitly configured
#851
fix(verify)!: Remove default none
support verify
methods, and require it to be explicitly configured
#851
Changes from 1 commit
1a95784
4021b3c
d46a1f6
5705a41
32ca143
6ab90a5
c673e4f
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 |
---|---|---|
|
@@ -41,16 +41,6 @@ describe('signing a token asynchronously', function() { | |
}); | ||
}); | ||
|
||
//Known bug: https://github.com/brianloveswords/node-jws/issues/62 | ||
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. Did we mean to remove this test? 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 is something I changed as part of David's comment and was changed when we were removing the |
||
//If you need this use case, you need to go for the non-callback-ish code style. | ||
it.skip('should work with none algorithm where secret is falsy', function(done) { | ||
jwt.sign({ foo: 'bar' }, undefined, { algorithm: 'none' }, function(err, token) { | ||
expect(token).to.be.a('string'); | ||
expect(token.split('.')).to.have.length(3); | ||
done(); | ||
}); | ||
}); | ||
|
||
it('should return error when secret is not a cert for RS256', function(done) { | ||
//this throw an error because the secret is not a cert and RS256 requires a cert. | ||
jwt.sign({ foo: 'bar' }, secret, { algorithm: 'RS256' }, function (err) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ const util = require('util'); | |
const testUtils = require('./test-utils'); | ||
|
||
function signWithAudience(audience, payload, callback) { | ||
const options = {algorithm: 'none'}; | ||
const options = {algorithm: 'HS256'}; | ||
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. We may be able to revert all these test changes with the latest change to 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. We would be able to revert but the places that we use none and verify we would need to configure that algorithm explicitly, do you see an advantage in using I think I prefer using Let me know what you think :) 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. Yep agreed :) |
||
if (audience !== undefined) { | ||
options.audience = audience; | ||
} | ||
|
@@ -15,7 +15,7 @@ function signWithAudience(audience, payload, callback) { | |
} | ||
|
||
function verifyWithAudience(token, audience, callback) { | ||
testUtils.verifyJWTHelper(token, undefined, {audience}, callback); | ||
testUtils.verifyJWTHelper(token, 'secret', {audience}, callback); | ||
} | ||
|
||
describe('audience', function() { | ||
|
@@ -47,7 +47,7 @@ describe('audience', function() { | |
|
||
// undefined needs special treatment because {} is not the same as {aud: undefined} | ||
it('should error with with value undefined', function (done) { | ||
testUtils.signJWTHelper({}, 'secret', {audience: undefined, algorithm: 'none'}, (err) => { | ||
testUtils.signJWTHelper({}, 'secret', {audience: undefined, algorithm: 'HS256'}, (err) => { | ||
testUtils.asyncCheck(done, () => { | ||
expect(err).to.be.instanceOf(Error); | ||
expect(err).to.have.property('message', '"audience" must be a string or array'); | ||
|
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.
Is this comment necessary? Since
algorithm
must be supplied, there's no danger of someone unintentionally creating an unsigned tokenThere 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.
Thats true, this sign method has always been explicit with the algorithm - my reasoning was mainly to make this as easily as possible for new people adopting the library.
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.
I'm not sure we want to make it easy to create
none
tokens :P I'd remove this personally, I think it adds confusion to someone wanting to use the library who isn't hugely experienced with best practices here - let's not call outnone
specifically.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.
Yeah maybe if this documentation was to exist it would need to be included in a best practises section, which describes that
none
tokens should only be used for testing and not for production. Removing this to avoid confusing users sounds fine to me.