Parameterized Dynamic Queries / SQL Sanitation NodeJS

Question

I am fairly new to node and have a problem with dynamic parameterized queries using sqlstring.

The issue with the below code is that filters are optional depending on what a user passes into the function so the order of them can change (making it hard to pass in each filter parameter separately). As far as i know Sqlstring uses the order to determine what parameter matches the proper question mark.

So i am left with passing in all the filters to Sqlstring at once, but If no filters are active then i just get left with an empty string for the filters variable, which will throw a sql syntax error.

let filter = idList !== '' ? ` AND id IN(${idList})` : '';
filter += locationsList !== '' ? ` AND a.locationID IN(${locationsList})` : '';
filter += start !== undefined ? ` AND a.lastEdited >= ${start}` : '';
filter += end !== undefined ? ` AND a.lastEdited <= ${end}` : '';
filter += name !== undefined ? ` AND a.name LIKE '%${name}%'` : '';

const qry = `
SELECT
a.id 'id'
,a.number 'number'
,a.name 'name'
,a.locationID 'locationID'
,a.location 'location'
,a.lastEdited 'lastEdited'
,a.userID 'owner'
FROM tbl_foo_${'?'} a
WHERE a.id > ${'?'} ${'?'} LIMIT ${'?'};`;

let values = [ id, cursor, filter, limit ];

const rows = query(db, qry, values);

//inside of the query function it does this and then runs the query against the database
if (qry.includes('?')) {
sanitizedQry = sqlstring.format(qry, values);
}

The query it produces would look like this:

SELECT
a.id 'id'
,a.number 'number'
,a.name 'name'
,a.locationID 'locationID'
,a.location 'location'
,a.lastEdited 'lastEdited'
,a.userID 'owner'
FROM tbl_foo_36 a
WHERE a.id > 1 '' LIMIT 100;

Is there a better way to do this?

Answer 1

If you take a look at the sqlstring npm page, it says this about SqlString.format:

This looks similar to prepared statements in MySQL, however it really just uses the same SqlString.escape() method internally.

You're going to be much better off making appropriate use of SqlString.escape.

But the problem is, you're not protecting against SQL injection where you need to be. For example, in this statement you should have injection protection, and you don't. Here's an example of what it should look like:

filter += name !== undefined ? ` AND a.name LIKE '%${SqlString.escape(name)}%'` : '';

name is the user input variable presumably, and you need to protect against SQL injection at that point. All user input variables need SQL injection protection applied directly to them.

You absolutely can't provide SQL injection protection after you already construct a SQL fragment with the variables. What you're doing when you build your filter SQL fragment and then try to insert it later...that's actually you doing intentional SQL injection.

Disclaimer: you'll get a higher level of protection using a prepared statement (or equivalent technology) than this escape-style protection. Escape style protection is the lowest level of protection against SQL injection attacks that you can possibly use and still have any protection at all.

Disclaimer 2: allowing the user to input part of the table name can also be extremely dangerous, and very hard to escape properly.

For example, let's say that you have a bunch of tables, tbl_foo_1 through tbl_foo_99. And you want your user to have access to all of these. But what happens if someone later adds a tbl_foo_secret? Guess what? Your users can access this too, because you let them choose their own table name. Or what if you add a whole schema, tbl_foo_top_secret? They can easily tell you the table suffix is tbl_foo_top_secret.table_name, and your code will accept it just fine, because in this library periods aren't escaped. So you would want to add your own custom check to make sure the table name suffix is acceptable.