10000 bug #26532 [HttpKernel] Correctly merging cache directives in HttpCac… · symfony/symfony@bb90359 · GitHub
[go: up one dir, main page]
Skip to content

Commit bb90359

Browse files
fabpotfabpot
committed
bug #26532 [HttpKernel] Correctly merging cache directives in HttpCache/ResponseCacheStrategy (aschempp)
This PR was squashed before being merged into the 3.4 branch (closes #26532). Discussion ---------- [HttpKernel] Correctly merging cache directives in HttpCache/ResponseCacheStrategy | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26245, #26352, #28872 | License | MIT | Doc PR | - This PR is a first draft to fix the incorrect merging of private and other cache-related headers that are not meant for the shared cache but the browser (see mentioned issues). The existing implementation of `HttpFoundation\Response` is very much tailored to the `HttpCache`, for example `isCacheable` returns `false` if the response is `private`, which is not true for a browser cache. That is why my implementation does not longer use much of the response methods. They are however still used by the `HttpCache` and we should keep them as-is. FYI, the `ResponseCacheStrategy` does **not** affect the stored data of `HttpCache` but is only applied to the result of multiple merged subrequests/ESI responses. I did read up a lot on RFC2616 as a reference. [Section 13.4](https://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.4) gives an overall view of when a response MAY be cached. [Section 14.9.1](https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.1) has more insight into the `Cache-Control` directives. Here's a summary of the relevant information I applied to the implementation: - > Unless specifically constrained by a cache-control (section 14.9) directive, a caching system MAY always store a successful response (see section 13.8) as a cache entry, MAY return it without validation if it is fresh, and MAY return it after successful validation. A response without cache control headers is totally fine, and it's up to the cache (shared or private) to decide what to do with it. That is why the implementation does not longer set `no-cache` if no `Cache-Control` headers are present. - > A response received with a status code of 200, 203, 206, 300, 301 or 410 MAY be stored […] unless a cache-control directive prohibits caching. > A response received with any other status code (e.g. status codes 302 and 307) MUST NOT be returned […] unless there are cache-control directives or another header(s) that explicitly allow it. This is what `ResponseCacheStrategy::isUncacheable` implements to decide whether a response is not cacheable at all. It differs from `Response::isCacheable` which only returns true if there are actual `Cache-Control` headers. - > [Section 13.2.3](https://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.2.3): When a response is generated from a cache entry, the cache MUST include a single Age header field in the response with a value equal to the cache entry's current_age. That's why the implementation **always** adds the `Age` header. It takes the oldest age of any of the responses as common denominator for the content. - > [Section 14.9.3](https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.3): If a response includes an s-maxage directive, then for a shared cache (but not for a private cache), the maximum age specified by this directive overrides the maximum age specified by either the max-age directive or the Expires header. This effectively means that `max-age`, `s-maxage` and `Expires` must all be kept on the response. My implementation assumes that we can only do that if they exist in **all** of the responses, and then takes the lowest value of any of them. Be aware the implementation might look confusing at first. Due to the fact that the `Age` header might come from another subresponse than the lowest expiration value, the values are stored relative to the current response date and then re-calculated based on the age header. The Symfony implementation did not and still does not implement the full RFC. As an example, some of the `Cache-Control` headers (like `private` and `no-cache`) MAY actually have a string value, but the implementation only supports boolean. Also, [Custom `Cache-Control` headers](https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.6) are currently not merged into the final response. **ToDo/Questions:** 1. [Section 13.5.2](https://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.5.2) specifies that we must add a [`Warning 214 Transformation applied`](https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.46) if we modify the response headers. 2. Should we add an `Expires` headers based on `max-age` if none is explicitly set in the responses? This would essentially provide the same information as `max-age` but with support for HTTP/1.0 proxies/clients. 3. I'm not sure about the implemented handling of the `private` directive. The directive is currently only added to the final response if it is present in all of the subresponses. This can effectively result in no cache-control directive, which does not tell a shared cache that the response must not be cached. However, adding a `private` might also tell a browser to actually cache it, even though non of the other responses asked for that. 4. > [Section 14.9.2](https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.2): The purpose of the `no-store` directive is to prevent the inadvertent release or retention of sensitive information […]. The `no-store` directive applies to the entire message, and MAY be sent either in a response or in a request. If sent in a request, a cache MUST NOT store any part of either this request or any response to it. If sent in a response, a cache MUST NOT store any part of either this response or the request that elicited it. I have not (yet) validated whether the `HttpCache` implementation respects any of this. 5. As far as I understand, the current implementation of [`ResponseHeaderBag::computeCacheControlValue`](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/ResponseHeaderBag.php#L313) is incorrect. `no-cache` means a response [must not be cached by a shared or private cache](https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.1), which overrides `private` automatically. 5. The unit tests are still very limited and I want to add plenty more to test and sort-of describe the implementation or assumptions on the RFC. /cc @nicolas-grekas #SymfonyConHackday2018 Commits ------- 893118f [HttpKernel] Correctly merging cache directives in HttpCache/ResponseCacheStrategy
2 parents 20b5fb0 + 893118f commit bb90359

File tree

2 files changed

+391
-36
lines changed

2 files changed

+391
-36
lines changed

‎src/Symfony/Component/HttpKernel/HttpCache/ResponseCacheStrategy.php

Lines changed: 162 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,6 @@
55
*
66
* (c) Fabien Potencier <fabien@symfony.com>
77
*
8-
* This code is partially based on the Rack-Cache library by Ryan Tomayko,
9-
* which is released under the MIT license.
10-
* (based on commit 02d2b48d75bcb63cf1c0c7149c077ad256542801)
11-
*
128
* For the full copyright and license information, please view the LICENSE
139
* file that was distributed with this source code.
1410
*/
@@ -28,30 +24,69 @@
2824
*/
2925
class ResponseCacheStrategy implements ResponseCacheStrategyInterface
3026
{
31-
private $cacheable = true;
27+
/**
28+
* Cache-Control headers that are sent to the final response if they appear in ANY of the responses.
29+
*/
30+
private static $overrideDirectives = ['private', 'no-cache', 'no-store', 'no-transform', 'must-revalidate', 'proxy-revalidate'];
31+
32+
/**
33+
* Cache-Control headers that are sent to the final response if they appear in ALL of the responses.
34+
*/
35+
private static $inheritDirectives = ['public', 'immutable'];
36+
3237
private $embeddedResponses = 0;
33-
private $ttls = [];
34-
private $maxAges = [];
3538
private $isNotCacheableResponseEmbedded = false;
39+
private $age = 0;
40+
private $flagDirectives = [
41+
'no-cache' => null,
42+
'no-store' => null,
43+
'no-transform' => null,
44+
'must-revalidate' => null,
45+
'proxy-revalidate' => null,
46+
'public' => null,
47+
'private' => null,
48+
'immutable' => null,
49+
];
50+
private $ageDirectives = [
51+
'max-age' => null,
52+
's-maxage' => null,
53+
'expires' => null,
54+
];
3655

3756
/**
3857
* {@inheritdoc}
3958
*/
4059
public function add(Response $response)
4160
{
42-
if (!$response->isFresh() || !$response->isCacheable()) {
43-
$this->cacheable = false;
44-
} else {
45-
$maxAge = $response->getMaxAge();
46-
$this->ttls[] = $response->getTtl();
47-
$this->maxAges[] = $maxAge;
48-
49-
if (null === $maxAge) {
50-
$this->isNotCacheableResponseEmbedded = true;
61+
++$this->embeddedResponses;
62+
63+
foreach (self::$overrideDirectives as $directive) {
64+
if ($response->headers->hasCacheControlDirective($directive)) {
65+
$this->flagDirectives[$directive] = true;
5166
}
5267
}
5368

54-
++$this->embeddedResponses;
69+
foreach (self::$inheritDirectives as $directive) {
70+
if (false !== $this->flagDirectives[$directive]) {
71+
$this->flagDirectives[$directive] = $response->headers->hasCacheControlDirective($directive);
72+
}
73+
}
74+
75+
$age = $response->getAge();
76+
$this->age = max($this->age, $age);
77+
78+
if ($this->willMakeFinalResponseUncacheable($response)) {
79+
$this->isNotCacheableResponseEmbedded = true;
80+
81+
return;
82+
}
83+
84+
$this->storeRelativeAgeDirective('max-age', $response->headers->getCacheControlDirective('max-age'), $age);
85+
$this->storeRelativeAgeDirective('s-maxage', $response->headers->getCacheControlDirective('s-maxage') ?: $response->headers->getCacheControlDirective('max-age'), $age);
86+
87+
$expires = $response->getExpires();
88+
$expires = null !== $expires ? $expires->format('U') - $response->getDate()->format('U') : null;
89+
$this->storeRelativeAgeDirective('expires', $expires >= 0 ? $expires : null, 0);
5590
}
5691

5792
/**
@@ -64,33 +99,124 @@ public function update(Response $response)
6499
return;
65100
}
66101

67-
// Remove validation related headers in order to avoid browsers using
68-
// their own cache, because some of the response content comes from
69-
// at least one embedded response (which likely has a different caching strategy).
70-
if ($response->isValidateable()) {
71-
$response->setEtag(null);
72-
$response->setLastModified(null);
102+
// Remove validation related headers of the master response,
103+
// because some of the response content comes from at least
104+
// one embedded response (which likely has a different caching strategy).
105+
$response->setEtag(null);
106+
$response->setLastModified(null);
107+
108+
$this->add($response);
109+
110+
$response->headers->set('Age', $this->age);
111+
112+
if ($this->isNotCacheableResponseEmbedded) {
113+
$response->setExpires($response->getDate());
114+
115+
if ($this->flagDirectives['no-store']) {
116+
$response->headers->set('Cache-Control', 'no-cache, no-store, must-revalidate');
117+
} else {
118+
$response->headers->set('Cache-Control', 'no-cache, must-revalidate');
119+
}
120+
121+
return;
122+
}
123+
124+
$flags = array_filter($this->flagDirectives);
125+
126+
if (isset($flags['must-revalidate'])) {
127+
$flags['no-cache'] = true;
73128
}
74129

75-
if (!$response->isFresh() || !$response->isCacheable()) {
76-
$this->cacheable = false;
130+
$response->headers->set('Cache-Control', implode(', ', array_keys($flags)));
131+
132+
$maxAge = null;
133+
$sMaxage = null;
134+
135+
if (\is_numeric($this->ageDirectives['max-age'])) {
136+
$maxAge = $this->ageDirectives['max-age'] + $this->age;
137+
$response->headers->addCacheControlDirective('max-age', $maxAge);
77138
}
78139

79-
if (!$this->cacheable) {
80-
$response->headers->set('Cache-Control', 'no-cache, must-revalidate');
140+
if (\is_numeric($this->ageDirectiv 10000 es['s-maxage'])) {
141+
$sMaxage = $this->ageDirectives['s-maxage'] + $this->age;
81142

82-
return;
143+
if ($maxAge !== $sMaxage) {
144+
$response->headers->addCacheControlDirective('s-maxage', $sMaxage);
145+
}
146+
}
147+
148+
if (\is_numeric($this->ageDirectives['expires'])) {
149+
$date = clone $response->getDate();
150+
$date->modify('+'.($this->ageDirectives['expires'] + $this->age).' seconds');
151+
$response->setExpires($date);
83152
}
153+
}
84154

85-
$this->ttls[] = $response->getTtl();
86-
$this->maxAges[] = $response->getMaxAge();
155+
/**
156+
* RFC2616, Section 13.4.
157+
*
158+
* @see https://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.4
159+
*
160+
* @return bool
161+
*/
162+
private function willMakeFinalResponseUncacheable(Response $response)
163+
{
164+
// RFC2616: A response received with a status code of 200, 203, 300, 301 or 410
165+
// MAY be stored by a cache […] unless a cache-control directive prohibits caching.
166+
if ($response->headers->hasCacheControlDirective('no-cache')
167+
|| $response->headers->getCacheControlDirective('no-store')
168+
) {
169+
return true;
170+
}
87171

88-
if ($this->isNotCacheableResponseEmbedded) {
89-
$response->headers->removeCacheControlDirective('s-maxage');
90-
} elseif (null !== $maxAge = min($this->maxAges)) {
91-
$response->setSharedMaxAge($maxAge);
92-
$response->headers->set('Age', $maxAge - min($this->ttls));
172+
// Last-Modified and Etag headers cannot be merged, they render the response uncacheable
173+
// by default (except if the response also has max-age etc.).
174+
if (\in_array($response->getStatusCode(), [200, 203, 300, 301, 410])
175+
&& null === $response->getLastModified()
176+
&& null === $response->getEtag()
177+
) {
178+
return false;
179+
}
180+
181+
// RFC2616: A response received with any other status code (e.g. status codes 302 and 307)
182+
// MUST NOT be returned in a reply to a subsequent request unless there are
183+
// cache-control directives or another header(s) that explicitly allow it.
184+
$cacheControl = ['max-age', 's-maxage', 'must-revalidate', 'proxy-revalidate', 'public', 'private'];
185+
foreach ($cacheControl as $key) {
186+
if ($response->headers->hasCacheControlDirective($key)) {
187+
return false;
188+
}
189+
}
190+
191+
if ($response->headers->has('Expires')) {
192+
return false;
193+
}
194+
195+
return true;
196+
}
197+
198+
/**
199+
* Store lowest max-age/s-maxage/expires for the final response.
200+
*
201+
* The response might have been stored in cache a while ago. To keep things comparable,
202+
* we have to subtract the age so that the value is normalized for an age of 0.
203+
*
204+
* If the value is lower than the currently stored value, we update the value, to keep a rolling
205+
* minimal value of each instruction. If the value is NULL, the directive will not be set on the final response.
206+
*
207+
* @param string $directive
208+
* @param int|null $value
209+
* @param int $age
210+
*/
211+
private function storeRelativeAgeDirective($directive, $value, $age)
212+
{
213+
if (null === $value) {
214+
$this->ageDirectives[$directive] = false;
215+
}
216+
217+
if (false !== $this->ageDirectives[$directive]) {
218+
$value = $value - $age;
219+
$this->ageDirectives[$directive] = null !== $this->ageDirectives[$directive] ? min($this->ageDirectives[$directive], $value) : $value;
93220
}
94-
$response->setMaxAge(0);
95221
}
96222
}

0 commit comments

Comments
 (0)
0