jwtk / jjwt

Java JWT: JSON Web Token for Java and Android
Apache License 2.0
10.21k stars 1.32k forks source link

isSigned returns true if normal JSON provided or singed with different key #582

Open SailReal opened 4 years ago

SailReal commented 4 years ago

I've two problems with the boolean isSigned(String jwt) method:

The following function call returns true if I provide a normal JSON (NOT a signed JWT):

Jwts //
    .parserBuilder() //
    .setSigningKey(getPublicKey()) //
    .build() //
    .isSigned(json);

If I change the method calls to the following:

Jwts //
    .parserBuilder() //
    .setSigningKey(getPublicKey()) //
    .build() //
    .parseClaimsJws(json);

a io.jsonwebtoken.MalformedJwtException: JWT strings must contain exactly 2 period characters. Found: 14 is thrown (which is the expected behavior).

As json a valid JSON is provided (NOT a JWT, maybe it can be any string?), e.g.

{
  "version": "foo",
  "url": "bar",
  "release_notes": "baz"
}

If I provide a valid JWT, signed with a different private key, isSigned also returns true.

From the doc:

* Returns {@code true} if the specified JWT compact string represents a signed JWT (aka a 'JWS'), {@code false}
* otherwise.
* <p>
* <p>Note that if you are reasonably sure that the token is signed, it is more efficient to attempt to
* parse the token (and catching exceptions if necessary) instead of calling this method first before parsing.</p>

Do I understand this method in a wrong way? I just want to check if a string is a JWT signed with the corresponding key. In my opinion isSigned should return false in both cases.

At a different code location I use parseClaimsJws, that works great 😍

As version I use the latest 0.11.1

lhazlewood commented 4 years ago

Thanks for the issue!

That method is intended to be used for checking strings reasonably expected to be compact JWT strings - not generic JSON.

Based on the confusion, it is probable that others experience it as well, so it's likely we'll deprecate this method in favor of parsing directly - or perhaps at least change the implementation to delegate to parsing, catch a MalformedJwtException and then return false as a result, which isn't exactly efficient, so I'm not so sure it's a good idea to propagate its usage.

If we do keep it, some thought needs to be put a changed implementation however to ensure that the implementation is feasible, especially given that encrypted JWTs (JWEs) are also usually signed in addition to being encrypted. (i.e. isSigned should return true for these types of JWEs as well).

I think it makes sense to keep this ticket open to represent the work to make that change. Thanks for reporting it!