SEC-1584: Backport to 2.0.x branch of request firewalling (normalization checks and path-parameter stripping from servletPath and pathInfo).

core/pom.xml

@@ -47,27 +47,27 @@
             <artifactId>spring-mock</artifactId>
             <optional>true</optional>
         </dependency>
-	    <dependency>
-	        <groupId>org.aspectj</groupId>
-		<artifactId>aspectjrt</artifactId>
-		<optional>true</optional>
-	    </dependency>
-	    <dependency>
-	        <groupId>org.aspectj</groupId>
-		<artifactId>aspectjweaver</artifactId>
-		<optional>true</optional>
-	    </dependency>	    
+        <dependency>
+            <groupId>org.aspectj</groupId>
+        <artifactId>aspectjrt</artifactId>
+        <optional>true</optional>
+        </dependency>
+        <dependency>
+            <groupId>org.aspectj</groupId>
+        <artifactId>aspectjweaver</artifactId>
+        <optional>true</optional>
+        </dependency>
         <dependency>
             <groupId>org.springframework.ldap</groupId>
             <artifactId>spring-ldap</artifactId>
             <optional>true</optional>
         </dependency>
-	    <dependency>
-	        <groupId>cglib</groupId>
-		    <artifactId>cglib-nodep</artifactId>
-		    <scope>test</scope>
-		    <optional>true</optional>
-	    </dependency>	    
+        <dependency>
+            <groupId>cglib</groupId>
+            <artifactId>cglib-nodep</artifactId>
+            <scope>test</scope>
+            <optional>true</optional>
+        </dependency>
         <dependency>
             <groupId>net.sf.ehcache</groupId>
             <artifactId>ehcache</artifactId>
@@ -95,7 +95,7 @@
             <artifactId>jaxen</artifactId>
             <version>1.1.1</version>
             <optional>true</optional>
-        </dependency>        
+        </dependency>
         <dependency>
             <groupId>javax.servlet</groupId>
             <artifactId>servlet-api</artifactId>
@@ -135,13 +135,19 @@
             <version>1.0.1</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-all</artifactId>
+            <version>1.8.5</version>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>log4j</groupId>
             <artifactId>log4j</artifactId>
             <optional>true</optional>
         </dependency>
     </dependencies>
-    
+
     <build>
         <plugins>
             <plugin>

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
+ * <a href="http://www.ietf.org/rfc/rfc2396.txt">RFC 2396</a>). 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.
+ * <p>
+ * 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;
+    }
+
+}

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.
+ * <p>
+ * 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.
+     * <p>
+     * An implementation can thus choose to modify the state of the request
+     * for the security infrastructure, while still maintaining the
+     */
+    public abstract void reset();
+}

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.
+ * <p>
+ * 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);
+}

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

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.
+ *
+ * <h3>Path Parameters</h3>
+ * Parameters (as defined in <a href="http://www.ietf.org/rfc/rfc2396.txt">RFC 2396</a>) are stripped from the path
+ * segments of the {@code servletPath} and {@code pathInfo} values of the request.
+ * <p>
+ * 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.
+ * <p>
+ * 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;
+    }
+}

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 <code>Filter</code> requests to a list of Spring-managed beans.
  * As of version 2.0, you shouldn't need to explicitly configure a <tt>FilterChainProxy</tt> 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 <tt>&lt;security:http /&gt</tt> namespace configuration options.
+ * by the default <tt>&lt;security:http /&gt;</tt> namespace configuration options.
  *
  * <p>The <code>FilterChainProxy</code> is loaded via a standard Spring {@link DelegatingFilterProxy} declaration in
  * <code>web.xml</code>. <code>FilterChainProxy</code> 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();
+            }
+        }
     }
 
 }

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) {
+            }
+        }
+    }
+}

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<String, String> testPaths = new LinkedHashMap<String,String>();
+
+    @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<String,String> 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<String,String> 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());
+        }
+    }
+
+}

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

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

pom.xml

@@ -8,8 +8,8 @@
     <packaging>pom</packaging>
 
     <modules>
-        <module>core</module>            
-        <module>core-tiger</module>        
+        <module>core</module>
+        <module>core-tiger</module>
         <!-- module>adapters</module -->
         <module>portlet</module>
         <module>ntlm</module>
@@ -43,9 +43,9 @@
     <ciManagement>
         <system>bamboo</system>
         <url>https://build.springframework.org/browse/SEC</url>
-    </ciManagement>    
+    </ciManagement>
 
-    <distributionManagement>	
+    <distributionManagement>
         <repository>
             <id>spring-release</id>
             <name>Spring Release Repository</name>
@@ -177,10 +177,10 @@
         </contributor>
         <contributor>
             <name>Ruud Senden</name>
-        </contributor>                
+        </contributor>
         <contributor>
             <name>Michael Mayr</name>
-        </contributor>        
+        </contributor>
     </contributors>
 
     <dependencies>
@@ -205,7 +205,7 @@
                 <plugin>
                     <groupId>com.springsource.bundlor</groupId>
                     <artifactId>com.springsource.bundlor.maven</artifactId>
-                    <version>1.0.0.M5</version>
+                    <version>1.0.0.M6</version>
                     <executions>
                         <execution>
                             <id>bundlor</id>
@@ -218,7 +218,7 @@
                 </plugin>
             </plugins>
         </pluginManagement>
-        <plugins>            
+        <plugins>
             <plugin>
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-help-plugin</artifactId>
@@ -435,11 +435,11 @@
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-surefire-report-plugin</artifactId>
                 <version>2.4.2</version>
-<!--                
+<!--
                 <configuration>
                     <aggregate>true</aggregate>
-                </configuration>                
--->                
+                </configuration>
+-->
             </plugin>
             <plugin>
                 <groupId>org.apache.maven.plugins</groupId>
@@ -449,7 +449,7 @@
                     <excludes>
                         <exclude>bigbank/**</exclude>
                     </excludes>
-                </configuration>                
+                </configuration>
             </plugin>
             <!--
                 <plugin>
@@ -469,14 +469,14 @@
             </plugin>
 
             <plugin>
-                <groupId>org.apache.maven.plugins</groupId>                    
+                <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-javadoc-plugin</artifactId>
                 <version>2.4</version>
                 <configuration>
                     <aggregate>true</aggregate>
                     <header>Spring Security Framework</header>
                     <quiet>true</quiet>
-                    <excludePackageNames>sample,bigbank,zzz</excludePackageNames>                                        
+                    <excludePackageNames>sample,bigbank,zzz</excludePackageNames>
                     <links>
                         <link>
                             http://java.sun.com/j2se/1.5.0/docs/api
@@ -520,7 +520,7 @@
                       <!--<report>test-javadoc</report> -->
                     </reports>
                   </reportSet>
-                </reportSets>                
+                </reportSets>
             </plugin>
             <plugin>
                 <groupId>org.codehaus.mojo</groupId>
@@ -530,7 +530,7 @@
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-project-info-reports-plugin</artifactId>
                 <version>2.0.1</version>
-<!--                
+<!--
                 <reportSets>
                     <reportSet>
                         <reports>
@@ -542,8 +542,8 @@
                         </reports>
                     </reportSet>
                 </reportSets>
--->                
-            </plugin>           
+-->
+            </plugin>
         </plugins>
     </reporting>
 
@@ -604,7 +604,7 @@
                 <artifactId>aspectjweaver</artifactId>
                 <optional>true</optional>
                 <version>1.5.4</version>
-            </dependency>	    
+            </dependency>
             <dependency>
                 <groupId>org.aspectj</groupId>
                 <artifactId>aspectjrt</artifactId>
@@ -613,15 +613,15 @@
             <dependency>
                 <groupId>org.springframework</groupId>
                 <artifactId>spring-webmvc</artifactId>
-                <version>${spring.version}</version>                
-            </dependency>            
+                <version>${spring.version}</version>
+            </dependency>
             <dependency>
                 <groupId>cglib</groupId>
                 <artifactId>cglib-nodep</artifactId>
                 <scope>test</scope>
                 <optional>true</optional>
                 <version>2.1_3</version>
-            </dependency>	    
+            </dependency>
             <dependency>
                 <groupId>log4j</groupId>
                 <artifactId>log4j</artifactId>
@@ -679,7 +679,7 @@
         <jstl.version>1.1.2</jstl.version>
 
         <docbook.source>${basedir}/src/docbkx</docbook.source>
-        <docbook.target>${basedir}/target/site/guide</docbook.target>        
+        <docbook.target>${basedir}/target/site/guide</docbook.target>
     </properties>
 
 </project>