String stripped from title and alt attribute if contains a colon

I guess this is the right component.

Sorry, but it's too late and we have much more important issues to deal with right now. A fix for this may be backported from D8.

Any word on the status of this? Just wondering how it sits in the queue. We're still seeing this behavior in 6.22 - but for the alt attribute.

Example Input:
<img alt="annen:ascj" src="/sites/default/files/AnnenbergA_75p.jpg" width="75" height="75" />

Example Output:
Example:
<img alt="ascj" src="/sites/default/files/AnnenbergA_75p.jpg" width="75" height="75" />

Thanks!
Chris

yeah, there should be a whitelist of attributes for which this check is unnecessary I suppose :x

possible related: #1328768: attributes 'xml:lang' and 'xml:id' transform to 'lang' and 'id' in filter_xss

Based on the initial comment, I added a condition to check for attributes that include the colon as part of their name. After this, the stated test cases worked as expected.

This happens when namespaced attributes are being used.

Hello, will a fix be backported to D7? There are quite common use cases for a colon in the title attribute, e.g. for adding copyright information to images.

Unless I am being clueless and misreading the patch at #9, it addresses the issue of attribute names with colons in them (which might help with e.g. #1328768: attributes 'xml:lang' and 'xml:id' transform to 'lang' and 'id' in filter_xss) but doesn't address attribute values with colons in them, which is the topic of this issue.

It does look as though Drupal's filter.module merrily calls filter_xss_bad_protocol() on every attribute value it ever sees, regardless of the attribute name or the tag type. The good thing about this is that it's maximally paranoid! The bad thing is that it zaps all colons from any attribute value, ever, unless the text before the colon just happens to match a whitelisted URI protocol.

One could fix this by whitelisting certain attribute names, like "title" and "alt" and... (any other suggestions?) Or one could do that more conservatively by whitelisting certain tag/attribute pairs - but that approach would appear to require more extensive refactoring in filter.module.

In order to reproduce the bug, make sure that your "text format" is using "Limit allowed HTML tags " filter.

#12 is not working any more, debugging.

One could fix this by whitelisting certain attribute names, like "title" and "alt" and... (any other suggestions?) Or one could do that more conservatively by whitelisting certain tag/attribute pairs - but that approach would appear to require more extensive refactoring in filter.module.

This filtering should only be done on attributes that can actually be a URL. A whitelist of attributes that are plaintext is probaby a good way to do this.

Whitelist from https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Preventio...

Safe HTML Attributes include: align, alink, alt, bgcolor, border, cellpadding, cellspacing, class, color, cols, colspan, coords, dir, face, height, hspace, ismap, lang, marginheight, marginwidth, multiple, nohref, noresize, noshade, nowrap, ref, rel, rev, rows, rowspan, scrolling, shape, span, summary, tabindex, title, usemap, valign, value, vlink, vspace, width

I didn't know value was safe though

Here is a patch that (kinda) works. There is one problem it doesn't read the changes in the YAML file. I cannot figure out why. YAML files are still new to me. It works because alt and title are add to the array if the configuration information isn't found.

UPDATE: #1998466: Convert filter_xss_admin and similar function to an Xss component just got committed and broke this patch.

Ok I have fixed the patch to work with the converted Xss.php class file. It still doesn't load the config in _drupal_code_bootstrap() and I don't know why.

Can anyone explain why?

+++ b/core/includes/common.incundefined
@@ -4349,6 +4349,15 @@ function _drupal_bootstrap_code() {
+  $allowed_attributes = \Drupal::config('system.filter')->get('attributes');

no need for \ before Drupal class..we only enforce those in namespaced code

+++ b/core/lib/Drupal/Component/Utility/Xss.phpundefined
@@ -22,6 +22,23 @@ class Xss {
+   * ¶

@@ -224,8 +242,14 @@ protected static function attributes($attributes) {
+            } ¶

trailing space, that should be removed prior to commit

+++ b/core/lib/Drupal/Component/Utility/Xss.phpundefined
@@ -224,8 +242,14 @@ protected static function attributes($attributes) {
+              $thisval = $match[1];

@@ -236,8 +260,12 @@ protected static function attributes($attributes) {
+              $thisval = UrlValidator::filterBadProtocol($match[1]);
...
+              $thisval = $match[1];

@@ -247,7 +275,12 @@ protected static function attributes($attributes) {
+              $thisval = UrlValidator::filterBadProtocol($match[1]);
...
+              $thisval = $match[1];

$thisval variable should be more explanatory..maybe $filtered_variable..dunno

Ok config does load properly. I had to either replace core\modules\system\config\system.filter.yml in your sites/[mysite]/files/config_XXXXXXX directory or place core\modules\system\config\system.filter.yml into the staging directory and sync the configuration.

FYI this is a major issue as it breaks accessibility when there is a colon in the alt and title attribute

Patch updated

Changing to needs review

triggering testbot

also this is not major

#20: drupal-whitelist_attributes-952964-20.patch queued for re-testing.

#20: drupal-whitelist_attributes-952964-20.patch queued for re-testing.

#20: drupal-whitelist_attributes-952964-20.patch queued for re-testing.

#20: drupal-whitelist_attributes-952964-20.patch queued for re-testing.

reroll

no longer applies.

Updating patch with reroll. Please review.

Ran into:

PHP Fatal error: Class 'Drupal\Component\Utility\Url' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Component/Utility/Xss.php on line 268

This ultimately led me to #2191873: Document the WYSIWYG XSS filtering concept and architecture for developers surprised at how complicated this patch is to just allow a ":" in a title/alt text.

Reroll now correctly uses the UrlHelper class again, not sure why it was changed to Url

This implies FilterHtml::getHTMLRestrictions() must also be updated.

This exact same problem also occurs for data-* attributes. I didn't know about this issue until today, so I was trying to fix this same problem without whitelisting attributes over at #2105841: Xss filter() mangles image captions and title/alt/data attributes. However, this approach seems quite a bit simpler, so … could you please also add support for wildcard attributes, so we could allow data-*, which would match data-align, data-caption, data-foo …?

This exact same problem also occurs for data-* attributes

A very similar problem also appears in namespaced HTML tags: #2321061: Xss::split() fails on custom HTML elements with colons in the name

This works fine now since #2105841: Xss filter() mangles image captions and title/alt/data attributes got in so closing this one as duplicate.

Will that fix be backported to D7?

@Liam Morland, if this is to be backported, this issue is not the right place. Since this issue didn't had a patch, there's nothing to backport. You'll be better off asking this question on the original issue: #2105841: Xss filter() mangles image captions and title/alt/data attributes. Since its a security fix it should be ported to D7 too if it also has a similar vulnerability but I'm not sure about that.