[WebProfilerExtension] Fixes escaping for a safe output with multiple roles #21385
Conversation
| @@ -66,7 +66,7 @@ public function leave(\Twig_Profiler_Profile $profile) | |||
| public function getFunctions() | |||
| { | |||
| $profilerDump = function (\Twig_Environment $env, $value, $maxDepth = 0) { | |||
| return $value instanceof Data ? $this->dumpData($env, $value, $maxDepth) : twig_escape_filter($env, $this->dumpValue($value)); | |||
| return $value instanceof Data ? $this->dumpData($env, $value, $maxDepth) : $this->dumpValue($value); | |||
There was a problem hiding this comment.
this is wrong. The ValueExporter does not provide safe output (it does not handle escaping)
|
Btw, dumpValue is deprecated. So if the security panel really uses this code path, we should upgrade it. |
|
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. |
|
Well, we need to figure out where the double-escaping takes place exactly. But removing this one looks wrong to me. The ValueExporter is not 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) |
|
I just identifier the real source of the bug, so I'm closing this PR and will open one with the real fix |
|
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