Opened 22 months ago
Last modified 4 months ago
#24248 new defect (bug)
'guid' not properly escaped
| Reported by: |
|
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)
Change History (14)
comment:2
@SergeyBiryukov
— 22 months ago
- Description modified (diff)
comment:3
@SergeyBiryukov
— 22 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:4
@SergeyBiryukov
— 22 months ago
Related: #13555
comment:5
@wonderboymusic
— 19 months ago
- Milestone changed from Future Release to 3.7
these are all marked 3.7-early
comment:6
@nacin
— 17 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:8
@SergeyBiryukov
— 11 months ago
#27663 was marked as a duplicate.
comment:9
@tlovett1
— 11 months ago
Tested patch, works well. Can we get this committed? This a huge pain when dealing with syndicated posts.
comment:10
@jared_smith
— 4 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.
comment:11
@meloniq
— 4 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
adds 'raw' context to get_post_field() call for 'guid' field