Deprecate check_url and convert usages to existing UrlHelper methods

+++ b/core/includes/common.inc
@@ -281,10 +281,20 @@ function valid_email_address($mail) {
 function check_url($uri) {
-  return SafeMarkup::checkPlain(UrlHelper::stripDangerousProtocols($uri));
+  return Html::escape(UrlHelper::stripDangerousProtocols($uri));
 }
 

The docblock for this didn't typehint the @return (which was SafeStringInterface). Do we want to explicitly state that is is string now?

@mpdonadio probably should, but it wasn't SafeStringInterface it was a string before, just got saved on the SafeMarkup::$safeStrings list.

Reviewed the code and big +1 to this issue. I'd RTBC it now but I'd like to see if anybody else has any concerns since it's less than an hour old.

Thanks for the correction @joelpittet. Got things confused in my head with all of the different patches I am following and working on.

I keep getting turned around by us having Html::filterBadProtocols() and Html::stripDangerousProtocols() and why we need both.

I just skimmed through the original issue that added drupal_strip_dangerous_protocols() - #715142: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain(). We added that due to a double-escaping bug.

The reason we ended up with both functions as well as check_url() is outlined in

#715142-7: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain() - then #10, #13. #14 (and several more later). Boils down to wanting to keep a check_url() check in preprocess as definitely safe, vs. requiring an additional check_plain() around it. That issue nearly removed check_url() at the time too as well as the check_plain() from filter_xss_bad_protocol(), with 8.x and autoescaping the arguments against doing that no-longer hold.

Also checked and in 8.x HEAD, we only have one call to filterBadProtocols() outside of tests. This issue adds one more.

Can we:

Convert those couple of cases to stripDangerousProtocols() and add an Html::escape() around them if it's needed?

If that's doable, additionally deprecate UrlHelper::filterBadProtocols()?

At the moment I can't properly understand this comment, and just @see UrlHelper::stripDangerousProtocols() @see Html::escape() would be a lot easier:

+++ b/core/includes/common.inc
@@ -281,10 +281,20 @@ function valid_email_address($mail) {
+ * @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 an equivalent method apart from the fact
+ *   the it can be called multiple times on the same url without double
+ *   escaping. Note that this method does not mark it's output as safe.

#7,

Also checked and in 8.x HEAD, we only have one call to filterBadProtocols() outside of tests. This issue adds one more.

The latest patch on #2544110: XSS attribute filtering is inconsistent and strips valid attributes touches this section in Xss, so I think it may be in scope to remove it as part of that issue. Leave a comment on that issue if you want me to do it there.

@mpdonadio thanks for the issue link.

From that issue I also saw #2545056: Attribute class does not sanitize for "javascript:alert('XSS')" which is talking about filtering bad protocols automatically in Attribute. If we did that, a lot of the calls to stripDangerousProtocols() might not be needed either.

Let's open a separate issue for UrlHelper::filterBadProtocols() - 2 is better than 3 here. We could mark it @internal for example.

\m/

Added comments look good and the rest looks fairly straightforward.

Added a draft CR at https://www.drupal.org/node/2560027 -- or should we put this in one big CR along with some of the other SafeMarkup-related changes?

+++ b/core/includes/install.inc
@@ -864,9 +864,11 @@ function install_goto($path) {
+ *   through \Drupal\Component\Utility\UrlHelper::filterBadProtocol() if it will be

80 cols

Just wrote some self sanity comments so it looks RTBC for me.

1. +++ b/core/includes/install.core.inc
   @@ -2071,7 +2071,7 @@ function install_check_translations($langcode, $server_pattern) {
   -        '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']))),
   

   So t() will do the escaping and UrlHelper the stripping of the bad protocol

2. +++ b/core/modules/aggregator/aggregator.theme.inc
   @@ -24,7 +25,7 @@ function template_preprocess_aggregator_item(&$variables) {
   -  $variables['url'] = check_url($item->getLink());
   +  $variables['url'] = UrlHelper::stripDangerousProtocols($item->getLink());
   

   Also here twig will autoescape

+1, updated the CR with those examples

The deprecation message still feels a bit too 'helpful', I probably would've stopped at the first sentence. But can't really push back on it being to helpful so...

Committed/pushed to 8.0.x, thanks!