check for improper use of %s

I must respectfully disagree:

$join = 'JOIN {term_node} td USING (nid)';
$where = 'AND td.tid IN ('. implode(', ', $terms) .')';
$result = db_query("SELECT * FROM {node} n %s WHERE n.type='faq' AND n.status=0 %s", $join, $where);

Quotes would mess this up.

$join = 'JOIN {term_node} td USING (nid)';
$where = 'AND td.tid IN ('. implode(', ', $terms) .')';
$result = db_query("SELECT * FROM {node} n %s WHERE n.type='faq' AND n.status=0 %s", $join, $where);

This is exactly the case coder should flag! %s is totally inappropriate here. And imploding $terms (who know what it contains) into the query is at best a weak point, at worst an actual vulnerability.

Make $where contain the right number of placeholders (%d) the just add $join and $where to the query and add $terms as array argument (follows the example, can be more efficient):

$join = 'JOIN {term_node} td USING (nid)';
$where = 'AND td.tid IN ('. implode(',', array_fill(0, count($terms), '%d')) .')';
$result = db_query("SELECT * FROM {node} n $join WHERE n.type='faq' AND n.status=0 $where', $terms);

I did look at the post with that patch. One problem here is that $terms is not always used in the query. There are cases when $where remains null, therefore $terms would not be used.

You can easily solve that differently.

A warning flag for %s and $var in queries has my support.

I agree with Heine, a warning should be produced for unquoted %s in queries. There's a new function, db_placeholders(), in D6 which is meant to help with this. See new helper function: db_placeholders for more info.

I've attached a patch for coder_6x.inc that should hopefully match unquoted %s. The detection of variable usage was already present. You may want to add it to coder_security instead, but the db_placeholders() function only exists in D6.

Cheers,
Stella

Coder is not just for D6 and I'm still coding for D5. But I will capitulate and bloat my code and execution time to avoid the message, and potential problems mentioned in the other post, even though it would take an extraordinary hacker with impeccable timing to damage these values.

[EDIT] Doug: I saw that post in the Developers' list that seemed to be directed at my less-than-optimal wording. I changed it.

To anyone: does db_query ignore optional parameters that aren't used? For example: db_query('SELECT * FROM {x} WHERE colname=%d', $p1, $p2)

I need to modify this patch to catch another few use cases. Bumping this so I don't forget!

Added in some extra use cases and moved it to the security rule set.

Committed.

Cheers,
Stella