From 593c126c03642f5afdd7cb162f6690e9439b4082 Mon Sep 17 00:00:00 2001 From: Vedran Pavic Date: Mon, 15 Jul 2019 23:55:36 +0200 Subject: [PATCH] Filtering for nested ERROR dispatch Resolves: #1470 See: spring-projects/spring-framework#23196 --- .../web/http/OncePerRequestFilter.java | 48 ++++++++++++++----- .../web/http/SessionRepositoryFilter.java | 6 +++ .../web/http/OncePerRequestFilterTests.java | 33 +++++++++++++ 3 files changed, 76 insertions(+), 11 deletions(-) diff --git a/spring-session-core/src/main/java/org/springframework/session/web/http/OncePerRequestFilter.java b/spring-session-core/src/main/java/org/springframework/session/web/http/OncePerRequestFilter.java index 8b0ff0f9..e793b90d 100644 --- a/spring-session-core/src/main/java/org/springframework/session/web/http/OncePerRequestFilter.java +++ b/spring-session-core/src/main/java/org/springframework/session/web/http/OncePerRequestFilter.java @@ -65,11 +65,13 @@ abstract class OncePerRequestFilter implements Filter { HttpServletRequest httpRequest = (HttpServletRequest) request; HttpServletResponse httpResponse = (HttpServletResponse) response; String alreadyFilteredAttributeName = this.alreadyFilteredAttributeName; - alreadyFilteredAttributeName = updateForErrorDispatch(alreadyFilteredAttributeName, request); boolean hasAlreadyFilteredAttribute = request.getAttribute(alreadyFilteredAttributeName) != null; if (hasAlreadyFilteredAttribute) { - + if (DispatcherType.ERROR.equals(request.getDispatcherType())) { + doFilterNestedErrorDispatch(httpRequest, httpResponse, filterChain); + return; + } // Proceed without invoking this filter... filterChain.doFilter(request, response); } @@ -86,15 +88,39 @@ abstract class OncePerRequestFilter implements Filter { } } - private String updateForErrorDispatch(String alreadyFilteredAttributeName, ServletRequest request) { - // Jetty does ERROR dispatch within sendError, so request attribute is still - // present - // Use a separate attribute for ERROR dispatches - if (DispatcherType.ERROR.equals(request.getDispatcherType()) - && request.getAttribute(alreadyFilteredAttributeName) != null) { - return alreadyFilteredAttributeName + ".ERROR"; - } - return alreadyFilteredAttributeName; + /** + * Return the name of the request attribute that identifies that a request is already + * filtered. + *

+ * The default implementation takes the configured name of the concrete filter + * instance and appends ".FILTERED". If the filter is not fully initialized, it falls + * back to its class name. + * @return the name of request attribute indicating already filtered request + * @see #ALREADY_FILTERED_SUFFIX + */ + protected String getAlreadyFilteredAttributeName() { + return this.alreadyFilteredAttributeName; + } + + /** + * Typically an ERROR dispatch happens after the REQUEST dispatch completes, and the + * filter chain starts anew. On some servers however the ERROR dispatch may be nested + * within the REQUEST dispatch, e.g. as a result of calling {@code sendError} on the + * response. In that case we are still in the filter chain, on the same thread, but + * the request and response have been switched to the original, unwrapped ones. + *

+ * Sub-classes may use this method to filter such nested ERROR dispatches and re-apply + * wrapping on the request or response. {@code ThreadLocal} context, if any, should + * still be active as we are still nested within the filter chain. + * @param request the request + * @param response the response + * @param filterChain the filter chain + * @throws ServletException if request is not HTTP request + * @throws IOException in case of I/O operation exception + */ + protected void doFilterNestedErrorDispatch(HttpServletRequest request, HttpServletResponse response, + FilterChain filterChain) throws ServletException, IOException { + doFilter(request, response, filterChain); } /** diff --git a/spring-session-core/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java b/spring-session-core/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java index d0350399..ff325a54 100644 --- a/spring-session-core/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java +++ b/spring-session-core/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java @@ -145,6 +145,12 @@ public class SessionRepositoryFilter extends OncePerRequestFi } } + @Override + protected void doFilterNestedErrorDispatch(HttpServletRequest request, HttpServletResponse response, + FilterChain filterChain) throws ServletException, IOException { + doFilterInternal(request, response, filterChain); + } + /** * Allows ensuring that the session is saved if the response is committed. * diff --git a/spring-session-core/src/test/java/org/springframework/session/web/http/OncePerRequestFilterTests.java b/spring-session-core/src/test/java/org/springframework/session/web/http/OncePerRequestFilterTests.java index c486eea2..00823fb5 100644 --- a/spring-session-core/src/test/java/org/springframework/session/web/http/OncePerRequestFilterTests.java +++ b/spring-session-core/src/test/java/org/springframework/session/web/http/OncePerRequestFilterTests.java @@ -20,6 +20,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.List; +import javax.servlet.DispatcherType; import javax.servlet.FilterChain; import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; @@ -32,6 +33,7 @@ import org.junit.jupiter.api.Test; import org.springframework.mock.web.MockFilterChain; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.web.util.WebUtils; import static org.assertj.core.api.Assertions.assertThat; @@ -97,4 +99,35 @@ class OncePerRequestFilterTests { assertThat(this.invocations).containsOnly(this.filter, filter2); } + @Test // gh-1470 + void filterNestedErrorDispatch() throws ServletException, IOException { + TestOncePerRequestFilter filter = new TestOncePerRequestFilter(); + this.request.setAttribute(filter.getAlreadyFilteredAttributeName(), Boolean.TRUE); + this.request.setDispatcherType(DispatcherType.ERROR); + this.request.setAttribute(WebUtils.ERROR_REQUEST_URI_ATTRIBUTE, "/error"); + filter.doFilter(this.request, new MockHttpServletResponse(), this.chain); + assertThat(filter.didFilter).isFalse(); + assertThat(filter.didFilterNestedErrorDispatch).isTrue(); + } + + private static class TestOncePerRequestFilter extends OncePerRequestFilter { + + private boolean didFilter; + + private boolean didFilterNestedErrorDispatch; + + @Override + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, + FilterChain filterChain) { + this.didFilter = true; + } + + @Override + protected void doFilterNestedErrorDispatch(HttpServletRequest request, HttpServletResponse response, + FilterChain filterChain) { + this.didFilterNestedErrorDispatch = true; + } + + } + }