From 9900575f9c089ec3139673a712b79a415371bd14 Mon Sep 17 00:00:00 2001 From: spencergibb Date: Fri, 30 Jun 2023 16:32:26 -0400 Subject: [PATCH 1/2] Allow customization of disallowed JdkClientHttpRequest headers By default, the JDK HttpClient's HttpRequest does not allow Connection, Content-Length, Expect, Host, or Upgrade headers to be set, but this can be overriden with the `jdk.httpclient.allowRestrictedHeaders` system property. See https://bugs.openjdk.org/browse/JDK-8213696 Closes gh-30787 --- .../http/client/JdkClientHttpRequest.java | 20 +++++++++++-- .../JdkClientHttpRequestFactoryTests.java | 29 +++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/client/JdkClientHttpRequest.java b/spring-web/src/main/java/org/springframework/http/client/JdkClientHttpRequest.java index d098c164d19..7001df92a78 100644 --- a/spring-web/src/main/java/org/springframework/http/client/JdkClientHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/client/JdkClientHttpRequest.java @@ -25,7 +25,9 @@ import java.net.http.HttpRequest; import java.net.http.HttpResponse; import java.nio.ByteBuffer; import java.time.Duration; -import java.util.List; +import java.util.Collections; +import java.util.Set; +import java.util.TreeSet; import java.util.concurrent.Executor; import java.util.concurrent.Flow; @@ -33,6 +35,7 @@ import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; import org.springframework.lang.Nullable; import org.springframework.util.StreamUtils; +import org.springframework.util.StringUtils; /** * {@link ClientHttpRequest} implementation based the Java {@link HttpClient}. @@ -48,8 +51,19 @@ class JdkClientHttpRequest extends AbstractStreamingClientHttpRequest { * The JDK HttpRequest doesn't allow all headers to be set. The named headers are taken from the default * implementation for HttpRequest. */ - private static final List DISALLOWED_HEADERS = - List.of("connection", "content-length", "expect", "host", "upgrade"); + protected static final Set DISALLOWED_HEADERS = getDisallowedHeaders(); + + private static Set getDisallowedHeaders() { + TreeSet headers = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); + headers.addAll(Set.of("connection", "content-length", "expect", "host", "upgrade")); + + String headersToAllow = System.getProperty("jdk.httpclient.allowRestrictedHeaders"); + if (headersToAllow != null) { + Set toAllow = StringUtils.commaDelimitedListToSet(headersToAllow); + headers.removeAll(toAllow); + } + return Collections.unmodifiableSet(headers); + } private final HttpClient httpClient; diff --git a/spring-web/src/test/java/org/springframework/http/client/JdkClientHttpRequestFactoryTests.java b/spring-web/src/test/java/org/springframework/http/client/JdkClientHttpRequestFactoryTests.java index f882e2bfa47..d03fca2f77b 100644 --- a/spring-web/src/test/java/org/springframework/http/client/JdkClientHttpRequestFactoryTests.java +++ b/spring-web/src/test/java/org/springframework/http/client/JdkClientHttpRequestFactoryTests.java @@ -16,10 +16,17 @@ package org.springframework.http.client; +import java.net.URI; +import java.net.http.HttpClient; +import java.time.Duration; +import java.util.concurrent.Executor; + import org.junit.jupiter.api.Test; import org.springframework.http.HttpMethod; +import static org.assertj.core.api.Assertions.assertThat; + /** * @author Marten Deinum */ @@ -37,4 +44,26 @@ public class JdkClientHttpRequestFactoryTests extends AbstractHttpRequestFactory assertHttpMethod("patch", HttpMethod.PATCH); } + @Test + public void customizeDisallowedHeaders() { + String original = System.getProperty("jdk.httpclient.allowRestrictedHeaders"); + System.setProperty("jdk.httpclient.allowRestrictedHeaders", "host"); + + assertThat(TestJdkClientHttpRequest.DISALLOWED_HEADERS).doesNotContain("host"); + + if (original != null) { + System.setProperty("jdk.httpclient.allowRestrictedHeaders", original); + } + else { + System.clearProperty("jdk.httpclient.allowRestrictedHeaders"); + } + } + + static class TestJdkClientHttpRequest extends JdkClientHttpRequest { + + public TestJdkClientHttpRequest(HttpClient httpClient, URI uri, HttpMethod method, Executor executor, Duration readTimeout) { + super(httpClient, uri, method, executor, readTimeout); + } + } + } From 46f1849c2fa5b0a9183b473b5574de9783ae7a69 Mon Sep 17 00:00:00 2001 From: Arjen Poutsma Date: Mon, 3 Jul 2023 12:28:29 +0200 Subject: [PATCH 2/2] Polishing external contribution See gh-30787 Closes gh-30788 --- .../http/client/JdkClientHttpRequest.java | 24 +++++---- .../client/AbstractMockWebServerTests.java | 5 ++ .../JdkClientHttpRequestFactoryTests.java | 51 +++++++++++-------- 3 files changed, 48 insertions(+), 32 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/client/JdkClientHttpRequest.java b/spring-web/src/main/java/org/springframework/http/client/JdkClientHttpRequest.java index 7001df92a78..3743bb294aa 100644 --- a/spring-web/src/main/java/org/springframework/http/client/JdkClientHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/client/JdkClientHttpRequest.java @@ -47,13 +47,16 @@ import org.springframework.util.StringUtils; */ class JdkClientHttpRequest extends AbstractStreamingClientHttpRequest { - /* - * The JDK HttpRequest doesn't allow all headers to be set. The named headers are taken from the default - * implementation for HttpRequest. + private static final Set DISALLOWED_HEADERS = disallowedHeaders(); + + /** + * By default, {@link HttpRequest} does not allow {@code Connection}, + * {@code Content-Length}, {@code Expect}, {@code Host}, or {@code Upgrade} + * headers to be set, but this can be overriden with the + * {@code jdk.httpclient.allowRestrictedHeaders} system property. + * @see jdk.internal.net.http.common.Utils#getDisallowedHeaders() */ - protected static final Set DISALLOWED_HEADERS = getDisallowedHeaders(); - - private static Set getDisallowedHeaders() { + private static Set disallowedHeaders() { TreeSet headers = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); headers.addAll(Set.of("connection", "content-length", "expect", "host", "upgrade")); @@ -65,6 +68,7 @@ class JdkClientHttpRequest extends AbstractStreamingClientHttpRequest { return Collections.unmodifiableSet(headers); } + private final HttpClient httpClient; private final HttpMethod method; @@ -123,11 +127,9 @@ class JdkClientHttpRequest extends AbstractStreamingClientHttpRequest { } headers.forEach((headerName, headerValues) -> { - if (!headerName.equalsIgnoreCase(HttpHeaders.CONTENT_LENGTH)) { - if (!DISALLOWED_HEADERS.contains(headerName.toLowerCase())) { - for (String headerValue : headerValues) { - builder.header(headerName, headerValue); - } + if (!DISALLOWED_HEADERS.contains(headerName.toLowerCase())) { + for (String headerValue : headerValues) { + builder.header(headerName, headerValue); } } }); diff --git a/spring-web/src/test/java/org/springframework/http/client/AbstractMockWebServerTests.java b/spring-web/src/test/java/org/springframework/http/client/AbstractMockWebServerTests.java index cbef27287d4..1577c81e610 100644 --- a/spring-web/src/test/java/org/springframework/http/client/AbstractMockWebServerTests.java +++ b/spring-web/src/test/java/org/springframework/http/client/AbstractMockWebServerTests.java @@ -79,6 +79,11 @@ public abstract class AbstractMockWebServerTests { else if(request.getPath().equals("/status/notfound")) { return new MockResponse().setResponseCode(404); } + else if (request.getPath().equals("/status/299")) { + assertThat(request.getHeader("Expect")) + .contains("299"); + return new MockResponse().setResponseCode(299); + } else if(request.getPath().startsWith("/params")) { assertThat(request.getPath()).contains("param1=value"); assertThat(request.getPath()).contains("param2=value1¶m2=value2"); diff --git a/spring-web/src/test/java/org/springframework/http/client/JdkClientHttpRequestFactoryTests.java b/spring-web/src/test/java/org/springframework/http/client/JdkClientHttpRequestFactoryTests.java index d03fca2f77b..cce3a37989e 100644 --- a/spring-web/src/test/java/org/springframework/http/client/JdkClientHttpRequestFactoryTests.java +++ b/spring-web/src/test/java/org/springframework/http/client/JdkClientHttpRequestFactoryTests.java @@ -16,14 +16,16 @@ package org.springframework.http.client; +import java.io.IOException; import java.net.URI; -import java.net.http.HttpClient; -import java.time.Duration; -import java.util.concurrent.Executor; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatusCode; +import org.springframework.lang.Nullable; import static org.assertj.core.api.Assertions.assertThat; @@ -32,6 +34,25 @@ import static org.assertj.core.api.Assertions.assertThat; */ public class JdkClientHttpRequestFactoryTests extends AbstractHttpRequestFactoryTests { + @Nullable + private static String originalPropertyValue; + + @BeforeAll + public static void setProperty() { + originalPropertyValue = System.getProperty("jdk.httpclient.allowRestrictedHeaders"); + System.setProperty("jdk.httpclient.allowRestrictedHeaders", "expect"); + } + + @AfterAll + public static void restoreProperty() { + if (originalPropertyValue != null) { + System.setProperty("jdk.httpclient.allowRestrictedHeaders", originalPropertyValue); + } + else { + System.clearProperty("jdk.httpclient.allowRestrictedHeaders"); + } + } + @Override protected ClientHttpRequestFactory createRequestFactory() { return new JdkClientHttpRequestFactory(); @@ -45,25 +66,13 @@ public class JdkClientHttpRequestFactoryTests extends AbstractHttpRequestFactory } @Test - public void customizeDisallowedHeaders() { - String original = System.getProperty("jdk.httpclient.allowRestrictedHeaders"); - System.setProperty("jdk.httpclient.allowRestrictedHeaders", "host"); + public void customizeDisallowedHeaders() throws IOException { + ClientHttpRequest request = factory.createRequest(URI.create(this.baseUrl + "/status/299"), HttpMethod.PUT); + request.getHeaders().set("Expect", "299"); - assertThat(TestJdkClientHttpRequest.DISALLOWED_HEADERS).doesNotContain("host"); - - if (original != null) { - System.setProperty("jdk.httpclient.allowRestrictedHeaders", original); - } - else { - System.clearProperty("jdk.httpclient.allowRestrictedHeaders"); - } - } - - static class TestJdkClientHttpRequest extends JdkClientHttpRequest { - - public TestJdkClientHttpRequest(HttpClient httpClient, URI uri, HttpMethod method, Executor executor, Duration readTimeout) { - super(httpClient, uri, method, executor, readTimeout); - } + try (ClientHttpResponse response = request.execute()) { + assertThat(response.getStatusCode()).as("Invalid status code").isEqualTo(HttpStatusCode.valueOf(299)); + } } }