Clarify in README that buffer/string payloads must be JSON

[davidjb]

It does kinda go without saying that JWTs (JSON Web Tokens) need to contain JSON but it's worth mentioning that signing doesn't check your payload. In some unit tests I was writing where the payload was a dummy (non-JSON parsable) string, JWTs were being signed okay but they're not valid according to the spec, which caused decoding of the JWTs to later fail.

[ziluvatar]

I'd like to get more knowledge about it. When I run this:

> token = jwt.sign('content', key)
'eyJhbGciOiJIUzI1NiJ9.Y29udGVudA.WyRVFmgOi-GCpfKgXZ14g1aG3NkB--P9349LLuZkKsE'
> jwt.verify(token, key)
'content'

It works, what is the input you are using?

[davidjb]

Sure, no problem. For note, the spec states that the JWT payload ("JWT Claims Set") represents a JSON object at https://tools.ietf.org/html/rfc7519#section-4.

A library like https://github.com/auth0/jwt-decode fails to decode your token there because it doesn't contain JSON:

decode = require('jwt-decode')
decode('eyJhbGciOiJIUzI1NiJ9.Y29udGVudA.WyRVFmgOi-GCpfKgXZ14g1aG3NkB--P9349LLuZkKsE')
> InvalidTokenError
    at Object.<anonymous> (/tmp/node_modules/jwt-decode/lib/index.js:9:31)
    at Module._compile (module.js:660:30)
    at Object.Module._extensions..js (module.js:671:10)
    at Module.load (module.js:573:32)
    at tryModuleLoad (module.js:513:12)
    at Function.Module._load (module.js:505:3)
    at Module.require (module.js:604:17)
    at require (internal/module.js:11:18)

Node's traceback is a little hard to follow but the code throws the exception here, in the JSON.parse call: https://github.com/auth0/jwt-decode/blob/master/lib/index.js#L20.

By comparison, node-jsonwebtoken doesn't verify JSON validity when signing -- or seemingly during verification as your example shows. The fact that invalid JWTs can be created may be indicative of a more detailed fix required in this package but it depends on whether the purpose of the library is to validate JSON strings or just trust the input. I wouldn't imagine validation would be hard to do (just attempt JSON.parse on strings and throw errors accordingly) if you wanted to go that way.

With this specific PR, I was solving the issue in a simpler way -- documenting the current behaviour so a user knows this package isn't going to verify JSON.

[ziluvatar]

Thanks @davidjb, merged, I guess we would need to tackle that in some future major version release to align with the spec.

[davidjb]

Great, thanks @ziluvatar. Even a simple solution like throwing an exception on a failed JSON.parse() would be enough. However, perhaps it's a good opportunity to rethink whether the existing (object) validation code and so on in jwt.sign() should/could apply to strings too.