From 18e88366d284a1edfc2936081cd971439e14a743 Mon Sep 17 00:00:00 2001 From: Marcus Da Coregio Date: Thu, 14 Sep 2023 17:09:56 +0100 Subject: [PATCH] Resolve The matchingRequestParameterName From The Query String Prior to this commit, the ServletRequest#getParameter method was used in order to verify if the matchingRequestParameterName was present in the request. That method has some side effects like interfering in the execution of the ServletRequest#getInputStream and ServletRequest#getReader method when the request is an HTTP POST (if those methods are invoked after getParameter, or vice-versa, the content won't be available). This commit makes that we only use the query string to check for the parameter, avoiding draining the request's input stream. Closes gh-13731 --- .../savedrequest/HttpSessionRequestCache.java | 15 ++++++++---- .../HttpSessionRequestCacheTests.java | 24 +++++++++++++++---- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/savedrequest/HttpSessionRequestCache.java b/web/src/main/java/org/springframework/security/web/savedrequest/HttpSessionRequestCache.java index 54493d465aa..1e4a7dcb60f 100644 --- a/web/src/main/java/org/springframework/security/web/savedrequest/HttpSessionRequestCache.java +++ b/web/src/main/java/org/springframework/security/web/savedrequest/HttpSessionRequestCache.java @@ -28,6 +28,8 @@ import org.springframework.security.web.util.UrlUtils; import org.springframework.security.web.util.matcher.AnyRequestMatcher; import org.springframework.security.web.util.matcher.RequestMatcher; +import org.springframework.util.StringUtils; +import org.springframework.web.util.UriComponentsBuilder; /** * {@code RequestCache} which stores the {@code SavedRequest} in the HttpSession. @@ -100,11 +102,14 @@ public void removeRequest(HttpServletRequest currentRequest, HttpServletResponse @Override public HttpServletRequest getMatchingRequest(HttpServletRequest request, HttpServletResponse response) { - if (this.matchingRequestParameterName != null - && request.getParameter(this.matchingRequestParameterName) == null) { - this.logger.trace( - "matchingRequestParameterName is required for getMatchingRequest to lookup a value, but not provided"); - return null; + if (this.matchingRequestParameterName != null) { + if (!StringUtils.hasText(request.getQueryString()) + || !UriComponentsBuilder.fromUriString(UrlUtils.buildRequestUrl(request)).build().getQueryParams() + .containsKey(this.matchingRequestParameterName)) { + this.logger.trace( + "matchingRequestParameterName is required for getMatchingRequest to lookup a value, but not provided"); + return null; + } } SavedRequest saved = getRequest(request, response); if (saved == null) { diff --git a/web/src/test/java/org/springframework/security/web/savedrequest/HttpSessionRequestCacheTests.java b/web/src/test/java/org/springframework/security/web/savedrequest/HttpSessionRequestCacheTests.java index 9361bec6378..35c7acfc86e 100644 --- a/web/src/test/java/org/springframework/security/web/savedrequest/HttpSessionRequestCacheTests.java +++ b/web/src/test/java/org/springframework/security/web/savedrequest/HttpSessionRequestCacheTests.java @@ -32,8 +32,9 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.anyBoolean; -import static org.mockito.Mockito.mock; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; /** @@ -100,7 +101,7 @@ public void testCustomSessionAttrName() { public void getMatchingRequestWhenMatchingRequestParameterNameSetThenSessionNotAccessed() { HttpSessionRequestCache cache = new HttpSessionRequestCache(); cache.setMatchingRequestParameterName("success"); - HttpServletRequest request = mock(HttpServletRequest.class); + HttpServletRequest request = spy(new MockHttpServletRequest()); HttpServletRequest matchingRequest = cache.getMatchingRequest(request, new MockHttpServletResponse()); assertThat(matchingRequest).isNull(); verify(request, never()).getSession(); @@ -115,7 +116,6 @@ public void getMatchingRequestWhenMatchingRequestParameterNameSetAndParameterExi cache.saveRequest(request, new MockHttpServletResponse()); MockHttpServletRequest requestToMatch = new MockHttpServletRequest(); requestToMatch.setQueryString("success"); // gh-12665 - requestToMatch.setParameter("success", ""); requestToMatch.setSession(request.getSession()); HttpServletRequest matchingRequest = cache.getMatchingRequest(requestToMatch, new MockHttpServletResponse()); assertThat(matchingRequest).isNotNull(); @@ -131,7 +131,6 @@ public void getMatchingRequestWhenMatchingRequestParameterNameSetAndParameterExi cache.saveRequest(request, new MockHttpServletResponse()); MockHttpServletRequest requestToMatch = new MockHttpServletRequest(); requestToMatch.setQueryString("param=true&success"); - requestToMatch.setParameter("success", ""); requestToMatch.setSession(request.getSession()); HttpServletRequest matchingRequest = cache.getMatchingRequest(requestToMatch, new MockHttpServletResponse()); assertThat(matchingRequest).isNotNull(); @@ -146,13 +145,28 @@ public void getMatchingRequestWhenMatchesThenRemoved() { assertThat(request.getSession().getAttribute(HttpSessionRequestCache.SAVED_REQUEST)).isNotNull(); MockHttpServletRequest requestToMatch = new MockHttpServletRequest(); requestToMatch.setQueryString("success"); - requestToMatch.setParameter("success", ""); requestToMatch.setSession(request.getSession()); HttpServletRequest matchingRequest = cache.getMatchingRequest(requestToMatch, new MockHttpServletResponse()); assertThat(matchingRequest).isNotNull(); assertThat(request.getSession().getAttribute(HttpSessionRequestCache.SAVED_REQUEST)).isNull(); } + // gh-13731 + @Test + public void getMatchingRequestWhenMatchingRequestParameterNameSetThenDoesNotInvokeGetParameterMethods() { + HttpSessionRequestCache cache = new HttpSessionRequestCache(); + cache.setMatchingRequestParameterName("success"); + MockHttpServletRequest mockRequest = new MockHttpServletRequest(); + mockRequest.setQueryString("success"); + HttpServletRequest request = spy(mockRequest); + HttpServletRequest matchingRequest = cache.getMatchingRequest(request, new MockHttpServletResponse()); + assertThat(matchingRequest).isNull(); + verify(request, never()).getParameter(anyString()); + verify(request, never()).getParameterValues(anyString()); + verify(request, never()).getParameterNames(); + verify(request, never()).getParameterMap(); + } + private static final class CustomSavedRequest implements SavedRequest { private final SavedRequest delegate;