Merge lp:~seif/zeitgeist/use-new-placeholders into lp:zeitgeist/0.1

[Seif Lotfy]

The SQLite docs say:
---
You shouldn’t assemble your query using Python’s string operations because doing so is insecure; it makes your program vulnerable to an SQL injection attack.

Instead, use the DB-API’s parameter substitution. Put ? as a placeholder wherever you want to use a value, and then provide a tuple of values as the second argument to the cursor’s execute() method.
---
This branch fixes it.

[Mikkel Kamstrup Erlandsen (kamstrup)]

I believe we use the current approach because the 'correct' one fails with
thousands of ids in the arguments
On Dec 7, 2010 3:01 PM, "Seif Lotfy" <email address hidden> wrote:
> Seif Lotfy has proposed merging lp:~seif/zeitgeist/use-new-placeholders
into lp:zeitgeist.
>
> Requested reviews:
> Zeitgeist Framework Team (zeitgeist)
>
>
> The SQLite docs say:
> ---
> You shouldn’t assemble your query using Python’s string operations because
doing so is insecure; it makes your program vulnerable to an SQL injection
attack.
>
> Instead, use the DB-API’s parameter substitution. Put ? as a placeholder
wherever you want to use a value, and then provide a tuple of values as the
second argument to the cursor’s execute() method.
> ---
> This branch fixes it.
> --
>
https://code.launchpad.net/~seif/zeitgeist/use-new-placeholders/+merge/42943
> Your team Zeitgeist Framework Team is requested to review the proposed
merge of lp:~seif/zeitgeist/use-new-placeholders into lp:zeitgeist.

[Siegfried Gevatter (rainct)]

2010/12/7 Mikkel Kamstrup Erlandsen <email address hidden>:
> I believe we use the current approach because the 'correct' one fails with
> thousands of ids in the arguments

Right.

Further, this change would give us no real benefit, since we're
already ensuring everything is a number.

Unmerged revisions

1645. By Seif Lotfy on 2010-12-07

Use ? placeholders instead of using Python's string operations. What we were doing is insecure

=== modified file '_zeitgeist/engine/main.py'
--- _zeitgeist/engine/main.py	2010-11-29 09:09:13 +0000
+++ _zeitgeist/engine/main.py	2010-12-07 14:01:20 +0000
@@ -169,7 +169,7 @@
 		rows = self._cursor.execute("""
 			SELECT * FROM event_view
 			WHERE id IN (%s)
-			""" % ",".join("%d" % id for id in ids)).fetchall()
+			""" % ",".join('?'*len(ids)), ids).fetchall()
 		
 		id_hash = defaultdict(list)
 		for n, id in enumerate(ids):
@@ -448,7 +448,7 @@
 			rows = self._cursor.execute("""
 				SELECT id, timestamp, subj_uri FROM event_view
 				WHERE id IN (%s)
-				""" % ",".join("%d" % id for id in pot)).fetchall()
+				""" % ",".join('?'*len(pot)), pot).fetchall()
 			
 			subject_uri_counter = defaultdict(int)
 			latest_uris = defaultdict(int)
@@ -600,7 +600,7 @@
 			SELECT MIN(timestamp), MAX(timestamp)
 			FROM event
 			WHERE id IN (%s)
-		""" % ",".join(str(int(_id)) for _id in ids))
+		""" % ",".join(["?"] * len(ids)), ids)
 		timestamps = self._cursor.fetchone()
 
 		# Make sure that we actually found some events with these ids...