From ef77b4064fe0cc6da3986059cbeac3f471a8fd92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Wed, 9 Oct 2024 11:39:03 +0200 Subject: [PATCH] Keep DefaultServerHttpRequestBuilder-mutated headers case-insensitive This change avoids the trap of creating a copy of `HttpHeaders` using a case-sensitive `MultiValueMap` by mistake. Since mutability is always desirable, we make a mutable copy by using `addAll` on an empty `HttpHeaders`. We can't simply rely on HttpHeaders' map-based constructor to detect read-only header in this particular case, because the container's original headers representation might in some cases be read-only. Closes gh-33666 --- .../DefaultServerHttpRequestBuilder.java | 9 +- .../DefaultServerHttpRequestBuilderTests.java | 117 ++++++++++++++++++ 2 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 spring-web/src/test/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilderTests.java diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java b/spring-web/src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java index 59ae6eb3f5..fe02d443ba 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java @@ -30,7 +30,6 @@ import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; import org.springframework.lang.Nullable; import org.springframework.util.Assert; -import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import org.springframework.util.StringUtils; @@ -71,8 +70,12 @@ class DefaultServerHttpRequestBuilder implements ServerHttpRequest.Builder { Assert.notNull(original, "ServerHttpRequest is required"); this.uri = original.getURI(); - // original headers can be immutable, so create a copy - this.headers = new HttpHeaders(new LinkedMultiValueMap<>(original.getHeaders())); + // Some containers (including Jetty and Netty4) can have an immutable + // representation of headers. Since mutability is always desirable here, + // we always create a mutable case-insensitive copy of the original + // headers by using the basic constructor and addAll. + this.headers = new HttpHeaders(); + this.headers.addAll(original.getHeaders()); this.httpMethod = original.getMethod(); this.contextPath = original.getPath().contextPath().value(); this.remoteAddress = original.getRemoteAddress(); diff --git a/spring-web/src/test/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilderTests.java b/spring-web/src/test/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilderTests.java new file mode 100644 index 0000000000..f7621a5268 --- /dev/null +++ b/spring-web/src/test/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilderTests.java @@ -0,0 +1,117 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.http.server.reactive; + +import java.util.Locale; +import java.util.stream.Stream; + +import io.netty.handler.codec.http.DefaultHttpHeaders; +import io.netty.handler.codec.http.ReadOnlyHttpHeaders; +import io.undertow.util.HeaderMap; +import org.apache.tomcat.util.http.MimeHeaders; +import org.eclipse.jetty.http.HttpFields; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.BDDMockito; + +import org.springframework.http.HttpHeaders; +import org.springframework.http.support.JettyHeadersAdapter; +import org.springframework.http.support.Netty4HeadersAdapter; +import org.springframework.http.support.Netty5HeadersAdapter; +import org.springframework.util.CollectionUtils; +import org.springframework.util.LinkedCaseInsensitiveMap; +import org.springframework.util.MultiValueMap; +import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatRuntimeException; +import static org.junit.jupiter.params.provider.Arguments.argumentSet; +import static org.mockito.BDDMockito.when; + +class DefaultServerHttpRequestBuilderTests { + + @ParameterizedTest + @MethodSource("headers") + void containerImmutableHeadersAreCopied(MultiValueMap headerMap, boolean isMutableMap) { + HttpHeaders originalHeaders = new HttpHeaders(headerMap); + ServerHttpRequest mockRequest = createMockRequest(originalHeaders); + final DefaultServerHttpRequestBuilder builder = new DefaultServerHttpRequestBuilder(mockRequest); + + //perform mutations on the map adapter of the container's headers if possible + if (isMutableMap) { + headerMap.set("CaseInsensitive", "original"); + assertThat(originalHeaders.getFirst("caseinsensitive")) + .as("original mutated") + .isEqualTo("original"); + } + else { + assertThatRuntimeException().isThrownBy(() -> headerMap.set("CaseInsensitive", "original")); + assertThat(originalHeaders.getFirst("caseinsensitive")) + .as("original not mutable") + .isEqualTo("unmodified"); + } + + // Mutating the headers in the build. Note directly mutating via + // .build().getHeaders() isn't applicable since/ headers are made + // read-only by build() + ServerHttpRequest req = builder + .header("CaseInsensitive", "modified") + .header("Additional", "header") + .build(); + + assertThat(req.getHeaders().getFirst("CaseInsensitive")) + .as("copy mutated") + .isEqualTo("modified"); + assertThat(req.getHeaders().getFirst("caseinsensitive")) + .as("copy case-insensitive") + .isEqualTo("modified"); + assertThat(req.getHeaders().getFirst("additional")) + .as("copy has additional header") + .isEqualTo("header"); + } + + private ServerHttpRequest createMockRequest(HttpHeaders originalHeaders) { + //we can't use only use a MockServerHttpRequest because it uses a ReadOnlyHttpHeaders internally + ServerHttpRequest mock = BDDMockito.spy(MockServerHttpRequest.get("/example").build()); + when(mock.getHeaders()).thenReturn(originalHeaders); + + return mock; + } + + static Arguments initHeader(String description, MultiValueMap headerMap) { + headerMap.add("CaseInsensitive", "unmodified"); + return argumentSet(description, headerMap, true); + } + + static Stream headers() { + return Stream.of( + initHeader("Map", CollectionUtils.toMultiValueMap(new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH))), + initHeader("Netty", new Netty4HeadersAdapter(new DefaultHttpHeaders())), + initHeader("Netty5", new Netty5HeadersAdapter(io.netty5.handler.codec.http.headers.HttpHeaders.newHeaders())), + initHeader("Tomcat", new TomcatHeadersAdapter(new MimeHeaders())), + initHeader("Undertow", new UndertowHeadersAdapter(new HeaderMap())), + initHeader("Jetty", new JettyHeadersAdapter(HttpFields.build())), + //immutable versions of some headers + argumentSet("Netty immutable", new Netty4HeadersAdapter(new ReadOnlyHttpHeaders(false, + "CaseInsensitive", "unmodified")), false), + argumentSet("Jetty immutable", new JettyHeadersAdapter(HttpFields.build() + .add("CaseInsensitive", "unmodified").asImmutable()), false) + ); + } + +}