Views block description is double-escaped if display name is set

Working fine on 9.1 and 9.2
Attached a screenshot for reference.

Working fine on 8.9.x-dev attaching screenshot for same.

This was very easy to reproduce using the steps provided (i.e. a views block, not a custom block) but I have updated the summary to be a bit clearer.

Hello @pameeela, I am using view block, and still not reproducible to me on a fresh install. Adding screenshot for reference.

@Gauravmahlawat your screenshot shows that you have set the block title, not the display name. Please see the screenshot referenced in the issue summary.

Updated STR to clarify.

@pameeela, thank you for clarifying. This issue reproducible now.

Before patch:

After patch:

Please review this.

Patch applied successfully on 8.9.x-dev. Issue is resolved after applying the patch, attaching screenshots for same:

#12 looks good. Moving to RTBC +1

Still needs tests - thanks

Adding new test cases with sample view.
Fixed existing failures.

Fixing phpcs issues in the patch.

I've just tested it.
Unfortunately, it isn't fixed now.

Sorry, I've made a mistake in my previous comment.
I've tested it again with this patch (https://www.drupal.org/files/issues/2021-03-03/3199730-18.patch), it works!

The last patch works fine
Before patch

After patch

RTBC.

Don't forget to change the status when moving to RTBC.

Thanks everyone, can we get a test-only patch here that fails, to demonstrate the new test-coverage resolves the issue.

+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.articles_and_pages.yml
--- a/core/modules/views/tests/src/Functional/Wizard/BasicTest.php
+++ b/core/modules/views/tests/src/Functional/Wizard/BasicTest.php

+++ b/core/modules/views/tests/src/Functional/Wizard/BasicTest.php
@@ -220,4 +227,14 @@ public function testWizardDefaultValues() {
+  public function testEscapedBlockDescription() {
+    // Visit the block placement URL directly and validate block description.

Any reason we're putting this in one of the Wizard tests and not in a general Views UI test?

Thanks, @larowlan. I'll add the test-only patch.

My main intention to keep in Wizard was that, while placing the block it follows the block placement flow. i.e. visit the admin/structure/block page etc. So I thought of to keep in Wizard/BasicTest.php and views module.

Later I realized that we can test it directly by visiting the page admin/structure/block/add/views_block%3Aarticles_and_pages-block_1/

Should I move it along with views_ui module's test cases?

\Drupal\Tests\views_ui\Functional\XssTest would be a good spot I guess, because it already has testNoDoubleEscaping

Refactored test cases and moved to relevant place.

This is is not related to Olivero.

@mherchel I think it got updated by mistake. Sorry for the confusion.

Moving it to needs review as failures is related to test-only patch.

I did the test locally and the title is getting correct, and the test that failed that was previously requested seems to be working correctly as well.

Committed fd9f708 and pushed to 9.2.x. Thanks!

Backported to 9.1.x

For future reference folks, one set of before/after screenshots is enough.

If you come to an issue and find someone else has added them, move on.

Was discussing this with @alexpott who pointed out we should be limiting the allowed tags via

'#allowed_tags' => Xss::getHtmlTagList()

So I've reverted this.

Can we add that function in folks.

This has been plain text since D7... see the following code from D7:

    $form['blocks'][$key]['info'] = array(
      '#markup' => check_plain($block['info']),
    );

so changing this to support html needs a big security think...

My original thought was to change to use a more restrictive tag list. This has the advantage or supporting some tags but being way more restrictive than the admin list that's the default for #markup.

The other option would be to do

    $form['admin_label'] = [
      '#type' => 'item',
      '#title' => $this->t('Block description'),
      '#plain_text' => PlainTextOutput::renderFromHtml($definition['admin_label']),
    ];

This would have the advantage of not changing the behaviour. However, I think restricting the taglist is probably best because then admin labels that have been translated can use tags like bdo which are specifically for translations.

Hi @larowlan @alexpott

    $form['admin_label'] = [
      '#type' => 'item',
      '#title' => $this->t('Block description'),
      '#markup' => $definition['admin_label'],
      '#allowed_tags' => Xss::getHtmlTagList(),
    ];

I tried to add #allowed_tags on the label field, somehow it is not working as expected.
Can it be because it is specifically for render elements?
To confirm this, I've added test-only patch.

For the implementation, I took different approach and added code inside ViewsBlock derivative plugins.
We can filter tags there and then render markup in BlockPluginTrait class.
I've updated patch accordingly.
Can you please confirm if this is the right approach.

Uploading patch by resolving phpcs issues.

@mohit_aghera '#allowed_tags' => Xss::getHtmlTagList(), works as I would expect it.

One thing that is apparent is that we need to match what we do in \Drupal\Core\Block\BlockPluginTrait::buildConfigurationForm() in \Drupal\block\BlockListBuilder::buildBlocksForm()

          $form[$entity_id]['info'] = [
            '#plain_text' => $info['status'] ? $info['label'] : $this->t('@label (disabled)', ['@label' => $info['label']]),
            '#wrapper_attributes' => [
              'class' => ['block'],
            ],
          ];

This needs the same change.

However a problem with using Xss::getHtmlTagList() is that it doesn't allow bdo or any of the other translation related tags and it does allow ul and li which have no business in the form or the listing page. So perhaps the PlainTextOutput::renderFromHtml() is the best solution for now. We should consider opening an issue to add a list of tags for titles that only includes inline html tags.

+++ b/core/lib/Drupal/Core/Block/BlockPluginTrait.php
@@ -161,7 +162,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
-      '#plain_text' => $definition['admin_label'],
+      '#markup' => PlainTextOutput::renderFromHtml($definition['admin_label']),

If we're going to use PlainTextOutput::renderFromHtml() then we can leave this as #plain_text.

This issue is a sort of duplicate of #2568045: Make it impossible to double escape with #plain_text - which is about the wider problem. Also this issue gives me an idea about a good fix for that.

#2568045: Make it impossible to double escape with #plain_text now has an alternate fix in the heart of the render system that will help all cases of #plain_text => MarkupInterface object - it includes some of the test developed here so if we decide to go forward with that we should transfer credit for people who developed the test to that issue and mark this one as a duplicate.

@alexpott
I think that makes sense.
We can close this one and continue work in #2568045
I think that patch seems not getting applied on 8.9.x branch. I can take care of that if we plan to close this one.