WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 18 months ago

Last modified 16 months ago

#22262 closed defect (bug) (fixed)

Possible invalid uses of wpdb::prepare()

Reported by: xknown Owned by: 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 18 months ago.

Download all attachments as: .zip

Change History (10)

comment:1 follow-up: nacin18 months 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?)

comment:2 in reply to: ↑ 1 xknown18 months 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.

comment:3 nacin18 months ago

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

nacin18 months ago

comment:4 scribu18 months ago

  • Cc scribu added

comment:5 helenyhou18 months ago

  • Keywords has-patch added

comment:6 miqrogroove18 months 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.

comment:7 nacin18 months ago

In 22428:

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

comment:8 nacin18 months 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.

comment:9 Otto4216 months 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'" ) );
Note: See TracTickets for help on using tickets.