Skip to content

Authorization Response should also match on query parameters #7329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Authorization Response should also match on query parameters #7329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 156 additions & 0 deletions ...g/annotation/web/configurers/oauth2/client/OAuth2AuthorizationCodeGrantRedirectTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
/*
* Copyright 2002-2019 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.security.config.annotation.web.configurers.oauth2.client;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.redirectedUrl;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

import java.util.Collections;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Configuration;
import org.springframework.security.config.annotation.web.builders.HttpSecurity;
import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter;
import org.springframework.security.config.test.SpringTestRule;
import org.springframework.security.oauth2.client.endpoint.OAuth2AccessTokenResponseClient;
import org.springframework.security.oauth2.client.endpoint.OAuth2AuthorizationCodeGrantRequest;
import org.springframework.security.oauth2.client.registration.ClientRegistration;
import org.springframework.security.oauth2.client.registration.InMemoryClientRegistrationRepository;
import org.springframework.security.oauth2.client.registration.TestClientRegistrations;
import org.springframework.security.oauth2.client.web.AuthorizationRequestRepository;
import org.springframework.security.oauth2.core.OAuth2AccessToken;
import org.springframework.security.oauth2.core.endpoint.OAuth2AccessTokenResponse;
import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest;
import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder;
import org.springframework.test.web.servlet.request.MockMvcRequestBuilders;

/**
* Tests for OAuth2 Authorization Code Grant final redirect
*
* @author Tadaya Tsuyukubo
* @see org.springframework.security.oauth2.client.web.OAuth2AuthorizationCodeGrantFilter
*/
public class OAuth2AuthorizationCodeGrantRedirectTests {

private static AuthorizationRequestRepository<OAuth2AuthorizationRequest> authorizationRequestRepository;
private static OAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> accessTokenResponseClient;

@Rule
public final SpringTestRule spring = new SpringTestRule();

@Autowired
private MockMvc mvc;

@Before
public void setUp() {
authorizationRequestRepository = mock(AuthorizationRequestRepository.class);
accessTokenResponseClient = mock(OAuth2AccessTokenResponseClient.class);
}

@Test
public void redirect() throws Exception {
perform("/redirect?code=MY-CODE&state=MY-STATE",
"http://localhost/redirect",
"http://localhost/redirect");
}

@Test
public void redirectWithParamAppended() throws Exception {
perform("/redirect?code=MY-CODE&state=MY-STATE&extra=EXTRA",
"http://localhost/redirect",
"http://localhost/redirect?extra=EXTRA");
}

@Test
public void redirectWithParameters() throws Exception {
perform("/redirect?foo=FOO&code=MY-CODE&state=MY-STATE",
"http://localhost/redirect?foo=FOO",
"http://localhost/redirect?foo=FOO");
}

@Test
public void redirectUrlWithParametersWithParamAppended() throws Exception {
perform("/redirect?foo=FOO&code=MY-CODE&state=MY-STATE&extra=EXTRA",
"http://localhost/redirect?foo=FOO",
"http://localhost/redirect?foo=FOO&extra=EXTRA");
}

private void perform(String requestUri, String authorizationRequestRedirectUri, String expectedRedirectUrl) throws Exception {
this.spring.register(WebSecurityConfiguration.class).autowire();

OAuth2AuthorizationRequest authorizationRequest = OAuth2AuthorizationRequest
.authorizationCode()
.authorizationUri("http://localhost/auth")
.clientId("example")
.state("MY-STATE")
.redirectUri(authorizationRequestRedirectUri)
.attributes(Collections.singletonMap(OAuth2ParameterNames.REGISTRATION_ID, "registration-id")) // comes from TestClientRegistrations.clientRegistration
.build();

when(authorizationRequestRepository.loadAuthorizationRequest(any(HttpServletRequest.class)))
.thenReturn(authorizationRequest);
when(authorizationRequestRepository.removeAuthorizationRequest(any(HttpServletRequest.class), any(HttpServletResponse.class)))
.thenReturn(authorizationRequest);

OAuth2AccessTokenResponse accessTokenResponse = OAuth2AccessTokenResponse
.withToken("MY-ACCESS-TOKEN")
.tokenType(OAuth2AccessToken.TokenType.BEARER)
.build();
when(accessTokenResponseClient
.getTokenResponse(any(OAuth2AuthorizationCodeGrantRequest.class)))
.thenReturn(accessTokenResponse);

MockHttpServletRequestBuilder builder = MockMvcRequestBuilders.get(requestUri);
this.mvc.perform(builder)
.andExpect(status().is3xxRedirection())
.andExpect(redirectedUrl(expectedRedirectUrl));
}

@Configuration
@EnableWebSecurity
static class WebSecurityConfiguration extends WebSecurityConfigurerAdapter {
@Override
protected void configure(HttpSecurity http) throws Exception {

ClientRegistration clientRegistration = TestClientRegistrations.clientRegistration().build();

InMemoryClientRegistrationRepository inMemoryClientRegistrationRepository =
new InMemoryClientRegistrationRepository(clientRegistration);

http.oauth2Client(oauth2client ->
oauth2client
.clientRegistrationRepository(inMemoryClientRegistrationRepository)
.authorizationCodeGrant()
.authorizationRequestRepository(authorizationRequestRepository)
.accessTokenResponseClient(accessTokenResponseClient)
);
}
}

}
107 changes: 102 additions & 5 deletions ...va/org/springframework/security/oauth2/client/web/OAuth2AuthorizationCodeGrantFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,18 @@
import org.springframework.util.MultiValueMap;
import org.springframework.util.StringUtils;
import org.springframework.web.filter.OncePerRequestFilter;
import org.springframework.web.util.UriComponents;
import org.springframework.web.util.UriComponentsBuilder;

import javax.servlet.FilterChain;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;

/**
* A {@code Filter} for the OAuth 2.0 Authorization Code Grant,
Expand Down Expand Up @@ -77,6 +82,7 @@
* </ul>
*
* @author Joe Grandja
* @author Tadaya Tsuyukubo
* @since 5.1
* @see OAuth2AuthorizationCodeAuthenticationToken
* @see OAuth2AuthorizationCodeAuthenticationProvider
Expand Down Expand Up @@ -145,16 +151,106 @@ private boolean shouldProcessAuthorizationResponse(HttpServletRequest request) {
if (authorizationRequest == null) {
return false;
}
String requestUrl = UrlUtils.buildFullRequestUrl(request.getScheme(), request.getServerName(),
request.getServerPort(), request.getRequestURI(), null);
MultiValueMap<String, String> params = OAuth2AuthorizationResponseUtils.toMultiMap(request.getParameterMap());
if (requestUrl.equals(authorizationRequest.getRedirectUri()) &&
OAuth2AuthorizationResponseUtils.isAuthorizationResponse(params)) {

if (OAuth2AuthorizationResponseUtils.isAuthorizationResponse(params) &&
isValidRedirectUrl(request, authorizationRequest.getRedirectUri())) {
return true;
}
return false;
}

private boolean isValidRedirectUrl(HttpServletRequest httpServletRequest, String redirectUrl) {
UriComponents request = UriComponentsBuilder.fromHttpUrl(UrlUtils.buildFullRequestUrl(httpServletRequest))
.replaceQueryParam(OAuth2ParameterNames.CODE)
.replaceQueryParam(OAuth2ParameterNames.STATE)
.build();

UriComponents redirect = UriComponentsBuilder.fromUriString(redirectUrl).build();

// Simple check first
if (request.toUriString().equals(redirectUrl)) {
return true;
}

// Compare each part of url one by one

if (!Objects.equals(request.getScheme(), redirect.getScheme())) {
return false;
}

if (!Objects.equals(request.getUserInfo(), redirect.getUserInfo())) {
return false;
}

if (!Objects.equals(request.getHost(), redirect.getHost())) {
return false;
}

if (!Objects.equals(request.getPort(), redirect.getPort())) {
return false;
}

if (!Objects.equals(request.getPath(), redirect.getPath())) {
return false;
}

if (redirect.getQuery() == null) {
return true; // no need to check request query params
}

// Compare request param/values are in exact order specified in redirect url.
// Request(Authorization Response) can have additional parameters appended which is allowed by spec.
// Since urls are hierarchical, UriComponents uses HierarchicalUriComponents which keeps the exact
// order of parameters.
MultiValueMap<String, String> requestParamMap = request.getQueryParams();
MultiValueMap<String, String> redirectParamMap = redirect.getQueryParams();

return containsRedirectUriParamsInOrder(requestParamMap, redirectParamMap);

}

private boolean containsRedirectUriParamsInOrder(MultiValueMap<String, String> requestMap, MultiValueMap<String, String> redirectMap) {
Iterator<Map.Entry<String, List<String>>> requestIterator = requestMap.entrySet().iterator();
Iterator<Map.Entry<String, List<String>>> redirectIterator = redirectMap.entrySet().iterator();

while (requestIterator.hasNext() && redirectIterator.hasNext()) {
Map.Entry<String, List<String>> requestEntry = requestIterator.next();
Map.Entry<String, List<String>> redirectEntry = redirectIterator.next();

String requestParam = requestEntry.getKey();
String redirectParam = redirectEntry.getKey();

if (!requestParam.equals(redirectParam)) {
return false; // param doesn't match
}

List<String> requestValues = requestEntry.getValue();
List<String> redirectValues = redirectEntry.getValue();

if (requestValues.size() < redirectValues.size()) {
return false; // request param values don't have ones specified in redirect param
}
// request may have additional param values; thus, iterate over redirect values
for (int i = 0; i < redirectValues.size(); i++) {
String requestValue = requestValues.get(i);
String redirectValue = redirectValues.get(i);

if (!requestValue.equals(redirectValue)) {
return false; // request param value doesn't match
}
}
}

// request may have additional params which is ok by spec

if (redirectIterator.hasNext()) {
return false; // request has less params than redirect params
}

return true;
}

private void processAuthorizationResponse(HttpServletRequest request, HttpServletResponse response)
throws IOException {

Expand All @@ -166,7 +262,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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

.replaceQueryParam(OAuth2ParameterNames.STATE)
.build()
.toUriString();
OAuth2AuthorizationResponse authorizationResponse = OAuth2AuthorizationResponseUtils.convert(params, redirectUri);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
63 changes: 62 additions & 1 deletion ...g/springframework/security/oauth2/client/web/OAuth2AuthorizationCodeGrantFilterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@
import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames;
import org.springframework.security.web.savedrequest.HttpSessionRequestCache;
import org.springframework.security.web.savedrequest.RequestCache;
import org.springframework.web.util.UriComponents;
import org.springframework.web.util.UriComponentsBuilder;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
Expand Down Expand Up @@ -362,13 +364,72 @@ public void doFilterWhenAuthorizationResponseSuccessAndAnonymousAccessNullAuthen
assertThat(authorizedClients.values().iterator().next()).isSameAs(authorizedClient);
}

@Test
public void doFilterWhenAuthorizationResponseSuccessThenRequestParameterCombinations() throws Exception {
// exact match
assertThat(isValidRedirect("http://localhost", "http://localhost")).isTrue();
assertThat(isValidRedirect("http://localhost/callback", "http://localhost/callback")).isTrue();
assertThat(isValidRedirect("http://localhost?key=FOO", "http://localhost?key=FOO")).isTrue();

// additional param
assertThat(isValidRedirect("http://localhost", "http://localhost?extra")).isTrue();
assertThat(isValidRedirect("http://localhost?key=FOO", "http://localhost?key=FOO&extra")).isTrue();
assertThat(isValidRedirect("http://localhost?key=FOO", "http://localhost?key=FOO&extra=EXTRA")).isTrue();
assertThat(isValidRedirect("http://localhost?key=FOO", "http://localhost?key=FOO&key=EXTRA")).isTrue();

// multi parameter
assertThat(isValidRedirect("http://localhost?key=FOO&key=BAR", "http://localhost?key=FOO&key=BAR")).isTrue();
assertThat(isValidRedirect("http://localhost?key=FOO&key=BAR", "http://localhost?key=FOO&key=BAR&key=EXTRA")).isTrue();
assertThat(isValidRedirect("http://localhost?key=FOO&key=BAR", "http://localhost?key=FOO&key=BAR&extra=EXTRA")).isTrue();


// wrong parameter
assertThat(isValidRedirect("http://localhost?key=FOO", "http://localhost?key=WRONG")).isFalse();
assertThat(isValidRedirect("http://localhost?key=FOO", "http://localhost?extra=EXTRA&key=FOO")).isFalse();
assertThat(isValidRedirect("http://localhost?key=FOO", "http://localhost?key=BAR&key=FOO")).isFalse();
assertThat(isValidRedirect("http://localhost?key=FOO", "http://localhost?key=FOOABC")).isFalse();
assertThat(isValidRedirect("http://localhost?key=FOO", "http://localhost")).isFalse();

assertThat(isValidRedirect("http://localhost?key=FOO&key=BAR", "http://localhost?key=BAR&key=FOO")).isFalse();
assertThat(isValidRedirect("http://localhost?key=FOO&key=BAR", "http://localhost?key=BAZ&key=FOO&key=BAR")).isFalse();
assertThat(isValidRedirect("http://localhost?key=FOO&key=BAR", "http://localhost?extra=EXTRA&key=FOO&key=BAR")).isFalse();
}

private boolean isValidRedirect(String redirectUrl, String requestUrl) throws Exception {
UriComponents redirect = UriComponentsBuilder.fromUriString(redirectUrl).build();
UriComponents request = UriComponentsBuilder.fromUriString(requestUrl).build();

MockHttpServletRequest mockRequest = new MockHttpServletRequest("GET", request.getPath());
mockRequest.setServletPath(request.getPath());
mockRequest.addParameter(OAuth2ParameterNames.CODE, "code");
mockRequest.addParameter(OAuth2ParameterNames.STATE, "state");
mockRequest.setQueryString(request.getQuery());

MockHttpServletResponse mockResponse = new MockHttpServletResponse();
FilterChain filterChain = mock(FilterChain.class);

this.setUpAuthorizationRequest(mockRequest, mockResponse, redirect.toUriString(), this.registration1);
this.setUpAuthenticationResult(this.registration1);

this.filter.doFilter(mockRequest, mockResponse, filterChain);

String redirected = mockResponse.getRedirectedUrl();
return requestUrl.equals(redirected);
}

private void setUpAuthorizationRequest(HttpServletRequest request, HttpServletResponse response,
ClientRegistration registration) {
String redirectUri = request.getRequestURL().toString();
setUpAuthorizationRequest(request, response, redirectUri, registration);
}

private void setUpAuthorizationRequest(HttpServletRequest request, HttpServletResponse response,
String redirectUri, ClientRegistration registration) {
Map<String, Object> additionalParameters = new HashMap<>();
additionalParameters.put(OAuth2ParameterNames.REGISTRATION_ID, registration.getRegistrationId());
OAuth2AuthorizationRequest authorizationRequest = request()
.additionalParameters(additionalParameters)
.redirectUri(request.getRequestURL().toString()).build();
.redirectUri(redirectUri).build();
this.authorizationRequestRepository.saveAuthorizationRequest(authorizationRequest, request, response);
}

Expand Down