WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#9727 closed defect (bug) (fixed)

missing escape in wp_update_post?

Reported by: Denis-de-Bernardy Owned by: ryan
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.8
Component: Security Keywords: has-patch commit
Focuses: Cc:

Description

Noticed this while looking into #9539. wp_update_post() starts like this:

function wp_update_post($postarr = array()) {
	if ( is_object($postarr) )
		$postarr = get_object_vars($postarr);

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

	// Escape data pulled from DB.
	$post = add_magic_quotes($post);

Shouldn't it be:

function wp_update_post($postarr = array()) {
	if ( is_object($postarr) ) {
		// non-escaped post was passed
		$postarr = get_object_vars($postarr);
		$postarr = add_magic_quotes($postarr);
	}

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

	// Escape data pulled from DB.
	$post = add_magic_quotes($post);

Attachments (1)

9727.diff (584 bytes) - added by Denis-de-Bernardy 5 years ago.
in case it should…

Download all attachments as: .zip

Change History (12)

comment:1 hakre5 years ago

documentation says:

 * @param array|object $postarr Post data.

i am still waiting on a developers feedback what this means when nothing is stated: raw or slashed.

ryan (?) once said lateöy all functions assume slashed if not otherwise noted. i doubted that but i can not find the ticket. a general statement would be really usefull.

comment:2 Denis-de-Bernardy5 years ago

  • Component changed from General to Security
  • Owner changed from anonymous to ryan

Denis-de-Bernardy5 years ago

in case it should...

comment:4 Denis-de-Bernardy5 years ago

  • Keywords has-patch added

comment:6 ryan5 years ago

We generally assume slashed, yes, although in this case it looks like we have a loophole for things passed as an object. Changing it might break existing expectations. Perhaps update phpdoc to note that objects are not expected slashed.

comment:7 ryan5 years ago

I don't see a single place in trunk that passes an object, so I can't tell what current expectations are. Assuming an object to be not-escaped makes sense though.

comment:8 Denis-de-Bernardy5 years ago

added this to the end of my wp-config.php file to give it a try:

$post_id = 119;
$post = get_post($post_id);
dump($post->post_title); // string(41) "Comment on Quisque \' Eget Dolor by admin"
wp_update_post($post);
unset($post);
$post = get_post($post_id); // string(40) "Comment on Quisque ' Eget Dolor by admin"
dump($post->post_title);

comment:9 Denis-de-Bernardy5 years ago

there might be other similar functions that need some attention.

comment:10 Denis-de-Bernardy5 years ago

  • Keywords commit added; 2nd-opinion dev-feedback removed

comment:11 ryan5 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.