Authorization Response should also match on query parameters #7329

ttddyy · 2019-08-29T18:13:38Z

This is about OAuth2 Authorization Code Grant flow.

When a service is registered with redirect_uri that has query parameter (e.g: http://some/auth?param=foo), current OAuth2AuthorizationCodeGrantFilter doesn't work well.

This is because OAuth2AuthorizationCodeGrantFilter#shouldProcessAuthorizationResponse compares registered redirect_uri (AuthRequest: http://some/auth?param=foo) against the redirected request URI without query parameters. (AuthResponse: http://some/auth)

This patch updates the redirect_uri comparison not only checking base uri but also checks query params in the registered redirect_uri whether they are all included in the authorization response's query parameters.

Also, in the patch, it now retrieves final redirect_uri from authorizationRequest since authorizationResponse.getRedirectUri() does not have query parameters.
This might be debatable and also may change based on #7324.

jgrandja

Thanks for the PR @ttddyy. Please see my comments.

jgrandja · 2019-09-30T20:28:27Z

...in/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationExchangeUtils.java

+ * @see OAuth2AuthorizationRequest
+ * @see OAuth2AuthorizationResponse
+ */
+public final class OAuth2AuthorizationExchangeUtils {


Can you please remove this and keep the changes contained in OAuth2AuthorizationResponseUtils and OAuth2AuthorizationCodeGrantFilter, as well as the Reactive counterparts. I would like to get this into a patch release.

NOTE: In processAuthorizationResponse(), I believe you can build the redirect-uri for OAuth2AuthorizationResponse as follows:

MultiValueMap<String, String> params = OAuth2AuthorizationResponseUtils.toMultiMap(request.getParameterMap()); String redirectUri = UriComponentsBuilder.fromHttpUrl(UrlUtils.buildFullRequestUrl(request)) .replaceQueryParam(OAuth2ParameterNames.CODE, (Object) null) .replaceQueryParam(OAuth2ParameterNames.STATE, (Object) null) .build() .toUriString(); OAuth2AuthorizationResponse authorizationResponse = OAuth2AuthorizationResponseUtils.convert(params, redirectUri);

The OAuth2AuthorizationResponseUtils is package scoped to org.springframework.security.oauth2.client.web, so cannot reference from provider/validator(org.springframework.security.oauth2.client.authentication).

So, ok to make it public??

OAuth2AuthorizationExchangeValidator does not need to change. What I was suggesting previously is to add the following to OAuth2AuthorizationResponseUtils:

static OAuth2AuthorizationResponse convert(HttpServletRequest request) { MultiValueMap<String, String> params = toMultiMap(request.getParameterMap()); MultiValueMap<String, String> redirectParams = new LinkedMultiValueMap<>(params); redirectParams.remove(OAuth2ParameterNames.CODE); redirectParams.remove(OAuth2ParameterNames.STATE); String redirectUri = UriComponentsBuilder.fromHttpUrl(UrlUtils.buildFullRequestUrl(request)) .replaceQueryParams(redirectParams) .build() .toUriString(); return convert(params, redirectUri); }

This utility method can than be used in shouldProcessAuthorizationResponse and processAuthorizationResponse. This will contain the changes only in OAuth2AuthorizationCodeGrantFilter.

ok, but I think there is a problem when multiple parameters are presented.

For example, following should match:

http://localhost/auth?foo=FOO&bar=BAR

http://localhost/auth?bar=BAR&foo=FOO

If it does simple string match of redirect_uri in OAuth2AuthorizationExchangeValidator, they are considered as different.

I'm not sure if this is an issue. I'm expecting the provider to perform the redirect using the registered redirect-uri as-is so it should be the same order as specified in ClientRegistration.redirect-uri.

I just tested with Okta and it's performing an exact match.

The registered redirect-uri is http://locahost:8080/authorized/okta2?param1=value1&param2=value2.

If I trigger the Authorization Request using the redirect-uri http://locahost:8080/authorized/okta2?param2=value2&param1=value1 it fails.

jgrandja · 2019-09-30T20:36:35Z

...main/java/org/springframework/security/oauth2/core/endpoint/OAuth2AuthorizationResponse.java

@@ -122,6 +133,7 @@ public static Builder error(String errorCode) {
 		private String errorCode;
 		private String errorDescription;
 		private String errorUri;
+		private MultiValueMap<String, String> parameters = new LinkedMultiValueMap<>();


This is not needed. Please see previous comment on how to build the OAuth2AuthorizationResponse.redirectUri with query parameters.

ttddyy · 2019-10-01T06:18:24Z

I have updated the PR

just kept the OAuth2AuthorizationExchangeUtils as is due to utility class scope
removed parameters from OAuth2AuthorizationResponse
updated the way to get redirectUri from http request in OAuth2AuthorizationCodeGrantFilter
put back to use authorizationResponse to get redirectUrl

ttddyy · 2019-10-02T19:47:00Z

Updated the PR.

Now, simply it checks the redirected url by removing code and state parameters.

Limitations would be:

the redirected url parameters need to be exactly same order in OAuth2AuthorizationRequest#redirectUri
Also, after successful authentication, the authorization server cannot add any additional parameters to the redirecting url (in order for exact match)

jgrandja

@ttddyy Thank you for the updates. Please see my comment for additional changes.

jgrandja · 2019-10-25T13:56:51Z

.../java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationCodeGrantFilter.java

@@ -166,7 +171,8 @@ private void processAuthorizationResponse(HttpServletRequest request, HttpServle

 		MultiValueMap<String, String> params = OAuth2AuthorizationResponseUtils.toMultiMap(request.getParameterMap());
 		String redirectUri = UriComponentsBuilder.fromHttpUrl(UrlUtils.buildFullRequestUrl(request))
-				.replaceQuery(null)
+				.replaceQueryParam(OAuth2ParameterNames.CODE)


@ttddyy After reviewing the spec:

The redirection endpoint URI MUST be an absolute URI as defined by
[RFC3986] Section 4.3. The endpoint URI MAY include an
"application/x-www-form-urlencoded" formatted (per Appendix B) query
component ([RFC3986] Section 3.4), which MUST be retained when adding
additional query parameters. The endpoint URI MUST NOT include a
fragment component.

NOTE the bold highlight. As per spec, the Authorization Server will ADD additional query parameters to the redirect-uri. This means that the query parameters in the redirect_uri (if any) should be in the correct order. Also, in addition to code and state, the Authorization Server may add additional query parameters. I've tested this with google and additional parameters are added on top of code and state. Can you please add a test for this scenario and adjust the code for this.

ttddyy · 2019-10-27T05:38:47Z

@jgrandja
Updated the logic to handle additional params from auth server.
For test, added a case to simulate additional parameter added by authorization server.
Also, rebased to the latest master.

jgrandja · 2019-11-01T20:07:04Z

.../java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationCodeGrantFilter.java

@@ -166,7 +197,8 @@ private void processAuthorizationResponse(HttpServletRequest request, HttpServle

 		MultiValueMap<String, String> params = OAuth2AuthorizationResponseUtils.toMultiMap(request.getParameterMap());
 		String redirectUri = UriComponentsBuilder.fromHttpUrl(UrlUtils.buildFullRequestUrl(request))
-				.replaceQuery(null)
+				.replaceQueryParam(OAuth2ParameterNames.CODE)
+				.replaceQueryParam(OAuth2ParameterNames.STATE)
 				.build()
 				.toUriString();
 		OAuth2AuthorizationResponse authorizationResponse = OAuth2AuthorizationResponseUtils.convert(params, redirectUri);


If the authorization server adds additional parameters to the authorization response, than the OAuth2AuthorizationResponse.redirectUri will contain the additional parameters and result in a failed validation check in OAuth2AuthorizationCodeAuthenticationProvider and the call to OAuth2AuthorizationExchangeValidator.validate() when it compares the redirectUri between the authorization request and response. This revealed that we need an integration test for this scenario. Can you please add an integration test and fix so it passes. Take a look at OAuth2LoginTests or OAuth2LoginConfigurerTests for an example for the integration test.

ah, true.
So, now the question is how you want to share the validation logic.
My original PR introduced OAuth2AuthorizationExchangeUtils class to share the validation logic between OAuth2AuthorizationCodeGrantFilter and OAuth2AuthorizationExchangeValidator. (also OidcAuthorizationCodeAuthenticationProvider). (link bit outdated)

This is because OAuth2AuthorizationResponseUtils seems good place to put the validation logic but it is package scoped class.
Since ~CodeGrantFilter and ~ExchangeValidator are in different package, we need a way to share this validation logic. So, if you don't oppose, I'm going to put back OAuth2AuthorizationExchangeUtils to share the redirect url validation logic.

So, now the question is how you want to share the validation logic.
My original PR introduced OAuth2AuthorizationExchangeUtils class to share the validation logic between OAuth2AuthorizationCodeGrantFilter and OAuth2AuthorizationExchangeValidator

We do not want to share the logic between OAuth2AuthorizationCodeGrantFilter and OAuth2AuthorizationExchangeValidator. Reason being is that OAuth2AuthorizationCodeGrantFilter is in the web sub-package and the OAuth2AuthorizationExchangeValidator is in the authentication sub-package, which is NOT a web component.

The validation check in OAuth2AuthorizationExchangeValidator can simply be changed to:

if (!authorizationResponse.getRedirectUri().startsWith(authorizationRequest.getRedirectUri()))

It's safe to use startsWith, since we know that any additional query parameters will be appended to the registered redirect-uri

The redirection endpoint URI MUST be an absolute URI as defined by
[RFC3986] Section 4.3. The endpoint URI MAY include an
"application/x-www-form-urlencoded" formatted (per Appendix B) query
component ([RFC3986] Section 3.4), which MUST be retained when adding
additional query parameters. The endpoint URI MUST NOT include a
fragment component.

jgrandja · 2019-11-11T11:59:28Z

.../java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationCodeGrantFilter.java

 		MultiValueMap<String, String> params = OAuth2AuthorizationResponseUtils.toMultiMap(request.getParameterMap());
-		if (requestUrl.equals(authorizationRequest.getRedirectUri()) &&
+
+		if (isValidRedirectUrl(request, authorizationRequest.getRedirectUri()) &&


@ttddyy This can be simplified as follows:

String requestUrl = UrlUtils.buildFullRequestUrl(request); MultiValueMap<String, String> params = OAuth2AuthorizationResponseUtils.toMultiMap(request.getParameterMap()); if (requestUrl.startsWith(authorizationRequest.getRedirectUri()) && OAuth2AuthorizationResponseUtils.isAuthorizationResponse(params)) { return true; }

@jgrandja
There is a problem with startWith() check which is caught by existing test.

Let's say here is changed to use startWith() as below:

String requestUrl = UriComponentsBuilder.fromHttpUrl(UrlUtils.buildFullRequestUrl(request)) .replaceQueryParam(OAuth2ParameterNames.CODE) .replaceQueryParam(OAuth2ParameterNames.STATE) .build() .toUriString(); MultiValueMap<String, String> params = OAuth2AuthorizationResponseUtils.toMultiMap(request.getParameterMap()); if (requestUrl.startsWith(authorizationRequest.getRedirectUri()) && OAuth2AuthorizationResponseUtils.isAuthorizationResponse(params)) { return true; } return false;

This logic matches when:

requestUrl = http://localhost/callback/client-1-no-match

authorizationRequest.getRedirectUri() = http://localhost/callback/client-1

Thus, simple startWith() check doesn't work.

So, if we don't want to share the code between filter and validator:

put same logic in both place

or, maybe perform through check(isValidRedirectUrl) in filter, but simply only do startWith() check in validator

@ttddyy Yes, you are right, there is a problem with using startsWith(). Instead we should perform exact matching on all parts of the URI, with the exception of the query parameters since there may be additional parameters sent in the authorization response.

In the shouldProcessAuthorizationResponse() method, can you make use of UriComponentsBuilder for both the request (authorization response) and authorizationRequest.getRedirectUri() and perform exact matching on each part, eg. scheme, userInfo, host, port, path. The query parameters (if any) in the authorizationRequest.getRedirectUri() should exist in the same order in request (authorization response).

On a side note, the validation logic for redirect-uri in OAuth2AuthorizationExchangeValidator is redundant. The matching happens in shouldProcessAuthorizationResponse() so we don't need to double validate in OAuth2AuthorizationExchangeValidator. Please remove the existing validation which will simply things.

ttddyy · 2019-11-11T23:02:24Z

Updated the PR adding an integration test, OAuth2AuthorizationCodeGrantRedirectTests.
Currently some of the tests are failing as expected since the validation logic is not yet updated.
Once we solve the validation logic issue, I'll update the PR.

jgrandja

@ttddyy Apologies for the delayed response. Please see my additional comments.

jgrandja · 2019-12-03T15:54:12Z

...rg/springframework/security/config/web/server/OAuth2AuthorizationCodeGrantRedirectTests.java

+ * limitations under the License.
+ */
+
+package org.springframework.security.config.web.server;


Please move to same package as org.springframework.security.config.annotation.web.configurers.oauth2.client.OAuth2ClientConfigurerTests

jgrandja · 2019-12-03T15:58:57Z

...rg/springframework/security/config/web/server/OAuth2AuthorizationCodeGrantRedirectTests.java

+
+	@Configuration
+	@EnableWebSecurity
+	public static class WebSecurityConfiguration extends WebSecurityConfigurerAdapter {


Please move the configuration to bottom of class.

jgrandja · 2019-12-03T16:08:36Z

.../java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationCodeGrantFilter.java

 		MultiValueMap<String, String> params = OAuth2AuthorizationResponseUtils.toMultiMap(request.getParameterMap());
-		if (requestUrl.equals(authorizationRequest.getRedirectUri()) &&
+
+		if (isValidRedirectUrl(request, authorizationRequest.getRedirectUri()) &&


@ttddyy Yes, you are right, there is a problem with using startsWith(). Instead we should perform exact matching on all parts of the URI, with the exception of the query parameters since there may be additional parameters sent in the authorization response.

In the shouldProcessAuthorizationResponse() method, can you make use of UriComponentsBuilder for both the request (authorization response) and authorizationRequest.getRedirectUri() and perform exact matching on each part, eg. scheme, userInfo, host, port, path. The query parameters (if any) in the authorizationRequest.getRedirectUri() should exist in the same order in request (authorization response).

On a side note, the validation logic for redirect-uri in OAuth2AuthorizationExchangeValidator is redundant. The matching happens in shouldProcessAuthorizationResponse() so we don't need to double validate in OAuth2AuthorizationExchangeValidator. Please remove the existing validation which will simply things.

jgrandja · 2019-12-06T17:23:27Z

@ttddyy

the validation logic for redirect-uri in OAuth2AuthorizationExchangeValidator is redundant. The matching happens in shouldProcessAuthorizationResponse() so we don't need to double validate in OAuth2AuthorizationExchangeValidator.

I removed the redundant validation in #7708 to help with this PR.

ttddyy · 2019-12-06T17:26:21Z

I removed the redundant validation in #7708 to help with this PR.

Thanks, will work on this ticket early next week with rebasing to the latest.

Fixes spring-projectsgh-7329

ttddyy · 2020-01-27T18:04:08Z

@jgrandja
Hi, the PR has updated. Can you check.
Thanks,

jgrandja · 2020-02-10T11:05:45Z

@ttddyy Thank you for the updates. After reviewing, I still feel the logic can be simplified and the code can be reduced further. Our ultimate goal with PR's is to introduce the minimal amount of code required to either fix a bug or introduce a new feature. The main reason is because the more code we add the more we need to maintain. It is definitely a challenging task but it is important.

Having said that, I decided to go ahead and apply the fix myself in this commit 3c86239. Thank you for your understanding.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 29, 2019

xak2000 mentioned this pull request Sep 15, 2019

Final redirect in OAuth2AuthorizationCodeGrantFilter needs to be configurable #7324

Closed

rwinch assigned jgrandja Sep 19, 2019

jgrandja requested changes Sep 30, 2019

View reviewed changes

jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 30, 2019

jgrandja added this to the 5.2.x milestone Sep 30, 2019

ttddyy force-pushed the oauth2-code-grant-redirect-with-query-params branch from 457fb2f to 52dcd13 Compare October 1, 2019 06:10

jgrandja changed the title ~~Allow query params in OAuth2 Authorization Request redirect_uri~~ Authorization Response should also match on query parameters Oct 1, 2019

jgrandja added type: bug A general bug and removed type: enhancement A general enhancement labels Oct 1, 2019

ttddyy force-pushed the oauth2-code-grant-redirect-with-query-params branch from 52dcd13 to 8741e19 Compare October 2, 2019 19:44

jgrandja requested changes Oct 25, 2019

View reviewed changes

ttddyy force-pushed the oauth2-code-grant-redirect-with-query-params branch from 8741e19 to 3e75e5c Compare October 27, 2019 05:36

jgrandja reviewed Nov 1, 2019

View reviewed changes

jgrandja modified the milestones: 5.2.x, 5.2.2 Nov 11, 2019

jgrandja reviewed Nov 11, 2019

View reviewed changes

ttddyy force-pushed the oauth2-code-grant-redirect-with-query-params branch from 3e75e5c to a55ff31 Compare November 11, 2019 23:00

jgrandja requested changes Dec 3, 2019

View reviewed changes

Allow query params in OAuth2 Authorization Request redirect_uri

008dbee

Fixes spring-projectsgh-7329

ttddyy force-pushed the oauth2-code-grant-redirect-with-query-params branch from a55ff31 to 008dbee Compare December 9, 2019 06:41

jgrandja modified the milestones: 5.2.2, 5.2.3 Feb 5, 2020

jgrandja closed this Feb 10, 2020

jgrandja removed this from the 5.2.3 milestone Feb 10, 2020

Authorization Response should also match on query parameters #7329

Authorization Response should also match on query parameters #7329

Uh oh!

Conversation

ttddyy commented Aug 29, 2019

Uh oh!

jgrandja left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrandja Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ttddyy Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrandja Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ttddyy commented Oct 1, 2019

Uh oh!

ttddyy commented Oct 2, 2019

Uh oh!

jgrandja left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ttddyy commented Oct 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ttddyy commented Nov 11, 2019

Uh oh!

jgrandja left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrandja commented Dec 6, 2019

Uh oh!

ttddyy commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ttddyy commented Jan 27, 2020

Uh oh!

jgrandja commented Feb 10, 2020

Uh oh!

Uh oh!

jgrandja Oct 1, 2019 •

edited

Loading

ttddyy Oct 1, 2019 •

edited

Loading

jgrandja Oct 1, 2019 •

edited

Loading

ttddyy commented Dec 6, 2019 •

edited

Loading