Proper use of prepared statements

[xknown]

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 %.

[markjaquith]

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.

[DD32]

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.

[ryan]

Because of escaping.

[zamoose]

​Boots of escaping? Or just regular escaping...

*grin*

[santosj]

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 ));

[santosj]

$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.

[santosj]

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.

[markjaquith]

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.

[santosj]

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

[markjaquith]

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