diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java index 8234ef002..1ad4d14a8 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java @@ -230,12 +230,6 @@ class AsyncCachingExec extends CachingExecBase implements AsyncExecChainHandler return; } - final SimpleHttpResponse fatalErrorResponse = getFatallyNonCompliantResponse(request, context); - if (fatalErrorResponse != null) { - triggerResponse(fatalErrorResponse, scope, asyncExecCallback); - return; - } - requestCompliance.makeRequestCompliant(request); request.addHeader(HttpHeaders.VIA,via); @@ -266,6 +260,12 @@ class AsyncCachingExec extends CachingExecBase implements AsyncExecChainHandler @Override public void completed(final HttpCacheEntry entry) { + final SimpleHttpResponse fatalErrorResponse = getFatallyNonCompliantResponse(request, context, entry != null); + if (fatalErrorResponse != null) { + triggerResponse(fatalErrorResponse, scope, asyncExecCallback); + return; + } + if (entry == null) { LOG.debug("Cache miss"); handleCacheMiss(requestCacheControl, target, request, entityProducer, scope, chain, asyncExecCallback); diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedHttpResponseGenerator.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedHttpResponseGenerator.java index 902740fdb..f9c27ff13 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedHttpResponseGenerator.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedHttpResponseGenerator.java @@ -189,7 +189,7 @@ class CachedHttpResponseGenerator { "Weak eTag not compatible with byte range", ContentType.DEFAULT_TEXT); case WEAK_ETAG_ON_PUTDELETE_METHOD_ERROR: - return SimpleHttpResponse.create(HttpStatus.SC_BAD_REQUEST, + return SimpleHttpResponse.create(HttpStatus.SC_PRECONDITION_FAILED, "Weak eTag not compatible with PUT or DELETE requests"); default: diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java index 9d39cc7e5..53472b8d7 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java @@ -208,8 +208,9 @@ class CachingExec extends CachingExecBase implements ExecChainHandler { setResponseStatus(context, CacheResponseStatus.CACHE_MODULE_RESPONSE); return new BasicClassicHttpResponse(HttpStatus.SC_NOT_IMPLEMENTED); } + final HttpCacheEntry entry = responseCache.getCacheEntry(target, request); - final SimpleHttpResponse fatalErrorResponse = getFatallyNonCompliantResponse(request, context); + final SimpleHttpResponse fatalErrorResponse = getFatallyNonCompliantResponse(request, context, entry != null); if (fatalErrorResponse != null) { return convert(fatalErrorResponse, scope); } @@ -224,7 +225,7 @@ class CachingExec extends CachingExecBase implements ExecChainHandler { return callBackend(target, request, scope, chain); } - final HttpCacheEntry entry = responseCache.getCacheEntry(target, request); + if (entry == null) { LOG.debug("Cache miss"); return handleCacheMiss(target, request, requestCacheControl, scope, chain); diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java index 535ec5e67..eb466d545 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java @@ -147,8 +147,9 @@ public class CachingExecBase { */ SimpleHttpResponse getFatallyNonCompliantResponse( final HttpRequest request, - final HttpContext context) { - final List fatalError = requestCompliance.requestIsFatallyNonCompliant(request); + final HttpContext context, + final boolean resourceExists) { + final List fatalError = requestCompliance.requestIsFatallyNonCompliant(request, resourceExists); if (fatalError != null && !fatalError.isEmpty()) { setResponseStatus(context, CacheResponseStatus.CACHE_MODULE_RESPONSE); return responseGenerator.getErrorForRequest(fatalError.get(0)); diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/RequestProtocolCompliance.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/RequestProtocolCompliance.java index 0f3c3f9dd..0fa43c893 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/RequestProtocolCompliance.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/RequestProtocolCompliance.java @@ -26,7 +26,11 @@ */ package org.apache.hc.client5.http.impl.cache; +import static org.apache.hc.client5.http.utils.DateUtils.parseStandardDate; + +import java.time.Instant; import java.util.ArrayList; +import java.util.Iterator; import java.util.List; import org.apache.hc.client5.http.cache.HeaderConstants; @@ -35,10 +39,14 @@ import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpRequest; import org.apache.hc.core5.http.HttpVersion; import org.apache.hc.core5.http.ProtocolVersion; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; class RequestProtocolCompliance { private final boolean weakETagOnPutDeleteAllowed; + private static final Logger LOG = LoggerFactory.getLogger(RequestProtocolCompliance.class); + public RequestProtocolCompliance() { super(); this.weakETagOnPutDeleteAllowed = false; @@ -56,7 +64,7 @@ class RequestProtocolCompliance { * @param request the HttpRequest Object * @return list of {@link RequestProtocolError} */ - public List requestIsFatallyNonCompliant(final HttpRequest request) { + public List requestIsFatallyNonCompliant(final HttpRequest request, final boolean resourceExists) { final List theErrors = new ArrayList<>(); RequestProtocolError anError = requestHasWeakETagAndRange(request); @@ -65,7 +73,7 @@ class RequestProtocolCompliance { } if (!weakETagOnPutDeleteAllowed) { - anError = requestHasWeekETagForPUTOrDELETEIfMatch(request); + anError = requestHasWeekETagForPUTOrDELETEIfMatch(request, resourceExists); if (anError != null) { theErrors.add(anError); } @@ -122,52 +130,66 @@ class RequestProtocolCompliance { } private RequestProtocolError requestHasWeakETagAndRange(final HttpRequest request) { - // TODO: Should these be looking at all the headers marked as Range? final String method = request.getMethod(); - if (!(HeaderConstants.GET_METHOD.equals(method))) { + if (!(HeaderConstants.GET_METHOD.equals(method) || HeaderConstants.HEAD_METHOD.equals(method))) { return null; } - final Header range = request.getFirstHeader(HttpHeaders.RANGE); - if (range == null) { + if (!request.containsHeader(HttpHeaders.RANGE)) { return null; } - final Header ifRange = request.getFirstHeader(HttpHeaders.IF_RANGE); - if (ifRange == null) { - return null; - } + final Instant ifRangeInstant = parseStandardDate(request, HttpHeaders.IF_RANGE); + final Instant lastModifiedInstant = parseStandardDate(request, HttpHeaders.LAST_MODIFIED); - final String val = ifRange.getValue(); - if (val.startsWith("W/")) { - return RequestProtocolError.WEAK_ETAG_AND_RANGE_ERROR; + for (final Iterator
it = request.headerIterator(HttpHeaders.IF_RANGE); it.hasNext(); ) { + final String val = it.next().getValue(); + if (val.startsWith("W/")) { + if (LOG.isDebugEnabled()) { + LOG.debug("Weak ETag found in If-Range header"); + } + return RequestProtocolError.WEAK_ETAG_AND_RANGE_ERROR; + } else { + // Not a strong validator or doesn't match Last-Modified + if (ifRangeInstant != null && lastModifiedInstant != null + && !ifRangeInstant.equals(lastModifiedInstant)) { + if (LOG.isDebugEnabled()) { + LOG.debug("If-Range does not match Last-Modified"); + } + return RequestProtocolError.WEAK_ETAG_AND_RANGE_ERROR; + } + } } - return null; } - private RequestProtocolError requestHasWeekETagForPUTOrDELETEIfMatch(final HttpRequest request) { - // TODO: Should these be looking at all the headers marked as If-Match/If-None-Match? - + private RequestProtocolError requestHasWeekETagForPUTOrDELETEIfMatch(final HttpRequest request, final boolean resourceExists) { final String method = request.getMethod(); - if (!(HeaderConstants.PUT_METHOD.equals(method) || HeaderConstants.DELETE_METHOD.equals(method))) { + if (!(HeaderConstants.PUT_METHOD.equals(method) || HeaderConstants.DELETE_METHOD.equals(method) + || HeaderConstants.POST_METHOD.equals(method)) + ) { return null; } - final Header ifMatch = request.getFirstHeader(HttpHeaders.IF_MATCH); - if (ifMatch != null) { - final String val = ifMatch.getValue(); - if (val.startsWith("W/")) { - return RequestProtocolError.WEAK_ETAG_ON_PUTDELETE_METHOD_ERROR; - } - } else { - final Header ifNoneMatch = request.getFirstHeader(HttpHeaders.IF_NONE_MATCH); - if (ifNoneMatch == null) { + for (final Iterator
it = request.headerIterator(HttpHeaders.IF_MATCH); it.hasNext();) { + final String val = it.next().getValue(); + if (val.equals("*") && !resourceExists) { return null; } + if (val.startsWith("W/")) { + if (LOG.isDebugEnabled()) { + LOG.debug("Weak ETag found in If-Match header"); + } + return RequestProtocolError.WEAK_ETAG_ON_PUTDELETE_METHOD_ERROR; + } + } - final String val2 = ifNoneMatch.getValue(); - if (val2.startsWith("W/")) { + for (final Iterator
it = request.headerIterator(HttpHeaders.IF_NONE_MATCH); it.hasNext();) { + final String val = it.next().getValue(); + if (val.startsWith("W/") || (val.equals("*") && resourceExists)) { + if (LOG.isDebugEnabled()) { + LOG.debug("Weak ETag found in If-None-Match header"); + } return RequestProtocolError.WEAK_ETAG_ON_PUTDELETE_METHOD_ERROR; } } diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachingExecChain.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachingExecChain.java index 07b3c67c9..0c17e1d34 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachingExecChain.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachingExecChain.java @@ -56,6 +56,7 @@ import org.apache.hc.client5.http.cache.HttpCacheStorage; import org.apache.hc.client5.http.cache.ResourceIOException; import org.apache.hc.client5.http.classic.ExecChain; import org.apache.hc.client5.http.classic.ExecRuntime; +import org.apache.hc.client5.http.classic.methods.HttpDelete; import org.apache.hc.client5.http.classic.methods.HttpGet; import org.apache.hc.client5.http.classic.methods.HttpOptions; import org.apache.hc.client5.http.protocol.HttpClientContext; @@ -64,6 +65,7 @@ import org.apache.hc.core5.http.ClassicHttpRequest; import org.apache.hc.core5.http.ClassicHttpResponse; import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpException; +import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpRequest; import org.apache.hc.core5.http.HttpStatus; @@ -1651,4 +1653,40 @@ public class TestCachingExecChain { Assertions.assertTrue(found110); } + + @Test + public void testRequestWithWeakETagAndRange() throws Exception { + final ClassicHttpRequest req1 = new HttpGet("http://foo1.example.com/"); + req1.addHeader(HttpHeaders.IF_MATCH, "W/\"weak1\""); + req1.addHeader(HttpHeaders.RANGE, "bytes=0-50"); + req1.addHeader(HttpHeaders.IF_RANGE, "W/\"weak2\""); // ETag doesn't match with If-Match ETag + final ClassicHttpResponse resp1 = new BasicClassicHttpResponse(HttpStatus.SC_OK, "OK"); + resp1.setEntity(HttpTestUtils.makeBody(128)); + resp1.setHeader("Content-Length", "128"); + resp1.setHeader("ETag", "\"etag\""); + resp1.setHeader("Date", DateUtils.formatStandardDate(Instant.now())); + resp1.setHeader("Cache-Control", "public, max-age=3600"); + + Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(resp1); + + final ClassicHttpResponse result = execute(req1); + Assertions.assertEquals(HttpStatus.SC_BAD_REQUEST, result.getCode()); + } + + @Test + public void testRequestWithWeakETagForPUTOrDELETEIfMatch() throws Exception { + final ClassicHttpRequest req1 = new HttpDelete("http://foo1.example.com/"); + req1.addHeader(HttpHeaders.IF_MATCH, "W/\"weak1\""); + final ClassicHttpResponse resp1 = new BasicClassicHttpResponse(HttpStatus.SC_OK, "OK"); + resp1.setEntity(HttpTestUtils.makeBody(128)); + resp1.setHeader("Content-Length", "128"); + resp1.setHeader("ETag", "\"etag\""); + resp1.setHeader("Date", DateUtils.formatStandardDate(Instant.now())); + resp1.setHeader("Cache-Control", "public, max-age=3600"); + + Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(resp1); + + final ClassicHttpResponse result = execute(req1); + Assertions.assertEquals(HttpStatus.SC_PRECONDITION_FAILED, result.getCode()); + } } diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestRequestProtocolCompliance.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestRequestProtocolCompliance.java index cb147a7ad..f1693d964 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestRequestProtocolCompliance.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestRequestProtocolCompliance.java @@ -31,7 +31,10 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.Collections; +import java.util.List; +import org.apache.hc.client5.http.cache.HeaderConstants; +import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpRequest; import org.apache.hc.core5.http.HttpVersion; import org.apache.hc.core5.http.ProtocolVersion; @@ -54,14 +57,14 @@ public class TestRequestProtocolCompliance { final HttpRequest req = new BasicHttpRequest("GET", "/"); req.setHeader("Range", "bytes=0-499"); req.setHeader("If-Range", "W/\"weak\""); - assertEquals(1, impl.requestIsFatallyNonCompliant(req).size()); + assertEquals(1, impl.requestIsFatallyNonCompliant(req, false).size()); } @Test public void testRequestWithWeekETagForPUTOrDELETEIfMatch() throws Exception { final HttpRequest req = new BasicHttpRequest("PUT", "http://example.com/"); req.setHeader("If-Match", "W/\"weak\""); - assertEquals(1, impl.requestIsFatallyNonCompliant(req).size()); + assertEquals(1, impl.requestIsFatallyNonCompliant(req, false).size()); } @Test @@ -69,7 +72,7 @@ public class TestRequestProtocolCompliance { final HttpRequest req = new BasicHttpRequest("PUT", "http://example.com/"); req.setHeader("If-Match", "W/\"weak\""); impl = new RequestProtocolCompliance(true); - assertEquals(Collections.emptyList(), impl.requestIsFatallyNonCompliant(req)); + assertEquals(Collections.emptyList(), impl.requestIsFatallyNonCompliant(req, false)); } @Test @@ -98,4 +101,88 @@ public class TestRequestProtocolCompliance { assertEquals(HttpVersion.HTTP_1_1, wrapper.getVersion()); } + @Test + public void testRequestWithMultipleIfMatchHeaders() { + final HttpRequest req = new BasicHttpRequest(HeaderConstants.PUT_METHOD, "http://example.com/"); + req.addHeader(HttpHeaders.IF_MATCH, "W/\"weak1\""); + req.addHeader(HttpHeaders.IF_MATCH, "W/\"weak2\""); + assertEquals(1, impl.requestIsFatallyNonCompliant(req, false).size()); + } + + @Test + public void testRequestWithMultipleIfNoneMatchHeaders() { + final HttpRequest req = new BasicHttpRequest(HeaderConstants.PUT_METHOD, "http://example.com/"); + req.addHeader(HttpHeaders.IF_NONE_MATCH, "W/\"weak1\""); + req.addHeader(HttpHeaders.IF_NONE_MATCH, "W/\"weak2\""); + assertEquals(1, impl.requestIsFatallyNonCompliant(req, false).size()); + } + + @Test + public void testRequestWithPreconditionFailed() { + final HttpRequest req = new BasicHttpRequest(HeaderConstants.GET_METHOD, "http://example.com/"); + req.addHeader(HttpHeaders.IF_MATCH, "W/\"weak1\""); + req.addHeader(HttpHeaders.RANGE, "1"); + req.addHeader(HttpHeaders.IF_RANGE, "W/\"weak2\""); // ETag doesn't match with If-Match ETag + // This will cause the precondition If-Match to fail because the ETags are different + final List requestProtocolErrors = impl.requestIsFatallyNonCompliant(req, false); + assertTrue(requestProtocolErrors.contains(RequestProtocolError.WEAK_ETAG_AND_RANGE_ERROR)); + } + + @Test + public void testRequestWithValidIfRangeDate() { + final HttpRequest req = new BasicHttpRequest(HeaderConstants.GET_METHOD, "http://example.com/"); + req.addHeader(HttpHeaders.RANGE, "bytes=0-499"); + req.addHeader(HttpHeaders.LAST_MODIFIED, "Wed, 21 Oct 2023 07:28:00 GMT"); + req.addHeader(HttpHeaders.IF_RANGE, "Wed, 21 Oct 2023 07:28:00 GMT"); + assertTrue(impl.requestIsFatallyNonCompliant(req, false).isEmpty()); + } + + @Test + public void testRequestWithInvalidDateFormat() { + final HttpRequest req = new BasicHttpRequest(HeaderConstants.GET_METHOD, "http://example.com/"); + req.addHeader(HttpHeaders.RANGE, "bytes=0-499"); + req.addHeader(HttpHeaders.LAST_MODIFIED, "Wed, 21 Oct 2023 07:28:00 GMT"); + req.addHeader(HttpHeaders.IF_RANGE, "20/10/2023"); + assertTrue(impl.requestIsFatallyNonCompliant(req, false).isEmpty()); + } + + @Test + public void testRequestWithMissingIfRangeDate() { + final HttpRequest req = new BasicHttpRequest(HeaderConstants.GET_METHOD, "http://example.com/"); + req.addHeader(HttpHeaders.RANGE, "bytes=0-499"); + req.addHeader(HttpHeaders.LAST_MODIFIED, "Wed, 21 Oct 2023 07:28:00 GMT"); + assertTrue(impl.requestIsFatallyNonCompliant(req, false).isEmpty()); + } + + @Test + public void testRequestWithWeakETagAndRangeAndDAte() { + // Setup request with GET method, Range header, If-Range header starting with "W/", + // and a Last-Modified date that doesn't match the If-Range date + final HttpRequest req = new BasicHttpRequest(HeaderConstants.GET_METHOD, "http://example.com/"); + req.addHeader(HttpHeaders.RANGE, "bytes=0-499"); + req.addHeader(HttpHeaders.LAST_MODIFIED, "Fri, 20 Oct 2023 07:28:00 GMT"); + req.addHeader(HttpHeaders.IF_RANGE, "Wed, 18 Oct 2023 07:28:00 GMT"); + + + // Use your implementation to check the request + final List errors = impl.requestIsFatallyNonCompliant(req, false); + + // Assert that the WEAK_ETAG_AND_RANGE_ERROR is in the list of errors + assertTrue(errors.contains(RequestProtocolError.WEAK_ETAG_AND_RANGE_ERROR)); + } + + @Test + public void testRequestWithWeekETagForPUTOrDELETEIfMatchWithStart() { + final HttpRequest req = new BasicHttpRequest("PUT", "http://example.com/"); + req.setHeader(HttpHeaders.IF_MATCH, "*"); + assertEquals(0, impl.requestIsFatallyNonCompliant(req, false).size()); + } + + @Test + public void testRequestOkETagForPUTOrDELETEIfMatch() { + final HttpRequest req = new BasicHttpRequest("PUT", "http://example.com/"); + req.setHeader(HttpHeaders.IF_MATCH, "1234"); + assertEquals(0, impl.requestIsFatallyNonCompliant(req, false).size()); + } + }