#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' => SafeMarkup::checkPlain($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.
Comment | File | Size | Author |
---|---|---|---|
#9 | review_all_usages_2527360-9.patch | 29.36 KB | tatarbj |
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_tags
property 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.