[wp-trac] [WordPress Trac] #11608: wpdb->prepare() is broken

[WordPress Trac]

#11608: wpdb->prepare() is broken
--------------------------+-------------------------------------------------
 Reporter:  hakre         |       Owner:  ryan     
     Type:  defect (bug)  |      Status:  new      
 Priority:  normal        |   Milestone:  3.0      
Component:  Database      |     Version:  2.9      
 Severity:  normal        |    Keywords:  has-patch
--------------------------+-------------------------------------------------
Changes (by hakre):

  * keywords:  reporter-feedback => has-patch


Comment:

 Replying to [comment:47 westi]:
 > After reading through all the comments above I can not see a clear
 definition of the '''bug''' here that exists in {{{$wpdb->prepare}}}.

 You're right, the clear definition of the bug is missing:

  1. Improper handling of input data in wpdb->prepare()
  2. Invalid docblock in wpdb->prepare()

 The function just does not behave properly on input data. Often it's
 argumented that problems will be catched by vsprintf() in it, but that's
 just not the case (just one example: {{{$wpdb->prepare( $query = '% wrong
 query', '' )}}}).

 I hope you share the point that wpdb->prepare() is an important function
 being interface to core coders and many plugin authors. Next to
 encouraging them to actually use that function, it should at least provide
 a certain level of stability and enough defensive potential that it can
 take up with the input by us and our beloved users.

 It was not far ago I criticised code in the WPDB class (again) that it
 does not provide a secure level of data escaping. In that discussion Sivel
 pointed to '''''wpdb->prepare() as the only function''''' that provides a
 proper mysql escape via the classes public interface. I would like to see
 that one at least properly implemented.

 The last paragraph is for background information regarding this ticket's
 title:[[BR]]
  '''wpdb->prepare() is broken'''.

 Flaws I saw so far:

  1. wpdb->prepare()'s docblock says it return NULL on error, actually that
 function does return FALSE and STRINGs on error as well. By returning
 STRINGs the user is not able to determine wether or not prepare failed on
 the query-pattern provided.
  2. wpdb->prepare()'s docblock does not properly document the substitution
 pattern, especially the usage of %%.
  3. wpdb->prepare() does not validate the $query parameter
  4. wpdb->prepare() passes unvalidated data to vsprintf().
  5. wpdb->prepare() ocasionally modifies the user's input on best guess
 (w/o being documented properly), by definition there is no need to do so.

 ''Regarding 5.:'' I think this is a bug, to retain backwards compability
 it might be too late to take this out ([comment:27 ref. to DDB]). I have
 at least improved that in my patch, but it does not solve that issue, so
 it is only ''more stable'' which is  relative to say at least. Let's
 attribute this as hardening.

-- 
Ticket URL: <http://core.trac.wordpress.org/ticket/11608#comment:52>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software