Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Don't use eval() to render ESI/SSI#50238
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fabpot commentedMay 5, 2023
Thank you@nicolas-grekas. |
| ob_start(); | ||
| if ($response->headers->has('X-Body-File')) { | ||
| include$response->headers->get('X-Body-File'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why still have a case relying on including a file here. Is this something we should also try to change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
or maybe combiningX-Body-File andX-Body-Eval is an impossible case ? And if we stop using PHP code, wouldn't we break this case that expect to include the file to evaluate it ?
nicolas-grekasMay 5, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There's no need to change this: the required file cannot come from any user input, and we do validate the name of the file, so that arbitrary file inclusion is not possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Well, from my understanding of re-reading this code, thisX-Body-File corresponds to a file written in the cache store. If we stop using PHP to implement the ESI logic, we might need to process the boundaries there instead of evaluating the file.
Do we have functional tests covering the case of a processing of ESI tags alongside a cached response of HttpCache ? Because I suspect that this is the case that is now broken (we would write the cache with boundaries instead of PHP code but read it as if it was PHP code).
| $i +=2; | ||
| } | ||
| $content =implode('',$chunks); | ||
| $content =$boundary.implode('',$chunks).$boundary; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
instead of putting the boundaries around the content of the returned response, which force the caller to be aware of it to remove it (even if the content has no ESI tag), we might put the boundary in a headerX-Body-Boundary (that the caller can still remove), which might make the removal easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The caller has to be aware of the content in any cases.
Putting it before+after allows a quick check to ensure it's correct in HttpCache:if (substr($content, -24) === $boundary = substr($content, 0, 24)) {
| $this->assertSame($response,$esi->process($request,$response)); | ||
| $this->assertEquals(' Keep this And this',$response->getContent()); | ||
| $this->assertEquals(' Keep this And this',substr($response->getContent(),24, -24)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks like a BC break not suitable in a patch release as projects might use the Esi class directly.
nicolas-grekasMay 5, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a security-related fix, I'd better break existing implems that do that so that they can adjust. (I also doubt this will hit anyone in practice 🤞 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
then the boundary length should at least be a public constant, so that they don't have to hardcode this24 everywhere
Because this might be an important security hardening, this PR is a backport of#50013 for 5.4.