Extend allowed attributes for non-admin users

[notnownikki]

Description

Adds a test and alters allowed kses attributes so that users with the author role can save the current blocks without having attributes removed.

How has this been tested?

New test runs the serialized fixtures through kses and makes sure the resulting HTML is equivalent.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[aduth]

How do we scale this?

• For attributes we include, how do we know by which blocks they are required, such that we can maintain the list as revisions are made to those blocks?
• If these extensions are to be expected, should it be the responsibility of the block's registration itself to add to the allowed attribute set?
• If we're saying it's okay to override the default sanitization for these attributes, is this something we should consider loosening at a framework level instead?
• If there's a reason they're sanitized in the first place, are we not opening ourselves to potential vulnerabilities by allowing these for blocks (since there aren't really any checks to validate whether the block markup is editor-produced)

[notnownikki]

Thinking out loud...

For attributes we include, how do we know by which blocks they are required, such that we can maintain the list as revisions are made to those blocks?

Would a comment on each attribute be enough?

If these extensions are to be expected, should it be the responsibility of the block's registration itself to add to the allowed attribute set?

Would that mean for blocks that need to extend things this way, there would have to be a server side and client side registration for the block? Perhaps if a plugin registers blocks that need extra attributes on the allow list, then it's up to the plugin to add the filter.

If we're saying it's okay to override the default sanitization for these attributes, is this something we should consider loosening at a framework level instead?

That's my, perhaps controversial, opinion, yes.

kses excludes everything and then includes only what we say is safe. Given that download isn't there for links, and there are aria related attributes missing, that suggests to me that kses is out of date and needs updating for an editor that has these capabilities built in.

If there's a reason they're sanitized in the first place, are we not opening ourselves to potential vulnerabilities by allowing these for blocks (since there aren't really any checks to validate whether the block markup is editor-produced)

For the cover image, where kses removed any CSS that has ( and could be used to sneak in tracking images and other nasties, yes that's a risk. However, my opinion is that kses allow list is out of date and that data-*, download, and aria-* are modern attributes that we've not seen problems with before because we haven't had an editor that does them in the same way Gutenberg does.

[pento]

However, my opinion is that kses allow list is out of date and that data-*, download, and aria-* are modern attributes that we've not seen problems with before because we haven't had an editor that does them in the same way Gutenberg does.

This is correct. KSES will need to be updated in Core to add these attributes, they haven't been added previously because there wasn't a big demand for them.

There are no security issues with any of these attributes that I'm aware of.

[aduth]

The function name is not very obvious to its purpose. Doesn't our PHPCS setup force function documentation? Might be good in this instance.

[aduth]

Something the previous implementation had accounted for which this does not is if a developer calls to unset( $tags['a'] ); in their own filter handler before this one is called. Here, I think an error may occur.

[notnownikki]

@pento do you think this is needed now? Or are these changes going directly into core?

[pento]

I don't think we need this PR any more. The required aria- attributes, as well as support for data-*, have already been added to Core. download is currently pending the outcome of #10693, and url() support in style attributes is pending a final review of WP#45067.

[notnownikki]

great, thanks @pento I'll close this now.

[notnownikki]

thanks @pento I'll close this now.