KSES: Allow PDFs to be embeded as objects

[pento]

​Gutenberg 10.5 added the ability to display PDFs as embeds, but made use of the <object> tag, which KSES doesn't allow.

It's certainly not feasible to allow <object> tags without limitations: while most of the original problematic uses of it are no longer supported in browsers (Java applets, ActiveX, Flash, etc), it would be challenging to prove that there are no potential security issues with allowing it for all object types.

Instead, this change allows the <object> tag only when it has a type attribute set to application/pdf.

Trac ticket: https://core.trac.wordpress.org/ticket/54261

Okay, I think this is pretty close. It could to with a solid review, including efforts to break it, given how important KSES is.

One particular behaviour I want to point out is the difference between self-closing tags, and non-self-closing tags. As KSES doesn't keep track of open/close tag pairs, it's not possible to reliably remove a tag and its innerHTML. So, rather than removing the opening tag (and leaving the closing tag), I think the safest option is to just strip all attributes from the opening tag.

eg:

<object type="application/exe" /> is removed entirely.
<object type="application/exe"></object> becomes <object></object>.

I'm not aware of any tags that could cause safety issues if _all_ attributes are removed (for example, <iframe> could become unsafe if just the sandbox attribute is removed, since src would be removed as well, it would be safe), but I'd appreciate some additional thoughts on this.

While I haven't executed the code, the direction and implementation seems correct to me after reading through the code flow of the KSES branch affected.

The only edge-cases I could think of are covered in the tests (I read the tests last).

I guess someone could be using $allowedposttags in another way and the new required-attribute branch could break that, but I don't see how or why someone would do that.. at most they'd only care about the tag keys.. surely..

There's also the potential that a plugin that adds the <object> tag to it could break or break this, but I think the plugin is likely broken already if that's the case.

[pento]

Thanks for the testing and feedback!

@peterwilsoncc: I've implemented the changes you recommended.

@dd32: I agree with your points on potential breakage, there's a small risk, but I think it's highly unlikely.

If someone is substantially changing the structure of $allowedposttags (and then restoring it in wp_kses_allowed_html, perhaps), that would be a totally unsupported use case. Same for if they're defining the required and values properties to mean something else.

If a plugin is adding the <object> tag to $allowedposttags, the only supported use case would be to work with KSES' existing behaviour, so the plugin should continue to function as is.

Just in case, however, I've flagged this ticket as needing a dev note, to ensure the changes are more widely disseminated.

[dd32]

Replying to pento:

I agree with your points on potential breakage, there's a small risk, but I think it's highly unlikely.

If someone is substantially changing the structure of $allowedposttags (and then restoring it in wp_kses_allowed_html, perhaps), that would be a totally unsupported use case.

Agreed, I mostly mentioned it out of completeness of looking for breakage points.

I've flagged this ticket as needing a dev note, to ensure the changes are more widely disseminated.

The most relevant thing to flag in a dev-note would be that "KSES can now have required attributes for tags, and whitelisted values for those attributes", and that a use-case is PDF <object>'s.

In addition to the one case I suggest, I wonder if it would also be good to add a test for if a filter is already adding object to wp_kses_allowed_html without requiring this specific type.

cf63ee7 adds a test for what I think would be the likely usage of this filter, but it can be tweaked further if you have particular use cases in mind. 🙂

​cf63ee7 adds a test for what I think would be the likely usage of this filter,

Does it need to account for modifications to $allowedposttags directly or do you think that will be dandy?

I don't know the answer to this, so it's a genuine question rather than a passive aggressive change request.

Does it need to account for modifications to $allowedposttags directly or do you think that will be dandy?

I don't think it needs to test for that as well: KSES runs $allowedposttags through the wp_kses_allowed_html filter before using it, so whilst the method could be different, the outcome is the same.

Does it need to account for modifications to $allowedposttags directly or do you think that will be dandy?

I don't think it needs to test for that as well: KSES runs $allowedposttags through the wp_kses_allowed_html filter before using it, so whilst the method could be different, the outcome is the same.

[pento]

In 51963:

KSES: Add options for restricting tags based upon their attributes.

This change adds two now attribute-related config options to KSES:

• An array of allowed values can be defined for attributes. If the attribute value doesn't fall into the list, the attribute will be removed from the tag.
• Attributes can be marked as required. If a required attribute is not present, KSES will remove all attributes from the tag. As KSES doesn't match opening and closing tags, it's not possible to safely remove the tag itself, the safest fallback is to strip all attributes from the tag, instead.

Included with this change is an implementation of these options, allowing the <object> tag to be stored in posts, but only when it has a type attribute set to application/pdf.

Props pento, swissspidy, peterwilsoncc, dd32, jorbin.
Fixes #54261.

[SergeyBiryukov]

In 52234:

Docs: Add a @since note and description to wp_kses_attr() for new attribute-related KSES options:

• Support for an array of allowed values for attributes.
• Support for required attributes.

Follow-up to [51963].

See #54261.

[peterwilsoncc]

In 52304:

KSES: Allow attributes to be restricted via callbacks.

Add callback validation to HTML tag attributes for increased flexibility over an array of values only.

In object tags, validate the data attribute via a callback to ensure it is a PDF and matches the type attribute. This prevents mime type mismatches in browsers.

Follow up to [51963].

Props Pento, dd32, swissspidy, xknown, peterwilsoncc.
Fixes #54261.

[ocean90]

[52304] assumes that the upload URL has no port so the check will fail for something like localhost:8888 due to the trailing slash in "http://$upload_host/". This can be seen in one of ​Gutenberg's check runs as reported by @youknowriad.

It's also odd that there seems to remain an empty object element.

[WIP]

Related to https://core.trac.wordpress.org/ticket/54261#comment:15

Testing a way to cater for port numbers in pdf $url argument.
Renaming $value to $url to match PHP doc comment.

Trac ticket:

Tests are passing now 😄

Are there any further tests required for this change (assuming the change itself is sound)?

[peterwilsoncc]

​PR 1998 covers the port oversight causing the tests to fail on the Gutenberg repo, it also adds some tests to for sites running on a port.

Props @ramonopoly

---

Replying to ocean90:

It's also odd that there seems to remain an empty object element.

KSES can't match opening to closing tags (and to do so would be a major refactor) so following a discussion on the PR, it was decided the empty object tag was the best solution available.

As this only becomes an issue if a user with unfiltered_html tries to mess with the input, I'm happy enough with that.

[ramonopoly]

Thanks for your help on this one @peterwilsoncc

[peterwilsoncc]

Replying to peterwilsoncc:

As this only becomes an issue if a user with unfiltered_html tries to mess with the input, I'm happy enough with that.

without unfiltered_html, rather.

It'd be good to put some unhappy paths in to make sure they fail with the ports too, for example example.org should be removed if the upload url is example.org:8888

I don't think the new test will need as many scenarios as the other data provider as it covers most of the unrelated to port unhappy paths.

According to the WordPress Slack, your on vacation at the moment so I can look in to that and push to your repo if you do not mind.

It'd be good to put some unhappy paths in to make sure they fail with the ports too, for example example.org should be removed if the upload url is example.org:8888

Great call. I've added a couple more, but feel free to edit/delete/change as you like.

According to the WordPress Slack, your on vacation at the moment so I can look in to that and push to your repo if you do not mind.

Hah, I'm mostly around. Thanks for offering!! I just like trees 🌴

[peterwilsoncc]

In 52309:

KSES: Accept port number in PDF upload paths.

Improves the URL validation in _wp_kses_allow_pdf_objects() to account for sites using an upload path that contains a port, for example wp.org:8080.

Follow up to [51963], [52304].

Props ocean90, ramonopoly, talldanwp.
See #54261.

[peterwilsoncc]

Fix to account for upload paths with port numbers committed above.

I've left the ticket open to address a question @swissspidy asked about supporting fragments. Chrome allows links to certain pages using fragments, eg document.pdf#page=10, so it's worth testing whether this works for embeds too.

If it's a standard or a faux-standard a majority of browsers support then I'm happy to account for it if it's possible to do so safely. If it's a Chrome only feature that's not part of a standard then I think the ticket can be closed.

Committed https://core.trac.wordpress.org/changeset/52309

[TobiasBg]

Suggestion: As it's a new function in WP 5.9, _wp_kses_allow_pdf_objects() could make use of the PHP 8 (but polyfilled via [52039] and [52040] in WP 5.9) str_contains() and str_ends_with() functions.

Implementing @TobiasBg's suggestion:

As it's a new function in WP 5.9, _wp_kses_allow_pdf_objects() could make use of the PHP 8 (but polyfilled via [52039] and [52040] in WP 5.9) str_contains() and str_ends_with() functions.

Trac ticket (comment): https://core.trac.wordpress.org/ticket/54261#comment:28

The tests in ​tests/kses.php should pass.

[SergeyBiryukov]

In 52326:

KSES: Use the polyfilled PHP 8 string functions in _wp_kses_allow_pdf_objects():

• str_contains()
• str_ends_with()
• str_starts_with()

Additionally, include a test for a PDF file in an <object> tag with an unsupported protocol.

Follow-up to [51963], [52039], [52040], [52304], [52309].

Props TobiasBg, ramonopoly.
See #54261.

Thanks for the PR! Merged in https://core.trac.wordpress.org/changeset/52326.

[peterwilsoncc]

I'm closing this fixed on the commits above.

The UI for the PDF embed block doesn't support altering the page displayed so allowing for that via fragments can be done in a follow-up ticket if the feature is added later.

[audrasjb]

Dev note published: ​https://make.wordpress.org/core/2022/01/04/miscellaneous-core-changes-in-wordpress-5-9/