mxstbr / passport-magic-login

Passwordless authentication with magic links for Passport.js.
MIT License
668 stars 45 forks source link

Address CVE-2022-23540 by updating jsonwebtoken #27

Closed enberg closed 1 year ago

enberg commented 1 year ago

Hi!

TL;DR: This updates the jsonwebtoken dependency to v9.0.0

There's an issue affecting the jsonwebtoken package in versions <= 8.5.1 which might lead to verifying unsigned tokens unintentionally. More detailed information is available in the Github Advisory Database

This has been fixed in v9.0.0 by requiring users to specify that they actually want to allow this by passing it along as an option. There's an upgrade guide available in the library docs and as far as I can tell the breaking changes shouldn't affect the intended use of this library.

The effect of this change can be tested by passing a falsy key an unsigned token to the decodeToken function, before this change it behaves like this:

> decodeToken('', 'eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJmb28iOiJiYXIiLCJpYXQiOjE2NzM0MzA0MTF9.')
{ foo: 'bar', iat: 1673430411 }

...and after the change:

> decodeToken('', 'eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJmb28iOiJiYXIiLCJpYXQiOjE2NzM0MzA0MTF9.')
Uncaught:
JsonWebTokenError: please specify "none" in "algorithms" to verify unsigned tokens
    at /Users/enberg/Projects/OpenSource/passport-magic-login/node_modules/jsonwebtoken/verify.js:117:19
    at getSecret (/Users/enberg/Projects/OpenSource/passport-magic-login/node_modules/jsonwebtoken/verify.js:97:14)
    at module.exports [as verify] (/Users/enberg/Projects/OpenSource/passport-magic-login/node_modules/jsonwebtoken/verify.js:101:10)
    at decodeToken ([eval]:29:14)

The way a user might bump into this is if they specify a falsy secret to MagicLoginStrategy which would probably be unintentional most of the time, e.g. a missing env var or the like.

I have a couple of questions I'll leave up to you:

Would you like to support the use-case of unsigned tokens? It could be done the same way as options are passed to the sign method now, but it would clutter up the interface a bit.

Would you like specific error handling around the error thrown by jsonwebtoken or is it sufficient to let it bubble up? My thoughts are around the wording of the error, it asks the user to specify options that are not visible to the end-users of this library.

mxstbr commented 1 year ago

Published in v1.2.2!