Posted by douggreen on August 29, 2008 at 2:13pm
| Project: | Coder |
| Version: | 6.x-2.x-dev |
| Component: | Review/Rules |
| Category: | task |
| Priority: | normal |
| Assigned: | douggreen |
| Status: | closed (fixed) |
Issue Summary
LIVE FROM DRUPALCON SZEGED. This idea came out of a session that Ken Rickards gave on node_access.
When you do a "SELECT FROM {node}", you should typically also wrap this in db_rewrite_sql. Since this is a security issue, but not a hard and fast rule, the rule should say something like "you are probably doing something wrong."
Comments
#1
Please try the attached rule.
#2
That patch doesn't work as it also alerts on queries which do use db_rewrite_sql(). I've re-rolled it with this change, please review.
Cheers,
Stella
p.s. looks like this will all be changing in D7, see http://drupal.org/node/310077
#3
Updated patch to use the new test structure, and moved it from the sql review to the security review.
#4
... and the patch !
#5
Looks good to me!
#6
The rule looks like it would miss queries that join the node table:
<?php$results = db_query("SELECT * FROM {mytable} t INNER JOIN {node} n ON t.nid = n.nid");
?>
Also, there is this:
+ $this->assertCoderPass(' $results = db_query(db_rewrite_sql("SELECT * FROM {node}"));');Ideally, that test should actually fail - since db_rewrite_sql also needs to know the correct alias for the node table, which it assumes to be 'n' unless you tell it otherwise. So:
<?php$results = db_query(db_rewrite_sql("SELECT * FROM {node} n")); // right
$results = db_query(db_rewrite_sql("SELECT * FROM {node} foo", 'foo')); // also right
$results = db_query(db_rewrite_sql("SELECT * FROM {node}", '{node}')); // also right, but rare
$results = db_query(db_rewrite_sql("SELECT * FROM {node}")); // wrong
?>
That would probably be a separate rule though; I just wanted to point out the bad test here.
#7
Hmm, and apparently SELECT * is wrong too, as the primary field has to actually exist in the query or db_distinct_field() will fail.
#8
revised rules and tests committed to 6.x-2.x and 7.x branches.
#9
Automatically closed -- issue fixed for 2 weeks with no activity.