From f9c1565f4ef031904c3545a70e63080aeade6e90 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Mon, 6 Jan 2020 16:39:14 +0100 Subject: [PATCH] Remove Content-* response headers for error handling Prior to this commit, when WebFlux handlers added `"Content-*"` response headers and an error happened while handling the request, all those headers would not be cleared from the response before error handling. This commit clears those headers from the response in two places: * when invoking the handler and adapting the response * when writing the response body Not removing those headers might break HTTP clients since they're given wrong information about how to interpret the HTTP response body: the error response body might be very different from the original one. Fixes gh-24238 --- .../org/springframework/http/HttpHeaders.java | 18 ++++++++- .../reactive/AbstractServerHttpResponse.java | 13 ++++--- .../reactive/ServerHttpResponseTests.java | 39 +++++++++++++------ .../RequestMappingHandlerAdapter.java | 4 +- 4 files changed, 53 insertions(+), 21 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/HttpHeaders.java b/spring-web/src/main/java/org/springframework/http/HttpHeaders.java index da4ffac8cfe9..68e693375d34 100644 --- a/spring-web/src/main/java/org/springframework/http/HttpHeaders.java +++ b/spring-web/src/main/java/org/springframework/http/HttpHeaders.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -1827,6 +1827,22 @@ public static String encodeBasicAuth(String username, String password, @Nullable return new String(encodedBytes, charset); } + /** + * Remove the well-known {@code "Content-*"} HTTP headers from the given instance. + *

Such headers should be cleared, if possible, from the response if the intended + * body can't be written due to errors. + * @since 5.2.3 + */ + public static void clearContentHeaders(HttpHeaders headers) { + headers.remove(HttpHeaders.CONTENT_DISPOSITION); + headers.remove(HttpHeaders.CONTENT_ENCODING); + headers.remove(HttpHeaders.CONTENT_LANGUAGE); + headers.remove(HttpHeaders.CONTENT_LENGTH); + headers.remove(HttpHeaders.CONTENT_LOCATION); + headers.remove(HttpHeaders.CONTENT_RANGE); + headers.remove(HttpHeaders.CONTENT_TYPE); + } + // Package-private: used in ResponseCookie static String formatDate(long date) { Instant instant = Instant.ofEpochMilli(date); 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 d5378f058e25..e53366e27643 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -181,21 +181,22 @@ public final Mono writeWith(Publisher body) { if (body instanceof Mono) { return ((Mono) body).flatMap(buffer -> doCommit(() -> writeWithInternal(Mono.just(buffer))) - .doOnDiscard(PooledDataBuffer.class, DataBufferUtils::release)); + .doOnDiscard(PooledDataBuffer.class, DataBufferUtils::release)) + .doOnError(t -> clearContentHeaders()); } return new ChannelSendOperator<>(body, inner -> doCommit(() -> writeWithInternal(inner))) - .doOnError(t -> removeContentLength()); + .doOnError(t -> clearContentHeaders()); } @Override public final Mono writeAndFlushWith(Publisher> body) { return new ChannelSendOperator<>(body, inner -> doCommit(() -> writeAndFlushWithInternal(inner))) - .doOnError(t -> removeContentLength()); + .doOnError(t -> clearContentHeaders()); } - private void removeContentLength() { + private void clearContentHeaders() { if (!this.isCommitted()) { - this.getHeaders().remove(HttpHeaders.CONTENT_LENGTH); + HttpHeaders.clearContentHeaders(this.getHeaders()); } } 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 acc7b4bd71d8..c8a8552dd346 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -30,6 +30,7 @@ import org.springframework.core.io.buffer.DefaultDataBuffer; import org.springframework.core.io.buffer.DefaultDataBufferFactory; import org.springframework.http.HttpHeaders; +import org.springframework.http.MediaType; import org.springframework.http.ResponseCookie; import static org.assertj.core.api.Assertions.assertThat; @@ -37,11 +38,12 @@ /** * @author Rossen Stoyanchev * @author Sebastien Deleuze + * @author Brian Clozel */ public class ServerHttpResponseTests { @Test - public void writeWith() throws Exception { + void writeWith() throws Exception { TestServerHttpResponse response = new TestServerHttpResponse(); response.writeWith(Flux.just(wrap("a"), wrap("b"), wrap("c"))).block(); @@ -56,7 +58,7 @@ public void writeWith() throws Exception { } @Test // SPR-14952 - public void writeAndFlushWithFluxOfDefaultDataBuffer() throws Exception { + void writeAndFlushWithFluxOfDefaultDataBuffer() throws Exception { TestServerHttpResponse response = new TestServerHttpResponse(); Flux> flux = Flux.just(Flux.just(wrap("foo"))); response.writeAndFlushWith(flux).block(); @@ -70,21 +72,35 @@ public void writeAndFlushWithFluxOfDefaultDataBuffer() throws Exception { } @Test - public void writeWithError() throws Exception { - TestServerHttpResponse response = new TestServerHttpResponse(); - response.getHeaders().setContentLength(12); + void writeWithFluxError() throws Exception { IllegalStateException error = new IllegalStateException("boo"); - response.writeWith(Flux.error(error)).onErrorResume(ex -> Mono.empty()).block(); + writeWithError(Flux.error(error)); + } + + @Test + void writeWithMonoError() throws Exception { + IllegalStateException error = new IllegalStateException("boo"); + writeWithError(Mono.error(error)); + } + + void writeWithError(Publisher body) throws Exception { + TestServerHttpResponse response = new TestServerHttpResponse(); + HttpHeaders headers = response.getHeaders(); + headers.setContentType(MediaType.APPLICATION_JSON); + headers.set(HttpHeaders.CONTENT_ENCODING, "gzip"); + headers.setContentLength(12); + response.writeWith(body).onErrorResume(ex -> Mono.empty()).block(); assertThat(response.statusCodeWritten).isFalse(); assertThat(response.headersWritten).isFalse(); assertThat(response.cookiesWritten).isFalse(); - assertThat(response.getHeaders().containsKey(HttpHeaders.CONTENT_LENGTH)).isFalse(); + assertThat(headers).doesNotContainKeys(HttpHeaders.CONTENT_TYPE, HttpHeaders.CONTENT_LENGTH, + HttpHeaders.CONTENT_ENCODING); assertThat(response.body.isEmpty()).isTrue(); } @Test - public void setComplete() throws Exception { + void setComplete() throws Exception { TestServerHttpResponse response = new TestServerHttpResponse(); response.setComplete().block(); @@ -95,7 +111,7 @@ public void setComplete() throws Exception { } @Test - public void beforeCommitWithComplete() throws Exception { + void beforeCommitWithComplete() throws Exception { ResponseCookie cookie = ResponseCookie.from("ID", "123").build(); TestServerHttpResponse response = new TestServerHttpResponse(); response.beforeCommit(() -> Mono.fromRunnable(() -> response.getCookies().add(cookie.getName(), cookie))); @@ -113,7 +129,7 @@ public void beforeCommitWithComplete() throws Exception { } @Test - public void beforeCommitActionWithSetComplete() throws Exception { + void beforeCommitActionWithSetComplete() throws Exception { ResponseCookie cookie = ResponseCookie.from("ID", "123").build(); TestServerHttpResponse response = new TestServerHttpResponse(); response.beforeCommit(() -> { @@ -130,7 +146,6 @@ public void beforeCommitActionWithSetComplete() throws Exception { } - private DefaultDataBuffer wrap(String a) { return new DefaultDataBufferFactory().wrap(ByteBuffer.wrap(a.getBytes(StandardCharsets.UTF_8))); } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerAdapter.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerAdapter.java index 4608df51ab31..a8f5a1efeac6 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerAdapter.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerAdapter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2020 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. @@ -211,7 +211,7 @@ private Mono handleException(Throwable exception, HandlerMethod h // Success and error responses may use different content types exchange.getAttributes().remove(HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE); if (!exchange.getResponse().isCommitted()) { - exchange.getResponse().getHeaders().remove(HttpHeaders.CONTENT_TYPE); + HttpHeaders.clearContentHeaders(exchange.getResponse().getHeaders()); } InvocableHandlerMethod invocable = this.methodResolver.getExceptionHandlerMethod(exception, handlerMethod);