Spring Security ChannelProcessingFilter misuse of 'committed'

Summary

ChannelProcessingFilter short-circuits the entire filter chain if the response is already committed.

Actual Behavior

If the response has already been committed by the time it hits ChannelProcessingFilter, the filter mistakenly assumes that the commit was done by the ChannelDecisionManager, and does not allow the filter chain to continue.

It also does not throw an exception or log anything.

For context: We faced this in the wild with an app that would omit entire tiles if the page exceeded the size of the JSP buffer (which causes a flush, and therefore commits the response).

Expected Behavior

The doc on ChannelProcessingFilter says that processing will not proceed if the response is committed by the ChannelDecisionManager: https://github.com/spring-projects/spring-security/blob/b93528138e2b2f7ad34d10a040e6b7ac50fc587a/web/src/main/java/org/springframework/security/web/access/channel/ChannelProcessingFilter.java#L45-L46

However, the code simply checks isResponseCommitted() once(after calling the decision manager), so it cannot possibly know whether the decision manger is the one that committed the response, or if the response was committed prior to the filter invocation: https://github.com/spring-projects/spring-security/blob/b93528138e2b2f7ad34d10a040e6b7ac50fc587a/web/src/main/java/org/springframework/security/web/access/channel/ChannelProcessingFilter.java#L150-L153

ChannelDecisionManager does a similar thing to determine if calls to ChannelProcessors resulted in a decision being made: https://github.com/spring-projects/spring-security/blob/b93528138e2b2f7ad34d10a040e6b7ac50fc587a/web/src/main/java/org/springframework/security/web/access/channel/ChannelDecisionManagerImpl.java#L76-L78

All of these void 'decide' methods seem like they ought to just be returning the decision results as a boolean or enum or something, rather than using side effects of response manipulation to communicate with the caller.

Version

1.5.19.RELEASE (I'm guessing that Boot 2.x is similarly vulnerable, but I'm not able to reproduce it with my Tiles scenario for some reason)

Comment From: linus87

Our company has a custom framework based on spring, the response is wrapped and overridden. Use response.sendRedirect() did't make it committed. This make ChannelProcessingFilter failed. I have to rewrite RetryWithHttpsEntryPoint.

Comment From: 2is10

For my company, ChannelProcessingFilter’s misuse of isCommitted() and short-circuiting of the filter chain results in missing log entries for many async requests.

Our request logging filter is based on Spring’s AbstractRequestLoggingFilter. It’s the next filter on the chain after Spring Security’s filter chain. During the ASYNC dispatch phase of async requests (endpoints that return a DeferredResult), our logging filter is never reached due to ChannelProcessingFilter aborting the entire filter chain for committed responses (even though channelDecisionManager had nothing to do with the response getting committed).

Here’s the problematic code in ChannelProcessingFilter again:

@Override
public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) … {
    …
    if (attributes != null) {
        this.logger.debug(…);
        this.channelDecisionManager.decide(filterInvocation, attributes);
        if (filterInvocation.getResponse().isCommitted()) {
            return; // ← THE PROBLEM
        }
    }
    chain.doFilter(request, response);
}

Looks like this issue has been waiting-for-triage for 4 years, 4 months. Will it ever be triaged?

Comment From: 2is10

@rwinch Could you assign this to someone to triage? The impact is pretty severe (makes Spring’s request logging filter unreliable), and this doesn’t seem difficult to fix. Thanks.