Skip to content
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

Rename decode function unsafeDecode to highlight the risk #952

Open
marine-mb opened this issue Dec 22, 2023 · 6 comments
Open

Rename decode function unsafeDecode to highlight the risk #952

marine-mb opened this issue Dec 22, 2023 · 6 comments

Comments

@marine-mb
Copy link

marine-mb commented Dec 22, 2023

Describe the problem you'd like to have solved

I'm a DevSec. I help developers teams find and fix their vulnerabilities.
In two of my last missions, I've seen developers using decode instead of verify even if the documentation has been improved.

Describe the ideal solution

To avoid this and reduce the vulnerable apps, I think it would be great to rename the decode function and call it unsafeDecode. (as suggested by @youssef-jbili)

Other libraries have done it and it helps developers reduce their mistakes:

  • dangerouslySetInnerHTML for React
  • bypassSecurityTrustHtml for Angular

I can make a Pull Request if you think it is a good idea.

aalu-love added a commit to aalu-love/node-jsonwebtoken that referenced this issue Dec 30, 2023
…ht the risk

auth0#952 I have made the changes as per above mentioned in the above comment.
@youssef-jbili
Copy link

Very interesting proposal. I would just suggest calling it unsafeDecode instead to maintain the same camel case naming

@jonaskello
Copy link

jonaskello commented Jan 8, 2024

I would suggest unverifiedDecode to keep it within the same wording as verify() function. I think it also would be nice to be able to take this unverified decoded token and verify it (see #955). Code would then be something like:

const unverifiedToken = unverifiedDecode(rawToken);
const verifiedToken = verifyDecoded(unverifiedToken);

The current verify() function actually does decoding also so a better name for that IMO would be verifiedDecode(). So IMO the ideal API would be:

  • unverifiedDecode(rawToken) - this is the same as current decode()
  • verifiedDecode(rawToken, ...) - this is the same as current verify()
  • verifyDecoded(unverifiedDecodedToken, ...) - new function to just verify an unverified but decoded token (returns Promise)

@marine-mb
Copy link
Author

Hi @jonaskello,

I think unverifiedDecode is less clear for developers.
It should be evident that it is dangerous!

Also, verifying a token after it has been decoded is impossible.
The JWT is split into three parts: header, payload and signature. If the JWT is modified, the signature is not valid anymore.
When you decode the JWT, you only get the payload (without the base64 encoding) so you can't verify the signature anymore.
I hope it's clear.

@aalu-love, thanks for the PR. Would you update it to use camelCase as @youssef-jbili suggested? Otherwise, I can do it at the end of the week 😁

@jonaskello
Copy link

@marine-mb When you decode you get all three parts as can be seen here

return {
      header: decoded.header,
      payload: payload,
      signature: decoded.signature
    };

So not only the payload. However I think you might be saying the signareture is calculated on the raw base64 string? In that case I understand the limitation.

@marine-mb
Copy link
Author

marine-mb commented Jan 12, 2024

Hi @jonaskello,

Thank you for your answer.
My mistake, the signature can still be recovered. However, as you said, you still need the encoded token for the verification.
As seen here in the verify function, we give the encoded JWT as a string with the algorithm and the key (secret or public) to jws library.
So we would have to encode it again.

And even if we could give the decoded token, I think it is dangerous to give the impression it is correct to exploit data in the JWT before verifying the signature.
It could lead to flaws such as injections, broken access control, etc...

@marine-mb marine-mb changed the title Rename decode function unsafe_decode to highlight the risk Rename decode function unsafeDecode to highlight the risk Jan 16, 2024
@georgejmx
Copy link

georgejmx commented Jun 9, 2024

#972 would allow sanitising/validating the token payload without using decode, so if using decode to check validity of the token then this MR would remove the need for it.

For example if a backend is expecting a certain field of the payload to be a certain value e.g. clientId === 123, we can now achieve this without using decode at all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants