Skip to content

Commit

Permalink
add a console.warn on invalid options for string payloads
Browse files Browse the repository at this point in the history
  • Loading branch information
jfromaniello committed Jan 4, 2016
1 parent 65b1f58 commit 71200f1
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 1 deletion.
20 changes: 19 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,33 @@ JWT.decode = function (jwt, options) {
return payload;
};

var payload_options = [
'expiresIn',
'notBefore',
'expiresInMinutes',
'expiresInSeconds',
'audience',
'issuer',
'subject',
'jwtid'
];

JWT.sign = function(payload, secretOrPrivateKey, options, callback) {
options = options || {};
payload = typeof payload === 'object' ? xtend(payload) : payload;
var header = {};

if (typeof payload === 'object') {
header.typ = 'JWT';
payload = xtend(payload);
} else {
var invalid_option = payload_options.filter(function (key) {
return typeof options[key] !== 'undefined';
})[0];

console.warn('invalid "' + invalid_option + '" option for ' + (typeof payload) + ' payload');

This comment has been minimized.

Copy link
@carlnordenfelt

carlnordenfelt Jan 4, 2016

This has been causing some confusion for my team and it took a bit of troubleshooting to find that the problem was not actually a problem.
It outputs the console.warn even if there are no options (or only undefined options) specified.

A nice improvement to this would be:

if (invalid_option) {
    console.warn(...);
}

This comment has been minimized.

Copy link
@jfromaniello

jfromaniello Jan 4, 2016

Author Member

you are right, sorry for the trouble

This comment has been minimized.

Copy link
@jfromaniello

jfromaniello Jan 4, 2016

Author Member

@polythene1337 fixed in v5.5.4

This comment has been minimized.

Copy link
@carlnordenfelt

carlnordenfelt Jan 4, 2016

Cheers,

I came across another thing that is somewhat related to this.

If I specify the payload as a string (which is possible according to the deocumentation) and pass options.expiresIn I get the following error message:

[Error: "expiresIn" should be a number of seconds or string representing a timespan eg: "1d", "20h", 60]

This appears to be due the payload never being converted to an object, thus setting payload.exp on line 105 doesn't do any good and causes the if (payload.exp) on line 106 check to fail with a message indicating that my time format is incorrect although I passed a valid time string. I'm not sure what code is at fault, if it's the docs, the error message or the loss of string payload to objet conversion.

This comment has been minimized.

Copy link
@jfromaniello

jfromaniello Jan 4, 2016

Author Member

yes, you are right about that too.

I think instead of a warning I should throw an error.. this has been source of lot of confusion in the past.

This comment has been minimized.

Copy link
@carlnordenfelt

carlnordenfelt Jan 4, 2016

I agree, if options are not allowed it should fail immediately here with a clear message saying why. I think some clarification in the docs would be good too :)

}


header.alg = options.algorithm || 'HS256';

if (options.headers) {
Expand Down
7 changes: 7 additions & 0 deletions test/non_object_values.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ describe('non_object_values values', function() {
expect(result).to.equal('hello');
});

//v6 version will throw in this case:
it.skip('should throw with expiresIn', function () {
expect(function () {
jwt.sign('hello', '123', { expiresIn: '12h' });
}).to.throw(/invalid expiresIn option for string payload/);
});

it('should fail to validate audience when the payload is string', function () {
var token = jwt.sign('hello', '123');
expect(function () {
Expand Down

0 comments on commit 71200f1

Please sign in to comment.