Fix port handling in HtmlUnitRequestBuilder

Prior to this commit, HtmlUnitRequestBuilder set the server port in the
MockHttpServletRequest to -1 if the URL did not contain an explicit
port. However, that can lead to errors in consumers of the request that
do not expect an invalid port number.

In addition, HtmlUnitRequestBuilder always set the remote port in the
MockHttpServletRequest to the value of the server port, which does not
make sense, since the remote port of the client has nothing to do with
the port on the server.

To address those issues, this commit revises HtmlUnitRequestBuilder so
that it:

- Does not set the server and local ports if the explicit or derived
  default value is -1.
- Consistently sets the server and local ports to the same valid value.
- Does not set the remote port.

Closes gh-35709

Author: sbrannen

spring-test/src/main/java/org/springframework/test/web/servlet/htmlunit/HtmlUnitRequestBuilder.java

@@ -211,14 +211,12 @@ private void parent(MockHttpServletRequest request, @Nullable RequestBuilder par
 
 	private void ports(UriComponents uriComponents, MockHttpServletRequest request) {
 		int serverPort = uriComponents.getPort();
-		request.setServerPort(serverPort);
 		if (serverPort == -1) {
-			int portConnection = this.webRequest.getUrl().getDefaultPort();
-			request.setLocalPort(serverPort);
-			request.setRemotePort(portConnection);
+			serverPort = this.webRequest.getUrl().getDefaultPort();
 		}
-		else {
-			request.setRemotePort(serverPort);
+		if (serverPort != -1) {
+			request.setServerPort(serverPort);
+			request.setLocalPort(serverPort);
 		}
 	}
 

spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/HtmlUnitRequestBuilderTests.java

@@ -26,6 +26,7 @@
 
 import jakarta.servlet.ServletContext;
 import jakarta.servlet.http.Cookie;
+import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpSession;
 import org.apache.commons.io.IOUtils;
 import org.apache.http.auth.UsernamePasswordCredentials;
@@ -339,22 +340,6 @@ void buildRequestLocalName() {
 		assertThat(actualRequest.getLocalName()).isEqualTo("localhost");
 	}
 
-	@Test
-	void buildRequestLocalPortMatchingDefault() throws Exception {
-		webRequest.setUrl(new URL("http://localhost:80/test/this/here"));
-		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
-
-		assertThat(actualRequest.getLocalPort()).isEqualTo(-1);
-	}
-
-	@Test
-	void buildRequestLocalMissing() throws Exception {
-		webRequest.setUrl(new URL("http://localhost/test/this"));
-		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
-
-		assertThat(actualRequest.getLocalPort()).isEqualTo(-1);
-	}
-
 	@Test
 	void buildRequestMethods() {
 		for (HttpMethod expectedMethod : HttpMethod.values()) {
@@ -534,71 +519,94 @@ void buildRequestReader() throws Exception {
 	}
 
 	@Test
-	void buildRequestRemoteAddr() {
+	void buildRequestRemoteAddressHostAndPort() {
 		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
 
 		assertThat(actualRequest.getRemoteAddr()).isEqualTo("127.0.0.1");
+		assertThat(actualRequest.getRemoteHost()).isEqualTo("localhost");
+		assertThat(actualRequest.getRemotePort()).isEqualTo(80);
 	}
 
 	@Test
-	void buildRequestRemoteHost() {
+	void buildRequestRequestedSessionId() {
+		String sessionId = "session-id";
+		webRequest.setAdditionalHeader("Cookie", "JSESSIONID=" + sessionId);
 		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
 
-		assertThat(actualRequest.getRemoteAddr()).isEqualTo("127.0.0.1");
+		assertThat(actualRequest.getRequestedSessionId()).isEqualTo(sessionId);
 	}
 
 	@Test
-	void buildRequestRemotePort() throws Exception {
-		webRequest.setUrl(new URL("http://localhost:80/test/this/here"));
+	void buildRequestRequestedSessionIdNull() {
 		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
 
-		assertThat(actualRequest.getRemotePort()).isEqualTo(80);
+		assertThat(actualRequest.getRequestedSessionId()).isNull();
 	}
 
 	@Test
-	void buildRequestRemotePort8080() throws Exception {
-		webRequest.setUrl(new URL("https://example.com:8080/"));
+	void buildRequestUri() {
+		String uri = requestBuilder.buildRequest(servletContext).getRequestURI();
+		assertThat(uri).isEqualTo("/test/this/here");
+	}
 
-		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+	@Test
+	void buildRequestWithSchemeHttpAndDefaultPort() throws Exception {
+		webRequest.setUrl(new URL("http://localhost/test"));
+		var request = requestBuilder.buildRequest(servletContext);
 
-		assertThat(actualRequest.getRemotePort()).isEqualTo(8080);
+		assertUrlAndPorts(request, "http://localhost/test", 80, false);
 	}
 
 	@Test
-	void buildRequestRemotePort80WithDefault() throws Exception {
-		webRequest.setUrl(new URL("http://company.example/"));
-
-		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+	void buildRequestWithSchemeHttpAndExplicitDefaultPort() throws Exception {
+		webRequest.setUrl(new URL("http://localhost:80/test"));
+		var request = requestBuilder.buildRequest(servletContext);
 
-		assertThat(actualRequest.getRemotePort()).isEqualTo(80);
+		assertUrlAndPorts(request, "http://localhost/test", 80, false);
 	}
 
 	@Test
-	void buildRequestRequestedSessionId() {
-		String sessionId = "session-id";
-		webRequest.setAdditionalHeader("Cookie", "JSESSIONID=" + sessionId);
-		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+	void buildRequestWithSchemeHttpAndExplicitPort() throws Exception {
+		webRequest.setUrl(new URL("http://localhost:8081/test"));
+		var request = requestBuilder.buildRequest(servletContext);
 
-		assertThat(actualRequest.getRequestedSessionId()).isEqualTo(sessionId);
+		assertUrlAndPorts(request, "http://localhost:8081/test", 8081, false);
+
+		// Unlikely scheme/port combination:
+		webRequest.setUrl(new URL("http://localhost:443/test"));
+		request = requestBuilder.buildRequest(servletContext);
+
+		assertUrlAndPorts(request, "http://localhost:443/test", 443, false);
 	}
 
 	@Test
-	void buildRequestRequestedSessionIdNull() {
-		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+	void buildRequestWithSchemeHttpsAndDefaultPort() throws Exception {
+		webRequest.setUrl(new URL("https://localhost/test"));
+		var request = requestBuilder.buildRequest(servletContext);
 
-		assertThat(actualRequest.getRequestedSessionId()).isNull();
+		assertUrlAndPorts(request, "https://localhost/test", 443, true);
 	}
 
 	@Test
-	void buildRequestUri() {
-		String uri = requestBuilder.buildRequest(servletContext).getRequestURI();
-		assertThat(uri).isEqualTo("/test/this/here");
+	void buildRequestWithSchemeHttpsAndExplicitDefaultPort() throws Exception {
+		webRequest.setUrl(new URL("https://localhost:443/test"));
+		var request = requestBuilder.buildRequest(servletContext);
+
+		assertUrlAndPorts(request, "https://localhost/test", 443, true);
 	}
 
 	@Test
-	void buildRequestUrl() {
-		String uri = requestBuilder.buildRequest(servletContext).getRequestURL().toString();
-		assertThat(uri).isEqualTo("https://example.com/test/this/here");
+	void buildRequestWithSchemeHttpsAndExplicitPort() throws Exception {
+		webRequest.setUrl(new URL("https://localhost:8443/test"));
+		var request = requestBuilder.buildRequest(servletContext);
+
+		assertUrlAndPorts(request, "https://localhost:8443/test", 8443, true);
+
+		// Unlikely scheme/port combination:
+		webRequest.setUrl(new URL("https://localhost:80/test"));
+		request = requestBuilder.buildRequest(servletContext);
+
+		assertUrlAndPorts(request, "https://localhost:80/test", 80, true);
 	}
 
 	@Test
@@ -624,30 +632,6 @@ void buildRequestServerName() {
 		assertThat(actualRequest.getServerName()).isEqualTo("example.com");
 	}
 
-	@Test
-	void buildRequestServerPort() throws Exception {
-		webRequest.setUrl(new URL("http://localhost:8080/test/this/here"));
-		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
-
-		assertThat(actualRequest.getServerPort()).isEqualTo(8080);
-	}
-
-	@Test
-	void buildRequestServerPortMatchingDefault() throws Exception {
-		webRequest.setUrl(new URL("http://localhost/test/this/here"));
-		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
-
-		assertThat(actualRequest.getServerPort()).isEqualTo(-1);
-	}
-
-	@Test
-	void buildRequestServerPortDefault() throws Exception {
-		webRequest.setUrl(new URL("https://example.com/"));
-		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
-
-		assertThat(actualRequest.getServerPort()).isEqualTo(-1);
-	}
-
 	@Test
 	void buildRequestServletContext() {
 		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
@@ -910,4 +894,21 @@ private String getContextPath() {
 		return (String) ReflectionTestUtils.getField(requestBuilder, "contextPath");
 	}
 
+	private static void assertUrlAndPorts(HttpServletRequest request, String url, int serverPort, boolean secure) {
+		assertThat(request.getRequestURL()).asString().as("url").isEqualTo(url);
+		assertThat(request.getServerPort()).as("server port").isEqualTo(serverPort);
+		// In a mocked request, the local post is always the same as the server port.
+		assertThat(request.getLocalPort()).as("local port").isEqualTo(serverPort);
+		// Remote Port is always 80 (MockHttpServletRequest.DEFAULT_SERVER_PORT),
+		// since a mocked request does not influence the remote host, port, or address.
+		assertThat(request.getRemotePort()).as("remote port").isEqualTo(80);
+
+		if (secure) {
+			assertThat(request.isSecure()).as("secure").isTrue();
+		}
+		else {
+			assertThat(request.isSecure()).as("secure").isFalse();
+		}
+	}
+
 }