WordPress-Coding-Standards icon indicating copy to clipboard operation
WordPress-Coding-Standards copied to clipboard

PreparedSQLPlaceholders.UnquotedComplexPlaceholder: allow for tablenames in backticks

Open jrfnl opened this issue 5 years ago • 7 comments

Is your feature request related to a problem?

Given the following code snippet:

$wpdb->query(
	$wpdb->prepare(
		'TRUNCATE TABLE `%1$s`',
		plugin_get_table_name( 'Name' )
	)
);

WPCS will currently throw the following warning:

WARNING | Complex placeholders used for values in the query string in $wpdb->prepare() will
          NOT be quoted automagically. Found: %1$s.
          (WordPress.DB.PreparedSQLPlaceholders.UnquotedComplexPlaceholder)

Describe the solution you'd like

In this particular case, I believe the warning should not be thrown. Instead the sniff should recognize the backticks as valid "quotes" for a table name.

Note: this should only be accepted for table names, so, the sniff should probably look for TABLE or FROM before the placeholder.

Some research may need to be done into the various SQL syntaxes to make sure that the sniff recognizes the correct keywords and doesn't miss any real unquoted placeholders.

jrfnl avatar Jun 21 '20 14:06 jrfnl

I think these should be allowed for column names as well.

mundschenk-at avatar Jun 23 '20 20:06 mundschenk-at

@mundschenk-at Good point. Agreed.

jrfnl avatar Jun 23 '20 20:06 jrfnl

I think these should be allowed for column names as well.

Yes, I totally agree with you. There should be a way to dynamically pass table name and column names otherwise it will just increase redundancy in code in CRUD operations for custom tables.

DevKishan avatar Aug 02 '20 09:08 DevKishan

Some research may need to be done into the various SQL syntaxes to make sure that the sniff recognizes the correct keywords and doesn't miss any real unquoted placeholders.

Just a quick note: Using backticks to quote identifiers is a MySQLism. I seem to remember some efforts to make WordPress use alternative DB backends, but since even the old Codex page asserts that doing so would be, ah, difficult, I assume that is not a relevant concern.

mundschenk-at avatar Aug 02 '20 10:08 mundschenk-at

@mundschenk-at Good point, but not something we should be concerned about until WP would support other DB backends. It does support MariaDB AFAIK, but that's a fork of MySQL, so syntax-wise they are compatible IIRC.

jrfnl avatar Aug 02 '20 14:08 jrfnl

Yes, for all practical purposes (as far as WordPress is concerned), MariaDB is MySQL.

mundschenk-at avatar Aug 02 '20 15:08 mundschenk-at

In WordPress 6.1 (October 2022), this won't be needed, as the %i placeholder has been added for Identifiers (table/field names):

$wpdb->prepare(
    'TRUNCATE TABLE %i',
    plugin_get_table_name( 'Name' )
);

This also avoids the issue where a developer may accidentally forget to add backtick quotes, or allow the user (attacker) to provide a value that contains backticks (which would need to escaped correctly), e.g.

$wpdb->prepare( 'WHERE `%1$s` IS NULL', $_GET['field'] ); // INSECURE, e.g. "WHERE `a`b` IS NULL"

Unfortunately this won't solve the problem for those still supporting 6.0 and below, so there might still be a case for ignoring this warning.

craigfrancis avatar Aug 16 '22 09:08 craigfrancis