From: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Date: Wed, 7 Sep 2016 15:58:59 +0000 (+1200)
Subject: HTTP: MUST always revalidate Cache-Control:no-cache responses.
X-Git-Tag: SQUID_3_5_21~5
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=25f3da5db37807a011e25a8a1b9013a2bf3fb0b6;p=thirdparty%2Fsquid.git

HTTP: MUST always revalidate Cache-Control:no-cache responses.

Squid MUST NOT use a CC:no-cache response for satisfying subsequent
requests without successful validation with the origin server. Squid
violated this MUST because Squid mapped both "no-cache" and
"must-revalidate"/etc. directives to the same ENTRY_REVALIDATE flag
which was interpreted as "revalidate when the cached response becomes
stale" rather than "revalidate on every request".

This patch splits ENTRY_REVALIDATE into:
- ENTRY_REVALIDATE_ALWAYS, for entries with CC:no-cache and
- ENTRY_REVALIDATE_STALE, for entries with other related CC directives
  like CC:must-revalidate and CC:proxy-revalidate.

Reusing ENTRY_CACHABLE_RESERVED_FOR_FUTURE_USE value for the more
forgiving ENTRY_REVALIDATE_STALE (instead of the more aggressive
ENTRY_REVALIDATE_ALWAYS) fixes the bug for the already cached entries -
they will be revalidated and stored with the correct flag on the next
request.
---

diff --git a/src/enums.h b/src/enums.h
index 4d04805e6d..07c90cc0ff 100644
--- a/src/enums.h
+++ b/src/enums.h
@@ -95,11 +95,11 @@ typedef enum {
  */
 enum {
     ENTRY_SPECIAL,
-    ENTRY_REVALIDATE,
+    ENTRY_REVALIDATE_ALWAYS,
     DELAY_SENDING,
     RELEASE_REQUEST,
     REFRESH_REQUEST,
-    ENTRY_CACHABLE_RESERVED_FOR_FUTURE_USE,
+    ENTRY_REVALIDATE_STALE,
     ENTRY_DISPATCHED,
     KEY_PRIVATE,
     ENTRY_FWD_HDR_WAIT,
diff --git a/src/http.cc b/src/http.cc
index 80bac019a9..4507f5353a 100644
--- a/src/http.cc
+++ b/src/http.cc
@@ -958,8 +958,10 @@ HttpStateData::haveParsedReplyHeaders()
             // CC:private (yes, these can sometimes be stored)
             const bool ccPrivate = rep->cache_control->hasPrivate();
 
-            if (ccMustRevalidate || ccNoCacheNoParams || ccSMaxAge || ccPrivate)
-                EBIT_SET(entry->flags, ENTRY_REVALIDATE);
+            if (ccNoCacheNoParams || ccPrivate)
+                EBIT_SET(entry->flags, ENTRY_REVALIDATE_ALWAYS);
+            else if (ccMustRevalidate || ccSMaxAge)
+                EBIT_SET(entry->flags, ENTRY_REVALIDATE_STALE);
         }
 #if USE_HTTP_VIOLATIONS // response header Pragma::no-cache is undefined in HTTP
         else {
@@ -969,7 +971,7 @@ HttpStateData::haveParsedReplyHeaders()
              * but servers like "Active Imaging Webcast/2.0" sure do use it */
             if (rep->header.has(HDR_PRAGMA) &&
                     rep->header.hasListMember(HDR_PRAGMA,"no-cache",','))
-                EBIT_SET(entry->flags, ENTRY_REVALIDATE);
+                EBIT_SET(entry->flags, ENTRY_REVALIDATE_ALWAYS);
         }
 #endif
     }
diff --git a/src/refresh.cc b/src/refresh.cc
index 19c71980df..4d62504689 100644
--- a/src/refresh.cc
+++ b/src/refresh.cc
@@ -356,12 +356,15 @@ refreshCheck(const StoreEntry * entry, HttpRequest * request, time_t delta)
      *   Cache-Control: proxy-revalidate
      * the spec says the response must always be revalidated if stale.
      */
-    if (EBIT_TEST(entry->flags, ENTRY_REVALIDATE) && staleness > -1
+    const bool revalidateAlways = EBIT_TEST(entry->flags, ENTRY_REVALIDATE_ALWAYS);
+    bool revalidateStale = staleness > -1 && EBIT_TEST(entry->flags, ENTRY_REVALIDATE_STALE);
 #if USE_HTTP_VIOLATIONS
-            && !R->flags.ignore_must_revalidate
+    revalidateStale = revalidateStale && !R->flags.ignore_must_revalidate;
 #endif
-       ) {
-        debugs(22, 3, "YES: Must revalidate stale object (origin set must-revalidate or proxy-revalidate)");
+    if (revalidateAlways || revalidateStale) {
+        debugs(22, 3, "YES: Must revalidate stale object (origin set " <<
+                (revalidateAlways ? "no-cache or private" :
+                 "must-revalidate, proxy-revalidate or s-maxage") << ")");
         if (request)
             request->flags.failOnValidationError = true;
         return STALE_MUST_REVALIDATE;
diff --git a/src/stat.cc b/src/stat.cc
index ae3dd697f0..ca2f07b065 100644
--- a/src/stat.cc
+++ b/src/stat.cc
@@ -286,8 +286,8 @@ storeEntryFlags(const StoreEntry * entry)
     if (EBIT_TEST(flags, ENTRY_SPECIAL))
         strcat(buf, "SPECIAL,");
 
-    if (EBIT_TEST(flags, ENTRY_REVALIDATE))
-        strcat(buf, "REVALIDATE,");
+    if (EBIT_TEST(flags, ENTRY_REVALIDATE_ALWAYS))
+        strcat(buf, "REVALIDATE_ALWAYS,");
 
     if (EBIT_TEST(flags, DELAY_SENDING))
         strcat(buf, "DELAY_SENDING,");
@@ -298,6 +298,9 @@ storeEntryFlags(const StoreEntry * entry)
     if (EBIT_TEST(flags, REFRESH_REQUEST))
         strcat(buf, "REFRESH_REQUEST,");
 
+    if (EBIT_TEST(flags, ENTRY_REVALIDATE_STALE))
+        strcat(buf, "REVALIDATE_STALE,");
+
     if (EBIT_TEST(flags, ENTRY_DISPATCHED))
         strcat(buf, "DISPATCHED,");
 
diff --git a/src/store.cc b/src/store.cc
index cbb267650e..5db02bdf5a 100644
--- a/src/store.cc
+++ b/src/store.cc
@@ -2122,10 +2122,11 @@ std::ostream &operator <<(std::ostream &os, const StoreEntry &e)
     // print only set flags, using unique letters
     if (e.flags) {
         if (EBIT_TEST(e.flags, ENTRY_SPECIAL)) os << 'S';
-        if (EBIT_TEST(e.flags, ENTRY_REVALIDATE)) os << 'R';
+        if (EBIT_TEST(e.flags, ENTRY_REVALIDATE_ALWAYS)) os << 'R';
         if (EBIT_TEST(e.flags, DELAY_SENDING)) os << 'P';
         if (EBIT_TEST(e.flags, RELEASE_REQUEST)) os << 'X';
         if (EBIT_TEST(e.flags, REFRESH_REQUEST)) os << 'F';
+        if (EBIT_TEST(e.flags, ENTRY_REVALIDATE_STALE)) os << 'E';
         if (EBIT_TEST(e.flags, ENTRY_DISPATCHED)) os << 'D';
         if (EBIT_TEST(e.flags, KEY_PRIVATE)) os << 'I';
         if (EBIT_TEST(e.flags, ENTRY_FWD_HDR_WAIT)) os << 'W';