8000 Auto add the `cache-control` headers? · Issue #25736 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Auto add the cache-control headers? #25736

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
devgogo opened this issue Jan 10, 2018 · 32 comments
Closed

Auto add the cache-control headers? #25736

devgogo opened this issue Jan 10, 2018 · 32 comments
Labels
HttpFoundation

Comments

@devgogo
Copy link
devgogo commented Jan 10, 2018 •
edited by javiereguiluz
Loading
Q A
Bug report? Maybe
Feature request? no
BC Break report? no
RFC? no
Symfony version 4.0.3

hello everybody,

I need my CDN service cache my website, so I set

    /**
     * @Route("/", defaults={"page": 1}, name="homepage")
     * @Method("GET")
     * @Cache(smaxage="3600")
     */
    public function indexAction(ArticleRepository $articles, int $page): Response
    {
        $articles = $articles->findLatest($page);

        return $this->render('article/index.html.twig', [
            'articles' => $articles
        ]);
    }

then the cache-control header is cache-control:max-age=0, must-revalidate, private, s-maxage=3600

It contains the max-age=0, must-revalidate, private is not what I want.

What should I do?

Because the response header, so my CDN cannot cache my pages.

thx.
@nicolas-grekas
Copy link
Member

That's because your page is using the session, so that it's different per-user, and thus cannot be shared.

@devgogo
Copy link
Author
devgogo commented Jan 10, 2018

@nicolas-grekas, Thanks.

But I not login my account, I want to shared, Because I need CDN cache my pages, can't be implemented?

@nicolas-grekas
Copy link
Member

I guess you need to understand where the session is used, and then figure out a way to not call that code.

@emodric
Copy link
Contributor
emodric commented Jan 10, 2018 •
edited
Loading

your page is using the session, so that it's different per-user, and thus cannot be shared.

@nicolas-grekas I still don't understand the rasoning behind this. Why force those headers in the first place? In eZ Platform, it's possible (and on Symfony 2.8 eZ Platform behaves that way) to have a single HTTP cache for two different users, since eZ Platform varies the cache not by cookie, but by user policies, by using X-User-Hash header from FOS HTTP Cache bundle and varying on it instead on cookie. So if two different users have a same set of policies, they can have the same http cache which is then cached by Varnish. With eZ Platform on Symfony 3.4, this is broken.

@emodus
Copy link
emodus commented Jan 12, 2018

Yes, on SF 3.4.3 varnish skipping cache.

@nicolas-grekas
Copy link
Member

@emodric that's very likely, yet Symfony doesn't know about this logic in eZ. Not sending this header was a bug, on which eZ relied apparently. This should be fixed on eZ side.

@emodric
Copy link
Contributor
emodric commented Jan 12, 2018

Agreed, but nevertheless, it breaks the way eZ worked for years.

Forcing these headers should at least be configurable.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jan 12, 2018 •
edited
Loading

Symfony should respect HTTP semantics by default, this is not an option IMHO.

@emodric
Copy link
Contributor
emodric commented Jan 12, 2018

I'd rather say Symfony should first respect what user/developer intended to do with the Response object in his controller. Symfony overriding something user might've explicitly set in his controller is not a good t 8000 hing IMO, without being able to disable it.

Is there a standard or RFC specifying that these headers must be set if session cookie is present?

@nicolas-grekas
Copy link
Member

Is there a standard or RFC specifying that these headers must be set if session cookie is present?

It's "if session cookie is present", it's "if the session is read". See HTTP RFC.

@emodric you should open an issue on eZ really. The behavior here is correct.

@andrerom
Copy link
Contributor
andrerom commented Jan 12, 2018 •
edited
Loading

@nicolas-grekas Not looked in depth on this, but when was it introduced?
From the sound of it be aware this breaks FOSHttpCache (which is what we are using/extending).

So no matter how you put it, introducing it in patch release sounds like a break.

@andrerom
Copy link
Contributor

That said, I agree on the RFC, however to really follow it we all need to look at this from a system perspective.

Where we want this behaviour is from the reverse proxy and out.
Not from app breaking reverse proxy behaviour on current applications and installs out there. In today's world we (frankly anyone using Varnish and touching it's VCL really) have custom logic in between app and reverse proxy, so reverse proxy is effectively part of the app here.

/cc @dbu

@dbu
Copy link
Contributor
dbu commented Jan 12, 2018

with FOSHttpCache, we explicitly set cache-control headers. unless you set a response to be cacheable, it will still be no-cache. is the current implementation in the response object preventing the application from explicitly setting cache-control headers or does it merely change defaults when there is a cookie?

@nicolas-grekas
Copy link
Member

Here is the most recent fix on this, it should give you the answers: #25699

@emodric
Copy link
Contributor
emodric commented Jan 12, 2018

@dbu As far as I understand, this prevents and overrides headers set by the app in controllers, no questions asked (presuming session is present and started)

@nicolas-grekas
Copy link
Member

That's correct, you need a listener registered after the SessionListener to override this behavior.
There is no way around internally, so you should do with.

@dbu
Copy link
Contributor
dbu commented Jan 12, 2018

ugh. well i think it makes sense in all cases except when we do special things where it does make sense to maybe cache a response. its not "hardcoded" in the Response itself, we can still overwrite it. we could put the FOSHttpCacheBundle listener to -1001 - but then if you just configure cache control rules that match on paths, but not use the user context system, we could re-introduce the cache mixup bugs that have been fixed in #25699 .

in FOSHttpCacheBundle we provide the reverse-proxy-ttl custom header and configuration for varnish / a listener for symfony cache kernel to use that over the cache-control header. that is safer, because all systems after the reverse proxy indeed must not cache this. we should encourage people to only use that for user context caching. we have to check if that header is still used when a response is private.

its a tricky thing either way - we do not want unsafe defaults, and want to make it at least difficult to shoot yourself into the foot.

i am quite busy atm, if somebody can have a deeper look into it and propose a pull request, please do.

@pixeltreiber
Copy link

This could be related #25826 (comment)

@nicolas-grekas
Copy link
Member

Closing as discussed.

@nicolas-grekas nicolas-grekas mentioned this issue Feb 19, 2018
Closed
@dbu dbu mentioned this issue Mar 8, 2018
Merged
6 tasks
@dbu dbu mentioned this issue Mar 20, 2018
Closed
@teohhanhui
Copy link
Contributor

@nicolas-grekas

It's "if session cookie is present", it's "if the session is read". See HTTP RFC.

Could you kindly point us to the relevant RFC sections on this? I cannot find any mention of it in RFC 7234: https://tools.ietf.org/html/rfc7234

@nicolas-grekas
Copy link
Member

@teohhanhui all the RFC. It's about what can be cached or not. As soon as a page contains a check based on the session, we cannot assume without external knowledge that it doesn't contain personal data/state. As such the default must be "private".

@teohhanhui
Copy link
Contributor
8000

I definitely agree about the default. No arguments there. But overriding what the user has explicitly set in their application is going much further than having a good default.

@apfelbox
Copy link
Contributor

we cannot assume without external knowledge

The default is fine. But this "external knowledge" is exactly the app developer, that says "yes, I know all of that, but the response is still cacheable". Why prevent it then?

Default is fine. Forcing it not and might be considered a bug imo.

@teohhanhui
Copy link
Contributor

If we set the header only if no cache-related headers have been set, it'll greatly reduce the pulling-out of hair for many people. 😄

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jun 26, 2018 •
edited
Loading

Using the session without noticing is super easy: just dump flash messages in your layout and done. I would not consider that the explicitly set "public" C-C is enough to consider it not a mistake when the session has also been used. The current logic considers this as a conflict and defaults to the safest. If you really want to push session-related state into shared caches, you are required to do extra work yes. That's expected to me.

@apfelbox
Copy link
Contributor
apfelbox commented Jun 26, 2018 •
edited
Loading

I don't argue against the intention, as in a general case it is very likely that caching a response that is using the session will be a bug.

I guess as workaround a custom response listener and passing the cache config in Request::$attributes and reapplying them in the listener might work.

However – do we log anything in case we are overriding explicitly set cache headers? Might reduce the amount of wtf in this case.

@teohhanhui
Copy link
Contributor

The correct thing to do is to let the user opt-in to this stricter check, and we could deprecate not using the strict mode, to be removed in the next major version.

@nicolas-grekas
Copy link
Member

@apfelbox I feel like you may have missed #26681

@apfelbox
Copy link
Contributor
apfelbox commented Jun 26, 2018 •
edited
Loading

@nicolas-grekas I did miss it, indeed. That looks perfect, so 👍 all around.

Sorry for the noise.

@Spomky Spomky mentioned this issue Jul 23, 2018
Closed
@deniz777
Copy link
deniz777 commented Sep 15, 2018 •
edited
Loading

I am using just "http foundation 4.1" and "twig 2.5" packages. They are creating duplicated response headers like that:
Cache-Control: max-age=0, private, must-revalidate
Cache-Control: no-cache, private
I know sending duplicated responses are acceptable but sending same values more than once is valid? I mean "private" value in this example. Can it cause any problem? Can somebody answer this?

@nicolas-grekas nicolas-grekas mentioned this issue Nov 9, 2018
Closed
@eugenmihailescu
Copy link

@apfelbox I feel like you may have missed #26681

Yes, especially the 3.4 hack as described by @yellow1912

@albingi
Copy link
albingi commented Feb 28, 2019

You can add this header'Symfony-Session-NoAutoCacheControl' in the response to force to use your Cache-Control :
https://github.com/symfony/http-kernel/blob/master/EventListener/AbstractSessionListener.php#L83-L90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
HttpFoundation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0