HttpCache does not consider ESI resources in HEAD requests #24243

mpdude · 2017-09-17T19:44:36Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

Due to this shortcut:

symfony/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php

Lines 634 to 642 in 3b42d88

    
           private function restoreResponseBody(Request $request, Response $response) 
        
           { 
        
               if ($request->isMethod('HEAD') || 304 === $response->getStatusCode()) { 
        
                   $response->setContent(null); 
        
                   $response->headers->remove('X-Body-Eval'); 
        
                   $response->headers->remove('X-Body-File'); 
        
                   return; 
        
               }

... the HttpCache never looks at the response body for HEAD requests. This makes it completely miss ESI-related tweaks like computing the correct TTL, removing validation headers or updating the Content-Length.

From RFC2616 (https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.4):

The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response. The metainformation contained in the HTTP headers in response to a HEAD request SHOULD be identical to the information sent in response to a GET request.

Although it says "SHOULD", I think it can be misleading at best when HEAD requests do, for example, return different (greater) s-maxage values than a corresponding GET request.

mpdude · 2017-09-17T19:45:20Z

src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php

@@ -1133,7 +1133,7 @@ public function testEsiCacheSendsTheLowestTtl()
            array(
                'status' => 200,
                'body' => 'Hello World!',
-                'headers' => array('Cache-Control' => 's-maxage=300'),
+                'headers' => array('Cache-Control' => 's-maxage=200'),


Using a different maxage here so one can better tell the three responses apart.

mpdude · 2017-09-17T19:46:12Z

src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php

@@ -1147,8 +1147,33 @@ public function testEsiCacheSendsTheLowestTtl()
        $this->request('GET', '/', array(), array(), true);
        $this->assertEquals('Hello World! My name is Bobby.', $this->response->getContent());

-        // check for 100 or 99 as the test can be executed after a second change
-        $this->assertTrue(in_array($this->response->getTtl(), array(99, 100)));


We probably don't need this workaround anymore since we've got clock mocking (agree @nicolas-grekas?)

mpdude · 2017-09-17T19:46:47Z

src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php

@@ -1192,7 +1248,6 @@ public function testEsiRecalculateContentLengthHeader()
                'body' => '<esi:include src="/foo" />',
                'headers' => array(
                    'Content-Length' => 26,
-                    'Cache-Control' => 's-maxage=300',


Removed as it is not relevant for this test case

mpdude · 2017-09-17T21:56:03Z

src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php

@@ -633,14 +633,6 @@ protected function store(Request $request, Response $response)
     */
    private function restoreResponseBody(Request $request, Response $response)
    {
-        if ($request->isMethod('HEAD') || 304 === $response->getStatusCode()) {


The 304 === ... part was added a long long time ago in #1413, seemingly not covered by tests.

I think it can safely be removed because $response is the response taken from the cache, which cannot be a 304.

mpdude · 2017-09-17T21:57:18Z

src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php

+            // Response does not include possibly dynamic content (ESI, SSI), so we need
+            // not handle the content for HEAD requests
+            if (!$request->isMethod('HEAD')) {
+                $response->setContent(file_get_contents($response->headers->get('X-Body-File')));


This tries to keep part of the performance optimization removed above – if X-Body-File is intended to mark responses that do not contain dynamic content like ESI/SSI...?

mpdude · 2017-09-26T14:07:15Z

@fabpot The license header in the src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php file is somewhat special. Anything I can do to make fabbot happy?

fabpot · 2017-09-27T04:48:51Z

@mpdude No, just keep it like it is.

fabpot

with minor comments

xabbuh · 2017-09-27T09:24:40Z

But doesn't this now always violate the "The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response." part?

mpdude · 2017-09-27T12:37:50Z

@xabbuh – not sure what you mean. That no body should be returned? There's code elsewhere (can figure it out if you like) that will remove the body for HEAD requests. Until then, we need it to determine Content-Length.

Does that answer your question?

xabbuh

@mpdude Thanks, we indeed call Response::prepare() afterwards. Thus, this looks safe to me. 👍

fabpot · 2017-09-28T12:21:11Z

Thank you @mpdude.

… (mpdude) This PR was squashed before being merged into the 2.7 branch (closes #24243). Discussion ---------- HttpCache does not consider ESI resources in HEAD requests | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Due to this shortcut: https://github.com/symfony/symfony/blob/3b42d8859ea98fdd17012e21f774f55d33cccc3f/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php#L634-L642 ... the `HttpCache` never looks at the response body for `HEAD` requests. This makes it completely miss ESI-related tweaks like computing the correct TTL, removing validation headers or updating the `Content-Length`. From RFC2616 (https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.4): > The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response. The metainformation contained in the HTTP headers in response to a HEAD request SHOULD be identical to the information sent in response to a GET request. Although it says "SHOULD", I think it can be misleading at best when HEAD requests do, for example, return different (greater) `s-maxage` values than a corresponding GET request. Commits ------- 4dd0e53 HttpCache does not consider ESI resources in HEAD requests

Add tests to cover ESI tag handling during HEAD requests

bcf00f1

carsonbot added the Status: Needs Review label Sep 17, 2017

mpdude commented Sep 17, 2017

View reviewed changes

For ESI/SSI responses, evaluate body also during HEAD requests

8614c35

mpdude commented Sep 17, 2017

View reviewed changes

Fix LICENSE header (although I did not touch it)

6c3602d

nicolas-grekas added this to the 2.7 milestone Sep 18, 2017

Fix license header

3beccc6

fabpot approved these changes Sep 27, 2017

View reviewed changes

xabbuh approved these changes Sep 28, 2017

View reviewed changes

xabbuh closed this Sep 28, 2017

mpdude deleted the esi_broken_in_head_requests branch September 28, 2017 14:05

This was referenced Oct 5, 2017

Release v2.7.35 #24450

Merged

Release v2.8.28 #24451

Merged

Release v3.3.10 #24452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HttpCache does not consider ESI resources in HEAD requests #24243

HttpCache does not consider ESI resources in HEAD requests #24243

mpdude commented Sep 17, 2017 •

edited

Loading

mpdude Sep 17, 2017

mpdude Sep 17, 2017

fabpot Sep 27, 2017

mpdude Sep 17, 2017

mpdude Sep 17, 2017

mpdude Sep 17, 2017

mpdude commented Sep 26, 2017

fabpot commented Sep 27, 2017

fabpot left a comment

xabbuh commented Sep 27, 2017

mpdude commented Sep 27, 2017

xabbuh left a comment

fabpot commented Sep 28, 2017

	private function restoreResponseBody(Request $request, Response $response)
	{
	if ($request->isMethod('HEAD') \|\| 304 === $response->getStatusCode()) {
	$response->setContent(null);
	$response->headers->remove('X-Body-Eval');
	$response->headers->remove('X-Body-File');

	return;
	}

HttpCache does not consider ESI resources in HEAD requests #24243

HttpCache does not consider ESI resources in HEAD requests #24243

Conversation

mpdude commented Sep 17, 2017 • edited Loading

mpdude Sep 17, 2017

Choose a reason for hiding this comment

mpdude Sep 17, 2017

Choose a reason for hiding this comment

fabpot Sep 27, 2017

Choose a reason for hiding this comment

mpdude Sep 17, 2017

Choose a reason for hiding this comment

mpdude Sep 17, 2017

Choose a reason for hiding this comment

mpdude Sep 17, 2017

Choose a reason for hiding this comment

mpdude commented Sep 26, 2017

fabpot commented Sep 27, 2017

fabpot left a comment

Choose a reason for hiding this comment

xabbuh commented Sep 27, 2017

mpdude commented Sep 27, 2017

xabbuh left a comment

Choose a reason for hiding this comment

fabpot commented Sep 28, 2017

mpdude commented Sep 17, 2017 •

edited

Loading