Use wp_kses in place of wp_filter_post_kses to sanitize post title #2840
Conversation
May fix #2788 This PR switches from using `wp_filter_post_kses()` for `post_title` sanitization to calling `wp_kses` directly: #2788 describes that the core behavior is to call `wp_filter_kses()`, but @westonruter notes in that thread that the slash handling in `wp_filter_kses` is lossy so using the underlying implementation of `wp_filter_kses` without the de-slashing and re-slashing should provide adequate sanitization without compromising the integrity of the content. Review requested especially from @rachelbaker or @westonruter
| @@ -824,9 +824,10 @@ protected function prepare_item_for_database( $request ) { | |||
| // Post title. | |||
| if ( ! empty( $schema['properties']['title'] ) && isset( $request['title'] ) ) { | |||
| if ( is_string( $request['title'] ) ) { | |||
| $prepared_post->post_title = wp_filter_post_kses( $request['title'] ); | |||
| // wp_filter_kses slash handling is lossy: use the underlying methods directly | |||
| $prepared_post->post_title = wp_kses( $request['title'], current_filter() ); | |||
There was a problem hiding this comment.
I think that current_filter() should be replaced with 'post'. I'm not sure if there are filters being applied at this point, but even if they are, for sure they're not going to be the same as what are being applied during a call to sanitize_post().
There was a problem hiding this comment.
I thought current_filter() here should be title_save_pre because post titles get a stricter set of HTML tags allowed than wp_kses_post()
There was a problem hiding this comment.
You're probably right. So the string literal 'title_save_pre' should be used instead of current_filter().
| } elseif ( ! empty( $request['title']['raw'] ) ) { | ||
| $prepared_post->post_title = wp_filter_post_kses( $request['title']['raw'] ); | ||
| $prepared_post->post_title = wp_kses( $request['title']['raw'], current_filter() ); |
|
@rachelbaker @westonruter thank you for clarifying, that makes sense. I've updated the patch |
|
@kadamwhite we use Recording this here, but can be done in another PR. |
|
@kadamwhite Only thing blocking this would be a unit test. |
|
Unit test would be e.g. create a post w/ a title including a disallowed HTML tag such as |
| $request->set_body_params( $params ); | ||
| $response = $this->server->dispatch( $request ); | ||
| $new_data = $response->get_data(); | ||
| $this->assertEquals( '\o/', $new_data['title']['raw'] ); |
There was a problem hiding this comment.
@westonruter @rachelbaker This test fails, but I would have expected it to pass based on our discussion previously. Any ideas?
There was a problem hiding this comment.
@kadamwhite My guess would be thatwp_kses_no_null() (called in wp_kses()) is stripping out the preceding slash.
There was a problem hiding this comment.
If we can't escape (har har) the unslashing, is the use of wp_kses overkill? if you can suggest a direction I will implement.
There was a problem hiding this comment.
@kadamwhite crazy idea, what happens if you run this test with wp_filter_kses instead?
There was a problem hiding this comment.
@kadamwhite yeah, I tested it too and was not able to get a passing test. 😞
I also WAS able to confirm that within the WP editor I can save a post with a title of Do what I mean \ Not what I say, and that is also exactly how the data is stored in the db. So we are doing something WRONG somewhere here.
There was a problem hiding this comment.
This is potentially an issue because of the unescaped slash? Try \\o/ instead?
There was a problem hiding this comment.
@rmccue Same results with '\\o/', with both wp_filter_kses and wp_kses
|
Closing, as I believe this has been superseded by 4.7 trunk |
May fix #2788
This PR switches from using
wp_filter_post_kses()forpost_titlesanitization to callingwp_ksesdirectly: #2788 describes that the core behavior is to callwp_filter_kses(), but @westonruter notes in that thread that the slash handling inwp_filter_ksesis lossy so using the underlying implementation ofwp_filter_kseswithout the de-slashing and re-slashing should provide adequate sanitization without compromising the integrity of the content.Review requested especially from @rachelbaker or @westonruter