taskcluster / ec2-manager

Mozilla Public License 2.0
2 stars 14 forks source link

Consider using knex.js for SQL #44

Open jonasfj opened 6 years ago

jonasfj commented 6 years ago

dustin and I were briefly scared of possible SQL injections, but it seems like this is NOT possible :)

looking at some of the code, notably: https://github.com/taskcluster/ec2-manager/blob/e3cd590f5a39fc4ccd329be25aa30dec7382c1c9/lib/state.js#L610-L642 It's really hard to read, and takes a while to ensure that the string interpolations are valid. Instead of doing SQL this way maybe consider knex.js: http://knexjs.org/

it's a pretty elegant way to write SQL, and it helps prevent accidental SQL injections... But really, I suspect it would make the code nicer and easier to read, which is probably the main benefit.

djmitche commented 6 years ago

It does appear that table and condition are always supplied as hard-coded values from elsewhere in the code -- only the value and conditions[condition] are user-supplied, and those end up in the list of parameter values. So this is safe as written! But those conditions come from api.js and it's not obvious there that any user input put into an object key would potentially allow SQL injection.

djmitche commented 6 years ago

I'm going to give this a try. If knex is good, then we will want to use it consistently (and thus in ec2-manager). If it's not good, this is a good way to find out :)

djmitche commented 6 years ago

https://public.etherpad-mozilla.org/p/dustin-knex-notes