Lenient handling of malformed query in ServletServerHttpRequest

Closes gh-30489
spring-projects · Oct 9, 2024 · 1f4743a · 1f4743a
1 parent af85d19
commit 1f4743a
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 35 deletions.
diff --git a/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpRequest.java b/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpRequest.java
@@ -50,6 +50,7 @@
 import org.springframework.util.Assert;
 import org.springframework.util.LinkedCaseInsensitiveMap;
 import org.springframework.util.StringUtils;
+import org.springframework.web.util.UriComponentsBuilder;
 
 /**
  * {@link ServerHttpRequest} implementation that is based on a {@link HttpServletRequest}.
@@ -119,31 +120,37 @@ public URI getURI() {
 	 */
 	public static URI initURI(HttpServletRequest servletRequest) {
 		String urlString = null;
+		String query = null;
 		boolean hasQuery = false;
 		try {
-			StringBuffer url = servletRequest.getRequestURL();
-			String query = servletRequest.getQueryString();
+			StringBuffer requestURL = servletRequest.getRequestURL();
+			query = servletRequest.getQueryString();
 			hasQuery = StringUtils.hasText(query);
 			if (hasQuery) {
-				url.append('?').append(query);
+				requestURL.append('?').append(query);
 			}
-			urlString = url.toString();
+			urlString = requestURL.toString();
 			return new URI(urlString);
 		}
 		catch (URISyntaxException ex) {
-			if (!hasQuery) {
-				throw new IllegalStateException(
-						"Could not resolve HttpServletRequest as URI: " + urlString, ex);
-			}
-			// Maybe a malformed query string... try plain request URL
-			try {
-				urlString = servletRequest.getRequestURL().toString();
-				return new URI(urlString);
-			}
-			catch (URISyntaxException ex2) {
-				throw new IllegalStateException(
-						"Could not resolve HttpServletRequest as URI: " + urlString, ex2);
+			if (hasQuery) {
+				try {
+					// Maybe malformed query, try to parse and encode it
+					query = UriComponentsBuilder.fromUriString("?" + query).build().toUri().getRawQuery();
+					return new URI(servletRequest.getRequestURL().toString() + "?" + query);
+				}
+				catch (URISyntaxException ex2) {
+					try {
+						// Try leaving it out
+						return new URI(servletRequest.getRequestURL().toString());
+					}
+					catch (URISyntaxException ex3) {
+						// ignore
+					}
+				}
 			}
+			throw new IllegalStateException(
+					"Could not resolve HttpServletRequest as URI: " + urlString, ex);
 		}
 	}
 

diff --git a/...-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.java b/...-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2023 the original author or authors.
+ * Copyright 2002-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -51,6 +51,7 @@
 import org.springframework.util.LinkedMultiValueMap;
 import org.springframework.util.MultiValueMap;
 import org.springframework.util.StringUtils;
+import org.springframework.web.util.UriComponentsBuilder;
 
 /**
  * Adapt {@link ServerHttpRequest} to the Servlet {@link HttpServletRequest}.
@@ -90,8 +91,8 @@ public ServletServerHttpRequest(MultiValueMap<String, String> headers, HttpServl
 			AsyncContext asyncContext, String servletPath, DataBufferFactory bufferFactory, int bufferSize)
 			throws IOException, URISyntaxException {
 
-		super(HttpMethod.valueOf(request.getMethod()), initUri(request), request.getContextPath() + servletPath,
-				initHeaders(headers, request));
+		super(HttpMethod.valueOf(request.getMethod()), initUri(request),
+				request.getContextPath() + servletPath, initHeaders(headers, request));
 
 		Assert.notNull(bufferFactory, "'bufferFactory' must not be null");
 		Assert.isTrue(bufferSize > 0, "'bufferSize' must be greater than 0");
@@ -121,14 +122,42 @@ private static MultiValueMap<String, String> createDefaultHttpHeaders(HttpServle
 		return headers;
 	}
 
-	private static URI initUri(HttpServletRequest request) throws URISyntaxException {
-		Assert.notNull(request, "'request' must not be null");
-		StringBuffer url = request.getRequestURL();
-		String query = request.getQueryString();
-		if (StringUtils.hasText(query)) {
-			url.append('?').append(query);
+	@SuppressWarnings("JavaExistingMethodCanBeUsed")
+	private static URI initUri(HttpServletRequest servletRequest) {
+		Assert.notNull(servletRequest, "'request' must not be null");
+		String urlString = null;
+		String query = null;
+		boolean hasQuery = false;
+		try {
+			StringBuffer requestURL = servletRequest.getRequestURL();
+			query = servletRequest.getQueryString();
+			hasQuery = StringUtils.hasText(query);
+			if (hasQuery) {
+				requestURL.append('?').append(query);
+			}
+			urlString = requestURL.toString();
+			return new URI(urlString);
+		}
+		catch (URISyntaxException ex) {
+			if (hasQuery) {
+				try {
+					// Maybe malformed query, try to parse and encode it
+					query = UriComponentsBuilder.fromUriString("?" + query).build().toUri().getRawQuery();
+					return new URI(servletRequest.getRequestURL().toString() + "?" + query);
+				}
+				catch (URISyntaxException ex2) {
+					try {
+						// Try leaving it out
+						return new URI(servletRequest.getRequestURL().toString());
+					}
+					catch (URISyntaxException ex3) {
+						// ignore
+					}
+				}
+			}
+			throw new IllegalStateException(
+					"Could not resolve HttpServletRequest as URI: " + urlString, ex);
 		}
-		return new URI(url.toString());
 	}
 
 	@SuppressWarnings("NullAway")

diff --git a/spring-web/src/test/java/org/springframework/http/server/ServletServerHttpRequestTests.java b/spring-web/src/test/java/org/springframework/http/server/ServletServerHttpRequestTests.java
@@ -23,6 +23,8 @@
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 
 import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpMethod;
@@ -31,6 +33,7 @@
 import org.springframework.web.testfixture.servlet.MockHttpServletRequest;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
 
 /**
  * @author Arjen Poutsma
@@ -78,24 +81,28 @@ void getUriWithQueryString() {
 		assertThat(request.getURI()).isEqualTo(uri);
 	}
 
-	@Test  // SPR-16414
-	void getUriWithQueryParam() {
+	// gh-20960
+	@ParameterizedTest(name = "{displayName}({arguments})")
+	@CsvSource(delimiter='|', value = {
+			"query=foo    | ?query=foo",
+			"query=foo%%x | ?query=foo%25%25x"
+	})
+	void getUriWithMalformedQueryParam(String inputQuery, String expectedQuery) {
 		mockRequest.setScheme("https");
 		mockRequest.setServerPort(443);
 		mockRequest.setServerName("example.com");
 		mockRequest.setRequestURI("/path");
-		mockRequest.setQueryString("query=foo");
-		assertThat(request.getURI()).isEqualTo(URI.create("https://example.com/path?query=foo"));
+		mockRequest.setQueryString(inputQuery);
+		assertThat(request.getURI()).isEqualTo(URI.create("https://example.com/path" + expectedQuery));
 	}
 
-	@Test  // SPR-16414
-	void getUriWithMalformedQueryParam() {
+	@Test
+	void getUriWithMalformedPath() {
 		mockRequest.setScheme("https");
 		mockRequest.setServerPort(443);
 		mockRequest.setServerName("example.com");
-		mockRequest.setRequestURI("/path");
-		mockRequest.setQueryString("query=foo%%x");
-		assertThat(request.getURI()).isEqualTo(URI.create("https://example.com/path"));
+		mockRequest.setRequestURI("/p%th");
+		assertThatIllegalStateException().isThrownBy(() -> request.getURI());
 	}
 
 	@Test  // SPR-13876