tediousjs / node-mssql

Microsoft SQL Server client for Node.js
https://tediousjs.github.io/node-mssql
MIT License
2.23k stars 466 forks source link

Prevent accidental misuse of template literals #1568

Open belgattitude opened 11 months ago

belgattitude commented 11 months ago

It's very easy to create security issues when using template literals....

For example

const email = 'test@acme.org';
const res = sql.query`select * from where email = ${email}`;  <-- Sanitized
const res = sql.query(`select * from where email = ${email}`) <-- Not sanitized

Related:

Expected behaviour:

Don't accept the () version.

Actual behaviour:

Using as a function -> doesn't sanitize !Risk !

Suggestion

Maybe take inspiration from Prisma (which fixed it in version 2.30 some time ago)

It would be nice to add a queryUnsafe part of the feature (the $queryRawUnsafe in prisma). That would be an escape hatch to give insights of the nature of the query

Software versions

dhensby commented 11 months ago

So if I understand the prisma feature correctly, there are essentially 2 query methods, one that will only accept tagged template literals and another that will accept strings. If implemented in this library it would mean any calls to something like .query(...) would error but .query`....` would be fine. Then you would have a .queryUnsafe(...) that would only accept strings and not tagged template literals.

I quite like this idea and it would go a long way to stopping people from mis-using the template strings. It's a very confusing syntax and very easy for people to assume the missing brackets are a mistake, this would solve the problem.

My only hesitation is that it would be a pretty disruptive change for anyone upgrading, but I suppose that's the whole point of semver!

belgattitude commented 10 months ago

Agree. I'd also rely internally on https://github.com/blakeembrey/sql-template-tag to allow more features. Like 'raw' -> to allow dynamic table names for example. It would also pave the way to query composition (join...) / pieces that can reused (ie -> cte's...)

dhensby commented 10 months ago

That library looks cool - I'm not sure about relying on it internally, but it definitely looks like it would be nice to accept as a query arg.