Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WebProfilerExtension] Fixes escaping for a safe output with multiple roles#21385
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
| { | ||
| $profilerDump =function (\Twig_Environment$env,$value,$maxDepth =0) { | ||
| return$valueinstanceof Data ?$this->dumpData($env,$value,$maxDepth) :twig_escape_filter($env,$this->dumpValue($value)); | ||
| return$valueinstanceof Data ?$this->dumpData($env,$value,$maxDepth) :$this->dumpValue($value); |
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 wrong. The ValueExporter does not provide safe output (it does not handle escaping)
stof commentedJan 24, 2017
Btw, dumpValue is deprecated. So if the security panel really uses this code path, we should upgrade it. |
linaori commentedJan 24, 2017
Well, this was the old behavior and that worked (regardless of safe), which broke in 3.2. I indeed noticed the deprecation. Is the I don't see any concrete implementations of this yet, can you point me at one or do you have an idea how this should be fixed in a generic way? To me it looks like all the DataCollectors have to be updated. |
stof commentedJan 24, 2017
Well, we need to figure out where the double-escaping takes place exactly. But removing this one looks wrong to me. The ValueExporter isnot escaping, meaning that this escaping cannot be removed (otherwise the output of the function is not safe anymore, thus lying to Twig and potentially opening XSS holes) |
stof commentedJan 24, 2017
I just identifier the real source of the bug, so I'm closing this PR and will open one with the real fix |
stof commentedJan 24, 2017
see#21387 for the real fix (and the explanation of the bug) |
This fixes the issue reported about escaping the the roles if multiple are shown. I don't think this should be filtered by twig as this was not present in the old version in 3.1. /cc@wouterj