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

Jaskanwal Pawar · Jaskanwal Pawar · commit d17b23fc3bf9 · 2018-06-14T16:03:02.000-07:00
- A valid/approved form_redirect_uri value has the same host as the receiving UAA server. [#158222161] https://www.pivotaltracker.com/story/show/158222161
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
@@ -219,4 +219,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/server/src/main/java/org/cloudfoundry/identity/uaa/web/UaaSavedRequestAwareAuthenticationSuccessHandler.java b/server/src/main/java/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
@@ -32,8 +32,10 @@
 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 {
@@ -360,6 +362,19 @@ public void test_validate_invalid_redirect_uri() {
         validateRedirectUri(convertToHttps(invalidHttpWildCardUrls), false);
     }
 
+    @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());
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -92,7 +92,9 @@ protected boolean shouldSaveFormRedirectParameter(HttpServletRequest request) {`
`92`	`92`	`if (StringUtils.isEmpty(formRedirect)) {`
`93`	`93`	`return false;`
`94`	`94`	`}`
`95`		`-`
	`95`	`+ if (!UaaUrlUtils.uriHasMatchingHost(formRedirect, request.getServerName())) {`
	`96`	`+ return false;`
	`97`	`+ }`
`96`	`98`	`if (hasSavedRequest(request)) {`
`97`	`99`	`return false;`
`98`	`100`	`}`