Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent accidental misuse of template literals #1568

Open
belgattitude opened this issue Oct 24, 2023 · 3 comments
Open

Prevent accidental misuse of template literals #1568

belgattitude opened this issue Oct 24, 2023 · 3 comments

Comments

@belgattitude
Copy link

belgattitude commented Oct 24, 2023

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

  • NodeJS:
  • node-mssql: 10+
  • SQL Server:
@dhensby
Copy link
Collaborator

dhensby commented Oct 25, 2023

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
Copy link
Author

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
Copy link
Collaborator

dhensby commented Nov 22, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants