Rename parsing methods with misleading names

[Marcono1234]

Fixes #212

The interface JwtParser has multiple methods with misleading names:

• parse: Allows both signed and unsigned tokens
• parseClaimsJwt, parsePlaintextJwt: Name is misleading because JWT only describes the data format, so a signed token can also be considered a JWT. Additionally there is only a one character difference (Jwt vs. Jws) between the method for unsigned and signed tokens, making it likely that the wrong method is chosen from IDE auto complete, and it is likely to be missed during code review.

This has lead to multiple applications being vulnerable due to having used the wrong method by accident. #212 lists a few, additionally some further prominent examples (based on https://github.com/github/securitylab/issues/333):

• DP3T
• Apache Pulsar
• NHS Contact Tracing App

This pull request deprecates these methods and instead adds methods with more expressive names. Note that this is a binary incompatible change because it introduces new abstract methods for the JwtParser interface. However, that is most likely not an issue because user code probably does not implement JwtParser. It appears this issue cannot be solved in a different way because this project is built with Java 7 which does not support default methods for interfaces.

[coveralls]

Coverage remained the same at 100.0% when pulling ace4245872c88236fdebcd1614f0ad9c49968e36 on Marcono1234:marcono1234/misleading-parse-names into a4130dd1ec5d4bef4b1835927a2a1d966a519109 on jwtk:master.

[Marcono1234]

@bdemers, what is your opinion on this?

[bdemers]

@Marcono1234, In general, I'm +1, but I think the methods should be structured something like:

public Jwt parse(String jwt, boolean allowUnsigned);

// default to require signature
public Jwt parse(String jwt) {
  this.parse(jwt, false);  
}

This would be a breaking change though, but it is something we can do for the next major release.

Thoughts?

[lhazlewood]

All of this has been addressed in the jwe branch: in that branch, it is not possible to misuse the parse* API method variants unless they explicitly override defaults to allow such.

This is because mandatory checks were added to the JWT RFC significantly after the JWT RFCs were submitted for review. They were ratified late in the finalization process to add extra criteria for security checks.

JJWT was written before this final ratification, so that's why there are multiple parse* methods (to reflect the possibilities in the RFC before finalization). Now that these things are no longer possible, the changes in the jwe branch ensure the same.

The jwe branch added the following to the JwtParserBuilder interface:

https://github.com/jwtk/jjwt/blob/3ba22feb0ee1655ffe0f08aac88c996f15ba11c1/api/src/main/java/io/jsonwebtoken/JwtParserBuilder.java#L44-L59

And this is checked during parsing - in any type of JWS or JWE, regardless of subtype or data - as follows (line 403 specifically):

https://github.com/jwtk/jjwt/blob/3ba22feb0ee1655ffe0f08aac88c996f15ba11c1/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParser.java#L387-L410

And as a result already covers the functionality in this PR (I'm pretty sure at least - please confirm). That being the case, I'd like to close this PR if possible to avoid further confusion. Let me know if there's anything I'm missing or that's not already solved in the jwe branch.

[Marcono1234]

Thanks for the insight, that change looks reasonable to me and would probably solve the issue with parse. However, it might still be worth to rename parseClaimsJwt and parsePlaintextJwt by deprecating them and adding new methods with more expressive names. If I see it correctly the jwe branch is not changing their behavior, and it is not unlikely that someone confuses the JWS methods with them. (But on the other hand they should notice during testing that their code does not accept signed JWTs at all when using the unsigned JWT methods by accident.)

What do you think?

[lhazlewood]

Based on the jwe branch change, method renames would just be for convenience since the standard parse logic will catch any alg header issue.

But I'm not sure that changing the names will matter at all - as you say, when a developer tries to use one when it should be the other, their tests (or app) will fail, at which point they'll probably read the JavaDoc or main documentation and then realize why they chose the wrong method.

There are essentially only 3 types of JWTs¹:

• Signed JWTs (called a JWS by the RFCs)

• Encrypted JWTs (called a JWE by the RFCs)

• Unsecured JWTs (called an Unsecured JWT by the RFCs).

  ¹ Technically there is also a concept of a Nested JWT, but it's really just any of the above three as a payload inside either a JWS or JWE. I'm not sure if this 'counts' as a separate type.

This explains JJWT's *parse methods: if you don't choose a *Jwe method or a *Jws method - i.e. a *Jwt method - you are by definition choosing the only thing that can possibly remain, an Unsecured JWT.

Since this is apparently not intuitive for people, the *Jwt methods could be renamed, but it's hard to know what they should be. Consistency across all methods is ideal, but I'm not sure that's possible, because they are ideally consistent already. There are three approaches for this IMO:

Approach A

Don't change anything. The parse method and alg header thing aren't issues. People that accidentally choose the wrong method will find out during testing or production with no negative security side-effects.

Approach B

Rename just the parse*Jwt methods to be:

parseUnsecuredClaimsJwt and parseUnsecuredPlaintextJwt

This leaves the parseClaimJws, parsePlaintextJws, parseClaimsJwe and parsePlaintextJwe methods untouched.

Approach C

Rename all methods to:

• parseUnsecuredClaimsJwt
• parseUnsecuredBinaryJwt
• parseSignedClaimsJwt
• parseSignedBinaryJwt
• parseEncryptedClaimsJwt
• parseEncryptedBinaryJwt

This ensures a more homogenous naming style, but has the downside of changing method names on methods that are not being misused.

But this poses a question for Nested JWTs: even though they're not a new type of JWT, does it mean that yet more methods would need to be added?

• parseSignedNestedJwt
• parseEncryptedNestedJwt

and then what about the JWT that is nested? Does that mean:

• parseSignedNestedClaimsJws
• parseSignedNestedBinaryJws
• parseSignedNestedClaimsJwe
• parseSignedNestedBinaryJwe
• parseSignedUnsecuredClaimsJwt
• parseSignedUnsecuredBinaryJwt
• parseEncryptedNestedClaimsJwe
• parseEncryptedNestedBinaryJwe
• ... etc ...

?

Probably not that last chunk - I'm assuming most people would agree that type of method explosion is undesirable.

---

My current thoughts are that, since it's no longer possible to accidentally use the wrong method, I think it's probably better to stick with Approach A. I dislike backwards-incompatible or name changes when they can be avoided without negative side-effects. But I definitely am open to feedback from others and go with the most desired approach.

Any opinions on which approach we should take long term?

@bdemers or @dogeared (or anyone else) - want to chime in?

[lhazlewood]

Closing just as a matter of housekeeping since parsing unprotected tokens is not possible by default. I created #834 to track associated work for name changes. Thank you @Marcono1234 !