[drupal-devel] [feature] move check_query() into the database
interface files
Steven
drupal-devel at drupal.org
Wed Jun 1 03:40:11 UTC 2005
Issue status update for http://drupal.org/node/13180
Project: Drupal
Version: cvs
Component: database system
Category: feature requests
Priority: normal
Assigned to: chx
Reported by: chx
Updated by: Steven
Status: patch
Actually, Drupal does nothing to ensure the database character set is
set to UTF-8 (can it?). Thus, escaping with mysql_real_escape_string()
is not advised.
Steven
Previous comments:
------------------------------------------------------------------------
November 19, 2004 - 21:35 : chx
Attachment: http://drupal.org/files/issues/check_output_patch.txt (1.54 KB)
The check_query documentation says 'Prepare user input for use in a
database query, preventing SQL injection attacks.' But the code itself
is a simple addslashes. This is simply not correct -- for mysql, yes,
but the world is not just mysql. There are _escape_string functions for
that: mysql_escape_string, pg_escape_string, sqlite_escape_string...
Read the manual for pg_escape_string: "Use of this function is
recommended instead of addslashes()." Or read sqlite_escape_string:
"addslashes() should NOT be used to quote your strings for SQLite
queries; it will lead to strange results when retrieving your data."
Ooooops.
So I propose moving check_query() declaration from bootstrap.inc to
database.*.inc. I've already written a letter to the devel list about
this, but drumm said "code is better than talk" so here is a patch.
At this moment, this does absolutely nothing, only opens the
possibility to use the aforementioned escape_string functions.
But database.sqlite.inc simply will not work without this change.
------------------------------------------------------------------------
November 19, 2004 - 21:42 : drumm
Looks straightfoward enough.
Is there any reason to not put in pg_escape_string() now?
------------------------------------------------------------------------
November 19, 2004 - 21:47 : chx
Actually, yes. Read on the mentioned manual page: "Note: This function
requires PostgreSQL 7.2 or later." I have no idea how the check it.
There is a pg_version() but I will leave this postgres business to
someone who has a working postgres install to code and check this
thing.
------------------------------------------------------------------------
November 19, 2004 - 22:58 : Chris Johnson
PostgreSQL 7.2 was released February 4, 2002. Major production releases
73. and 7.4, along with many point releases, have been issued since
then. Current production is 7.4.6 and 8.0 is in the 4th revision of
beta.
It seems pretty safe to assume that any Drupal installation using
PostgreSQL would have version 7.2 or later. Document if a user
encounters errors with PostgreSQL that they need to check the version
is 7.2 or later.
------------------------------------------------------------------------
November 20, 2004 - 15:42 : Dries
+1
I suggest creating a db_escape_string() function, and removing
check_query() all together.
------------------------------------------------------------------------
November 20, 2004 - 15:55 : Goba
+1 on Dries
------------------------------------------------------------------------
November 20, 2004 - 19:32 : chx
Attachment: http://drupal.org/files/issues/check_output_patch_0.txt (1.6 KB)
db_escape_string sounds find to me, it's really a better name, thanks
Dries. But removing check_query? I'd suggest deprecating it and
removing it later. I am afraid It would break a lot of custom modules.
Here is a slightly modified patch with pg_escape_string included and
documented.
------------------------------------------------------------------------
November 20, 2004 - 19:43 : killes at www.drop.org
We were never afraid of breakign things for the good of the project.
Please don't change this policy.
------------------------------------------------------------------------
November 20, 2004 - 20:26 : chx
Attachment: http://drupal.org/files/issues/db_escape_string.txt (20.87 KB)
Let's do it.
------------------------------------------------------------------------
November 20, 2004 - 20:29 : chx
Attachment: http://drupal.org/files/issues/db_escape_string_0.txt (20.88 KB)
Something funny happened to the PostgreSQL db_escape_string comment and
I have missed it, sorry. Reposting the patch.
------------------------------------------------------------------------
November 21, 2004 - 10:29 : Dries
I committed the patch. Thanks chx. Keep it coming.
I've also updated the upgrade notes at http://drupal.org/node/12347.
I'll update some contributed modules unless someone objects.
------------------------------------------------------------------------
November 21, 2004 - 13:27 : chx
Attachment: http://drupal.org/files/issues/mysql_escape_patch.txt (572 bytes)
Keep it coming? Well, let's see the MySQL case... It's far from being
trivial, and I expect some controversy over it.
------------------------------------------------------------------------
March 10, 2005 - 01:36 : chx
Attachment: http://drupal.org/files/issues/mysql_escape_patch_0.txt (573 bytes)
OK, here is mysql version a bit corrected, this should get into 4.6 --
addslashes is not a correct way any more.
------------------------------------------------------------------------
March 10, 2005 - 20:29 : Dries
Can we read up on that somewhere?
------------------------------------------------------------------------
March 10, 2005 - 20:37 : Goba
Dries, sure: http://php.net/mysql_escape_string [1] and
http://php.net/mysql_real_escape_string [2]. The latter also uses the
connections charset settings to properly escape, so it is the best (but
is not available in old PHP versions). The addslashes page user notes
also have more information [3].
[1] http://php.net/mysql_escape_string
[2] http://php.net/mysql_real_escape_string
[3] http://hu.php.net/addslashes#20013
------------------------------------------------------------------------
March 12, 2005 - 10:50 : Dries
addslashes() has been working for ages (and is simpler code-wise). I
guess the mysql_ functions are slightly more secure or not even that?
That aside, I'm not really a big fan of version_compare()s. Also note
that the patch violates Drupal's coding standards (again) but that's
easy enough to fix.
------------------------------------------------------------------------
March 13, 2005 - 21:49 : chx
I can only repeat what Goba said: . mysql_real_escape_string uses the
connections charset settings to properly escape, so it is the best (but
is not available in old PHP versions).
More information about the drupal-devel
mailing list