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.
API changes
None
Data model changes
None
Release notes snippet
None
Comment | File | Size | Author |
---|---|---|---|
#53 | after_patch.png | 41.04 KB | smustgrave |
#53 | before_patch.png | 49.75 KB | smustgrave |
#51 | AfterPatch3061148-50.png | 17.18 KB | Akhildev.cs |
#51 | BeforePatch3061148-50.png | 19.45 KB | Akhildev.cs |
#50 | 3061148-50.patch | 2.72 KB | lauriii |
Issue fork drupal-3061148
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:
Comments
Comment #2
idebr CreditAttribution: idebr at iO commentedComment #5
Ramya Balasubramanian CreditAttribution: Ramya Balasubramanian at Srijan | A Material+ Company for Drupal India Association commentedComment #6
Ramya Balasubramanian CreditAttribution: Ramya Balasubramanian at Srijan | A Material+ Company for Drupal India Association commentedHi @idebr,
I have added a patch and screenshot. Please have a look and let me know if there are any issues.
Comment #7
kurinjiselvan v CreditAttribution: kurinjiselvan v commentedComment #8
xjmThanks 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:@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:
(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!
Comment #9
Ramya Balasubramanian CreditAttribution: Ramya Balasubramanian at Srijan | A Material+ Company for Drupal India Association commentedThanks @kurinjiselvan and @xjm. I will work on that and apply the patch for the same.
Comment #10
Ramya Balasubramanian CreditAttribution: Ramya Balasubramanian at Srijan | A Material+ Company for Drupal India Association commentedHi @xjm,
I have updated the patch and attached the screenshots as well. Please have a look. Let me know if there are any issues.
Comment #11
Ramya Balasubramanian CreditAttribution: Ramya Balasubramanian at Srijan | A Material+ Company for Drupal India Association commentedComment #12
pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commentedpatch looks good.
Comment #13
xjmThanks @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:
Comment #14
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commented@xjm Test Added, Please review.
Comment #15
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedComment #16
rajneeshb CreditAttribution: rajneeshb as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@vsujeetkumar as per #8 the test case is missing for the active block.
Comment #17
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commented@rajneeshb test added, Please review.
Comment #18
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedVerified 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 -
After -
Comment #19
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedComment #21
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedApplied 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:
After:
Comment #22
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedComment #23
quietone CreditAttribution: quietone as a volunteer commentedWhat 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.
These changes look to be out of scope. Are these changes necessary?
this variable is not used.
drupalPostFopm is deprecated
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.
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.
Comment #24
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedComment #25
raman.b CreditAttribution: raman.b at OpenSense Labs commentedAddressing pointers from #23
Comment #26
raman.b CreditAttribution: raman.b at OpenSense Labs commentedApplying IS template
Comment #29
mitthukumawat CreditAttribution: mitthukumawat as a volunteer and at Zyxware Technologies for Drupal Association commentedPatch #25 applied successfully and the block title is looking fine when it disabled. Adding screenshots for reference.
Comment #30
quietone CreditAttribution: quietone as a volunteer commented@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.
Why not just change this to
#markup' => $info['status'] ? $info['label'] : $this->t('@label (disabled)', ['@label' => $info['label']]),
?Comment #31
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedRe-roll patch created for 9.3.x.
Comment #32
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commented@quietone I have worked on #30, And its working fine, Patch creatd, Please have a look.
Comment #33
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #34
joachim CreditAttribution: joachim at Factorial GmbH commentedPossibly because
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.
Comment #36
vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commentedApplied patch #25 applied successfully
Looking good for me
Thanks for the patch
Comment #37
bnjmnmRemoving credit for #36 which provides screenshots already added in #29
Comment #38
quietone CreditAttribution: quietone as a volunteer commentedNeeds a reroll.
Comment #39
karishmaamin CreditAttribution: karishmaamin at Specbee commentedRe-rolled patch against 9.4.x. Please review
Comment #40
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #42
ankithashettyUpdated the rerolled patch and attached an diff file between the perviously rerolled patch and the old patch as well, thanks!
Comment #43
quietone CreditAttribution: quietone as a volunteer commentedI 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.
Delete this and just use 'foo', for the name in the path later.
There is no need to use a random method here, 'test' is fine.
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).
Comment #44
pmaguniaComment #45
pmaguniaWhen I tested on my local I'm still getting a failing test:
This was using #42
Comment #46
pmaguniaI think it was because I was using 9.3. Let me see if this works.
Comment #47
pmaguniaYay! It passed.
Comment #48
Anishnirmal CreditAttribution: Anishnirmal as a volunteer and at Eleviant Tech commentedI 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:
After Patch:
Comment #49
Anishnirmal CreditAttribution: Anishnirmal as a volunteer and at Eleviant Tech commentedThe patch at #46 works fine. Added before and after patch screenshots in the above comment.
Comment #50
lauriiiThe 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.
Comment #51
Akhildev.cs CreditAttribution: Akhildev.cs as a volunteer and at Zyxware Technologies commentedHi,
patch#50 applied successfully and work's fine for me.
Comment #53
smustgrave CreditAttribution: smustgrave at Mobomo commentedTested #50
Verified the issue
Verified the patch fixes the issue
Attaching screenshots for proof. I see the tests patch as well.
Comment #54
alexpottRemoved 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.