From 8f6ddb0f171eb6011a44879a557c7932be857eb2 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Sun, 3 Oct 2010 23:57:04 +0100 Subject: [PATCH] SEC-1584: Backport to 2.0.x branch of request firewalling (normalization checks and path-parameter stripping from servletPath and pathInfo). --- core/pom.xml | 42 +-- .../firewall/DefaultHttpFirewall.java | 67 +++++ .../security/firewall/FirewalledRequest.java | 34 +++ .../security/firewall/HttpFirewall.java | 32 +++ .../firewall/RequestRejectedException.java | 10 + .../security/firewall/RequestWrapper.java | 98 +++++++ .../security/util/FilterChainProxy.java | 30 +- .../firewall/DefaultHttpFirewallTests.java | 44 +++ .../firewall/RequestWrapperTests.java | 62 ++++ .../util/FilterChainProxyConfigTests.java | 237 +++++++++++++++ .../security/util/FilterChainProxyTests.java | 272 +++++------------- pom.xml | 46 +-- 12 files changed, 726 insertions(+), 248 deletions(-) create mode 100644 core/src/main/java/org/springframework/security/firewall/DefaultHttpFirewall.java create mode 100644 core/src/main/java/org/springframework/security/firewall/FirewalledRequest.java create mode 100644 core/src/main/java/org/springframework/security/firewall/HttpFirewall.java create mode 100644 core/src/main/java/org/springframework/security/firewall/RequestRejectedException.java create mode 100644 core/src/main/java/org/springframework/security/firewall/RequestWrapper.java create mode 100644 core/src/test/java/org/springframework/security/firewall/DefaultHttpFirewallTests.java create mode 100644 core/src/test/java/org/springframework/security/firewall/RequestWrapperTests.java create mode 100644 core/src/test/java/org/springframework/security/util/FilterChainProxyConfigTests.java diff --git a/core/pom.xml b/core/pom.xml index 236a13d966..5cc7fb6e9e 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -47,27 +47,27 @@ spring-mock true - - org.aspectj - aspectjrt - true - - - org.aspectj - aspectjweaver - true - + + org.aspectj + aspectjrt + true + + + org.aspectj + aspectjweaver + true + org.springframework.ldap spring-ldap true - - cglib - cglib-nodep - test - true - + + cglib + cglib-nodep + test + true + net.sf.ehcache ehcache @@ -95,7 +95,7 @@ jaxen 1.1.1 true - + javax.servlet servlet-api @@ -135,13 +135,19 @@ 1.0.1 test + + org.mockito + mockito-all + 1.8.5 + test + log4j log4j true - + diff --git a/core/src/main/java/org/springframework/security/firewall/DefaultHttpFirewall.java b/core/src/main/java/org/springframework/security/firewall/DefaultHttpFirewall.java new file mode 100644 index 0000000000..1b8a806d17 --- /dev/null +++ b/core/src/main/java/org/springframework/security/firewall/DefaultHttpFirewall.java @@ -0,0 +1,67 @@ +package org.springframework.security.firewall; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +/** + * Default implementation which wraps requests in order to provide consistent values of the {@code servletPath} and + * {@code pathInfo}, which do not contain path parameters (as defined in + * RFC 2396). Different servlet containers + * interpret the servlet spec differently as to how path parameters are treated and it is possible they might be added + * in order to bypass particular security constraints. When using this implementation, they will be removed for all + * requests as the request passes through the security filter chain. Note that this means that any segments in the + * decoded path which contain a semi-colon, will have the part following the semi-colon removed for + * request matching. Your application should not contain any valid paths which contain semi-colons. + *

+ * If any un-normalized paths are found (containing directory-traversal character sequences), the request will be + * rejected immediately. Most containers normalize the paths before performing the servlet-mapping, but again this is + * not guaranteed by the servlet spec. + * + * @author Luke Taylor + */ +public class DefaultHttpFirewall implements HttpFirewall { + + public FirewalledRequest getFirewalledRequest(HttpServletRequest request) throws RequestRejectedException { + FirewalledRequest fwr = new RequestWrapper(request); + + if (!isNormalized(fwr.getServletPath()) || !isNormalized(fwr.getPathInfo())) { + throw new RequestRejectedException("Un-normalized paths are not supported: " + fwr.getServletPath() + + (fwr.getPathInfo() != null ? fwr.getPathInfo() : "")); + } + + return fwr; + } + + public HttpServletResponse getFirewalledResponse(HttpServletResponse response) { + return response; + } + + /** + * Checks whether a path is normalized (doesn't contain path traversal sequences like "./", "/../" or "/.") + * + * @param path the path to test + * @return true if the path doesn't contain any path-traversal character sequences. + */ + private boolean isNormalized(String path) { + if (path == null) { + return true; + } + + for (int j = path.length(); j > 0;) { + int i = path.lastIndexOf('/', j - 1); + int gap = j - i; + + if (gap == 2 && path.charAt(i+1) == '.') { + // ".", "/./" or "/." + return false; + } else if (gap == 3 && path.charAt(i+1) == '.'&& path.charAt(i+2) == '.') { + return false; + } + + j = i; + } + + return true; + } + +} diff --git a/core/src/main/java/org/springframework/security/firewall/FirewalledRequest.java b/core/src/main/java/org/springframework/security/firewall/FirewalledRequest.java new file mode 100644 index 0000000000..361127a8ea --- /dev/null +++ b/core/src/main/java/org/springframework/security/firewall/FirewalledRequest.java @@ -0,0 +1,34 @@ +package org.springframework.security.firewall; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletRequestWrapper; + +/** + * Request wrapper which is returned by the {@code HttpFirewall} interface. + *

+ * The only difference is the {@code reset} method which allows some + * or all of the state to be reset by the {@code FilterChainProxy} when the + * request leaves the security filter chain. + * + * @author Luke Taylor + */ +public abstract class FirewalledRequest extends HttpServletRequestWrapper { + /** + * Constructs a request object wrapping the given request. + * + * @throws IllegalArgumentException if the request is null + */ + public FirewalledRequest(HttpServletRequest request) { + super(request); + } + + /** + * This method will be called once the request has passed through the + * security filter chain, when it is about to proceed to the application + * proper. + *

+ * An implementation can thus choose to modify the state of the request + * for the security infrastructure, while still maintaining the + */ + public abstract void reset(); +} diff --git a/core/src/main/java/org/springframework/security/firewall/HttpFirewall.java b/core/src/main/java/org/springframework/security/firewall/HttpFirewall.java new file mode 100644 index 0000000000..1739dae357 --- /dev/null +++ b/core/src/main/java/org/springframework/security/firewall/HttpFirewall.java @@ -0,0 +1,32 @@ +package org.springframework.security.firewall; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +/** + * Interface which can be used to reject potentially dangerous requests and/or wrap them to + * control their behaviour. + *

+ * The implementation is injected into the {@code FilterChainProxy} and will be invoked before + * sending any request through the filter chain. It can also provide a response wrapper if the response + * behaviour should also be restricted. + * + * @author Luke Taylor + */ +public interface HttpFirewall { + + /** + * Provides the request object which will be passed through the filter chain. + * + * @throws RequestRejectedException if the request should be rejected immediately + */ + FirewalledRequest getFirewalledRequest(HttpServletRequest request) throws RequestRejectedException; + + /** + * Provides the response which will be passed through the filter chain. + * + * @param response the original response + * @return either the original response or a replacement/wrapper. + */ + HttpServletResponse getFirewalledResponse(HttpServletResponse response); +} diff --git a/core/src/main/java/org/springframework/security/firewall/RequestRejectedException.java b/core/src/main/java/org/springframework/security/firewall/RequestRejectedException.java new file mode 100644 index 0000000000..efaf882432 --- /dev/null +++ b/core/src/main/java/org/springframework/security/firewall/RequestRejectedException.java @@ -0,0 +1,10 @@ +package org.springframework.security.firewall; + +/** + * @author Luke Taylor + */ +public class RequestRejectedException extends RuntimeException { + public RequestRejectedException(String message) { + super(message); + } +} diff --git a/core/src/main/java/org/springframework/security/firewall/RequestWrapper.java b/core/src/main/java/org/springframework/security/firewall/RequestWrapper.java new file mode 100644 index 0000000000..352749a568 --- /dev/null +++ b/core/src/main/java/org/springframework/security/firewall/RequestWrapper.java @@ -0,0 +1,98 @@ +package org.springframework.security.firewall; + +import javax.servlet.http.HttpServletRequest; +import java.util.*; + +/** + * Request wrapper which ensures values of {@code servletPath} and {@code pathInfo} are returned which are suitable for + * pattern matching against. It strips out path parameters and extra consecutive '/' characters. + * + *

Path Parameters

+ * Parameters (as defined in RFC 2396) are stripped from the path + * segments of the {@code servletPath} and {@code pathInfo} values of the request. + *

+ * The parameter sequence is demarcated by a semi-colon, so each segment is checked for the occurrence of a ";" + * character and truncated at that point if it is present. + *

+ * The behaviour differs between servlet containers in how they interpret the servlet spec, which + * does not clearly state what the behaviour should be. For consistency, we make sure they are always removed, to + * avoid the risk of URL matching rules being bypassed by the malicious addition of parameters to the path component. + * + * @author Luke Taylor + */ +final class RequestWrapper extends FirewalledRequest { + private final String strippedServletPath; + private final String strippedPathInfo; + private boolean stripPaths = true; + + public RequestWrapper(HttpServletRequest request) { + super(request); + strippedServletPath = strip(request.getServletPath()); + String pathInfo = strip(request.getPathInfo()); + if (pathInfo != null && pathInfo.length() == 0) { + pathInfo = null; + } + strippedPathInfo = pathInfo; + } + + /** + * Removes path parameters from each path segment in the supplied path and truncates sequences of multiple '/' + * characters to a single '/'. + * + * @param path either the {@code servletPath} and {@code pathInfo} from the original request + * + * @return the supplied value, with path parameters removed and sequences of multiple '/' characters truncated, + * or null if the supplied path was null. + */ + private String strip(String path) { + if (path == null) { + return null; + } + + int scIndex = path.indexOf(';'); + + if (scIndex < 0) { + int doubleSlashIndex = path.indexOf("//"); + if (doubleSlashIndex < 0) { + // Most likely case, no parameters in any segment and no '//', so no stripping required + return path; + } + } + + StringTokenizer st = new StringTokenizer(path, "/"); + StringBuilder stripped = new StringBuilder(path.length()); + + if (path.charAt(0) == '/') { + stripped.append('/'); + } + + while(st.hasMoreTokens()) { + String segment = st.nextToken(); + scIndex = segment.indexOf(';'); + + if (scIndex >= 0) { + segment = segment.substring(0, scIndex); + } + stripped.append(segment).append('/'); + } + + // Remove the trailing slash if the original path didn't have one + if (path.charAt(path.length() - 1) != '/') { + stripped.deleteCharAt(stripped.length() - 1); + } + + return stripped.toString(); + } + + public String getPathInfo() { + return stripPaths ? strippedPathInfo : super.getPathInfo(); + } + + public String getServletPath() { + return stripPaths ? strippedServletPath : super.getServletPath(); + } + + public void reset() { + this.stripPaths = false; + } +} diff --git a/core/src/main/java/org/springframework/security/util/FilterChainProxy.java b/core/src/main/java/org/springframework/security/util/FilterChainProxy.java index f605cf9afa..8b05cd145a 100644 --- a/core/src/main/java/org/springframework/security/util/FilterChainProxy.java +++ b/core/src/main/java/org/springframework/security/util/FilterChainProxy.java @@ -21,11 +21,17 @@ import org.springframework.beans.BeansException; import org.springframework.beans.factory.InitializingBean; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; +import org.springframework.security.firewall.DefaultHttpFirewall; +import org.springframework.security.firewall.FirewalledRequest; +import org.springframework.security.firewall.HttpFirewall; import org.springframework.security.intercept.web.*; import org.springframework.util.Assert; import org.springframework.web.filter.DelegatingFilterProxy; import javax.servlet.*; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + import java.io.IOException; import java.util.*; @@ -34,7 +40,7 @@ import java.util.*; * Delegates Filter requests to a list of Spring-managed beans. * As of version 2.0, you shouldn't need to explicitly configure a FilterChainProxy bean in your application * context unless you need very fine control over the filter chain contents. Most cases should be adequately covered - * by the default <security:http /> namespace configuration options. + * by the default <security:http /> namespace configuration options. * *

The FilterChainProxy is loaded via a standard Spring {@link DelegatingFilterProxy} declaration in * web.xml. FilterChainProxy will then pass {@link #init(FilterConfig)}, {@link #destroy()} @@ -109,6 +115,7 @@ public class FilterChainProxy implements Filter, InitializingBean, ApplicationCo private UrlMatcher matcher = new AntUrlPathMatcher(); private boolean stripQueryStringFromUrls = true; private DefaultFilterInvocationDefinitionSource fids; + private HttpFirewall firewall = new DefaultHttpFirewall(); //~ Methods ======================================================================================================== @@ -154,10 +161,13 @@ public class FilterChainProxy implements Filter, InitializingBean, ApplicationCo } } - public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + public void doFilter(ServletRequest servletRequest, ServletResponse response, FilterChain chain) throws IOException, ServletException { - FilterInvocation fi = new FilterInvocation(request, response, chain); + FirewalledRequest fwRequest = firewall.getFirewalledRequest((HttpServletRequest) servletRequest); + HttpServletResponse fwResponse = firewall.getFirewalledResponse((HttpServletResponse) response); + + FilterInvocation fi = new FilterInvocation(fwRequest, fwResponse, chain); List filters = getFilters(fi.getRequestUrl()); if (filters == null || filters.size() == 0) { @@ -166,7 +176,7 @@ public class FilterChainProxy implements Filter, InitializingBean, ApplicationCo filters == null ? " has no matching filters" : " has an empty filter list"); } - chain.doFilter(request, response); + chain.doFilter(fwRequest, response); return; } @@ -374,6 +384,8 @@ public class FilterChainProxy implements Filter, InitializingBean, ApplicationCo logger.debug(fi.getRequestUrl() + " reached end of additional filter chain; proceeding with original chain"); } + // Deactivate path stripping as we exit the security filter chain + resetWrapper(request); fi.getChain().doFilter(request, response); } else { @@ -390,6 +402,16 @@ public class FilterChainProxy implements Filter, InitializingBean, ApplicationCo nextFilter.doFilter(request, response, this); } } + + private void resetWrapper(ServletRequest request) { + while (request instanceof ServletRequestWrapper) { + if (request instanceof FirewalledRequest) { + ((FirewalledRequest)request).reset(); + break; + } + request = ((ServletRequestWrapper)request).getRequest(); + } + } } } diff --git a/core/src/test/java/org/springframework/security/firewall/DefaultHttpFirewallTests.java b/core/src/test/java/org/springframework/security/firewall/DefaultHttpFirewallTests.java new file mode 100644 index 0000000000..f20225d9a7 --- /dev/null +++ b/core/src/test/java/org/springframework/security/firewall/DefaultHttpFirewallTests.java @@ -0,0 +1,44 @@ +package org.springframework.security.firewall; + +import static org.junit.Assert.fail; + +import org.junit.Test; +import org.springframework.mock.web.MockHttpServletRequest; + +/** + * @author Luke Taylor + */ +public class DefaultHttpFirewallTests { + public String[] unnormalizedPaths = { + "/..", + "/./path/", + "/path/path/.", + "/path/path//.", + "./path/../path//.", + "./path", + ".//path", + "." + }; + + @Test + public void unnormalizedPathsAreRejected() throws Exception { + DefaultHttpFirewall fw = new DefaultHttpFirewall(); + + MockHttpServletRequest request; + for (String path : unnormalizedPaths) { + request = new MockHttpServletRequest(); + request.setServletPath(path); + try { + fw.getFirewalledRequest(request); + fail(path + " is un-normalized"); + } catch (RequestRejectedException expected) { + } + request.setPathInfo(path); + try { + fw.getFirewalledRequest(request); + fail(path + " is un-normalized"); + } catch (RequestRejectedException expected) { + } + } + } +} diff --git a/core/src/test/java/org/springframework/security/firewall/RequestWrapperTests.java b/core/src/test/java/org/springframework/security/firewall/RequestWrapperTests.java new file mode 100644 index 0000000000..d8d117bf4e --- /dev/null +++ b/core/src/test/java/org/springframework/security/firewall/RequestWrapperTests.java @@ -0,0 +1,62 @@ +package org.springframework.security.firewall; + +import static org.junit.Assert.assertEquals; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.springframework.mock.web.MockHttpServletRequest; + +import java.util.*; + +/** + * @author Luke Taylor + */ +public class RequestWrapperTests { + private static Map testPaths = new LinkedHashMap(); + + @BeforeClass + // Some of these may be unrealistic values, but we can't be sure because of the + // inconsistency in the spec. + public static void createTestMap() { + testPaths.put("/path1;x=y;z=w/path2;x=y/path3;x=y", "/path1/path2/path3"); + testPaths.put("/path1;x=y/path2;x=y/", "/path1/path2/"); + testPaths.put("/path1//path2/", "/path1/path2/"); + testPaths.put("//path1/path2//", "/path1/path2/"); + testPaths.put(";x=y;z=w", ""); + } + + @Test + public void pathParametersAreRemovedFromServletPath() { + MockHttpServletRequest request = new MockHttpServletRequest(); + + for (Map.Entry entry : testPaths.entrySet()) { + String path = entry.getKey(); + String expectedResult = entry.getValue(); + request.setServletPath(path); + RequestWrapper wrapper = new RequestWrapper(request); + assertEquals(expectedResult, wrapper.getServletPath()); + wrapper.reset(); + assertEquals(path, wrapper.getServletPath()); + } + } + + @Test + public void pathParametersAreRemovedFromPathInfo() { + MockHttpServletRequest request = new MockHttpServletRequest(); + + for (Map.Entry entry : testPaths.entrySet()) { + String path = entry.getKey(); + String expectedResult = entry.getValue(); + // Should be null when stripped value is empty + if (expectedResult.length() == 0) { + expectedResult = null; + } + request.setPathInfo(path); + RequestWrapper wrapper = new RequestWrapper(request); + assertEquals(expectedResult, wrapper.getPathInfo()); + wrapper.reset(); + assertEquals(path, wrapper.getPathInfo()); + } + } + +} diff --git a/core/src/test/java/org/springframework/security/util/FilterChainProxyConfigTests.java b/core/src/test/java/org/springframework/security/util/FilterChainProxyConfigTests.java new file mode 100644 index 0000000000..f7909c1c74 --- /dev/null +++ b/core/src/test/java/org/springframework/security/util/FilterChainProxyConfigTests.java @@ -0,0 +1,237 @@ +/* Copyright 2004, 2005, 2006 Acegi Technology Pty Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.util; + +import org.springframework.security.ConfigAttribute; +import org.springframework.security.ConfigAttributeDefinition; +import org.springframework.security.MockFilterConfig; +import org.springframework.security.context.HttpSessionContextIntegrationFilter; +import org.springframework.security.intercept.web.MockFilterInvocationDefinitionSource; +import org.springframework.security.intercept.web.DefaultFilterInvocationDefinitionSource; +import org.springframework.security.intercept.web.RequestKey; +import org.springframework.security.ui.webapp.AuthenticationProcessingFilter; + +import org.springframework.beans.factory.BeanCreationException; +import org.springframework.context.support.ClassPathXmlApplicationContext; +import org.springframework.context.support.StaticApplicationContext; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; + +import org.junit.After; +import static org.junit.Assert.*; +import org.junit.Before; +import org.junit.Test; + +import java.util.LinkedHashMap; +import java.util.List; + +/** + * Tests {@link FilterChainProxy}. + * + * @author Carlos Sanchez + * @author Ben Alex + * @version $Id$ + */ +public class FilterChainProxyConfigTests { + private ClassPathXmlApplicationContext appCtx; + + //~ Methods ======================================================================================================== + + @Before + public void loadContext() { + appCtx = new ClassPathXmlApplicationContext("org/springframework/security/util/filtertest-valid.xml"); + } + + @After + public void closeContext() { + if (appCtx != null) { + appCtx.close(); + } + } + + @Test(expected=IllegalArgumentException.class) + public void testDetectsFilterInvocationDefinitionSourceThatDoesNotReturnAllConfigAttributes() throws Exception { + FilterChainProxy filterChainProxy = new FilterChainProxy(); + filterChainProxy.setApplicationContext(new StaticApplicationContext()); + + filterChainProxy.setFilterInvocationDefinitionSource(new MockFilterInvocationDefinitionSource(false, false)); + filterChainProxy.afterPropertiesSet(); + } + + @Test(expected=IllegalArgumentException.class) + public void testDetectsIfConfigAttributeDoesNotReturnValueForGetAttributeMethod() throws Exception { + FilterChainProxy filterChainProxy = new FilterChainProxy(); + filterChainProxy.setApplicationContext(new StaticApplicationContext()); + + ConfigAttributeDefinition cad = new ConfigAttributeDefinition(new MockConfigAttribute()); + + LinkedHashMap map = new LinkedHashMap(); + map.put(new RequestKey("/**"), cad); + DefaultFilterInvocationDefinitionSource fids = + new DefaultFilterInvocationDefinitionSource(new AntUrlPathMatcher(), map); + + filterChainProxy.setFilterInvocationDefinitionSource(fids); + + filterChainProxy.afterPropertiesSet(); + filterChainProxy.init(new MockFilterConfig()); + } + + @Test(expected = IllegalArgumentException.class) + public void testDetectsMissingFilterInvocationDefinitionSource() throws Exception { + FilterChainProxy filterChainProxy = new FilterChainProxy(); + filterChainProxy.setApplicationContext(new StaticApplicationContext()); + + filterChainProxy.afterPropertiesSet(); + } + + @Test + public void testDoNotFilter() throws Exception { + FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("filterChain", FilterChainProxy.class); + MockFilter filter = (MockFilter) appCtx.getBean("mockFilter", MockFilter.class); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setServletPath("/do/not/filter/somefile.html"); + + MockHttpServletResponse response = new MockHttpServletResponse(); + MockFilterChain chain = new MockFilterChain(true); + + filterChainProxy.doFilter(request, response, chain); + assertFalse(filter.isWasInitialized()); + assertFalse(filter.isWasDoFiltered()); + assertFalse(filter.isWasDestroyed()); + } + + @Test + public void misplacedUniversalPathShouldBeDetected() throws Exception { + try { + appCtx.getBean("newFilterChainProxyWrongPathOrder", FilterChainProxy.class); + fail("Expected BeanCreationException"); + } catch (BeanCreationException expected) { + } + } + + @Test + public void normalOperation() throws Exception { + FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("filterChain", FilterChainProxy.class); + doNormalOperation(filterChainProxy); + } + + @Test + public void proxyPathWithoutLowerCaseConversionShouldntMatchDifferentCasePath() throws Exception { + FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("filterChainNonLowerCase", FilterChainProxy.class); + assertNull(filterChainProxy.getFilters("/some/other/path/blah")); + } + + @Test + public void normalOperationWithNewConfig() throws Exception { + FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxy", FilterChainProxy.class); + checkPathAndFilterOrder(filterChainProxy); + doNormalOperation(filterChainProxy); + } + + @Test + public void normalOperationWithNewConfigRegex() throws Exception { + FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxyRegex", FilterChainProxy.class); + checkPathAndFilterOrder(filterChainProxy); + doNormalOperation(filterChainProxy); + } + + @Test + public void normalOperationWithNewConfigNonNamespace() throws Exception { + FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxyNonNamespace", FilterChainProxy.class); + checkPathAndFilterOrder(filterChainProxy); + doNormalOperation(filterChainProxy); + } + + @Test + public void pathWithNoMatchHasNoFilters() throws Exception { + FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxyNoDefaultPath", FilterChainProxy.class); + assertEquals(null, filterChainProxy.getFilters("/nomatch")); + } + + @Test + public void urlStrippingPropertyIsRespected() throws Exception { + FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxyNoDefaultPath", FilterChainProxy.class); + + // Should only match if we are stripping the query string + String url = "/blah.bar?x=something"; + assertNotNull(filterChainProxy.getFilters(url)); + assertEquals(2, filterChainProxy.getFilters(url).size()); + filterChainProxy.setStripQueryStringFromUrls(false); + assertNull(filterChainProxy.getFilters(url)); + } + + private void checkPathAndFilterOrder(FilterChainProxy filterChainProxy) throws Exception { + List filters = filterChainProxy.getFilters("/foo/blah"); + assertEquals(1, filters.size()); + assertTrue(filters.get(0) instanceof MockFilter); + + filters = filterChainProxy.getFilters("/some/other/path/blah"); + assertNotNull(filters); + assertEquals(3, filters.size()); + assertTrue(filters.get(0) instanceof HttpSessionContextIntegrationFilter); + assertTrue(filters.get(1) instanceof MockFilter); + assertTrue(filters.get(2) instanceof MockFilter); + + filters = filterChainProxy.getFilters("/do/not/filter"); + assertEquals(0, filters.size()); + + filters = filterChainProxy.getFilters("/another/nonspecificmatch"); + assertEquals(3, filters.size()); + assertTrue(filters.get(0) instanceof HttpSessionContextIntegrationFilter); + assertTrue(filters.get(1) instanceof AuthenticationProcessingFilter); + assertTrue(filters.get(2) instanceof MockFilter); + } + + private void doNormalOperation(FilterChainProxy filterChainProxy) throws Exception { + MockFilter filter = (MockFilter) appCtx.getBean("mockFilter", MockFilter.class); + assertFalse(filter.isWasInitialized()); + assertFalse(filter.isWasDoFiltered()); + assertFalse(filter.isWasDestroyed()); + + filterChainProxy.init(new MockFilterConfig()); + assertTrue(filter.isWasInitialized()); + assertFalse(filter.isWasDoFiltered()); + assertFalse(filter.isWasDestroyed()); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setServletPath("/foo/secure/super/somefile.html"); + + MockHttpServletResponse response = new MockHttpServletResponse(); + MockFilterChain chain = new MockFilterChain(true); + + filterChainProxy.doFilter(request, response, chain); + assertTrue(filter.isWasInitialized()); + assertTrue(filter.isWasDoFiltered()); + assertFalse(filter.isWasDestroyed()); + + request.setServletPath("/a/path/which/doesnt/match/any/filter.html"); + filterChainProxy.doFilter(request, response, chain); + + filterChainProxy.destroy(); + assertTrue(filter.isWasInitialized()); + assertTrue(filter.isWasDoFiltered()); + assertTrue(filter.isWasDestroyed()); + } + + //~ Inner Classes ================================================================================================== + + private class MockConfigAttribute implements ConfigAttribute { + public String getAttribute() { + return null; + } + } +} diff --git a/core/src/test/java/org/springframework/security/util/FilterChainProxyTests.java b/core/src/test/java/org/springframework/security/util/FilterChainProxyTests.java index 3d9842df13..5876c4c12e 100644 --- a/core/src/test/java/org/springframework/security/util/FilterChainProxyTests.java +++ b/core/src/test/java/org/springframework/security/util/FilterChainProxyTests.java @@ -1,237 +1,103 @@ -/* Copyright 2004, 2005, 2006 Acegi Technology Pty Limited - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - package org.springframework.security.util; -import org.springframework.security.ConfigAttribute; -import org.springframework.security.ConfigAttributeDefinition; -import org.springframework.security.MockFilterConfig; -import org.springframework.security.context.HttpSessionContextIntegrationFilter; -import org.springframework.security.intercept.web.MockFilterInvocationDefinitionSource; -import org.springframework.security.intercept.web.DefaultFilterInvocationDefinitionSource; -import org.springframework.security.intercept.web.RequestKey; -import org.springframework.security.ui.webapp.AuthenticationProcessingFilter; +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.*; -import org.springframework.beans.factory.BeanCreationException; -import org.springframework.context.support.ClassPathXmlApplicationContext; -import org.springframework.context.support.StaticApplicationContext; -import org.springframework.mock.web.MockHttpServletRequest; -import org.springframework.mock.web.MockHttpServletResponse; - -import org.junit.After; -import static org.junit.Assert.*; import org.junit.Before; import org.junit.Test; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.firewall.FirewalledRequest; -import java.util.LinkedHashMap; -import java.util.List; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import java.util.*; /** - * Tests {@link FilterChainProxy}. - * - * @author Carlos Sanchez - * @author Ben Alex - * @version $Id$ + * @author Luke Taylor */ +@SuppressWarnings({"unchecked"}) public class FilterChainProxyTests { - private ClassPathXmlApplicationContext appCtx; - - //~ Methods ======================================================================================================== + private FilterChainProxy fcp; + private MockHttpServletRequest request; + private MockHttpServletResponse response; + private FilterChain chain; + private Filter filter; @Before - public void loadContext() { - appCtx = new ClassPathXmlApplicationContext("org/springframework/security/util/filtertest-valid.xml"); - } - - @After - public void closeContext() { - if (appCtx != null) { - appCtx.close(); - } - } - - @Test(expected=IllegalArgumentException.class) - public void testDetectsFilterInvocationDefinitionSourceThatDoesNotReturnAllConfigAttributes() throws Exception { - FilterChainProxy filterChainProxy = new FilterChainProxy(); - filterChainProxy.setApplicationContext(new StaticApplicationContext()); - - filterChainProxy.setFilterInvocationDefinitionSource(new MockFilterInvocationDefinitionSource(false, false)); - filterChainProxy.afterPropertiesSet(); - } - - @Test(expected=IllegalArgumentException.class) - public void testDetectsIfConfigAttributeDoesNotReturnValueForGetAttributeMethod() throws Exception { - FilterChainProxy filterChainProxy = new FilterChainProxy(); - filterChainProxy.setApplicationContext(new StaticApplicationContext()); - - ConfigAttributeDefinition cad = new ConfigAttributeDefinition(new MockConfigAttribute()); - + public void setup() throws Exception { + fcp = new FilterChainProxy(); + filter = mock(Filter.class); + doAnswer(new Answer() { + public Object answer(InvocationOnMock inv) throws Throwable { + Object[] args = inv.getArguments(); + FilterChain fc = (FilterChain) args[2]; + fc.doFilter((HttpServletRequest) args[0], (HttpServletResponse) args[1]); + return null; + } + }).when(filter).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class), any(FilterChain.class)); LinkedHashMap map = new LinkedHashMap(); - map.put(new RequestKey("/**"), cad); - DefaultFilterInvocationDefinitionSource fids = - new DefaultFilterInvocationDefinitionSource(new AntUrlPathMatcher(), map); - - filterChainProxy.setFilterInvocationDefinitionSource(fids); - - filterChainProxy.afterPropertiesSet(); - filterChainProxy.init(new MockFilterConfig()); - } - - @Test(expected = IllegalArgumentException.class) - public void testDetectsMissingFilterInvocationDefinitionSource() throws Exception { - FilterChainProxy filterChainProxy = new FilterChainProxy(); - filterChainProxy.setApplicationContext(new StaticApplicationContext()); - - filterChainProxy.afterPropertiesSet(); + map.put("/match", Arrays.asList(filter)); + fcp.setFilterChainMap(map); + request = new MockHttpServletRequest(); + request.setServletPath("/match"); + response = new MockHttpServletResponse(); + chain = mock(FilterChain.class); } @Test - public void testDoNotFilter() throws Exception { - FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("filterChain", FilterChainProxy.class); - MockFilter filter = (MockFilter) appCtx.getBean("mockFilter", MockFilter.class); - - MockHttpServletRequest request = new MockHttpServletRequest(); - request.setServletPath("/do/not/filter/somefile.html"); - - MockHttpServletResponse response = new MockHttpServletResponse(); - MockFilterChain chain = new MockFilterChain(true); - - filterChainProxy.doFilter(request, response, chain); - assertFalse(filter.isWasInitialized()); - assertFalse(filter.isWasDoFiltered()); - assertFalse(filter.isWasDestroyed()); + public void toStringCallSucceeds() throws Exception { + fcp.afterPropertiesSet(); + fcp.toString(); } @Test - public void misplacedUniversalPathShouldBeDetected() throws Exception { - try { - appCtx.getBean("newFilterChainProxyWrongPathOrder", FilterChainProxy.class); - fail("Expected BeanCreationException"); - } catch (BeanCreationException expected) { - } + public void securityFilterChainIsNotInvokedIfMatchFails() throws Exception { + request.setServletPath("/nomatch"); + fcp.doFilter(request, response, chain); + assertEquals(1, fcp.getFilterChainMap().size()); + + verifyZeroInteractions(filter); + // The actual filter chain should be invoked though + verify(chain).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class)); } @Test - public void normalOperation() throws Exception { - FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("filterChain", FilterChainProxy.class); - doNormalOperation(filterChainProxy); + public void originalChainIsInvokedAfterSecurityChainIfMatchSucceeds() throws Exception { + fcp.doFilter(request, response, chain); + + verify(filter).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class), any(FilterChain.class)); + verify(chain).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class)); } @Test - public void proxyPathWithoutLowerCaseConversionShouldntMatchDifferentCasePath() throws Exception { - FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("filterChainNonLowerCase", FilterChainProxy.class); - assertNull(filterChainProxy.getFilters("/some/other/path/blah")); + public void originalFilterChainIsInvokedIfMatchingSecurityChainIsEmpty() throws Exception { + LinkedHashMap map = new LinkedHashMap(); + map.put("/match", Collections.emptyList()); + fcp.setFilterChainMap(map); + + fcp.doFilter(request, response, chain); + + verify(chain).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class)); } @Test - public void normalOperationWithNewConfig() throws Exception { - FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxy", FilterChainProxy.class); - checkPathAndFilterOrder(filterChainProxy); - doNormalOperation(filterChainProxy); + public void requestIsWrappedForFilteringWhenMatchIsFound() throws Exception { + fcp.doFilter(request, response, chain); + verify(filter).doFilter(any(FirewalledRequest.class), any(HttpServletResponse.class), any(FilterChain.class)); + verify(chain).doFilter(any(FirewalledRequest.class), any(HttpServletResponse.class)); } @Test - public void normalOperationWithNewConfigRegex() throws Exception { - FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxyRegex", FilterChainProxy.class); - checkPathAndFilterOrder(filterChainProxy); - doNormalOperation(filterChainProxy); + public void requestIsWrappedForFilteringWhenMatchIsNotFound() throws Exception { + request.setServletPath("/nomatch"); + fcp.doFilter(request, response, chain); + verifyZeroInteractions(filter); + verify(chain).doFilter(any(FirewalledRequest.class), any(HttpServletResponse.class)); } - @Test - public void normalOperationWithNewConfigNonNamespace() throws Exception { - FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxyNonNamespace", FilterChainProxy.class); - checkPathAndFilterOrder(filterChainProxy); - doNormalOperation(filterChainProxy); - } - - @Test - public void pathWithNoMatchHasNoFilters() throws Exception { - FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxyNoDefaultPath", FilterChainProxy.class); - assertEquals(null, filterChainProxy.getFilters("/nomatch")); - } - - @Test - public void urlStrippingPropertyIsRespected() throws Exception { - FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxyNoDefaultPath", FilterChainProxy.class); - - // Should only match if we are stripping the query string - String url = "/blah.bar?x=something"; - assertNotNull(filterChainProxy.getFilters(url)); - assertEquals(2, filterChainProxy.getFilters(url).size()); - filterChainProxy.setStripQueryStringFromUrls(false); - assertNull(filterChainProxy.getFilters(url)); - } - - private void checkPathAndFilterOrder(FilterChainProxy filterChainProxy) throws Exception { - List filters = filterChainProxy.getFilters("/foo/blah"); - assertEquals(1, filters.size()); - assertTrue(filters.get(0) instanceof MockFilter); - - filters = filterChainProxy.getFilters("/some/other/path/blah"); - assertNotNull(filters); - assertEquals(3, filters.size()); - assertTrue(filters.get(0) instanceof HttpSessionContextIntegrationFilter); - assertTrue(filters.get(1) instanceof MockFilter); - assertTrue(filters.get(2) instanceof MockFilter); - - filters = filterChainProxy.getFilters("/do/not/filter"); - assertEquals(0, filters.size()); - - filters = filterChainProxy.getFilters("/another/nonspecificmatch"); - assertEquals(3, filters.size()); - assertTrue(filters.get(0) instanceof HttpSessionContextIntegrationFilter); - assertTrue(filters.get(1) instanceof AuthenticationProcessingFilter); - assertTrue(filters.get(2) instanceof MockFilter); - } - - private void doNormalOperation(FilterChainProxy filterChainProxy) throws Exception { - MockFilter filter = (MockFilter) appCtx.getBean("mockFilter", MockFilter.class); - assertFalse(filter.isWasInitialized()); - assertFalse(filter.isWasDoFiltered()); - assertFalse(filter.isWasDestroyed()); - - filterChainProxy.init(new MockFilterConfig()); - assertTrue(filter.isWasInitialized()); - assertFalse(filter.isWasDoFiltered()); - assertFalse(filter.isWasDestroyed()); - - MockHttpServletRequest request = new MockHttpServletRequest(); - request.setServletPath("/foo/secure/super/somefile.html"); - - MockHttpServletResponse response = new MockHttpServletResponse(); - MockFilterChain chain = new MockFilterChain(true); - - filterChainProxy.doFilter(request, response, chain); - assertTrue(filter.isWasInitialized()); - assertTrue(filter.isWasDoFiltered()); - assertFalse(filter.isWasDestroyed()); - - request.setServletPath("/a/path/which/doesnt/match/any/filter.html"); - filterChainProxy.doFilter(request, response, chain); - - filterChainProxy.destroy(); - assertTrue(filter.isWasInitialized()); - assertTrue(filter.isWasDoFiltered()); - assertTrue(filter.isWasDestroyed()); - } - - //~ Inner Classes ================================================================================================== - - private class MockConfigAttribute implements ConfigAttribute { - public String getAttribute() { - return null; - } - } } diff --git a/pom.xml b/pom.xml index 5e4ff91925..f96d24dd49 100644 --- a/pom.xml +++ b/pom.xml @@ -8,8 +8,8 @@ pom - core - core-tiger + core + core-tiger portlet ntlm @@ -43,9 +43,9 @@ bamboo https://build.springframework.org/browse/SEC - + - + spring-release Spring Release Repository @@ -177,10 +177,10 @@ Ruud Senden - + Michael Mayr - + @@ -205,7 +205,7 @@ com.springsource.bundlor com.springsource.bundlor.maven - 1.0.0.M5 + 1.0.0.M6 bundlor @@ -218,7 +218,7 @@ - + org.apache.maven.plugins maven-help-plugin @@ -435,11 +435,11 @@ org.apache.maven.plugins maven-surefire-report-plugin 2.4.2 - + +--> org.apache.maven.plugins @@ -449,7 +449,7 @@ bigbank/** - + - + org.codehaus.mojo @@ -530,7 +530,7 @@ org.apache.maven.plugins maven-project-info-reports-plugin 2.0.1 - - +--> + @@ -604,7 +604,7 @@ aspectjweaver true 1.5.4 - + org.aspectj aspectjrt @@ -613,15 +613,15 @@ org.springframework spring-webmvc - ${spring.version} - + ${spring.version} + cglib cglib-nodep test true 2.1_3 - + log4j log4j @@ -679,7 +679,7 @@ 1.1.2 ${basedir}/src/docbkx - ${basedir}/target/site/guide + ${basedir}/target/site/guide