Skip to content

[HttpFoundation] Fix isNotModified determination logic #40545

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

Closed
wants to merge 8 commits into from
Closed

[HttpFoundation] Fix isNotModified determination logic #40545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
87430ff
up version
ol0lll Mar 30, 2021
5059145
fix logic, add tests
ol0lll Mar 30, 2021
9eb7eb5
fix cs
ol0lll Mar 30, 2021
a7d701d
Renamed test
ol0lll Apr 12, 2021
0293524
Refactor request header handling for if_none_match and if_modified_si…
ol0lll Apr 12, 2021
07d51e2
license headers
ol0lll Apr 12, 2021
fda3608
Bump version
ol0lll May 17, 2021
b639fe6
Bump version above latest
ol0lll May 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Symfony/Component/HttpFoundation/Request.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1564,7 +1564,7 @@ public function getContent($asResource = false)
*/
public function getETags()
{
return preg_split('/\s*,\s*/', $this->headers->get('if_none_match', ''), -1, \PREG_SPLIT_NO_EMPTY);
return preg_split('/\s*,\s*/', $this->headers->get('If-None-Match', ''), -1, \PREG_SPLIT_NO_EMPTY);
}

/**
Expand Down
25 changes: 20 additions & 5 deletions src/Symfony/Component/HttpFoundation/Response.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1079,12 +1079,27 @@ public function isNotModified(Request $request): bool
$lastModified = $this->headers->get('Last-Modified');
$modifiedSince = $request->headers->get('If-Modified-Since');

if ($etags = $request->getETags()) {
$notModified = \in_array($this->getEtag(), $etags) || \in_array('*', $etags);
}
if ($ifNoneMatchEtags = $request->getETags()) {
$etag = $this->getEtag();
if (0 == strncmp($etag, 'W/', 2)) {
$etag = substr($etag, 2);
}

// Use weak comparison as per https://tools.ietf.org/html/rfc7232#section-3.2.
foreach ($ifNoneMatchEtags as $ifNoneMatchEtag) {
if (0 == strncmp($ifNoneMatchEtag, 'W/', 2)) {
$ifNoneMatchEtag = substr($ifNoneMatchEtag, 2);
}

if ($modifiedSince && $lastModified) {
$notModified = strtotime($modifiedSince) >= strtotime($lastModified) && (!$etags || $notModified);
if ($ifNoneMatchEtag === $etag || '*' === $ifNoneMatchEtag) {
$notModified = true;
break;
}
}
}
// Only do If-Modified-Since date comparison when If-None-Match is not present as per https://tools.ietf.org/html/rfc7232#section-3.3.
elseif ($modifiedSince && $lastModified) {
$notModified = strtotime($modifiedSince) >= strtotime($lastModified);
}

if ($notModified) {
Expand Down
40 changes: 36 additions & 4 deletions src/Symfony/Component/HttpFoundation/Tests/ResponseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ public function testIsNotModifiedEtag()
$etagTwo = 'randomly_generated_etag_2';

$request = new Request();
$request->headers->set('if_none_match', sprintf('%s, %s, %s', $etagOne, $etagTwo, 'etagThree'));
$request->headers->set('If-None-Match', sprintf('%s, %s, %s', $etagOne, $etagTwo, 'etagThree'));

$response = new Response();

Expand All @@ -206,6 +206,38 @@ public function testIsNotModifiedEtag()

$response->headers->set('ETag', '');
$this->assertFalse($response->isNotModified($request));

// Test wildcard
$request = new Request();
$request->headers->set('If-None-Match', '*');

$response->headers->set('ETag', $etagOne);
$this->assertTrue($response->isNotModified($request));
}

public function testIsNotModifiedWeakEtag()
{
$etag = 'randomly_generated_etag';
$weakEtag = 'W/randomly_generated_etag';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for noticing.
Could you submit a PR fixing this with a small explanation?


$request = new Request();
$request->headers->set('If-None-Match', $etag);
$response = new Response();

$response->headers->set('ETag', $etag);
$this->assertTrue($response->isNotModified($request));

$response->headers->set('ETag', $weakEtag);
$this->assertTrue($response->isNotModified($request));

$request->headers->set('If-None-Match', $weakEtag);
$response = new Response();

$response->headers->set('ETag', $etag);
$this->assertTrue($response->isNotModified($request));

$response->headers->set('ETag', $weakEtag);
$this->assertTrue($response->isNotModified($request));
}

public function testIsNotModifiedLastModifiedAndEtag()
Expand All @@ -216,14 +248,14 @@ public function testIsNotModifiedLastModifiedAndEtag()
$etag = 'randomly_generated_etag';

$request = new Request();
$request->headers->set('if_none_match', sprintf('%s, %s', $etag, 'etagThree'));
$request->headers->set('If-None-Match', sprintf('%s, %s', $etag, 'etagThree'));
$request->headers->set('If-Modified-Since', $modified);

$response = new Response();

$response->headers->set('ETag', $etag);
$response->headers->set('Last-Modified', $after);
$this->assertFalse($response->isNotModified($request));
$this->assertTrue($response->isNotModified($request));

$response->headers->set('ETag', 'non-existent-etag');
$response->headers->set('Last-Modified', $before);
Expand All @@ -240,7 +272,7 @@ public function testIsNotModifiedIfModifiedSinceAndEtagWithoutLastModified()
$etag = 'randomly_generated_etag';

$request = new Request();
$request->headers->set('if_none_match', sprintf('%s, %s', $etag, 'etagThree'));
$request->headers->set('If-None-Match', sprintf('%s, %s', $etag, 'etagThree'));
$request->headers->set('If-Modified-Since', $modified);

$response = new Response();
Expand Down
17 changes: 13 additions & 4 deletions src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

/*
* This file is part of the Symfony package.
*
Expand Down Expand Up @@ -382,7 +391,7 @@ protected function validate(Request $request, Response $entry, $catch = false)

// add our cached last-modified validator
if ($entry->headers->has('Last-Modified')) {
$subRequest->headers->set('if_modified_since', $entry->headers->get('Last-Modified'));
$subRequest->headers->set('If-Modified-Since', $entry->headers->get('Last-Modified'));
}

// Add our cached etag validator to the environment.
Expand All @@ -391,7 +400,7 @@ protected function validate(Request $request, Response $entry, $catch = false)
$cachedEtags = $entry->getEtag() ? [$entry->getEtag()] : [];
$requestEtags = $request->getETags();
if ($etags = array_unique(array_merge($cachedEtags, $requestEtags))) {
$subRequest->headers->set('if_none_match', implode(', ', $etags));
$subRequest->headers->set('If-None-Match', implode(', ', $etags));
}

$response = $this->forward($subRequest, $catch, $entry);
Expand Down Expand Up @@ -444,8 +453,8 @@ protected function fetch(Request $request, $catch = false)
}

// avoid that the backend sends no content
$subRequest->headers->remove('if_modified_since');
$subRequest->headers->remove('if_none_match');
$subRequest->headers->remove('If-Modified-Since');
$subRequest->headers->remove('If-None-Match');

$response = $this->forward($subRequest, $catch);

Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ public function testRespondsWith304WhenIfNoneMatchMatchesETag()
$this->assertTraceContains('store');
}

public function testRespondsWith304OnlyIfIfNoneMatchAndIfModifiedSinceBothMatch()
public function testRespondsWith304WhenIfNoneMatchAndIfModifiedSinceBothMatch()
{
$time = \DateTime::createFromFormat('U', time());

Expand All @@ -172,7 +172,7 @@ public function testRespondsWith304OnlyIfIfNoneMatchAndIfModifiedSinceBothMatch(
$t = \DateTime::createFromFormat('U', time() - 3600);
$this->request('GET', '/', ['HTTP_IF_NONE_MATCH' => '12345', 'HTTP_IF_MODIFIED_SINCE' => $t->format(\DATE_RFC2822)]);
$this->assertHttpKernelIsCalled();
$this->assertEquals(200, $this->response->getStatusCode());
$this->assertEquals(304, $this->response->getStatusCode());

// only Last-Modified matches
$this->request('GET', '/', ['HTTP_IF_NONE_MATCH' => '1234', 'HTTP_IF_MODIFIED_SINCE' => $time->format(\DATE_RFC2822)]);
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/HttpKernel/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"symfony/error-handler": "^4.4",
"symfony/event-dispatcher": "^4.4",
"symfony/http-client-contracts": "^1.1|^2",
"symfony/http-foundation": "^4.4|^5.0",
"symfony/http-foundation": "^4.4.24|^5.2.9",
"symfony/polyfill-ctype": "^1.8",
"symfony/polyfill-php73": "^1.9",
"symfony/polyfill-php80": "^1.15",
Expand Down