From: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Date: Tue, 6 Sep 2016 07:41:16 +0000 (+1200)
Subject: HTTP: MUST always revalidate Cache-Control:no-cache responses.
X-Git-Tag: SQUID_4_0_14~9
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=fa83b766a208b27abed8da4c9073cf8784cf10fa;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 a7e7cfa445..6151b13f46 100644
--- a/src/enums.h
+++ b/src/enums.h
@@ -67,11 +67,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 a0a30cb80b..847e47be10 100644
--- a/src/http.cc
+++ b/src/http.cc
@@ -988,8 +988,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 {
@@ -999,7 +1001,7 @@ HttpStateData::haveParsedReplyHeaders()
              * but servers like "Active Imaging Webcast/2.0" sure do use it */
             if (rep->header.has(Http::HdrType::PRAGMA) &&
                     rep->header.hasListMember(Http::HdrType::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 44d23335b5..a8364b350a 100644
--- a/src/refresh.cc
+++ b/src/refresh.cc
@@ -346,8 +346,12 @@ 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) {
-        debugs(22, 3, "YES: Must revalidate stale object (origin set must-revalidate, proxy-revalidate, no-cache, s-maxage, or private)");
+    const bool revalidateAlways = EBIT_TEST(entry->flags, ENTRY_REVALIDATE_ALWAYS);
+    if (revalidateAlways || (staleness > -1 &&
+                EBIT_TEST(entry->flags, ENTRY_REVALIDATE_STALE))) {
+        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 c2b1073541..5ece07850d 100644
--- a/src/stat.cc
+++ b/src/stat.cc
@@ -288,8 +288,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,");
@@ -300,6 +300,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 494eb3cb5f..b77d5562e5 100644
--- a/src/store.cc
+++ b/src/store.cc
@@ -2098,10 +2098,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';