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

Files: 
CommentFileSizeAuthor
#13 block-1892504-13.patch4.17 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,418 pass(es).
[ View ]
#13 interdiff.txt2.7 KBtim.plunkett
#9 drupal-block_title_xss-1892504-9.fail_.patch2.92 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 50,741 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#9 drupal-block_title_xss-1892504-9.pass_.patch4.36 KBgrisendo
PASSED: [[SimpleTest]]: [MySQL] 50,700 pass(es).
[ View ]
#2 drupal-block_title_xss-1892504-6955968.patch1.43 KBgrisendo
PASSED: [[SimpleTest]]: [MySQL] 50,584 pass(es).
[ View ]

Comments

grisendo’s picture

I attach a patch

grisendo’s picture

StatusFileSize
new1.43 KB
PASSED: [[SimpleTest]]: [MySQL] 50,584 pass(es).
[ View ]

Status:Needs review» Needs work

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

webchick’s picture

Priority:Normal» Critical

That is un-good. :(

nod_’s picture

Status:Needs work» Needs review
grisendo’s picture

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

Status:Needs review» Reviewed & tested by the community

It's OK to change issue status now?

chx’s picture

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

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new4.36 KB
PASSED: [[SimpleTest]]: [MySQL] 50,700 pass(es).
[ View ]
new2.92 KB
FAILED: [[SimpleTest]]: [MySQL] 50,741 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Tests attached as patches.

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

ParisLiakos’s picture

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

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

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

StatusFileSize
new2.7 KB
new4.17 KB
PASSED: [[SimpleTest]]: [MySQL] 49,418 pass(es).
[ View ]

Here it is, brought up to coding standards.

xjm’s picture

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

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

Assigned:xjm» Unassigned

OK, I'll make a followup.

xjm’s picture

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

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.