[HttpKernel] ESI fragment content may be missing in conditional requests #58015
Conversation
|
Remaining test failures have to do with Twig and seem unrelated |
There was a problem hiding this comment.
I tried to step through the HttpCache to validate this, because I was unsure if ESI is actually a subrequest. I think I can confirm this works, with a side not though.
Unfortunately, the HttpCache does currently not store the final response in the cache. Which means to validate Last-Modified, we have to fetch the main content and all ESI contents including their headers from the cache, then reuse the ResponseCacheStragey on all of these responses to check if Last-Modified is still the same. If we had the final content cached, we could skip this whole process. Caching the final response only makes sense if there is a validation (Last-Modified or maybe Etag) in it. And it is certainly out of the scope of this PR 😅
Brain dump™ on the process:
HttpCache::handlereceives a request and tries to load it from cache or backend (dependin on whether the method is safe etc.)- then replaces ESI (
HttpCache::restoreResponseBody) on the content - then updates the
ResponseCacheStrategy,addorupdatedepending on the request type (main/sub) - then validates if the
Last-Modifiedheader matches and might throw away the content
This process is the same for the main request and sub request. However, a sub request must not throw away its content, because it must be merged into the main request. Because only after mergin all subrequest, the ResponseCacheStrategy can determine if the content is needed or can be thrown away.
|
@nicolas-grekas updated |
4090d5b to
9fed8dc
Compare
|
Thank you @mpdude. |
The content for ESI-embedded fragments may be missing from the combined (main) response generated by the
HttpCacheunder certain conditions:If-Modified-SinceorIf-None-MatchHttpCachedoes not match the validatorCondition 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 theHttpCachedoes not provideETags, so a client has nothing to validate against.Only since #42355 (merged in to 6.2) the main response will include a
Last-Modifiedheader, 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 provideLast-Modifieddata.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: