SEC-297: Stop prepending of context path to full url default targets. Also added more stringent checks on format of injected defaultTargetUrl property.

core/src/main/java/org/acegisecurity/ui/AbstractProcessingFilter.java

@@ -313,13 +313,13 @@ public abstract class AbstractProcessingFilter implements Filter, InitializingBe
         return uri.endsWith(request.getContextPath() + filterProcessesUrl);
     }
 
-    protected void sendRedirect(HttpServletRequest request, HttpServletResponse response, String failureUrl)
+    protected void sendRedirect(HttpServletRequest request, HttpServletResponse response, String url)
         throws IOException {
-        if (!failureUrl.startsWith("http://") && !failureUrl.startsWith("https://")) {
-            failureUrl = request.getContextPath() + failureUrl;
+        if (!url.startsWith("http://") && !url.startsWith("https://")) {
+            url = request.getContextPath() + url;
         }
 
-        response.sendRedirect(response.encodeRedirectURL(failureUrl));
+        response.sendRedirect(response.encodeRedirectURL(url));
     }
 
     public void setAlwaysUseDefaultTargetUrl(boolean alwaysUseDefaultTargetUrl) {
@@ -348,6 +348,8 @@ public abstract class AbstractProcessingFilter implements Filter, InitializingBe
     }
 
     public void setDefaultTargetUrl(String defaultTargetUrl) {
+        Assert.isTrue(defaultTargetUrl.startsWith("/") | defaultTargetUrl.startsWith("http"),
+                "defaultTarget must start with '/' or with 'http(s)'");
         this.defaultTargetUrl = defaultTargetUrl;
     }
 
@@ -379,14 +381,11 @@ public abstract class AbstractProcessingFilter implements Filter, InitializingBe
             logger.debug("Updated SecurityContextHolder to contain the following Authentication: '" + authResult + "'");
         }
 
-        String targetUrl = obtainFullRequestUrl(request);
-
-        if (alwaysUseDefaultTargetUrl) {
-            targetUrl = null;
-        }
+        // Don't attempt to obtain the url from the saved request if alwaysUsedefaultTargetUrl is set
+        String targetUrl = alwaysUseDefaultTargetUrl ? null : obtainFullRequestUrl(request);
 
         if (targetUrl == null) {
-            targetUrl = request.getContextPath() + getDefaultTargetUrl();
+            targetUrl = getDefaultTargetUrl();
         }
 
         if (logger.isDebugEnabled()) {
@@ -402,7 +401,7 @@ public abstract class AbstractProcessingFilter implements Filter, InitializingBe
             eventPublisher.publishEvent(new InteractiveAuthenticationSuccessEvent(authResult, this.getClass()));
         }
 
-        response.sendRedirect(response.encodeRedirectURL(targetUrl));
+        sendRedirect(request, response, targetUrl);
     }
 
     protected void unsuccessfulAuthentication(HttpServletRequest request, HttpServletResponse response,

core/src/test/java/org/acegisecurity/ui/AbstractProcessingFilterTests.java

@@ -70,6 +70,7 @@ public class AbstractProcessingFilterTests extends TestCase {
         request.setScheme("http");
         request.setServerName("www.example.com");
         request.setRequestURI("/mycontext/j_mock_post");
+        request.setContextPath("/mycontext");
 
         return request;
     }
@@ -154,27 +155,27 @@ public class AbstractProcessingFilterTests extends TestCase {
 
         // Setup our test object, to deny access
         MockAbstractProcessingFilter filter = new MockAbstractProcessingFilter(false);
-        filter.setAuthenticationFailureUrl("/myApp/failed.jsp");
+        filter.setAuthenticationFailureUrl("/failed.jsp");
 
         // Test
         executeFilterInContainerSimulator(config, filter, request, response, chain);
 
-        assertEquals("/myApp/failed.jsp", response.getRedirectedUrl());
+        assertEquals("/mycontext/failed.jsp", response.getRedirectedUrl());
         assertNull(SecurityContextHolder.getContext().getAuthentication());
 
         //Prepare again, this time using the exception mapping
         filter = new MockAbstractProcessingFilter(new AccountExpiredException("You're account is expired"));
-        filter.setAuthenticationFailureUrl("/myApp/failed.jsp");
+        filter.setAuthenticationFailureUrl("/failed.jsp");
 
         Properties exceptionMappings = filter.getExceptionMappings();
-        exceptionMappings.setProperty(AccountExpiredException.class.getName(), "/myApp/accountExpired.jsp");
+        exceptionMappings.setProperty(AccountExpiredException.class.getName(), "/accountExpired.jsp");
         filter.setExceptionMappings(exceptionMappings);
         response = new MockHttpServletResponse();
 
         // Test
         executeFilterInContainerSimulator(config, filter, request, response, chain);
 
-        assertEquals("/myApp/accountExpired.jsp", response.getRedirectedUrl());
+        assertEquals("/mycontext/accountExpired.jsp", response.getRedirectedUrl());
         assertNull(SecurityContextHolder.getContext().getAuthentication());
     }
 
@@ -199,7 +200,7 @@ public class AbstractProcessingFilterTests extends TestCase {
 
         // Test
         executeFilterInContainerSimulator(config, filter, request, response, chain);
-        assertEquals("/logged_in.jsp", response.getRedirectedUrl());
+        assertEquals("/mycontext/logged_in.jsp", response.getRedirectedUrl());
         assertNotNull(SecurityContextHolder.getContext().getAuthentication());
         assertEquals("test", SecurityContextHolder.getContext().getAuthentication().getPrincipal().toString());
     }
@@ -226,6 +227,19 @@ public class AbstractProcessingFilterTests extends TestCase {
         assertEquals("/fail", filter.getAuthenticationFailureUrl());
     }
 
+    public void testDefaultUrlMuststartWithSlashOrHttpScheme() {
+        AbstractProcessingFilter filter = new MockAbstractProcessingFilter();
+
+        filter.setDefaultTargetUrl("/acceptableRelativeUrl");
+        filter.setDefaultTargetUrl("http://some.site.org/index.html");
+        filter.setDefaultTargetUrl("https://some.site.org/index.html");
+
+        try {
+            filter.setDefaultTargetUrl("missingSlash");
+            fail("Shouldn't accept default target without leading slash");
+        } catch (IllegalArgumentException expected) {}
+    }
+
     public void testIgnoresAnyServletPathOtherThanFilterProcessesUrl()
         throws Exception {
         // Setup our HTTP request
@@ -269,7 +283,7 @@ public class AbstractProcessingFilterTests extends TestCase {
 
         // Test
         executeFilterInContainerSimulator(config, filter, request, response, chain);
-        assertEquals("/logged_in.jsp", response.getRedirectedUrl());
+        assertEquals("/mycontext/logged_in.jsp", response.getRedirectedUrl());
         assertNotNull(SecurityContextHolder.getContext().getAuthentication());
         assertEquals("test", SecurityContextHolder.getContext().getAuthentication().getPrincipal().toString());
     }
@@ -354,7 +368,7 @@ public class AbstractProcessingFilterTests extends TestCase {
 
         // Test
         executeFilterInContainerSimulator(config, filter, request, response, chain);
-        assertEquals("/logged_in.jsp", response.getRedirectedUrl());
+        assertEquals("/mycontext/logged_in.jsp", response.getRedirectedUrl());
         assertNotNull(SecurityContextHolder.getContext().getAuthentication());
         assertEquals("test", SecurityContextHolder.getContext().getAuthentication().getPrincipal().toString());
 
@@ -397,7 +411,7 @@ public class AbstractProcessingFilterTests extends TestCase {
 
         // Test
         executeFilterInContainerSimulator(config, filter, request, response, chain);
-        assertEquals("/foobar", response.getRedirectedUrl());
+        assertEquals("/mycontext/foobar", response.getRedirectedUrl());
         assertNotNull(SecurityContextHolder.getContext().getAuthentication());
     }
 
@@ -424,6 +438,27 @@ public class AbstractProcessingFilterTests extends TestCase {
         assertNotNull(SecurityContextHolder.getContext().getAuthentication());
     }
 
+    /**
+     * SEC-297 fix. 
+     */
+    public void testFullDefaultTargetUrlDoesNotHaveContextPathPrepended() throws Exception {
+        MockHttpServletRequest request = createMockRequest();
+        MockFilterConfig config = new MockFilterConfig(null, null);
+
+        MockFilterChain chain = new MockFilterChain(true);
+        MockHttpServletResponse response = new MockHttpServletResponse();
+
+        // Setup our test object, to grant access
+        MockAbstractProcessingFilter filter = new MockAbstractProcessingFilter(true);
+        filter.setFilterProcessesUrl("/j_mock_post");
+        filter.setDefaultTargetUrl("http://monkeymachine.co.uk/");
+        filter.setAlwaysUseDefaultTargetUrl(true);
+
+        executeFilterInContainerSimulator(config, filter, request, response, chain);
+        assertEquals("http://monkeymachine.co.uk/", response.getRedirectedUrl());
+        assertNotNull(SecurityContextHolder.getContext().getAuthentication());
+    }
+
     //~ Inner Classes ==================================================================================================
 
     private class MockAbstractProcessingFilter extends AbstractProcessingFilter {

core/src/test/java/org/acegisecurity/ui/webapp/SiteminderAuthenticationProcessingFilterTests.java

@@ -86,8 +86,8 @@ public class SiteminderAuthenticationProcessingFilterTests extends TestCase {
         filter.setContinueChainBeforeSuccessfulAuthentication(true);
         assertTrue(filter.isContinueChainBeforeSuccessfulAuthentication());
 
-        filter.setDefaultTargetUrl("bar");
-        assertEquals("bar", filter.getDefaultTargetUrl());
+        filter.setDefaultTargetUrl("/bar");
+        assertEquals("/bar", filter.getDefaultTargetUrl());
 
         filter.setFilterProcessesUrl("foobar");
         assertEquals("foobar", filter.getFilterProcessesUrl());