Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
I get this issue during usage of Tailwind CSS framework. This framework allows to use classes with prefixes like md:mx-auto, lg:text-red-100. But Xss::filter() removes such values from the "class" attribute because process them as a protocol. I found a couple of existing issues, but all of them are not about how to add the "class" attributes to a whitelist.
Steps to reproduce
- configure Basic HTML filter
/admin/config/content/formats/manage/basic_html: add<div class="">to the Allowed HTML tags - create any node with HTML like that:
<div class="md:mx-auto lg:text-red-100">(Body with selected Basic HTML filter) - see that the
mdprefix is filtered and doesn't exist in page's HTML source
Proposed resolution
In the Drupal\Component\Utility\XSS::attributes() exists next code:
$skip_protocol_filtering = substr($attribute_name, 0, 5) === 'data-' || in_array($attribute_name, [
'title',
'alt',
'rel',
'property',
]);
I guess the "class" attribute doesn't affect on a site's security (correct me if I'm wrong). So the 1st option is to add 'class' attributes to the list. The 2nd option is to allow to expand this list - for example, like it's implemented for the filter_protocols in the default.services.yml.
Known workaround
According this comment (link) it's possible to add Tailwind CSS md, lg class prefixes as allowed protocols. And your class attribute value MUST start from the prefixed class to pass a protocol filtering. Because the class="m-0 md:text-red-100" is read by Drupal as a value with m-0 md: protocol instead of whitelisted md:.
Remaining tasks
tbd
User interface changes
n/a
API changes
n/a
Data model changes
tbd
Release notes snippet
tbd
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 3252587-6.patch | 1.35 KB | longwave |
| #6 | 3252587-6-test-only.patch | 934 bytes | longwave |
Comments
Comment #2
cilefen CreditAttribution: cilefen commentedIs this the same as #3050007: CSS identifiers created via `Html::cleanCssIdentifier` shouldn't strip colons?
Comment #3
larowlanComment #4
Chalk CreditAttribution: Chalk at DrupalSquad commented@cilefen it looks pretty similar, but I'm not sure that the
Html::cleanCssIdentifierwas in my call stack. In my case the root cause isXSS::attributes().Comment #5
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis is also causing issues with the uswds library which is being pushed on U.S. sites.
Comment #6
longwaveComment #8
smustgrave CreditAttribution: smustgrave at Mobomo commentedAre there any security implications to whitelisting the entire class attribute?
Comment #9
longwaveI don't think so, the security issues are around URLs (that could contain code such as the
javascript:URL scheme) and as far as I know theclassattribute is never parsed as if it were a URL. If this were extended to other attributes we would have to consider them individually but adding justclassis safe.Comment #10
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo following the code it does appear it sends the class through UrlHelper::filterBadProtocol which splits along ":" and see the left side as invalid protocal.
I believe you're right about safe to whitelist class I just don't know XSS stuff well enough to know for sure.
Your patch does work.
Comment #11
smustgrave CreditAttribution: smustgrave at Mobomo commentedActually I take my previous statement back because all you are doing is ignoring class for protocol checking. So I don't see any issue.
Comment #13
longwaveUnrelated fails, see #3267124: Temporarily skip failing tests
Back to RTBC.
Comment #15
longwaveMore unrelated fails, back to RTBC.
Comment #16
lauriiiThe class attribute cannot contain URI, meaning it should be fine to skip it from XSS perspective. I guess there's the very unlikely scenario that this could have an impact on someones custom JavaScript, meaning we should probably add a change record for this.
Comment #18
smustgrave CreditAttribution: smustgrave at Mobomo commentedI’ll work on the change record.
Comment #19
smustgrave CreditAttribution: smustgrave at Mobomo commentedAdded a change request here https://www.drupal.org/node/3292741
Comment #20
borisson_Change record looks to be complete, I have one nitpick about this sentence, would it be clearer to say that colons are allowed in class names (because as far as I can read the issue, this is only changed for classes?)
> This change will mark colons as allowed markup allowing such libraries to be used inside the ckeditor.
Comment #21
longwaveThanks for the CR. Agree with @borisson_ - I updated the CR a little to make it clearer that this affects classes only.
Comment #22
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis is big for those that have to use uswds so thank you!
Comment #27
lauriiiAsked for another set of review from @alexpott. He confirmed he checked https://html5sec.org/ and didn't find anything worrying.
Committed b6217bd and pushed to 10.1.x. Cherry-picked to 10.0.x and 9.5.x. Thanks!