From ae55e0452252380fbbe0a8fca8afa4c0dbf66e5d Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Tue, 27 Jun 2006 23:26:25 +0000 Subject: [PATCH] SEC-297: Stop prepending of context path to full url default targets. Also added more stringent checks on format of injected defaultTargetUrl property. --- .../ui/AbstractProcessingFilter.java | 21 ++++---- .../ui/AbstractProcessingFilterTests.java | 53 +++++++++++++++---- ...erAuthenticationProcessingFilterTests.java | 4 +- 3 files changed, 56 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/org/acegisecurity/ui/AbstractProcessingFilter.java b/core/src/main/java/org/acegisecurity/ui/AbstractProcessingFilter.java index b48e71aaa5..be0bd48e5e 100644 --- a/core/src/main/java/org/acegisecurity/ui/AbstractProcessingFilter.java +++ b/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, diff --git a/core/src/test/java/org/acegisecurity/ui/AbstractProcessingFilterTests.java b/core/src/test/java/org/acegisecurity/ui/AbstractProcessingFilterTests.java index 88d348e22c..1001d20548 100644 --- a/core/src/test/java/org/acegisecurity/ui/AbstractProcessingFilterTests.java +++ b/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 { diff --git a/core/src/test/java/org/acegisecurity/ui/webapp/SiteminderAuthenticationProcessingFilterTests.java b/core/src/test/java/org/acegisecurity/ui/webapp/SiteminderAuthenticationProcessingFilterTests.java index 094555b547..88db18a340 100644 --- a/core/src/test/java/org/acegisecurity/ui/webapp/SiteminderAuthenticationProcessingFilterTests.java +++ b/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());