HTTP: MUST ignore a [revalidation] response with an older Date header.

Before this patch, Squid violated the RFC 7234 section 4 MUST
requirement: "When more than one suitable response is stored, a cache
MUST use the most recent response (as determined by the Date header
field)." This problem may be damaging in cache hierarchies where
parent caches may have different responses. Trusting the older response
may lead to excessive IMS requests, cache thrashing and other problems.

diff --git a/src/HttpReply.cc b/src/HttpReply.cc
index d94a135bfa4e43c96c5e74b412b65c3a556d10e5..23e8edcab8724dd9ee2bf1b02f4a11d52fc3e6f1 100644 (file)
--- a/src/HttpReply.cc
+++ b/src/HttpReply.cc
@@ -628,3 +628,11 @@ String HttpReply::removeStaleWarningValues(const String &value)
     return newValue;
 }
 
+bool
+HttpReply::olderThan(const HttpReply *them) const
+{
+    if (!them || !them->date || !date)
+        return false;
+    return date < them->date;
+}
+

diff --git a/src/HttpReply.h b/src/HttpReply.h
index c500b9f4a7f6d8a1a2f40363efe5d3e4fffdd853..29b4a6851775bcdfa291ba70792991b43f8a5505 100644 (file)
--- a/src/HttpReply.h
+++ b/src/HttpReply.h
@@ -112,6 +112,10 @@ public:
 
     virtual void hdrCacheInit();
 
+    /// whether our Date header value is smaller than theirs
+    /// \returns false if any information is missing
+    bool olderThan(const HttpReply *them) const;
+
 private:
     /** initialize */
     void init();

diff --git a/src/LogTags.h b/src/LogTags.h
index 3116933680561c8f879be394b8490698297e32a0..579fc0a8606e0fc8d9e8aff2bc41067495f34ab1 100644 (file)
--- a/src/LogTags.h
+++ b/src/LogTags.h
@@ -25,6 +25,7 @@ typedef enum {
     LOG_TCP_REFRESH_FAIL_OLD,   // refresh from origin failed, stale reply sent
     LOG_TCP_REFRESH_FAIL_ERR,   // refresh from origin failed, error forwarded
     LOG_TCP_REFRESH_MODIFIED,   // refresh from origin replaced existing entry
+    LOG_TCP_REFRESH_IGNORED,    // refresh from origin ignored, stale entry sent
     LOG_TCP_CLIENT_REFRESH_MISS,
     LOG_TCP_IMS_HIT,
     LOG_TCP_SWAPFAIL_MISS,

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 7a5fec1ca603edf42d40fd5b9f142144ba4c383a..a7046b86ce1d7bd1ae995836e13741af1dac3504 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -388,7 +388,7 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result)
     if (deleting)
         return;
 
-    debugs(88, 3, "handleIMSReply: " << http->storeEntry()->url() << ", " << (long unsigned) result.length << " bytes" );
+    debugs(88, 3, http->storeEntry()->url() << ", " << (long unsigned) result.length << " bytes");
 
     if (http->storeEntry() == NULL)
         return;
@@ -403,7 +403,7 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result)
 
     // request to origin was aborted
     if (EBIT_TEST(http->storeEntry()->flags, ENTRY_ABORTED)) {
-        debugs(88, 3, "handleIMSReply: request to origin aborted '" << http->storeEntry()->url() << "', sending old entry to client" );
+        debugs(88, 3, "request to origin aborted '" << http->storeEntry()->url() << "', sending old entry to client");
         http->logType = LOG_TCP_REFRESH_FAIL_OLD;
         sendClientOldEntry();
     }
@@ -423,11 +423,11 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result)
 
         if (http->request->flags.ims && !old_entry->modifiedSince(http->request->ims, http->request->imslen)) {
             // forward the 304 from origin
-            debugs(88, 3, "handleIMSReply: origin replied 304, revalidating existing entry and forwarding 304 to client");
+            debugs(88, 3, "origin replied 304, revalidating existing entry and forwarding 304 to client");
             sendClientUpstreamResponse();
         } else {
             // send existing entry, it's still valid
-            debugs(88, 3, "handleIMSReply: origin replied 304, revalidating existing entry and sending " <<
+            debugs(88, 3, "origin replied 304, revalidating existing entry and sending " <<
                    old_rep->sline.status() << " to client");
             sendClientOldEntry();
         }
@@ -435,26 +435,37 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result)
 
     // origin replied with a non-error code
     else if (status > Http::scNone && status < Http::scInternalServerError) {
-        // forward response from origin
-        http->logType = LOG_TCP_REFRESH_MODIFIED;
-        debugs(88, 3, "handleIMSReply: origin replied " << status << ", replacing existing entry and forwarding to client");
+        const HttpReply *new_rep = http->storeEntry()->getReply();
+        // RFC 7234 section 4: a cache MUST use the most recent response
+        // (as determined by the Date header field)
+        if (new_rep->olderThan(old_rep)) {
+            http->logType = LOG_TCP_REFRESH_IGNORED;
+            debugs(88, 3, "origin replied " << status <<
+                   " but with an older date header, sending old entry (" <<
+                   old_rep->sline.status() << ") to client");
+            sendClientOldEntry();
+        } else {
+            http->logType = LOG_TCP_REFRESH_MODIFIED;
+            debugs(88, 3, "origin replied " << status <<
+                   ", replacing existing entry and forwarding to client");
 
-        if (collapsedRevalidation)
-            http->storeEntry()->clearPublicKeyScope();
+            if (collapsedRevalidation)
+                http->storeEntry()->clearPublicKeyScope();
 
-        sendClientUpstreamResponse();
+            sendClientUpstreamResponse();
+        }
     }
 
     // origin replied with an error
     else if (http->request->flags.failOnValidationError) {
         http->logType = LOG_TCP_REFRESH_FAIL_ERR;
-        debugs(88, 3, "handleIMSReply: origin replied with error " << status <<
+        debugs(88, 3, "origin replied with error " << status <<
                ", forwarding to client due to fail_on_validation_err");
         sendClientUpstreamResponse();
     } else {
         // ignore and let client have old entry
         http->logType = LOG_TCP_REFRESH_FAIL_OLD;
-        debugs(88, 3, "handleIMSReply: origin replied with error " <<
+        debugs(88, 3, "origin replied with error " <<
                status << ", sending old entry (" << old_rep->sline.status() << ") to client");
         sendClientOldEntry();
     }

diff --git a/src/http.cc b/src/http.cc
index 4507f5353a6a58a2b19c4f6c28622f149cdb775b..24649aa04db57323ee6d73a4ef76d6d5eac318e9 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -84,7 +84,8 @@ void httpHdrAdd(HttpHeader *heads, HttpRequest *request, const AccessLogEntryPoi
 
 HttpStateData::HttpStateData(FwdState *theFwdState) : AsyncJob("HttpStateData"), Client(theFwdState),
     lastChunk(0), header_bytes_read(0), reply_bytes_read(0),
-    body_bytes_truncated(0), httpChunkDecoder(NULL)
+    body_bytes_truncated(0), httpChunkDecoder(NULL),
+    sawDateGoBack(false)
 {
     debugs(11,5,HERE << "HttpStateData " << this << " created");
     ignoreCacheControl = false;
@@ -169,6 +170,14 @@ HttpStateData::httpTimeout(const CommTimeoutCbParams &params)
     mustStop("HttpStateData::httpTimeout");
 }
 
+static StoreEntry *
+findPreviouslyCachedEntry(StoreEntry *newEntry) {
+    assert(newEntry->mem_obj);
+    return newEntry->mem_obj->request ?
+           storeGetPublicByRequest(newEntry->mem_obj->request) :
+           storeGetPublic(newEntry->mem_obj->storeId(), newEntry->mem_obj->method);
+}
+
 /// Remove an existing public store entry if the incoming response (to be
 /// stored in a currently private entry) is going to invalidate it.
 static void
@@ -176,7 +185,6 @@ httpMaybeRemovePublic(StoreEntry * e, Http::StatusCode status)
 {
     int remove = 0;
     int forbidden = 0;
-    StoreEntry *pe;
 
     // If the incoming response already goes into a public entry, then there is
     // nothing to remove. This protects ready-for-collapsing entries as well.
@@ -235,12 +243,7 @@ httpMaybeRemovePublic(StoreEntry * e, Http::StatusCode status)
     if (!remove && !forbidden)
         return;
 
-    assert(e->mem_obj);
-
-    if (e->mem_obj->request)
-        pe = storeGetPublicByRequest(e->mem_obj->request);
-    else
-        pe = storeGetPublic(e->mem_obj->storeId(), e->mem_obj->method);
+    StoreEntry *pe = findPreviouslyCachedEntry(e);
 
     if (pe != NULL) {
         assert(e != pe);
@@ -331,6 +334,13 @@ HttpStateData::cacheableReply()
         return 0;
     }
 
+    // RFC 7234 section 4: a cache MUST use the most recent response
+    // (as determined by the Date header field)
+    if (sawDateGoBack) {
+        debugs(22, 3, "NO because " << *entry << " has an older date header.");
+        return 0;
+    }
+
     // Check for Surrogate/1.0 protocol conditions
     // NP: reverse-proxy traffic our parent server has instructed us never to cache
     if (surrogateNoStore) {
@@ -886,7 +896,10 @@ HttpStateData::haveParsedReplyHeaders()
     /* Check if object is cacheable or not based on reply code */
     debugs(11, 3, "HTTP CODE: " << rep->sline.status());
 
-    if (neighbors_do_private_keys)
+    if (const StoreEntry *oldEntry = findPreviouslyCachedEntry(entry))
+        sawDateGoBack = rep->olderThan(oldEntry->getReply());
+
+    if (neighbors_do_private_keys && !sawDateGoBack)
         httpMaybeRemovePublic(entry, rep->sline.status());
 
     bool varyFailure = false;

diff --git a/src/http.h b/src/http.h
index 65bfccd5e194aaa82c5f7a819afab3141fc8b4a8..74f0e5c30a83edf92bb68eeb7bbae816585b24ad 100644 (file)
--- a/src/http.h
+++ b/src/http.h
@@ -112,6 +112,9 @@ private:
     bool peerSupportsConnectionPinning() const;
 
     ChunkedCodingParser *httpChunkDecoder;
+    /// Whether we received a Date header older than that of a matching
+    /// cached response.
+    bool sawDateGoBack;
 private:
     CBDATA_CLASS2(HttpStateData);
 };