diff --git a/core/includes/common.inc b/core/includes/common.inc index 8c1cfad..da14330 100644 --- a/core/includes/common.inc +++ b/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)); } /** diff --git a/core/includes/install.core.inc b/core/includes/install.core.inc index 3b4c4ab..b5cc151 100644 --- a/core/includes/install.core.inc +++ b/core/includes/install.core.inc @@ -2042,7 +2042,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. Choose a different language 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. Choose a different language or select English and translate your website later.', array('%language' => $language, '@url' => UrlHelper::stripDangerousProtocols($_SERVER['SCRIPT_NAME']))), ); } else { @@ -2061,7 +2061,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. Choose a different language 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. Choose a different language or select English and translate your website later.', array('%language' => $language, '@url' => UrlHelper::stripDangerousProtocols($_SERVER['SCRIPT_NAME']))), ); } } @@ -2271,11 +2271,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 retry, or you may choose to continue anyway.', array('!retry' => check_url(drupal_requirements_url(REQUIREMENT_ERROR)), '!cont' => check_url(drupal_requirements_url($severity)))); + $build['#suffix'] = t('Check the messages and retry, or you may choose to continue anyway.', 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 try again.', array('!url' => check_url(drupal_requirements_url($severity)))); + $build['#suffix'] = t('Check the messages and try again.', array('@url' => UrlHelper::stripDangerousProtocols(drupal_requirements_url($severity)))); } return $build; } diff --git a/core/includes/install.inc b/core/includes/install.inc index f021373..c885553 100644 --- a/core/includes/install.inc +++ b/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(); diff --git a/core/lib/Drupal/Component/Utility/UrlHelper.php b/core/lib/Drupal/Component/Utility/UrlHelper.php index 608a5da..b442c57 100644 --- a/core/lib/Drupal/Component/Utility/UrlHelper.php +++ b/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. diff --git a/core/modules/aggregator/aggregator.theme.inc b/core/modules/aggregator/aggregator.theme.inc index ab6c3dc..a8f7060 100644 --- a/core/modules/aggregator/aggregator.theme.inc +++ b/core/modules/aggregator/aggregator.theme.inc @@ -6,6 +6,7 @@ */ use Drupal\Component\Utility\SafeMarkup; +use Drupal\Component\Utility\UrlHelper; use Drupal\Core\Render\Element; /** @@ -25,7 +26,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'] = SafeMarkup::checkPlain($item->label()); } diff --git a/core/modules/comment/comment.tokens.inc b/core/modules/comment/comment.tokens.inc index 5b11466..500533e 100644 --- a/core/modules/comment/comment.tokens.inc +++ b/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': diff --git a/core/modules/comment/src/Tests/CommentTokenReplaceTest.php b/core/modules/comment/src/Tests/CommentTokenReplaceTest.php index 0267346..0bc6be6 100644 --- a/core/modules/comment/src/Tests/CommentTokenReplaceTest.php +++ b/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()); diff --git a/core/modules/filter/filter.module b/core/modules/filter/filter.module index 86a6ef8..d58054c 100644 --- a/core/modules/filter/filter.module +++ b/core/modules/filter/filter.module @@ -496,11 +496,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) . ':(?://)?'; diff --git a/core/modules/search/search.pages.inc b/core/modules/search/search.pages.inc index b50a6cd..da25b7a 100644 --- a/core/modules/search/search.pages.inc +++ b/core/modules/search/search.pages.inc @@ -6,6 +6,7 @@ */ use Drupal\Component\Utility\SafeMarkup; +use Drupal\Component\Utility\UrlHelper; use Drupal\Core\Language\LanguageInterface; /** @@ -35,7 +36,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'] = SafeMarkup::checkPlain($result['title']); if (isset($result['language']) && $result['language'] != $language_interface->getId() && $result['language'] != LanguageInterface::LANGCODE_NOT_SPECIFIED) { $variables['title_attributes']['lang'] = $result['language']; diff --git a/core/modules/system/src/Tests/Common/UrlTest.php b/core/modules/system/src/Tests/Common/UrlTest.php index e597139..7584c10 100644 --- a/core/modules/system/src/Tests/Common/UrlTest.php +++ b/core/modules/system/src/Tests/Common/UrlTest.php @@ -33,14 +33,14 @@ function testLinkXSS() { // Test \Drupal::l(). $text = $this->randomMachineName(); $path = ""; + $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])); } /** diff --git a/core/modules/system/src/Tests/Common/XssUnitTest.php b/core/modules/system/src/Tests/Common/XssUnitTest.php index f1358e0..2890f55 100644 --- a/core/modules/system/src/Tests/Common/XssUnitTest.php +++ b/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.'); + } } diff --git a/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php b/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php index 6823dc1..761fa69 100644 --- a/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php +++ b/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))); } /**