[HttpKernel] AbstractSessionListener should not override the cache lifetime for private responses #48651

rodmen · 2022-12-14T16:39:30Z

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #47660 AbstractSessionListener should not override the cache lifetime
License	MIT

#47660 is opened as a bug
This PR fix that AbstractSessionListener override the max-age and the expires cache headers if cache control is private and these values are explicit defined

carsonbot · 2022-12-14T16:39:38Z

It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you.

Cheers!

Carsonbot

nicolas-grekas · 2022-12-15T11:04:53Z

src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php

@@ -200,10 +200,14 @@ public function onKernelResponse(ResponseEvent $event)
        }

        if ($autoCacheControl) {
+            $maxAge = 0;
+            if (!$response->headers->hasCacheControlDirective('public') && (bool) $response->getMaxAge()) {


I'd suggest this CS instead:

if ($autoCacheControl) { + $maxAge = $response->headers->hasCacheControlDirective('public') ? 0 : (int) $response->getMaxAge(); $response - ->setExpires(new \DateTime()) + ->setExpires(new \DateTimeImmutable('+'.$maxAge.' seconds'))

But LGTM otherwise thanks!

(please allow edits from maintainers, that makes it easier to deal with contributions on our side)

Hi @nicolas-grekas ,
I have applied the suggested CS, looks better now.
I can't allow edits for maintainers... I think it's because I opened the PR from an organization repository and that option is not available in that case (please correct me if I'm wrong).

…fetime for private responses

nicolas-grekas · 2022-12-15T13:04:33Z

Thank you @rodmen.

pkruithof · 2023-01-04T15:32:58Z

I'm not sure whether this is the appropriate place, but I think I found a bug regarding this change (I'll open an actual issue if I'm correct about this).

In our case, we have the following situation:

a secured url is being requested, at which point an AuthenticationEntryPoint returns a redirect response to an external auth provider
this response is marked with cache-control: private, no-cache
following this, the response sets the expires to -1 (see https://github.com/symfony/http-foundation/blob/9d081ead9d3432e2e8002178d14c4c9dd4b8ffbf/Response.php#L312)
the listener in this PR now sets the max-age to the value of $response->getMaxAge(), basically overwriting it with the same value.

At least, this is the intention (I think). What actually happens now is this:

getMaxAge() calculates the value when not explicitly set by subtracting the response date from the expires date
getExpires() can't form a DateTimeImmutable from its -1 value and falls back on a default date in the past
this way getMaxAge() eventually does {date in the past} - {current date}, which in my particular case (at this moment) yields -172800

new \DateTimeImmutable('+'.$maxAge.' seconds') is now tomorrow:

php > var_dump((new DateTimeImmutable('+-172800 seconds'))->format('c'));
string(25) "2023-01-06T15:28:04+00:00"
# (it's currently Jan. 5th)

So now we have a 302 response that has private, no-cache and max-age=0, but with an Expires header in the future. And apparently Firefox treats this as cacheable, so even after logging in, when visiting the original url I still get redirected to the auth provider because it serves the original 302 response from cache.

Sorry for the long description, but do you agree this is a bug? If so I'll file an issue or just a PR right away. I think the intended behaviour should be to only overwrite max-age=0 when the response is public.

stof · 2023-01-04T15:40:35Z

this indeed looks like a bug

…uithof, fabpot) 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 #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 ------- 2639c43 [Response] `getMaxAge()` returns non-negative integer

…uithof, fabpot) 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

carsonbot added Status: Needs Review Bug HttpKernel labels Dec 14, 2022

carsonbot added this to the 5.4 milestone Dec 14, 2022

nicolas-grekas reviewed Dec 15, 2022

View reviewed changes

[HttpKernel] AbstractSessionListener should not override the cache li…

36ad0be

…fetime for private responses

nicolas-grekas merged commit 7e2a88a into symfony:5.4 Dec 15, 2022

fabpot mentioned this pull request Dec 16, 2022

Release v6.2.2 #48687

Merged

This was referenced Dec 28, 2022

Release v5.4.17 #48806

Merged

Release v6.0.17 #48807

Merged

Release v6.1.9 #48808

Merged

pkruithof mentioned this pull request Jan 5, 2023

[Response] getMaxAge() returns non-negative integer #48880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpKernel] AbstractSessionListener should not override the cache lifetime for private responses #48651

[HttpKernel] AbstractSessionListener should not override the cache lifetime for private responses #48651

[HttpKernel] AbstractSessionListener should not override the cache lifetime for private responses #48651

[HttpKernel] AbstractSessionListener should not override the cache lifetime for private responses #48651

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment