Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
29.96 KB

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.

ParisLiakos’s picture

+++ 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
26.15 KB

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.

ParisLiakos’s picture

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

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
26.47 KB

here, that should fix it.

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

dawehner’s picture

FileSize
5.68 KB
23.87 KB

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.86 KB
27.57 KB

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
27.56 KB

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.

ParisLiakos’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.57 KB
27.94 KB

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.

dawehner’s picture

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

dawehner’s picture

FileSize
1.59 KB
27.93 KB

Let's move it to _drupal_bootstrap_code()

dawehner’s picture

FileSize
28.96 KB

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.

ParisLiakos’s picture

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

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

ParisLiakos’s picture

+++ 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

dawehner’s picture

FileSize
1.91 KB
27.99 KB

Good catch!

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

ready to go, thanks!

ParisLiakos’s picture

Title: Convert filter_xss_admin and similar function to the string class » Convert 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.

ParisLiakos’s picture

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

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

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

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

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.25 KB
31.17 KB

Renamed Url to UrlValidator and put valid_url

damiankloip’s picture

+++ 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....

dawehner’s picture

FileSize
5.09 KB
31.42 KB

Better variable name.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. That looks good now!!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

dawehner’s picture

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.