PageCache caching uncacheable responses (violating HTTP/1.0 spec) + D8 intentionally disabling HTTP/1.0 proxies = WTF

This seems like quite a major issue - but no activity on it in the past two years ...?

Is it still a current issue?

I'm testing the attached patch in combination with the cache control override module and a couple of patches to it (https://www.drupal.org/files/issues/2019-09-26/cache_control_override_ex... and https://www.drupal.org/files/issues/2020-01-09/cache_control_override-31...), to good effect.

Here's a patch that turns off caching in the page cache if it's set to not cacheable in FinishResponseSubscriber::onRespond().

It's similar to patch #9, but is more explicit.

+      if ($expires == \DateTime::createFromFormat('j-M-Y H:i:s T', '19-Nov-1978 05:00:00 UTC')) {
+        // If explicitly set to this date, do not cache.
+        return FALSE;
+      }

The only place that date is used in Drupal core is FinishResponseSubscriber.

UPDATE: the next patch I added adds a more clear note about this.

This will make it more obvious to developers that their caching isn't working properly, for example if they've disabled it!

I tried Wim's suggestion to check the max age, but it was set to a negative number in this case. I like the idea of checking the same date to which the FinishResponseSubscriber sets the expires.

I ran into this with the Sitewide Alert module. https://www.drupal.org/project/sitewide_alert/issues/3189144

If you set expires header on a CacheableJsonResponse returned in a controller, like this:

$response->setExpires($expireDate->getPhpDateTime());

but you have set cache.page.max_age to 0 (<no caching>) at /admin/config/development/performance, the CacheableJsonResponse gets mangled in \Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond() and set to not cacheable, but Page Cache caches it anyway with 'expire' set to -1. This breaks the scheduling feature. The solution in my case easy, to set the cache.page.max_age to something other than 0.

I was also able to fix it with the kill switch. But I'm glad I figured out that had forgot to set my max-page in the performance settings.

Just adding a little more info in the comment.

Wim, can you add a little more info on this suggestion:

Make \Drupal\page_cache\StackMiddleware\PageCache a HTTP/1.1 proxy: make it inspect Cache-Control rather than Expires

Symfony has a \Symfony\Component\HttpFoundation\Response::isCacheable() method that looks to me to be perfect here. I don't see it used in Drupal core at all.

Perhaps instead of checking the expires we could check if it's cacheable using that method.

Wim, I don't think the second part of the description is still true:

even when you configure a non-zero "max age" at /admin/config/development/performance, \Drupal\Core\EventSubscriber\FinishResponseSubscriber::setResponseCacheable() will still cause that same Expires header to be set

When I was testing tonight, with a CacheableJsonResponse from a controller, this wasn't the case. I think as long as it the response is an instance of CacheableResponseInterface, and cache.page.max_age is greater than zero, the expires is not altered.

Hello everybody. Here is the module that solves this problem and has settings - https://www.drupal.org/project/custom_cache

#13 had many failures and was not ready for review.
Which show this will need it's own test case and work for BC.

This was a tough one, but I believe I've managed to sort things out.

Some unexpected behaviors were occurring, such as the FinishResponseSubscriber setting an Expires header, which made the response non-cacheable—inside the setResponseCacheable() method, which should theoretically do the opposite.

Additionally, some functional tests in PageCacheTest were incorrectly expecting the response to be non-cacheable when it actually should have been. As a result, they were effectively masking the issue with cacheability headers.

A more in-deep review is necessary, as this is a complex scenario with multiple edge cases involved, and a thorough understanding of the HTTP cacheability headers specification is required to do so.

Also, I haven't been able to mark the MR as "Ready" I'm not sure if someone with higher privileges could do it, so I don't have to create a new one.

Tagging with Performance. I think @catch have the habit of checking that tag, might have some valuable feedback on the MR. I have none.

I tested the patch, hoping it would solve my "Anonymous never get fresh pages" problem for a contrib field module.
Reading the summary closely, it will not, since my use case does not set the Expire, only max-age (there is no guidance how to apply setExpire() by contrib either.)
Anyway, the Summary lacks a test script. I added one. Please correct it if necessary.

After digging into the code, I realized I made a mistake. To prevent multiple tests from failing, I mistakenly kept the very piece of code that was wrong in the first place:

  $expire = ($date > $request_time) ? $date : Cache::PERMANENT;

I also confirmed that this issue is only related to the Expires header and not to PageCache’s behavior of ignoring max-age. For that second issue, please refer to #2352009: Bubbling of elements' max-age to the page's headers and the page cache @johnv.

I've finally fixed the caching behavior and all tests.

Things I'm concerned about:

• I had to remove the weak (W/) prefix from the ETag header, as it was sometimes being added, causing multiple EntityResourceTestBase tests to fail. I was able to reproduce the issue, but I'm not entirely sure if it's due to an Nginx configuration or something else.
• Now that system.performance.cache.page.max_age is being properly honored, I had to set a max_age value in several tests expecting responses to be cacheable. I'm not sure if this is the correct approach or if we should configure it globally in BrowserTestBase.
• I've assumed that having two Vary headers in the response is correct, but I haven't determined why they are being set separately. An additional Accept-Encoding value is added in a second Vary header instead of being merged into the existing Vary header containing Cookie and Origin values. To account for this, I modified CorsIntegrationTest to check for "Origin" across all possible Vary headers.

By the way, now that this fix ensures the system.performance.cache.page.max_age config value is properly honored by PageCache, I'm not sure how it will overlap with #2352009: Bubbling of elements' max-age to the page's headers and the page cache, but it's certainly related.

Sorry for the late answer, commenting on the issue summary and the two points raised by @Wim Leers:

1. Drupal 8 intentionally disables HTTP/1.0 proxies

This has been the case since Drupal 7 (and also Pressflow - Drupal 6 optimized for high performance content oriented websites). It has been necessary for any web application which uses cookies to disable HTTP/1.0 caches. There is no way in HTTP/1.0 to maintain different page variations under the same URL. Everything which influences the result (like desired language, session and login state) has to be encoded in the URL. Yes, session identifiers were passed via request parameters back then.

When HTTP/1.1 introduced cookies and cache control, it was specified that the Cache-Control: max-age directive takes precedence over the HTTP/1.0 Expires header.

"Modern" web applications which serve multiple variants of a page under the same URL (depending on the application state maintained in the cookie header) can be made compatible with both HTTP protocol versions by serving a suitable Cache-Control and Expires header. The way to do this is to serve an appropriate Cache-Control header to permit storage in HTTP/1.1 caches (if appropriate) and serve an Expires header which is set into the past in order to disable caching in HTTP/1.0 caches. Remember, HTTP/1.0 caches assume that the only thing which can influence the response is the URL.

This pattern is not specific to Drupal. Everybody does this.

Drupal 8 ships with a HTTP/1.0 reverse proxy that is breaking the spec: Page Cache

Responses that have Expires: Sun, 19 Nov 1978 05:00:00 GMT are meant to not be cached, because it's an expiration date in the past. Yet PageCache caches them permanently:

The page cache module is not a HTTP/1.0 reverse proxy. It is neither a HTTP/1.1 reverse proxy. The caching logic has its roots in Pressflow and is tailored to content oriented websites.

In unmodified HTTP/1.1+ reverse proxies and browser caches, the only way to get rid of cached pages is to limit their validity to a certain time frame (i.e., max-age or s-max-age). The system.performance.cache.page.max_age config value is the way to specify that value for those downstream caches which are otherwise out-of-reach for Drupal.

With the built-in cache the situation is different. It is possible to remove cached pages whenever a change is made on the website. Stock Drupal 7 did nuke the whole page cache whenever any content was changed. Some contrib modules rectified that heavy handed approach and made it possible to define rules which pages were purged depending on which content was changed.

D8+ introduced cache tags and that removed all the guess work. No site builder has to think about which pages to purge from the cache nowadays.

For this reason the page cache module keeps rendered pages in the cache for an unspecified amount of time. This has been the way it worked since Pressflow.

The following remark in the issue summary is indeed pointing to a problem point in the page caching module:

PageCache determines which responses to cache by inspecting the Expires header. (This should be updated to use the Cache-Control header instead, but that's out of scope here.)

The page cache module really shouldn't inspect the Expires header. That logic needs to be deprecated indeed.

Updating issue status to Postponed (maintainer needs more info). I will wait a couple of weeks and then set it to Closed (works as designed) unless there is significant new (and valid) information added to the issue summary.

Thanks @znerol for a detailed response. It makes me feel better after have worked in vain, due to misunderstanding the Page Cache role in the whole picture.

I think that the only change this issue fixes, then, is setting an Expires header which honors system_performance.cache.page.max_age, which would be possibly beneficial for HTTP 1.0 proxies down the line of a response.

Before:

 protected function setResponseCacheable(Response $response, Request $request) {
    // HTTP/1.0 proxies do not support the Vary header, so prevent any caching
    // by sending an Expires date in the past. HTTP/1.1 clients ignore the
    // Expires header if a Cache-Control: max-age directive is specified (see
    // RFC 2616, section 14.9.3).
    if (!$response->headers->has('Expires')) {
      $this->setExpiresNoCache($response);
    }

    $max_age = $this->config->get('cache.page.max_age');
    $response->headers->set('Cache-Control', 'public, max-age=' . $max_age);

After:

 protected function setResponseCacheable(Response $response, Request $request) {
    $max_age = $this->config->get('cache.page.max_age');
    $response->headers->set('Cache-Control', 'public, max-age=' . $max_age);

    // If the response does not already have an Expires header, set one.
    if (is_int($max_age) && !$response->headers->has('Expires')) {
      $response->setExpires((new \DateTime())->setTimestamp($this->time->getRequestTime() + $max_age));
    }

Which I believe is not in the scope of the original issue and could fit better as an additional task to #2352009: Bubbling of elements' max-age to the page's headers and the page cache.

But, as you said, a problem remains unsolved:

PageCache determines which responses to cache by inspecting the Expires header. (This should be updated to use the Cache-Control header instead, but that's out of scope here.)

So, what's next? I'm not sure at all...

1. Bring the discussion of max_age config parameter honoring to #2352009: Bubbling of elements' max-age to the page's headers and the page cache
2. Close this issue as "Works as designed" or leave only the fix that fixes the Expires header and go on to Fixed? I'm not sure if it's beneficial for HTTP 1.0 proxies or if they really exist yet nowadays.
3. Create another issue to fix the aforementioned remaining problem with PageCache inspecting Expires header instead of Cache-Control, so this one don't get "noisy"?

Am I right? :)

I think that the only change this issue fixes, then, is setting an Expires header which honors system_performance.cache.page.max_age, which would be possibly beneficial for HTTP 1.0 proxies down the line of a response.

No, that wouldn't be desirable. If Drupal did this, then that would lead to potential information leak via HTTP/1.0 caches. This is because HTTP/1.0 caches would store/serve every page, regardless of whether there were cookies on the request or not. HTTP/1.0 caches are unable to differentiate between authenticated requests and anonymous ones simply because they do not know anything about cookies.

Bring the discussion of max_age config parameter honoring to #2352009: Bubbling of elements' max-age to the page's headers and the page cache

Depends. If you mean that no page should be cached in the page cache longer than system_performance.cache.page.max_age, then no. If you mean that page cache should honor the max-age directive collected from render metadata, then yes, that is the correct issue.

Create another issue to fix the mentioned remaining problem with PageCache inspectiong Expires header instead of Cache-Control, so this one don't get "noisy"?

No need for that, this is covered by #2352009: Bubbling of elements' max-age to the page's headers and the page cache.

Oh, my bad—I mixed things up again...

Now it’s clear to me: HTTP 1.0 proxies must always be skipped using a past Expires header. So, there’s nothing to fix. Thanks for rephrasing the explanation—it helped me finally grasp the full picture.

Since the third point is covered by #2352009: Bubbling of elements' max-age to the page's headers and the page cache, there's nothing else to address. I agree on closing this as "Works as designed" and leaving it as is. 🙂

Thanks all!