KSES: Allow PDFs to be embeded as objects #1757
Conversation
pento
changed the title
pento
force-pushed
the
fix/54261-kses-object-tag
branch
from
574bf4e
to
f8c556e
Compare
|
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:
I'm not aware of any tags that could cause safety issues if all attributes are removed (for example, |
I've done some branch testing and this looks good. As this involves kses, it would be good to get a second approval.
I've tried to find any HTML attributes with dangerous default attributes values and it doesn't appear there are any. I've waiting to hear back from some front end types who I've asked the same question of.
This is what I sent through kses to see if I could see anything
wp> wp_kses_post( '<object src="https://pwcc.cc"></object>' );
=>
string(17) "<object></object>"
wp> wp_kses_post( '<object href="https://pwcc.cc"></object>' );
=>
string(17) "<object></object>"
wp> wp_kses_post( '<iframe type="application/pdf" data="https://wordpress.org/foo.pdf"></iframe>' );
=>
string(0) ""
wp> wp_kses_post( '<object type="application/pdf" data="https://wordpress.org/foo.pdf"></object>' );
=>
string(77) "<object type="application/pdf" data="https://wordpress.org/foo.pdf"></object>"
wp> wp_kses_post( '<object type="application/pdf" data="https://wordpress.org/foo.pdf" />' );
=>
string(70) "<object type="application/pdf" data="https://wordpress.org/foo.pdf" />"
wp> wp_kses_post( '<object type="application/pdf" data="https://wordpress.org/foo.pdf" /><object type="application/exe" data="https://wordpress.org/foo.exe" />' );
=>
string(70) "<object type="application/pdf" data="https://wordpress.org/foo.pdf" />"
wp> wp_kses_post( '<object type="application/exe" data="https://wordpress.org/foo.exe" /><object type="application/pdf" data="https://wordpress.org/foo.pdf" />' );
=>
string(70) "<object type="application/pdf" data="https://wordpress.org/foo.pdf" />"
wp> wp_kses_post( '<iframe type="application/pdf" data="https://wordpress.org/foo.pdf"></iframe>' );
=>
string(0) ""
wp> exit
|
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 There's also the potential that a plugin that adds the |
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. |
Does it need to account for modifications to I don't know the answer to this, so it's a genuine question rather than a passive aggressive change request. |
I don't think it needs to test for that as well: KSES runs |
Trac ticket: https://core.trac.wordpress.org/ticket/54261
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.