QIT security throws false positives regarding $wpdb->prepare usage.

[leewillis77]

See example code below:

$sql = 'DELETE FROM %i WHERE locale = %s';
$wpdb->query(
    $wpdb->prepare( $sql, [ $table, $locale ] )
);

I'd expect that code to throw no errors, but it reports 'Use placeholders and $wpdb->prepare(); found $sql' / 'WordPress.DB.PreparedSQL.NotPrepared'

[Luc45]

Hi @leewillis77 , thanks for the ping.

QIT utilizes PHP Code Sniffer with WordPress Coding Standards to detect issues, which is an industry standard in the WordPress ecosystem.

It's important to understand the inherent limitations of static code analysis. When the tool parses the code, it identifies the following:

• $wpdb (an instance of \wpdb)
• Method invocation: query
• The method query receives \wpdb::prepare as it's first parameter
• And at this point, it encounters a variable $sql, and unlike humans, the parser lacks the context to safely interpret this variable. To the parser, $sql could potentially incorporate user input or read directly from the database.

Given that the parser cannot ascertain the safety of $sql due to its inability to comprehend the surrounding context, it flags this as a potential issue.

This limitation is intrinsic to static code analysis on any language, on all softwares of the world. Be it PHP, WordPress, Java, Ruby, you name it, that's how it is for static code analysis.

A quote from an OWASP article (Open Worldwide Application Security Project):

Ideally, such tools would automatically find security flaws with a high degree of confidence that what is found is indeed a flaw. However, this is beyond the state of the art for many types of application security flaws. Thus, such tools frequently serve as aids for an analyst to help them zero in on security relevant portions of code so they can find flaws more efficiently, rather than a tool that simply finds flaws automatically.

To navigate around these tooling limitations, I recommend the following approaches:

Refactor the Query: If possible, rewrite the SQL query directly in the prepare statement, like so:

$wpdb->query(
    $wpdb->prepare( 'DELETE FROM %i WHERE locale = %s', [ $table, $locale ] )
);

Suppress the Warning: If rewriting the query isn't feasible or if you are certain your dynamic SQL statement construction is secure, you can suppress the specific warning for this line of code using a comment. Remember that this should be used sparingly, as overriding the linter bypasses the safety checks it is designed to provide:

// phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared -- Safe, static value used for SQL.
$wpdb->query(
    $wpdb->prepare( $sql, [ $table, $locale ] )
);

Suppresing warnings is also a form of verification, which is a valid "fix". However, it's recommended to refactor the query to be static code analysis friendly, so that if the query changes in the future and introduces a security issue, it can be picked up.

[leewillis77]

Noted. I actually thought I was running that sniff locally (so was confused about why QIT was flagging it when I wasn't getting anything locally). Turns out I'd excluded that sniff for precisely that reason. I'll go through and manually whitelist / refactor where I can. Thanks for the detailed reply!