Re: [PATCH] support parameters for no-cache and private

On 02/16/2013 06:51 PM, Amos Jeffries wrote:

> 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.

The patch appears to do more than just detection -- Squid starts to
ignore CC:no-cache and, in some cases, CC:private (with or without
parameters, depending on circumstances).

> 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

> + } else if (/* p &&*/ !httpHeaderParseQuotedString(p, (ilen-nlen-1), &private_)) {
> + debugs(65, 2, "cc: invalid private= specs near '" << item << "'");
> + clearPrivate();
> + } else {

It feels wrong to clear and ignore a private CC directive that we know
is there, even though we cannot parse its value. Should we be
conservative and treat this as private without parameters instead of
possibly violating HTTP by ignoring CC:private (e.g., if our parser is
wrong)?

Also, if this code stays, please reshape it to handle the successful
parsing case (with side-effect of setting private_) first, for clarity:

  if (!p) {
     private_.clean(); // last CC:private wins
     setMask(true, true);
  } else if (parse(private_)) {
    setMask(true, true); // the above [re]sets private_
  } else {
    clearPrivate(); // ignore this and previous CC:private
  }

Same concerns for no-cache, of course.

> + void Private(String &v) {setMask(CC_PRIVATE,true); private_.append(v);} // uses append for multi-line headers

Would not append() merge two header names together without delimiters if
the directive is split between two header fields? For example, would not
the private_ value become "foobar" in the example below?

  Foo: 1
  Cache-Control: private="foo", private="bar"
  Bar: 2

For the future code to work, the values from multiple directives should
be concatenated using ",". However, it may be better to store them in a
list or map of some kind instead (since that future code will have to
check individual header names for a match anyway). Thus, the best
structure for this may be rather different. This doubles my concern that
we are adding code that should be essentially unused (IMO), that adds
overhead, but that may have to be fixed/rewritten further when it is used.

Also, I think the code will mishandle this case:

  Foo: 1
  Cache-Control: private
  Cache-Control: private="bar"
  Bar: 2

because it will treat the above as the following less restrictive case:

  Foo: 1
  Cache-Control: private="bar"
  Bar: 2

instead of treating it as

  Foo: 1
  Cache-Control: private
  Bar: 2

Does HTTP say that [later] less restrictive CC directives overwrite
[earlier] more restrictive ones?

> + // CC:no-cache (not only if there are no parameters)
> + const bool CcNoCacheNoParams = (rep->cache_control->hasNoCache() && rep->cache_control->noCache().undefined());

The comment says "not only if there are no parameters" but the boolean
condition says "only if there are no parameters".

> + // CC:private (if stored)
> + const bool CcPrivate = rep->cache_control->hasPrivate();

Does "if stored" apply to all other CC directives in this context as well?

Please start local variable names such as CcPrivate with a small letter.

> // NP: request CC:no-cache only means cache READ is forbidden. STORE is permitted.
> + if (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;
> + }

The new comment and behavior seems inconsistent, especially in light of
the old "NP" comment that is left intact: It seems like we should store
(or not) regardless of the header names presence. Why does presence of
header names make a difference in this case?

> // 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)) {

Same concern here. Perhaps I am missing the meaning of including those
header names, but I think if we cannot handle them properly, we should
ignore their presence instead of ignoring the presence of the entire
CC:no-cache directive!

> - debugs(22, 3, HERE << " Authenticated but server reply Cache-Control:s-maxage");
> + debugs(22, 3, HERE << "Authenticated but server reply Cache-Control:s-maxage");

Please remove this non-change.

Thank you,

Alex.
Received on Mon Feb 18 2013 - 17:48:23 MST