Support from Acquia helps fund testing for Drupal Acquia logo

Comments

stefan.r created an issue. See original summary.

stefan.r’s picture

stefan.r’s picture

Status: Active » Needs review
Issue tags: +Needs manual testing
FileSize
5.1 KB

Took these "as is" out of the parent patch

stefan.r’s picture

Title: Remove use of SafeMarkup::checkPlain() from adminSummary() in views plugins » Remove use of SafeMarkup::checkPlain() from adminSummary() and adminLabel() in views plugins
FileSize
2.56 KB
8.01 KB

including a fix to https://www.drupal.org/node/2545972#comment-10225417 (from #74 in the parent)

stefan.r’s picture

reverting an unrelated change

The last submitted patch, 4: 2564283-4.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: 2564283-5.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.3 KB
8.17 KB

So Drupal\views_ui\Tests\XssTest in HEAD is hilarious - it is testing for double escaping :)

Removed a few Html::escape() - the whole point here is to rely on Twig to autoescape.

alexpott’s picture

Needed a reroll.

stefan.r’s picture

Issue summary: View changes
FileSize
10.7 KB
14.38 KB
27.74 KB
34.84 KB
21.77 KB
54.18 KB
+++ b/core/modules/views/src/Plugin/views/HandlerBase.php
@@ -181,8 +181,7 @@ protected function defineOptions() {
+      return $this->options['admin_label'];

Looks like we may have some double escaping here?

This one is double escaped both in HEAD and in the patch:

These look the same:

alexpott’s picture

@stefan.r can I get some steps to reproduce - thanks.

joelpittet’s picture

Adding a test for double escaping. With the steps to reproduce we can add to this.

dawehner’s picture

Quick note: EntityRow.php no longer needs the SafeMarkup use statement.

  1. +++ b/core/modules/views_ui/src/Tests/XssTest.php
    @@ -29,8 +29,22 @@ public function testViewsUi() {
    -    $this->assertRaw('[title] == <marquee>test</marquee>', 'Token label is properly escaped.');
    -    $this->assertRaw('[title_1] == <script>alert("XSS")</script>', 'Token label is properly escaped.');
    +    $this->assertEscaped('[title] == <marquee>test</marquee>', 'Token label is properly escaped.');
    +    $this->assertEscaped('[title_1] == <script>alert("XSS")</script>', 'Token label is properly escaped.');
    

    This is so much more readable!

  2. +++ b/core/modules/views_ui/src/Tests/XssTest.php
    @@ -29,8 +29,22 @@ public function testViewsUi() {
    +  public function testNoDoubleEscaping() {
    +    $this->drupalGet('admin/structure/views');
    +    $this->assertNoEscaped('&lt;');
    +
    +    $this->drupalGet('admin/structure/views/view/sa_contrib_2013_035');
    +    $this->assertNoEscaped('&lt;');
    +
    +    $this->drupalGet('admin/structure/views/nojs/handler/sa_contrib_2013_035/page_1/header/area');
    +    $this->assertNoEscaped('&lt;');
    

    Nice test!

alexpott’s picture

joelpittet’s picture

While trying to reproduce this I found an actual alert(), which is rare consider we escape things like gangbusters.

  1. Steps, add a plain text single value "field_alert" with <script>alert('Title');</script> in every field possible on article.
  2. Go to the admin content view and add that new field as a filter.
  3. As soon as you click the checkbox, boom right in the kisser

Will try to figure out where that is coming from.

joelpittet’s picture

re: #15 it's the field label value, it's on click, and this patch doesn't change it (bug in head), needs a follow-up.

joelpittet’s picture

alexpott’s picture

Address pre 1 of #13.

alexpott’s picture

dawehner’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -Needs steps to reproduce

Well, then nevermind my last comment.

joelpittet’s picture

RTBC++ I'm in a recursive comment! in a recursive comment!

joelpittet’s picture

The double escaping mentioned in #10 looks to be mostly there before this patch. Going to need to track that down and add some double escaping tests for that. @stefan.r could you hook us up with your steps to reproduce that?

dawehner’s picture

joelpittet’s picture

Oh wasn't following that one yet, thanks for the heads up

  • catch committed 52d995f on 8.0.x
    Issue #2564283 by alexpott, stefan.r, joelpittet: Remove use of...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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