spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.71k stars 5.86k forks source link

Possible bug in AbstractRequestMatcherRegistry#requireOnlyPathMappedDispatcherServlet? (DispatcherServlet not found when resolving request matcher) #15684

Open mauromol opened 3 weeks ago

mauromol commented 3 weeks ago

Describe the bug Once I added a DispatcherServlet to my EAR application deployed on JBoss 7.4, I started to get the following exception:

Exception handling request to /myapp/rest/foo/hello: java.lang.IllegalArgumentException: This method cannot decide whether these patterns are Spring MVC patterns or not. If this endpoint is a Spring MVC endpoint, please use requestMatchers(MvcRequestMatcher); otherwise, please use requestMatchers(AntPathRequestMatcher).

This is because there is more than one mappable servlet in your servlet context:
[indeed, I have a lot of mapped servlets and the DispatcherServlet is not the first one]

For each MvcRequestMatcher, call MvcRequestMatcher#setServletPath to indicate the servlet path.

To Reproduce Adding a security filter chain with some request matchers; something like this:

  @Bean
  public SecurityFilterChain mySecurityFilterChain(final HttpSecurity http) throws Exception {
    http.csrf()
        .disable()
        .authorizeHttpRequests(new MyRules())
        ... // and so on
  }

public class MyRules implements Customizer<AuthorizeHttpRequestsConfigurer<HttpSecurity>.AuthorizationManagerRequestMatcherRegistry> {
  @Override
  public void customize(AuthorizeHttpRequestsConfigurer<HttpSecurity>.AuthorizationManagerRequestMatcherRegistry rules) {

    // static resources
    rules.requestMatchers("/index.html").permitAll();
    rules.requestMatchers("/static/**").permitAll();
    rules.requestMatchers("/**").denyAll();
  }
}

Expected behavior I would expect Spring Security to find my DispatcherServlet mapping and so to use a MvcRequestMatcher.

Please have a look at this line: https://github.com/spring-projects/spring-security/blob/e90a6b66fe6af1316aadda0a66ca20d8cfba3404/config/src/main/java/org/springframework/security/config/annotation/web/AbstractRequestMatcherRegistry.java#L289

Shouldn't it be continue instead of return null, just like it is for requireOneRootDispatcherServlet? Otherwise this loop will always end on the first mapping if it's not a DispatcherServlet...

Please note I'm on Spring Security 5.8.13 and that line is number 408 instead.

mauromol commented 3 weeks ago

Perhaps I'm starting to understand the logic behind this implementation, which is probably as intended. So, Spring Security uses MVC matchers when using AbstractRequestMatcherRegistry#requestMatchers(java.lang.String...) if either there is a "root" DispatcherServlet (that is, mapped to "/"), or if there are only DispatcherServlets (and no other servlet) registered in the ServletContext. Indeed, it uses the detected DispatcherServlet to extract the servlet path prefix to correctly match incoming request URIs. In the latter case, for some reason, it makes the last DispatcherServlet found win, which sounds a bit odd to me, also because I found no trace of the fact that servlet registrations are/can be sorted in any way.

TBH, it was not easy to grasp this and I think the documentation is not so clear. Looking at the documentation at:

it seems to suggest that just having Spring Web MVC on the classpath makes usage of AbstractRequestMatcherRegistry#requestMatchers(java.lang.String...) method automagically work for basic usages and patterns, whichever is the servlet that will handle the target resource. Also, the code in AbstractRequestMatcherRegistry is not well documented. However, what seems like to emerge is that the whole thing works only if you have either no Spring Dispatcher Servlet usage or if you are only registering Spring Dispatcher Servlets (so in a fully-Spring Web MVC managed webapp). But in a mixed application, using different kind of servlets to handle different URIs, that method will never work and you always have to use explicit Ant or MVC request matchers.

Is my understanding right? Why do you assume that, if you have some DispatcherServlets registered in the servlet context, then the default behaviour expected by the user when defining URL matching patterns to secure requests with AbstractRequestMatcherRegistry#requestMatchers(java.lang.String...) is to use paths relative to the mapping of one (the last one?) of those DispatcherServlets and not to the "root" servlet path returned by the ServletContext? I'm not getting this.