rename 'decode' to 'unsafeDecode'

[punmechanic]

Description

'decode' is often reached to by well-meaning developers who assume that the call is safe. Unfortunately, this leads to untrusted JWTs being accepted as otherwise acceptable input.

Renaming 'decode' to 'unsafeDecode' signals to the end user that the method they are calling is indeed unsafe without them having to remember the difference between 'decode' and 'verify'.

This is a breaking change as it changes the public API and as such should be a major version bump. Ultimately this PR more serves to highlight this issue - I don't have a problem if this PR is not accepted as long as it starts a discussion on whether or not being able to decode a JWT without actually verifying it should be part of the public API of this library.

Should this PR be accepted, I would recommend a deprecation warning on the next minor (patch?) version bump with the rename taking place on the next major version bump.

References

Prompted by https://news.ycombinator.com/item?id=24347519 (thanks @deadmonstor) and experiencing this issue myself a few times over the past few weeks when reviewing code at my company.

Testing

It's a method rename, so all that needs to be done is run the existing tests (They all pass)

Checklist

• I have added documentation for new/changed functionality in this PR or in auth0.com/docs
• All active GitHub checks for tests, formatting, and security are passing
• The correct base branch is being used, if not master

[punmechanic]

Ah, it appears my auto-formatter was a bit overzealous, I'll fix that

[deadmonstor]

I think this is the HackerNews link you are looking for

[punmechanic]

It is! thank you

[mcastany]

Hi @danpantry,

First, big thank you for kicking off this conversation. After an internal discussion, which included inspecting the adjacent library space, we believe the decode function name serves a purpose and should remain. That said, this conversation has surfaced some changes that we believe would make the distinction of verify and decode much clearer and easier to discover. Here is our thinking.

“Decode” is a widely adopted term that is used to describe translating data from one (encoded) format into something readable. Decoding is commonly used with Base64, URLs, SAML Tokens and in all of these contexts it has the same meaning. There’s no relationship between decoding and security.

We recognize that it's possible for a developer to misinterpret "decode" as a function that should be called, when in most cases "verify" is more appropriate. So we looked at the ways API methods are discovered and checked to ensure that both our API documentation and the DefinitelyTyped documentation actually explicitly mention that “decode” does not verify the token.

In addition, we are going to put more structure into our API documentation, making it less likely a developer stumbles upon “decode” rather than “verify.” We will put more emphasis on “decode” not doing verification.

Lastly, we plan to make the “decode” function non-enumerable just in case REPL is ever the means of method discovery.

Thank you again to our awesome developer community, and keep the feedback coming.

[punmechanic]

Hey Marcos, Thanks for the reply. My concern is that many people are just going to download the package through npm, boot up $TEXTEDITOR, use autocomplete to come across decode() and then use that. Obviously, there's a certain limit to how much you should protect someone from themselves, but I still don't know if I agree that improving documentation will help that much here; The documentation is already quite explicit that decode **does not** verify tokens. That said, I think this is still a step in the right direction, especially as we hear of more high-profile exploits that utilise this exact vulnerability in other software of a similar nature <https://krystalgamer.github.io/avengers-emulator/>.

…

On Fri, Sep 4, 2020 at 8:52 PM Marcos Castany ***@***.***> wrote: Hi @danpantry <https://github.com/danpantry>, First, big thank you for kicking off this conversation. After an internal discussion, which included inspecting the adjacent library space, we believe the decode function name serves a purpose and should remain. That said, this conversation has surfaced some changes that we believe would make the distinction of verify and decode much clearer and easier to discover. Here is our thinking. “Decode” is a widely adopted term that is used to describe translating data from one (encoded) format into something readable. Decoding is commonly used with Base64, URLs, SAML Tokens and in all of these contexts it has the same meaning. There’s no relationship between decoding and security. We recognize that it's possible for a developer to misinterpret "decode" as a function that should be called, when in most cases "verify" is more appropriate. So we looked at the ways API methods are discovered and checked to ensure that both our API documentation <https://github.com/auth0/node-jsonwebtoken#jwtdecodetoken--options> and the DefinitelyTyped documentation <https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/jsonwebtoken/index.d.ts#L203-L210> actually explicitly mention that “decode” does not verify the token. In addition, we are going to put more structure into our API documentation, making it less likely a developer stumbles upon “decode” rather than “verify.” We will put more emphasis on “decode” not doing verification. Lastly, we plan to make the “decode” function non-enumerable just in case REPL is ever the means of method discovery. Thank you again to our awesome developer community, and keep the feedback coming. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#741 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWNSJMOMRRU7CQ4DTFJEO3SEFAPDANCNFSM4QS3BGUA> .