I believe the central query in the user_roles function (found in modules/user/user.module) could be improved. Instead of passing in the permission variable in between two %'s, why attach these characters to the variable first and then query:

$result = db_query("SELECT r.* FROM {role} r INNER JOIN {permission} p ON r.rid = p.rid WHERE p.perm LIKE '%s' ORDER BY r.name", '%'.$permission.'%');

I don't believe there is a huge SQL injection security concern, but it does keep the variable parsing a bit more standard across queries. See the attached patch.

I ran into this as my employer, Biggs-Gilmore, has been doing several Drupal projects in the past few months for one of our main clients who uses Oracle 10g as their database back-end. We've had to port the Drupal base and additional modules to support this platform. In our implementation, all instances of '%s' are turned into proper bind variables. This query didn't fit that mold so we adjusted it. It may help in the long run when more PHP database APIs start supporting bind variables.

This "bug" is also in the 5.x version, which is actually the branch we are using more regularly.

CommentFileSizeAuthor
user_roles.patch608 bytescelstonvml

Comments

gpk’s picture

gpk commented
Component: user system » database system

This is very much "by design" at present and I can see 12 other places where core uses this syntax.

For the query in question, $permission is the argument - the variable part - that is to be substituted into the query and according to http://api.drupal.org/api/function/db_query/6, %% is the modifier used to get a single % in the resultant query - this corresponds to the synax of printf/sprintf. All this I'm sure you well know.

In the DB interface code for MySQL and PgSQL, both the original query in user_roles() and your modified version result in identical queries being passed to the DB server via mysql_query() or pg_query(). Probably ignorance is holdling me up here as my understanding of bind variables was nil till I just looked it up,* but would it not be possible for your interface code for Oracle to handle the syntax currently used in user_roles() and elsewhere? You could re-implement http://api.drupal.org/api/function/_db_query_callback/6 to trap %%%s%% (and %%%s and %s%% for that matter) to let you use bind variables without the need to change the Drupal syntax, which to my mind does have a certain logic.

TBH I can't see changes in the DB layer getting into 6.x (or 5.x) since I can't see the current approach being accepted as a bug, but if there are improvements needed to make support for Oracle easier then I'm sure 7.x would welcome them with open arms ;). I know others have been working on an Oracle interface layer - you may want to search around a bit. Also this should be of interest http://drupal.org/node/225450.

HTH...

* http://www.google.co.uk/search?hl=en&q=database+bind+variable&btnG=Googl...

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Status: Active » Closed (works as designed)
gpk’s picture

gpk commented
Version: 6.2 » 7.x-dev
Status: Closed (works as designed) » Fixed

This will be fixed by the new DB API layer in D7. See http://api.drupal.org/api/group/database/7 - %s syntax is going. As yet, not all queries in HEAD have been converted to use the new syntax (see http://api.drupal.org/api/function/_db_query_process_args/7).

Anonymous’s picture

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

Automatically closed -- issue fixed for two weeks with no activity.