[Security Risk] Dangerous flow when JWT is incorrect or missing about passport-magic-login

[mxstbr]

Thanks for double-checking my work, I had missed that nuance. Fixed in v1.0.11!

[mxstbr]

Published the second idea in v1.0.10. I appreciate the detailed writeup and suggestions @lukasz-wronski!

[mxstbr]

Thank you for the careful read of the source; I agree that this isn't an optimal implication of the API design. (even though I wish that JS-based ORMs handled this properly 😬)

Do you think something like if (!payload.destination) throw new Error('Please provide a destination') would solve this potential hazard?

[lukasz-wronski]

Hey @mxstbr, thanks for quick reaction. In fact this is discovery was made with a real system I was pentesting. Developers were confident the links are verified correctly and I've managed to login as a random user just by messing the token up.

Regarding the code change. In my opinion it would be best to catch it as soon as possible adding this exception right inside the decodeToken function. In example like this:

export const decodeToken = (secret: string, token?: string) => {
  try {
    return jwt.verify(token, secret) as JwtPayload;
  } catch (err) {
    throw new Error('JWT incorrect or missing');
  }
};

Please note that if (typeof token !== 'string') you have now is redundant as jsonwebtoken library already checks this in the line below:

https://github.com/auth0/node-jsonwebtoken/blob/74d5719bd03993fcf71e3b176621f133eb6138c0/verify.js#L56

I think it's safe to just put anything into jwt.verify and rely on @auth0 to have all these checks implemented correctly.

If you want to make it even simplier you can make decodeToken to directly call jwt.verify and do no try..catch wrapping. Then your users will get more detailed error about the problem with the JWT provided. Your choice, both options seem to be right.

Let me know if I can assist you further.

[lukasz-wronski]

@mxstbr One thing that I can see you've left the if (typeof token !== 'string') and it's still working the same way as before in case I'm not adding ?token in query string. Undefined is not a string so it returns false. It still needs to be fixed.