Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 6 years ago

#22262 closed defect (bug) (fixed)

Possible invalid uses of wpdb::prepare()

Reported by: xknown's profile xknown Owned by: nacin's profile nacin
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: Database Keywords: has-patch
Focuses: Cc:

Description

One common error I see about wpdb::prepare() is that many developers tend to use it by passing only a SQL query, which may produce bugs due to the implementation of this method.

As you know, wpdb::prepare() does, generally speaking, a string replacement of the placeholders (%s, %d, etc) by using sprintf/vsprintf under the scenes. So, if one passes a SQL query with placeholders and no other parameters, then this method returns a blank string. For example:

$query = $wpdb->prepare( 'select * from table where column = %s', $user_input );

$result_set = $wpdb->get_results( $wpdb->prepare( $query ) );

If $user_input contains a placeholder (i.e. "hola%s mundo"), the query will not be executed.

I used a simple static code analyzer to detect this calls on the core and found two instances.

We should call _doing_it_wrong() if wpdb::prepare() receives only one parameter.

Attachments (1)

22262.diff (2.0 KB) - added by nacin 11 years ago.

Download all attachments as: .zip

Change History (11)

#1 follow-up: @nacin
11 years ago

  • Component changed from General to Database
  • Milestone changed from Awaiting Review to 3.5

I agree on all counts.

What if we declared prepare() with a second argument, automatically causing E_WARNINGs? Might not be the prettiest for sites that show errors. (Maybe after having notices there for a few releases?)

#2 in reply to: ↑ 1 @xknown
11 years ago

Replying to nacin:

I agree on all counts.

What if we declared prepare() with a second argument, automatically causing E_WARNINGs? Might not be the prettiest for sites that show errors. (Maybe after having notices there for a few releases?)

I think is okay to add another required parameter. However, we can also remove the @ of @vsprintf( $query, $args ) in prepare(), which is the one that hides the PHP warning produced by this invalid uses.

#3 @nacin
11 years ago

Too few parameters also triggers a warning: vsprintf( '%s %s', array( 1 ) ).

@nacin
11 years ago

#4 @scribu
11 years ago

  • Cc scribu added

#5 @helenyhou
11 years ago

  • Keywords has-patch added

#6 @miqrogroove
11 years ago

In wp-db.php with a second parameter required, the call to isset() inside of prepare() is no longer required. Removing it is an option, although it would cause a second PHP debug message when called with only one param.

#7 @nacin
11 years ago

In 22428:

Remove single-argument calls to wpdb:prepare(), which are invalid as nothing is being prepared. see #22262.

#8 @nacin
11 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 22429:

Enforce a minimum of two arguments for wpdb::prepare(). The first argument is the query (or fragment thereof), which is required. Additional arguments are values to substitute into placeholders.

This will generate E_WARNINGs for insufficient arguments when prepare() is called with no additional arguments. This should discourage improper uses of prepare() under the guise of safely running a query.

props xknown. fixes #22262.

#9 @Otto42
11 years ago

Looks like somebody didn't tell Akismet. In Akismet admin.php:

$waiting = $wpdb->get_var( $wpdb->prepare( "SELECT COUNT(*) FROM $wpdb->commentmeta WHERE meta_key = 'akismet_error'" ) );

This ticket was mentioned in Slack in #core by sergey. View the logs.


6 years ago

Note: See TracTickets for help on using tickets.