[HttpKernel] ResponseCacheStrategy does not allow client (= browser) cache

[aschempp]

Q	A
Bug report?	yes
Feature request?	no
BC Break report?	no
RFC?	no
Symfony version	3.4

This issue is a follow-up to #25902. @leofeyer, @Toflar and me re-analyzed the problem: The ResponseCacheStrategy sets the response to no-cache, must-revalidate if any of the responses is set to private. This is incorrect, as private means it should be cacheable by the browser, just not by any shared cache.

Because the Response::isCacheable method returns false if a response is private, the ResponseCacheStrategy replaces the private header with a no-cache, must-revalidate.

RFC 7234 says:

The "private" response directive indicates that the response message is intended for a single user and MUST NOT be stored by a shared cache. A private cache MAY store the response and reuse it for later requests, even if the response would normally be non-cacheable.

/cc @nicolas-grekas

[nicolas-grekas]

Your analysis looks good to me. Can you think of any PR you could provide?

[Toflar]

I'm wondering about the best way to implement a bugfix, honestly.
The problem we face is that Response::getMaxAge() as well as Response::isCacheable() returns the correct values from the reverse proxy point of view. Both should behave differently (getMaxAge() should ignore s-max-age and isCacheable() should ignore private) when asking from the client point of view.

However, we cannot just adjust these for BC reasons so I wonder how we should best implement this. Shall we just add comments that the existing methods are from the reverse proxy point of view and add new ones? Or not adding new ones and just access the stuff we need in ResponseCacheStrategy, basically leaving the trap open for other people? Or should we have a parameter $forReverseProxy = true for both of these methods so you can pass false if you want a different result for the client point of view?

WDYT?

[Toflar]

Any feedback on the preferred solution?

[nicolas-grekas]

Please review #26352

[mpdude]

Tried this on Symfony 2.8:

<?php

use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
use Symfony\Component\HttpFoundation\Response;

class TestController
{
    /**
     * @Route("/test/private")
     */
    public function privateAction()
    {
        $response = new Response("foo");
        $response->setPrivate();

        return $response;
    }

    /**
     * @Route("/test/embeds")
     */
    public function embedsAction()
    {
        $response = new Response('<esi:include src="/test/private" />');
        $response->setPrivate();

        return $response;
    }

}

Then, with the HttpCache enabled:

$ curl -I -X GET http://localhost/test/embeds 
HTTP/1.1 200 OK
Date: Wed, 21 Mar 2018 21:55:29 GMT
Cache-Control: must-revalidate, no-cache, private
...
Content-Length: 3
X-Symfony-Cache: GET /test/embeds: miss; GET /test/private: miss
Content-Type: text/html; charset=UTF-8

Am I getting you right that you're looking for the private Cache-Control in the combined/merged response?

[aschempp]

Well, yes. But also there should not be a no-cache, because that renders private obsolete. I'm also looking for a max-age and s-maxage merged from multiple responses (picking the lowest value).

[mpdude]

Regarding (s)max-age, we'd need to make sure that smax-age is for shared caches. private responses probably must look at max-age only?

What do you mean with no-cache rendering private obsolete?

[aschempp]

Sorry, I forgot to mention that #26532 is a replacement for this ticket. I have added extensive information about the RFC to that pull request.

Quoting RFC 2616, Section 14.9.1

private: Indicates that all or part of the response message is intended for a single user and MUST NOT be cached by a shared cache.

no-cache: […] a cache MUST NOT use the response to satisfy a subsequent request without successful revalidation with the origin server. […] A private (non-shared) cache MAY cache the response.

A no-cache directive essentially tells all caches (and that includes the Browser, which is a private cache) to no cache the response. The private directive can never be used.

[mpdude]

I started here because #26532 mentioned it as a fixed ticket, so I thought it might help me understand your requirements there.

A no-cache directive essentially tells all caches (and that includes the Browser, which is a private cache) to no cache the response.

I think you're misreading the RFC you quoted? no-cache means "don't serve this from a cache without revalidation", private means "store in private caches only". no-store is the way of stating "don't keep this in caches".

Does that make sense?

[aschempp]

Are you ok with discussing this in #26532 ?

[mpdude]

Sure

[aschempp]

Closing in favor of #26532