Problem/Motivation

If a block's admin title contains text such as ' then when it's disabled, that is double-escaped.

Eg, not disabled:

This block's great!

Disabled:

This block's great! (disabled)

Steps to reproduce

From #18:

Steps to test -
1. Go to the admin site.
2. Go to /admin/structure/block/list/bartik.
3. Click on Place block.
4. Pop-up will open, click on Add custom block.
5. Add a custom block, use a title with characters that are HTML escaped, and click on save.
6. Select the region where it should display and click on save block.
7. Click on Configure, select disable.
8. Verify.

Proposed resolution

From #8:

The #plain_text is meant to escape $info['label'], so this is introducing an XSS sanitization bypasses. The problem is having
#plain_text used for a filed that might sometimes contain:

$this->t('@label (disabled)', ['@label' => $info['label']])
@placholders also do their own plain-text escaping, so that means it happens twice for that case. So we still need it to be escaped at least once when the block is rendered.

An alternative would be to rewrite it with something like:

  if ($info_status) {
    $form[$entity_id]['info'][#'plain_text'] = $info['label'];
  }
  else {
    $form[$entity_id]['info'][#'markup'] = $this->t('@label (disabled)', ['@label' => $info['label']]);
  }

Remaining tasks

Write patch
Write tests
Review

User interface changes

Yes, disable block title displays correctly.

Before
After

API changes

None

Data model changes

None

Release notes snippet

None

CommentFileSizeAuthor
#53 after_patch.png41.04 KBsmustgrave
#53 before_patch.png49.75 KBsmustgrave
#51 AfterPatch3061148-50.png17.18 KBAkhildev.cs
#51 BeforePatch3061148-50.png19.45 KBAkhildev.cs
#50 3061148-50.patch2.72 KBlauriii
#50 interdiff.txt2.73 KBlauriii
#48 Block-layout-Drupal-3061148-BeforePatch.png155.29 KBAnishnirmal
#48 Block-layout-Drupal-3061148-AfterPatch.png141.41 KBAnishnirmal
#46 interdiff_42-46.txt1.68 KBpmagunia
#46 disabled-block-escape-3061148-46.patch2.29 KBpmagunia
#43 After.png6.66 KBquietone
#43 Before.png7.3 KBquietone
#42 reroll_diff_3061148_32-42.txt869 bytesankithashetty
#42 interdiff_3061148_39-42.txt502 bytesankithashetty
#42 3061148-42.patch2.41 KBankithashetty
#39 3061148-39.patch2.44 KBkarishmaamin
#36 3061148--after--patch--pic.png13.35 KBvikashsoni
#36 3061148--before--patch--pic.png23.65 KBvikashsoni
#32 interdiff_31-32.txt1.08 KBvsujeetkumar
#32 3061148_32.patch2.41 KBvsujeetkumar
#31 3061148_31.patch2.82 KBvsujeetkumar
#29 After-patch.png62.66 KBmitthukumawat
#29 Before-patch.png60.63 KBmitthukumawat
#25 interdiff_17-25.txt2.69 KBraman.b
#25 3061148_25.patch2.72 KBraman.b
#25 3061148_25-test-only.patch1.56 KBraman.b
#21 3061148-after_patch-17.png10.53 KBAbhijith S
#21 3061148-before_patch-17.png11.36 KBAbhijith S
#18 After_2.png69.62 KBpriyanka.sahni
#18 After#17.png64.27 KBpriyanka.sahni
#18 Before#17.png58.13 KBpriyanka.sahni
#17 interdiff_14-17.txt780 bytesvsujeetkumar
#17 3061148_17.patch2.97 KBvsujeetkumar
#14 interdiff_10-14.txt1.25 KBvsujeetkumar
#14 3061148_14.patch2.71 KBvsujeetkumar
#10 block-admin-title-double-escaped-review-3061148-10.patch1.34 KBRamya Balasubramanian
#10 screenshot-localhost_8888-2020.05.13-11_40_56.png97.79 KBRamya Balasubramanian
#6 screenshot-localhost_8888-2020.05.11-17_02_25.png50.6 KBRamya Balasubramanian
#6 block-admin-title-double-escaped-3061148-6.patch771 bytesRamya Balasubramanian

Issue fork drupal-3061148

Command icon Show commands

Start 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

idebr’s picture

Issue summary: View changes

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Ramya Balasubramanian’s picture

Assigned: Unassigned » Ramya Balasubramanian
Ramya Balasubramanian’s picture

Hi @idebr,
I have added a patch and screenshot. Please have a look and let me know if there are any issues.

kurinjiselvan v’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thanks for reporting this issue!

This isn't the correct fix; the #plain_text is meant to escape $info['label'], so this is introducing an XSS sanitization bypasses. The problem is having
#plain_text used for a filed that might sometimes contain:

$this->t('@label (disabled)', ['@label' => $info['label']])

@placholders also do their own plain-text escaping, so that means it happens twice for that case. So we still need it to be escaped at least once when the block is rendered.

An alternative would be to rewrite it with something like:

  if ($info_status) {
    $form[$entity_id]['info'][#'plain_text] = $info['label'];
  }
  else {
    $form[$entity_id]['info'][#'marktup] = $this->t('@label (disabled)', ['@label' => $info['label']]);
  }

(and then of course add the other attributes).

This also needs tests to prove both that an active block label is escaped at least once, and that a disabled block label ins't escaped any more than that.

Thanks!

Ramya Balasubramanian’s picture

Assigned: Unassigned » Ramya Balasubramanian

Thanks @kurinjiselvan and @xjm. I will work on that and apply the patch for the same.

Ramya Balasubramanian’s picture

Hi @xjm,
I have updated the patch and attached the screenshots as well. Please have a look. Let me know if there are any issues.

Ramya Balasubramanian’s picture

Assigned: Ramya Balasubramanian » Unassigned
Status: Needs work » Needs review
pradeepjha’s picture

Status: Needs review » Reviewed & tested by the community

patch looks good.

xjm’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

Thanks @Ramya Balasubramanian for the updated patch!

We still need the tests here as described in #8 -- an automated test that will fail by itself, and pass when combined with this patch. (In order to commit a bug fix, we need an automated to test to prove that we've fixed the bug and ensure that we don't break it again in the future.) For more information about writing tests in Drupal 8 see the following links:

  1. https://www.drupal.org/docs/8/testing
  2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/8.9.x
vsujeetkumar’s picture

Status: Needs work » Needs review
FileSize
2.71 KB
1.25 KB

@xjm Test Added, Please review.

priyanka.sahni’s picture

Assigned: Unassigned » priyanka.sahni
rajneeshb’s picture

Status: Needs review » Needs work

@vsujeetkumar as per #8 the test case is missing for the active block.

vsujeetkumar’s picture

Status: Needs work » Needs review
FileSize
2.97 KB
780 bytes

@rajneeshb test added, Please review.

priyanka.sahni’s picture

FileSize
58.13 KB
64.27 KB
69.62 KB

Verified and tested by applying the patch#17.It looks good to me.Can be moved to RTBC.RTBC +1.

Steps to test -
1. Go to the admin site.
2. Go to /admin/structure/block/list/bartik.
3. Click on Place block.
4. Pop-up will open , click on Add custom block.
5. Add a custom block and click on save.
6. Select the region where it should display and click on save block.
7. Click on Configure , select disable.
8. Verify.

Before -
BeforePatch

After -
After Patch

After Patch

priyanka.sahni’s picture

Assigned: priyanka.sahni » Unassigned

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Abhijith S’s picture

Applied patch #17 on 9.2.x and it works fine. The title for the disabled block is correctly showing after this patch. Adding screenshots below.

Before:
before

After:
after

Abhijith S’s picture

Status: Needs review » Reviewed & tested by the community
quietone’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests +Needs issue summary update

What a nice wee fix!

Starting a review.

Started with the IS. There is no proposed resolution here. Even though this looks to be straightforward having it stated makes it clear for everyone. There is a screen shot in the IS, which is from May 2020 well before the data of the latest patch. Scanning the issue there are screen shots in #18 and #21 both testing the same patch. Why two tests of the same patch? @Abhijith S, can you explain the difference between what you did to create the screen shots than what was done in #18. I suggest using the issue template and completing it and adding screens shots from the latest patch and adding the steps to reproduce from #18 to the IS. Tagging for an IS update.

Then look at the patch.

  1. +++ b/core/modules/block/src/BlockListBuilder.php
    @@ -269,12 +269,22 @@ protected function buildBlocksForm() {
    +            $form[$entity_id]['info'] = [
    +              '#markup' => $this->t('@label (disabled)', ['@label' => $info['label']]),
    +              '#wrapper_attributes' => [
    +                'class' => ['block'],
    

    These changes look to be out of scope. Are these changes necessary?

  2. +++ b/core/modules/block/tests/src/Functional/BlockTest.php
    @@ -556,4 +556,33 @@ public function testBlockUserRoleDelete() {
    +    $block_id = $edit['id'];
    

    this variable is not used.

  3. +++ b/core/modules/block/tests/src/Functional/BlockTest.php
    @@ -556,4 +556,33 @@ public function testBlockUserRoleDelete() {
    +    $this->drupalPostForm('admin/structure/block/add/' . $block_name . '/' . $default_theme, $edit, t('Save block'));
    

    drupalPostFopm is deprecated

  4. +++ b/core/modules/block/tests/src/Functional/BlockTest.php
    @@ -556,4 +556,33 @@ public function testBlockUserRoleDelete() {
    +    $this->assertEqual($elements[0]->getText(), "This block's great! (disabled)", 'Found disabled block title properly.');
    ...
    +    $this->assertEqual($elements[0]->getText(), "This block's great!", 'Found block title properly.');
    

    Assertions should not use the message field unless in a loop or other wise needed to make finding the error easy. I don't think they are needed here. And assertEqual is deprecate.

  5. +++ b/core/modules/block/tests/src/Functional/BlockTest.php
    @@ -556,4 +556,33 @@ public function testBlockUserRoleDelete() {
    +    // Test block title after enable the block
    

    Needs a full stop at the end.

Then I looked at the tags. This does have tests so removing the 'needs tests' tag.

This also needs a fail patch as well.

Setting to NW for the above.

vsujeetkumar’s picture

Assigned: Unassigned » vsujeetkumar
raman.b’s picture

Assigned: vsujeetkumar » Unassigned
Status: Needs work » Needs review
FileSize
1.56 KB
2.72 KB
2.69 KB

Addressing pointers from #23

raman.b’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Applying IS template

The last submitted patch, 25: 3061148_25-test-only.patch, failed testing. View results

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mitthukumawat’s picture

FileSize
60.63 KB
62.66 KB

Patch #25 applied successfully and the block title is looking fine when it disabled. Adding screenshots for reference.

quietone’s picture

Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative

@raman.b, thanks for the IS update, that really helps. It isn't clear to me but I think the proposed resolution is using the 'alternative' mentioned there.

I haven't reviewed the patch because it no longer applies. But I do have a question.

+++ b/core/modules/block/src/BlockListBuilder.php
@@ -269,11 +269,14 @@ protected function buildBlocksForm() {
-            '#plain_text' => $info['status'] ? $info['label'] : $this->t('@label (disabled)', ['@label' => $info['label']]),

Why not just change this to #markup' => $info['status'] ? $info['label'] : $this->t('@label (disabled)', ['@label' => $info['label']]),?

vsujeetkumar’s picture

Status: Needs work » Needs review
FileSize
2.82 KB

Re-roll patch created for 9.3.x.

vsujeetkumar’s picture

@quietone I have worked on #30, And its working fine, Patch creatd, Please have a look.

quietone’s picture

@vsujeetkumar, in #30 I was simply asking the question. If you think it is OK to change that, can you explain why? I would like to know.

joachim’s picture

Possibly because

#markup' => $info['label']

doesn't do the sanitization that's needed? I'm not sure TBH. The tests are passing, and I would have thought there would be coverage for sanitization of the block titles.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

vikashsoni’s picture

Applied patch #25 applied successfully
Looking good for me
Thanks for the patch

bnjmnm’s picture

Removing credit for #36 which provides screenshots already added in #29

quietone’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll.

karishmaamin’s picture

Status: Needs work » Needs review
FileSize
2.44 KB

Re-rolled patch against 9.4.x. Please review

quietone’s picture

Status: Needs review » Needs work

@kurinjiselvan v, thanks for the reroll. Two things. To help reviewers add an interdiff, or a diff, whichever is appropriate. There are instructions for creating an interdiff . If you think a diff is not needed, add a comment stating why.

The other is to run the spelling and coding standards locally before uploading the patch. Have a look at the instructions for running the coding standard checks locally so you can be sure the tests will run before uploading a patch. It also has the advantage of saving resources, including money, for the Drupal association. There are real costs for running the tests.

henzzapp made their first commit to this issue’s fork.

ankithashetty’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.41 KB
502 bytes
869 bytes

Updated the rerolled patch and attached an diff file between the perviously rerolled patch and the old patch as well, thanks!

quietone’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
7.3 KB
6.66 KB

I applied the patch, tested and it works as expected. I made new screenshots using a different test string than the test added here.

A few tweaks to the test and this will be good to go.

  1. +++ b/core/modules/block/tests/src/Functional/BlockTest.php
    @@ -574,4 +574,33 @@ public function testBlockUserRoleDelete() {
    +    $block_name = 'system_powered_by_block';
    

    Delete this and just use 'foo', for the name in the path later.

  2. +++ b/core/modules/block/tests/src/Functional/BlockTest.php
    @@ -574,4 +574,33 @@ public function testBlockUserRoleDelete() {
    +      'id' => strtolower($this->randomMachineName(8)),
    

    There is no need to use a random method here, 'test' is fine.

  3. +++ b/core/modules/block/tests/src/Functional/BlockTest.php
    @@ -574,4 +574,33 @@ public function testBlockUserRoleDelete() {
    +    $this->assertEquals("This block's great! (disabled)", $elements[0]->getText());
    ...
    +    $this->assertEquals("This block's great!", $elements[0]->getText());
    

    If would be better to use $title in these assertions. That way we are sure there is no typo and it would be simpler to change later, if needed.

Also, Removing credit for duplicate sets of screenshots made on the same change (sometimes the patch changed but it was only changes to the test).

pmagunia’s picture

Assigned: Unassigned » pmagunia
pmagunia’s picture

Assigned: pmagunia » Unassigned

When I tested on my local I'm still getting a failing test:

--

There was 1 failure:

1) Drupal\Tests\block\Functional\BlockTest::testBlockTitle
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'This block's great! (disabled)'
+'This block's great! (disabled)'

This was using #42

pmagunia’s picture

Status: Needs work » Needs review
FileSize
2.29 KB
1.68 KB

I think it was because I was using 9.3. Let me see if this works.

pmagunia’s picture

Yay! It passed.

Anishnirmal’s picture

I have verified the patch and tested it against the Drupal version 9.4.x-dev. The patch works fine and I have added the before and after screenshots for reference.

Before Patch:

Before Patch

After Patch:

After Patch

Anishnirmal’s picture

Status: Needs review » Reviewed & tested by the community

The patch at #46 works fine. Added before and after patch screenshots in the above comment.

lauriii’s picture

Priority: Minor » Normal
Status: Reviewed & tested by the community » Needs review
FileSize
2.73 KB
2.72 KB

The patch in #32 re-introduces the problems that were pointed out by #8. I added test coverage for the specific bug this would have introduced so that it doesn't get accidentally introduced again.

Akhildev.cs’s picture

Hi,
patch#50 applied successfully and work's fine for me.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
49.75 KB
41.04 KB

Tested #50

Verified the issue
Verified the patch fixes the issue

Attaching screenshots for proof. I see the tests patch as well.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Removed issue credit for @smustgrave because the screenshots are duplicates of the screenshots in #51
Removed issue credit for @Abhijith S becuase the screenshots are duplicated of #18
Removed issue credit for @henzzapp because I couldn't see any work on the MR

Screenshots for @Akhildev.cs, @mitthukumawat, @Anishnirmal and @priyanka.sahni has got issue credit for screenshots because they were for new patches.

Ran new test against 10.0.x and it passed.

Committed and pushed 509179528e to 10.1.x and 3d93ff7804 to 10.0.x and 845ded5368 to 9.5.x. Thanks!

I don't think we should backport this to 9.4.x because it changes render array structure and we try not to change that in patch releases and this fix doesn't not seem to warrant that.

  • alexpott committed 5091795 on 10.1.x
    Issue #3061148 by vsujeetkumar, Ramya Balasubramanian, raman.b,...

  • alexpott committed 3d93ff7 on 10.0.x
    Issue #3061148 by vsujeetkumar, Ramya Balasubramanian, raman.b,...

  • alexpott committed 845ded5 on 9.5.x
    Issue #3061148 by vsujeetkumar, Ramya Balasubramanian, raman.b,...

Status: Fixed » Closed (fixed)

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