WordPress.org

Make WordPress Core

Opened 5 days ago

Last modified 82 minutes ago

#54261 assigned enhancement

KSES: Allow PDFs to be embeded as objects

Reported by: pento Owned by: pento
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch 2nd-opinion needs-testing has-unit-tests needs-dev-note
Focuses: Cc:

Description

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.

Change History (11)

This ticket was mentioned in PR #1757 on WordPress/wordpress-develop by pento.


5 days ago

  • Keywords has-unit-tests added

#2 @pento
5 days ago

  • Keywords has-unit-tests removed
  • Summary changed from KSES: Allow PDFs to be embed as objects to KSES: Allow PDFs to be embeded as objects

#3 @pento
5 days ago

  • Keywords has-unit-tests added

#4 @prbot
28 hours ago

pento commented on PR #1757:

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.

#5 @prbot
24 hours ago

dd32 commented on PR #1757:

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.

#6 follow-up: @pento
23 hours ago

  • Keywords needs-dev-note added

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.

#7 in reply to: ↑ 6 @dd32
23 hours ago

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.

#8 @prbot
6 hours ago

pento commented on PR #1757:

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

#9 @prbot
2 hours ago

peterwilsoncc commented on PR #1757:

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.

#10 @prbot
82 minutes ago

pento commented on PR #1757:

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.

#11 @prbot
82 minutes ago

pento commented on PR #1757:

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.

Note: See TracTickets for help on using tickets.