bug #48880 [Response] getMaxAge() returns non-negative integer (pkruithof, fabpot)

fabpot · fabpot · commit ba0cd9adbaae · 2023-02-04T09:34:25.000+01:00
This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [Response] `getMaxAge()` returns non-negative integer | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Refs symfony/symfony#48651 (comment) | License | MIT | Doc PR | The `max-age` directive should be a non-negative integer, see [MDN](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control): > The max-age=N request directive indicates that the client allows a stored response that is generated on the origin server within N seconds — where N may be any non-negative integer (including 0). In case the value is negative, it's encouraged to be treated as 0: > In other words, for any max-age value that isn't an integer or isn't non-negative, the caching behavior that's encouraged is to treat the value as if it were 0. In my case, it lead to a response that was `private,no-cache` but with an `Expires` header set in the future. Not every browser handled this inconsistency the same, which eventually led to authentication issues (see linked comment for a more elaborate explanation). Commits ------- 2639c4353a [Response] `getMaxAge()` returns non-negative integer
diff --git a/Response.php b/Response.php
@@ -774,8 +774,10 @@ public function getMaxAge(): ?int
             return (int) $this->headers->getCacheControlDirective('max-age');
         }
 
-        if (null !== $this->getExpires()) {
-            return (int) $this->getExpires()->format('U') - (int) $this->getDate()->format('U');
+        if (null !== $expires = $this->getExpires()) {
+            $maxAge = (int) $expires->format('U') - (int) $this->getDate()->format('U');
+
+            return max($maxAge, 0);
         }
 
         return null;
@@ -819,7 +821,7 @@ public function setSharedMaxAge(int $value): object
      *
      * It returns null when no freshness information is present in the response.
      *
-     * When the responses TTL is <= 0, the response may not be served from cache without first
+     * When the response's TTL is 0, the response may not be served from cache without first
      * revalidating with the origin.
      *
      * @final
@@ -828,7 +830,7 @@ public function getTtl(): ?int
     {
         $maxAge = $this->getMaxAge();
 
-        return null !== $maxAge ? $maxAge - $this->getAge() : null;
+        return null !== $maxAge ? max($maxAge - $this->getAge(), 0) : null;
     }
 
     /**
diff --git a/Tests/ResponseTest.php b/Tests/ResponseTest.php
@@ -353,9 +353,8 @@ public function testGetMaxAge()
         $this->assertEquals(3600, $response->getMaxAge(), '->getMaxAge() falls back to Expires when no max-age or s-maxage directive present');
 
         $response = new Response();
-        $response->headers->set('Cache-Control', 'must-revalidate');
         $response->headers->set('Expires', -1);
-        $this->assertLessThanOrEqual(time() - 2 * 86400, $response->getExpires()->format('U'));
+        $this->assertSame(0, $response->getMaxAge());
 
         $response = new Response();
         $this->assertNull($response->getMaxAge(), '->getMaxAge() returns null if no freshness information available');
@@ -436,7 +435,7 @@ public function testGetTtl()
 
         $response = new Response();
         $response->headers->set('Expires', $this->createDateTimeOneHourAgo()->format(\DATE_RFC2822));
-        $this->assertLessThan(0, $response->getTtl(), '->getTtl() returns negative values when Expires is in past');
+        $this->assertSame(0, $response->getTtl(), '->getTtl() returns zero when Expires is in past');
 
         $response = new Response();
         $response->headers->set('Expires', $response->getDate()->format(\DATE_RFC2822));

Original file line number	Diff line number	Diff line change
`@@ -774,8 +774,10 @@ public function getMaxAge(): ?int`
`774`	`774`	`return (int) $this->headers->getCacheControlDirective('max-age');`
`775`	`775`	`}`
`776`	`776`
`777`		`- if (null !== $this->getExpires()) {`
`778`		`- return (int) $this->getExpires()->format('U') - (int) $this->getDate()->format('U');`
	`777`	`+ if (null !== $expires = $this->getExpires()) {`
	`778`	`+ $maxAge = (int) $expires->format('U') - (int) $this->getDate()->format('U');`
	`779`	`+`
	`780`	`+ return max($maxAge, 0);`
`779`	`781`	`}`
`780`	`782`
`781`	`783`	`return null;`
`@@ -819,7 +821,7 @@ public function setSharedMaxAge(int $value): object`
`819`	`821`	`*`
`820`	`822`	`* It returns null when no freshness information is present in the response.`
`821`	`823`	`*`
`822`		`- * When the responses TTL is <= 0, the response may not be served from cache without first`
	`824`	`+ * When the response's TTL is 0, the response may not be served from cache without first`
`823`	`825`	`* revalidating with the origin.`
`824`	`826`	`*`
`825`	`827`	`* @final`
`@@ -828,7 +830,7 @@ public function getTtl(): ?int`
`828`	`830`	`{`
`829`	`831`	`$maxAge = $this->getMaxAge();`
`830`	`832`
`831`		`- return null !== $maxAge ? $maxAge - $this->getAge() : null;`
	`833`	`+ return null !== $maxAge ? max($maxAge - $this->getAge(), 0) : null;`
`832`	`834`	`}`
`833`	`835`
`834`	`836`	`/**`