bug #26643 Fix that ESI/SSI processing can turn a "private" response "public" (mpdude)

fabpot · fabpot · commit d17d38d29156 · 2018-04-16T19:47:59.000+02:00
This PR was squashed before being merged into the 2.7 branch (closes #26643). Discussion ---------- Fix that ESI/SSI processing can turn a "private" response "public" | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Under the condition that * we are merging in at least one *embedded* response, * all *embedded* responses are `public`, * the *main* response is `private` and * all responses use expiration-based caching (note: no `s-maxage` on the *main* response) ... the resulting response will turn to `Cache-Control: public`. The real issue is that when all responses use expiration-based caching, a combined max age is computed. This is set on the *main* response using `Response::setSharedMaxAge()`, which implicitly sets `Cache-Control: public`. The fix provided in this PR solves the problem by applying the same logic to the *main* response that is applied for *embedded* responses, namely that responses with `!Response::isCacheable()` will make the resulting response have `Cache-Control: private, no-cache, must-revalidate` and have `(s)max-age` removed. This makes the change easy to understand, but makes responses uncacheable too often. This is because the `Response::isCacheable()` method was written to determine whether it is safe for a shared cache to keep the response, which is not the case as soon as a `private` response is involved. This might be improved upon in another PR. Commits ------- 3d27b59 Fix that ESI/SSI processing can turn a \"private\" response \"public\"
diff --git a/src/Symfony/Component/HttpFoundation/Response.php b/src/Symfony/Component/HttpFoundation/Response.php
@@ -507,13 +507,19 @@ public function getCharset()
     }
 
     /**
-     * Returns true if the response is worth caching under any circumstance.
+     * Returns true if the response may safely be kept in a shared (surrogate) cache.
      *
      * Responses marked "private" with an explicit Cache-Control directive are
      * considered uncacheable.
      *
      * Responses with neither a freshness lifetime (Expires, max-age) nor cache
-     * validator (Last-Modified, ETag) are considered uncacheable.
+     * validator (Last-Modified, ETag) are considered uncacheable because there is
+     * no way to tell when or how to remove them from the cache.
+     *
+     * Note that RFC 7231 and RFC 7234 possibly allow for a more permissive implementation,
+     * for example "status codes that are defined as cacheable by default [...]
+     * can be reused by a cache with heuristic expiration unless otherwise indicated"
+     * (https://tools.ietf.org/html/rfc7231#section-6.1)
      *
      * @return bool true if the response is worth caching, false otherwise
      */
diff --git a/src/Symfony/Component/HttpKernel/HttpCache/ResponseCacheStrategy.php b/src/Symfony/Component/HttpKernel/HttpCache/ResponseCacheStrategy.php
@@ -72,7 +72,7 @@ public function update(Response $response)
             $response->setLastModified(null);
         }
 
-        if (!$response->isFresh()) {
+        if (!$response->isFresh() || !$response->isCacheable()) {
             $this->cacheable = false;
         }
 
diff --git a/src/Symfony/Component/HttpKernel/Tests/HttpCache/ResponseCacheStrategyTest.php b/src/Symfony/Component/HttpKernel/Tests/HttpCache/ResponseCacheStrategyTest.php
@@ -175,8 +175,26 @@ public function testEmbeddingPrivateResponseMakesMainResponsePrivate()
         $cacheStrategy->update($masterResponse);
 
         $this->assertTrue($masterResponse->headers->hasCacheControlDirective('private'));
-        // Not sure if we should pass "max-age: 60" in this case, as long as the response is private and
-        // that's the more conservative of both the master and embedded response...?
+        $this->assertFalse($masterResponse->headers->hasCacheControlDirective('public'));
+    }
+
+    public function testEmbeddingPublicResponseDoesNotMakeMainResponsePublic()
+    {
+        $cacheStrategy = new ResponseCacheStrategy();
+
+        $masterResponse = new Response();
+        $masterResponse->setPrivate(); // this is the default, but let's be explicit
+        $masterResponse->setMaxAge(100);
+
+        $embeddedResponse = new Response();
+        $embeddedResponse->setPublic();
+        $embeddedResponse->setSharedMaxAge(100);
+
+        $cacheStrategy->add($embeddedResponse);
+        $cacheStrategy->update($masterResponse);
+
+        $this->assertTrue($masterResponse->headers->hasCacheControlDirective('private'));
+        $this->assertFalse($masterResponse->headers->hasCacheControlDirective('public'));
     }
 
     public function testResponseIsExiprableWhenEmbeddedResponseCombinesExpiryAndValidation()

Original file line number	Diff line number	Diff line change
`@@ -507,13 +507,19 @@ public function getCharset()`
`507`	`507`	`}`
`508`	`508`
`509`	`509`	`/**`
`510`		`- * Returns true if the response is worth caching under any circumstance.`
	`510`	`+ * Returns true if the response may safely be kept in a shared (surrogate) cache.`
`511`	`511`	`*`
`512`	`512`	`* Responses marked "private" with an explicit Cache-Control directive are`
`513`	`513`	`* considered uncacheable.`
`514`	`514`	`*`
`515`	`515`	`* Responses with neither a freshness lifetime (Expires, max-age) nor cache`
`516`		`- * validator (Last-Modified, ETag) are considered uncacheable.`
	`516`	`+ * validator (Last-Modified, ETag) are considered uncacheable because there is`
	`517`	`+ * no way to tell when or how to remove them from the cache.`
	`518`	`+ *`
	`519`	`+ * Note that RFC 7231 and RFC 7234 possibly allow for a more permissive implementation,`
	`520`	`+ * for example "status codes that are defined as cacheable by default [...]`
	`521`	`+ * can be reused by a cache with heuristic expiration unless otherwise indicated"`
	`522`	`+ * (https://tools.ietf.org/html/rfc7231#section-6.1)`
`517`	`523`	`*`
`518`	`524`	`* @return bool true if the response is worth caching, false otherwise`
`519`	`525`	`*/`
Original file line number	Diff line number	Diff line change
`@@ -72,7 +72,7 @@ public function update(Response $response)`
`72`	`72`	`$response->setLastModified(null);`
`73`	`73`	`}`
`74`	`74`
`75`		`- if (!$response->isFresh()) {`
	`75`	`+ if (!$response->isFresh() \|\| !$response->isCacheable()) {`
`76`	`76`	`$this->cacheable = false;`
`77`	`77`	`}`
`78`	`78`