[sechole] Remove blacklist mode from Filter:XSS

One bad call to this and your site is toast.

Yes, calling it incorrectly will lead to security problems. Just like forgetting to call Xss::filter() at all will lead to security problems.

Your proposal sounds fine though — as long as the logic continues to work in the same way (i.e. functionally equivalent), I'm fine with that.

RTBC, the proposal looks great to me.

Thanks chx for reminding us that by providing insecure APIs, they will be used in strange ways.

Secure-by-default FTW!

+++ b/core/modules/editor/src/EditorXssFilter/Standard.php
diff --git a/core/tests/Drupal/Tests/Component/Utility/XssTest.php b/core/tests/Drupal/Tests/Component/Utility/XssTest.php
index 7f45b17..7665f7c 100644

index 7f45b17..7665f7c 100644
--- a/core/tests/Drupal/Tests/Component/Utility/XssTest.php

--- a/core/tests/Drupal/Tests/Component/Utility/XssTest.php
+++ b/core/tests/Drupal/Tests/Component/Utility/XssTest.php

+++ b/core/tests/Drupal/Tests/Component/Utility/XssTest.php
+++ b/core/tests/Drupal/Tests/Component/Utility/XssTest.php
@@ -10,6 +10,7 @@

@@ -10,6 +10,7 @@
 use Drupal\Component\Utility\Xss;
+use Drupal\editor\EditorXssFilter\Standard;
 use Drupal\Tests\UnitTestCase;
 
 /**

@@ -462,7 +463,7 @@ public function providerTestFilterXssNotNormalized() {
    */
   public function testBlacklistMode($value, $expected, $message, array $disallowed_tags) {
-    $value = Xss::filter($value, $disallowed_tags, Xss::FILTER_MODE_BLACKLIST);
+    $value = Standard::filter($value, $disallowed_tags);
     $this->assertSame($expected, $value, $message);
   }

I think chx did not wanted to already RTBC this issue.

We do have a compser.json as part of Drupal\Component\Utility so we should ensure that our tests don't rely on some module code,
or in other words you can't really commit and issue which still has remaining tasks :)

@chx
Idealy 3) should be a blocker in general, as it has shown a clear problem in our process.

Woops, sorry. I missed that part in the IS. Thanks, dawehner.

I could not find anything problematic here, the patch looks good. Test coverage is the same and applies to its own scope => RTBC

Patch looks good.

For some reason Drupal\Tests\editor\Unit\EditorXssFilter\StandardTest does not pass locally for me - sending back for a retest.

I broke HEAD - that has nothing to do with my local fails unfortunately. Investigating.

+1

So this is a php version thing.

PHP 5.5.8 (cli) (built: Jan 12 2014 18:50:29) fails
PHP 5.5.18 (cli) (built: Oct 19 2014 15:32:33) passes

I've been working with 5.5.11 for many months now. So this must be something that's changed in PHP in version 5.5.12 or later. Looking at the changelog now.

Most likely candidates:

• http://bugs.php.net/67043 (5.5.12)
• https://bugs.php.net/bug.php?id=67081 (5.5.13)
• http://bugs.php.net/67985 (5.5.18)

Disregard #27 and #28; apparently HEAD works fine with 5.5.18, it's specifically this patch that triggers a failure on 5.5.18. I'm now betting it's late static binding, because that's the only noteworthy change here.

PHP 5.4.24 (cli) (built: Jan 12 2014 18:13:14) fails too
PHP 5.6.0alpha1 (cli) (built: Jan 24 2014 09:16:15) fails

PHP 5.4.34 (cli) (built: Oct 19 2014 15:13:21) passes
PHP 5.6.2 (cli) (built: Oct 19 2014 15:53:04) passes

Looks like https://bugs.php.net/bug.php?id=66622 is catching us.

    $splitter = function ($matches) use ($html_tags) {
      return static::split($matches[1], $html_tags);
    };

We should run our unit tests through travis-ci to test on more PHP versions? (but that is really off-topic - follow-up?)

Added this issue to #1867192: Testbots need to run on 5.4, 5.5, 5.6 and 7.

Did someone verified whether self:: works here without problems? The amount of usecases to override just that particular method is pretty low to be honest.

This fixes the issue by kind of doing our own late static binding. Tested on PHP 5.5.8 and 5.5.18 - works a treat.

A very nice idea! :)

Back to RTBC

This looks good and the workaround for the static binding/anon function issue is both nifty and self-contained.

Committed/pushed to 8.0.x, thanks!

Publish the change record.