DomainException is missing in the "throws" list

[dakujem]

Hello guys, I noticed a problem with the doc comment of JWT::decode method, in the list of what the method throws, DomainException is missing. This happens for malformed JWT (like the one below).
Also, InvalidArgumentException is thrown too, even though in a different context.

I'd suggest adding these lines:

     * @throws DomainException              Provided JWT is malformed
     * @throws InvalidArgumentException     Provided key is empty

The doc comment makes devs (me 🤷‍♂️) assume that it is enough to catch the UnexpectedValueException, which is not true.
Everything works until someone sends in a malformed token.

How to replicate:

JWT::decode(
    'foo.bar.qux',
    'anything',
    ['doesntmatter']
);

Would you be interested in a PR instead of an issue?

[dakujem]

On the second thought, I believe that this is a token-related issue and should be caught and rethrown as an instance of UnexpectedValueException.

Rationale:

JWT::decode('foo.bar.qux', 'anything', ['doesntmatter']);  // throws DomainException
JWT::decode('foo.bar', 'anything', ['doesntmatter']);      // throws UnexpectedValueException

I believe both tokens are malformed and should result in the same kind of exception for consistency. Any thoughts?

[markrandall]

I didn't notice this issue before posting my own, but I have now linked it to this.

I agree with the author's comments, the DomainException should be at the very least documented, but ideally should be caught and rethrown as UnexpectedValueException as to give a single hierarchy that can be caught.

Better yet, it should throw a class extending UnexpectedValueException e.g. MalformedException that would cover bad Base64 encoding, as well as bad JSON encoding.

IMHO the same should apply to what is currently InvalidArgumentException.

[dakujem]

The InvalidArgumentException is thrown for cases where the key is empty - hence the 'Key may not be empty' message.

Usually, this is a developer-caused issue. Typically, one stores the key on the server as a secret and decodes user input.
I think that any problem with the first argument ($jwt) should be thrown by the same exception class, but different to the problems caused by the second argument, as they originate from different context.

The following code will report 4xx and 5xx errors based on the exception being thrown:

        try {
            return JWT::decode(
                $token,
                $this->secret,
                $this->supportedAlgos
            );
        } catch (UnexpectedValueException $throwable) {
            // Report this as 4xx client error
        } catch (DomainException $throwable) {
            // Report this as 4xx client error
        } catch(InvalidArgumentException $throwable){
            // Report this as 5xx server error
        }

Catching the DomainException should be redundant, as all the other exceptions thrown by JWT::decode are subclasses of UnexpectedValueException, as all are related to the input value.

Would any of the maintainers be interested in a PR where the DomainException exception is caught and rethrown as an UnexpectedValueException instead?

[markrandall]

I accept your point about having a different hierarchy for programmer errors.

It seems to be complicated by the use of DomainException within verify as well, that throws in the event of errors with openssl, or a lack of extension support such as libsodium.

[dakujem]

Yes, I noticed. That's why I only made a PR to fix the documentation/doc-comments.

The greatest problem of the solution I suggest here is that it would cause a breaking change.