Part of #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping

Problem/Motivation

SafeMarkup::checkPlain() is unnecessary.

Proposed resolution

When escaping thing for render arrays we can use #plain_text => $something or #markup => nl2br(Html::escape($something))

Remaining tasks

Do it
Review
Commit

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott’s picture

Fixing the tests.

The last submitted patch, 2: 2560641.2.patch, failed testing.

Xano’s picture

Your patch looks good. I found some additional usages of SafeMarkup::checkPlain() in render arrays. There are many more places where escaped strings eventually end up in render arrays, but those usages are in separate methods and it sounds like we want to convert those in different issues.

Status: Needs review » Needs work

The last submitted patch, 5: drupal_2560641_5.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
8.2 KB

@Xano #3 was quite carefully scoped... re-uploading #3

  1. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
    @@ -106,19 +105,6 @@ protected function getEntityIds() {
       /**
    -   * Gets the escaped label of an entity.
    -   *
    -   * @param \Drupal\Core\Entity\EntityInterface $entity
    -   *   The entity being listed.
    -   *
    -   * @return string
    -   *   The escaped entity label.
    -   */
    -  protected function getLabel(EntityInterface $entity) {
    -    return SafeMarkup::checkPlain($entity->label());
    -  }
    

    I'm going to separate this out into a separate change... we should deprecate this and remove the checkPlain() rather than remove - it is an unnecessary break.

  2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
    @@ -183,7 +183,7 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
    -            $choice->option = array($term->id() => str_repeat('-', $term->depth) . SafeMarkup::checkPlain(\Drupal::entityManager()->getTranslationFromContext($term)->label()));
    +            $choice->option = array($term->id() => str_repeat('-', $term->depth) . \Drupal::entityManager()->getTranslationFromContext($term)->label());
    
    @@ -201,7 +201,7 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
    -          $options[$term->id()] = SafeMarkup::checkPlain(\Drupal::entityManager()->getTranslationFromContext($term)->label());
    +          $options[$term->id()] = \Drupal::entityManager()->getTranslationFromContext($term)->label();
    
    +++ b/core/modules/user/src/Plugin/views/argument_validator/User.php
    @@ -65,7 +65,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    -      '#options' => array_map(array('\Drupal\Component\Utility\SafeMarkup', 'checkPlain'), user_role_names(TRUE)),
    +      '#options' => user_role_names(TRUE),
    
    +++ b/core/modules/views/src/Plugin/views/exposed_form/ExposedFormPluginBase.php
    @@ -211,7 +210,7 @@ public function exposedFormAlter(&$form, FormStateInterface $form_state) {
    -        $exposed_sorts[$id] = SafeMarkup::checkPlain($handler->options['expose']['label']);
    +        $exposed_sorts[$id] = $handler->options['expose']['label'];
    

    Options are to be handled in another patch.

  3. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
    @@ -740,7 +740,7 @@ public function testRenderCacheProperties(array $expected_results) {
    -      '#custom_property' => SafeMarkup::checkPlain('custom_value'),
    +      '#custom_property' => 'custom_value',
    
    @@ -761,11 +761,6 @@ public function testRenderCacheProperties(array $expected_results) {
    -    // #custom_property_array can not be a safe_cache_property.
    -    $safe_cache_properties = array_diff(Element::properties(array_filter($expected_results)), ['#custom_property_array']);
    -    foreach ($safe_cache_properties as $cache_property) {
    -      $this->assertTrue(SafeMarkup::isSafe($data[$cache_property]), "$cache_property is marked as a safe string");
    -    }
    

    Let's leave this change for later.

alexpott’s picture

lauriii’s picture

Status: Needs review » Needs work

I guess that is patch from wrong issue..

alexpott’s picture

FileSize
51.34 KB

Oops... too many issues :)

alexpott’s picture

Status: Needs work » Needs review
lauriii’s picture

I went through all of the SafeMarkup::checkPlain() calls and only one of the remaining ones is in a render array. I have removed that in the latest patch. Otherwise the patch is RTBC but can someone confirm that my change is fine?

alexpott’s picture

I've made similar changes to @lauriii's several times in the parent issue. I think the changes are correct. The whole SafeMarkup::checkPlain() effect on the title has gone because everything is simpler without it. We're testing that escaping occurs and that tags are stripped in the title in the head section. All looks good to me.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @alexpott for confirming that the changes are fine. With that feedback I'll RTBC the whole.

catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/user/src/Tests/Views/AccessPermissionTest.php
    @@ -35,7 +35,9 @@ function testAccessPerm() {
    +    $this->assertEqual((string) $access_plugin->pluginTitle(), t('Permission'));
    

    Doesn't this change depend on the t() providing a TranslationWrapper issue? It'll work without it, but looks a bit early here. Also this isn't a SafeMarkup::checkPlain() call anyway. Probably just needs dropping from the patch.

  2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -1062,7 +1062,7 @@ public function getArgumentsTokens() {
    +       // were removed by \Drupal\Component\Utility\Html::escape().
    

    Not introduced here but Html::escape() encodes rather than removes. We could probably just do s/removed/encoded/ or not do it here too.

lauriii’s picture

Status: Needs work » Needs review
FileSize
53.45 KB
1.46 KB

Fixed points from #15

alexpott’s picture

Looks like @catch was correct. #16 will be green and the patch to go for...

lauriii’s picture

Hiding patches

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Since #15 where minor nits setting back to rtbc.

tim.plunkett’s picture

+++ b/core/modules/block/src/BlockViewBuilder.php
@@ -164,8 +163,6 @@ protected static function buildPreRenderableBlock($entity, ModuleHandlerInterfac
-    $build['#configuration']['label'] = SafeMarkup::checkPlain($configuration['label']);
-

Not clear why this is being removed. #configuration is used in template_preprocess_block()

alexpott’s picture

@tim.plunkett - because it is already set correctly...

    $build = [
      '#theme' => 'block',
      '#attributes' => [],
      // All blocks get a "Configure block" contextual link.
      '#contextual_links' => [
        'block' => [
          'route_parameters' => ['block' => $entity->id()],
        ],
      ],
      '#weight' => $entity->getWeight(),
      '#configuration' => $configuration,
      '#plugin_id' => $plugin_id,
      '#base_plugin_id' => $base_id,
      '#derivative_plugin_id' => $derivative_id,
      '#id' => $entity->id(),
      '#pre_render' => [
        static::class . '::preRender',
      ],
      // Add the entity so that it can be used in the #pre_render method.
      '#block' => $entity,
    ];
ianthomas_uk’s picture

Status: Reviewed & tested by the community » Needs work

With #16 applied, <title>s for nodes are always double escaped. Before the patch, they are only escaped when rendering from cache (see #2531430: Page's <title> double escaped)

alexpott’s picture

FileSize
74.24 KB
80.3 KB

What @ianthomas_uk (nice spot btw) is referring to is the title tag on the page. Actually before the patch title tags are escaped correctly ie:

<title>&lt;em&gt;MArkup&lt;/em&gt; | Site-Install</title>

and after they are double escaped

<title>&amp;lt;em&amp;gt;MArkup&amp;lt;/em&amp;gt; | Site-Install</title>

Also note that if you use chrome's or firefox's inspector then it does the a single HTML entity decode for you. In order to see what is really going on you have to view the actual page source.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.9 KB
55.35 KB

Fixed it and included the test from #2531430: Page's <title> double escaped - I think we should close that one as a dupe and give credit to everyone who worked on it on this issue too.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

RTBC if this passes

borisson_’s picture

Commenting on this issue for credit. (#2531430: Page's <title> double escaped led me here).

alexpott’s picture

Saving credit. @olli should be added to the commit message if he has not commented by the time this is committed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2560641-2.25.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.74 KB
57.02 KB

So testing a page's title escaping and markup using $this->assertTitle() is a mess because it is use xpath and xpath completely messes with escaping by decoding html entities. Even using ->asXml() doesn't work because quotes are still decoded :(. We should be doing a regular expression on the raw content.

And even the test added by #2531430: Page's <title> double escaped was not asserting what it thought it was as quote are escaped in the title (both in HEAD and in this patch).

We should file a followup to merge NodeTitleXssTest into NodeTitleTest - having two test like this is unnecessary.

I would not be surprised if the changes to assertTitle() cause more test fails but I think we should fix them all here... or move the assertTitle() change into its own issue that blocks this one if there are large amounts of fails.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

#2562359: Merge NodeTitleTest and NodeTitleXSSTest filed the follow-up for the NodeTitleXSSTest and NodeTitleTest. I'm also happy for the current patch.

  • catch committed 3100760 on 8.0.x
    Issue #2560641 by alexpott, lauriii, Xano, borisson_, ianthomas_uk:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 56bcfe4 on 8.0.x
    Issue #2560641 by alexpott, lauriii, Xano, borisson_, ianthomas_uk, olli...
  • catch committed f13b2d5 on 8.0.x
    Revert "Issue #2560641 by alexpott, lauriii, Xano, borisson_,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.