DefaultRedirectResolver matches on userinfo and query

phrinx · jgrandja · commit 16d39adbc04b · 2019-02-18T20:50:34.000-05:00
Fixes gh-1585
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
@@ -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,13 +21,16 @@
 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;
 
 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<String> 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
 	 * <p>
 	 * 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<String> 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<String, String> registeredRedirectUriQueryParams,
+									 MultiValueMap<String, String> requestedRedirectUriQueryParams) {
+
+
+		Iterator<String> iter = registeredRedirectUriQueryParams.keySet().iterator();
+		while (iter.hasNext()) {
+			String key = iter.next();
+			List<String> registeredRedirectUriQueryParamsValues = registeredRedirectUriQueryParams.get(key);
+			List<String> 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<String> redirectUris, String requestedRedirect) {
@@ -161,11 +208,26 @@ private String obtainMatchingRedirect(Set<String> 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
@@ -23,7 +23,7 @@ public class SubdomainRedirectResolverTests
 
 
 	@Test
-	public void testRedirectWatchdox() throws Exception 
+	public void testRedirectMatch() throws Exception
 	{
 		Set<String> redirectUris = new HashSet<String>(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<String> redirectUris = new HashSet<String>(Arrays.asList("http//watchdox.com"));
+		Set<String> redirectUris = new HashSet<String>(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
@@ -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<String> redirectUris = new HashSet<String>(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<String> redirectUris = new HashSet<String>(Arrays.asList("http://userinfo@anywhere.com"));
+		client.setRegisteredRedirectUri(redirectUris);
+		resolver.resolveRedirect("http://anywhere.com", client);
+	}
+
+	// gh-1566
+	@Test()
+	public void testRedirectRegisteredUserInfoMatching() throws Exception {
+		Set<String> redirectUris = new HashSet<String>(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<String> redirectUris = new HashSet<String>(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<String> redirectUris = new HashSet<String>(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<String> redirectUris = new HashSet<String>(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<String> redirectUris = new HashSet<String>(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<String> redirectUris = new HashSet<String>(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<String> redirectUris = new HashSet<String>(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<String> redirectUris = new HashSet<String>(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<String> redirectUris = new HashSet<String>(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<String> redirectUris = new HashSet<String>(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));
+	}
 }
diff --git 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<String, String> 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
@@ -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<String, String> form = new LinkedMultiValueMap<>();
 		form.set("foo", "bar");

Original file line number	Diff line number	Diff line change
`@@ -23,7 +23,7 @@ public class SubdomainRedirectResolverTests`
`23`	`23`
`24`	`24`
`25`	`25`	`@Test`
`26`		`- public void testRedirectWatchdox() throws Exception`
	`26`	`+ public void testRedirectMatch() throws Exception`
`27`	`27`	`{`
`28`	`28`	`Set<String> redirectUris = new HashSet<String>(Arrays.asList("http://watchdox.com"));`
`29`	`29`	`client.setRegisteredRedirectUri(redirectUris);`
`@@ -32,9 +32,9 @@ public void testRedirectWatchdox() throws Exception`
`32`	`32`	`}`
`33`	`33`
`34`	`34`	`@Test(expected=RedirectMismatchException.class)`
`35`		`- public void testRedirectBadWatchdox() throws Exception`
	`35`	`+ public void testRedirectNoMatch() throws Exception`
`36`	`36`	`{`
`37`		`- Set<String> redirectUris = new HashSet<String>(Arrays.asList("http//watchdox.com"));`
	`37`	`+ Set<String> redirectUris = new HashSet<String>(Arrays.asList("http://watchdox.com"));`
`38`	`38`	`client.setRegisteredRedirectUri(redirectUris);`
`39`	`39`	`String requestedRedirect = "http://anywhere.google.com";`
`40`	`40`	`assertEquals(requestedRedirect, resolver.resolveRedirect(requestedRedirect, client));`