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% 1. Configure a SpringOpaqueTokenIntrospector or NimbusOpaqueTokenIntrospector to use that client 2. Attempt to use the introspector with the Spring Authorization Server. 3. 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.

Comment From: jzheaux

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.

Comment From: jzheaux

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

Comment From: ngocnhan-tran1996

@jzheaux

SpringOpaqueTokenIntrospector use BasicAuthenticationInterceptor for encoding clientId and clientSecret. So clientSecret will be encoded before calling BasicAuthenticationInterceptor. Please correct me if I am wrong

Comment From: jzheaux

@ngocnhan-tran1996, my understanding is that the OAuth 2.0 spec indicates that the client id and secret should be URL encoded before performing the Basic Auth encoding of username and password together (pseudocode follows):

header = base64(urlencode(username) + ':' + urlencode(password))

So this ticket would make it so that the values are URL encoded before formulating the Authorization: Basic header.

Does that clarify, or am I misunderstanding your question?

Comment From: ngocnhan-tran1996

@jzheaux

Let me make more clearly Currently, SpringOpaqueTokenIntrospector use BasicAuthenticationInterceptor for encoding clientId and clientSecret

https://github.com/spring-projects/spring-security/blob/9d2ca3da6a285f31ebd2da5f019127e1527e5042/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/SpringOpaqueTokenIntrospector.java#L88

And deep into this, it use HttpHeaders#encodeBasicAuth (belongs to Spring Framework) like your pseudocode 

String credentialsString = username + ":" + password;
byte[] encodedBytes = Base64.getEncoder().encode(credentialsString.getBytes(charset));
return new String(encodedBytes, charset);

So if we want to encode password in side SpringOpaqueTokenIntrospector, we will do something like 

restTemplate.getInterceptors().add(new BasicAuthenticationInterceptor(urlencode(clientId), urlencode(clientSecret)));

This part confuses me, we need to encode from Spring Framework side or Spring Security side if somewhere in Spring Security also use HttpHeaders#encodeBasicAuth

Comment From: jzheaux

The encoding that Spring Framework provides is Base64 encoding. The OAuth spec indicates that the values should be URL encoded and then also thereafter Base64 encoded.

If you look carefully at the Framework code, you'll see that it is doing:

header = base64(username + ':' + password)

which is different from 

header = base64(urlencode(username) + ':' + urlencode(password))

Comment From: ngocnhan-tran1996

Thank for your explantion, can I work on this?

Comment From: jzheaux

Sure! Note that it is targeted for 6.5, so the PR wouldn't merge for a couple of months.