Make WordPress Core

Opened 11 years ago

Last modified 19 months ago

#24248 new defect (bug)

'guid' not properly escaped

Reported by: meloniq's profile meloniq Owned by:
Milestone: 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 (7)

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

Download all attachments as: .zip

Change History (25)

@meloniq
11 years ago

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

#1 @meloniq
11 years ago

  • Cc meloniq@… added

#2 @SergeyBiryukov
11 years ago

  • Description modified (diff)

#3 @SergeyBiryukov
11 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
11 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#6 @nacin
11 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
10 years ago

  • Milestone changed from 3.8 to Future Release

#8 @SergeyBiryukov
10 years ago

#27663 was marked as a duplicate.

#9 @tlovett1
10 years ago

Tested patch, works well.

Version 0, edited 10 years ago by tlovett1 (next)

#10 @jared_smith
9 years 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
9 years ago

Updated patch and unit test

#11 @meloniq
9 years 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
9 years ago

adds 'raw' context to get_post_field() calls

@pareshradadiya
9 years ago

Guid field sanitation fix while insert and php unit testcase added

#12 @pareshradadiya
9 years ago

  • Keywords needs-unit-tests 3.7-early removed

#13 @pareshradadiya
9 years ago

  • Keywords dev-feedback added

@MikeHansenMe
9 years ago

#14 @MikeHansenMe
9 years ago

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

#15 @jared_smith
8 years ago

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

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

@jared_smith
8 years ago

Updated patch with code changes and unit tests

#16 @rachelbaker
8 years ago

#34459 was marked as a duplicate.

@MikeHansenMe
8 years ago

#17 @MikeHansenMe
8 years ago

24248.3.diff is a refresh of 24248.2.diff which just moves the unit-tests lower since they could not apply.

#18 @eceleste
19 months ago

This still seems to be a problem with WordPress version 6.0.2. I am seeing all sorts of posts with the & in place of the &. In fact, every single custom post type (or at least every GUID that includes an explicit post_type attribute) has this problem. In my case I am seeing this with WooCommerce 'product', 'shop_order', and 'shop_subscription' posts, a whole variety of Meta Box post types, 'formulator_forms', and of course all of my own custom post types.

This is very problematic, because if a page is referenced with this GUID then it does not load properly. For example, WordPress will treat a request for https://tca-woo.local/?post_type=course&p=260 as a request for page 260 using the page template for course&p=260, which does not exist. The page will display, but not with the appropriate course modifications. This can lead to all sorts of very strange problems.

Is there any work afoot to resolve this bug? Or has it been decided that it is OK for WordPress sites around the world just filling up with these strange and bogus GUIDs?

Note: See TracTickets for help on using tickets.