From 9410998897edf558a8708d46ce29ee0db546be7c Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Fri, 21 Oct 2022 15:11:08 +0200 Subject: [PATCH] Add caching headers to If-Unmodified-Since responses Conditional requests using "If-Unmodified-Since" headers are generally used as precondition checks for state-changing methods (POST, PUT, DELETE). See https://datatracker.ietf.org/doc/html/rfc7232#section-3.4 The spec also allows for idempotent methods like GET and HEAD. Prior to this commit, the "If-Unmodified-Since" processing done in `checkNotModified` (see `ServletWebRequest` and `DefaultServerWebExchange`) would only focus on the state changing methods and not take into account the safe methods. For those cases, the "ETag" and "Last-Modified" would be missing from the response. This commit ensures that such headers are added as expected in these cases. Fixes gh-29362 --- .../context/request/ServletWebRequest.java | 6 +++ .../adapter/DefaultServerWebExchange.java | 7 ++++ .../ServletWebRequestHttpMethodsTests.java | 41 ++++++++++++------- ...erverWebExchangeCheckNotModifiedTests.java | 13 ++++++ 4 files changed, 52 insertions(+), 15 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java b/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java index 5fef231f283b..581163a86692 100644 --- a/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java +++ b/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java @@ -220,6 +220,12 @@ public boolean checkNotModified(@Nullable String etag, long lastModifiedTimestam if (this.notModified && response != null) { response.setStatus(HttpStatus.PRECONDITION_FAILED.value()); } + if (SAFE_METHODS.contains(getRequest().getMethod())) { + if (StringUtils.hasLength(etag) && response.getHeader(HttpHeaders.ETAG) == null) { + response.setHeader(HttpHeaders.ETAG, padEtagIfNecessary(etag)); + } + response.setDateHeader(HttpHeaders.LAST_MODIFIED, lastModifiedTimestamp); + } return this.notModified; } diff --git a/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java b/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java index 984615c6d37c..79d3d2db2fa8 100644 --- a/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java +++ b/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java @@ -56,6 +56,7 @@ * Default implementation of {@link ServerWebExchange}. * * @author Rossen Stoyanchev + * @author Brian Clozel * @since 5.0 */ public class DefaultServerWebExchange implements ServerWebExchange { @@ -261,6 +262,12 @@ public boolean checkNotModified(@Nullable String etag, Instant lastModified) { if (this.notModified) { getResponse().setStatusCode(HttpStatus.PRECONDITION_FAILED); } + if (SAFE_METHODS.contains(getRequest().getMethod())) { + if (StringUtils.hasLength(etag) && getResponseHeaders().getETag() == null) { + getResponseHeaders().setETag(padEtagIfNecessary(etag)); + } + getResponseHeaders().setLastModified(lastModified.toEpochMilli()); + } return this.notModified; } diff --git a/spring-web/src/test/java/org/springframework/web/context/request/ServletWebRequestHttpMethodsTests.java b/spring-web/src/test/java/org/springframework/web/context/request/ServletWebRequestHttpMethodsTests.java index 27a42f96f7b6..9092a0030769 100644 --- a/spring-web/src/test/java/org/springframework/web/context/request/ServletWebRequestHttpMethodsTests.java +++ b/spring-web/src/test/java/org/springframework/web/context/request/ServletWebRequestHttpMethodsTests.java @@ -23,6 +23,7 @@ import java.time.ZonedDateTime; import java.util.Date; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -286,9 +287,9 @@ void checkNotModifiedMultipleETags(String method) { assertThat(servletResponse.getHeader("ETag")).isEqualTo(etag); } - @ParameterizedHttpMethodTest - void checkNotModifiedTimestampWithLengthPart(String method) { - setUpRequest(method); + @Test + void checkNotModifiedTimestampWithLengthPart() { + setUpRequest("GET"); long epochTime = ZonedDateTime.parse(CURRENT_TIME, RFC_1123_DATE_TIME).toInstant().toEpochMilli(); servletRequest.setMethod("GET"); @@ -299,12 +300,11 @@ void checkNotModifiedTimestampWithLengthPart(String method) { assertThat(servletResponse.getDateHeader("Last-Modified") / 1000).isEqualTo(epochTime / 1000); } - @ParameterizedHttpMethodTest - void checkModifiedTimestampWithLengthPart(String method) { - setUpRequest(method); + @Test + void checkModifiedTimestampWithLengthPart() { + setUpRequest("GET"); long epochTime = ZonedDateTime.parse(CURRENT_TIME, RFC_1123_DATE_TIME).toInstant().toEpochMilli(); - servletRequest.setMethod("GET"); servletRequest.addHeader("If-Modified-Since", "Wed, 08 Apr 2014 09:57:42 GMT; length=13774"); assertThat(request.checkNotModified(epochTime)).isFalse(); @@ -312,13 +312,12 @@ void checkModifiedTimestampWithLengthPart(String method) { assertThat(servletResponse.getDateHeader("Last-Modified") / 1000).isEqualTo(epochTime / 1000); } - @ParameterizedHttpMethodTest - void checkNotModifiedTimestampConditionalPut(String method) { - setUpRequest(method); + @Test + void checkNotModifiedTimestampConditionalPut() { + setUpRequest("PUT"); long currentEpoch = currentDate.getTime(); long oneMinuteAgo = currentEpoch - (1000 * 60); - servletRequest.setMethod("PUT"); servletRequest.addHeader("If-UnModified-Since", currentEpoch); assertThat(request.checkNotModified(oneMinuteAgo)).isFalse(); @@ -326,13 +325,12 @@ void checkNotModifiedTimestampConditionalPut(String method) { assertThat(servletResponse.getHeader("Last-Modified")).isNull(); } - @ParameterizedHttpMethodTest - void checkNotModifiedTimestampConditionalPutConflict(String method) { - setUpRequest(method); + @Test + void checkNotModifiedTimestampConditionalPutConflict() { + setUpRequest("PUT"); long currentEpoch = currentDate.getTime(); long oneMinuteAgo = currentEpoch - (1000 * 60); - servletRequest.setMethod("PUT"); servletRequest.addHeader("If-UnModified-Since", oneMinuteAgo); assertThat(request.checkNotModified(currentEpoch)).isTrue(); @@ -340,6 +338,19 @@ void checkNotModifiedTimestampConditionalPutConflict(String method) { assertThat(servletResponse.getHeader("Last-Modified")).isNull(); } + @ParameterizedHttpMethodTest + void checkNotModifiedConditionalGet(String method) { + setUpRequest(method); + long currentEpoch = currentDate.getTime(); + long oneMinuteAgo = currentEpoch - (1000 * 60); + servletRequest.addHeader("If-UnModified-Since", currentEpoch); + + assertThat(request.checkNotModified(oneMinuteAgo)).isFalse(); + assertThat(servletResponse.getStatus()).isEqualTo(200); + assertThat(servletResponse.getDateHeader("Last-Modified") / 1000).isEqualTo(oneMinuteAgo / 1000); + } + + private void setUpRequest(String method) { this.servletRequest.setMethod(method); this.servletRequest.setRequestURI("https://example.org"); diff --git a/spring-web/src/test/java/org/springframework/web/server/adapter/DefaultServerWebExchangeCheckNotModifiedTests.java b/spring-web/src/test/java/org/springframework/web/server/adapter/DefaultServerWebExchangeCheckNotModifiedTests.java index 97b963bd32f8..cbfd8840d1f6 100644 --- a/spring-web/src/test/java/org/springframework/web/server/adapter/DefaultServerWebExchangeCheckNotModifiedTests.java +++ b/spring-web/src/test/java/org/springframework/web/server/adapter/DefaultServerWebExchangeCheckNotModifiedTests.java @@ -307,4 +307,17 @@ void checkNotModifiedTimestampConditionalPutConflict() throws Exception { assertThat(exchange.getResponse().getHeaders().getLastModified()).isEqualTo(-1); } + @Test + void checkNotModifiedTimestampConditionalGet() throws Exception { + String eTag = "\"Test\""; + Instant oneMinuteAgo = currentDate.minusSeconds(60); + MockServerHttpRequest request = MockServerHttpRequest.get("/").ifUnmodifiedSince(currentDate.toEpochMilli()).build(); + MockServerWebExchange exchange = MockServerWebExchange.from(request); + + assertThat(exchange.checkNotModified(eTag, oneMinuteAgo)).isFalse(); + assertThat(exchange.getResponse().getStatusCode()).isNull(); + assertThat(exchange.getResponse().getHeaders().getLastModified()).isEqualTo(oneMinuteAgo.toEpochMilli()); + assertThat(exchange.getResponse().getHeaders().getETag()).isEqualTo(eTag); + } + }