HTTPCLIENT-1920: Add a check in CachedResponseSuitabilityChecker to e…

…nsure that cache entries created by HEAD requests are not used to serve GET requests.
apache · Apr 27, 2023 · 5505e84 · 5505e84
1 parent 772cbf3
commit 5505e84
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 31 deletions.
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
@@ -623,7 +623,7 @@ private void handleCacheHit(
         final HttpClientContext context  = scope.clientContext;
         recordCacheHit(target, request);
         final Instant now = getCurrentDate();
-        if (suitabilityChecker.canCachedResponseBeUsed(target, request, entry, now)) {
+        if (suitabilityChecker.canCachedResponseBeUsed(request, entry, now)) {
             LOG.debug("Cache hit");
             try {
                 final SimpleHttpResponse cacheResponse = generateCachedResponse(request, context, entry, now);

diff --git a/...src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java b/...src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java
@@ -34,7 +34,6 @@
 import org.apache.hc.client5.http.utils.DateUtils;
 import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.HeaderElement;
-import org.apache.hc.core5.http.HttpHost;
 import org.apache.hc.core5.http.HttpRequest;
 import org.apache.hc.core5.http.HttpStatus;
 import org.apache.hc.core5.http.message.MessageSupport;
@@ -57,7 +56,7 @@ class CachedResponseSuitabilityChecker {
     private final CacheValidityPolicy validityStrategy;
 
     CachedResponseSuitabilityChecker(final CacheValidityPolicy validityStrategy,
-            final CacheConfig config) {
+                                     final CacheConfig config) {
         super();
         this.validityStrategy = validityStrategy;
         this.sharedCache = config.isSharedCache();
@@ -96,7 +95,7 @@ private boolean originInsistsOnFreshness(final HttpCacheEntry entry) {
             return false;
         }
         return validityStrategy.proxyRevalidate(entry) ||
-            validityStrategy.hasCacheControlDirective(entry, "s-maxage");
+                validityStrategy.hasCacheControlDirective(entry, "s-maxage");
     }
 
     private long getMaxStale(final HttpRequest request) {
@@ -131,17 +130,22 @@ private long getMaxStale(final HttpRequest request) {
      * Determine if I can utilize a {@link HttpCacheEntry} to respond to the given
      * {@link HttpRequest}
      *
-     * @param host
-     *            {@link HttpHost}
      * @param request
      *            {@link HttpRequest}
      * @param entry
      *            {@link HttpCacheEntry}
      * @param now
      *            Right now in time
      * @return boolean yes/no answer
+     * @since 5.3
      */
-    public boolean canCachedResponseBeUsed(final HttpHost host, final HttpRequest request, final HttpCacheEntry entry, final Instant now) {
+    public boolean canCachedResponseBeUsed(final HttpRequest request, final HttpCacheEntry entry, final Instant now) {
+
+        if (isGetRequestWithHeadCacheEntry(request, entry)) {
+            LOG.debug("Cache entry created by HEAD request cannot be used to serve GET request");
+            return false;
+        }
+
         if (!isFreshEnough(entry, request, now)) {
             LOG.debug("Cache entry is not fresh enough");
             return false;
@@ -169,7 +173,7 @@ public boolean canCachedResponseBeUsed(final HttpHost host, final HttpRequest re
 
         if (hasUnsupportedCacheEntryForGet(request, entry)) {
             LOG.debug("HEAD response caching enabled but the cache entry does not contain a " +
-                      "request method, entity or a 204 response");
+                    "request method, entity or a 204 response");
             return false;
         }
         final Iterator<HeaderElement> it = MessageSupport.iterate(request, HeaderConstants.CACHE_CONTROL);
@@ -266,6 +270,21 @@ public boolean isConditional(final HttpRequest request) {
         return hasSupportedEtagValidator(request) || hasSupportedLastModifiedValidator(request);
     }
 
+    /**
+     * Determines whether the given request is a {@link HeaderConstants#GET_METHOD} request and the associated cache entry was created by a
+     * {@link HeaderConstants#HEAD_METHOD} request.
+     *
+     * @param request The {@link HttpRequest} to check if it is a {@link HeaderConstants#GET_METHOD} request.
+     * @param entry   The {@link HttpCacheEntry} to check if it was created by a {@link HeaderConstants#HEAD_METHOD} request.
+     * @return true if the request is a {@link HeaderConstants#GET_METHOD} request and the cache entry was created by a
+     * {@link HeaderConstants#HEAD_METHOD} request, otherwise {@code false}.
+     * @since 5.3
+     */
+    public boolean isGetRequestWithHeadCacheEntry(final HttpRequest request, final HttpCacheEntry entry) {
+        return isGet(request) && HeaderConstants.HEAD_METHOD.equalsIgnoreCase(entry.getRequestMethod());
+    }
+
+
     /**
      * Check that conditionals that are part of this request match
      * @param request The current httpRequest being made
@@ -281,7 +300,7 @@ public boolean allConditionalsMatch(final HttpRequest request, final HttpCacheEn
         final boolean lastModifiedValidatorMatches = (hasLastModifiedValidator) && lastModifiedValidatorMatches(request, entry, now);
 
         if ((hasEtagValidator && hasLastModifiedValidator)
-            && !(etagValidatorMatches && lastModifiedValidatorMatches)) {
+                && !(etagValidatorMatches && lastModifiedValidatorMatches)) {
             return false;
         } else if (hasEtagValidator && !etagValidatorMatches) {
             return false;
@@ -356,4 +375,4 @@ private boolean hasValidDateField(final HttpRequest request, final String header
         }
         return false;
     }
-}
+}
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
@@ -254,7 +254,7 @@ private ClassicHttpResponse handleCacheHit(
         context.setAttribute(HttpCoreContext.HTTP_REQUEST, request);
         recordCacheHit(target, request);
         final Instant now = getCurrentDate();
-        if (suitabilityChecker.canCachedResponseBeUsed(target, request, entry, now)) {
+        if (suitabilityChecker.canCachedResponseBeUsed(request, entry, now)) {
             LOG.debug("Cache hit");
             try {
                 return convert(generateCachedResponse(request, context, entry, now), scope);

diff --git a/...test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java b/...test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java
@@ -28,6 +28,7 @@
 
 import java.time.Instant;
 
+import org.apache.hc.client5.http.cache.HeaderConstants;
 import org.apache.hc.client5.http.cache.HttpCacheEntry;
 import org.apache.hc.client5.http.utils.DateUtils;
 import org.apache.hc.core5.http.Header;
@@ -78,7 +79,7 @@ public void testNotSuitableIfContentLengthHeaderIsWrong() {
                 new BasicHeader("Content-Length","1")
         };
         entry = getEntry(headers);
-        Assertions.assertFalse(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertFalse(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -89,7 +90,7 @@ public void testSuitableIfCacheEntryIsFresh() {
                 new BasicHeader("Content-Length","128")
         };
         entry = getEntry(headers);
-        Assertions.assertTrue(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertTrue(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -100,7 +101,7 @@ public void testNotSuitableIfCacheEntryIsNotFresh() {
                 new BasicHeader("Content-Length","128")
         };
         entry = getEntry(headers);
-        Assertions.assertFalse(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertFalse(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -112,7 +113,7 @@ public void testNotSuitableIfRequestHasNoCache() {
                 new BasicHeader("Content-Length","128")
         };
         entry = getEntry(headers);
-        Assertions.assertFalse(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertFalse(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -124,7 +125,7 @@ public void testNotSuitableIfAgeExceedsRequestMaxAge() {
                 new BasicHeader("Content-Length","128")
         };
         entry = getEntry(headers);
-        Assertions.assertFalse(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertFalse(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -136,7 +137,7 @@ public void testSuitableIfFreshAndAgeIsUnderRequestMaxAge() {
                 new BasicHeader("Content-Length","128")
         };
         entry = getEntry(headers);
-        Assertions.assertTrue(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertTrue(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -148,7 +149,7 @@ public void testSuitableIfFreshAndFreshnessLifetimeGreaterThanRequestMinFresh()
                 new BasicHeader("Content-Length","128")
         };
         entry = getEntry(headers);
-        Assertions.assertTrue(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertTrue(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -160,7 +161,7 @@ public void testNotSuitableIfFreshnessLifetimeLessThanRequestMinFresh() {
                 new BasicHeader("Content-Length","128")
         };
         entry = getEntry(headers);
-        Assertions.assertFalse(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertFalse(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -172,7 +173,7 @@ public void testSuitableEvenIfStaleButPermittedByRequestMaxStale() {
                 new BasicHeader("Content-Length","128")
         };
         entry = getEntry(headers);
-        Assertions.assertTrue(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertTrue(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -184,7 +185,7 @@ public void testNotSuitableIfStaleButTooStaleForRequestMaxStale() {
                 new BasicHeader("Content-Length","128")
         };
         entry = getEntry(headers);
-        Assertions.assertFalse(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertFalse(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
 
@@ -197,7 +198,7 @@ public void testMalformedCacheControlMaxAgeRequestHeaderCausesUnsuitableEntry()
                 new BasicHeader("Content-Length","128")
         };
         entry = getEntry(headers);
-        Assertions.assertFalse(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertFalse(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -209,7 +210,7 @@ public void testMalformedCacheControlMinFreshRequestHeaderCausesUnsuitableEntry(
                 new BasicHeader("Content-Length","128")
         };
         entry = getEntry(headers);
-        Assertions.assertFalse(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertFalse(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -230,7 +231,7 @@ public void testSuitableIfCacheEntryIsHeuristicallyFreshEnough() {
             .setHeuristicCoefficient(0.1f).build();
         impl = new CachedResponseSuitabilityChecker(config);
 
-        Assertions.assertTrue(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertTrue(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -248,7 +249,7 @@ public void testSuitableIfCacheEntryIsHeuristicallyFreshEnoughByDefault() {
             .build();
         impl = new CachedResponseSuitabilityChecker(config);
 
-        Assertions.assertTrue(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertTrue(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -261,7 +262,7 @@ public void testSuitableIfRequestMethodisHEAD() {
         };
         entry = getEntry(headers);
 
-        Assertions.assertTrue(impl.canCachedResponseBeUsed(host, headRequest, entry, now));
+        Assertions.assertTrue(impl.canCachedResponseBeUsed(headRequest, entry, now));
     }
 
     @Test
@@ -273,7 +274,7 @@ public void testNotSuitableIfRequestMethodIsGETAndEntryResourceIsNull() {
         };
         entry = HttpTestUtils.makeHeadCacheEntry(headers);
 
-        Assertions.assertFalse(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertFalse(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -286,7 +287,7 @@ public void testNotSuitableForGETIfEntryDoesNotSpecifyARequestMethodOrEntity() {
         };
         entry = HttpTestUtils.makeCacheEntryWithNoRequestMethodOrEntity(headers);
 
-        Assertions.assertFalse(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertFalse(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -299,7 +300,7 @@ public void testSuitableForGETIfEntryDoesNotSpecifyARequestMethodButContainsEnti
         };
         entry = HttpTestUtils.makeCacheEntryWithNoRequestMethod(headers);
 
-        Assertions.assertTrue(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertTrue(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -311,7 +312,7 @@ public void testSuitableForGETIfHeadResponseCachingEnabledAndEntryDoesNotSpecify
         };
         entry = HttpTestUtils.make204CacheEntryWithNoRequestMethod(headers);
 
-        Assertions.assertTrue(impl.canCachedResponseBeUsed(host, request, entry, now));
+        Assertions.assertTrue(impl.canCachedResponseBeUsed(request, entry, now));
     }
 
     @Test
@@ -325,6 +326,19 @@ public void testSuitableForHEADIfHeadResponseCachingEnabledAndEntryDoesNotSpecif
         };
         entry = HttpTestUtils.makeHeadCacheEntryWithNoRequestMethod(headers);
 
-        Assertions.assertTrue(impl.canCachedResponseBeUsed(host, headRequest, entry, now));
+        Assertions.assertTrue(impl.canCachedResponseBeUsed(headRequest, entry, now));
+    }
+
+    @Test
+    public void testNotSuitableIfGetRequestWithHeadCacheEntry() {
+        // Prepare a cache entry with HEAD method
+        final Header[] headers = {
+                new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)),
+                new BasicHeader("Cache-Control", "max-age=3600"),
+                new BasicHeader("Hc-Request-Method", HeaderConstants.HEAD_METHOD)
+        };
+        entry = getEntry(headers);
+        // Validate that the cache entry is not suitable for the GET request
+        Assertions.assertFalse(impl.canCachedResponseBeUsed(request, entry, now));
     }
 }