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); }