XSS in block titles, both in administration pages and in public pages.

CommentFileSizeAuthor
#13 block-1892504-13.patch4.17 KBtim.plunkett
#13 interdiff.txt2.7 KBtim.plunkett
#9 drupal-block_title_xss-1892504-9.fail_.patch2.92 KBgrisendo
#9 drupal-block_title_xss-1892504-9.pass_.patch4.36 KBgrisendo
#2 drupal-block_title_xss-1892504-6955968.patch1.43 KBgrisendo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

grisendo’s picture

grisendo CreditAttribution: grisendo commented

I attach a patch

grisendo’s picture

grisendo CreditAttribution: grisendo commented
FileSize
drupal-block_title_xss-1892504-6955968.patch1.43 KB

Status: Needs review » Needs work

The last submitted patch, drupal-block_title_xss-1892504-6955968.patch, failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Priority: Normal » Critical

That is un-good. :(

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs work » Needs review

#2: drupal-block_title_xss-1892504-6955968.patch queued for re-testing.

grisendo’s picture

grisendo CreditAttribution: grisendo commented

Looks like it passed now.
What's going on with autocomplete in d8? Can't make it work since long time ago...

grisendo’s picture

grisendo CreditAttribution: grisendo commented
Status: Needs review » Reviewed & tested by the community

It's OK to change issue status now?

chx’s picture

chx CreditAttribution: chx commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
grisendo’s picture

grisendo CreditAttribution: grisendo commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
drupal-block_title_xss-1892504-9.pass_.patch4.36 KB
drupal-block_title_xss-1892504-9.fail_.patch2.92 KB

Tests attached as patches.

Two patches: fail patch (tests only) and pass patch (tests + patch).

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

patch overall seems good to me, just

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTitleXSSTest.phpundefined
@@ -0,0 +1,59 @@
+  protected $admin_user;
...
+    $this->admin_user = $this->drupalCreateUser(array('administer blocks', 'access administration pages'));

i am not sure if we do this anymore

grisendo’s picture

grisendo CreditAttribution: grisendo commented

What do you mean exactly? I can see it at:

core/modules/block/lib/Drupal/block/Tests/BlockTest.php, line 22

protected $adminUser;

and

core/modules/block/lib/Drupal/block/Tests/BlockTest.php, line 48

$this->adminUser = $this->drupalCreateUser(array(
  'administer blocks',
  filter_permission_name($full_html_format),
  'access administration pages',
));
ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

i mean there is no reason for them to be properties.
Edit: i think i found an example in AggregatorTestBase
If i read this correctly all you have to do in construct is to login your user:

$web_user = $this->drupalCreateUser(array('administer blocks', 'access administration pages'));
$this->drupalLogin($web_user);
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
interdiff.txt2.7 KB
block-1892504-13.patch4.17 KB

Here it is, brought up to coding standards.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Assigned: Unassigned » xjm

We probably want to add an additional template test similar to #1811684: XSS: Bartik's node.tpl.php prone to XSS (prints $title). I'll look at this.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

I think all of the entities could use some generic XSS checks similar to that other issue, but that'd just be additional coverage. This issue fixes the problem and has test coverage for the bug.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Assigned: xjm » Unassigned

OK, I'll make a followup.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Oh, for the record, #15 is not just @tim.plunkett RTBCing his own patch reroll. I reviewed it too on the 23rd. ;)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Looks good to me. I think it's OK to have a specific test for this. But yeah it wouldn't hurt to have similar "stick some XSS in here and see if it comes out somewhere else" tests for other entities at all.

Status: Fixed » Closed (fixed)

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