spring-projects / spring-security

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

Fail when any filter chain declared after an AnyRequestMatcher filter chain #15220

Closed jzheaux closed 1 week ago

jzheaux commented 2 weeks ago

By default, a filter chain's securityMatcher defaults to any request:

http
//  .securityMatcher("/**") <-- implied by default
    .authorizeHttpRequests(...)
    .httpBasic(...)
    ...

This means that configurations that use multiple default filter chains can get unexpected results. Since technically both chains match any request, the first chain is always picked and the second one is never picked.

Spring Security should help by failing during startup when an application has any filter chain that is defined after an "any request" filter chain. For example, a configuration like this is problematic:

@Bean 
@Order(0)
SecurityFilterChain api(HttpSecurity http) throws Exception {
    http
        .authorizeHttpRequests(...)
        .httpBasic(...)

    return http.build();
}

@Bean 
@Order(1)
SecurityFilterChain app(HttpSecurity http) throws Exception {
    http
        .securityMatcher("/app/**")
        .authorizeHttpRequests(...)
        .formLogin(...)

    return http.build();
}

since the first filter chain will always consume any request and the second filter chain will never get exercised.

For clarity, an example fix in the above case could be:

@Bean 
@Order(0)
SecurityFilterChain api(HttpSecurity http) throws Exception {
    http
        .securityMatcher("/api/**")
        .authorizeHttpRequests(...)
        .httpBasic(...)

    return http.build();
}

@Bean 
@Order(1)
SecurityFilterChain app(HttpSecurity http) throws Exception {
    http
        .securityMatcher("/app/**")
        .authorizeHttpRequests(...)
        .formLogin(...)

    return http.build();
}

so that neither of them captures every request.

Or a more preferred one would be:

@Bean 
@Order(0)
SecurityFilterChain app(HttpSecurity http) throws Exception {
    http
        .securityMatcher("/app/**")
        .authorizeHttpRequests(...)
        .formLogin(...)

    return http.build();
}

@Bean 
@Order(1)
SecurityFilterChain api(HttpSecurity http) throws Exception {
    http
        .authorizeHttpRequests(...)
        .httpBasic(...)

    return http.build();
}

as this one still has an "any request" filter chain that acts as a catch-all and is correctly placed as the last filter chain.

I think this check could be done in WebSecurity#performBuild after all the SecurityFilterChain instances are fully built. Something like this might do the trick:

+ boolean anyRequestConfigured = false;
for (SecurityBuilder<? extends SecurityFilterChain> securityFilterChainBuilder : this.securityFilterChainBuilders) {
    SecurityFilterChain securityFilterChain = securityFilterChainBuilder.build();
+    Assert.isTrue(!anyRequestConfigured, "A filter chain that matches any request has already been configured, which means that this filter chain for [" + matcher + "] will never get invoked. Please use `HttpSecurity#securityMatcher` to ensure that there is only one filter chain configured for 'any request' and that the 'any request' filter chain is published last.")
+    if (securityFilterChain.getRequestMatcher() instanceof AnyRequestMatcher matcher) {
+        anyRequestConfigured = true;
+    }
    securityFilterChains.add(securityFilterChain);
    // ...
}
CrazyParanoid commented 1 week ago

Hi @jzheaux. This will only work with DefaultSecurityFilterChain, so I added the necessary check. But one question worries me - how many people used this, thinking that it was not a bug but a feature? And won't they get an unpleasant surprise?

jzheaux commented 1 week ago

Good question, @CrazyParanoid. Yes, it may be a surprise, though it's an important one since their configuration is broken. When an any-request filter chain comes before another filter chain, that second filter chain is never consulted, perhaps unbeknownst to the developer. Updating to the next minor release will help them fix this configuration bug.