If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Switch to prepared statements for data access and get rid of mysql_real_escape_string/AMO->clean madness

RESOLVED WONTFIX

Status

addons.mozilla.org Graveyard
Public Pages
P1
enhancement
RESOLVED WONTFIX
11 years ago
2 years ago

People

(Reporter: Wladimir Palant, Unassigned)

Tracking

4.x (triaged)

Details

(URL)

(Reporter)

Description

11 years ago
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.

Updated

11 years ago
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.

Comment 2

11 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.
> 
> 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

10 years ago
Target Milestone: --- → 3.4
This may not be possible because we use memcache to return cached results...
Target Milestone: 3.4 → 3.x (triaged)
(Reporter)

Comment 5

10 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

9 years ago
Target Milestone: 3.x (triaged) → 4.0.1

Updated

9 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

Updated

9 years ago
Duplicate of this bug: 431399

Updated

9 years ago
Duplicate of this bug: 435022

Comment 8

9 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.
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
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
(Assignee)

Updated

2 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.