WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#38609 closed defect (bug) (fixed)

REST API: `unfiltered_html` and slashing: posts and attachments

Reported by: jnylen0 Owned by: rmccue
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests dev-feedback
Focuses: Cc:
PR Number:

Description

Migrated from https://github.com/WP-API/WP-API/issues/2787, comments paraphrased from @rachelbacker @kadamwhite and @rmccue.


Match WordPress Core's logic when users with the unfiltered_html capability are creating/updating Posts or Comments

  • For Posts: Core removes the appropriate kses filter from sanitizing post_content and post_title.
  • For Comments: Core changes the active kses filter used for sanitizing comment_content to wp_filter_post_kses().

See the kses_init() function in WP Core for the complete logic.


Per discussion in 2016-10-17 API team chat, the API should honor unfiltered_html; proposed next steps:

  1. Let's take a look at introducing a new function into core that checks unfiltered_html and sanitizes the input appropriately, and this could be used in core rather than the remove_filter craziness. This depends on back-compat as to whether core can use it, but is a good path forward.
  2. Let's also switch our behaviour ASAP, then use core's behaviour if/when it exists.

This also came up again in recent Slack discussion regarding inserting data: URIs, which requires unfiltered_html.

Attachments (6)

38609.0.diff (6.8 KB) - added by westonruter 3 years ago.
38609.1.diff (18.8 KB) - added by jnylen0 3 years ago.
Fix post handling and add tests for posts
38609.2.diff (53.2 KB) - added by jnylen0 3 years ago.
Now includes post and attachment tests
38609.3.diff (50.3 KB) - added by jnylen0 3 years ago.
38609.4.diff (50.9 KB) - added by jnylen0 3 years ago.
Only includes posts and attachments changes
38609.5.diff (42.5 KB) - added by jnylen0 3 years ago.
Just the post and attachment changes, now that #38679 is in.

Download all attachments as: .zip

Change History (26)

#1 @rmccue
3 years ago

I'd started on this, but there are so many kses functions that it ended up being pretty bloated, so I'd suggest we only implement a subset of these. The issue is that wp_kses takes a $allowed_html parameter, which is usually the context. There are wrappers to set this to post (wp_kses_post), the current filter (wp_kses_data), and slashing variants of all of these.

I'd propose we add two more functions:

  • wp_maybe_kses
  • wp_maybe_kses_post

Both of this shortcircuit if the user has unfiltered_html.

Unfortunately, due to the highly undefined nature of the existing filters, likely not easy to migrate these over. These filters all use the slashed variant, and rely on the name of the current filter in wp_filter_kses, which seems insane.

#2 @rachelbaker
3 years ago

  • Milestone changed from Awaiting Review to 4.7

This ticket was mentioned in Slack in #core by jnylen. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.


3 years ago

#5 @danielbachhuber
3 years ago

@jnylen0 @westonruter and I chatted on a Hangout about this for ~10 minutes. James is going to focus on putting together a variety of test cases to assert expected behavior for handling slashed data / unfiltered_html behavior. From that, we'll be able to determine which tests are failing and what we need to do to correct the behavior.

For reference, WP-CLI had this problem with slashed data a while back:

@westonruter
3 years ago

@jnylen0
3 years ago

Fix post handling and add tests for posts

#6 @jnylen0
3 years ago

It looks to me like @westonruter's patch will pretty much fix all the things here, with some minor tweaks.

In 38609.1.diff:

  • Fix creating posts by calling wp_slash( (array) $post ) instead of wp_slash( $post )
  • Add a bunch of tests for creating and updating posts with the nastiest HTML I could think of

In trunk, 10/12 of these new tests fail (8/12 in multisite mode). With this patch, they all pass in both modes.

This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by nerrad. View the logs.


3 years ago

@jnylen0
3 years ago

Now includes post and attachment tests

#9 @jnylen0
3 years ago

In 38609.2.diff:

  • Simplify test harness and test rendered content also
  • Add tests for attachments

This patch *includes* https://core.trac.wordpress.org/attachment/ticket/38679/38679.diff because they are closely related. Adding tests for attachments here surfaced the issue in #38679, which I think should be patched separately.

There are a couple of "TODO" / "FIXME" comments in the tests. Right now, the rendered description for media items includes the <p class="attachment"> tag containing the attachment content itself. Perhaps the filters we apply to attachment description should be tweaked, and this should show up in the content property instead?

#10 @jnylen0
3 years ago

  • Keywords has-patch has-unit-tests dev-feedback added

This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by jnylen. View the logs.


3 years ago

#13 @jnylen0
3 years ago

  • Summary changed from REST API should honor the `unfiltered_html` capability to REST API: `unfiltered_html` and slashing: posts and attachments

@jnylen0
3 years ago

@jnylen0
3 years ago

Only includes posts and attachments changes

#14 @jnylen0
3 years ago

38609.3.diff was a bad patch with a couple of failing tests; please ignore it.

38609.4.diff includes the changes to fix unfiltered_html and slashing for posts and attachments without touching other object types. I'll create other tickets for those.

Note that 38679.diff is still applied here to fix content handling for attachments.

Last edited 3 years ago by jnylen0 (previous) (diff)

@jnylen0
3 years ago

Just the post and attachment changes, now that #38679 is in.

#15 @rmccue
3 years ago

  • Owner set to rmccue
  • Resolution set to fixed
  • Status changed from new to closed

In 39155:

REST API: Respect unfiltered_html for HTML post fields.

This necessitates a change to our slashing code as well. Ah slashing, the cause of, and solution to, all of life's problems.

Props jnylen0.
Fixes #38609.

#16 @rmccue
3 years ago

In 39156:

REST API: Remove ship emoji from slashing tests.

Introduced in [39155], but not necessary for the slashing tests. These fail on 5.3, which encodes the emoji as HTML entities.

Props dd32.
See #38609.

#17 @rmccue
3 years ago

In 39157:

REST API: Respect unfiltered_html for HTML comment fields.

Same as [39155], but for comments, natch.

Props jnylen0.
Fixes #38704, see #38609.

#18 @rmccue
3 years ago

In 39160:

REST API: Remove more emoji from tests.

More from [39156].

See #38609.

#19 @joehoyle
3 years ago

In 39190:

REST API: unfiltered_html and slashing: terms.

Follow-up to #38609 and #38704; handle slashes correctly for taxonomy terms.

Props westonruter, jnylen0.
Fixes #38726, see #38609.

This ticket was mentioned in Slack in #core by jnylen. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.