Skip to content

Commit e1bbaf2

Browse files
nicolas-grekasnicolas-grekas
committed
bug symfony#58015 [HttpKernel] ESI fragment content may be missing in conditional requests (mpdude)
This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [HttpKernel] ESI fragment content may be missing in conditional requests | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | | License | MIT The content for ESI-embedded fragments may be missing from the combined (main) response generated by the `HttpCache` under certain conditions: 1. The main request sent to the cache must be a conditional request with either `If-Modified-Since` or `If-None-Match` 2. The embedded response processed by the cache matches the validator (last-modified or etag) 3. The resulting (combined) response generated by the `HttpCache` does not match the validator Condition 3 is necessary since otherwise the cache returns an empty 304 response. In that case, the issue still exists, but we don't see or care about it and the wrong body is not sent at all. Regarding condition 2, it does not matter where this embedded response comes from. It may be a fresh (cached) response taken from the cache, it may be a stale cache entry that has been revalidated; probably it can even be a non-cacheable response. In practice, the conditional request will always use `If-Modified-Since`: We're dealing with ESI subrequests, and the combined response created by the `HttpCache` does not provide `ETag`s, so a client has nothing to validate against. Only since symfony#42355 (merged in to 6.2) the main response will include a `Last-Modified` header, given that all of the included responses provided one. Probably that is the reason why this bug was not spotted earlier - it required that change and all of the responses processed by the cache must provide `Last-Modified` data. Conditions 2 + 3 together seem unlikely, but may in fact happen easily when you have an application that generates different chunks of cacheable content for the main and embedded requests and adds last-modified information to them. For example: * First request: Main response modified at time 1, embedded fragment modified at time 2 -> last-modified at 2 * Data for main response changes at time 3 * Second request with "If-Modified-Since" time 2 * Embedded fragment still modified at time 2, main response at time 3 -> last-modified at 3 * the embedded fragment is considered as not modified, content is stripped * main response is generated, fragment content is missing Commits ------- 9fed8dc [HttpKernel] ESI fragment content may be missing in conditional requests
2 parents 643fcae + 9fed8dc commit e1bbaf2

File tree

2 files changed

+172
-1
lines changed

2 files changed

+172
-1
lines changed

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,9 @@ public function handle(Request $request, int $type = HttpKernelInterface::MAIN_R
237237

238238
$response->prepare($request);
239239

240-
$response->isNotModified($request);
240+
if (HttpKernelInterface::MAIN_REQUEST === $type) {
241+
$response->isNotModified($request);
242+
}
241243

242244
return $response;
243245
}

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

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1329,6 +1329,175 @@ public function testEsiCacheSendsTheLowestTtlForHeadRequests()
13291329
$this->assertEquals(100, $this->response->getTtl());
13301330
}
13311331

1332+
public function testEsiCacheIncludesEmbeddedResponseContentWhenMainResponseFailsRevalidationAndEmbeddedResponseIsFresh()
1333+
{
1334+
$this->setNextResponses([
1335+
[
1336+
'status' => 200,
1337+
'body' => 'main <esi:include src="/foo" />',
1338+
'headers' => [
1339+
'Cache-Control' => 's-maxage=0', // goes stale immediately
1340+
'Surrogate-Control' => 'content="ESI/1.0"',
1341+
'Last-Modified' => 'Mon, 12 Aug 2024 10:00:00 +0000',
1342+
],
1343+
],
1344+
[
1345+
'status' => 200,
1346+
'body' => 'embedded',
1347+
'headers' => [
1348+
'Cache-Control' => 's-maxage=10', // stays fresh
1349+
'Last-Modified' => 'Mon, 12 Aug 2024 10:05:00 +0000',
1350+
]
1351+
],
1352+
]);
1353+
1354+
// prime the cache
1355+
$this->request('GET', '/', [], [], true);
1356+
$this->assertSame(200, $this->response->getStatusCode());
1357+
$this->assertSame('main embedded', $this->response->getContent());
1358+
$this->assertSame('Mon, 12 Aug 2024 10:05:00 +0000', $this->response->getLastModified()->format(\DATE_RFC2822)); // max of both values
1359+
1360+
$this->setNextResponses([
1361+
[
1362+
// On the next request, the main response has an updated Last-Modified (main page was modified)...
1363+
'status' => 200,
1364+
'body' => 'main <esi:include src="/foo" />',
1365+
'headers' => [
1366+
'Cache-Control' => 's-maxage=0',
1367+
'Surrogate-Control' => 'content="ESI/1.0"',
1368+
'Last-Modified' => 'Mon, 12 Aug 2024 10:10:00 +0000',
1369+
],
1370+
],
1371+
// no revalidation request happens for the embedded response, since it is still fresh
1372+
]);
1373+
1374+
// Re-request with Last-Modified time that we received when the cache was primed
1375+
$this->request('GET', '/', ['HTTP_IF_MODIFIED_SINCE' => 'Mon, 12 Aug 2024 10:05:00 +0000'], [], true);
1376+
1377+
$this->assertSame(200, $this->response->getStatusCode());
1378+
1379+
// The cache should use the content ("embedded") from the cached entry
1380+
$this->assertSame('main embedded', $this->response->getContent());
1381+
1382+
$traces = $this->cache->getTraces();
1383+
$this->assertSame(['stale', 'invalid', 'store'], $traces['GET /']);
1384+
1385+
// The embedded resource was still fresh
1386+
$this->assertSame(['fresh'], $traces['GET /foo']);
1387+
}
1388+
1389+
public function testEsiCacheIncludesEmbeddedResponseContentWhenMainResponseFailsRevalidationAndEmbeddedResponseIsValid()
1390+
{
1391+
$this->setNextResponses([
1392+
[
1393+
'status' => 200,
1394+
'body' => 'main <esi:include src="/foo" />',
1395+
'headers' => [
1396+
'Cache-Control' => 's-maxage=0', // goes stale immediately
1397+
'Surrogate-Control' => 'content="ESI/1.0"',
1398+
'Last-Modified' => 'Mon, 12 Aug 2024 10:00:00 +0000',
1399+
],
1400+
],
1401+
[
1402+
'status' => 200,
1403+
'body' => 'embedded',
1404+
'headers' => [
1405+
'Cache-Control' => 's-maxage=0', // goes stale immediately
1406+
'Last-Modified' => 'Mon, 12 Aug 2024 10:05:00 +0000',
1407+
]
1408+
],
1409+
]);
1410+
1411+
// prime the cache
1412+
$this->request('GET', '/', [], [], true);
1413+
$this->assertSame(200, $this->response->getStatusCode());
1414+
$this->assertSame('main embedded', $this->response->getContent());
1415+
$this->assertSame('Mon, 12 Aug 2024 10:05:00 +0000', $this->response->getLastModified()->format(\DATE_RFC2822)); // max of both values
1416+
1417+
$this->setNextResponses([
1418+
[
1419+
// On the next request, the main response has an updated Last-Modified (main page was modified)...
1420+
'status' => 200,
1421+
'body' => 'main <esi:include src="/foo" />',
1422+
'headers' => [
1423+
'Cache-Control' => 's-maxage=0',
1424+
'Surrogate-Control' => 'content="ESI/1.0"',
1425+
'Last-Modified' => 'Mon, 12 Aug 2024 10:10:00 +0000',
1426+
],
1427+
],
1428+
[
1429+
// We have a stale cache entry for the embedded response which will be revalidated.
1430+
// Let's assume the resource did not change, so the controller sends a 304 without content body.
1431+
'status' => 304,
1432+
'body' => '',
1433+
'headers' => [
1434+
'Cache-Control' => 's-maxage=0',
1435+
],
1436+
],
1437+
]);
1438+
1439+
// Re-request with Last-Modified time that we received when the cache was primed
1440+
$this->request('GET', '/', ['HTTP_IF_MODIFIED_SINCE' => 'Mon, 12 Aug 2024 10:05:00 +0000'], [], true);
1441+
1442+
$this->assertSame(200, $this->response->getStatusCode());
1443+
1444+
// The cache should use the content ("embedded") from the cached entry
1445+
$this->assertSame('main embedded', $this->response->getContent());
1446+
1447+
$traces = $this->cache->getTraces();
1448+
$this->assertSame(['stale', 'invalid', 'store'], $traces['GET /']);
1449+
1450+
// Check that the embedded resource was successfully revalidated
1451+
$this->assertSame(['stale', 'valid', 'store'], $traces['GET /foo']);
1452+
}
1453+
1454+
public function testEsiCacheIncludesEmbeddedResponseContentWhenMainAndEmbeddedResponseAreFresh()
1455+
{
1456+
$this->setNextResponses([
1457+
[
1458+
'status' => 200,
1459+
'body' => 'main <esi:include src="/foo" />',
1460+
'headers' => [
1461+
'Cache-Control' => 's-maxage=10',
1462+
'Surrogate-Control' => 'content="ESI/1.0"',
1463+
'Last-Modified' => 'Mon, 12 Aug 2024 10:05:00 +0000',
1464+
],
1465+
],
1466+
[
1467+
'status' => 200,
1468+
'body' => 'embedded',
1469+
'headers' => [
1470+
'Cache-Control' => 's-maxage=10',
1471+
'Last-Modified' => 'Mon, 12 Aug 2024 10:00:00 +0000',
1472+
]
1473+
],
1474+
]);
1475+
1476+
// prime the cache
1477+
$this->request('GET', '/', [], [], true);
1478+
$this->assertSame(200, $this->response->getStatusCode());
1479+
$this->assertSame('main embedded', $this->response->getContent());
1480+
$this->assertSame('Mon, 12 Aug 2024 10:05:00 +0000', $this->response->getLastModified()->format(\DATE_RFC2822));
1481+
1482+
// Assume that a client received 'Mon, 12 Aug 2024 10:00:00 +0000' as last-modified information in the past. This may, for example,
1483+
// be the case when the "main" response at that point had an older Last-Modified time, so the embedded response's Last-Modified time
1484+
// governed the result for the combined response. In other words, the client received a Last-Modified time that still validates the
1485+
// embedded response as of now, but no longer matches the Last-Modified time of the "main" resource.
1486+
// Now this client does a revalidation request.
1487+
$this->request('GET', '/', ['HTTP_IF_MODIFIED_SINCE' => 'Mon, 12 Aug 2024 10:00:00 +0000'], [], true);
1488+
1489+
$this->assertSame(200, $this->response->getStatusCode());
1490+
1491+
// The cache should use the content ("embedded") from the cached entry
1492+
$this->assertSame('main embedded', $this->response->getContent());
1493+
1494+
$traces = $this->cache->getTraces();
1495+
$this->assertSame(['fresh'], $traces['GET /']);
1496+
1497+
// Check that the embedded resource was successfully revalidated
1498+
$this->assertSame(['fresh'], $traces['GET /foo']);
1499+
}
1500+
13321501
public function testEsiCacheForceValidation()
13331502
{
13341503
$responses = [

0 commit comments

Comments
 (0)