WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 2 months ago

Last modified 2 months ago

#25604 closed enhancement (fixed)

first argument of wpdb::prepare should have a placeholder

Reported by: ounziw Owned by: nacin
Milestone: 3.9 Priority: normal
Severity: normal Version:
Component: Database Keywords: has-patch commit
Focuses: Cc:

Description

wpdb::prepare is a function which escapes for SQL.

Some plugins/themes use wpdb::prepare in a wrong way, and they may have a vulerability.

Bad Example
$wpdb->prepare( "SELECT * FROM table WHERE id = $id", null);

I propose to add

		if ( strpos($query,'%') === false || strpos($query,'$') !== false )
			_doing_it_wrong( 'wpdb::prepare', 'first argument of wpdb::prepare() should have a preceholder.', '3.7' );

to check the first argument of wpdb::prepare

Change History (15)

comment:1 in reply to: ↑ description ounziw6 months ago


		if ( strpos($query,'%') === false || strpos($query,'$') !== false )
			_doing_it_wrong( 'wpdb::prepare', 'first argument of wpdb::prepare() should have a preceholder.', '3.7' );

it should be

 		if ( strpos($query,'%') === false && strpos($query,'$') !== false )
 			_doing_it_wrong( 'wpdb::prepare', 'first argument of wpdb::prepare() should have a preceholder.', '3.7' );

(If no %, it means no placeholder, and if there is $, it must be a variable which should be escaped)

comment:2 nacin6 months ago

I like the idea of checking for the complete absence of a placeholder. The check for $ won't work (the variable will already be parsed by then).

comment:3 ounziw6 months ago

Thanks, Nacin.

I removed the $ check.

 		if ( strpos($query,'%') === false )
 			_doing_it_wrong( 'wpdb::prepare', 'first argument of wpdb::prepare() should have a preceholder.', '3.7' );

comment:4 rmccue6 months ago

preceholder should be placeholder and first should be capitalised.

comment:6 jdgrimes4 months ago

  • Cc jdg@… added

comment:7 nacin3 months ago

  • Component changed from Warnings/Notices to Database
  • Keywords has-patch needs-refresh added
  • Milestone changed from Awaiting Review to 3.9

Looking good. This patch needs to be updated a bit. Namely:

  • Whitespace inside strpos() parentheses and after the comma to match our coding standards.
  • The string should probably be translated by being wrapped in the __() function.
  • Braces should be used for this if conditional (recent coding standards change).
  • Version number will need to be updated to 3.9.
  • __METHOD__ can be used in lieu of 'wpdb::prepare'.

comment:8 ounziw3 months ago

Thanks for suggestion, nacin.

I updated my patch.

comment:9 nacin3 months ago

  • Keywords commit added; needs-refresh removed

I'd like to review the text of the warning itself but this is otherwise good. Thanks!

comment:10 nacin2 months ago

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

In 27073:

Throw an incorrect usage notice when the query argument of wpdb::prepare() does not include a placeholder.

props ounziw.
fixes #25604.

comment:11 nacin2 months ago

In 27074:

Add a comment for [27073] as someone will inevitably complain it is tricked by % in a string. see #25604.

Note: See TracTickets for help on using tickets.