Table and column identifiers are not properly escaped.

[javier-garcia-meteologica]

A malicious user could send the following input.

// A regular user is expected to send `{ id: '123' }`
// Malicious user sends this
req.body = { 'id" = ""); SELECT * FROM "passwords"; --': '' }

// And we run the following query
console.log(zp.sql`SELECT * FROM ${'users'} WHERE ${req.body}`.compile());

This would result in SQL injection.

{
  text: 'SELECT * FROM "users" WHERE ("id" = ""); SELECT * FROM "passwords"; --" = $1)',
  values: [ '' ]
}

On the one hand, developers should never use column or table identifiers from input fields. But on the other hand, zapatos should escape correctly table and column identifiers.

[jawj]

(1) Don't do this (relevant documentation). (2) To help you not do this, that query gives a type error, since the object passed in is not typed as a Whereable.

[jawj]

OK, I see your pull request.

I need to remind myself why we check if a string is already quoted before we quote it there, since on the face of it that does seem like a bit of a code smell.

But it remains the case that this quoting is not designed as a security measure, but only to protect JS-style camelCase identifiers from being downcased by Postgres.

[javier-garcia-meteologica]

req.body is expected to be typed as { id: string }. The problem comes from non-validated user input. Static analysis is not enough to detect malicious input present at runtime.

[jawj]

But if req.body is actually typed as { id: string }, then I think you must at some point have cast it to that without checking it, no?

The right way to write this query is probably:

db.sql`SELECT * FROM ${'users'} WHERE ${{ id: req.body.id }}`.compile();

or equivalently

const { id } = req.body;
db.sql`SELECT * FROM ${'users'} WHERE ${{ id }}`.compile();

Fundamentally I'm not convinced it's Zapatos' job to stop you (or other libraries you use) from lying to the type system. I'm not sure it's even possible, in the general case. But as I said, I will look at that quoting mechanism at some point.

[javier-garcia-meteologica]

at some point have cast it to that without checking it

Yes, in this issue the developer can definitely be blamed for not validating input. E.g.

router.post('/example', (req, res, next) => {
  // Junior developer not validating input
  const filter = req.body as { id: string };

  console.log(zp.sql`SELECT * FROM ${'users'} WHERE ${filter}`.compile());
});

But zapatos could provide a higher level of robustness for free, like I propose in #99. Zapatos is already quoting identifiers, so why not escape them?

My fear is that junior developers get comfortable with typescript very quickly. They think that runtime values are always constrained to types, but that assumption is dangerous when it comes to security. Solving this issue lowers significantly the cost of mistakes.