Skip to content
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.

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

[HttpKernel] Fix Apache mod_expires Session Cache-Control issue #33487

Merged
merged 1 commit into from Sep 8, 2019

Conversation

@pbowyer
Copy link
Contributor

pbowyer commented Sep 6, 2019

Q A
Branch? 3.4 for bug fixes
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

Apaches's mod_expires is a widely used module to set HTTP caching headers. It allows you to set a default cache lifetime as well as lifetimes by mime_type.

When an application server has set a Cache-Control header, mod_expires ignores this and sets its own, resulting in duplicate Cache-Control headers and conflicting information. It does this unless the application server sets an Expires header, in which case mod_expires does nothing. This is documented on the link above:

When the Expires header is already part of the response generated by the server, for example when generated by a CGI script or proxied from an origin server, this module does not change or add an Expires or Cache-Control header.

Symfony automatically sets a Cache-Control header if a session exists. This patch adds an Expires header to ensure it's respected by mod_expires.

Example 1

With the following Apache config:

<IfModule mod_expires.c>
    ExpiresActive on
    ExpiresDefault                                      "access plus 1 month"
</IfModule>

The HTTP response headers are:

Without the patch

HTTP/1.1 200 OK
Date: Fri, 06 Sep 2019 08:02:02 GMT
Server: Apache/2.4.37 (Ubuntu)
Cache-Control: max-age=0, must-revalidate, private
Cache-Control: max-age=2592000
Expires: Sun, 06 Oct 2019 08:02:00 GMT
Vary: Accept-Encoding
Content-Encoding: gzip
Content-Length: 13099
Connection: close
Content-Type: text/html; charset=UTF-8

With the patch

HTTP/1.1 200 OK
Date: Fri, 06 Sep 2019 08:21:34 GMT
Server: Apache/2.4.37 (Ubuntu)
Cache-Control: max-age=0, must-revalidate, private
Expires: Fri, 06 Sep 2019 08:21:34 GMT
Vary: Accept-Encoding
Content-Encoding: gzip
Content-Length: 13098
Connection: close
Content-Type: text/html; charset=UTF-8

Example 2

With the following Apache config:

<IfModule mod_expires.c>
    ExpiresActive on
    ExpiresDefault                                      "access plus 1 month"
    ExpiresByType text/html                             "access plus 0 seconds"
</IfModule>

Without the patch

HTTP/1.1 200 OK
Date: Fri, 06 Sep 2019 08:18:40 GMT
Server: Apache/2.4.37 (Ubuntu)
Cache-Control: max-age=0, must-revalidate, private
Cache-Control: max-age=0
Expires: Fri, 06 Sep 2019 08:18:39 GMT
Vary: Accept-Encoding
Content-Encoding: gzip
Content-Length: 13099
Connection: close
Content-Type: text/html; charset=UTF-8

With the patch

HTTP/1.1 200 OK
Date: Fri, 06 Sep 2019 08:20:40 GMT
Server: Apache/2.4.37 (Ubuntu)
Cache-Control: max-age=0, must-revalidate, private
Expires: Fri, 06 Sep 2019 08:20:40 GMT
Vary: Accept-Encoding
Content-Encoding: gzip
Content-Length: 13100
Connection: close
Content-Type: text/html; charset=UTF-8
@pbowyer pbowyer changed the title Fix Apache mod_expires Cache-Control issue Fix Apache mod_expires Session Cache-Control issue Sep 6, 2019
@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Sep 6, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Sep 7, 2019

That's for 3.4, right? Could you please add a test case also?

@nicolas-grekas nicolas-grekas changed the title Fix Apache mod_expires Session Cache-Control issue [HttpKernel] Fix Apache mod_expires Session Cache-Control issue Sep 7, 2019
@pbowyer

This comment has been minimized.

Copy link
Contributor Author

pbowyer commented Sep 7, 2019

Yes, will redo against 3.4. The code didn't apply to both versions, so it needs a separate patch for each.

@pbowyer pbowyer force-pushed the pbowyer:patch-1 branch from 2cb4a65 to 8c6f860 Sep 7, 2019
@pbowyer pbowyer changed the base branch from 4.3 to 3.4 Sep 7, 2019
@pbowyer pbowyer force-pushed the pbowyer:patch-1 branch from 8c6f860 to 665aa14 Sep 7, 2019
@pbowyer

This comment has been minimized.

Copy link
Contributor Author

pbowyer commented Sep 7, 2019

@nicolas-grekas ✔️ Now against 3.4 ✔️ Test coverage added

Had to force-push a second time to get Appveyor to rerun, as I force-pushed the first time before changing the base branch for the PR.

@fabpot
fabpot approved these changes Sep 8, 2019
@fabpot fabpot force-pushed the pbowyer:patch-1 branch from 665aa14 to 9e94276 Sep 8, 2019
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Sep 8, 2019

Thank you @pbowyer.

fabpot added a commit that referenced this pull request Sep 8, 2019
…issue (pbowyer)

This PR was squashed before being merged into the 3.4 branch (closes #33487).

Discussion
----------

[HttpKernel] Fix Apache mod_expires Session Cache-Control issue

| Q             | A
| ------------- | ---
| Branch?       | 3.4 for bug fixes <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| License       | MIT

Apaches's [mod_expires](https://httpd.apache.org/docs/current/mod/mod_expires.html) is a widely used module to set HTTP caching headers. It allows you to set a default cache lifetime as well as lifetimes by mime_type.

When an application server has set a `Cache-Control` header, mod_expires ignores this and sets its own, resulting in duplicate `Cache-Control` headers and conflicting information. It does this _unless_ the application server sets an `Expires` header, in which case mod_expires does nothing. This is documented on the link above:

> When the `Expires` header is already part of the response generated by the server, for example when generated by a CGI script or proxied from an origin server, this module does not change or add an `Expires` or `Cache-Control` header.

Symfony automatically sets a `Cache-Control` header if a session exists. This patch adds an `Expires` header to ensure it's respected by mod_expires.

## Example 1
With the following Apache config:
```apache
<IfModule mod_expires.c>
    ExpiresActive on
    ExpiresDefault                                      "access plus 1 month"
</IfModule>
```
The HTTP response headers are:
### Without the patch
```
HTTP/1.1 200 OK
Date: Fri, 06 Sep 2019 08:02:02 GMT
Server: Apache/2.4.37 (Ubuntu)
Cache-Control: max-age=0, must-revalidate, private
Cache-Control: max-age=2592000
Expires: Sun, 06 Oct 2019 08:02:00 GMT
Vary: Accept-Encoding
Content-Encoding: gzip
Content-Length: 13099
Connection: close
Content-Type: text/html; charset=UTF-8
```
### With the patch
```
HTTP/1.1 200 OK
Date: Fri, 06 Sep 2019 08:21:34 GMT
Server: Apache/2.4.37 (Ubuntu)
Cache-Control: max-age=0, must-revalidate, private
Expires: Fri, 06 Sep 2019 08:21:34 GMT
Vary: Accept-Encoding
Content-Encoding: gzip
Content-Length: 13098
Connection: close
Content-Type: text/html; charset=UTF-8
```

## Example 2
With the following Apache config:
```apache
<IfModule mod_expires.c>
    ExpiresActive on
    ExpiresDefault                                      "access plus 1 month"
    ExpiresByType text/html                             "access plus 0 seconds"
</IfModule>
```
### Without the patch
```
HTTP/1.1 200 OK
Date: Fri, 06 Sep 2019 08:18:40 GMT
Server: Apache/2.4.37 (Ubuntu)
Cache-Control: max-age=0, must-revalidate, private
Cache-Control: max-age=0
Expires: Fri, 06 Sep 2019 08:18:39 GMT
Vary: Accept-Encoding
Content-Encoding: gzip
Content-Length: 13099
Connection: close
Content-Type: text/html; charset=UTF-8
```
### With the patch
```
HTTP/1.1 200 OK
Date: Fri, 06 Sep 2019 08:20:40 GMT
Server: Apache/2.4.37 (Ubuntu)
Cache-Control: max-age=0, must-revalidate, private
Expires: Fri, 06 Sep 2019 08:20:40 GMT
Vary: Accept-Encoding
Content-Encoding: gzip
Content-Length: 13100
Connection: close
Content-Type: text/html; charset=UTF-8
```

Commits
-------

9e94276 [HttpKernel] Fix Apache mod_expires Session Cache-Control issue
@fabpot fabpot merged commit 9e94276 into symfony:3.4 Sep 8, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
This was referenced Oct 7, 2019
@@ -56,6 +56,7 @@ public function onKernelResponse(FilterResponseEvent $event)
if ($session instanceof Session ? $session->getUsageIndex() !== end($this->sessionUsageStack) : $session->isStarted()) {
$event->getResponse()
->setExpires(new \DateTime())

This comment has been minimized.

Copy link
@danrot

danrot Oct 8, 2019

Contributor

Is it on purpose, that this even overrides the expires header that has been set before this line?

This comment has been minimized.

Copy link
@stof

stof Oct 8, 2019

Member

this is about forcing the page to be uncacheable anyway. So yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.