Warn on double prepare?

[iandunn]

It's not an active vulnerability anymore, but maybe it should still be considered a bad practice, since it makes the code harder to reason about? Like escaping, a single prepare() at the point of query execution is more straight-forward.

https://blog.ircmaxell.com/2017/10/disclosure-wordpress-wpdb-sql-injection-technical.html https://make.wordpress.org/security/2017/11/13/the-war-on-sqli-or-what-happened-in-4-8-2-and-4-8-3/

$where = $wpdb->prepare(
	" WHERE foo = %s",
	$_GET['data']
);

$wpdb->get_results( $wpdb->prepare(
	"SELECT * FROM something $where LIMIT %d, %d",
	1,
	2
) );

[iandunn]

I think double prepares often result from building a query conditionally, but passing a static list of arguments to prepare(). A better practice IMO would be to pass an array of arguments, which is also built conditionally:

$where = '';
$prepare_values = array();

if ( $foo ) {
	$where = " WHERE foo = %s";
	$prepare_values[] = $_GET['data'] );
}

$prepare_values[] = 1;
$prepare_values[] = 2;

$wpdb->get_results( $wpdb->prepare(
	"SELECT * FROM something $where LIMIT %d, %d",
	$prepare_values
) );

That way the scanner (and humans) can easily see that $where is a literal string, and therefore safe.

That should definitely just be a warning, though, since there's plenty of existing secure code that double-prepares, including in Core.