AntPathRequestMatcher and MvcRequestMatcher have inconsistent behaviour against requests with null method

[symposion]

If you configure a new AntPathRequestMatcher(HttpMethod.OPTIONS, "/**") and ask it to match an HttpRequest that returns null from getMethod(), it will always match.

If you configure an equivalent MvcRequestMatcher - usually indirectly by doing http.authorizeHttpRequests().requestMatchers(HttpMethod.OPTIONS, "/**) - and ask it to match an HttpRequest that returns null from getMethod(), it will never match.

It might seem that a request returning a null method would, at best, be a very unlikely edge case, but sadly it's not, because this is exactly what DefaultWebInvocationPrivilegeEvaluator and AuthorizationManagerWebInvocationPrivilegeEvaluator do if you call the overloads that don't take a method parameter.

This was especially problematic in Spring Boot 2.x because ErrorPageSecurityFilter by default made use of a WebInvocationPrivilegeEvaluator instance to decide if the user was permitted access to the error page. Switching from using explict antMatchers to just calling the requestMatchers method could produce completely different authorization results for ERROR dispatches as a result in extremely surprising ways. Thankfully in Spring Boot 3/Spring Security 6 this whole mechanism has been removed in favour of the AuthorizationFilter applying to all dispatch types.

Nevertheless the WebInvocationPrivilegeEvaluator mechanism still exists and I think it's at the very least extremely surprising - and perhaps even wrong - that these two matcher mechanisms behave differently in this regard. If WebInvocationPrivilegeEvaluator is going to be permitted to ask about privileges without specifying a method - which I guess is itself slightly questionable, but is a long-established interface now - I think these two RequestMatcher implementations should be made consistent in their behaviour so that apparently innocuous changes in the way a request pattern is specified don't have unexpected consequences for application privileges.

[jzheaux]

Thanks @symposion for the detailed report.

Even though, AntPathRequestMatcher and MvcRequestMatcher will be deprecated in the next release, I think this still merits a look.

The intent of WebInvocationPrivilegeEvaluator#isAllowed(String, Authentication) is to match regardless of the method, but MvcRequestMatcher does not support ignoring the method when it is not present on HttpServletRequest.

Note that in the mean time, you can use the static factory antMatcher to stay with AntPathRequestMatcher for the time being:

.requestMatchers(antMatcher(HttpMethod.OPTIONS, "/**"))

Out of curiosity, is there a reason that you are permitting OPTIONS? Since cors() installs a filter before the authorization filter, permitting OPTIONS should be unnecessary. Perhaps you aren't using Spring's support for CORS. What happens if you remove your method-specific authorization rules?

Either way, I will take this back to the team, specifically in light of the above-linked efforts, which replace both matchers with a single implementation, forcing this decision one way or the other.

[jzheaux]

When someone passes null for the method to WebInvocationPrivilegeEvaluator, they are saying that the method doesn't matter to them for matching purposes. However, it's non-trivial to make an authorization decision in any circumstance where the relevant request matcher does require a method.

Imagine the following arrangement:

.requestMatchers(HttpMethod.GET, "/path").hasAuthority("USER")
.requestMatchers(HttpMethod.POST, "/path").permitAll()

When WebInvocationPrivilegeEvaluator#isAllowed("/path", authentication) is called, which matcher's authorization rules should it use?

This happens in the more generic case as well:

.requestMatchers(HttpMethod.GET, "/path").hasAuthority("USER")
.anyRequest().denyAll()

Or, in other words: "if GET /path, then require USER authority; if POST | PUT | DELETE /path, then deny". Here, also, there's no way to know which the user intends, unless they also specify a method.

As such, I believe it's reasonable to require passing a method if you want method-specific request matchers to be considered.

Given that, I think we should do a few things:

1. Update the JavaDoc in WebInvocationPrivilegeEvaluator to clarify that null method means it will only match authorization rules that don't require a specific method - Clarify WebInvocationPrivilegeEvaluator JavaDoc #16529
2. Update the JSP documentation in the same way - Clarify in JSP documentation where method attribute is required #16530
3. Add migration steps to the 5.8 migration guide
4. Improve AntPathRequestMatcher to only match when the HTTP and request matcher methods match

I'm not yet clear on whether the last one would be considered a breaking change as the servlet spec strongly implies that getMethod does not return null.

[symposion]

Out of curiosity, is there a reason that you are permitting OPTIONS? Since cors() installs a filter before the authorization filter, permitting OPTIONS should be unnecessary. Perhaps you aren't using Spring's support for CORS. What happens if you remove your method-specific authorization rules?

Honestly, I suspect that there isn't a good reason. We discovered this while updating an old authorization service that was based on the previous @EnableAuthorizationServer OAuth2 server implementation. I strongly suspect that this rule was there for CORS reasons, but we are using Spring Security's CORS implementation and I'm not sure the rule was ever actually required; and as you say, it definitely shouldn't be required now.

This is not blocking us in any way, we just spent a bunch of time scratching our heads as a result of the interaction with the ErrorPageSecurityFilter when we changed the matcher implementation and I thought I'd flag it up as a potential source of confusion for others. I definitely wouldn't consider it a high-priority issue.

Thanks for taking the time to investigate further!

[jzheaux]

I'm closing this ticket since it is superceded by AntPathRequestMatcher's and MvcRequestMatcher's removal.