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]
Product: addons.mozilla.org → addons.mozilla.org Graveyard
This pull request was merged. https://github.com/mozilla/amo-validator/pull/256
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.