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.

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

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

Merged
merged 2 commits into from Sep 14, 2018

Conversation

Projects
None yet
4 participants
@notnownikki
Member

notnownikki commented Sep 13, 2018

Description

The Gallery block uses data-id and data-link on the images,
and for non-admin users, kses strips these out, which makes Gutenberg
error when loading a post with a Gallery that's been authored by a non-admin
user.

This PR adds a filter function that adds the attributes we need.

How has this been tested?

Log in with a user with the author role.
Make a new post, add a gallery block.
Save the post.
Reload the editor.

The gallery block should load and be editable as expected.

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
if ( isset( $tags['img'] ) ) {
$tags['img']['data-link'] = true;
$tags['img']['data-id'] = true;
}

This comment has been minimized.

@mcsf

mcsf Sep 13, 2018

Contributor

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?

@mcsf

mcsf Sep 13, 2018

Contributor

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?

This comment has been minimized.

@notnownikki

notnownikki Sep 13, 2018

Member

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...

@notnownikki

notnownikki Sep 13, 2018

Member

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...

This comment has been minimized.

@mcsf

mcsf Sep 13, 2018

Contributor

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.

@mcsf

mcsf Sep 13, 2018

Contributor

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.

This comment has been minimized.

@aduth

aduth Sep 13, 2018

Member

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.

@aduth

aduth Sep 13, 2018

Member

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.

This comment has been minimized.

@pento

pento Sep 14, 2018

Member

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.

@pento

pento Sep 14, 2018

Member

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.

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Sep 14, 2018

Member

@notnownikki: For testing this, it'd be good to have a test that ran over each *.serialized.html file in the /test/integration/full-content/fixtures directory, and ran it through wp_unslash( wp_filter_post_kses( wp_slash( $html ) ) ), to check that other attributes aren't being removed.

class-parsing-test.php has similar functionality that you could copy/pasta. 😉

Member

pento commented Sep 14, 2018

@notnownikki: For testing this, it'd be good to have a test that ran over each *.serialized.html file in the /test/integration/full-content/fixtures directory, and ran it through wp_unslash( wp_filter_post_kses( wp_slash( $html ) ) ), to check that other attributes aren't being removed.

class-parsing-test.php has similar functionality that you could copy/pasta. 😉

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 14, 2018

Member

@pento that sounds like a good idea. Will get that added to this PR!

Member

notnownikki commented Sep 14, 2018

@pento that sounds like a good idea. Will get that added to this PR!

@mcsf mcsf added this to the 3.9 milestone Sep 14, 2018

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 14, 2018

Member

@pento ok, I wrote the test, and we have other failures. I'd like to get this in to 3.9 and address the other failures in a different PR. Some of the failures are just simple spacing fails, but some are aria attributes that kses is stripping out, which I don't think affects the block loading/saving functionality like it does with the gallery, but will take more time to go through and fix.

Member

notnownikki commented Sep 14, 2018

@pento ok, I wrote the test, and we have other failures. I'd like to get this in to 3.9 and address the other failures in a different PR. Some of the failures are just simple spacing fails, but some are aria attributes that kses is stripping out, which I don't think affects the block loading/saving functionality like it does with the gallery, but will take more time to go through and fix.

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Sep 14, 2018

Contributor

@notnownikki: that seems fair, given the circumstances. Seems like we've uncovered something bigger, especially considering a11y in WordPress. :)

As for the spacing, that might be OK: we already have some clever diffing with isEquivalentHTML et al., we could expand on that.

Contributor

mcsf commented Sep 14, 2018

@notnownikki: that seems fair, given the circumstances. Seems like we've uncovered something bigger, especially considering a11y in WordPress. :)

As for the spacing, that might be OK: we already have some clever diffing with isEquivalentHTML et al., we could expand on that.

@mcsf

mcsf approved these changes Sep 14, 2018

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 14, 2018

Member

@mcsf fantastic, I'm working on the new test now, there are a few elements getting stripped out that affect bits of functionality. PR coming when they're fixed!

Member

notnownikki commented Sep 14, 2018

@mcsf fantastic, I'm working on the new test now, there are a few elements getting stripped out that affect bits of functionality. PR coming when they're fixed!

@notnownikki notnownikki merged commit 3f1ba08 into master Sep 14, 2018

2 checks passed

codecov/project 49.04% remains the same compared to 02d530a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mtias mtias deleted the fix/gallery-with-author-role branch Sep 14, 2018

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Sep 14, 2018

Member

I was a little curious about whether the test would uncover similar issues. 😉

Thank you for diving into this, @notnownikki!

Member

pento commented Sep 14, 2018

I was a little curious about whether the test would uncover similar issues. 😉

Thank you for diving into this, @notnownikki!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment