Public responses without lifetime should not remove lifetime for the …

…resulting response
symfony · fabpot · Jun 24, 2021 · Jun 11, 2021 · Jun 24, 2021 · ad1f057ce208b507c18fbcd28907a0cf70ae1c6a
commit ad1f057ce208b507c18fbcd28907a0cf70ae1c6a
diff --git a/src/Symfony/Component/HttpKernel/HttpCache/ResponseCacheStrategy.php b/src/Symfony/Component/HttpKernel/HttpCache/ResponseCacheStrategy.php
@@ -81,14 +81,15 @@ public function add(Response $response)
            return;
        }

+        $isHeuristicallyCacheable = $response->headers->hasCacheControlDirective('public');
        $maxAge = $response->headers->hasCacheControlDirective('max-age') ? (int) $response->headers->getCacheControlDirective('max-age') : null;
-        $this->storeRelativeAgeDirective('max-age', $maxAge, $age);
+        $this->storeRelativeAgeDirective('max-age', $maxAge, $age, $isHeuristicallyCacheable);
        $sharedMaxAge = $response->headers->hasCacheControlDirective('s-maxage') ? (int) $response->headers->getCacheControlDirective('s-maxage') : $maxAge;
-        $this->storeRelativeAgeDirective('s-maxage', $sharedMaxAge, $age);
+        $this->storeRelativeAgeDirective('s-maxage', $sharedMaxAge, $age, $isHeuristicallyCacheable);

        $expires = $response->getExpires();
        $expires = null !== $expires ? (int) $expires->format('U') - (int) $response->getDate()->format('U') : null;
-        $this->storeRelativeAgeDirective('expires', $expires >= 0 ? $expires : null, 0);
+        $this->storeRelativeAgeDirective('expires', $expires >= 0 ? $expires : null, 0, $isHeuristicallyCacheable);
    }

    /**
@@ -199,11 +200,29 @@ private function willMakeFinalResponseUncacheable(Response $response): bool
     * we have to subtract the age so that the value is normalized for an age of 0.
     *
     * If the value is lower than the currently stored value, we update the value, to keep a rolling
-     * minimal value of each instruction. If the value is NULL, the directive will not be set on the final response.
+     * minimal value of each instruction.
+     *
+     * If the value is NULL and the isHeuristicallyCacheable parameter is false, the directive will
+     * not be set on the final response. In this case, not all responses had the directive set and no
+     * value can be found that satisfies the requirements of all responses. The directive will be dropped
+     * from the final response.
+     *
+     * If the isHeuristicallyCacheable parameter is true, however, the current response has been marked
+     * as cacheable in a public (shared) cache, but did not provide an explicit lifetime that would serve
+     * as an upper bound. In this case, we can proceed and possibly keep the directive on the final response.
     */
-    private function storeRelativeAgeDirective(string $directive, ?int $value, int $age)
+    private function storeRelativeAgeDirective(string $directive, ?int $value, int $age, bool $isHeuristicallyCacheable)
    {
        if (null === $value) {
+            if ($isHeuristicallyCacheable) {
+                /*
+                 * See https://datatracker.ietf.org/doc/html/rfc7234#section-4.2.2
+                 * This particular response does not require maximum lifetime; heuristics might be applied.
+                 * Other responses, however, might have more stringent requirements on maximum lifetime.
+                 * So, return early here so that the final response can have the more limiting value set.
+                 */
+                return;
+            }
            $this->ageDirectives[$directive] = false;
        }


diff --git a/src/Symfony/Component/HttpKernel/Tests/HttpCache/ResponseCacheStrategyTest.php b/src/Symfony/Component/HttpKernel/Tests/HttpCache/ResponseCacheStrategyTest.php
@@ -370,7 +370,7 @@ public function cacheControlMergingProvider()
        ];

        yield 'merge max-age and s-maxage' => [
-            ['public' => true, 's-maxage' => '60', 'max-age' => null],
+            ['public' => true, 'max-age' => '60'],
            ['public' => true, 's-maxage' => 3600],
            [
                ['public' => true, 'max-age' => 60],
@@ -393,6 +393,30 @@ public function cacheControlMergingProvider()
            ],
        ];

+        yield 'public subresponse without lifetime does not remove lifetime for main response' => [
+            ['public' => true, 's-maxage' => '30', 'max-age' => null],
+            ['public' => true, 's-maxage' => '30'],
+            [
+                ['public' => true],
+            ],
+        ];
+
+        yield 'lifetime for subresponse is kept when main response has no lifetime' => [
+            ['public' => true, 'max-age' => '30'],
+            ['public' => true],
+            [
+                ['public' => true, 'max-age' => '30'],
+            ],
+        ];
+
+        yield 's-maxage on the subresponse implies public, so the result is public as well' => [
+            ['public' => true, 'max-age' => '10', 's-maxage' => null],
+            ['public' => true, 'max-age' => '10'],
+            [
+                ['max-age' => '30', 's-maxage' => '20'],
+            ],
+        ];
+
        yield 'result is private when combining private responses' => [
            ['no-cache' => false, 'must-revalidate' => false, 'private' => true],
            ['s-maxage' => 60, 'private' => true],