Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.DraggableListBuilder::buildForm() includes the following code:
$row = $this->buildRow($entity);
if (isset($row['label'])) {
$row['label'] = array('#markup' => $row['label']);
}
Implementations of buildRow() are expected to set $row['label']; in core, at least SearchPageListBuilder and RoleListBuilder do this. If the entity label contains an HTML tag, it is not escaped. I don't think this is a security issue as it seems to be filtered for XSS and I could not manage to execute any JavaScript but it still allows unexpected HTML to be output.
To reproduce this simply create a search page or role with an HTML tag such as <b> in the label and view the listing page.
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | 2656570-23.patch | 1.63 KB | idebr |
| #23 | 2656570-23-test-only.patch | 949 bytes | idebr |
| #20 | draggablelistbuilder-label-2656570-20.patch | 718 bytes | mrinalini9 |
| #3 | 2656570-draggablelistbuilder-label.patch | 722 bytes | longwave |
Comments
Comment #2
longwaveComment #3
longwave#plain_text seems a better choice than #markup.
Comment #12
Kristen PolHmm... maybe I'm misunderstanding. I added a search page without the patch and with the patch and put <b> around the label text but I see the markup around the text in the tab in both cases.
Comment #13
Kristen PolComment #14
joachim CreditAttribution: joachim commented> view the listing page
That means go to the admin list of search pages, not the search page itself.
Comment #15
Kristen PolAh, right. In both cases for me, they showed up as bold. So maybe this has been fixed.
And the tab issue seems like a bug
Comment #16
Kristen PolForgot to include screenshots of listing page.
Comment #17
Kristen PolComment #18
Kristen PolOk, I think I see what's going on. The patch doesn't apply to 9.0.x or 9.1.x. I was using simplytest.me and it didn't complain about the patch not applying. Needs a reroll. Sorry for the confusion.
Comment #19
Kristen PolComment #20
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch #3 for 9.1.x branch, please review.
Comment #21
joachim CreditAttribution: joachim as a volunteer commented> In both cases for me, they showed up as bold
According to the IS, they shouldn't though -- the B tag in the config entity label shouldn't result in actual HTML shown in the admin listing.
Comment #22
longwaveWe need a test to cover this now.
Comment #23
idebr CreditAttribution: idebr at iO commentedAttached patch adds test coverage.
Interdiff = 2656570-23-test-only.patch
Comment #25
Kristen Pol@joachim Regarding
"According to the IS, they shouldn't though -- the B tag in the config entity
label shouldn't result in actual HTML shown in the admin listing"
Yeah. I realized that after I made that comment and forgot to update it. Sorry for the confusion. I was thrown off by the tab showing markup.
Comment #26
Kristen PolThanks for the update.
1) Test code is clear and tests the issue in question.
2) Patch still applies cleanly.
3) Tests pass.
4) Test-only test fails as expected.
5) Manual testing was successful (see screenshots).
Marking RTBC!
Comment #27
alexpottSo this is definitely the correct behaviour for roles - the title is escape on the user account checkboxes and the permissions page. Wondering if this can be said to be generically true for all config entity labels - especially ones a non admin user might see. For example - vocabulary names. So the vocab label is not escaped in the title for the vocab page. And it's not escaped on the entity reference field checkbox. But if you enable layout and add a block to display the vocabulary label it is escaped. I love titles and escaping. Always inconsistent.
I guess the most important place to check is menus because this is a place people often use html in labels. So if you create a label with html in it - it's escaped on the page you can sort the menu list and it is escaped in the menu.
Given the above I think this fix is correct. Escaping html in here makes sense since the labels are used a drag handles so you don't want links to get in the way and this shouldn't affect the disable of labels in a non draggable situation. Given there is no security issue here - this is used in admin situations and we have XSS filtering I'm going to commit this to 9.1.x only just in case this has an unexpected effect. There are many more types of config entities than what I checked.
Committed 687dcd4 and pushed to 9.1.x. Thanks!