WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 months ago

#24248 new defect (bug)

'guid' not properly escaped

Reported by: meloniq Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.5
Component: Posts, Post Types Keywords: has-patch has-unit-tests
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Probably related issues: #18274 #19248

'guid' being saved in database not properly escaped, example:
http://www.wordpress.dev/?post_type=changeset&p=57 , see the ampersand encode &
It supposed to be & or at least &

Once 'auto-draft' saved, 'guid' is correct: http://www.wordpress.dev/?post_type=changeset&p=57

Once post is saved as 'draft' or published (triggered 'update post' on auto-draft), 'guid' gets malformed.

Source of issue: inappropriate usage of get_post_field() function in the wp_insert_post()

get_post_field() defaults to 'display' context, we not specify context while obtaining field, and in the wp_insert_post() we are not going to display it anywhere, just get, check, and save again, correct?

Attached patch adds the 'raw' context to usage of get_post_field() with 'guid'

Attachments (6)

guid-context.patch (646 bytes) - added by meloniq 3 years ago.
adds 'raw' context to get_post_field() call for 'guid' field
24248.patch (1.8 KB) - added by jared_smith 16 months ago.
Updated patch and unit test
24248-raw-context.diff (1.2 KB) - added by meloniq 15 months ago.
adds 'raw' context to get_post_field() calls
24248-updated.diff (2.8 KB) - added by pareshradadiya 10 months ago.
Guid field sanitation fix while insert and php unit testcase added
24248.diff (3.1 KB) - added by MikeHansenMe 4 months ago.
24248.2.diff (3.1 KB) - added by jared_smith 2 months ago.
Updated patch with code changes and unit tests

Download all attachments as: .zip

Change History (21)

@meloniq
3 years ago

adds 'raw' context to get_post_field() call for 'guid' field

#1 @meloniq
3 years ago

  • Cc meloniq@… added

#2 @SergeyBiryukov
3 years ago

  • Description modified (diff)

#3 @SergeyBiryukov
3 years ago

  • Keywords needs-unit-tests 3.7-early added; needs-testing removed
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from trunk to 2.5

Introduced in [6593]. Triggered by the introduction of custom post types in 2.9, since their permalinks can contain an ampersand.

guid-context.patch seems good. Other get_post_field() instances in wp_insert_post() should probably use 'raw' context too.

#5 @wonderboymusic
3 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#6 @nacin
2 years ago

  • Milestone changed from 3.7 to 3.8

This is old and there is a concern there are other potential mines in wp_insert_post() as well.

I'm also concerned about tests for this.

#7 @nacin
2 years ago

  • Milestone changed from 3.8 to Future Release

#8 @SergeyBiryukov
22 months ago

#27663 was marked as a duplicate.

#9 @tlovett1
22 months ago

Tested patch, works well. Can we get this committed? This a huge pain when dealing with syndicated posts.

Last edited 22 months ago by tlovett1 (previous) (diff)

#10 @jared_smith
16 months ago

I've attempted to work on your patch after WordCamp SF, but unfortunately, this patch doesn't appear to fix the problem. I've re-rolled your patch to apply to current trunk, and added a unit test, but even with the patch the unit test fails.

The reason for this is that the conversion of "&" to "&" is happening on the database insert, not on the retrieval from the database.

@jared_smith
16 months ago

Updated patch and unit test

#11 @meloniq
15 months ago

The reason for this is that the conversion of "&" to "&" is happening on the database insert, not on the retrieval from the database.

Conversion happening on post sanitization at beginning of this function $postarr = sanitize_post($postarr, 'db');

The 'url fields' are filtered with wp_strip_all_tags(), esc_url_raw(), and wp_filter_kses() - the last one converts & to &

Filters added here: https://github.com/WordPress/WordPress/blob/master/wp-includes/default-filters.php#L59-L65

@meloniq
15 months ago

adds 'raw' context to get_post_field() calls

@pareshradadiya
10 months ago

Guid field sanitation fix while insert and php unit testcase added

#12 @pareshradadiya
10 months ago

  • Keywords needs-unit-tests 3.7-early removed

#13 @pareshradadiya
10 months ago

  • Keywords dev-feedback added

@MikeHansenMe
4 months ago

#14 @MikeHansenMe
4 months ago

24248.diff is a refresh that moves the code changes to post-functions.php

#15 @jared_smith
2 months ago

  • Keywords has-unit-tests added; dev-feedback removed

Refreshed the patch to move the code back to post.php.

@jared_smith
2 months ago

Updated patch with code changes and unit tests

Note: See TracTickets for help on using tickets.