-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Description
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:
Lines 45 to 46 in b935281
* {@link ChannelDecisionManager}. If a response is committed by the | |
* {@code ChannelDecisionManager}, the filter chain will not proceed. |
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:
Lines 150 to 153 in b935281
this.channelDecisionManager.decide(fi, attr); | |
if (fi.getResponse().isCommitted()) { | |
return; |
ChannelDecisionManager does a similar thing to determine if calls to ChannelProcessors resulted in a decision being made:
Lines 76 to 78 in b935281
processor.decide(invocation, config); | |
if (invocation.getResponse().isCommitted()) { |
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)