WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 4 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 needs-unit-tests 3.7-early
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 (3)

guid-context.patch (646 bytes) - added by meloniq 22 months ago.
adds 'raw' context to get_post_field() call for 'guid' field
24248.patch (1.8 KB) - added by jared_smith 4 months ago.
Updated patch and unit test
24248-raw-context.diff (1.2 KB) - added by meloniq 4 months ago.
adds 'raw' context to get_post_field() calls

Download all attachments as: .zip

Change History (14)

@meloniq22 months ago

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

comment:1 @meloniq22 months ago

  • Cc meloniq@… added

comment:2 @SergeyBiryukov22 months ago

  • Description modified (diff)

comment:3 @SergeyBiryukov22 months 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.

comment:5 @wonderboymusic19 months ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

comment:6 @nacin17 months 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.

comment:7 @nacin15 months ago

  • Milestone changed from 3.8 to Future Release

comment:8 @SergeyBiryukov11 months ago

#27663 was marked as a duplicate.

comment:9 @tlovett111 months ago

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

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

comment:10 @jared_smith4 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_smith4 months ago

Updated patch and unit test

comment:11 @meloniq4 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

@meloniq4 months ago

adds 'raw' context to get_post_field() calls

Note: See TracTickets for help on using tickets.