I have a view that has a "Contains Any Word" Filter on node title. Everything works fine. But after extensive testing I found that if a user had to search for a word starting with s or a words start with b then the query would break or come back with no results.

After doing some investigating I found it was any of the argument replacement values in _db_query_callback(). (%d, %s, %, %f, %b).

EG
If I had to search for the word 'search'

This
(UPPER(%s) LIKE UPPER('%%%s%%'))
Would become this
(UPPER(node.title) LIKE UPPER('nodeearch%')))

Later I was able to work out that by the time views_build_view() as calling pager_query() the arguments had already been replace but still $info['args'] were being passed into pager_query().

So from the if ($view->use_pager) { (because I was using a pager) I removed $info['args'] from the end of pager_query().
Then I got this.
(UPPER(node.title) LIKE UPPER('earch%')))

I don’t think views should be doing its own argument replacement? It is causing major problems with filters. With filters being user input there might be bigger problems that we just have not found yet.

I then circumvented _views_replace_args() by returning $clause, without being modified and let pager_query() do the argument replacement. From what I can see _views_replace_args() is only being used by _views_get_query() so we should be able to remove it no problems.

But I don’t know views well enough to really diagnose the problem.

I hope I explained that well enough for people to understand.

CommentFileSizeAuthor
#1 views_args_fix.patch1.69 KBNaX

Comments

NaX’s picture

Title: Filter arguments replacing incorrectly » Double $args replacing breaks queries with exposed filters
Version: 5.x-1.x-dev » 5.x-1.6
Status: Active » Needs review
StatusFileSize
new1.69 KB

Ok I have and a look around and did some of my own digging into the problem. I don’t know why this has not been fixed yet. It is a major problem. And the fix seems so simple. I think there could even be possibility for SQL Injection, but I have not investigated or tested for this.

I found the following open issues that found this problem a long time ago.

http://drupal.org/node/165611
http://drupal.org/node/165667

The problem is simple, args get replace twice into queries, once by views and once by db_query. I also think queries are being cached after args have been replaced and I don’t think that is correct.

So this attached patch is taken mostly form http://drupal.org/node/165611#comment-300462

It fixes the args replacing problem and the caching problem.

Once this patch is installed all your views should crash, you need to clear the views cache. From my testing this all works fine. I would really like a second opinion on this and I really think this needs to be fixed fast.

NaX’s picture

Ok I have and a look around and did some of my own digging into the problem. I don’t know why this has not been fixed yet. It is a major problem. And the fix seems so simple. I think there could even be possibility for SQL Injection, but I have not investigated or tested for this.

I found the following open issues that found this problem a long time ago.

http://drupal.org/node/165611
http://drupal.org/node/165667

The problem is simple, args get replace twice into queries, once by views and once by db_query. I also think queries are being cached after args have been replaced and I don’t think that is correct.

So this attached patch is taken mostly form http://drupal.org/node/165611#comment-300462

It fixes the args replacing problem and the caching problem.

Once this patch is installed all your views should crash, you need to clear the views cache. From my testing this all works fine. I would really like a second opinion on this and I really think this needs to be fixed fast.

moshe weitzman’s picture

Status: Needs review » Closed (duplicate)

I rerolled this patch so it applies cleanly and then moved it to http://drupal.org/node/165611 which was similar and already RTBC. Thanks for your work, Nax. I'll let earl commit it since i'm not sure how he is handling branches right now.