Skip to content

Payload is inaccessible in Expired / BeforeValid scenarios #291

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

Closed
bendoh opened this issue Apr 6, 2020 · 8 comments
Closed

Payload is inaccessible in Expired / BeforeValid scenarios #291

bendoh opened this issue Apr 6, 2020 · 8 comments
Assignees

Comments

@bendoh
Copy link

bendoh commented Apr 6, 2020

Though the JWT is cryptographically valid in scenarios where it is being accessed chronologically out of bounds, callers have no easy way of seeing the payload that would be considered valid otherwise.

It would be useful to attach the "would-be" payload to thrown exceptions BeforeValidException and ExpiredException, so then callers would still be able to check the claims therein and act on them knowing that they are still invalid to some degree.

@Krisell
Copy link

Krisell commented Apr 6, 2020

What would the use case be for that?

The specification (https://tools.ietf.org/html/rfc7519#section-4.1.4) states that The "exp" (expiration time) claim identifies the expiration time on or after which the JWT MUST NOT be accepted for processing. One could argue that decoding and returning the data is "processing", thus not complying with the standard.

@bendoh
Copy link
Author

bendoh commented Apr 7, 2020

So my use case is, "this user gave me a token, I know it's a valid token, but it's either expired or before its time: what are its claims anyway?"

I'd say "MUST NOT be accepted for processing" is compatible with this idea: the token is not accepted (the exception is still thrown,) but I do know that it was/will be valid: what claims did it make?

One pretty obvious use case: The user's token expired, and I want to know which user that is.

Attaching the payload to the thrown exception would make it easier to find out, rather than having to decode the JWT parts again in the application layer.

@DanteMarshal
Copy link

DanteMarshal commented Apr 20, 2020

I have created a Pull request for this

@OssiPesonen
Copy link

OssiPesonen commented May 4, 2020

I have a use-case for this too.

We receive the token as a means of identifying a user when he closes a chat on one of our applications. The token gets stored by the chat when he opens the chat and by the time he closes it, it might have expired.

We can't read the users verified information due to the expiration, which should be ignored at that point as we do not return any data from our API, we just store information. Right now our API throws an exception when attempting to decode the token and the process has to stop there, preventing us from storing chat logs that we need for legal reasons.

I would maybe not attach the payload to the exception, but rather have an override to tell the decode() function that I don't care if the token is expired.

public static function decode($jwt, $key, array $allowed_algs = array())

public static function decode($jwt, $key, array $allowed_algs = array(), bool $ignoreExpiration = 

// Check if this token has expired.
if (!$ignoreException && isset($payload->exp) && ($timestamp - static::$leeway) >= $payload->exp) {
    throw new ExpiredException('Expired token');
}

This would extend the current code with minor addition. But that's just a 5 minute idea.

@bendoh
Copy link
Author

bendoh commented May 19, 2020

@OssiPesonen I'd be very wary of adding extra arguments, especially boolean arguments, since they really cause the method to mean two different things, and so it should be written as two different methods instead, e.g., decode and decodeIgnoringExceptions, but just attaching the payload to the exception avoids this weirdness and IMO is more correct since indeed it is an error condition.

@OssiPesonen
Copy link

I would argue that the name decode() means that you should be returned with the payload of a decoded token. The function should not be responsible in reading the payload and throwing exceptions if it is expired, but leave that option for the user to do with it as he pleases, or have some mechanism to enable/disable it on a use-case basis.

In any case as this does not seem to be progressing (1.5 months) we might fork the library and continue development in-house.

@DanteMarshal
Copy link

In any case as this does not seem to be progressing (1.5 months) we might fork the library and continue development in-house.

I'm using My own fork in which I fixed this issue.
You can fork that and continue from there

@ajupazhamayil
Copy link
Collaborator

I see the PR #521 has merged to resolve this issue. Hence closing it. Thank you for raising this issue!

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

No branches or pull requests

6 participants