WordPress.org

Make WordPress Core

Opened 14 years ago

Closed 14 years ago

Last modified 12 years ago

#5145 closed defect (bug) (fixed)

Proper use of prepared statements

Reported by: xknown Owned by: markjaquith
Milestone: 2.5 Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: Cc:

Description

I upgraded my blog to the latest svn revision (6188) and it fails to update/create a post if it contains %d, %s or any (printf) type modifier in a field that can accept strings: post_content, post_title, post_excerpt...

A quick look at the code revealed the problem:

$wpdb->query(
	$wpdb->prepare(
	"UPDATE IGNORE $wpdb->posts SET
	post_author = '$post_author',
	post_date = '$post_date',
	post_date_gmt = '$post_date_gmt',
	post_content = '$post_content',
	post_content_filtered = '$post_content_filtered',
	post_title = '$post_title',
	post_excerpt = '$post_excerpt',
	post_status = '$post_status',
	post_type = '$post_type',
	comment_status = '$comment_status',
	ping_status = '$ping_status',
	post_password = '$post_password',
	post_name = '$post_name',
	to_ping = '$to_ping',
	pinged = '$pinged',
	post_modified = '".current_time('mysql')."',
	post_modified_gmt = '".current_time('mysql',1)."',
	post_parent = %d,
	menu_order = '$menu_order'
	WHERE ID = %d"
	, $post_parent, $post_ID ));

You shouldn't concatenate variables if its value will likely contain a %.

Change History (10)

#1 @markjaquith
14 years ago

  • Owner changed from anonymous to markjaquith
  • Status changed from new to assigned

Yep. Problem of partially implementing $wpdb->prepare(). Ideally, we'd be getting all these vars in an unslashed state and then we could insert them using prepare(). Might have to revert any partial implementations like this.

#2 @DD32
14 years ago

Is there a reason why this wasnt implemented as such:

$wpdb->prepare(
	"UPDATE IGNORE $wpdb->posts SET
	post_author = %s,
	post_date = %s,
...

Its a bit harder to read, However, it'll pass through prepare() as expected.

#3 @ryan
14 years ago

Because of escaping.

#4 @zamoose
14 years ago

Boots of escaping? Or just regular escaping...

*grin*

#5 @santosj
14 years ago

I think the problem is that instead of defeating the purpose of even having the function, the variables should be outside the string and instead also used the %{whatever}.

$wpdb->prepare(
	"UPDATE IGNORE $wpdb->posts SET
	post_author = '$post_author',
	post_date = '$post_date',
	post_date_gmt = '$post_date_gmt',
	post_content = '$post_content',
	post_content_filtered = '$post_content_filtered',
	post_title = '$post_title',
	post_excerpt = '$post_excerpt',
	post_status = '$post_status',
	post_type = '$post_type',
	comment_status = '$comment_status',
	ping_status = '$ping_status',
	post_password = '$post_password',
	post_name = '$post_name',
	to_ping = '$to_ping',
	pinged = '$pinged',
	post_modified = '".current_time('mysql')."',
	post_modified_gmt = '".current_time('mysql',1)."',
	post_parent = %d,
	menu_order = '$menu_order'
	WHERE ID = %d"
	, $post_parent, $post_ID ));

should be, like:

{{{$wpdb->prepare(

"UPDATE IGNORE $wpdb->posts SET
post_author = '%s',
post_date = '%s',
post_date_gmt = '%s',
post_content = '$post_content',
post_content_filtered = '$post_content_filtered',
post_title = '$post_title',
post_excerpt = '$post_excerpt',
post_status = '$post_status',
post_type = '$post_type',
comment_status = '$comment_status',
ping_status = '$ping_status',
post_password = '$post_password',
post_name = '$post_name',
to_ping = '$to_ping',
pinged = '$pinged',
post_modified = '".current_time('mysql')."',
post_modified_gmt = '".current_time('mysql',1)."',
post_parent = %d,
menu_order = '$menu_order'
WHERE ID = %d"
, $post_author, $post_date, $post_date_gmt, ..., $post_parent, $post_ID ));

}}}

And so on and so forth.

I think the whole concept is being defeated by using it for numeric values when you could use $post_author, $post_date, $post_date_gmt, ..., (num) $post_parent, (num) $post_ID ));

#6 @santosj
14 years ago

$wpdb->prepare( 

"UPDATE IGNORE $wpdb->posts SET post_author = '%s', 
post_date = '%s', 
post_date_gmt = '%s', 
post_content = '$post_content', 
post_content_filtered = '$post_content_filtered', 
post_title = '$post_title', 
post_excerpt = '$post_excerpt', post_status = '$post_status', post_type = '$post_type', comment_status = '$comment_status', ping_status = '$ping_status', post_password = '$post_password', post_name = '$post_name', to_ping = '$to_ping', pinged = '$pinged', 
post_modified = '".current_time('mysql')."', 
post_modified_gmt = '".current_time('mysql',1)."', 
post_parent = %d, 
menu_order = '$menu_order' 
WHERE ID = %d" , 
$post_author, $post_date, $post_date_gmt, ..., $post_parent, $post_ID )); 

What I mean to say, is that every variable should be outside the SQL string, much like the PDO prepare(). It doesn't seem logical have the whole concept for escaping strings and only use it for numeric values. There are already methods for sanitizing those values.

#7 @santosj
14 years ago

My above post should be the correct fix, but I think that is what Mark is talking about. I probably didn't understand him correctly.

#8 @markjaquith
14 years ago

Yes, the ideal solution is to have the strings outside the query, and use %s placeholders. But %s replacements get slash-escaped, and the variables in this instance are already slash-escaped. So that would lead to double-escaping. What I'm in the process of doing is converting to use $wpdb->prepare() any query that can be converted, and marking up any place that can't be converted because the function expects pre-slashed data. My mistake was in doing partial implementation for some queries. It has to be all or nothing, or we risk concatenating in a printf() token.

#9 @santosj
14 years ago

I should have thought more before I made any disrespect. None was meant by the way.

#10 @markjaquith
14 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [6216]) Revert partial prepare() implementation. Needs to be all or nothing. Props xknown. fixes #5145

Note: See TracTickets for help on using tickets.