WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 6 weeks ago

#45121 closed defect (bug) (fixed)

wp_update_post() can modify post tag

Reported by: kaggdesign Owned by: SergeyBiryukov
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.9.8
Component: Posts, Post Types Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

Problem

Update of post by means of wp_update_post() can modify tags assigned to the post.

Steps to reproduce:

Fresh WP install.

Create two tags with the same name (let say wp_update_post_tag) and different slugs (let say wp_update_post_tag_1} and wp_update_post_tag_2, in this sequence).

Create a post with tag wp_update_post_tag_2. Check that it has tag wp_update_post_tag_2.

Use wp_update_post( $post ). Check that now it has tag wp_update_post_tag_1.

This sequence is demonstrated by the plugin https://github.com/kagg-design/update-post-bug.


Reason

Problem is caused by the following lines in wp_insert_post():

	if ( isset( $postarr['tags_input'] ) && is_object_in_taxonomy( $post_type, 'post_tag' ) ) {
		wp_set_post_tags( $post_ID, $postarr['tags_input'] );
	}

At this point, $postarr['tags_input'] already is array( 'wp_update_post_tag' ), containing tag_name, not ID. This is because wp_update_post() executes

	// First, get all of the original fields.
	$post = get_post($postarr['ID'], ARRAY_A);

Here $post gets tags_input as array( 'wp_update_post_tag' ), containing tag_name, not ID.

Attachments (5)

45121.diff (590 bytes) - added by kaggdesign 15 months ago.
45121-test.diff (1.1 KB) - added by kaggdesign 15 months ago.
45121-improved-test.diff (1.3 KB) - added by kaggdesign 15 months ago.
Improved test, to check situation when 3 tags with the same name exist, and 2 of them are assigned to the post.
wp_update_post.diff (2.0 KB) - added by kaggdesign 6 months ago.
All-inclusive patch for the ticket. Fix by itself and test.
45121.2.diff (2.8 KB) - added by SergeyBiryukov 6 weeks ago.

Download all attachments as: .zip

Change History (21)

#1 follow-up: @SergeyBiryukov
18 months ago

Related: #30615

#2 in reply to: ↑ 1 @kaggdesign
18 months ago

Replying to SergeyBiryukov:

Related: #30615

Yes, it is related, with the only difference that issue is properly fixed in /wp-admin/includes, but not in /wp-includes. I will try to propose similar commit with relevant tests.

#3 @kaggdesign
15 months ago

If several tags with same name exist, and post has one of such tags, then wp_update_post() will modify tag assigned to post.

This is caused by $post = get_post($postarr['ID'], ARRAY_A); in wp_update_post(), which returns 'tags_input' as strings, not ids.

We fix the issue by adding conversion of strings in 'tags_input' to ids. )

@kaggdesign
15 months ago

#4 @kaggdesign
15 months ago

  • Keywords has-patch has-unit-tests added

@kaggdesign
15 months ago

Improved test, to check situation when 3 tags with the same name exist, and 2 of them are assigned to the post.

#5 @SergeyBiryukov
14 months ago

  • Milestone changed from Awaiting Review to 5.2
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

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


12 months ago

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


12 months ago

#8 @pento
11 months ago

  • Milestone changed from 5.2 to 5.3

As WP 5.2 RC1 is being released today, I'm moving this ticket to 5.3.

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


6 months ago

#10 @davidbaumwald
6 months ago

  • Keywords needs-refresh added

This ticket was discussed during today's bug scrub. The most recent patch needs a refresh, and the tests should be in an all-inclusive patch.

#11 @kaggdesign
6 months ago

@davidbaumwald @SergeyBiryukov Please find attached all-inclusive patch wp_update_post.diff with fix by itself and test. Code in the patch is refreshed and applicable to WordPress 5.3-beta1-46286.

Thank you.

Last edited 6 months ago by kaggdesign (previous) (diff)

@kaggdesign
6 months ago

All-inclusive patch for the ticket. Fix by itself and test.

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


6 months ago

#13 @desrosj
6 months ago

  • Milestone changed from 5.3 to 5.4

With 5.3 RC 1 in a few hours and this needing more work, I'm going to punt this.

If someone has the time before RC to properly review and commit, it can be moved back.

#14 @SergeyBiryukov
6 weeks ago

In 47311:

Tests: Remove an irrelevant tags_input property assertion from test_get_page_template_property().

This appears to be a copy/paste from the test_get_tags_input_property() test above.

See #45121.

#15 @SergeyBiryukov
6 weeks ago

  • Keywords commit added; needs-refresh removed

wp_update_post.diff does solve the reported problem, but it also prevents any modification of post tags via the tags_input parameter, even if it's intentional.

Something along the lines of [31359] seemed like it might work, however in practice it doesn't, since the tags_input value returned by WP_Post::__get() contains tag names, and if they are the same, there's no way to disambiguate them at that point.

I think the correct solution here would be to disregard the tags_input parameter if it's the same as existing post tags. That way tags are only modified if tags_input was explicitly provided, and is different from the existing tags.

See 45121.2.diff.

#16 @SergeyBiryukov
6 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 47317:

Posts, Post Types: Discard tags_input parameter in wp_update_post() if it's the same as existing post tags.

This ensures that wp_update_post() does not unintentionally modify post tags if the post has several tags with the same name but different slugs.

Tags should only be modified if tags_input parameter was explicitly provided, and is different from the existing tags.

Props kaggdesign, SergeyBiryukov.
Fixes #45121.

Note: See TracTickets for help on using tickets.