Add sniff to ensure that $wpdb->query() is passed prepared SQL

[westonruter]

[Rarst]

Criteria for preparedness? Insisting on nested prepare() method won't work nicely (false positives where non-issue).

[westonruter]

Here's an example that should be caught when the WordPress-Extra standard is used:

$wpdb->query( "SELECT * FROM $wpdb->posts WHERE post_title LIKE '{$_GET['title']}';" ); // BAD!!! SQL INJECTION

Any variables used inside of a string (aside from $wpdb itself, of course) passed to a $wpdb query method as SQL should get flagged as errors.

[Rarst]

I disagree on "any variable". Variable might be previously sanitized. Simply put something like this won't work without inferring type and content of the variable reliably and static analysis isn't good for that.

It just will be same issue as trying to sniff escaping in templates — it is highly unreliable. Which results in annoying false positives, which results in it being preferable to disable sniff than bother with it.

I don't mind it too much in extra set, but my concern is that one can open file, get nothing but bunch of all positives and ditch it altogether as useless. I already have to maintain a custom standard, throwing some sniffs out (like WordPress.XSS.EscapeOutput) because they are impractical like that.

[westonruter]

I disagree on "any variable". Variable might be previously sanitized.

It is a best practice to escape as late as possible.

[Rarst]

I know that perfectly well. I am just questioning suitability and capability of CodeSniffer to detect it.

1. There should be spaces inside of function declaration and there are none = good deterministic sniff.
2. I have no idea what in this variable or what it means, but it shouldn't be here because = vague unreliable sniff.

[westonruter]

By forcing all variables passed into a SQL string to be sent through $wpdb->prepare() then there is no question about whether it is safe or not. It can be checked statically. Anyway, like you said, it's Extra.