From 8ad2d681ab8e952b9629aa68f9a31c9a2a1087e9 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Wed, 7 May 2008 13:49:20 +0000 Subject: [PATCH] SEC-818: Changed redirect URL validation to ignore potential property placeholders at parsing time and report a warning through the parser context rather than an error. Also validated the URLs in the beans themselves using Asserts, so an exception will occur later when the beans have been created rather than while assembling the bean definitions. --- .../security/concurrent/ConcurrentSessionFilter.java | 2 ++ .../springframework/security/config/ConfigUtils.java | 7 ++++--- .../security/ui/AbstractProcessingFilter.java | 8 ++++---- .../security/ui/AccessDeniedHandlerImpl.java | 4 ++-- .../security/ui/logout/LogoutFilter.java | 3 +++ .../org/springframework/security/util/UrlUtils.java | 5 +++++ .../config/HttpSecurityBeanDefinitionParserTests.java | 11 ++++++----- 7 files changed, 26 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/springframework/security/concurrent/ConcurrentSessionFilter.java b/core/src/main/java/org/springframework/security/concurrent/ConcurrentSessionFilter.java index 3f96121660..e4ef0f8570 100644 --- a/core/src/main/java/org/springframework/security/concurrent/ConcurrentSessionFilter.java +++ b/core/src/main/java/org/springframework/security/concurrent/ConcurrentSessionFilter.java @@ -21,6 +21,7 @@ import org.springframework.security.ui.FilterChainOrder; import org.springframework.security.ui.SpringSecurityFilter; import org.springframework.security.ui.logout.LogoutHandler; import org.springframework.security.ui.logout.SecurityContextLogoutHandler; +import org.springframework.security.util.UrlUtils; import org.springframework.beans.factory.InitializingBean; import org.springframework.util.Assert; @@ -60,6 +61,7 @@ public class ConcurrentSessionFilter extends SpringSecurityFilter implements Ini public void afterPropertiesSet() throws Exception { Assert.notNull(sessionRegistry, "SessionRegistry required"); + Assert.isTrue(UrlUtils.isValidRedirectUrl(expiredUrl), expiredUrl + " isn't a valid redirect URL"); } public void doFilterHttp(HttpServletRequest request, HttpServletResponse response, FilterChain chain) diff --git a/core/src/main/java/org/springframework/security/config/ConfigUtils.java b/core/src/main/java/org/springframework/security/config/ConfigUtils.java index 6566c95f4b..c40fecefa8 100644 --- a/core/src/main/java/org/springframework/security/config/ConfigUtils.java +++ b/core/src/main/java/org/springframework/security/config/ConfigUtils.java @@ -16,6 +16,7 @@ import org.springframework.beans.factory.xml.ParserContext; import org.springframework.security.afterinvocation.AfterInvocationProviderManager; import org.springframework.security.providers.ProviderManager; import org.springframework.security.userdetails.UserDetailsService; +import org.springframework.security.util.UrlUtils; import org.springframework.security.vote.AffirmativeBased; import org.springframework.security.vote.AuthenticatedVoter; import org.springframework.security.vote.RoleVoter; @@ -180,12 +181,12 @@ public abstract class ConfigUtils { /** * Checks the value of an XML attribute which represents a redirect URL. - * If not empty or starting with "/" or "http" it will raise an error. + * If not empty or starting with "$" (potential placeholder), "/" or "http" it will raise an error. */ static void validateHttpRedirect(String url, ParserContext pc, Object source) { - if (!StringUtils.hasText(url) || url.startsWith("/") || url.toLowerCase().startsWith("http")) { + if (UrlUtils.isValidRedirectUrl(url) || url.startsWith("$")) { return; } - pc.getReaderContext().error(url + " is not a valid redirect URL (must start with '/' or http(s))", source); + pc.getReaderContext().warning(url + " is not a valid redirect URL (must start with '/' or http(s))", source); } } diff --git a/core/src/main/java/org/springframework/security/ui/AbstractProcessingFilter.java b/core/src/main/java/org/springframework/security/ui/AbstractProcessingFilter.java index 0cd54de647..855db390fe 100644 --- a/core/src/main/java/org/springframework/security/ui/AbstractProcessingFilter.java +++ b/core/src/main/java/org/springframework/security/ui/AbstractProcessingFilter.java @@ -21,6 +21,7 @@ import org.springframework.security.AuthenticationException; import org.springframework.security.AuthenticationManager; import org.springframework.security.util.RedirectUtils; import org.springframework.security.util.SessionUtils; +import org.springframework.security.util.UrlUtils; import org.springframework.security.context.SecurityContextHolder; @@ -43,10 +44,6 @@ import org.springframework.util.Assert; import java.io.IOException; import java.util.Properties; -import java.util.Enumeration; -import java.util.HashMap; -import java.util.Iterator; -import java.util.Map; import javax.servlet.FilterChain; import javax.servlet.ServletException; @@ -215,8 +212,11 @@ public abstract class AbstractProcessingFilter extends SpringSecurityFilter impl public void afterPropertiesSet() throws Exception { Assert.hasLength(filterProcessesUrl, "filterProcessesUrl must be specified"); + Assert.isTrue(UrlUtils.isValidRedirectUrl(filterProcessesUrl), filterProcessesUrl + " isn't a valid redirect URL"); Assert.hasLength(defaultTargetUrl, "defaultTargetUrl must be specified"); + Assert.isTrue(UrlUtils.isValidRedirectUrl(defaultTargetUrl), defaultTargetUrl + " isn't a valid redirect URL"); // Assert.hasLength(authenticationFailureUrl, "authenticationFailureUrl must be specified"); + Assert.isTrue(UrlUtils.isValidRedirectUrl(authenticationFailureUrl), authenticationFailureUrl + " isn't a valid redirect URL"); Assert.notNull(authenticationManager, "authenticationManager must be specified"); Assert.notNull(rememberMeServices, "rememberMeServices cannot be null"); Assert.notNull(targetUrlResolver, "targetUrlResolver cannot be null"); diff --git a/core/src/main/java/org/springframework/security/ui/AccessDeniedHandlerImpl.java b/core/src/main/java/org/springframework/security/ui/AccessDeniedHandlerImpl.java index 3d9213b298..42085462b3 100644 --- a/core/src/main/java/org/springframework/security/ui/AccessDeniedHandlerImpl.java +++ b/core/src/main/java/org/springframework/security/ui/AccessDeniedHandlerImpl.java @@ -54,7 +54,7 @@ public class AccessDeniedHandlerImpl implements AccessDeniedHandler { //~ Methods ======================================================================================================== public void handle(ServletRequest request, ServletResponse response, AccessDeniedException accessDeniedException) - throws IOException, ServletException { + throws IOException, ServletException { if (errorPage != null) { // Put exception into request scope (perhaps of use to a view) ((HttpServletRequest) request).setAttribute(SPRING_SECURITY_ACCESS_DENIED_EXCEPTION_KEY, @@ -80,7 +80,7 @@ public class AccessDeniedHandlerImpl implements AccessDeniedHandler { */ public void setErrorPage(String errorPage) { if ((errorPage != null) && !errorPage.startsWith("/")) { - throw new IllegalArgumentException("ErrorPage must begin with '/'"); + throw new IllegalArgumentException("errorPage must begin with '/'"); } this.errorPage = errorPage; diff --git a/core/src/main/java/org/springframework/security/ui/logout/LogoutFilter.java b/core/src/main/java/org/springframework/security/ui/logout/LogoutFilter.java index 5502f8a618..846b52eea3 100644 --- a/core/src/main/java/org/springframework/security/ui/logout/LogoutFilter.java +++ b/core/src/main/java/org/springframework/security/ui/logout/LogoutFilter.java @@ -26,6 +26,7 @@ import org.springframework.security.Authentication; import org.springframework.security.ui.SpringSecurityFilter; import org.springframework.security.ui.FilterChainOrder; import org.springframework.security.util.RedirectUtils; +import org.springframework.security.util.UrlUtils; import org.springframework.security.context.SecurityContextHolder; import org.springframework.util.Assert; import org.springframework.util.StringUtils; @@ -58,6 +59,7 @@ public class LogoutFilter extends SpringSecurityFilter { public LogoutFilter(String logoutSuccessUrl, LogoutHandler[] handlers) { Assert.notEmpty(handlers, "LogoutHandlers are required"); this.logoutSuccessUrl = logoutSuccessUrl; + Assert.isTrue(UrlUtils.isValidRedirectUrl(logoutSuccessUrl), logoutSuccessUrl + " isn't a valid redirect URL"); this.handlers = handlers; } @@ -161,6 +163,7 @@ public class LogoutFilter extends SpringSecurityFilter { public void setFilterProcessesUrl(String filterProcessesUrl) { Assert.hasText(filterProcessesUrl, "FilterProcessesUrl required"); + Assert.isTrue(UrlUtils.isValidRedirectUrl(filterProcessesUrl), filterProcessesUrl + " isn't a valid redirect URL"); this.filterProcessesUrl = filterProcessesUrl; } diff --git a/core/src/main/java/org/springframework/security/util/UrlUtils.java b/core/src/main/java/org/springframework/security/util/UrlUtils.java index 9e2c65de07..ff97935aed 100644 --- a/core/src/main/java/org/springframework/security/util/UrlUtils.java +++ b/core/src/main/java/org/springframework/security/util/UrlUtils.java @@ -18,6 +18,7 @@ package org.springframework.security.util; import org.springframework.security.intercept.web.FilterInvocation; import org.springframework.security.ui.savedrequest.SavedRequest; +import org.springframework.util.StringUtils; import javax.servlet.http.HttpServletRequest; @@ -117,4 +118,8 @@ public final class UrlUtils { return buildRequestUrl(sr.getServletPath(), sr.getRequestURI(), sr.getContextPath(), sr.getPathInfo(), sr.getQueryString()); } + + public static boolean isValidRedirectUrl(String url) { + return !StringUtils.hasText(url) || url.startsWith("/") || url.toLowerCase().startsWith("http"); + } } diff --git a/core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java b/core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java index 5b8180d602..3692de7776 100644 --- a/core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java +++ b/core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java @@ -9,6 +9,7 @@ import java.util.List; import org.junit.After; import org.junit.Test; import org.springframework.beans.factory.BeanCreationException; +import org.springframework.beans.factory.BeanDefinitionStoreException; import org.springframework.beans.factory.parsing.BeanDefinitionParsingException; import org.springframework.context.support.AbstractXmlApplicationContext; import org.springframework.mock.web.MockHttpServletRequest; @@ -174,7 +175,7 @@ public class HttpSecurityBeanDefinitionParserTests { assertEquals(Boolean.TRUE, FieldUtils.getFieldValue(filter, "alwaysUseDefaultTargetUrl")); } - @Test(expected=BeanDefinitionParsingException.class) + @Test(expected=BeanCreationException.class) public void invalidLoginPageIsDetected() throws Exception { setContext( "" + @@ -182,7 +183,7 @@ public class HttpSecurityBeanDefinitionParserTests { "" + AUTH_PROVIDER_XML); } - @Test(expected=BeanDefinitionParsingException.class) + @Test(expected=BeanCreationException.class) public void invalidDefaultTargetUrlIsDetected() throws Exception { setContext( "" + @@ -190,7 +191,7 @@ public class HttpSecurityBeanDefinitionParserTests { "" + AUTH_PROVIDER_XML); } - @Test(expected=BeanDefinitionParsingException.class) + @Test(expected=BeanCreationException.class) public void invalidLogoutUrlIsDetected() throws Exception { setContext( "" + @@ -199,7 +200,7 @@ public class HttpSecurityBeanDefinitionParserTests { "" + AUTH_PROVIDER_XML); } - @Test(expected=BeanDefinitionParsingException.class) + @Test(expected=BeanCreationException.class) public void invalidLogoutSuccessUrlIsDetected() throws Exception { setContext( "" + @@ -265,7 +266,7 @@ public class HttpSecurityBeanDefinitionParserTests { assertEquals("/access-denied", FieldUtils.getFieldValue(etf, "accessDeniedHandler.errorPage")); } - @Test(expected=BeanDefinitionParsingException.class) + @Test(expected=BeanDefinitionStoreException.class) public void invalidAccessDeniedUrlIsDetected() throws Exception { setContext("" + AUTH_PROVIDER_XML); }