Spring Security AuthenticationWebFilter executes filter chain twice per request

I've noticed this for some time in my application logs and thought I was just misconfiguring Spring Security WebFlux somehow. But upon closer examination, I think there's a bug in AuthenticationWebFilter that causes this behavior:

https://github.com/spring-projects/spring-security/blob/8e2a4bf3562133c78230ec5a96ec993c5c92374b/web/src/main/java/org/springframework/security/web/server/authentication/AuthenticationWebFilter.java#L114

        @Override
    public Mono<Void> filter(ServerWebExchange exchange, WebFilterChain chain) {
        return this.requiresAuthenticationMatcher.matches(exchange)
            .filter((matchResult) -> matchResult.isMatch())
            .flatMap((matchResult) -> this.authenticationConverter.convert(exchange))
            .switchIfEmpty(chain.filter(exchange).then(Mono.empty()))
            .flatMap((token) -> authenticate(exchange, chain, token))
            .onErrorResume(AuthenticationException.class, (ex) -> this.authenticationFailureHandler
                .onAuthenticationFailure(new WebFilterExchange(exchange, chain), ex));
    }

The .switchIfEmpty() here is actually subscribing to the filter chain on downstream subscription, rather than deferring it until it is needed. Shouldn't that line be more like:

.switchIfEmpy(Mono.defer(() -> chain.filter(exchange).then(Mono.empty()))

And, in fact, the method directly below this one uses Mono.defer() for error cases.

Comment From: ngocnhan-tran1996

I search with keyword .switchIfEmpty(chain.filter( and result

*Filter may be run twice as you said and I am not sure whether it's a bug

Comment From: pat-mccusker

Out of curiosity, do you know what implementation of WebSecurityFilterChain you have selected? In spring-web's DefaultWebFilterChain the filter method contains its own deferred Mono:

@Override
public Mono<Void> filter(ServerWebExchange exchange) {
    return Mono.defer(() ->
        this.currentFilter != null && this.chain != null ?
                invokeFilter(this.currentFilter, this.chain, exchange) :
                this.handler.handle(exchange));
}

Comment From: jdolan-chwy

I'm mocking it. Pretty sure I see this in the application when it's running as well. I'll confirm that tomorrow.

@Test
void doFilterSuccessful() {

  final var req = MockServerHttpRequest.get("/api/customer/test")
      .cookie(new HttpCookie(ACCESS_TOKEN_COOKIE_NAME, AUTHENTICATED.getAccessTokenValue()))
      .build();

  final var ex = MockServerWebExchange.from(req);
  final var chain = mock(WebFilterChain.class);
  when(chain.filter(ex)).thenReturn(Mono.empty());

  when(authContextService.getAuthContext(any(), any())).thenReturn(AUTHENTICATED);

  StepVerifier.create(filter.filter(ex, chain).verifyComplete();

  verify(authContextService).getAuthContext(any(), any());

  // https://github.com/spring-projects/spring-security/issues/16553
  verify(chain, times(2)).filter(ex);
}

Comment From: pat-mccusker

I think it all depends on what your real WebSecurityFilterChain is doing. Multiple invocations of that mock doesn't itself indicate a problem if the real chain that is used at runtime would handle the deferral itself.

Comment From: jdolan-chwy

I can see that side of it -- that in practice this shouldn't happen due to the default implementation. But shouldn't AuthenticationWebFilter treat the chain as a black box and not rely on its implementation details to correct for its eager subscriptions?

Comment From: pat-mccusker

There probably wouldn't be any harm in deferrals on both sides, and I definitely think that issues around eagerness are bound to arise with reactor's switchIfEmpty behavior. Interestingly enough, reactor's own kotlin switchIfEmpty extension function includes a deferral in its definition, seemingly indicating an accounting for what might be a common problem.

All that being the case, it may not be the cause of your current duplicated execution problem.

Comment From: jdolan-chwy

Yea, that's very telling that they put foam padding around that method for Kotlin 😄