Similar to #1938972: Start moving string functions into a utility class let's move filter_xss and all it dependencies into the String class.

Files: 
CommentFileSizeAuthor
#29 drupal-1998466-28.patch31.42 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,703 pass(es).
[ View ]
#29 interdiff.txt5.09 KBdawehner
#27 drupal-1998466-27.patch31.17 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,825 pass(es).
[ View ]
#27 interdiff.txt7.25 KBdawehner
#21 drupal-1998466-21.patch27.99 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,921 pass(es).
[ View ]
#21 interdiff.txt1.91 KBdawehner
#17 drupal-1998466-17.patch28.96 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,690 pass(es).
[ View ]
#16 drupal-1998466-16.patch27.93 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,817 pass(es).
[ View ]
#16 interdiff.txt1.59 KBdawehner
#14 drupal-1998466-14.patch27.94 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,677 pass(es).
[ View ]
#14 interdiff.txt3.57 KBdawehner
#12 drupal-1998466-12.patch27.56 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,693 pass(es).
[ View ]
#12 interdiff.txt1.33 KBdawehner
#10 durpal-1998466-9.patch27.57 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 54,555 pass(es), 723 fail(s), and 95 exception(s).
[ View ]
#10 interdiff.txt7.86 KBdawehner
#8 durpal-1998466-8.patch23.87 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#8 interdiff.txt5.68 KBdawehner
#7 drupal-1998466-7.patch26.47 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,058 pass(es).
[ View ]
#4 drupal-1998466-4.patch26.15 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 54,521 pass(es), 30 fail(s), and 510 exception(s).
[ View ]
#1 drupal-1998466-1.patch29.96 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,294 pass(es), 29 fail(s), and 58 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new29.96 KB
FAILED: [[SimpleTest]]: [MySQL] 55,294 pass(es), 29 fail(s), and 58 exception(s).
[ View ]

This one doesn't move the tests yet to PHPUnit but we could, if needed.

ParisLiakos suggested to inject the allowed protocols via a static method.

+++ b/core/lib/Drupal/Component/Utility/String.phpundefined
@@ -13,6 +13,23 @@
+   * The list of allowed protocols configured for the drupal site.

i think we should not have the "drupal" word there, since you know this is a component, and could be used outside from Drupal and stuff

+++ b/core/lib/Drupal/Component/Utility/String.phpundefined
@@ -122,4 +140,370 @@ public static function placeholder($text) {
+   * @return mixed|string An XSS safe version of $string, or an empty string if $string is not@see drupal_validate_utf8()

oops, should be splitted to some lines

+++ b/core/lib/Drupal/Component/Utility/String.phpundefined
@@ -122,4 +140,370 @@ public static function placeholder($text) {
+    return preg_replace_callback('%
+    (
+    <(?=[^a-zA-Z!/])  # a lone <
+    |                 # or
+    <!--.*?-->        # a comment
+    |                 # or
+    <[^>]*(>|$)       # a string that starts with a <, up until the > or the end of the string
+    |                 # or
+    >                 # just a >
+    )%x', array('\Drupal\Component\Utility\String', 'xssSplit'), $string);

needs identation

+++ b/core/lib/Drupal/Component/Utility/String.phpundefined
@@ -122,4 +140,370 @@ public static function placeholder($text) {
+            $thisval = filter_xss_bad_protocol($match[1]);
...
+            $thisval = filter_xss_bad_protocol($match[1]);
...
+            $thisval = filter_xss_bad_protocol($match[1]);

should use the class method

+++ b/core/lib/Drupal/Component/Utility/String.phpundefined
@@ -122,4 +140,370 @@ public static function placeholder($text) {
+        $attr = preg_replace('/
+        ^
+        (
+        "[^"]*("|$)     # - a string that starts with a double quote, up until the next double quote or the end of the string
+        |               # or
+        \'[^\']*(\'|$)| # - a string that starts with a quote, up until the next quote or the end of the string
+        |               # or
+        \S              # - a non-whitespace character
+        )*              # any number of the above three
+        \s*             # any number of whitespaces
+        /x', '', $attr);

indentation again

+++ b/core/lib/Drupal/Component/Utility/String.phpundefined
@@ -122,4 +140,370 @@ public static function placeholder($text) {
diff --git a/core/modules/system/lib/Drupal/system/Tests/Common/ColorTest.php b/core/modules/system/lib/Drupal/system/Tests/Common/ColorTest.php
diff --git a/core/modules/system/lib/Drupal/system/Tests/Common/ColorTest.php b/core/modules/system/lib/Drupal/system/Tests/Common/ColorTest.php
deleted file mode 100644
index 2c32b31..0000000
index 2c32b31..0000000
--- a/core/modules/system/lib/Drupal/system/Tests/Common/ColorTest.php
--- a/core/modules/system/lib/Drupal/system/Tests/Common/ColorTest.php
+++ /dev/nullundefined
+++ /dev/nullundefined
+++ /dev/nullundefined
@@ -1,100 +0,0 @@
@@ -1,100 +0,0 @@
-<?php
-
-/**
- * @file
- * Definition of Drupal\system\Tests\Common\ColorTest.

oops:P

Status:Needs review» Needs work

The last submitted patch, drupal-1998466-1.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.15 KB
FAILED: [[SimpleTest]]: [MySQL] 54,521 pass(es), 30 fail(s), and 510 exception(s).
[ View ]

Let's see whether this works better, but i'mt not optimistic.

Status:Needs review» Needs work

The last submitted patch, drupal-1998466-4.patch, failed testing.

Uncaught PHP Exception Drupal\\Core\\Database\\DatabaseExceptionWrapper: "SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd8.simpletest65608router' doesn't exist: SELECT name, route FROM {router} WHERE pattern_outline IN (..)
during upgrade...this is...weird

Status:Needs work» Needs review
StatusFileSize
new26.47 KB
PASSED: [[SimpleTest]]: [MySQL] 56,058 pass(es).
[ View ]

here, that should fix it.

i think...we should move the allowed protocols stuff out of the string class

StatusFileSize
new5.68 KB
new23.87 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Let's move the protocol specific bits to an URL helper class.

Status:Needs review» Needs work

The last submitted patch, durpal-1998466-8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.86 KB
new27.57 KB
FAILED: [[SimpleTest]]: [MySQL] 54,555 pass(es), 723 fail(s), and 95 exception(s).
[ View ]

Adding the new files would certainly help. Paris mentioned that validateUtf8 should be on Unicode.

Status:Needs review» Needs work

The last submitted patch, durpal-1998466-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.33 KB
new27.56 KB
PASSED: [[SimpleTest]]: [MySQL] 55,693 pass(es).
[ View ]

I'm wondering whether it's okay to call something on the URL class out of the String class, but if we can't do that filterXSS has to be part of something else.

Status:Needs review» Needs work

i dont see, why that would be wrong

+++ b/core/includes/common.incundefined
@@ -912,37 +914,7 @@ function valid_number_step($value, $step, $offset = 0.0) {
function drupal_strip_dangerous_protocols($uri) {

this could use an @see Url::stripD...

+++ b/core/includes/common.incundefined
@@ -960,10 +932,10 @@ function drupal_strip_dangerous_protocols($uri) {
+ * @see \Drupal\Component\Utility\String::stripDangerousProtocols()

This needs update

+++ b/core/includes/common.incundefined
@@ -1222,27 +997,14 @@ function _filter_xss_attributes($attr) {
+function filter_xss_bad_protocol($string) {
+  return String::filterBadProtocol($string);

this should be Url::..seems there is no coverage there:/

+++ b/core/lib/Drupal/Component/Utility/String.phpundefined
@@ -122,4 +132,265 @@ public static function placeholder($text) {
+   * @param array $m
...
+   * @param bool $store
+   *   Whether to store $m.
...
+  protected static function xssSplit($m, $store = FALSE) {
+    static $allowed_html;

this is really freaking ugly, but i understand that might be out of scope here, since we are just moving arounnd code

+++ b/core/lib/Drupal/Component/Utility/Url.phpundefined
@@ -0,0 +1,97 @@
+ * Helper to class to support filtering bad protocols from an url.

the first "to" should be removed

Status:Needs work» Needs review
StatusFileSize
new3.57 KB
new27.94 KB
PASSED: [[SimpleTest]]: [MySQL] 55,677 pass(es).
[ View ]

this should be Url::..seems there is no coverage there:/

Well, everything which called filter_xss_bad_protocol before got already converted to use the methods on the classes.

Just in case someone (like paris) is wondering: _drupal_bootstrap_configuration() is before _drupal_bootstrap_kernel() so you don't have the config
available.

StatusFileSize
new1.59 KB
new27.93 KB
PASSED: [[SimpleTest]]: [MySQL] 55,817 pass(es).
[ View ]

Let's move it to _drupal_bootstrap_code()

StatusFileSize
new28.96 KB
PASSED: [[SimpleTest]]: [MySQL] 55,690 pass(es).
[ View ]

Moved from the String class to a Xss class.

Status:Needs review» Needs work
Issue tags:-PHPUnit Blocker

The last submitted patch, drupal-1998466-17.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+PHPUnit Blocker

#17: drupal-1998466-17.patch queued for re-testing.

+++ b/core/includes/common.incundefined
@@ -975,9 +949,17 @@ function check_url($uri) {
+ * * @see \Drupal\Component\Utility\String::filterXssAdmin()
@@ -1003,246 +985,27 @@ function filter_xss_admin($string) {
+ * @see \Drupal\Component\Utility\String::filterXss()

need update to point to Xss class
+++ b/core/lib/Drupal/Component/Utility/String.phpundefined
@@ -13,6 +13,15 @@
+   * The list of html tags allowed by filterAdmin().
+   *
+   * @var array
+   *
+   * @see \Drupal\Component\Utility\String::filterXssAdmin()
+   */
+  protected static $xssAdminTags = array('a', 'abbr', 'acronym', 'address', 'article', 'aside', 'b', 'bdi', 'bdo', 'big', 'blockquote', 'br', 'caption', 'cite', 'code', 'col', 'colgroup', 'command', 'dd', 'del', 'details', 'dfn', 'div', 'dl', 'dt', 'em', 'figcaption', 'figure', 'footer', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'header', 'hgroup', 'hr', 'i', 'img', 'ins', 'kbd', 'li', 'mark', 'menu', 'meter', 'nav', 'ol', 'output', 'p', 'pre', 'progress', 'q', 'rp', 'rt', 'ruby', 's', 'samp', 'section', 'small', 'span', 'strong', 'sub', 'summary', 'sup', 'table', 'tbody', 'td', 'tfoot', 'th', 'thead', 'time', 'tr', 'tt', 'u', 'ul', 'var', 'wbr');
+
+  /**
@@ -25,6 +34,7 @@ class String {
+   *

no longer needed

StatusFileSize
new1.91 KB
new27.99 KB
PASSED: [[SimpleTest]]: [MySQL] 55,921 pass(es).
[ View ]

Good catch!

Status:Needs review» Reviewed & tested by the community

ready to go, thanks!

Title:Convert filter_xss_admin and similar function to the string classConvert filter_xss_admin and similar function to an Xss component

Status:Reviewed & tested by the community» Needs work
Issue tags:-PHPUnit Blocker

The last submitted patch, drupal-1998466-21.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+PHPUnit Blocker

#21: drupal-1998466-21.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

sigh, entity translation module is infested with random failures:(

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new7.25 KB
new31.17 KB
PASSED: [[SimpleTest]]: [MySQL] 55,825 pass(es).
[ View ]

Renamed Url to UrlValidator and put valid_url

+++ b/core/lib/Drupal/Component/Utility/Xss.phpundefined
@@ -0,0 +1,285 @@
+  protected static function attributes($attr) {
+    $attrarr = array();
...
+    $attrname = '';

Seems like we could improve the param/variable names here, and go for $attribute etc.. maybe attribute_array....

StatusFileSize
new5.09 KB
new31.42 KB
PASSED: [[SimpleTest]]: [MySQL] 55,703 pass(es).
[ View ]

Better variable name.

Status:Needs review» Reviewed & tested by the community

Thanks. That looks good now!!

Status:Reviewed & tested by the community» Fixed

Committed 23b5912 and pushed to 8.x. Thanks!

Thanks for committing.

Added a follow up for the phpunit tests: #2006568: Convert filter_xss tests to unit tests

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