On 23/02/2013 2:56 p.m., Amos Jeffries wrote:
On 23/02/2013 10:20 a.m., Alex Rousskov wrote:
On 02/21/2013 06:26 PM, Amos Jeffries wrote:
Adjusted patch to drop the odd NP, rework CC:private operation on
broken
parameters, and fix the segfault.
<snip>
Since we now support HTTP/1.1 storage and revalidation of
Cache-Control:no-cache it is important that we at least detect the
cases where no-cache= and private= contain parameters and handle them
properly whenever possible.
AFAIK these are still rare occurances due to the historic lack of
support. So for now Squid just detects and exempts these responses
from the caching performed. The basic framework for adding future
support of these HTTP/1.1 features is made available
Please run this past Co-Advisor to confirm the private="..." and
no-cache="..." cases are now all "Precondition Failed".
Confirmed. Please note that CC:private="..." cases were failing
precondition before the patch as well. Apparently, somebody fixed
CC:private handling some time after r12394 and before your patch.
Please re-check this adjusted patch. It should meet the additional
points of retaining the most conservative interpretation on broken input
headers.
Amos
=== modified file 'src/HttpHdrCc.cc'
--- src/HttpHdrCc.cc 2012-09-04 11:58:36 +0000
+++ src/HttpHdrCc.cc 2013-03-01 03:39:49 +0000
@@ -194,15 +194,42 @@
}
break;
+ case CC_PRIVATE: {
+ String temp;
+ if (!p) {
+ // Value parameter is optional.
+ private_.clean();
+ } else if (/* p &&*/ httpHeaderParseQuotedString(p,
(ilen-nlen-1), &temp)) {
+ private_.append(temp);
+ } else {
+ debugs(65, 2, "cc: invalid private= specs near '" << item <<
"'");
+ }
+ // to be safe we ignore broken parameters, but always remember the
'private' part.
+ setMask(type,true);
+ }
+ break;
+
+ case CC_NO_CACHE: {
+ String temp;
+ if (!p) {
+ // On Requests, missing value parameter is expected syntax.
+ // On Responses, value parameter is optional.
+ setMask(type,true);
+ no_cache.clean();
+ } else if (/* p &&*/ httpHeaderParseQuotedString(p, (ilen-nlen-1),
&temp)) {
+ // On Requests, a value parameter is invalid syntax.
+ // XXX: identify when parsing request header and dump err
message here.
+ setMask(type,true);
+ no_cache.append(temp);
+ } else {
+ debugs(65, 2, "cc: invalid no-cache= specs near '" << item <<
"'");
+ }
+ }
+ break;
+
case CC_PUBLIC:
Public(true);
break;
- case CC_PRIVATE:
- Private(true);
- break;
- case CC_NO_CACHE:
- noCache(true);
- break;
case CC_NO_STORE:
noStore(true);
break;
=== modified file 'src/HttpHdrCc.h'
--- src/HttpHdrCc.h 2012-09-21 14:57:30 +0000
+++ src/HttpHdrCc.h 2013-03-01 03:39:49 +0000
@@ -74,15 +74,27 @@
//manipulation for Cache-Control: private header
bool hasPrivate() const {return isSet(CC_PRIVATE);}
- bool Private() const {return isSet(CC_PRIVATE);}
- void Private(bool v) {setMask(CC_PRIVATE,v);}
- void clearPrivate() {setMask(CC_PRIVATE,false);}
+ const String &Private() const {return private_;}
+ void Private(String &v) {
+ setMask(CC_PRIVATE,true);
+ // uses append for multi-line headers
+ if (private_.defined())
+ private_.append(",");
+ private_.append(v);
+ }
+ void clearPrivate() {setMask(CC_PRIVATE,false); private_.clean();}
//manipulation for Cache-Control: no-cache header
bool hasNoCache() const {return isSet(CC_NO_CACHE);}
- bool noCache() const {return isSet(CC_NO_CACHE);}
- void noCache(bool v) {setMask(CC_NO_CACHE,v);}
- void clearNoCache() {setMask(CC_NO_CACHE,false);}
+ const String &noCache() const {return no_cache;}
+ void noCache(String &v) {
+ setMask(CC_NO_CACHE,true);
+ // uses append for multi-line headers
+ if (no_cache.defined())
+ no_cache.append(",");
+ no_cache.append(v);
+ }
+ void clearNoCache() {setMask(CC_NO_CACHE,false); no_cache.clean();}
//manipulation for Cache-Control: no-store header
bool hasNoStore() const {return isSet(CC_NO_STORE);}
@@ -166,6 +178,9 @@
int32_t max_stale;
int32_t stale_if_error;
int32_t min_fresh;
+ String private_; ///< List of headers sent as value for CC:private="...".
May be empty/undefined if the value is missing.
+ String no_cache; ///< List of header sent as value for CC:no-cache="...".
May be empty/undefined if the value is missing.
+
/// low-level part of the public set method, performs no checks
_SQUID_INLINE_ void setMask(http_hdr_cc_type id, bool newval=true);
_SQUID_INLINE_ void setValue(int32_t &value, int32_t new_value,
http_hdr_cc_type hdr, bool setting=true);
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2013-02-12 11:34:35 +0000
+++ src/client_side_request.cc 2013-03-01 03:40:38 +0000
@@ -1094,7 +1094,7 @@
if (!request->flags.ignoreCc) {
if (request->cache_control) {
- if (request->cache_control->noCache())
+ if (request->cache_control->hasNoCache())
no_cache=true;
// RFC 2616: treat Pragma:no-cache as if it was
Cache-Control:no-cache when Cache-Control is missing
=== modified file 'src/http.cc'
--- src/http.cc 2013-02-19 00:06:48 +0000
+++ src/http.cc 2013-03-01 03:40:58 +0000
@@ -362,6 +362,16 @@
}
// NP: request CC:no-cache only means cache READ is forbidden. STORE
is permitted.
+ if (rep->cache_control && rep->cache_control->hasNoCache() &&
rep->cache_control->noCache().defined()) {
+ /* TODO: we are allowed to cache when no-cache= has parameters.
+ * Provided we strip away any of the listed headers unless they
are revalidated
+ * successfully (ie, must revalidate AND these headers are
prohibited on stale replies).
+ * That is a bit tricky for squid right now so we avoid caching
entirely.
+ */
+ debugs(22, 3, HERE << "NO because server reply
Cache-Control:no-cache has parameters");
+ return 0;
+ }
+
// NP: request CC:private is undefined. We ignore.
// NP: other request CC flags are limiters on HIT/MISS. We don't care
about here.
@@ -373,16 +383,21 @@
}
// RFC 2616 section 14.9.1 - MUST NOT cache any response with
CC:private in a shared cache like Squid.
+ // CC:private overrides CC:public when both are present in a response.
// TODO: add a shared/private cache configuration possibility.
if (rep->cache_control &&
- rep->cache_control->Private() &&
+ rep->cache_control->hasPrivate() &&
!REFRESH_OVERRIDE(ignore_private)) {
+ /* TODO: we are allowed to cache when private= has parameters.
+ * Provided we strip away any of the listed headers unless they
are revalidated
+ * successfully (ie, must revalidate AND these headers are
prohibited on stale replies).
+ * That is a bit tricky for squid right now so we avoid caching
entirely.
+ */
debugs(22, 3, HERE << "NO because server reply
Cache-Control:private");
return 0;
}
- // NP: being conservative; CC:private overrides CC:public when both
are present in a response.
+ }
- }
// RFC 2068, sec 14.9.4 - MUST NOT cache any response with Authentication
UNLESS certain CC controls are present
// allow HTTP violations to IGNORE those controls (ie re-block caching
Auth)
if (request && (request->flags.auth || request->flags.authSent) &&
!REFRESH_OVERRIDE(ignore_auth)) {
@@ -411,8 +426,8 @@
// NP: given the must-revalidate exception we should also be able
to exempt no-cache.
// HTTPbis WG verdict on this is that it is omitted from the spec
due to being 'unexpected' by
// some. The caching+revalidate is not exactly unsafe though with
Squids interpretation of no-cache
- // as equivalent to must-revalidate in the reply.
- } else if (rep->cache_control->noCache() &&
!REFRESH_OVERRIDE(ignore_must_revalidate)) {
+ // (without parameters) as equivalent to must-revalidate in the
reply.
+ } else if (rep->cache_control->hasNoCache() &&
!rep->cache_control->noCache().defined() &&
!REFRESH_OVERRIDE(ignore_must_revalidate)) {
debugs(22, 3, HERE << "Authenticated but server reply
Cache-Control:no-cache (equivalent to must-revalidate)");
mayStore = true;
#endif
@@ -964,10 +979,22 @@
if (!ignoreCacheControl) {
if (rep->cache_control) {
- if (rep->cache_control->proxyRevalidate() ||
- rep->cache_control->mustRevalidate() ||
- rep->cache_control->noCache() ||
- rep->cache_control->hasSMaxAge())
+ // We are required to revalidate on many conditions.
+ // For security reasons we do so even if storage was caused by
refresh_pattern ignore-* option
+
+ // CC:must-revalidate or CC:proxy-revalidate
+ const bool ccMustRevalidate =
(rep->cache_control->proxyRevalidate() || rep->cache_control->mustRevalidate());
+
+ // CC:no-cache (only if there are no parameters)
+ const bool ccNoCacheNoParams = (rep->cache_control->hasNoCache()
&& rep->cache_control->noCache().undefined());
+
+ // CC:s-maxage=N
+ const bool ccSMaxAge = rep->cache_control->hasSMaxAge();
+
+ // 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 USE_HTTP_VIOLATIONS // response header Pragma::no-cache is undefined in
HTTP
@@ -1803,7 +1830,7 @@
#endif
/* Add max-age only without no-cache */
- if (!cc->hasMaxAge() && !cc->noCache()) {
+ if (!cc->hasMaxAge() && !cc->hasNoCache()) {
const char *url =
entry ? entry->url() : urlCanonical(request);
cc->maxAge(getMaxAge(url));
