#2506195: Remove SafeMarkup::set() from Xss::filter() has revealed that we should be relying on the render system's auto-escape and auto-filtering more. If a string is not marked safe then the render system will automatically escape HTML. Therefore calls to SafeMarkup::checkPlain() like:
$form['admin_label'] = array(
'#type' => 'item',
'#title' => $this->t('Block description'),
'#markup' => Html::escape($definition['admin_label']),
);
Are unnecessary. Here we could just change #markup to #value and rely on auto-escaping.
The calls to Xss::filterAdmin can be replaced by converting to a render array. For example:
$variables['types'][$type->id()] = array(
'type' => $type->id(),
'add_link' => \Drupal::l($type->label(), new Url('node.add', array('node_type' => $type->id()))),
'description' => Xss::filterAdmin($type->getDescription()),
);
We can just do 'description' => array('#markup' => $type->getDescription()), instead because all #markup is auto-filtering for the admin tag list by the render system.
Have a look at the code and remove calls that easily be removed.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #1
alexpottComment #2
xjmComment #7
tatarbjHi all,
let me start to work on this issue, seems a good security improvements for core.
Bests,
Balazs.
Comment #8
tatarbjComment #9
tatarbjHere are the 2 scenarios that should be followed in this issue:
1.
Xss::filterAdmin()because it calls directlyXss::filter()with the$adminTags. So if we rely on auto-filtering for the admin tag by the render system, we can do that is adviced in the description (to change it to render array).So the transition is
Xss:filterAdmin($whatever)->['#markup' => $whatever]2. When we see the
Xss::filter()it can get a parameter with the allowed tags. We can change it with specifically set the#allowed_tagsproperty for the render array. See Render API overview.So here the transition would be:
Xss::filter($whatever, $tags)->['#markup' => $whatever, '#allowed_tags' => $tags].Let me attach a first patch on 8.5.x that makes these changes on non-test classes (and where it makes sense).
I'm still thinking about what's really wrong with Html::escape() as it only triggers a native php function htmlspecialchars(). Anyone could help me to understand why we should also drop them out and how those strings get escaped if we do? (Should be the same pattern followed to use render array for them? It seems a bit weird for me...)
Cheers,
Balazs.
Comment #22
duaelfrThis is quite old!
I was digging into an issue with inline templating within Views and found this piece of code from
\Drupal\views\Plugin\views\PluginBase::viewsTokenReplace():Here, even if we are using Twig's render engine, filterAdmin is called on post_process without any way to bypass it. In my case, I have an inline template that includes an image field rendered with a responsive style. The
<picture>and<srcset>tags are dropped by this post process so I must use an external template instead of the builtin feature of views.Comment #25
prudloff commentedI had a look and only found this call that could easily be removed. But I might have missed some others.
I think several calls have been removed over the years (#2569381: Drupal\views\Plugin\views\area\Result does an unnecessary XSS::adminFilter() for example).
@duaelfr I'm not sure this is related. I think there was some issue about being able to set the list of allowed tags in view templates but I can't find it.
Comment #26
smustgrave commentedNot sure I follow why
Markup::create(Xss::filterAdmin($element['#value']));is no longer needed.Comment #27
xjm@smustgrave Re: #26, wrong issue?
Comment #28
smustgrave commentedUpdated comment sorry!
Comment #29
prudloff commented#markup already calls Xss::filterAdmin() when it is rendered.
Comment #30
xjmA call in the
HtmlTagelement itself; very nice!FWIW regarding #22 which I did not read closely before, I think that is works as designed.
Comment #31
xjmIs any of #9 forward-portable?
Comment #32
prudloff commentedThe patch from #9 blindly replaces Xss::filter() calls with a render array, even in places where the expected type is a string.
For example:
This would definitely not work.
Which is why I did not use it as basis for the MR and started from scratch.
Comment #33
xjmAh good call; I did not actually review it. Thanks @prudloff.
Comment #34
smustgrave commentedTrying to catch up on issues that have been in review for a while.
Can we update the summary with what was done to determine this was the only spot? Was it a search?
If only 1 may be worth a title update as well.