string formatting used for IN operator in some mware classes

VERIFIED FIXED in 71

Status

Socorro Graveyard
Middleware
VERIFIED FIXED
4 years ago
a year ago

People

(Reporter: rhelmer, Assigned: peterbe)

Tracking

unspecified

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
I noticed that some classes are using string formatting instead of parameterized queries, presumably to support the "IN" operator:

https://github.com/mozilla/socorro/blob/master/socorro/external/postgresql/bugs.py#L38

However according to the docs (e.g. http://initd.org/psycopg/docs/usage.html) and a little test I just ran this should work:

params = ((1,2),)
stmt = "SELECT * FROM test WHERE col1 IN %s"
print cur.mogrify(stmt,params)

Output is:

SELECT * FROM test WHERE col1 IN (1, 2)

Alternatively you could do:

params = [1,2]

And psycopg2 will output:

SELECT * FROM test WHERE col1 IN ARRAY[1, 2]

Is there a reason not to do it this way? I am a lot more comfortable with Postgres doing the paramerization and not doing it with Python strings ;)
(Reporter)

Comment 1

4 years ago
See comment 0
Flags: needinfo?(adrian)
So there's no security issue with that example because what it does is simply building a list of parameters (like "signature%1, signature%2") that psycopg will then securely replace with actual values. However, the code that you are showing looks much better than the current code.
Flags: needinfo?(adrian)
(Assignee)

Comment 3

4 years ago
Yes. I agree. We should always use psycopg's serializer. I think the trick is that it needs to be a tuple. 

I think the code in bugs.py should be refactored. There's no need to make it things like `signature%1`, right?
(Assignee)

Updated

4 years ago
Assignee: nobody → peterbe
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
Created attachment 8355659 [details] [review]
Github PR

Comment 5

4 years ago
Commit pushed to master at https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/e2639de48a502868f597d27547e0e40995ac5782
fixes bug 950385 - use psycopg's IN operator serializer, r=adrian

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Whiteboard: [qa-]

Updated

4 years ago
Target Milestone: --- → 71
Bumping to verified as [qa-]
Status: RESOLVED → VERIFIED
Product: Socorro → Socorro Graveyard
You need to log in before you can comment on or make changes to this bug.