spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.79k stars 5.9k forks source link

Implementations of OpaqueTokenIntrospector fail to URL encode client secret #15988

Open joelossher opened 4 hours ago

joelossher commented 4 hours ago

Describe the bug Both the SpringOpaqueTokenIntrospector and NimbusOpaqueTokenIntrospector use the clientId and clientSecret to authenticate the calls to the authorization server.

This is done via basic authentication added using a BasicAuthenticationInterceptor. This does not perform any URL encoding.

This issue was addressed in https://github.com/spring-projects/spring-security/issues/9610 for the token granting client, but persists for the introspection client.

The workaround at the moment is to manually encode the secret when instantiating the introspector.

To Reproduce

  1. Set up a Spring Authorization Server with a client with a secret such as badSecret%
  2. Configure a SpringOpaqueTokenIntrospector or NimbusOpaqueTokenIntrospector to use that client
  3. Attempt to use the introspector with the Spring Authorization Server.
  4. See the server respond with a 400 invalid_request error and see the following cause in the logs:
    Caused by: java.lang.IllegalArgumentException: URLDecoder: Incomplete trailing escape (%) pattern
        at java.base/java.net.URLDecoder.decode(URLDecoder.java:230) ~[?:?]
        at java.base/java.net.URLDecoder.decode(URLDecoder.java:147) ~[?:?]
        at org.springframework.security.oauth2.server.authorization.web.authentication.ClientSecretBasicAuthenticationConverter.convert(ClientSecretBasicAuthenticationConverter.java:85) ~[spring-security-oauth2-authorization-server-1.3.2.jar!/:1.3.2]
        ... 103 more

Expected behavior The token introspector should URL encode the secret.

jzheaux commented 3 hours ago

Thanks for this report, @joelossher. I agree that this should be taken care of.

My primary concern is that there are applications like yours that are self-encoding already (like you are). To start encoding by default at this point would break those applications.

Because there is a constructor that accepts a RestOperations instance, there is already a workaround, should the fix cause any issues; we just don't want to break folks in a point release.

For that reason, I'll schedule this fix for 6.5.

jzheaux commented 3 hours ago

Are you able to provide a PR to add the encoding and a test?