added strict URI matching option to redirect resolver (off by default)

openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/BlacklistAwareRedirectResolver.java

@@ -27,7 +27,13 @@ import org.springframework.security.oauth2.provider.ClientDetails;
 import org.springframework.security.oauth2.provider.endpoint.DefaultRedirectResolver;
 import org.springframework.stereotype.Component;
 
+import com.google.common.base.Strings;
+
 /**
+ * 
+ * A redirect resolver that knows how to check against the blacklisted URIs
+ * for forbidden values. Can be configured to do strict string matching also.
+ * 
  * @author jricher
  *
  */
@@ -36,7 +42,9 @@ public class BlacklistAwareRedirectResolver extends DefaultRedirectResolver {
 
 	@Autowired
 	private BlacklistedSiteService blacklistService;
-
+	
+	private boolean strictMatch = false;
+	
 	/* (non-Javadoc)
 	 * @see org.springframework.security.oauth2.provider.endpoint.RedirectResolver#resolveRedirect(java.lang.String, org.springframework.security.oauth2.provider.ClientDetails)
 	 */
@@ -52,4 +60,38 @@ public class BlacklistAwareRedirectResolver extends DefaultRedirectResolver {
 		}
 	}
 
+	/* (non-Javadoc)
+	 * @see org.springframework.security.oauth2.provider.endpoint.DefaultRedirectResolver#redirectMatches(java.lang.String, java.lang.String)
+	 */
+	@Override
+	protected boolean redirectMatches(String requestedRedirect, String redirectUri) {
+		
+		if (isStrictMatch()) {
+			// we're doing a strict string match for all clients
+			return Strings.nullToEmpty(requestedRedirect).equals(redirectUri);
+		} else {
+			// otherwise do the prefix-match from the library
+			return super.redirectMatches(requestedRedirect, redirectUri);
+		}
+		
+	}
+
+	/**
+	 * @return the strictMatch
+	 */
+	public boolean isStrictMatch() {
+		return strictMatch;
+	}
+
+	/**
+	 * Set this to true to require exact string matches for all redirect URIs. (Default is false)
+	 * 
+	 * @param strictMatch the strictMatch to set
+	 */
+	public void setStrictMatch(boolean strictMatch) {
+		this.strictMatch = strictMatch;
+	}
+
+	
+	
 }

openid-connect-server/src/test/java/org/mitre/oauth2/service/impl/TestBlacklistAwareRedirectResolver.java

@@ -0,0 +1,132 @@
+/*******************************************************************************
+ * Copyright 2015 The MITRE Corporation
+ *   and the MIT Kerberos and Internet Trust Consortium
+ *
+ * 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.mitre.oauth2.service.impl;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mitre.openid.connect.service.BlacklistedSiteService;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+import org.springframework.security.oauth2.common.exceptions.InvalidRequestException;
+import org.springframework.security.oauth2.provider.ClientDetails;
+
+import com.google.common.collect.ImmutableSet;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.is;
+
+import static org.mockito.Matchers.anyString;
+
+import static org.mockito.Mockito.when;
+
+import static org.junit.Assert.assertThat;
+
+/**
+ * @author jricher
+ *
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class TestBlacklistAwareRedirectResolver {
+
+	@Mock
+	private BlacklistedSiteService blacklistService;
+	
+	@Mock
+	private ClientDetails client;
+	
+	@InjectMocks
+	private BlacklistAwareRedirectResolver resolver;
+	
+	private String blacklistedUri = "https://evil.example.com/";
+
+	private String goodUri = "https://good.example.com/";
+	
+	private String pathUri = "https://good.example.com/with/path";
+	
+	/**
+	 * @throws java.lang.Exception
+	 */
+	@Before
+	public void setUp() throws Exception {
+		
+		when(blacklistService.isBlacklisted(anyString())).thenReturn(false);
+		when(blacklistService.isBlacklisted(blacklistedUri)).thenReturn(true);
+		
+		when(client.getAuthorizedGrantTypes()).thenReturn(ImmutableSet.of("authorization_code"));
+		when(client.getRegisteredRedirectUri()).thenReturn(ImmutableSet.of(goodUri, blacklistedUri));
+		
+	}
+
+	@Test
+	public void testResolveRedirect_safe() {
+
+		// default uses prefix matching, both of these should work
+		
+		String res1 = resolver.resolveRedirect(goodUri, client);
+		
+		assertThat(res1, is(equalTo(goodUri)));
+		
+		String res2 = resolver.resolveRedirect(pathUri, client);
+		
+		assertThat(res2, is(equalTo(pathUri)));
+		
+		
+	}
+	
+	@Test(expected = InvalidRequestException.class)
+	public void testResolveRedirect_blacklisted() {
+		
+		// this should fail with an error 
+		resolver.resolveRedirect(blacklistedUri, client);
+		
+	}
+
+	@Test
+	public void testRedirectMatches_strict() {
+		resolver.setStrictMatch(true);
+		
+		// this is not an exact match
+		boolean res1 = resolver.redirectMatches(pathUri, goodUri);
+		
+		assertThat(res1, is(false));
+		
+		// this is an exact match
+		boolean res2 = resolver.redirectMatches(goodUri, goodUri);
+		
+		assertThat(res2, is(true));
+		
+	}
+	
+	@Test
+	public void testRedirectMatches_default() {
+		
+		// this is not an exact match (but that's OK)
+		boolean res1 = resolver.redirectMatches(pathUri, goodUri);
+		
+		assertThat(res1, is(true));
+		
+		// this is an exact match
+		boolean res2 = resolver.redirectMatches(goodUri, goodUri);
+		
+		assertThat(res2, is(true));
+
+	}
+
+}