angular-oauth2-oidc: Clock skew is in wrong direction

Describe the bug Unless the intent is to consider an access/id token valid for 10 minutes (by default) after it has expired, the clock skew calculation is in the wrong direction.

This, somewhat recent, commit seems to be the culprit: https://github.com/manfredsteyer/angular-oauth2-oidc/commit/68238fb6ea4a2f88ada97b03b13663d1454b001c#diff-685e26c24f3c008856834f8b4a350d5472b6aea01a623c31a8513f6cc599c57fR2391 The pluses should not have been changed to minuses.

It causes the current time to be considered 10 minutes (by default) in the past, causing the token to be considered valid for longer. I.e.: with the current code, the current time needs to be 10 minutes after the token expiration time for the comparison to turn true and the return value (indicating validity of token) to become false. Perhaps it could be worth refactoring this code for readability. At least, I had some difficulty understanding it.

Expected behavior Tokens should be considered expired once they expire.

Versions Library version 12.1.0

Additional context I was experiencing problems with access tokens considered valid by the hasValidAccessToken() method, where I knew they were expired.

I can’t even use negative values to fix it, because of these lines: https://github.com/manfredsteyer/angular-oauth2-oidc/blob/12.1/projects/lib/src/oauth-service.ts#L2233 that will throw ‘Token has expired’ Errors if I use a reasonable, but negative, value for clockSkewInSec.

I also consider it a bug that setting the clock skew to 0 causes the default to be used, because the value is checked for truthiness, instead of compared against undefined and/or null. This is also a new change, here: https://github.com/manfredsteyer/angular-oauth2-oidc/commit/68238fb6ea4a2f88ada97b03b13663d1454b001c#diff-685e26c24f3c008856834f8b4a350d5472b6aea01a623c31a8513f6cc599c57fR2121

About this issue

  • Original URL
  • State: closed
  • Created 3 years ago
  • Reactions: 10
  • Comments: 27 (5 by maintainers)

Most upvoted comments

why has this bug not been fixed so far? Because nobody created a PR? Or does somebody have reservations about fixing this?!

Yeah, I’m surprised this hasn’t been fixed yet.

I ended up writing my own short method for checking whether access token is expired:

private get isAccessTokenExpired() {
  // custom check of token expiration to avoid bug in library: https://github.com/manfredsteyer/angular-oauth2-oidc/issues/1135
  const expiresAt = this.oAuthService.getAccessTokenExpiration() as number | null; // can return null https://github.com/manfredsteyer/angular-oauth2-oidc/blob/12.1/projects/lib/src/oauth-service.ts#L2354
  return Date.now() > (expiresAt ?? 0) - (this.oAuthService.clockSkewInSec ?? 0);
}

I’ve wasted sooooo much time trying to figure out why sometimes I was logged out with no reason. This is a MAJOR bug, please fix it as soon as possible.

Im new to OAuth in my applications, but isnt this a serious issue? How can I determine if a user is authenticated if not by hasValidAccessToken()? If I can’t trust hasValidAccessToken() I have to use refreshToken() every time I have to check auth status. Am I missing something?

Just added the new decreaseExpirationBySec.

Feel free to reopen this issue if this doesn’t solve the situation for you.

@jeroenheijmans ok, it was a bit confusing, sorry, and thanks for clarifying.

@rasifix If the client’s time is not in sync with the current time he will always have an invalid token. For me, an idea of skew is to forgive small differences, to make it possible to work with such users (wrong time or slow internet).

that is not the issue. if the client time is perfectly in sync, the token will be used even after effective expiry for another 10 minutes. Setting the clock skew to -10 minutes fixes the problem. The client will think the token is expired, even if it has not expired, but that is perfectly fine as we can simply login again and get a new token.

@rasifix If the client’s time is not in sync with the current time he will always have an invalid token. For me, an idea of skew is to forgive small differences, to make it possible to work with such users (wrong time or slow internet).

Perhaps there is a misunderstanding, but IMHO the code does what it is supposed to do: Making the token be valid a bit longer b/c clocks might be not in sync.

Maybe I’m just missing the use case of that.

I figured the intend would be to consider it invalid a bit earlier, because clocks might not be in sync. I can see use cases for this:

  1. I want to check validity of my access token before use, perhaps because I want to refresh the token beforehand when it has expired. If the clocks are out of sync, I want to consider it invalid earlier, because I would rather refresh earlier than needed, than call backend with an expired token (for whatever reason).
  2. I have already called my backend and have not been given access. Now I want to check whether the reason could be that my token has expired. If clocks are out of sync, the backend might consider it expired before I do, so I want to consider it invalid earlier.
  3. Network transmission delay is high, and even if the token is valid when I send my request from frontend, by the time the backend receives my token, it can be expired. I might want the clockSkew parameter to also help me in this case, by considering a token invalid a bit earlier.

And then there’s also the issue that setting clockSkew to 0 actually makes the default 10 minutes be used instead. (this might have been fixed, I haven’t checked) And maybe it’s just me, but 10 minutes seems like an extremely long default skew. I would maybe have expected 10 seconds.

As some time has went by and we went into deeper researches we identified the client time as the source of error for us. If the users local time is completely out of sync validation of the token fails (of course). The only bullet proof solution would be getting the current time from the server that issued the token to use this for validation. But we decided to show a error message instead.

Sorry If I messed things up here. We just have this problem now and got the token expired error and I thought our “research” could help in the overall context. And this is the only thread I found which is active and seems to have a similar cause for the error to occur. Thanks for your answers @jeroenheijmans we will try consider your comment.

I have run into a similar problem with this variable being used wrongly. I also was relying on it to check if the computer time was wrong, and show a page to the user if they need to fix their computer time. However, this is being calculated wrongly here https://github.com/manfredsteyer/angular-oauth2-oidc/blob/d95d7da788e2c1390346c66de62dc31f10d2b852/projects/lib/src/oauth-service.ts#L2266-L2267 It first checks for the issued at time - clockskew, then for the expired at time + the clock skew, and throws a ‘Token expired exception’. There is more than one problem between those lines. It tries to do two different checks at once, and both of them are incomplete. Yes, if the expired at time + the clock skew is greater than the current time, the token is really expired. But what is really needed is to check if the issued at time + the clock skew is greater than the current time, so that we can know if the computer time is off. If the computer time is off, the token expiration is still wrong, but we will just notice it later.

Also, this check explained above only checks the ID token, but the access token never get checked like that, which is also weird.

I agree with all the issues raised above. It is difficult to understand why the default value is 10 minutes since it is not mentioned in the documentation. The tsdoc for the AuthConfig#clockSkewInSec parameter does not mention the default value and it only states it used for id tokens (“validating id_token’s iat and exp values”). Moreover it is difficult to locate this default value since it is buried in getClockSkewInMsec private method

I also consider it a bug that setting the clock skew to 0 causes the default to be used, because the value is checked for truthiness, instead of compared against undefined and/or null.

I also think this is a bug since I do not see a reason why 0 would be an invalid value for the clock skew.