wp_kses_attr_check fails to process html data-* attributes

[isoftware]

wp_kses_attr_check does not take into account data attributes and retuns false on valid elements.

You can reproduce this issue with shortcodes in data attributes using the following scenario
<a data-trigger="[button-trigger]" href="#">[button-name]</>

[ocean90]

It depends on the unfiltered_html capability which only Administrators and Editors have by default. The data- attributes are not whitelisted.

[isoftware]

Thank you for your swift repsonse.

I have the following shortcode on a site that has only one admin user so all content is created from admin:

...
<a data-slide-index="[counter-increment output='true']" href="#"></a>
...

On debug I can see that the counter-increment shortcode is returning the correct integer value but after the wp_kses_attr_check returns false, wp_kses_one_attr returns empty string and the returned shortcode content is :

...
<a href="#"></a>
...

[isoftware]

In addition I could not find a way to generically whitelist the data-attributes. One needs to hook into the wp_kses_allowed_html filter to register explicitly data attributes for specific elements which seems quite complicated for the shortcodes example.

[miqrogroove]

Please see the explanation at ​https://make.wordpress.org/core/2015/07/23/changes-to-the-shortcode-api/

[isoftware]

Replying to miqrogroove:

Please see the explanation at ​https://make.wordpress.org/core/2015/07/23/changes-to-the-shortcode-api/

Please note that in the link supplied there is no mention of html data attributes beeing excluded and there should be a mention somewhere in the documentation that they are considered unsafe.

[sswells]

Can we please reopen this? This looks like a bug to me, and not explained by the API changes. For example, if I use something like this:
<a href="[the-link]" data-trigger="[button-trigger]">[button-name]</a>

Then the shortcode inside the data attribute is not longer processed, but the others are. The shortcodes should be processed inside a data attribute, just as they are inside an href.

[miqrogroove]

At the 9 September meeting, participants agreed to close this ticket because fixing it would require a reversion of a previous security patch and would involve generally unwanted changes to the Shortcode API.

For details, please see ​https://make.wordpress.org/core/2015/09/29/shortcode-roadmap-draft-two/

[pento]

Reopening, as data-* attributes will need to be allowed by KSES for several default Gutenberg blocks.

[azaozz]

In 33121.diff:

• Allow data-* attributes. Needs to be set as data-* when adding it, not to be mixed with the HTML 4.0 data attribute (​https://www.w3.org/TR/html40/struct/objects.html#adef-data).
• Add data-* to all elements by default.

Wondering if we should pass the restrictions (array of possible values) or just short circuit as soon as we match a data-* attribute. The thing is that the restrictions will apply to all data attributes in the element, not to a specific attribute.

[azaozz]

In 33121.2.diff: also, do not override a specifically set data-something attribute.

[peterwilsoncc]

In 33121.3.diff​ I've taken a quite different approach:

• Removed hard coding of data- prefix, any wildcard can be added in _wp_add_global_attributes().
• Wildcard checks are only run if the full attribute doesn't exist in $allowed_attr
• Added in a couple of unit tests
• Regardless of approach, test_wp_kses_attr_data_attribute_is_allowed() should be a valid test.

[azaozz]

Looking at 33121.3.diff, don't think it is a good idea to allow adding attributes with a wildcard. (Note that _wp_add_global_attributes() is just a convenience function to add some attribute names to all tags. There is no way to check how an attribute name was added. So plugins will also be able to add the data-* attribute name which is the expected behavior.)

That would mean --somebody-- can add on-* or even o-* and allow all onerror, onclick, onmouseover, etc. attributes.

What's worse there is no way to sanitize the values of these wildcard attributes (this part in the patch $allowed_attr[$name_low] = true;), so things like onerror="alert(document.cookie)" become very possible and not immediately recognizable.

KSES already has support for allowing any attribute on any tag: adding it by full name. So if somebody wants to add onerror for images, they can easily do that and specify a list of acceptable values. Why would we need to bypass that with a wildcard and reduce functionality (not possible to check/sanitize attribute values)?

The only exception to the above are attributes with variable names, and the only HTML 5.0 attribute with a variable name is data-*. Thinking we should extend KSES to allow data attributes, but hardcode the data- part. This is inline with the existing logic in KSES.

Uh, sorry for the longer comment :) The TL;DR: don't think allowing wildcard attributes in KSES is a good thing. It brings us to a pretty dangerous place and at the same time reduces some of the existing functionality: sanitizing attribute values.

[azaozz]

@peterwilsoncc thanks for adding the test :)

Looking at data--invaild="gone" and data-also-invaild-="gone", it seems having two hyphens or a hyphen as last char of the data-* attribute name is valid per ​https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/data-* and ​https://www.w3.org/TR/REC-xml/#NT-Name. Also seems quite a few chars are valid there, but still thinking we should only support a-z0-9_-.

Also preg_match( '/^' . preg_quote( $prefix ) . '(-[a-z0-9_]+)*$/', $name_low ) would mean we don't allow attribute manes containing two hyphens like data-wp-id (which is somewhat common).

[peterwilsoncc]

tl;dr in 33121.4.diff​:

• Test added to ensure prefix is followed by a hyphen
• Test added to ensure attributes with multiple hyphens allowed
• Regex altered to require one or more instance of (-[a-z0-9_]+), ie '/^' . preg_quote( $prefix ) . '(-[a-z0-9_]+)+$/'

Replying to azaozz:

@peterwilsoncc thanks for adding the test :)

And thanks for the review.

Looking at data--invaild="gone" and data-also-invaild-="gone", it seems having two hyphens or a hyphen as last char of the data-* attribute name is valid per ​https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/data-* and ​https://www.w3.org/TR/REC-xml/#NT-Name. Also seems quite a few chars are valid there, but still thinking we should only support a-z0-9_-.

This is true but it does some strange things to the element.dataset property available in JavaScript so I decided to prevent it. I've created a bin with an example ​https://jsbin.com/muloxeq/edit?html,js,console,output

I'm happy to change this if needs be.

The TL;DR: don't think allowing wildcard attributes in KSES is a good thing. It brings us to a pretty dangerous place and at the same time reduces some of the existing functionality: sanitizing attribute values.

I'm not sure this is the case if we require the hyphen following any prefixes, so a developer won't be able to add href-* and bypass checking. The regex change I mention below hardens against this.

I'm also prepared to be misunderstanding something, so are you able to let me know if that's the case.

That would mean --somebody-- can add on-* or even o-* and allow all onerror, onclick, onmouseover, etc. attributes.

This isn't the case as the hyphen is required before any characters in the regex group (-[a-z0-9_]+).

As it's important to block, I've added a test in 33121.4.diff​ to ensure against it.

Also preg_match( '/^' . preg_quote( $prefix ) . '(-[a-z0-9_]+)*$/', $name_low ) would mean we don't allow attribute names containing two hyphens like data-wp-id (which is somewhat common).

This is also incorrect, I've added such an example as a test in 33121.4.diff​.

However, the zero or more regex (*) did allow users to add data="something", so I've changed that in the latest patch to be one or more (+).

[azaozz]

Replying to peterwilsoncc:

This (two hyphens or end hyphen) is true but it does some strange things to the element.dataset property available in JavaScript

Good point. Lets not allow it :)

I'm not sure this (overriding of unset attributes) is the case if we require the hyphen following any prefixes, so a developer won't be able to add href-* and bypass checking.

Ah, I see, my error. Now the regex matches only attribute names that have a hyphen.

I'm still not sold on supporting all of these attribute names "globally" with no possibility to check the values. Except data-*, none of them have variable names and imho should be added the same way like the rest of the attributes are added: by full name.

In any case, what is the purpose of allowing all attributes with a hyphen?

• It would support data-* which we should have.
• It would also support aria-* which would be nice to have. There are about 45 aria attributes and many of them have a list of possible values, like:

      aria-busy ( true | false ) 'false'
      aria-checked ( true | false | mixed | undefined ) 'undefined'
      aria-disabled ( true | false ) 'false'
      aria-expanded ( true | false | undefined ) 'undefined'
      aria-grabbed ( true | false | undefined ) 'undefined'
      aria-hidden ( true | false ) 'false'
      aria-invalid ( grammar | false | spelling | true ) 'false'
      aria-pressed ( true | false | mixed | undefined ) 'undefined'
      aria-selected ( true | false | undefined ) 'undefined'
  

(see ​http://www.w3.org/MarkUp/DTD/aria-attributes-1.mod)

For a security/sanitizing function I'd expect all of these to be hard-coded, both the attribute name and possible values, after a security audit of whether any of the values may be misused. Again, this is inline with how KSES works.

Apart from data-* and aria-* there are accept-charset and http-equiv (see ​https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes). Don't think it makes sense supporting these.

Am I missing something? Are there other user cases for allowing attributes with a hyphen and not support checking of their values?

[azaozz]

In 33121.5.diff:

• Based on 33121.2.diff.
• Fix the regex so it doesn't allow trailing hyphen or two consecutive hyphens.
• Add the tests.

All tests from 33121.4.diff pass except the array( '33121-*', true ), changed that to array( 'data-*', true ), :)

[azaozz]

Thinking more about this, a "middle point" between the two approaches would be to tweak _wp_add_global_attributes() so it can return just the array of $global_attributes. Then check the prefix against it. That way only hard-coded wildcard attributes will be allowed. (That would still require sanity checking/security auditing of all aria-* attributes if we decide to add them.)

[peterwilsoncc]

I like the mixed approach in 33121.5.diff​.

The main thing I was trying to avoid was doubling up on adding the data-* prefix in wp_kses_attr_check() and _wp_add_global_attributes().

It occurs to me that in your patches the prefix doesn't need to be added to _wp_add_global_attributes(). If core needs multiple allowed prefixes in the future we can create a new private function, until then we can use your regex.

[peterwilsoncc]

33121.6.diff​ is based on the previous patch with:

• multiline comment format changed
• @since added to docblocks
• Modified test_wildcard_attribute_prefixes to try variations of data- prefix to validate

Following my last comment I could see why you included data-* in two locations. Let's go with the compromise version.

[pento]

33121.6.diff​ looks good, let's party!

[peterwilsoncc]

I call 33121.7.diff​ a valuable reminder I should have run the full test suite three days ago ;)

• alters some REST API tests ensuring API requests are run through kses for users without unfiltered_html caps.

[peterwilsoncc]

In 43727:

KSES: Allow HTML data-* attributes.

Add global support for HTML attributes prefixed data- for authors and contributors, as required by the new editor.

Props azaozz, peterwilsoncc.
Fixes #33121.

[peterwilsoncc]

Reopened for trunk.

[jeremyfelt]

In 43981:

KSES: Allow HTML data-* attributes.

Add global support for HTML attributes prefixed data- for authors and contributors, as required by the new editor.

Merges [43727] to trunk.

Props azaozz, peterwilsoncc.
Fixes #33121.

[yura1980]

Replying to jeremyfelt:

In 43981:

KSES: Allow HTML data-* attributes.

Add global support for HTML attributes prefixed data- for authors and contributors, as required by the new editor.

Merges [43727] to trunk.

Props azaozz, peterwilsoncc.
Fixes #33121.

I'm trying to modify HTML markup inside WooCommerce Gutenberg block using this filter:

woocommerce_blocks_product_grid_item_html

wp_kses and wp_kses_post functions stripping out 'add to cart' button's data attributes with underscore sign:

data-product_id
data-product_sku

I'm wondering can I prevent stripping these attributes by wp_kses function?