diff --git a/.gitignore b/.gitignore index 7ab06b3337..9144c45efc 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,7 @@ target/ .project .DS_Store .settings/ +.idea/ out/ intellij/ build/ diff --git a/config/src/test/java/org/springframework/security/config/FilterChainProxyConfigTests.java b/config/src/test/java/org/springframework/security/config/FilterChainProxyConfigTests.java index ceaeb41120..33d83e460d 100644 --- a/config/src/test/java/org/springframework/security/config/FilterChainProxyConfigTests.java +++ b/config/src/test/java/org/springframework/security/config/FilterChainProxyConfigTests.java @@ -116,18 +116,18 @@ public class FilterChainProxyConfigTests { } private void checkPathAndFilterOrder(FilterChainProxy filterChainProxy) throws Exception { - List filters = filterChainProxy.getFilters("/foo/blah"); + List filters = filterChainProxy.getFilters("/foo/blah;x=1"); assertEquals(1, filters.size()); assertTrue(filters.get(0) instanceof SecurityContextHolderAwareRequestFilter); - filters = filterChainProxy.getFilters("/some/other/path/blah"); + filters = filterChainProxy.getFilters("/some;x=2,y=3/other/path;z=4/blah"); assertNotNull(filters); assertEquals(3, filters.size()); assertTrue(filters.get(0) instanceof SecurityContextPersistenceFilter); assertTrue(filters.get(1) instanceof SecurityContextHolderAwareRequestFilter); assertTrue(filters.get(2) instanceof SecurityContextHolderAwareRequestFilter); - filters = filterChainProxy.getFilters("/do/not/filter"); + filters = filterChainProxy.getFilters("/do/not/filter;x=7"); assertEquals(0, filters.size()); filters = filterChainProxy.getFilters("/another/nonspecificmatch"); diff --git a/web/src/main/java/org/springframework/security/web/FilterChainProxy.java b/web/src/main/java/org/springframework/security/web/FilterChainProxy.java index 27edef4515..5d18483226 100644 --- a/web/src/main/java/org/springframework/security/web/FilterChainProxy.java +++ b/web/src/main/java/org/springframework/security/web/FilterChainProxy.java @@ -17,64 +17,100 @@ package org.springframework.security.web; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.security.web.firewall.DefaultHttpFirewall; +import org.springframework.security.web.firewall.FirewalledRequest; +import org.springframework.security.web.firewall.HttpFirewall; import org.springframework.security.web.util.AnyRequestMatcher; import org.springframework.security.web.util.RequestMatcher; +import org.springframework.security.web.util.UrlUtils; import org.springframework.util.Assert; import org.springframework.web.filter.DelegatingFilterProxy; import org.springframework.web.filter.GenericFilterBean; -import javax.servlet.*; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletRequestWrapper; +import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import java.io.IOException; 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 + * Delegates {@code Filter} requests to a list of Spring-managed filter beans. + * As of version 2.0, you shouldn't need to explicitly configure a {@code 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 {@code <security:http />} namespace configuration options. *

- * The FilterChainProxy is loaded via a standard Spring {@link DelegatingFilterProxy} declaration in - * web.xml. + * The {@code FilterChainProxy} is linked into the servlet container filter chain by adding a standard + * Spring {@link DelegatingFilterProxy} declaration in the application {@code web.xml} file. + * + *

Configuration

*

- * As of version 3.1, FilterChainProxy is configured using an ordered Map of {@link RequestMatcher} instances - * to Lists of Filters. The Map instance will normally be created while parsing the namespace - * configuration, so doesn't have to be set explicitly. Instead the <filter-chain-map> element should be used - * within the FilterChainProxy bean declaration. - * This in turn should have a list of child <filter-chain> elements which each define a URI pattern and the list - * of filters (as comma-separated bean names) which should be applied to requests which match the pattern. - * An example configuration might look like this: + * As of version 3.1, {@code FilterChainProxy} is configured using an ordered Map of {@link RequestMatcher} instances + * to {@code List}s of {@code Filter}s. The Map instance will normally be created while parsing the namespace + * configuration, so doesn't have to be set explicitly. Instead the {@code <filter-chain-map>} + * element should be used within the bean declaration. + * This in turn should have a list of child {@code <filter-chain>} elements which each define a URI pattern and + * the list of filters (as comma-separated bean names) which should be applied to requests which match the pattern. + * The default pattern matching strategy is to use {@link org.springframework.security.web.util.AntPathRequestMatcher + * Ant-style paths}. An example configuration might look like this: * * 

  <bean id="myfilterChainProxy" class="org.springframework.security.util.FilterChainProxy">
      <security:filter-chain-map request-matcher="ant">
-         <security:filter-chain pattern="/do/not/filter" filters="none"/>
+         <security:filter-chain pattern="/do/not/filter*" filters="none"/>
          <security:filter-chain pattern="/**" filters="filter1,filter2,filter3"/>
      </security:filter-chain-map>
  </bean>
  *
* - * The names "filter1", "filter2", "filter3" should be the bean names of Filter instances defined in the + * The names "filter1", "filter2", "filter3" should be the bean names of {@code Filter} instances defined in the * application context. The order of the names defines the order in which the filters will be applied. As shown above, * use of the value "none" for the "filters" can be used to exclude a request pattern from the security filter chain * entirely. Please consult the security namespace schema file for a full list of available configuration options. + * + *

Request Handling

*

- * Each possible pattern that FilterChainProxy should service must be entered. - * The first match for a given request will be used to define all of the Filters that apply to that - * request. This means you must put most specific matches at the top of the list, and ensure all Filters + * Each possible pattern that the {@code FilterChainProxy} should service must be entered. + * The first match for a given request will be used to define all of the {@code Filter}s that apply to that + * request. This means you must put most specific matches at the top of the list, and ensure all {@code Filter}s * that should apply for a given matcher are entered against the respective entry. - * The FilterChainProxy will not iterate through the remainder of the map entries to locate additional - * Filters. + * The {@code FilterChainProxy} will not iterate through the remainder of the map entries to locate additional + * {@code Filter}s. *

- * FilterChainProxy respects normal handling of Filters that elect not to call {@link + * {@code FilterChainProxy} respects normal handling of {@code Filter}s that elect not to call {@link * javax.servlet.Filter#doFilter(javax.servlet.ServletRequest, javax.servlet.ServletResponse, - * javax.servlet.FilterChain)}, in that the remainder of the original or FilterChainProxy-declared filter + * javax.servlet.FilterChain)}, in that the remainder of the original or {@code FilterChainProxy}-declared filter * chain will not be called. + * + *

Request Firewalling

+ * + * An {@link HttpFirewall} instance is used to validate incoming requests and create a wrapped request which provides + * consistent path values for matching against. See {@link DefaultHttpFirewall}, for more information on the type of + * attacks which the default implementation protects against. A custom implementation can be injected to provide + * stricter control over the request contents or if an application needs to support certain types of request which + * are rejected by default. *

- * Note the Filter lifecycle mismatch between the servlet container and IoC - * container. As described in the {@link DelegatingFilterProxy} JavaDocs, we recommend you allow the IoC - * container to manage the lifecycle instead of the servlet container. + * Note that this means that you must use the Spring Security filters in combination with a {@code FilterChainProxy} + * if you want this protection. Don't define them explicitly in your {@code web.xml} file. + *

+ * {@code FilterChainProxy} will use the firewall instance to obtain both request and response objects which will be + * fed down the filter chain, so it is also possible to use this functionality to control the functionality of the + * response. When the request has passed through the security filter chain, the {@code reset} method will be called. + * With the default implementation this means that the original values of {@code servletPath} and {@code pathInfo} will + * be returned thereafter, instead of the modified ones used for security pattern matching. + * + * + *

Filter Lifecycle

+ *

+ * Note the {@code Filter} lifecycle mismatch between the servlet container and IoC + * container. As described in the {@link DelegatingFilterProxy} Javadocs, we recommend you allow the IoC + * container to manage the lifecycle instead of the servlet container. {@code FilterChainProxy} does not invoke the + * standard filter lifecycle methods on any filter beans that you add to the application context. * * @author Carlos Sanchez * @author Ben Alex @@ -92,6 +128,8 @@ public class FilterChainProxy extends GenericFilterBean { private FilterChainValidator filterChainValidator = new NullFilterChainValidator(); + private HttpFirewall firewall = new DefaultHttpFirewall(); + //~ Methods ======================================================================================================== @Override @@ -103,25 +141,27 @@ public class FilterChainProxy extends GenericFilterBean { public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { - FilterInvocation fi = new FilterInvocation(request, response, chain); - List filters = getFilters(fi.getRequest()); + FirewalledRequest fwRequest = firewall.getFirewalledRequest((HttpServletRequest) request); + HttpServletResponse fwResponse = firewall.getFirewalledResponse((HttpServletResponse) response); + String url = UrlUtils.buildRequestUrl(fwRequest); + + List filters = getFilters(fwRequest); if (filters == null || filters.size() == 0) { if (logger.isDebugEnabled()) { - logger.debug(fi.getRequestUrl() + + logger.debug(url + (filters == null ? " has no matching filters" : " has an empty filter list")); } - chain.doFilter(request, response); + chain.doFilter(fwRequest, fwResponse); return; } - VirtualFilterChain virtualFilterChain = new VirtualFilterChain(fi, filters); - virtualFilterChain.doFilter(fi.getRequest(), fi.getResponse()); + VirtualFilterChain vfc = new VirtualFilterChain(url, chain, filters); + vfc.doFilter(fwRequest, fwResponse); } - /** * Returns the first filter chain matching the supplied URL. * @@ -147,20 +187,20 @@ public class FilterChainProxy extends GenericFilterBean { * @return matching filter list */ public List getFilters(String url) { - return getFilters(new FilterInvocation(url, null).getRequest()); + return getFilters(firewall.getFirewalledRequest((new FilterInvocation(url, null).getRequest()))); } /** * Sets the mapping of URL patterns to filter chains. * - * The map keys should be the paths and the values should be arrays of Filter objects. + * The map keys should be the paths and the values should be arrays of {@code Filter} objects. * It's VERY important that the type of map used preserves ordering - the order in which the iterator * returns the entries must be the same as the order they were added to the map, otherwise you have no way * of guaranteeing that the most specific patterns are returned before the more general ones. So make sure - * the Map used is an instance of LinkedHashMap or an equivalent, rather than a plain HashMap, for + * the Map used is an instance of {@code LinkedHashMap} or an equivalent, rather than a plain {@code HashMap}, for * example. * - * @param filterChainMap the map of path Strings to List<Filter>s. + * @param filterChainMap the map of path Strings to {@code List<Filter>}s. */ @SuppressWarnings("unchecked") public void setFilterChainMap(Map filterChainMap) { @@ -198,7 +238,7 @@ public class FilterChainProxy extends GenericFilterBean { /** * Returns a copy of the underlying filter chain map. Modifications to the map contents - * will not affect the FilterChainProxy state - to change the map call setFilterChainMap. + * will not affect the FilterChainProxy state - to change the map call {@code setFilterChainMap}. * * @return the map of path pattern Strings to filter chain lists (with ordering guaranteed). */ @@ -216,6 +256,16 @@ public class FilterChainProxy extends GenericFilterBean { this.filterChainValidator = filterChainValidator; } + /** + * Sets the "firewall" implementation which will be used to validate and wrap (or potentially reject) the + * incoming requests. The default implementation should be satisfactory for most requirements. + * + * @param firewall + */ + public void setFirewall(HttpFirewall firewall) { + this.firewall = firewall; + } + public String toString() { StringBuilder sb = new StringBuilder(); sb.append("FilterChainProxy["); @@ -230,18 +280,18 @@ public class FilterChainProxy extends GenericFilterBean { /** * Internal {@code FilterChain} implementation that is used to pass a request through the additional - * internal list of filters which match the request. Records the position in the additional filter chain and, when - * completed, passes the request back to the original {@code FilterChain} supplied by the servlet container. + * internal list of filters which match the request. */ private static class VirtualFilterChain implements FilterChain { - private final FilterInvocation fi; + private final FilterChain originalChain; private final List additionalFilters; + private final String url; private final int size; private int currentPosition = 0; - - private VirtualFilterChain(FilterInvocation filterInvocation, List additionalFilters) { - this.fi = filterInvocation; + private VirtualFilterChain(String url, FilterChain chain, List additionalFilters) { + this.originalChain = chain; + this.url = url; this.additionalFilters = additionalFilters; this.size = additionalFilters.size(); } @@ -249,23 +299,35 @@ public class FilterChainProxy extends GenericFilterBean { public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { if (currentPosition == size) { if (logger.isDebugEnabled()) { - logger.debug(fi.getRequestUrl() - + " reached end of additional filter chain; proceeding with original chain"); + logger.debug(url + " reached end of additional filter chain; proceeding with original chain"); } - fi.getChain().doFilter(request, response); + // Deactivate path stripping as we exit the security filter chain + resetWrapper(request); + + originalChain.doFilter(request, response); } else { currentPosition++; Filter nextFilter = additionalFilters.get(currentPosition - 1); if (logger.isDebugEnabled()) { - logger.debug(fi.getRequestUrl() + " at position " + currentPosition + " of " + logger.debug(url + " at position " + currentPosition + " of " + size + " in additional filter chain; firing Filter: '" - + nextFilter + "'"); + + nextFilter.getClass().getSimpleName() + "'"); } - nextFilter.doFilter(request, response, this); + 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/web/src/main/java/org/springframework/security/web/firewall/DefaultHttpFirewall.java b/web/src/main/java/org/springframework/security/web/firewall/DefaultHttpFirewall.java new file mode 100644 index 0000000000..a40fa435dc --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/firewall/DefaultHttpFirewall.java @@ -0,0 +1,67 @@ +package org.springframework.security.web.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/web/src/main/java/org/springframework/security/web/firewall/FirewalledRequest.java b/web/src/main/java/org/springframework/security/web/firewall/FirewalledRequest.java new file mode 100644 index 0000000000..48cf40acc2 --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/firewall/FirewalledRequest.java @@ -0,0 +1,34 @@ +package org.springframework.security.web.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/web/src/main/java/org/springframework/security/web/firewall/HttpFirewall.java b/web/src/main/java/org/springframework/security/web/firewall/HttpFirewall.java new file mode 100644 index 0000000000..de7b9af17e --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/firewall/HttpFirewall.java @@ -0,0 +1,32 @@ +package org.springframework.security.web.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/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedException.java b/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedException.java new file mode 100644 index 0000000000..fe334219a0 --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedException.java @@ -0,0 +1,10 @@ +package org.springframework.security.web.firewall; + +/** + * @author Luke Taylor + */ +public class RequestRejectedException extends RuntimeException { + public RequestRejectedException(String message) { + super(message); + } +} diff --git a/web/src/main/java/org/springframework/security/web/firewall/RequestWrapper.java b/web/src/main/java/org/springframework/security/web/firewall/RequestWrapper.java new file mode 100644 index 0000000000..9cc3921905 --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/firewall/RequestWrapper.java @@ -0,0 +1,103 @@ +package org.springframework.security.web.firewall; + +import org.springframework.security.web.util.UrlUtils; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletRequestWrapper; +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(); + } + + @Override + public String getPathInfo() { + return stripPaths ? strippedPathInfo : super.getPathInfo(); + } + + @Override + public String getServletPath() { + return stripPaths ? strippedServletPath : super.getServletPath(); + } + + public void reset() { + this.stripPaths = false; + } +} diff --git a/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java b/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java new file mode 100644 index 0000000000..917dc47063 --- /dev/null +++ b/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java @@ -0,0 +1,116 @@ +package org.springframework.security.web; + +import static org.junit.Assert.*; +import static org.mockito.Mockito.*; + +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.web.firewall.FirewalledRequest; +import org.springframework.security.web.util.RequestMatcher; + +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletRequestWrapper; +import javax.servlet.http.HttpServletResponse; +import java.util.*; + +/** + * @author Luke Taylor + */ +@SuppressWarnings({"unchecked"}) +public class FilterChainProxyTests { + private FilterChainProxy fcp; + private RequestMatcher matcher; + private MockHttpServletRequest request; + private MockHttpServletResponse response; + private FilterChain chain; + private Filter filter; + + @Before + public void setup() throws Exception { + fcp = new FilterChainProxy(); + fcp.setFilterChainValidator(mock(FilterChainProxy.FilterChainValidator.class)); + matcher = mock(RequestMatcher.class); + filter = mock(Filter.class); + doAnswer(new Answer() { + public Object answer(InvocationOnMock inv) throws Throwable { + Object[] args = inv.getArguments(); + FilterChain fc = (FilterChain) args[2]; + HttpServletRequestWrapper extraWrapper = + new HttpServletRequestWrapper((HttpServletRequest) args[0]); + fc.doFilter(extraWrapper, (HttpServletResponse) args[1]); + return null; + } + }).when(filter).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class), any(FilterChain.class)); + LinkedHashMap map = new LinkedHashMap(); + map.put(matcher, Arrays.asList(filter)); + fcp.setFilterChainMap(map); + request = new MockHttpServletRequest(); + request.setServletPath("/path"); + response = new MockHttpServletResponse(); + chain = mock(FilterChain.class); + } + + @Test + public void toStringCallSucceeds() throws Exception { + fcp.afterPropertiesSet(); + fcp.toString(); + } + + @Test + public void securityFilterChainIsNotInvokedIfMatchFails() throws Exception { + when(matcher.matches(any(HttpServletRequest.class))).thenReturn(false); + fcp.doFilter(request, response, chain); + assertEquals(1, fcp.getFilterChainMap().size()); + assertSame(filter, fcp.getFilterChainMap().get(matcher).get(0)); + + verifyZeroInteractions(filter); + // The actual filter chain should be invoked though + verify(chain).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class)); + } + + @Test + public void originalChainIsInvokedAfterSecurityChainIfMatchSucceeds() throws Exception { + when(matcher.matches(any(HttpServletRequest.class))).thenReturn(true); + 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 originalFilterChainIsInvokedIfMatchingSecurityChainIsEmpty() throws Exception { + LinkedHashMap map = new LinkedHashMap(); + map.put(matcher, Collections.emptyList()); + fcp.setFilterChainMap(map); + + when(matcher.matches(any(HttpServletRequest.class))).thenReturn(true); + fcp.doFilter(request, response, chain); + + verify(chain).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class)); + } + + @Test + public void requestIsWrappedForMatchingAndFilteringWhenMatchIsFound() throws Exception { + when(matcher.matches(any(HttpServletRequest.class))).thenReturn(true); + fcp.doFilter(request, response, chain); + verify(matcher).matches(any(FirewalledRequest.class)); + verify(filter).doFilter(any(FirewalledRequest.class), any(HttpServletResponse.class), any(FilterChain.class)); + verify(chain).doFilter(any(FirewalledRequest.class), any(HttpServletResponse.class)); + } + + @Test + public void requestIsWrappedForMatchingAndFilteringWhenMatchIsNotFound() throws Exception { + when(matcher.matches(any(HttpServletRequest.class))).thenReturn(false); + fcp.doFilter(request, response, chain); + verify(matcher).matches(any(FirewalledRequest.class)); + verifyZeroInteractions(filter); + verify(chain).doFilter(any(FirewalledRequest.class), any(HttpServletResponse.class)); + } + +} diff --git a/web/src/test/java/org/springframework/security/web/firewall/DefaultHttpFirewallTests.java b/web/src/test/java/org/springframework/security/web/firewall/DefaultHttpFirewallTests.java new file mode 100644 index 0000000000..8b3df587e9 --- /dev/null +++ b/web/src/test/java/org/springframework/security/web/firewall/DefaultHttpFirewallTests.java @@ -0,0 +1,44 @@ +package org.springframework.security.web.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/web/src/test/java/org/springframework/security/web/firewall/RequestWrapperTests.java b/web/src/test/java/org/springframework/security/web/firewall/RequestWrapperTests.java new file mode 100644 index 0000000000..b462251440 --- /dev/null +++ b/web/src/test/java/org/springframework/security/web/firewall/RequestWrapperTests.java @@ -0,0 +1,62 @@ +package org.springframework.security.web.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()); + } + } + +}