Request cache only saves requests with form_redirect_uri if it's valid

- A valid/approved form_redirect_uri value has the same host as the receiving UAA server. [#158222161] https://www.pivotaltracker.com/story/show/158222161 Signed-off-by: Jaskanwal Pawar <jpawar@pivotal.io> Signed-off-by: Jennifer Hamon <jhamon@pivotal.io>
cloudfoundry · Jun 13, 2018 · 8a59944 · 8a59944
1 parent 7d750e0
commit 8a59944
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 37 deletions.
diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtils.java b/server/src/main/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtils.java
@@ -14,6 +14,15 @@
  */
 package org.cloudfoundry.identity.uaa.util;
 
+import org.cloudfoundry.identity.uaa.zone.IdentityZone;
+import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder;
+import org.springframework.util.AntPathMatcher;
+import org.springframework.util.MultiValueMap;
+import org.springframework.util.StringUtils;
+import org.springframework.web.servlet.support.ServletUriComponentsBuilder;
+import org.springframework.web.util.UriComponentsBuilder;
+import org.springframework.web.util.UriUtils;
+
 import javax.servlet.http.HttpServletRequest;
 import java.io.UnsupportedEncodingException;
 import java.net.MalformedURLException;
@@ -25,16 +34,6 @@
 import java.util.Map;
 import java.util.regex.Pattern;
 
-import org.cloudfoundry.identity.uaa.zone.IdentityZone;
-import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder;
-
-import org.springframework.util.AntPathMatcher;
-import org.springframework.util.MultiValueMap;
-import org.springframework.util.StringUtils;
-import org.springframework.web.servlet.support.ServletUriComponentsBuilder;
-import org.springframework.web.util.UriComponentsBuilder;
-import org.springframework.web.util.UriUtils;
-
 import static java.util.Collections.emptyList;
 import static java.util.Optional.ofNullable;
 import static org.springframework.util.StringUtils.hasText;
@@ -228,4 +227,17 @@ public static String getRequestPath(HttpServletRequest request) {
         String path = String.format("%s%s", servletPath, pathInfo);
         return path;
     }
+
+    public static boolean uriHasMatchingHost(String uri, String hostname) {
+        if (uri == null) {
+            return false;
+        }
+
+        try {
+            URL url = new URL(uri);
+            return hostname.equals(url.getHost());
+        } catch (MalformedURLException e) {
+            return false;
+        }
+    }
 }
diff --git a/...a/org/cloudfoundry/identity/uaa/web/UaaSavedRequestAwareAuthenticationSuccessHandler.java b/...a/org/cloudfoundry/identity/uaa/web/UaaSavedRequestAwareAuthenticationSuccessHandler.java
@@ -17,12 +17,11 @@
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
+import org.cloudfoundry.identity.uaa.util.UaaUrlUtils;
 import org.springframework.security.web.authentication.SavedRequestAwareAuthenticationSuccessHandler;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
-import java.net.MalformedURLException;
-import java.net.URL;
 
 public class UaaSavedRequestAwareAuthenticationSuccessHandler extends SavedRequestAwareAuthenticationSuccessHandler {
     public static final String SAVED_REQUEST_SESSION_ATTRIBUTE = "SPRING_SECURITY_SAVED_REQUEST";
@@ -40,23 +39,10 @@ public String determineTargetUrl(HttpServletRequest request, HttpServletResponse
         if (redirectAttribute !=null) {
             logger.debug("Returning redirectAttribute saved URI:"+redirectAttribute);
             return (String) redirectAttribute;
-        } else if (isApprovedFormRedirectUri(request, redirectFormParam)) {
+        } else if (UaaUrlUtils.uriHasMatchingHost(redirectFormParam, request.getServerName())) {
             return redirectFormParam;
         } else {
             return super.determineTargetUrl(request, response);
         }
     }
-
-    private boolean isApprovedFormRedirectUri(HttpServletRequest request, String redirectUri) {
-        if (redirectUri == null) {
-            return false;
-        }
-
-        try {
-            URL url = new URL(redirectUri);
-            return request.getServerName().equals(url.getHost());
-        } catch (MalformedURLException e) {
-            return false;
-        }
-    }
 }
diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/web/UaaSavedRequestCache.java b/server/src/main/java/org/cloudfoundry/identity/uaa/web/UaaSavedRequestCache.java
@@ -92,7 +92,9 @@ protected boolean shouldSaveFormRedirectParameter(HttpServletRequest request) {
         if (StringUtils.isEmpty(formRedirect)) {
             return false;
         }
-
+        if (!UaaUrlUtils.uriHasMatchingHost(formRedirect, request.getServerName())) {
+            return false;
+        }
         if (hasSavedRequest(request)) {
             return false;
         }

diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtilsTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtilsTest.java
@@ -12,29 +12,30 @@
  *******************************************************************************/
 package org.cloudfoundry.identity.uaa.util;
 
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.stream.Collectors;
-
 import org.cloudfoundry.identity.uaa.zone.IdentityZone;
 import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder;
 import org.cloudfoundry.identity.uaa.zone.MultitenancyFixture;
-
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.web.context.request.RequestContextHolder;
 import org.springframework.web.context.request.ServletRequestAttributes;
 
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 public class UaaUrlUtilsTest {
@@ -422,6 +423,19 @@ public void addSubdomain_handlesUnexpectedDotInSubdomain_currentZone() {
         assertEquals("http://somezone2.localhost:8080", url2);
     }
 
+    @Test
+    public void testUriHasMatchingHost() {
+        assertTrue(UaaUrlUtils.uriHasMatchingHost("http://test.com/test", "test.com"));
+        assertTrue(UaaUrlUtils.uriHasMatchingHost("http://subdomain.test.com/test", "subdomain.test.com"));
+        assertTrue(UaaUrlUtils.uriHasMatchingHost("http://1.2.3.4/test", "1.2.3.4"));
+
+        assertFalse(UaaUrlUtils.uriHasMatchingHost(null, "test.com"));
+        assertFalse(UaaUrlUtils.uriHasMatchingHost("http://not-test.com/test", "test.com"));
+        assertFalse(UaaUrlUtils.uriHasMatchingHost("not-valid-url", "test.com"));
+        assertFalse(UaaUrlUtils.uriHasMatchingHost("http://1.2.3.4/test", "test.com"));
+        assertFalse(UaaUrlUtils.uriHasMatchingHost("http://test.com/test", "1.2.3.4"));
+    }
+
     private void validateRedirectUri(List<String> urls, boolean result) {
         Map<String, String> failed = getFailedUrls(urls, result);
         if (!failed.isEmpty()) {

diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/web/UaaSavedRequestCacheTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/web/UaaSavedRequestCacheTests.java
@@ -28,8 +28,13 @@
 import org.springframework.security.web.savedrequest.SavedRequest;
 
 import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
 import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpSession;
+import java.io.IOException;
+import java.net.MalformedURLException;
+import java.net.URL;
 
 import static org.cloudfoundry.identity.uaa.web.UaaSavedRequestAwareAuthenticationSuccessHandler.FORM_REDIRECT_PARAMETER;
 import static org.cloudfoundry.identity.uaa.web.UaaSavedRequestAwareAuthenticationSuccessHandler.SAVED_REQUEST_SESSION_ATTRIBUTE;
@@ -38,6 +43,7 @@
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Matchers.anyObject;
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Mockito.mock;
@@ -103,6 +109,7 @@ public void filter_saves_when_needed() throws Exception {
         request.setPathInfo("/login.do");
         request.setRequestURI("/login.do");
         request.setParameter(FORM_REDIRECT_PARAMETER, redirectUri);
+        request.setServerName(new URL(redirectUri).getHost());
         assertTrue(cache.shouldSaveFormRedirectParameter(request));
         ServletResponse response = new MockHttpServletResponse();
 
@@ -145,8 +152,11 @@ public void saveFormRedirectRequest_GET_Method() throws Exception {
 
     @Test
     public void saveFormRedirectRequest() throws Exception {
+        String redirectUri = "http://login";
         request.setSession(session);
-        request.setParameter(FORM_REDIRECT_PARAMETER, "http://login");
+        request.setParameter(FORM_REDIRECT_PARAMETER, redirectUri);
+        request.setServerName(new URL(redirectUri).getHost());
+
         spy.saveRequest(request, new MockHttpServletResponse());
         verify(spy).saveClientRedirect(request, request.getParameter(FORM_REDIRECT_PARAMETER));
     }
@@ -169,14 +179,19 @@ public void only_save_for_POST_calls() {
     }
 
     @Test
-    public void should_save_condition_works() {
+    public void should_save_condition_works() throws MalformedURLException {
         assertFalse(cache.shouldSaveFormRedirectParameter(request));
+
         request.setPathInfo("/login.do");
         assertFalse(cache.shouldSaveFormRedirectParameter(request));
+
         request.setParameter(FORM_REDIRECT_PARAMETER, redirectUri);
+        request.setServerName(new URL(redirectUri).getHost());
         assertTrue(cache.shouldSaveFormRedirectParameter(request));
+
         request.setSession(session);
         assertTrue(cache.shouldSaveFormRedirectParameter(request));
+
         ClientRedirectSavedRequest savedRequest = new ClientRedirectSavedRequest(request, redirectUri);
         session.setAttribute(SAVED_REQUEST_SESSION_ATTRIBUTE, savedRequest);
         assertFalse(cache.shouldSaveFormRedirectParameter(request));
@@ -215,5 +230,16 @@ public void saved_request_matcher() {
 
     }
 
+    @Test
+    public void unapprovedFormRedirectRequestDoesNotSave() throws IOException, ServletException {
+        request.setPathInfo("/login.do");
+        request.setRequestURI("/login.do");
+        request.setMethod(HttpMethod.POST.name());
+        request.setParameter(FORM_REDIRECT_PARAMETER, "http://test.com");
+        request.setServerName("not-test.com");
+
+        spy.doFilter(request, new MockHttpServletResponse(), mock(FilterChain.class));
 
+        verify(spy, never()).saveClientRedirect(any(HttpServletRequest.class), anyString());
+    }
 }