From 5342c6e493395f9b986d1144bceff99b3ee8c6bd Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Mon, 19 Nov 2018 15:33:06 +0100 Subject: [PATCH] Remove Content-Length response header from errors Prior to this commit, when errors happened before the response was committed, the `Content-Length` response header would be left as is. This can be problematic since the error can be handled later in the chain and the response body changed accordingly. For example, Spring Boot renders error pages in those cases. If the `Content-Length` is set, HTTP clients can get confused and only consider part of the error response body. This commit ensures that any `Content-Length` response header is removed in case of errors, if the response is not already committed. This is done at the `AbstractServerHttpResponse` level, since errors can be handled in multiple places and the response itself is the safest place to handle this case. As a consequence, this commit also removes `Content-Length` checks in `EncoderHttpMessageWriter` since we now consider that we should rely on the response body we're about to write rather than any previously set value. Issue: SPR-17502 (Cherry-picked from 3203d39821) --- .../http/codec/EncoderHttpMessageWriter.java | 3 +-- .../server/reactive/AbstractServerHttpResponse.java | 13 +++++++++++-- .../server/reactive/ServerHttpResponseTests.java | 3 +++ 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/codec/EncoderHttpMessageWriter.java b/spring-web/src/main/java/org/springframework/http/codec/EncoderHttpMessageWriter.java index 9ccca39bca..41315265ad 100644 --- a/spring-web/src/main/java/org/springframework/http/codec/EncoderHttpMessageWriter.java +++ b/spring-web/src/main/java/org/springframework/http/codec/EncoderHttpMessageWriter.java @@ -28,7 +28,6 @@ import reactor.core.publisher.Mono; import org.springframework.core.ResolvableType; import org.springframework.core.codec.Encoder; import org.springframework.core.io.buffer.DataBuffer; -import org.springframework.core.io.buffer.DataBufferFactory; import org.springframework.http.HttpHeaders; import org.springframework.http.MediaType; import org.springframework.http.ReactiveHttpOutputMessage; @@ -106,7 +105,7 @@ public class EncoderHttpMessageWriter implements HttpMessageWriter { if (inputStream instanceof Mono) { HttpHeaders headers = message.getHeaders(); - if (headers.getContentLength() < 0 && !headers.containsKey(HttpHeaders.TRANSFER_ENCODING)) { + if (!headers.containsKey(HttpHeaders.TRANSFER_ENCODING)) { return Mono.from(body) .defaultIfEmpty(message.bufferFactory().wrap(new byte[0])) .flatMap(buffer -> { diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java index ec6d5aaf94..49a1267320 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java @@ -45,6 +45,7 @@ import org.springframework.util.MultiValueMap; * @author Rossen Stoyanchev * @author Juergen Hoeller * @author Sebastien Deleuze + * @author Brian Clozel * @since 5.0 */ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { @@ -173,13 +174,21 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { @Override public final Mono writeWith(Publisher body) { return new ChannelSendOperator<>(body, - writePublisher -> doCommit(() -> writeWithInternal(writePublisher))); + writePublisher -> doCommit(() -> writeWithInternal(writePublisher))) + .doOnError(t -> removeContentLength()); } @Override public final Mono writeAndFlushWith(Publisher> body) { return new ChannelSendOperator<>(body, - writePublisher -> doCommit(() -> writeAndFlushWithInternal(writePublisher))); + writePublisher -> doCommit(() -> writeAndFlushWithInternal(writePublisher))) + .doOnError(t -> removeContentLength()); + } + + private void removeContentLength() { + if (!this.isCommitted()) { + this.getHeaders().remove(HttpHeaders.CONTENT_LENGTH); + } } @Override diff --git a/spring-web/src/test/java/org/springframework/http/server/reactive/ServerHttpResponseTests.java b/spring-web/src/test/java/org/springframework/http/server/reactive/ServerHttpResponseTests.java index 651e9c55bd..55746952ca 100644 --- a/spring-web/src/test/java/org/springframework/http/server/reactive/ServerHttpResponseTests.java +++ b/spring-web/src/test/java/org/springframework/http/server/reactive/ServerHttpResponseTests.java @@ -29,6 +29,7 @@ import reactor.core.publisher.Mono; import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.io.buffer.DefaultDataBuffer; import org.springframework.core.io.buffer.DefaultDataBufferFactory; +import org.springframework.http.HttpHeaders; import org.springframework.http.ResponseCookie; import static junit.framework.TestCase.assertTrue; @@ -75,12 +76,14 @@ public class ServerHttpResponseTests { @Test public void writeWithError() throws Exception { TestServerHttpResponse response = new TestServerHttpResponse(); + response.getHeaders().setContentLength(12); IllegalStateException error = new IllegalStateException("boo"); response.writeWith(Flux.error(error)).onErrorResume(ex -> Mono.empty()).block(); assertFalse(response.statusCodeWritten); assertFalse(response.headersWritten); assertFalse(response.cookiesWritten); + assertFalse(response.getHeaders().containsKey(HttpHeaders.CONTENT_LENGTH)); assertTrue(response.body.isEmpty()); }