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 spring-projectsgh-13731

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) {

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;