Problem/Motivation

HTML5 custom data attribute names containing numerals are getting mangled by \Drupal\Component\Utility\Xss::attributes(). This occurs in Xss::attributes() because the regex pattern in $mode 0 does not match numerals.

Reproduce by running <a data-a2a-url="foo"></a> through Xss::filter:
print_r(\Drupal\Component\Utility\Xss::filter('<a data-a2a-url="foo"></a>'));

The data attribute name is partly stripped from the first numeral:
<a a-url="foo"></a>

Proposed resolution

In attribute names, permit numerals after the first character, because the HTML5 specification permits numerals in custom data attribute names, such as data-j2-range (an example from the spec).

Remaining tasks

  • Patch needed
  • Review needed

User interface changes

None.

API changes

None.

Comments

micropat created an issue. See original summary.

micropat’s picture

Status: Needs work » Needs review
StatusFileSize
new1008 bytes
micropat’s picture

Issue summary: View changes
dawehner’s picture

@micropat It would be great if you could expand the test coverage on \Drupal\Tests\Component\Utility\XssTest for that.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Yes we should have some tests. Thanks!

cilefen’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new597 bytes
new1.57 KB

The last submitted patch, 6: xss_attributes-2609928-6-TEST.patch, failed testing.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

What's the upstream bugfix needed? Patch is ready to go I'd say but not sure when it can get in…

The last submitted patch, 6: xss_attributes-2609928-6-TEST.patch, failed testing.

micropat’s picture

Issue summary: View changes
Issue tags: -Needs upstream bugfix +Contributed project blocker

Fixed my tag (oops). This issue is at least blocking the AddToAny contrib module. Anyone else think the patch qualifies for rc target triage?

chx’s picture

Status: Reviewed & tested by the community » Needs work

I am sorry but this is a classic example of a regex where the backtracking will break down horribly because there are so many ways this can match. You wanted [-a-zA-Z][-a-zA-Z0-9]*.

dawehner’s picture

http://www.regular-expressions.info/catastrophic.html explains this a bit better, thank you @chx for the review + pointing to the URL.

chx’s picture

Well it's not that bad because this is only O(n^2) as there's only one loop but still: the simpler regexp is just O(n).

star-szr’s picture

Excellent point, I agree!

micropat’s picture

Status: Needs work » Needs review
StatusFileSize
new1.57 KB
new1.01 KB

Thanks @chx!

chx’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: drupal-xss-attributes-2609928-15.patch, failed testing.

star-szr’s picture

Status: Needs work » Reviewed & tested by the community

Bot

  • alexpott committed ea1ec54 on 8.1.x
    Issue #2609928 by micropat, cilefen, Cottser, chx: Xss::attributes()...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Based on @xjm's post release triage document, committed 3e28af5 and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed 3e28af5 on 8.0.x
    Issue #2609928 by micropat, cilefen, Cottser, chx: Xss::attributes()...

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

This seems like it should be backported to Drupal 7 - I created #2847553: XSS attribute handling mangles valid attribute names containing numbers (D7 backport) to do that.