Spring Security Potential NullPointerExceptions when AccessToken expiresAt is null

Summary

Per RFC 6749 section 5.1 the expires_in in the response is recommended, but not required. Thus, OAuth2AccessToken.getExpiresAt() can be null. However, all the of built in providers token expiration checks assume it is non-null.

Actual Behavior

See e.g. PasswordOAuth2AuthorizedClientProvider.hasTokenExpired where the logic is:

return this.clock.instant().isAfter(token.getExpiresAt().minus(this.clockSkew));

If it's null, it will throw an NPE. This logic is replicated across all the client providers (including the reactive ones).

Expected Behavior

hasTokenExpired should return false when getExpiresAt() == null and the token should be attempted to be used (of course it may have actually expired, and a new token will be needed, but at this point it's not possible to know this).

Comment From: sjohnr

Hi @motinis, thanks for reaching out!

Take a look at DefaultMapOAuth2AccessTokenResponseConverter and OAuth2AccessTokenResponse.

https://github.com/spring-projects/spring-security/blob/d124fa2858b6261f092917f4fec998053cd4d692/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/DefaultMapOAuth2AccessTokenResponseConverter.java#L76-L78

While it could be advantageous to check for null in the hasTokenExpired() method, the framework takes care to ensure it is never null at that point. The reactive implementation uses the Nimbus API which also ensures that the expires_in parameter has a default value.

Can you provide a sample that reproduces a NullPointerException?

Comment From: motinis

@sjohnr I've gone through it now looking closer - and you're right. I see that the logic instead will result in an expires time of issuedAt plus one second, where issuedAt will be calculated as Instant.now() the first time it's called. Together with clock skew this is basically turning the access token into a "one time use", and each time a new access token will be requested. So then I guess the issue is more around annotations / contracts. Since replicating the current code will generate a warning about the possible NPE.

Comment From: sjohnr

Thanks @motinis. Since the code referred to here is internal to the framework, I think we can leave it as is. The general contract of OAuth2AccessToken seems intentional in that it allows null.

https://github.com/spring-projects/spring-security/blob/f4049c18b1715783f8f8c7be509f44d80f8a3e05/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/AbstractOAuth2Token.java#L60

Any non-framework usage of this class is free to use the contract as-is but this doesn't seem to require a framework change. I'm going to close this issue with that in mind, but let me know if you feel I've missed anything.