OPEN - issue SEC-953: Query string isn't ignored while url - filterchain pattern matching

http://jira.springframework.org/browse/SEC-953. Added stripQueryStringFromUrls parameter to FilterChainProxy which works the same as the one on DefaultFilterInvocationDefinitionSource. This defaults to true when used with ant path matching.

core/src/main/java/org/springframework/security/config/HttpSecurityBeanDefinitionParser.java

@@ -208,6 +208,7 @@ public class HttpSecurityBeanDefinitionParser implements BeanDefinitionParser {
         RootBeanDefinition filterChainProxy = new RootBeanDefinition(FilterChainProxy.class);
         filterChainProxy.setSource(source);
         filterChainProxy.getPropertyValues().addPropertyValue("matcher", matcher);
+        filterChainProxy.getPropertyValues().addPropertyValue("stripQueryStringFromUrls", matcher instanceof AntUrlPathMatcher);
         filterChainProxy.getPropertyValues().addPropertyValue("filterChainMap", filterChainMap);
         pc.getRegistry().registerBeanDefinition(BeanIds.FILTER_CHAIN_PROXY, filterChainProxy);
         pc.getRegistry().registerAlias(BeanIds.FILTER_CHAIN_PROXY, BeanIds.SPRING_SECURITY_FILTER_CHAIN);

core/src/main/java/org/springframework/security/util/FilterChainProxy.java

@@ -107,6 +107,7 @@ public class FilterChainProxy implements Filter, InitializingBean, ApplicationCo
     /** Compiled pattern version of the filter chain map */
     private Map filterChainMap;
     private UrlMatcher matcher = new AntUrlPathMatcher();
+    private boolean stripQueryStringFromUrls = true;
     private DefaultFilterInvocationDefinitionSource fids;
 
     //~ Methods ========================================================================================================
@@ -181,6 +182,16 @@ public class FilterChainProxy implements Filter, InitializingBean, ApplicationCo
      * @return an ordered array of Filters defining the filter chain
      */
     public List getFilters(String url)  {
+        if (stripQueryStringFromUrls) {
+            // String query string - see SEC-953
+            int firstQuestionMarkIndex = url.indexOf("?");
+
+            if (firstQuestionMarkIndex != -1) {
+                url = url.substring(0, firstQuestionMarkIndex);
+            }
+        }
+
+
         Iterator filterChains = filterChainMap.entrySet().iterator();
 
         while (filterChains.hasNext()) {
@@ -319,6 +330,14 @@ public class FilterChainProxy implements Filter, InitializingBean, ApplicationCo
         return matcher;
     }
 
+    /**
+     * If set to 'true', the query string will be stripped from the request URL before
+     * attempting to find a matching filter chain. This is the default value.
+     */
+    public void setStripQueryStringFromUrls(boolean stripQueryStringFromUrls) {
+        this.stripQueryStringFromUrls = stripQueryStringFromUrls;
+    }
+
     public String toString() {
         StringBuffer sb = new StringBuffer();
         sb.append("FilterChainProxy[");

core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java

@@ -79,6 +79,8 @@ public class HttpSecurityBeanDefinitionParserTests {
         List filterList = getFilters("/anyurl");
 
         checkAutoConfigFilters(filterList);
+
+        assertEquals(true, FieldUtils.getFieldValue(appContext.getBean("_filterChainProxy"), "stripQueryStringFromUrls"));
         assertEquals(true, FieldUtils.getFieldValue(filterList.get(10), "objectDefinitionSource.stripQueryStringFromUrls"));
     }
 
@@ -136,6 +138,7 @@ public class HttpSecurityBeanDefinitionParserTests {
         // This will be matched by the default pattern ".*"
         List allFilters = getFilters("/ImCaughtByTheUniversalMatchPattern");
         checkAutoConfigFilters(allFilters);
+        assertEquals(false, FieldUtils.getFieldValue(appContext.getBean("_filterChainProxy"), "stripQueryStringFromUrls"));
         assertEquals(false, FieldUtils.getFieldValue(allFilters.get(10), "objectDefinitionSource.stripQueryStringFromUrls"));
     }
 
@@ -149,7 +152,6 @@ public class HttpSecurityBeanDefinitionParserTests {
         // These will be matched by the default pattern "/**"
         checkAutoConfigFilters(getFilters("/secure"));
         checkAutoConfigFilters(getFilters("/ImCaughtByTheUniversalMatchPattern"));
-
     }
 
     @Test

core/src/test/java/org/springframework/security/util/FilterChainProxyTests.java

@@ -62,20 +62,16 @@ public class FilterChainProxyTests {
         }
     }
 
-    @Test
+    @Test(expected=IllegalArgumentException.class)
     public void testDetectsFilterInvocationDefinitionSourceThatDoesNotReturnAllConfigAttributes() throws Exception {
         FilterChainProxy filterChainProxy = new FilterChainProxy();
         filterChainProxy.setApplicationContext(new StaticApplicationContext());
 
-        try {
         filterChainProxy.setFilterInvocationDefinitionSource(new MockFilterInvocationDefinitionSource(false, false));
         filterChainProxy.afterPropertiesSet();
-            fail("Should have thrown IllegalArgumentException");
-        } catch (IllegalArgumentException expected) {
-        }
     }
 
-    @Test
+    @Test(expected=IllegalArgumentException.class)
     public void testDetectsIfConfigAttributeDoesNotReturnValueForGetAttributeMethod() throws Exception {
         FilterChainProxy filterChainProxy = new FilterChainProxy();
         filterChainProxy.setApplicationContext(new StaticApplicationContext());
@@ -89,12 +85,8 @@ public class FilterChainProxyTests {
 
         filterChainProxy.setFilterInvocationDefinitionSource(fids);
 
-        try {
         filterChainProxy.afterPropertiesSet();
         filterChainProxy.init(new MockFilterConfig());
-            fail("Should have thrown IllegalArgumentException");
-        } catch (IllegalArgumentException expected) {
-        }
     }
 
     @Test(expected = IllegalArgumentException.class)
@@ -164,6 +156,24 @@ public class FilterChainProxyTests {
         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());

core/src/test/resources/org/springframework/security/util/filtertest-valid.xml

@@ -73,6 +73,13 @@ http://www.springframework.org/schema/security http://www.springframework.org/sc
         </sec:filter-chain-map>
     </bean>
 
+    <bean id="newFilterChainProxyNoDefaultPath" class="org.springframework.security.util.FilterChainProxy">
+        <sec:filter-chain-map path-type="ant">
+            <sec:filter-chain pattern="/foo/**" filters="mockFilter"/>
+            <sec:filter-chain pattern="/*.bar" filters="mockFilter,mockFilter2"/>
+        </sec:filter-chain-map>
+    </bean>
+
     <bean id="newFilterChainProxyWrongPathOrder" class="org.springframework.security.util.FilterChainProxy">
         <sec:filter-chain-map path-type="ant">
             <sec:filter-chain pattern="/foo/**" filters="mockFilter"/>