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 19b1256eff..ebc46b3491 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 @@ -17,12 +17,18 @@ package org.springframework.web.context.request; import java.security.Principal; -import java.util.Date; +import java.text.ParseException; +import java.text.SimpleDateFormat; +import java.util.Arrays; +import java.util.Enumeration; import java.util.Iterator; +import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.TimeZone; import java.util.regex.Matcher; import java.util.regex.Pattern; + import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; @@ -44,25 +50,17 @@ import org.springframework.web.util.WebUtils; */ public class ServletWebRequest extends ServletRequestAttributes implements NativeWebRequest { - private static final String HEADER_ETAG = "ETag"; + private static final String ETAG = "ETag"; - private static final String HEADER_IF_MODIFIED_SINCE = "If-Modified-Since"; + private static final String IF_MODIFIED_SINCE = "If-Modified-Since"; - private static final String HEADER_IF_UNMODIFIED_SINCE = "If-Unmodified-Since"; + private static final String IF_UNMODIFIED_SINCE = "If-Unmodified-Since"; - private static final String HEADER_IF_NONE_MATCH = "If-None-Match"; + private static final String IF_NONE_MATCH = "If-None-Match"; - private static final String HEADER_LAST_MODIFIED = "Last-Modified"; + private static final String LAST_MODIFIED = "Last-Modified"; - private static final String METHOD_GET = "GET"; - - private static final String METHOD_HEAD = "HEAD"; - - private static final String METHOD_POST = "POST"; - - private static final String METHOD_PUT = "PUT"; - - private static final String METHOD_DELETE = "DELETE"; + private static final List SAFE_METHODS = Arrays.asList("GET", "HEAD"); /** * Pattern matching ETag multiple field values in headers such as "If-Match", "If-None-Match" @@ -70,6 +68,17 @@ public class ServletWebRequest extends ServletRequestAttributes implements Nativ */ private static final Pattern ETAG_HEADER_VALUE_PATTERN = Pattern.compile("\\*|\\s*((W\\/)?(\"[^\"]*\"))\\s*,?"); + /** + * Date formats as specified in the HTTP RFC + * @see Section 7.1.1.1 of RFC 7231 + */ + private static final String[] DATE_FORMATS = new String[] { + "EEE, dd MMM yyyy HH:mm:ss zzz", + "EEE, dd-MMM-yy HH:mm:ss zzz", + "EEE MMM dd HH:mm:ss yyyy" + }; + + private static TimeZone GMT = TimeZone.getTimeZone("GMT"); private boolean notModified = false; @@ -189,141 +198,128 @@ public class ServletWebRequest extends ServletRequestAttributes implements Nativ @Override public boolean checkNotModified(long lastModifiedTimestamp) { - HttpServletResponse response = getResponse(); - if (lastModifiedTimestamp >= 0 && !this.notModified) { - if (isCompatibleWithConditionalRequests(response)) { - this.notModified = isTimestampNotModified(lastModifiedTimestamp); - if (response != null) { - if (supportsNotModifiedStatus()) { - if (this.notModified) { - response.setStatus(HttpServletResponse.SC_NOT_MODIFIED); - } - if (isHeaderAbsent(response, HEADER_LAST_MODIFIED)) { - response.setDateHeader(HEADER_LAST_MODIFIED, lastModifiedTimestamp); - } - } - else if (supportsConditionalUpdate()) { - if (this.notModified) { - response.setStatus(HttpServletResponse.SC_PRECONDITION_FAILED); - } - } - } - } - } - return this.notModified; + return checkNotModified(null, lastModifiedTimestamp); } @Override public boolean checkNotModified(String etag) { - HttpServletResponse response = getResponse(); - if (StringUtils.hasLength(etag) && !this.notModified) { - if (isCompatibleWithConditionalRequests(response)) { - etag = addEtagPadding(etag); - if (hasRequestHeader(HEADER_IF_NONE_MATCH)) { - this.notModified = isEtagNotModified(etag); - } - if (response != null) { - if (this.notModified && supportsNotModifiedStatus()) { - response.setStatus(HttpServletResponse.SC_NOT_MODIFIED); - } - if (isHeaderAbsent(response, HEADER_ETAG)) { - response.setHeader(HEADER_ETAG, etag); - } - } - } - } - return this.notModified; + return checkNotModified(etag, -1); } @Override public boolean checkNotModified(String etag, long lastModifiedTimestamp) { HttpServletResponse response = getResponse(); - if (StringUtils.hasLength(etag) && !this.notModified) { - if (isCompatibleWithConditionalRequests(response)) { - etag = addEtagPadding(etag); - if (hasRequestHeader(HEADER_IF_NONE_MATCH)) { - this.notModified = isEtagNotModified(etag); - } - else if (hasRequestHeader(HEADER_IF_MODIFIED_SINCE)) { - this.notModified = isTimestampNotModified(lastModifiedTimestamp); - } - if (response != null) { - if (supportsNotModifiedStatus()) { - if (this.notModified) { - response.setStatus(HttpServletResponse.SC_NOT_MODIFIED); - } - if (isHeaderAbsent(response, HEADER_ETAG)) { - response.setHeader(HEADER_ETAG, etag); - } - if (isHeaderAbsent(response, HEADER_LAST_MODIFIED)) { - response.setDateHeader(HEADER_LAST_MODIFIED, lastModifiedTimestamp); - } - } - else if (supportsConditionalUpdate()) { - if (this.notModified) { - response.setStatus(HttpServletResponse.SC_PRECONDITION_FAILED); - } - } + if (this.notModified || HttpStatus.OK.value() != response.getStatus()) { + return this.notModified; + } + + // Evaluate conditions in order of precedence. + // See https://tools.ietf.org/html/rfc7232#section-6 + + if (validateIfUnmodifiedSince(lastModifiedTimestamp)) { + if (this.notModified) { + response.setStatus(HttpStatus.PRECONDITION_FAILED.value()); + } + return this.notModified; + } + + boolean validated = validateIfNoneMatch(etag); + + if (!validated) { + validateIfModifiedSince(lastModifiedTimestamp); + } + + // Update response + + boolean isHttpGetOrHead = SAFE_METHODS.contains(getRequest().getMethod()); + if (this.notModified) { + response.setStatus(isHttpGetOrHead ? + HttpStatus.NOT_MODIFIED.value() : HttpStatus.PRECONDITION_FAILED.value()); + } + if (isHttpGetOrHead) { + if(lastModifiedTimestamp > 0 && parseDateValue(response.getHeader(LAST_MODIFIED)) == -1) { + response.setDateHeader(LAST_MODIFIED, lastModifiedTimestamp); + } + if (StringUtils.hasLength(etag) && response.getHeader(ETAG) == null) { + response.setHeader(ETAG, padEtagIfNecessary(etag)); + } + } + + return this.notModified; + } + + private boolean validateIfUnmodifiedSince(long lastModifiedTimestamp) { + if (lastModifiedTimestamp < 0) { + return false; + } + long ifUnmodifiedSince = parseDateHeader(IF_UNMODIFIED_SINCE); + if (ifUnmodifiedSince == -1) { + return false; + } + // We will perform this validation... + this.notModified = (ifUnmodifiedSince < (lastModifiedTimestamp / 1000 * 1000)); + return true; + } + + private boolean validateIfNoneMatch(String etag) { + if (!StringUtils.hasLength(etag)) { + return false; + } + Enumeration ifNoneMatch; + try { + ifNoneMatch = getRequest().getHeaders(IF_NONE_MATCH); + } + catch (IllegalArgumentException ex) { + return false; + } + if (!ifNoneMatch.hasMoreElements()) { + return false; + } + // We will perform this validation... + etag = padEtagIfNecessary(etag); + while (ifNoneMatch.hasMoreElements()) { + String clientETags = ifNoneMatch.nextElement(); + + Matcher eTagMatcher = ETAG_HEADER_VALUE_PATTERN.matcher(clientETags); + // Compare weak/strong ETags as per https://tools.ietf.org/html/rfc7232#section-2.3 + while (eTagMatcher.find()) { + if (StringUtils.hasLength(eTagMatcher.group()) + && etag.replaceFirst("^W/", "").equals(eTagMatcher.group(3))) { + this.notModified = true; + break; } } } - return this.notModified; + return true; + } + + private String padEtagIfNecessary(String etag) { + if (!StringUtils.hasLength(etag)) { + return etag; + } + if ((etag.startsWith("\"") || etag.startsWith("W/\"")) && etag.endsWith("\"")) { + return etag; + } + return "\"" + etag + "\""; + } + + private boolean validateIfModifiedSince(long lastModifiedTimestamp) { + if (lastModifiedTimestamp < 0) { + return false; + } + long ifModifiedSince = parseDateHeader(IF_MODIFIED_SINCE); + if (ifModifiedSince == -1) { + return false; + } + // We will perform this validation... + this.notModified = ifModifiedSince >= (lastModifiedTimestamp / 1000 * 1000); + return true; } public boolean isNotModified() { return this.notModified; } - - private boolean isCompatibleWithConditionalRequests(HttpServletResponse response) { - try { - if (response == null) { - // Can't check response.getStatus() - let's assume we're good - return true; - } - return HttpStatus.valueOf(response.getStatus()).is2xxSuccessful(); - } - catch (IllegalArgumentException ex) { - return true; - } - } - - private boolean isHeaderAbsent(HttpServletResponse response, String header) { - if (response == null) { - // Can't check response.getHeader(header) - let's assume it's not set - return true; - } - return (response.getHeader(header) == null); - } - - private boolean hasRequestHeader(String headerName) { - return StringUtils.hasLength(getHeader(headerName)); - } - - private boolean supportsNotModifiedStatus() { - String method = getRequest().getMethod(); - return (METHOD_GET.equals(method) || METHOD_HEAD.equals(method)); - } - - private boolean supportsConditionalUpdate() { - String method = getRequest().getMethod(); - return (METHOD_POST.equals(method) || METHOD_PUT.equals(method) || METHOD_DELETE.equals(method)) - && hasRequestHeader(HEADER_IF_UNMODIFIED_SINCE); - } - - private boolean isTimestampNotModified(long lastModifiedTimestamp) { - long ifModifiedSince = parseDateHeader(HEADER_IF_MODIFIED_SINCE); - if (ifModifiedSince != -1) { - return (ifModifiedSince >= (lastModifiedTimestamp / 1000 * 1000)); - } - long ifUnmodifiedSince = parseDateHeader(HEADER_IF_UNMODIFIED_SINCE); - if (ifUnmodifiedSince != -1) { - return (ifUnmodifiedSince < (lastModifiedTimestamp / 1000 * 1000)); - } - return false; - } - - @SuppressWarnings("deprecation") private long parseDateHeader(String headerName) { long dateValue = -1; try { @@ -335,36 +331,32 @@ public class ServletWebRequest extends ServletRequestAttributes implements Nativ int separatorIndex = headerValue.indexOf(';'); if (separatorIndex != -1) { String datePart = headerValue.substring(0, separatorIndex); - try { - dateValue = Date.parse(datePart); - } - catch (IllegalArgumentException ex2) { - // Giving up - } + dateValue = parseDateValue(datePart); } } return dateValue; } - private boolean isEtagNotModified(String etag) { - String ifNoneMatch = getHeader(HEADER_IF_NONE_MATCH); - // compare weak/strong ETag as per https://tools.ietf.org/html/rfc7232#section-2.3 - String serverETag = etag.replaceFirst("^W/", ""); - Matcher eTagMatcher = ETAG_HEADER_VALUE_PATTERN.matcher(ifNoneMatch); - while (eTagMatcher.find()) { - if ("*".equals(eTagMatcher.group()) - || serverETag.equals(eTagMatcher.group(3))) { - return true; + private long parseDateValue(String headerValue) { + if (headerValue == null) { + // No header value sent at all + return -1; + } + if (headerValue.length() >= 3) { + // Short "0" or "-1" like values are never valid HTTP date headers... + // Let's only bother with SimpleDateFormat parsing for long enough values. + for (String dateFormat : DATE_FORMATS) { + SimpleDateFormat simpleDateFormat = new SimpleDateFormat(dateFormat, Locale.US); + simpleDateFormat.setTimeZone(GMT); + try { + return simpleDateFormat.parse(headerValue).getTime(); + } + catch (ParseException ex) { + // ignore + } } } - return false; - } - - private String addEtagPadding(String etag) { - if (!(etag.startsWith("\"") || etag.startsWith("W/\"")) || !etag.endsWith("\"")) { - etag = "\"" + etag + "\""; - } - return etag; + return -1; } @Override 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 e5ff3c2c85..de9e3eed17 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 @@ -94,9 +94,7 @@ public class ServletWebRequestHttpMethodsTests { servletRequest.addHeader("If-Modified-Since", epochTime); servletResponse.setStatus(0); - assertTrue(request.checkNotModified(epochTime)); - assertEquals(304, servletResponse.getStatus()); - assertEquals(dateFormat.format(epochTime), servletResponse.getHeader("Last-Modified")); + assertFalse(request.checkNotModified(epochTime)); } @Test // SPR-14559 @@ -202,13 +200,13 @@ public class ServletWebRequestHttpMethodsTests { } @Test - public void checkNotModifiedWildcardETag() { + public void checkNotModifiedWildcardIsIgnored() { String eTag = "\"Foo\""; servletRequest.addHeader("If-None-Match", "*"); - assertTrue(request.checkNotModified(eTag)); + assertFalse(request.checkNotModified(eTag)); - assertEquals(304, servletResponse.getStatus()); + assertEquals(200, servletResponse.getStatus()); assertEquals(eTag, servletResponse.getHeader("ETag")); } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java index ea4f7de791..3cc7228066 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java @@ -28,8 +28,6 @@ import org.springframework.core.MethodParameter; import org.springframework.core.ResolvableType; import org.springframework.http.HttpEntity; import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpMethod; -import org.springframework.http.HttpStatus; import org.springframework.http.RequestEntity; import org.springframework.http.ResponseEntity; import org.springframework.http.converter.HttpMessageConverter; @@ -41,6 +39,7 @@ import org.springframework.web.HttpMediaTypeNotSupportedException; import org.springframework.web.accept.ContentNegotiationManager; import org.springframework.web.bind.support.WebDataBinderFactory; import org.springframework.web.context.request.NativeWebRequest; +import org.springframework.web.context.request.ServletWebRequest; import org.springframework.web.method.support.ModelAndViewContainer; /** @@ -182,15 +181,15 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro } if (responseEntity instanceof ResponseEntity) { - outputMessage.getServletResponse().setStatus(((ResponseEntity) responseEntity).getStatusCodeValue()); - HttpMethod method = inputMessage.getMethod(); - boolean isGetOrHead = (HttpMethod.GET == method || HttpMethod.HEAD == method); - if (isGetOrHead && isResourceNotModified(inputMessage, outputMessage)) { - outputMessage.setStatusCode(HttpStatus.NOT_MODIFIED); - // Ensure headers are flushed, no body should be written. - outputMessage.flush(); - // Skip call to converters, as they may update the body. - return; + int returnStatus = ((ResponseEntity) responseEntity).getStatusCodeValue(); + outputMessage.getServletResponse().setStatus(returnStatus); + if(returnStatus == 200) { + if (isResourceNotModified(inputMessage, outputMessage)) { + // Ensure headers are flushed, no body should be written. + outputMessage.flush(); + // Skip call to converters, as they may update the body. + return; + } } } @@ -223,55 +222,15 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro } private boolean isResourceNotModified(ServletServerHttpRequest inputMessage, ServletServerHttpResponse outputMessage) { - boolean notModified = false; - try { - long ifModifiedSince = inputMessage.getHeaders().getIfModifiedSince(); - String eTag = addEtagPadding(outputMessage.getHeaders().getETag()); - long lastModified = outputMessage.getHeaders().getLastModified(); - List ifNoneMatch = inputMessage.getHeaders().getIfNoneMatch(); - if (!ifNoneMatch.isEmpty() && (inputMessage.getHeaders().containsKey(HttpHeaders.IF_UNMODIFIED_SINCE) - || inputMessage.getHeaders().containsKey(HttpHeaders.IF_MATCH))) { - // invalid conditional request, do not process - } - else if (lastModified != -1 && StringUtils.hasLength(eTag)) { - notModified = isETagNotModified(ifNoneMatch, eTag) && isTimeStampNotModified(ifModifiedSince, lastModified); - } - else if (lastModified != -1) { - notModified = isTimeStampNotModified(ifModifiedSince, lastModified); - } - else if (StringUtils.hasLength(eTag)) { - notModified = isETagNotModified(ifNoneMatch, eTag); - } - } - catch (IllegalArgumentException exc) { - // invalid conditional request, do not process - } - return notModified; - } + ServletWebRequest servletWebRequest = + new ServletWebRequest(inputMessage.getServletRequest(), outputMessage.getServletResponse()); + HttpHeaders responseHeaders = outputMessage.getHeaders(); + String etag = responseHeaders.getETag(); + long lastModifiedTimestamp = responseHeaders.getLastModified(); + responseHeaders.remove(HttpHeaders.ETAG); + responseHeaders.remove(HttpHeaders.LAST_MODIFIED); - private boolean isETagNotModified(List ifNoneMatch, String etag) { - if (StringUtils.hasLength(etag)) { - for (String clientETag : ifNoneMatch) { - // Compare weak/strong ETags as per https://tools.ietf.org/html/rfc7232#section-2.3 - if (StringUtils.hasLength(clientETag) && - (clientETag.replaceFirst("^W/", "").equals(etag.replaceFirst("^W/", "")))) { - return true; - } - } - } - return false; - } - - private boolean isTimeStampNotModified(long ifModifiedSince, long lastModifiedTimestamp) { - return (ifModifiedSince >= (lastModifiedTimestamp / 1000 * 1000)); - } - - private String addEtagPadding(String etag) { - if (StringUtils.hasLength(etag) && - (!(etag.startsWith("\"") || etag.startsWith("W/\"")) || !etag.endsWith("\"")) ) { - etag = "\"" + etag + "\""; - } - return etag; + return servletWebRequest.checkNotModified(etag, lastModifiedTimestamp); } @Override diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorMockTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorMockTests.java index 9b3f1e4071..021dffaf60 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorMockTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorMockTests.java @@ -476,8 +476,7 @@ public class HttpEntityMethodProcessorMockTests { processor.handleReturnValue(returnValue, returnTypeResponseEntity, mavContainer, webRequest); assertResponseOkWithBody("body"); - assertEquals(1, servletResponse.getHeaderValues(HttpHeaders.ETAG).size()); - assertEquals(etagValue, servletResponse.getHeader(HttpHeaders.ETAG)); + assertEquals(0, servletResponse.getHeaderValues(HttpHeaders.ETAG).size()); } // SPR-13626 @@ -511,7 +510,7 @@ public class HttpEntityMethodProcessorMockTests { initStringMessageConversion(MediaType.TEXT_PLAIN); processor.handleReturnValue(returnValue, returnTypeResponseEntity, mavContainer, webRequest); - assertResponseOkWithBody("body"); + assertResponseNotModified(); assertEquals(1, servletResponse.getHeaderValues(HttpHeaders.ETAG).size()); assertEquals(etagValue, servletResponse.getHeader(HttpHeaders.ETAG)); } @@ -529,7 +528,7 @@ public class HttpEntityMethodProcessorMockTests { initStringMessageConversion(MediaType.TEXT_PLAIN); processor.handleReturnValue(returnValue, returnTypeResponseEntity, mavContainer, webRequest); - assertResponseOkWithBody("body"); + assertResponseNotModified(); assertEquals(1, servletResponse.getHeaderValues(HttpHeaders.ETAG).size()); assertEquals(etagValue, servletResponse.getHeader(HttpHeaders.ETAG)); }