False positive for interpolated table names (WordPress.DB.PreparedSQL.NotPrepared)

[mundschenk-at]

trafficstars

Bug Description

Interpolated table names that are not $wpdb properties incorrectly raise WordPress.DB.PreparedSQL.NotPrepared.

What did you expect to happen? What actually happened? Interpolated table names in FROM clauses should be ignored.

Minimal Code Snippet

$table = 'foo';
$id      = 6;
$wpdb->prepare( "SELECT * FROM `{$table}` WHERE id = %d", $id);

Error Code

WordPress.DB.PreparedSQL.NotPrepared

Environment

Question	Answer
PHP version	7.2.12
PHP_CodeSniffer version	3.4.0
WPCS version	1.2.1
WPCS install type	Composer project local

Tested Against develop branch?

• [ ] I have verified the issue still exists in the develop branch of WPCS.

[dossy]

Also, if you need to interpolate in the database name (you can't let prepare() wrap it it with single-quotes), like this:

    $wpdb->prepare( 'SELECT ... FROM ' . className::CONSTANT_VAR . '.tablename WHERE ...' )

will also raise a WordPress.DB.PreparedSQL.NotPrepared, but there's really no good way around that -- maybe using sprintf() as the arg to that prepare(), but then you have to double encode the % for the actual placeholders that you want prepare() to interpolate ... :-(

[jrfnl]

@dossy When interpolating the database name, you should use something along the lines of "... FROM {$wpdb->prefix}tablename ..." or "... FROM $wpdb->wp_native_tablename ...". The sniff will not trigger on those.

classname::CONSTANT_VAR can never contain the user-defined $table_prefix as defined in wp-config.php as variables are not allowed in the initial value of constants and constants can not be changed, so I don't know what you're trying to do here, but this seems like a case of doing it wrong™ to begin with.

There is a way round the WPCS error, but I don't think it's valid in your use-case as, as I say above, you're doing it wrong no matter what, so using a work-around would be compounding the code error.

[mundschenk-at]

@jrfnl I can't comment on @dossy's use case, but I'd like to note that using ... FROM {$wpdb->prefix}tablename ... as a workaround for my initial report is not feasible (without code duplication) for plugins that can use either per-site or global tables in a multisite environment.

[dossy]

@jrfnl I'm dealing with queries that are pulling in data from outside of WordPress entirely, in a different database, whose name we're storing as a class constant.

Why must I instantiate an object and use an object member variable when I'm dealing with an actual constant?

I understand the rationale behind the WPCS sniff - to prevent people from unintentionally introducing SQL injection errors into their code and queries - and, yes, I wrap the queries in phpcs:enable/phpcs:disable for the WordPress.DB.PreparedSQL.NotPrepared sniff. In the end, I'll say that the benefit that this sniff provides far outweighs the minor inconvenience of having to disable the sniff around a few queries.

[jonshipman]

Perhaps introducing a new sniff that only whitelists table name variables? Like using the example from @mundschenk-at: ... FROM {$wpdb->prefix}tablename ... could become ... FROM {$wpdb->prefix}{$table_name} ... // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared.TableName where the sniff looks as the variable ceding the $wpdb->prefix variable.

This would allow people to specifically ignore table names while not blanket ignoring the whole prepared statement.

[EvanShaw]

@jonshipman is spot on. {$wpdb->prefix}{$table_name} should work.

@jrfnl {$wpdb->prefix}tablename does make the error go away, but referring to the table with a literal in this way only makes sense if you are accessing this table one time in your entire project. Writing tablename in every query where it is needed introduces duplication and makes it difficult to refactor if the tables name ever changes. I'd be surprised if saving custom table names as constants/variables ($wpdb->prefix excluded) was NOT the norm for developers.

[jrfnl]

I'd like to suggest closing this issue in favour of encouraging people to start using %i for table names once it is available (likely to be in WP 6.2).

Refs:

• https://core.trac.wordpress.org/ticket/52506
• https://github.com/WordPress/WordPress-Coding-Standards/pull/2072

[jrfnl]

Closing as the %i placeholder feature was shipped in WP 6.2.

Refs:

• https://make.wordpress.org/core/2022/10/08/escaping-table-and-field-names-with-wpdbprepare-in-wordpress-6-1/
• https://make.wordpress.org/core/2022/10/31/postponed-to-wp-6-2-escaping-table-and-field-names-with-wpdbprepare/

[eddr]

The "%i" approach doesn't work when submitting a new extension via WooCommerce vendors dashboard..

[jrfnl]

@eddr What do you mean ? and what does that have to do with WPCS ?

[eddr]

Hi, apologies for being unclear

WooCommerce has a marketplace which you can submit extension to, via this link https://woocommerce.com/vendor-signup/

It uses automatic testing of the plugins, based on WPCS rules However, it doesn't allow exclusions ( like phpcs comments ), using " --warning-severity=0 --ignore-annotations" parameters So, the result is that one cannot submit a new extension if using - for example - the $wpdb->prepare code

• I tried to suggested approaches here and they do not work
• Including "%i" which WC automatic testing does not recognize for some reason
• And including using "{$wpdb->prefix}{$table_name}"

Any clue?

[jrfnl]

@eddr Well, first clue would be to report this to the WooCommerce market place.

Second clue would be that the WPCS support for the %i feature has not been released yet and is only available in the develop branch and I suspect the WooCommerce market place may be using the last stable release.

Either way, this is not something we can help you with as it is not a WPCS problem, but a problem with how the WooCommerce market place uses WPCS.

[eddr]

Just want to make sure I'm not mistaken - there is no way to bypass this issue when these parameters are in use, right?

Thanks