Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add needed attributes to kses allowed tags for the Gallery block #9875

Merged
merged 2 commits into from
Sep 14, 2018
Merged

Add needed attributes to kses allowed tags for the Gallery block #9875

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ba10a6
Add needed attributes to kses allowed tags for the Gallery block
notnownikki Sep 13, 2018
3072144
Lint and doc fix
notnownikki Sep 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,3 +475,20 @@ function gutenberg_add_admin_body_class( $classes ) {
return "$classes gutenberg-editor-page is-fullscreen-mode";
}
}

/**
* Adds attributes to kses allowed tags that aren't in the default list
* and that Gutenberg needs to save blocks such as the Gallery block.
*
* @param array $tags Allowed HTML.
* @return array (Maybe) modified allowed HTML.
*/
function gutenberg_kses_allowedtags( $tags ) {
if ( isset( $tags['img'] ) ) {
$tags['img']['data-link'] = true;
$tags['img']['data-id'] = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's too bad that we have to explicitly allow given data attributes, rather than accept all data-*. It begs the question: are we doing things wrong in Gallery? Is WordPress/KSES approaching this the wrong way with a blanket exclusion of data attributes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My opinion is that data-* should be allowed, and WordPress/KSES is doing it wrong when it comes to those attributes.

But others might know better...

Copy link
Contributor

@mcsf mcsf Sep 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can patch this specifically for Gallery, but if so we need a proper solution in the very near future. I also wonder if this a change that can be effected directly in core as part of 5.0 work. Pinging @pento, curious if you have thoughts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is WordPress/KSES approaching this the wrong way with a blanket exclusion of data attributes?

For that matter, I'm not totally sure what's being accomplished by stripping any attribute. Without considering its value, I can't see any security-related issue with an attribute presence alone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of anything off the top of my head, I suspect it's just something that hasn't been added: KSES is built on an allow list of individual attributes, adding a wildcard would require a little more care to implement.

I'm going to float it with the security team, but it's probably going to take some time to explore and create a patch, so let's just go with this PR to fix the immediate issue for now.

return $tags;
}

add_filter( 'wp_kses_allowed_html', 'gutenberg_kses_allowedtags', 10, 2 );