Closed
Bug 373767
Opened 17 years ago
Closed 14 years ago
Switch to prepared statements for data access and get rid of mysql_real_escape_string/AMO->clean madness
Categories
(addons.mozilla.org Graveyard :: Public Pages, enhancement, P1)
Tracking
(Not tracked)
RESOLVED
WONTFIX
4.x (triaged)
People
(Reporter: jwkbugzilla, Unassigned)
References
()
Details
Currently AMO->clean tends to corrupt input data unnecessarily (try to enter quotes in the homepage field). It is also a bad solution because it depends on the developers actually using it and people tend to forget. It is far better not to use dynamic SQL anywhere. The SQL queries should be fixed and only contain placeholders for parameters. The database driver will take care of all the necessary escaping once parameters are bound. See the documentation on prepared statements: http://php.net/mysqli_stmt_prepare. The catch is: it only works with PHP 5.0 and higher. Same goes for the PDO extension which has similar functionality. So that this change won't happen any time soon I guess. Btw, what is blocking the switch to PHP5? According to http://manual.cakephp.org/chapter/installing Cake works with PHP5.
I agree, using prepared statements would make this problem go away in the general case, though I'm surprised that you can't use prepared statements with php4, given a new enough version of mysql. I think there is a php5 setup on the cluster now, though there wasn't when we started on Remora, it might be worth looking at that switch once we get through this initial release.
Comment 2•17 years ago
|
||
I concur. There are probably a handful of reasons why switching to PHP5 is a good idea, and I believe this is one of the bigger ones.
Comment 3•17 years ago
|
||
>
> I think there is a php5 setup on the cluster now, though there wasn't when we
> started on Remora, it might be worth looking at that switch once we get through
> this initial release.
>
Yep, php5 is available.
Updated•16 years ago
|
Target Milestone: --- → 3.4
Comment 4•16 years ago
|
||
This may not be possible because we use memcache to return cached results...
Target Milestone: 3.4 → 3.x (triaged)
Reporter | ||
Comment 5•16 years ago
|
||
Don't see why that would be a problem - AppModel.findAll() already serializes all function parameters into the cache key, AppModel.query() would do the same with query parameters.
Updated•16 years ago
|
Target Milestone: 3.x (triaged) → 4.0.1
Updated•16 years ago
|
Summary: Switch to prepared statements for data access and get rid of mysql_real_escape_string madness → Switch to prepared statements for data access and get rid of mysql_real_escape_string/AMO->clean madness
Comment 8•16 years ago
|
||
CakePHP 1.2 uses prepped statements so that should solve the problem when we upgrade (timeline TBA). This change will also require a rewrite of our query caching in memcache. Should probably be dependent on 425315, or at least done at the3 same time as that bug.
Updated•16 years ago
|
Target Milestone: 4.0.1 → 3.x (triaged)
Comment 9•15 years ago
|
||
When we pass unescaped data to the models (which is okay), and Cake will escape all its queries properly, we need to make sure the translation framework does so as well: The automatic saving/updating of translations currently relies on getting passed escaped data from the controller and does clean it up itself.
Updated•15 years ago
|
Priority: -- → P1
Comment 10•14 years ago
|
||
Zamboni to the rescue.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
Assignee | ||
Updated•8 years ago
|
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•