From 16d39adbc04bb0cdf217226803d05d6956595d85 Mon Sep 17 00:00:00 2001 From: Dirk Koehler Date: Mon, 28 Jan 2019 23:13:55 -0800 Subject: [PATCH] DefaultRedirectResolver matches on userinfo and query Fixes gh-1585 --- .../endpoint/DefaultRedirectResolver.java | 98 +++++++++++--- .../code/SubdomainRedirectResolverTests.java | 6 +- .../DefaultRedirectResolverTests.java | 126 ++++++++++++++++-- .../AuthorizationCodeProviderCookieTests.java | 2 +- .../AuthorizationCodeProviderCookieTests.java | 2 +- 5 files changed, 201 insertions(+), 33 deletions(-) diff --git a/spring-security-oauth2/src/main/java/org/springframework/security/oauth2/provider/endpoint/DefaultRedirectResolver.java b/spring-security-oauth2/src/main/java/org/springframework/security/oauth2/provider/endpoint/DefaultRedirectResolver.java index 3ef111621..d521b5417 100644 --- a/spring-security-oauth2/src/main/java/org/springframework/security/oauth2/provider/endpoint/DefaultRedirectResolver.java +++ b/spring-security-oauth2/src/main/java/org/springframework/security/oauth2/provider/endpoint/DefaultRedirectResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * 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. @@ -21,6 +21,7 @@ import org.springframework.security.oauth2.common.exceptions.RedirectMismatchException; import org.springframework.security.oauth2.provider.ClientDetails; import org.springframework.util.Assert; +import org.springframework.util.MultiValueMap; import org.springframework.util.StringUtils; import org.springframework.web.util.UriComponents; import org.springframework.web.util.UriComponentsBuilder; @@ -28,6 +29,8 @@ import java.util.Arrays; import java.util.Collection; import java.util.HashSet; +import java.util.Iterator; +import java.util.List; import java.util.Set; /** @@ -47,7 +50,7 @@ public class DefaultRedirectResolver implements RedirectResolver { /** * Flag to indicate that requested URIs will match if they are a subdomain of the registered value. * - * @param matchSubdomains the flag value to set (deafult true) + * @param matchSubdomains the flag value to set (default true) */ public void setMatchSubdomains(boolean matchSubdomains) { this.matchSubdomains = matchSubdomains; @@ -105,7 +108,8 @@ private boolean containsRedirectGrantType(Set grantTypes) { /** * Whether the requested redirect URI "matches" the specified redirect URI. For a URL, this implementation tests if * the user requested redirect starts with the registered redirect, so it would have the same host and root path if - * it is an HTTP URL. The port is also matched. + * it is an HTTP URL. The port, userinfo, query params also matched. Request redirect uri path can include + * additional parameters which are ignored for the match *

* For other (non-URL) cases, such as for some implicit clients, the redirect_uri must be an exact match. * @@ -115,22 +119,65 @@ private boolean containsRedirectGrantType(Set grantTypes) { */ protected boolean redirectMatches(String requestedRedirect, String redirectUri) { UriComponents requestedRedirectUri = UriComponentsBuilder.fromUriString(requestedRedirect).build(); - String requestedRedirectUriScheme = (requestedRedirectUri.getScheme() != null ? requestedRedirectUri.getScheme() : ""); - String requestedRedirectUriHost = (requestedRedirectUri.getHost() != null ? requestedRedirectUri.getHost() : ""); - String requestedRedirectUriPath = (requestedRedirectUri.getPath() != null ? requestedRedirectUri.getPath() : ""); - UriComponents registeredRedirectUri = UriComponentsBuilder.fromUriString(redirectUri).build(); - String registeredRedirectUriScheme = (registeredRedirectUri.getScheme() != null ? registeredRedirectUri.getScheme() : ""); - String registeredRedirectUriHost = (registeredRedirectUri.getHost() != null ? registeredRedirectUri.getHost() : ""); - String registeredRedirectUriPath = (registeredRedirectUri.getPath() != null ? registeredRedirectUri.getPath() : ""); - boolean portsMatch = this.matchPorts ? (registeredRedirectUri.getPort() == requestedRedirectUri.getPort()) : true; + boolean schemeMatch = isEqual(registeredRedirectUri.getScheme(), requestedRedirectUri.getScheme()); + boolean userInfoMatch = isEqual(registeredRedirectUri.getUserInfo(), requestedRedirectUri.getUserInfo()); + boolean hostMatch = hostMatches(registeredRedirectUri.getHost(), requestedRedirectUri.getHost()); + boolean portMatch = matchPorts ? registeredRedirectUri.getPort() == requestedRedirectUri.getPort() : true; + boolean pathMatch = isEqual(registeredRedirectUri.getPath(), + StringUtils.cleanPath(requestedRedirectUri.getPath())); + boolean queryParamMatch = matchQueryParams(registeredRedirectUri.getQueryParams(), + requestedRedirectUri.getQueryParams()); + + return schemeMatch && userInfoMatch && hostMatch && portMatch && pathMatch && queryParamMatch; + } + + + /** + * Checks whether the registered redirect uri query params key and values contains match the requested set + * + * The requested redirect uri query params are allowed to contain additional params which will be retained + * + * @param registeredRedirectUriQueryParams + * @param requestedRedirectUriQueryParams + * @return whether the params match + */ + private boolean matchQueryParams(MultiValueMap registeredRedirectUriQueryParams, + MultiValueMap requestedRedirectUriQueryParams) { + + + Iterator iter = registeredRedirectUriQueryParams.keySet().iterator(); + while (iter.hasNext()) { + String key = iter.next(); + List registeredRedirectUriQueryParamsValues = registeredRedirectUriQueryParams.get(key); + List requestedRedirectUriQueryParamsValues = requestedRedirectUriQueryParams.get(key); + + if (!registeredRedirectUriQueryParamsValues.equals(requestedRedirectUriQueryParamsValues)) { + return false; + } + } + + return true; + } + + - return registeredRedirectUriScheme.equals(requestedRedirectUriScheme) && - hostMatches(registeredRedirectUriHost, requestedRedirectUriHost) && - portsMatch && - // Ensure exact path matching - registeredRedirectUriPath.equals(StringUtils.cleanPath(requestedRedirectUriPath)); + /** + * Compares two strings but treats empty string or null equal + * + * @param str1 + * @param str2 + * @return true if strings are equal, false otherwise + */ + private boolean isEqual(String str1, String str2) { + if (StringUtils.isEmpty(str1) && StringUtils.isEmpty(str2)) { + return true; + } else if (!StringUtils.isEmpty(str1)) { + return str1.equals(str2); + } else { + return false; + } } /** @@ -152,7 +199,7 @@ protected boolean hostMatches(String registered, String requested) { * * @param redirectUris the set of the registered URIs to try and find a match. This cannot be null or empty. * @param requestedRedirect the URI used as part of the request - * @return the matching URI + * @return redirect uri * @throws RedirectMismatchException if no match was found */ private String obtainMatchingRedirect(Set redirectUris, String requestedRedirect) { @@ -161,11 +208,26 @@ private String obtainMatchingRedirect(Set redirectUris, String requested if (redirectUris.size() == 1 && requestedRedirect == null) { return redirectUris.iterator().next(); } + for (String redirectUri : redirectUris) { if (requestedRedirect != null && redirectMatches(requestedRedirect, redirectUri)) { - return requestedRedirect; + // Initialize with the registered redirect-uri + UriComponentsBuilder redirectUriBuilder = UriComponentsBuilder.fromUriString(redirectUri); + + UriComponents requestedRedirectUri = UriComponentsBuilder.fromUriString(requestedRedirect).build(); + + if (this.matchSubdomains) { + redirectUriBuilder.host(requestedRedirectUri.getHost()); + } + if (!this.matchPorts) { + redirectUriBuilder.port(requestedRedirectUri.getPort()); + } + redirectUriBuilder.replaceQuery(requestedRedirectUri.getQuery()); // retain additional params (if any) + redirectUriBuilder.fragment(null); + return redirectUriBuilder.build().toUriString(); } } + throw new RedirectMismatchException("Invalid redirect: " + requestedRedirect + " does not match one of the registered values: " + redirectUris.toString()); } diff --git a/spring-security-oauth2/src/test/java/org/springframework/security/oauth2/provider/code/SubdomainRedirectResolverTests.java b/spring-security-oauth2/src/test/java/org/springframework/security/oauth2/provider/code/SubdomainRedirectResolverTests.java index ebfd3fe2f..0f2f37196 100644 --- a/spring-security-oauth2/src/test/java/org/springframework/security/oauth2/provider/code/SubdomainRedirectResolverTests.java +++ b/spring-security-oauth2/src/test/java/org/springframework/security/oauth2/provider/code/SubdomainRedirectResolverTests.java @@ -23,7 +23,7 @@ public class SubdomainRedirectResolverTests @Test - public void testRedirectWatchdox() throws Exception + public void testRedirectMatch() throws Exception { Set redirectUris = new HashSet(Arrays.asList("http://watchdox.com")); client.setRegisteredRedirectUri(redirectUris); @@ -32,9 +32,9 @@ public void testRedirectWatchdox() throws Exception } @Test(expected=RedirectMismatchException.class) - public void testRedirectBadWatchdox() throws Exception + public void testRedirectNoMatch() throws Exception { - Set redirectUris = new HashSet(Arrays.asList("http//watchdox.com")); + Set redirectUris = new HashSet(Arrays.asList("http://watchdox.com")); client.setRegisteredRedirectUri(redirectUris); String requestedRedirect = "http://anywhere.google.com"; assertEquals(requestedRedirect, resolver.resolveRedirect(requestedRedirect, client)); diff --git a/spring-security-oauth2/src/test/java/org/springframework/security/oauth2/provider/endpoint/DefaultRedirectResolverTests.java b/spring-security-oauth2/src/test/java/org/springframework/security/oauth2/provider/endpoint/DefaultRedirectResolverTests.java index 0be833390..6935ea74e 100644 --- a/spring-security-oauth2/src/test/java/org/springframework/security/oauth2/provider/endpoint/DefaultRedirectResolverTests.java +++ b/spring-security-oauth2/src/test/java/org/springframework/security/oauth2/provider/endpoint/DefaultRedirectResolverTests.java @@ -1,14 +1,17 @@ /* - * Copyright 2006-2011 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 - * - * http://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. + * 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 + * + * http://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.oauth2.provider.endpoint; @@ -180,4 +183,107 @@ public void testRedirectMatchPortsFalse() throws Exception { String requestedRedirect = "http://anywhere.com:91"; assertEquals(requestedRedirect, resolver.resolveRedirect(requestedRedirect, client)); } + + // gh-1566 + @Test(expected = RedirectMismatchException.class) + public void testRedirectRegisteredUserInfoNotMatching() throws Exception { + Set redirectUris = new HashSet(Arrays.asList("http://userinfo@anywhere.com")); + client.setRegisteredRedirectUri(redirectUris); + resolver.resolveRedirect("http://otheruserinfo@anywhere.com", client); + } + + // gh-1566 + @Test(expected = RedirectMismatchException.class) + public void testRedirectRegisteredNoUserInfoNotMatching() throws Exception { + Set redirectUris = new HashSet(Arrays.asList("http://userinfo@anywhere.com")); + client.setRegisteredRedirectUri(redirectUris); + resolver.resolveRedirect("http://anywhere.com", client); + } + + // gh-1566 + @Test() + public void testRedirectRegisteredUserInfoMatching() throws Exception { + Set redirectUris = new HashSet(Arrays.asList("http://userinfo@anywhere.com")); + client.setRegisteredRedirectUri(redirectUris); + String requestedRedirect = "http://userinfo@anywhere.com"; + assertEquals(requestedRedirect, resolver.resolveRedirect(requestedRedirect, client)); + } + + // gh-1566 + @Test() + public void testRedirectRegisteredFragmentIgnoredAndStripped() throws Exception { + Set redirectUris = new HashSet(Arrays.asList("http://userinfo@anywhere.com/foo/bar#baz")); + client.setRegisteredRedirectUri(redirectUris); + String requestedRedirect = "http://userinfo@anywhere.com/foo/bar"; + assertEquals(requestedRedirect, resolver.resolveRedirect(requestedRedirect + "#bar", client)); + } + + // gh-1566 + @Test() + public void testRedirectRegisteredQueryParamsMatching() throws Exception { + Set redirectUris = new HashSet(Arrays.asList("http://anywhere.com/?p1=v1&p2=v2")); + client.setRegisteredRedirectUri(redirectUris); + String requestedRedirect = "http://anywhere.com/?p1=v1&p2=v2"; + assertEquals(requestedRedirect, resolver.resolveRedirect(requestedRedirect, client)); + } + + // gh-1566 + @Test() + public void testRedirectRegisteredQueryParamsMatchingIgnoringAdditionalParams() throws Exception { + Set redirectUris = new HashSet(Arrays.asList("http://anywhere.com/?p1=v1&p2=v2")); + client.setRegisteredRedirectUri(redirectUris); + String requestedRedirect = "http://anywhere.com/?p1=v1&p2=v2&p3=v3"; + assertEquals(requestedRedirect, resolver.resolveRedirect(requestedRedirect, client)); + } + + // gh-1566 + @Test() + public void testRedirectRegisteredQueryParamsMatchingDifferentOrder() throws Exception { + Set redirectUris = new HashSet(Arrays.asList("http://anywhere.com/?p1=v1&p2=v2")); + client.setRegisteredRedirectUri(redirectUris); + String requestedRedirect = "http://anywhere.com/?p2=v2&p1=v1"; + assertEquals(requestedRedirect, resolver.resolveRedirect(requestedRedirect, client)); + } + + // gh-1566 + @Test(expected = RedirectMismatchException.class) + public void testRedirectRegisteredQueryParamsWithDifferentValues() throws Exception { + Set redirectUris = new HashSet(Arrays.asList("http://anywhere.com/?p1=v1&p2=v2")); + client.setRegisteredRedirectUri(redirectUris); + resolver.resolveRedirect("http://anywhere.com/?p1=v1&p2=v3", client); + } + + // gh-1566 + @Test(expected = RedirectMismatchException.class) + public void testRedirectRegisteredQueryParamsNotMatching() throws Exception { + Set redirectUris = new HashSet(Arrays.asList("http://anywhere.com/?p1=v1")); + client.setRegisteredRedirectUri(redirectUris); + resolver.resolveRedirect("http://anywhere.com/?p2=v2", client); + } + + // gh-1566 + @Test(expected = RedirectMismatchException.class) + public void testRedirectRegisteredQueryParamsPartiallyMatching() throws Exception { + Set redirectUris = new HashSet(Arrays.asList("http://anywhere.com/?p1=v1&p2=v2")); + client.setRegisteredRedirectUri(redirectUris); + resolver.resolveRedirect("http://anywhere.com/?p2=v2&p3=v3", client); + } + + // gh-1566 + @Test + public void testRedirectRegisteredQueryParamsMatchingWithMultipleValuesInRegistered() throws Exception { + Set redirectUris = new HashSet(Arrays.asList("http://anywhere.com/?p1=v11&p1=v12")); + client.setRegisteredRedirectUri(redirectUris); + String requestedRedirect = "http://anywhere.com/?p1=v11&p1=v12"; + assertEquals(requestedRedirect, resolver.resolveRedirect(requestedRedirect, client)); + } + + // gh-1566 + @Test + public void testRedirectRegisteredQueryParamsMatchingWithParamWithNoValue() throws Exception { + Set redirectUris = new HashSet(Arrays.asList("http://anywhere.com/?p1&p2=v2")); + client.setRegisteredRedirectUri(redirectUris); + String requestedRedirect = "http://anywhere.com/?p1&p2=v2"; + assertEquals(requestedRedirect, resolver.resolveRedirect(requestedRedirect, client)); + } } \ No newline at end of file diff --git a/tests/annotation/jpa/src/test/java/demo/AuthorizationCodeProviderCookieTests.java b/tests/annotation/jpa/src/test/java/demo/AuthorizationCodeProviderCookieTests.java index fdfae6dc2..26d88d46a 100644 --- a/tests/annotation/jpa/src/test/java/demo/AuthorizationCodeProviderCookieTests.java +++ b/tests/annotation/jpa/src/test/java/demo/AuthorizationCodeProviderCookieTests.java @@ -31,7 +31,7 @@ public class AuthorizationCodeProviderCookieTests extends AbstractEmptyAuthoriza @Test @OAuth2ContextConfiguration(resource = MyClientWithRegisteredRedirect.class, initialize = false) public void testPostToProtectedResource() throws Exception { - approveAccessTokenGrant("http://anywhere", true); + approveAccessTokenGrant("http://anywhere?key=value", true); assertNotNull(context.getAccessToken()); LinkedMultiValueMap form = new LinkedMultiValueMap<>(); form.set("foo", "bar"); diff --git a/tests/annotation/vanilla/src/test/java/demo/AuthorizationCodeProviderCookieTests.java b/tests/annotation/vanilla/src/test/java/demo/AuthorizationCodeProviderCookieTests.java index fdfae6dc2..26d88d46a 100644 --- a/tests/annotation/vanilla/src/test/java/demo/AuthorizationCodeProviderCookieTests.java +++ b/tests/annotation/vanilla/src/test/java/demo/AuthorizationCodeProviderCookieTests.java @@ -31,7 +31,7 @@ public class AuthorizationCodeProviderCookieTests extends AbstractEmptyAuthoriza @Test @OAuth2ContextConfiguration(resource = MyClientWithRegisteredRedirect.class, initialize = false) public void testPostToProtectedResource() throws Exception { - approveAccessTokenGrant("http://anywhere", true); + approveAccessTokenGrant("http://anywhere?key=value", true); assertNotNull(context.getAccessToken()); LinkedMultiValueMap form = new LinkedMultiValueMap<>(); form.set("foo", "bar");