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)

enhancement

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.
Depends on: 373764
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.
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.
> 
> 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.
Target Milestone: --- → 3.4
This may not be possible because we use memcache to return cached results...
Target Milestone: 3.4 → 3.x (triaged)
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.
Target Milestone: 3.x (triaged) → 4.0.1
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
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.
Target Milestone: 4.0.1 → 3.x (triaged)
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.
Priority: -- → P1
Zamboni to the rescue.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.