openid / AppAuth-JS

JavaScript client SDK for communicating with OAuth 2.0 and OpenID Connect providers.
Apache License 2.0
977 stars 162 forks source link

token_response.isValid returns true for expired tokens because of the buffer parameter #123

Closed AndrWeisR closed 5 years ago

AndrWeisR commented 5 years ago

Expected Behavior

If you have a token which expires in 1 hour, and you specify a buffer for the validity check in token_response.isValid of 10 minutes, I would expect the isValid() function to say the token is valid up to 50 minutes after it was issued (i.e. 60 - 10), to err on the side of caution and avoid the situation where you think the token is valid but the issuer declares it is invalid, because of a timing mismatch. This means that after only 50 minutes, you'd consider the token to be invalid and get a new token.

Actual Behavior

Because token_response.isValid adds the buffer to the token expiry time, the isValid() function will return that the token is valid for up to 70 minutes after it was issued, when in fact it has expired after 60 minutes.

token_response.isValid should subtract the buffer from the token expiry time to get the desired result.

[REQUIRED] Environment

tikurahul commented 5 years ago

Are you sure ? buffer is being added to the issue time. expiresIn is relative and is an offset from the time of issue.

AndrWeisR commented 5 years ago

return now < this.issuedAt + this.expiresIn + buffer;

Let's say: issuedAt = 14:00 (or equivalent) expiresIn = 60 mins (so the token will expire at 15:00) buffer = 10 mins

The code says the token is valid if now < 14:00 + 60 mins + 10 mins, or 15:10. But the token actually expires at 15:00. So the code returns the wrong result between 15:00 and 15:10, saying the token is valid when it has in fact expired.

The code should subtract the buffer, so that it conservatively says the token is valid until 14:50, giving a 10 minute safety buffer.

tikurahul commented 5 years ago

You are right. The default values for buffer need to be negated. 🤦‍♂

The reason why I never ran into this was because I did not use the defaults. Thanks for the bug report.