Issue #2557467 by alexpott: Deprecate check_url and convert usages to existing UrlHelper methods

core/includes/common.inc

@@ -271,7 +271,7 @@ function valid_email_address($mail) {
  * @param $uri
  *   A plain-text URI that might contain dangerous protocols.
  *
- * @return
+ * @return string
  *   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\Core\Template\Attribute,
@@ -281,10 +281,20 @@ function valid_email_address($mail) {
  *   \Drupal\Component\Utility\UrlHelper::stripDangerousProtocols() instead.
  *
  * @see \Drupal\Component\Utility\UrlHelper::stripDangerousProtocols()
- * @see \Drupal\Component\Utility\SafeMarkup::checkPlain()
+ * @see \Drupal\Component\Utility\UrlHelper::filterBadProtocol()
+ *
+ * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
+ *   Use UrlHelper::stripDangerousProtocols() or UrlHelper::filterBadProtocol()
+ *   instead. UrlHelper::stripDangerousProtocols() can be used in conjunction
+ *   with \Drupal\Component\Utility\SafeMarkup::format() and an @variable
+ *   placeholder which will perform the necessary escaping.
+ *   UrlHelper::filterBadProtocol() is functionality equivalent to check_url()
+ *   apart from the fact it is protected from double escaping bugs. Note that
+ *   this method no longer marks its output as safe.
+ *
  */
 function check_url($uri) {
-  return SafeMarkup::checkPlain(UrlHelper::stripDangerousProtocols($uri));
+  return Html::escape(UrlHelper::stripDangerousProtocols($uri));
 }
 
 /**

core/includes/install.core.inc

@@ -2052,7 +2052,7 @@ function install_check_translations($langcode, $server_pattern) {
         'title'       => t('Translation'),
         'value'       => t('The %language translation is not available.', array('%language' => $language)),
         'severity'    => REQUIREMENT_ERROR,
-        'description' => t('The %language translation file is not available at the translation server. <a href="!url">Choose a different language</a> or select English and translate your website later.', array('%language' => $language, '!url' => check_url($_SERVER['SCRIPT_NAME']))),
+        'description' => t('The %language translation file is not available at the translation server. <a href="@url">Choose a different language</a> or select English and translate your website later.', array('%language' => $language, '@url' => UrlHelper::stripDangerousProtocols($_SERVER['SCRIPT_NAME']))),
       );
     }
     else {
@@ -2071,7 +2071,7 @@ function install_check_translations($langcode, $server_pattern) {
         'title'       => t('Translation'),
         'value'       => t('The %language translation could not be downloaded.', array('%language' => $language)),
         'severity'    => REQUIREMENT_ERROR,
-        'description' => t('The %language translation file could not be downloaded. <a href="!url">Choose a different language</a> or select English and translate your website later.', array('%language' => $language, '!url' => check_url($_SERVER['SCRIPT_NAME']))),
+        'description' => t('The %language translation file could not be downloaded. <a href="@url">Choose a different language</a> or select English and translate your website later.', array('%language' => $language, '@url' => UrlHelper::stripDangerousProtocols($_SERVER['SCRIPT_NAME']))),
       );
     }
   }
@@ -2281,11 +2281,11 @@ function install_display_requirements($install_state, $requirements) {
       $build['report']['#requirements'] = $requirements;
       if ($severity == REQUIREMENT_WARNING) {
         $build['#title'] = t('Requirements review');
-        $build['#suffix'] = t('Check the messages and <a href="!retry">retry</a>, or you may choose to <a href="!cont">continue anyway</a>.', array('!retry' => check_url(drupal_requirements_url(REQUIREMENT_ERROR)), '!cont' => check_url(drupal_requirements_url($severity))));
+        $build['#suffix'] = t('Check the messages and <a href="@retry">retry</a>, or you may choose to <a href="@cont">continue anyway</a>.', array('@retry' => UrlHelper::stripDangerousProtocols(drupal_requirements_url(REQUIREMENT_ERROR)), '@cont' => UrlHelper::stripDangerousProtocols(drupal_requirements_url($severity))));
       }
       else {
         $build['#title'] = t('Requirements problem');
-        $build['#suffix'] = t('Check the messages and <a href="!url">try again</a>.', array('!url' => check_url(drupal_requirements_url($severity))));
+        $build['#suffix'] = t('Check the messages and <a href="@url">try again</a>.', array('@url' => UrlHelper::stripDangerousProtocols(drupal_requirements_url($severity))));
       }
       return $build;
     }

core/includes/install.inc

@@ -864,9 +864,11 @@ function install_goto($path) {
  * @return
  *   The URL of the current script, with query parameters modified by the
  *   passed-in $query. The URL is not sanitized, so it still needs to be run
- *   through check_url() if it will be used as an HTML attribute value.
+ *   through \Drupal\Component\Utility\UrlHelper::filterBadProtocol() if it will be
+ *   used as an HTML attribute value.
  *
  * @see drupal_requirements_url()
+ * @see Drupal\Component\Utility\UrlHelper::filterBadProtocol()
  */
 function drupal_current_script_url($query = array()) {
   $uri = $_SERVER['SCRIPT_NAME'];
@@ -890,10 +892,12 @@ function drupal_current_script_url($query = array()) {
  *
  * @return
  *   A URL for attempting to proceed to the next step of the script. The URL is
- *   not sanitized, so it still needs to be run through check_url() if it will
- *   be used as an HTML attribute value.
+ *   not sanitized, so it still needs to be run through
+ *   \Drupal\Component\Utility\UrlHelper::filterBadProtocol() if it will be used
+ *   as an HTML attribute value.
  *
  * @see drupal_current_script_url()
+ * @see \Drupal\Component\Utility\UrlHelper::filterBadProtocol()
  */
 function drupal_requirements_url($severity) {
   $query = array();

core/lib/Drupal/Component/Utility/UrlHelper.php

@@ -300,10 +300,11 @@ public static function setAllowedProtocols(array $protocols = array()) {
    *
    * 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 Drupal\Component\Utility\Xss::filter(), 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 functions
-   * that will call \Drupal\Component\Utility\SafeMarkup::checkPlain() separately.
+   * \Drupal\Component\Utility\UrlHelper::filterBadProtocol() or
+   * \Drupal\Component\Utility\Xss::filter(), 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 functions that will
+   * call \Drupal\Component\Utility\SafeMarkup::checkPlain() separately.
    *
    * @param string $uri
    *   A plain-text URI that might contain dangerous protocols.

core/modules/aggregator/aggregator.theme.inc

@@ -5,6 +5,7 @@
  * Preprocessors and theme functions of Aggregator module.
  */
 
+use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\Render\Element;
 
 /**
@@ -24,7 +25,7 @@ function template_preprocess_aggregator_item(&$variables) {
     $variables['content'][$key] = $variables['elements'][$key];
   }
 
-  $variables['url'] = check_url($item->getLink());
+  $variables['url'] = UrlHelper::stripDangerousProtocols($item->getLink());
   $variables['title'] = $item->label();
 }
 

core/modules/comment/comment.tokens.inc

@@ -6,6 +6,7 @@
  */
 
 use Drupal\Component\Utility\Html;
+use Drupal\Component\Utility\UrlHelper;
 use Drupal\Component\Utility\Xss;
 use Drupal\Core\Datetime\Entity\DateFormat;
 use Drupal\Core\Render\BubbleableMetadata;
@@ -149,7 +150,7 @@ function comment_tokens($type, $tokens, array $data, array $options, BubbleableM
           break;
 
         case 'homepage':
-          $replacements[$original] = $sanitize ? check_url($comment->getHomepage()) : $comment->getHomepage();
+          $replacements[$original] = $sanitize ? UrlHelper::filterBadProtocol($comment->getHomepage()) : $comment->getHomepage();
           break;
 
         case 'title':

core/modules/comment/src/Tests/CommentTokenReplaceTest.php

@@ -8,6 +8,7 @@
 namespace Drupal\comment\Tests;
 
 use Drupal\Component\Utility\Html;
+use Drupal\Component\Utility\UrlHelper;
 use Drupal\Component\Utility\Xss;
 use Drupal\comment\Entity\Comment;
 use Drupal\Core\Render\BubbleableMetadata;
@@ -55,7 +56,7 @@ function testCommentTokenReplacement() {
     $tests['[comment:hostname]'] = Html::escape($comment->getHostname());
     $tests['[comment:author]'] = Xss::filter($comment->getAuthorName());
     $tests['[comment:mail]'] = Html::escape($this->adminUser->getEmail());
-    $tests['[comment:homepage]'] = check_url($comment->getHomepage());
+    $tests['[comment:homepage]'] = UrlHelper::filterBadProtocol($comment->getHomepage());
     $tests['[comment:title]'] = Xss::filter($comment->getSubject());
     $tests['[comment:body]'] = $comment->comment_body->processed;
     $tests['[comment:langcode]'] = Html::escape($comment->language()->getId());

core/modules/filter/filter.module

@@ -495,11 +495,12 @@ function _filter_url($text, $filter) {
   $tasks = array();
 
   // Prepare protocols pattern for absolute URLs.
-  // check_url() will replace any bad protocols with HTTP, so we need to support
-  // the identical list. While '//' is technically optional for MAILTO only,
-  // we cannot cleanly differ between protocols here without hard-coding MAILTO,
-  // so '//' is optional for all protocols.
-  // @see \Drupal\Component\Utility\UrlHelper::filterBadProtocol()
+  // \Drupal\Component\Utility\UrlHelper::stripDangerousProtocols() will replace
+  // any bad protocols with HTTP, so we need to support the identical list.
+  // While '//' is technically optional for MAILTO only, we cannot cleanly
+  // differ between protocols here without hard-coding MAILTO, so '//' is
+  // optional for all protocols.
+  // @see \Drupal\Component\Utility\UrlHelper::stripDangerousProtocols()
   $protocols = \Drupal::getContainer()->getParameter('filter_protocols');
   $protocols = implode(':(?://)?|', $protocols) . ':(?://)?';
 

core/modules/search/search.pages.inc

@@ -5,6 +5,7 @@
  * User page callbacks for the Search module.
  */
 
+use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\Language\LanguageInterface;
 
 /**
@@ -34,7 +35,7 @@ function template_preprocess_search_result(&$variables) {
   $language_interface = \Drupal::languageManager()->getCurrentLanguage();
 
   $result = $variables['result'];
-  $variables['url'] = check_url($result['link']);
+  $variables['url'] = UrlHelper::stripDangerousProtocols($result['link']);
   $variables['title'] = $result['title'];
   if (isset($result['language']) && $result['language'] != $language_interface->getId() && $result['language'] != LanguageInterface::LANGCODE_NOT_SPECIFIED) {
     $variables['title_attributes']['lang'] = $result['language'];

core/modules/system/src/Tests/Common/UrlTest.php

@@ -33,14 +33,14 @@ function testLinkXSS() {
     // Test \Drupal::l().
     $text = $this->randomMachineName();
     $path = "<SCRIPT>alert('XSS')</SCRIPT>";
+    $encoded_path = "3CSCRIPT%3Ealert%28%27XSS%27%29%3C/SCRIPT%3E";
+
     $link = \Drupal::l($text, Url::fromUserInput('/' . $path));
-    $sanitized_path = check_url(Url::fromUri('base:' . $path)->toString());
-    $this->assertTrue(strpos($link, $sanitized_path) !== FALSE, format_string('XSS attack @path was filtered by _l().', array('@path' => $path)));
+    $this->assertTrue(strpos($link, $encoded_path) !== FALSE && strpos($link, $path) === FALSE, format_string('XSS attack @path was filtered by _l().', array('@path' => $path)));
 
     // Test \Drupal\Core\Url.
     $link = Url::fromUri('base:' . $path)->toString();
-    $sanitized_path = check_url(Url::fromUri('base:' . $path)->toString());
-    $this->assertTrue(strpos($link, $sanitized_path) !== FALSE, format_string('XSS attack @path was filtered by #theme', ['@path' => $path]));
+    $this->assertTrue(strpos($link, $encoded_path) !== FALSE && strpos($link, $path) === FALSE, format_string('XSS attack @path was filtered by #theme', ['@path' => $path]));
   }
 
   /**

core/modules/system/src/Tests/Common/XssUnitTest.php

@@ -56,6 +56,8 @@ function testBadProtocolStripping() {
     $expected_plain = 'http://www.example.com/?x=1&y=2';
     $expected_html = 'http://www.example.com/?x=1&y=2';
     $this->assertIdentical(check_url($url), $expected_html, 'check_url() filters a URL and encodes it for HTML.');
-    $this->assertIdentical(UrlHelper::stripDangerousProtocols($url), $expected_plain, '\Drupal\Component\Utility\Url::stripDangerousProtocols() filters a URL and returns plain text.');
+    $this->assertIdentical(UrlHelper::filterBadProtocol($url), $expected_html, '\Drupal\Component\Utility\UrlHelper::filterBadProtocol() filters a URL and encodes it for HTML.');
+    $this->assertIdentical(UrlHelper::stripDangerousProtocols($url), $expected_plain, '\Drupal\Component\Utility\UrlHelper::stripDangerousProtocols() filters a URL and returns plain text.');
+
   }
 }

core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php

@@ -380,8 +380,10 @@ public static function providerTestIsExternal() {
    */
   public function testFilterBadProtocol($uri, $expected, $protocols) {
     UrlHelper::setAllowedProtocols($protocols);
-    $filtered = UrlHelper::filterBadProtocol($uri);
-    $this->assertEquals($expected, $filtered);
+    $this->assertEquals($expected, UrlHelper::filterBadProtocol($uri));
+    // Multiple calls to UrlHelper::filterBadProtocol() do not cause double
+    // escaping.
+    $this->assertEquals($expected, UrlHelper::filterBadProtocol(UrlHelper::filterBadProtocol($uri)));
   }
 
   /**