UrlHelper::filterBadProtocol() really ought not to be setting safe strings - it's only used in Xss::attributes() - which should not be marking anything as safe.

UrlHelper::filterBadProtocol() has existing test coverage in Drupal\Tests\Component\Utility\UrlHelperTest

Filing this in case #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping does not land.

This is a major bug because Xss filter should not lead to any text being marked as safe. It's major because it is very unexpected given the recent work in #2506195: Remove SafeMarkup::set() from Xss::filter()

CommentFileSizeAuthor
#8 2546232.8.patch620 bytesalexpott
#2 2546232.2.patch771 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Category: Task » Bug report
Issue summary: View changes
Status: Active » Needs review
FileSize
2546232.2.patch771 bytes

Here's a patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Related issues: +#2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Should this be blocked on #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping?

+++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
@@ -271,8 +271,8 @@ public static function externalIsLocal($url, $base_url) {
-    $string = Html::decodeEntities($string);
...
+    $string = html_entity_decode($string, ENT_QUOTES, 'UTF-8');

These two lines are identical functionality-wise.

I'm assuming you're doing this solely to make the symmetry more apparent?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Well @xjm and others think #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping is wrong and tbh fixing this is more important.

Yep I think making the symmetry obvious makes reading the method simpler.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Postponed

Sorry I'm postponing this on #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping, both wrappers are useful.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Postponed » Needs review
FileSize
2546232.8.patch620 bytes

Used Html::escape() not that it is not the exact opposite of Html::decodeEntities() but we have good test coverage around Xss and UrlHelper and this is the expected behaviour.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

  • catch committed 2016469 on 8.0.x 
    Issue #2546232 by alexpott: Remove SafeMarkup::checkPlain() from...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.