Closed Bug 676165 Opened 13 years ago Closed 8 years ago

Flag SQL statements with non-static strings

Categories

(addons.mozilla.org Graveyard :: Add-on Validation, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2015-02

People

(Reporter: kmag, Assigned: kmag)

Details

(Whiteboard: [ReviewTeam:P2])

SQL injection is the bane of server-side web development, and now, alas, your client code can suffer too. Given that the preferred method of creating dynamic SQL statements is to use placeholders, I think the following methods should be flagged if they're called using non-static strings:

   createStatement
   createAsyncStatement
   executeSimpleSQL

They may as well be flagged as universal methods. The names are fairly unambiguous, and there are too many ways to get and store a database connection.

The message should be something along the lines of:

Dynamically creating SQL statements is a common cause of errors, including SQL injection vulnerabilities. Please use placeholders instead: https://developer.mozilla.org/en/storage#section_8
jorgev for decisions/wording.  Please give to mbasta when finished.
Assignee: nobody → jorge
The warning message looks good, and I agree that we should flag this. I'm not sure how accurate we can be detecting non-static strings, but as long as we cover most cases, it should be a win.
Assignee: jorge → mattbasta
Component: Developer Pages → Add-on Validation
QA Contact: developers → add-on-validation
Whiteboard: [required amo-editors]
Matt, is this possible?
We already do something similar for functions like createElement.

From the docs, I gather that you create a database connection with:

    Services.storage.openDatabase()

Is that the only way?
The doc indicates 3 open functions:
https://developer.mozilla.org/en/XPCOM_Interface_Reference/mozIStorageService

You can get the service from the Services object, or getting the XPCOM service directly, using getService.
There are a few builtin services that return them from the DBConnection property.

I suspect it would be easiest and most effective to just implement these as universal methods. Most of the SQL code I see passes the relevant objects as function parameters.
I feel dirty implementing them as instance actions/properties, because as we add more and more of them, it becomes more and more likely that add-on developers will start seeing false positives because of unfortunately named functions.
I'd rather deal with that problem when we come to it. The names are sufficiently distinct I don't expect false positives to be nearly as much of an issue as false negatives from trying to be too smart about it.
Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...
Whiteboard: [required amo-editors] → [ReviewTeam:P2]
Assignee: mattbasta → kmaglione+bmo
Target Milestone: --- → 2015-02
Product: addons.mozilla.org → addons.mozilla.org Graveyard
This pull request was merged.

https://github.com/mozilla/amo-validator/pull/256
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.