[wp-trac] [WordPress Trac] #54042: Extending wpdb::prepare() to support IN() operator (was: Extending wpdb::prepare() to support table/field names, and IN() operator)

[WordPress Trac]

#54042: Extending wpdb::prepare() to support IN() operator
-------------------------------------------------+-------------------------
 Reporter:  craigfrancis                         |       Owner:  (none)
     Type:  enhancement                          |      Status:  new
 Priority:  normal                               |   Milestone:  Future
                                                 |  Release
Component:  Database                             |     Version:
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch dev-feedback needs-        |     Focuses:
  testing early has-unit-tests                   |
-------------------------------------------------+-------------------------
Description changed by craigfrancis:

Old description:

> wpdb::prepare() has really helped avoid SQL Injection vulnerabilities,
> ensuring most variables are escaped correctly.
>
> But it does not support table/field names, so developers need to
> implement their own escaping, which is not always done safely.
> [https://github.com/WordPress/WordPress/blob/988c8be693e0d12aeacae30f1d4bebfb98f7e5a0
> /wp-includes/wp-db.php#L2778 If this example was copied], with no other
> checks, there could be an issue if `$table` included backtick or other
> invalid characters:
>
> {{{#!php
> <?php
> $table_parts = explode( '.', $table );
> $table       = '`' . implode( '`.`', $table_parts ) . '`'; // INSECURE?
> }}}
>
> Likewise, it's also fairly common to make a mistake when including values
> with the `IN ()` operator, for example:
>
> {{{#!php
> <?php
> $where = 'WHERE id IN (' . implode( ',', $ids ) . ')'; // INSECURE?
> }}}
>
> Developers need to be sure that `$ids` has come from a trusted source, or
> use something like `wp_parse_id_list()` or `array_map('intval', $ids)`.
>
> ----
>
> Considering the `wpdb::prepare()` documentation says it "Uses
> sprintf()-like syntax", could we add a couple of placeholders to safely
> include these values? e.g.
>
> {{{#!php
> <?php
> $wpdb->prepare('SELECT * FROM %i WHERE ID IN (%...d)', $table, $ids);
> }}}
>
> Where `%i` would be used for Identifiers (e.g. table/field names), where
> the value would be quoted with backticks, and invalid characters removed.
>
> And `%...d` or `%...s` would safely (and easily) include a comma
> separated array of integers or strings - taking the idea of using '...'
> for variadics in PHP.
>
> https://dev.mysql.com/doc/refman/8.0/en/identifiers.html
> https://dev.mysql.com/doc/refman/8.0/en/comparison-
> operators.html#operator_in

New description:

 wpdb::prepare() helps avoid SQL Injection vulnerabilities, by escaping
 most variables correctly.

 WP 6.1 added support for Identifiers (table/field names) with `%i`, in
 #52506.

 But it's also fairly common to make a mistake to include values with the
 `IN()` operator, for example:

 {{{#!php
 <?php
 $where = 'WHERE id IN (' . implode( ',', $ids ) . ')'; // INSECURE?
 }}}

 Developers need to be sure `$ids` has come from a trusted source, or use
 something like `wp_parse_id_list()` or `array_map('intval', $ids)`.

 ----

 Maybe support could be added with:

 {{{#!php
 <?php
 $wpdb->prepare('WHERE id IN (%...d)', $ids);
 }}}

 Where `%...d` or `%...s` would safely (and easily) include a comma
 separated array of integers or strings - taking the idea of using '...'
 for variadics in PHP.

 https://wiki.php.net/rfc/variadics
 https://www.php.net/manual/en/functions.arguments.php#functions.variable-
 arg-list
 https://dev.mysql.com/doc/refman/8.0/en/comparison-
 operators.html#operator_in

--

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/54042#comment:38>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform