#715142 by effulgentsia, msmithgu, mr.baileys, Damien Tournoud, sun: Fixed Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain().

5 files changed, 105 insertions, 50 deletions

diff --git a/includes/common.inc b/includes/common.inc
index 99464d2d3..cd08238bd 100644
--- a/includes/common.inc
+++ b/includes/common.inc
@@ -1202,10 +1202,75 @@ function flood_is_allowed($name, $threshold, $window = 3600, $identifier = NULL)
  */
 
 /**
- * Prepare a URL for use in an HTML attribute. Strips harmful protocols.
+ * Strips dangerous protocols (e.g. 'javascript:') from a URI.
+ *
+ * This function must be called for all URIs within user-entered input prior
+ * to being output to an HTML attribute value. It is often called as part of
+ * check_url() or filter_xss(), but those functions return an HTML-encoded
+ * string, so this function can be called independently when the output needs to
+ * be a plain-text string for passing to t(), l(), drupal_attributes(), or
+ * another function that will call check_plain() separately.
+ *
+ * @param $uri
+ *   A plain-text URI that might contain dangerous protocols.
+ *
+ * @return
+ *   A plain-text URI stripped of dangerous protocols. As with all plain-text
+ *   strings, this return value must not be output to an HTML page without
+ *   check_plain() being called on it. However, it can be passed to functions
+ *   expecting plain-text strings.
+ *
+ * @see check_url()
+ */
+function drupal_strip_dangerous_protocols($uri) {
+  static $allowed_protocols;
+
+  if (!isset($allowed_protocols)) {
+    $allowed_protocols = array_flip(variable_get('filter_allowed_protocols', array('ftp', 'http', 'https', 'irc', 'mailto', 'news', 'nntp', 'rtsp', 'sftp', 'ssh', 'telnet', 'webcal')));
+  }
+
+  // Iteratively remove any invalid protocol found.
+  do {
+    $before = $uri;
+    $colonpos = strpos($uri, ':');
+    if ($colonpos > 0) {
+      // We found a colon, possibly a protocol. Verify.
+      $protocol = substr($uri, 0, $colonpos);
+      // If a colon is preceded by a slash, question mark or hash, it cannot
+      // possibly be part of the URL scheme. This must be a relative URL, which
+      // inherits the (safe) protocol of the base document.
+      if (preg_match('![/?#]!', $protocol)) {
+        break;
+      }
+      // Check if this is a disallowed protocol. Per RFC2616, section 3.2.3
+      // (URI Comparison) scheme comparison must be case-insensitive.
+      if (!isset($allowed_protocols[strtolower($protocol)])) {
+        $uri = substr($uri, $colonpos + 1);
+      }
+    }
+  } while ($before != $uri);
+
+  return $uri;
+}
+
+/**
+ * Strips dangerous protocols (e.g. 'javascript:') from a URI and encodes it for output to an HTML attribute value.
+ *
+ * @param $uri
+ *   A plain-text URI that might contain dangerous protocols.
+ *
+ * @return
+ *   A URI stripped of dangerous protocols and encoded for output to an HTML
+ *   attribute value. Because it is already encoded, it should not be set as a
+ *   value within a $attributes array passed to drupal_attributes(), because
+ *   drupal_attributes() expects those values to be plain-text strings. To pass
+ *   a filtered URI to drupal_attributes(), call
+ *   drupal_strip_dangerous_protocols() instead.
+ *
+ * @see drupal_strip_dangerous_protocols()
  */
 function check_url($uri) {
-  return filter_xss_bad_protocol($uri, FALSE);
+  return check_plain(drupal_strip_dangerous_protocols($uri));
 }
 
 /**
@@ -1453,45 +1518,21 @@ function _filter_xss_attributes($attr) {
  * @param $string
  *   The string with the attribute value.
  * @param $decode
- *   Whether to decode entities in the $string. Set to FALSE if the $string
- *   is in plain text, TRUE otherwise. Defaults to TRUE.
+ *   (Deprecated) Whether to decode entities in the $string. Set to FALSE if the
+ *   $string is in plain text, TRUE otherwise. Defaults to TRUE. This parameter
+ *   is deprecated and will be removed in Drupal 8. To process a plain-text URI,
+ *   call drupal_strip_dangerous_protocols() or check_url() instead.
  * @return
  *   Cleaned up and HTML-escaped version of $string.
  */
 function filter_xss_bad_protocol($string, $decode = TRUE) {
-  static $allowed_protocols;
-
-  if (!isset($allowed_protocols)) {
-    $allowed_protocols = array_flip(variable_get('filter_allowed_protocols', array('ftp', 'http', 'https', 'irc', 'mailto', 'news', 'nntp', 'rtsp', 'sftp', 'ssh', 'telnet', 'webcal')));
-  }
-
   // Get the plain text representation of the attribute value (i.e. its meaning).
+  // @todo Remove the $decode parameter in Drupal 8, and always assume an HTML
+  //   string that needs decoding.
   if ($decode) {
     $string = decode_entities($string);
   }
-
-  // Iteratively remove any invalid protocol found.
-  do {
-    $before = $string;
-    $colonpos = strpos($string, ':');
-    if ($colonpos > 0) {
-      // We found a colon, possibly a protocol. Verify.
-      $protocol = substr($string, 0, $colonpos);
-      // If a colon is preceded by a slash, question mark or hash, it cannot
-      // possibly be part of the URL scheme. This must be a relative URL,
-      // which inherits the (safe) protocol of the base document.
-      if (preg_match('![/?#]!', $protocol)) {
-        break;
-      }
-      // Per RFC2616, section 3.2.3 (URI Comparison) scheme comparison must be case-insensitive
-      // Check if this is a disallowed protocol.
-      if (!isset($allowed_protocols[strtolower($protocol)])) {
-        $string = substr($string, $colonpos + 1);
-      }
-    }
-  } while ($before != $string);
-
-  return check_plain($string);
+  return check_plain(drupal_strip_dangerous_protocols($string));
 }
 
 /**
@@ -1993,13 +2034,13 @@ function url($path = NULL, array $options = array()) {
   );
 
   if (!isset($options['external'])) {
-    // Return an external link if $path contains an allowed absolute URL.
-    // Only call the slow filter_xss_bad_protocol if $path contains a ':'
-    // before any / ? or #.
-    // Note: we could use url_is_external($path) here, but that would
-    // require another function call, and performance inside url() is critical.
+    // Return an external link if $path contains an allowed absolute URL. Only
+    // call the slow drupal_strip_dangerous_protocols() if $path contains a ':'
+    // before any / ? or #. Note: we could use url_is_external($path) here, but
+    // that would require another function call, and performance inside url() is
+    // critical.
     $colonpos = strpos($path, ':');
-    $options['external'] = ($colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && filter_xss_bad_protocol($path, FALSE) == check_plain($path));
+    $options['external'] = ($colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && drupal_strip_dangerous_protocols($path) == $path);
   }
 
   // Preserve the original path before altering or aliasing.
@@ -2113,9 +2154,9 @@ function url($path = NULL, array $options = array()) {
  */
 function url_is_external($path) {
   $colonpos = strpos($path, ':');
-  // Only call the slow filter_xss_bad_protocol if $path contains a ':'
-  // before any / ? or #.
-  return $colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && filter_xss_bad_protocol($path, FALSE) == check_plain($path);
+  // Only call the slow drupal_strip_dangerous_protocols() if $path contains a
+  // ':' before any / ? or #.
+  return $colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && drupal_strip_dangerous_protocols($path) == $path;
 }
 
 /**
diff --git a/includes/theme.inc b/includes/theme.inc
index 6c32aa8f3..67406a8d9 100644
--- a/includes/theme.inc
+++ b/includes/theme.inc
@@ -1854,7 +1854,7 @@ function theme_item_list($variables) {
  *   - url: The url for the link.
  */
 function theme_more_help_link($variables) {
-  return '<div class="more-help-link">' . t('<a href="@link">More help</a>', array('@link' => check_url($variables['url']))) . '</div>';
+  return '<div class="more-help-link">' . l(t('More help'), $variables['url']) . '</div>';
 }
 
 /**
@@ -1868,7 +1868,7 @@ function theme_more_help_link($variables) {
 function theme_feed_icon($variables) {
   $text = t('Subscribe to @feed-title', array('@feed-title' => $variables['title']));
   if ($image = theme('image', array('path' => 'misc/feed.png', 'alt' => $text))) {
-    return '<a href="' . check_url($variables['url']) . '" title="' . $text . '" class="feed-icon">' . $image . '</a>';
+    return l($image, $variables['url'], array('html' => TRUE, 'attributes' => array('class' => array('feed-icon'), 'title' => $text)));
   }
 }
 
@@ -1918,7 +1918,7 @@ function theme_html_tag($variables) {
  *   - title: A descriptive verb for the link, like 'Read more'.
  */
 function theme_more_link($variables) {
-  return '<div class="more-link">' . t('<a href="@link" title="@title">More</a>', array('@link' => check_url($variables['url']), '@title' => $variables['title'])) . '</div>';
+  return '<div class="more-link">' . l(t('More'), $variables['url'], array('attributes' => array('title' => $variables['title']))) . '</div>';
 }
 
 /**
@@ -2172,7 +2172,7 @@ function template_preprocess_html(&$variables) {
   if (theme_get_setting('toggle_favicon')) {
     $favicon = theme_get_setting('favicon');
     $type = theme_get_setting('favicon_mimetype');
-    drupal_add_html_head_link(array('rel' => 'shortcut icon', 'href' => check_url($favicon), 'type' => $type));
+    drupal_add_html_head_link(array('rel' => 'shortcut icon', 'href' => drupal_strip_dangerous_protocols($favicon), 'type' => $type));
   }
 
   // Construct page title.
@@ -2355,7 +2355,7 @@ function template_preprocess_maintenance_page(&$variables) {
   if (theme_get_setting('toggle_favicon')) {
     $favicon = theme_get_setting('favicon');
     $type = theme_get_setting('favicon_mimetype');
-    drupal_add_html_head_link(array('rel' => 'shortcut icon', 'href' => check_url($favicon), 'type' => $type));
+    drupal_add_html_head_link(array('rel' => 'shortcut icon', 'href' => drupal_strip_dangerous_protocols($favicon), 'type' => $type));
   }
 
   global $theme;
diff --git a/modules/comment/comment.test b/modules/comment/comment.test
index ae680cecd..cf5376fec 100644
--- a/modules/comment/comment.test
+++ b/modules/comment/comment.test
@@ -1190,7 +1190,7 @@ class CommentTokenReplaceTestCase extends CommentHelperCase {
     $tests['[comment:hostname]'] = check_plain($comment->hostname);
     $tests['[comment:name]'] = filter_xss($comment->name);
     $tests['[comment:mail]'] = check_plain($this->admin_user->mail);
-    $tests['[comment:homepage]'] = filter_xss_bad_protocol($comment->homepage);
+    $tests['[comment:homepage]'] = check_url($comment->homepage);
     $tests['[comment:title]'] = filter_xss($comment->subject);
     $tests['[comment:body]'] = _text_sanitize($instance, LANGUAGE_NONE, $comment->comment_body[LANGUAGE_NONE][0], 'value');
     $tests['[comment:url]'] = url('comment/' . $comment->cid, $url_options + array('fragment' => 'comment-' . $comment->cid));
diff --git a/modules/comment/comment.tokens.inc b/modules/comment/comment.tokens.inc
index 3f233733d..2375cfb6f 100644
--- a/modules/comment/comment.tokens.inc
+++ b/modules/comment/comment.tokens.inc
@@ -148,7 +148,7 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
           break;
 
         case 'homepage':
-          $replacements[$original] = $sanitize ? filter_xss_bad_protocol($comment->homepage) : $comment->homepage;
+          $replacements[$original] = $sanitize ? check_url($comment->homepage) : $comment->homepage;
           break;
 
         case 'title':
diff --git a/modules/simpletest/tests/common.test b/modules/simpletest/tests/common.test
index f374cf04b..01635a5ea 100644
--- a/modules/simpletest/tests/common.test
+++ b/modules/simpletest/tests/common.test
@@ -345,7 +345,7 @@ class CommonXssUnitTest extends DrupalUnitTestCase {
   public static function getInfo() {
     return array(
       'name' => 'String filtering tests',
-      'description' => 'Confirm that check_plain() and filter_xss() work correctly, including invalid multi-byte sequences.',
+      'description' => 'Confirm that check_plain(), filter_xss(), and check_url() work correctly, including invalid multi-byte sequences.',
       'group' => 'System',
     );
   }
@@ -372,6 +372,20 @@ class CommonXssUnitTest extends DrupalUnitTestCase {
      $text = check_plain("<script>");
      $this->assertEqual($text, '&lt;script&gt;', 'check_plain() escapes &lt;script&gt;');
   }
+
+  /**
+   * Check that harmful protocols are stripped.
+   */
+  function testBadProtocolStripping() {
+    // Ensure that check_url() strips out harmful protocols, and encodes for
+    // HTML. Ensure drupal_strip_dangerous_protocols() can be used to return a
+    // plain-text string stripped of harmful protocols.
+    $url = 'javascript:http://www.example.com/?x=1&y=2';
+    $expected_plain = 'http://www.example.com/?x=1&y=2';
+    $expected_html = 'http://www.example.com/?x=1&amp;y=2';
+    $this->assertIdentical(check_url($url), $expected_html, t('check_url() filters a URL and encodes it for HTML.'));
+    $this->assertIdentical(drupal_strip_dangerous_protocols($url), $expected_plain, t('drupal_strip_dangerous_protocols() filters a URL and returns plain text.'));
+  }
 }
 
 class CommonSizeTestCase extends DrupalUnitTestCase {