Say, I have a query that selects nodes using an OR expression. For example:

db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.created > 123456789 OR n.updated > 123456789");

Now, this query doesn't make much sense, but is very well possible to build a sensible query with some joined tables and an OR.

When I run this query through db_rewrite_sql I expect the resulting query to only ever return nodes the user has access to. Take a look at what the resulting query is:

SELECT DISTINCT(n.nid) FROM {node} n INNER JOIN {node_access} na ON na.nid = n.nid WHERE (na.grant_view >= 1 AND ((na.gid = 0 AND na.realm = 'all'))) AND n.created > 123456789 OR n.updated > 123456789

so

SELECT ... WHERE [node access] AND n.created > 123456789 OR n.updated > 123456789

As you can see, this query will return nodes that have been updated after 123456789 whether the user can access them or not.

Two possible solutions:

  • document the fact that you should always add brackets to the expression after WHERE when using db_rewrite_sql;
  • make db_rewrite_sql add brackets to the existing WHERE expression before appending additional stuff.

This bug applies at least to Drupal 4.7 and 5.0.

CommentFileSizeAuthor
#11 db_rewrite_sql_head_1.patch1.25 KBdecafdennis
#10 db_rewrite_sql_head_0.patch1.41 KBdecafdennis
#6 db_rewrite_sql_bench.csv7.55 KBdecafdennis
#5 db_rewrite_sql_47.patch1.88 KBdecafdennis
#4 db_rewrite_sql_head.patch1.86 KBdecafdennis
#2 db_rewrite_2.patch415 byteschx
#1 db_rewrite_1.patch414 byteschx

Comments

chx’s picture

chx commented
Priority: Normal » Critical
Status: Active » Needs review
StatusFileSize
new db_rewrite_1.patch414 bytes

Critical because of the security implications.

chx’s picture

chx commented
StatusFileSize
new db_rewrite_2.patch415 bytes

Better patch :)

decafdennis’s picture

decafdennis commented
Assigned: Unassigned » decafdennis
Status: Needs review » Needs work

Above patches don't fix this issue.

I'm working on a patch, if that doesn't appear to be a viable solution we'll document the needed parentheses.

decafdennis’s picture

decafdennis commented
Status: Needs work » Needs review
StatusFileSize
new db_rewrite_sql_head.patch1.86 KB

Here's a patch, the whole of the $where and $join insertion logic is changed. It doesn't look very pretty, but it works.

I tested it reasonably extensive and it seems to rewrite all queries correctly. Only difference with what the current function produces is whitespace and the added parentheses to the existing WHERE part. I even did some quick benchmarking using 20000 samples for 14 different queries, and in the worst case scenario (there is already a WHERE and just LIMIT was used, not GROUP BY or ORDER BY) the new function takes about 4% longer.

Here are patches for HEAD and DRUPAL-4-7 and my extremely scientific benchmark results.

decafdennis’s picture

decafdennis commented
StatusFileSize
new db_rewrite_sql_47.patch1.88 KB

Patch for 4.7.

decafdennis’s picture

decafdennis commented
StatusFileSize
new db_rewrite_sql_bench.csv7.55 KB

Primitive benchmark.

decafdennis’s picture

decafdennis commented
Status: Needs review » Needs work

Declared evil by chx.

cog.rusty’s picture

cog.rusty commented

Yes... the time is not right for meta-rewrites.

chx’s picture

chx commented

Not evil , but the code is really ugly. Sorry. Whether this can or need to be fixed in Drupal 5 is not mine to decide.

decafdennis’s picture

decafdennis commented
StatusFileSize
new db_rewrite_sql_head_0.patch1.41 KB

I do know it's ugly, I said so myself it isn't pretty.

Here's another one using a regular expression. Not much prettier I must say. I still need to have some ifs because there's no such thing as conditional backreferencing or anything like that. I need to determine whether to add AND, for example.

decafdennis’s picture

decafdennis commented
Status: Needs work » Needs review
StatusFileSize
new db_rewrite_sql_head_1.patch1.25 KB

This time a pretty patch, conceived by chx.

decafdennis’s picture

decafdennis commented

Same patch can and should be used for 4.7.

+1 from me, I've already patched my production site (jantinbergencollege.nl) and it works great.

dries’s picture

dries commented
Status: Needs review » Patch (to be ported)

I committed this patch to CVS HEAD, although it could do with more testing. There are lots of small corner cases so hopefully we find any bugs real soon.

It should probably be backported to Drupal 4.7 once it has been proven to be rock stable.

pwolanin’s picture

pwolanin commented

For 5.x, Modr8 uses db_rewrite_sql() to prevent nodes in moderation from being listed at /node, in the tracker, etc. (but is not actually a node access module). This patch seems to work fine with Modr8.

dries’s picture

dries commented

We're also testing this patch on groups.drupal.org now ... so far it seems to work with OG but the site might be a little too busy to tell for sure ...

dries’s picture

dries commented
Version: 5.x-dev » 4.7.x-dev

No problems in more than 24 hours -- looks good. I'm going to switch the version.

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Patch (to be ported) » Fixed

applied

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)