Prisma-client-js: Raw API concerns

[steebchen]

I have concerns about the current Prisma Client JS raw API.

The tagged template literal raw API is indeed nice to use:

var id = "title"
await client.raw`SELECT * FROM Post WHERE title = ${id}`

That works fine. No sql injections here because we handle that for the user.

However, since we’re encouraging users to use that syntax, I can imagine sooner or later a user will invoke the method directly instead and not change the variables in the template string to parameters for the function:

var id = "'title'; DROP TABLE User;"
await client.raw(`SELECT * FROM Post WHERE title = ${id}`)

and boom, SQL injection!

It seems that the editor's autocomplete feature also uses parentheses per default.

We need to think how we can prevent this from happening. My suggestion is that we only accept the template literal method for one method, and create a new function which makes it clear that the input does not get escaped, like rawNoEscape(...), similar to react's dangerouslySetInnerHtml.

[thebiglabasky]

After some more discussions, here is the detailed situation as of today:

Escaping happens whenever using the tagged template form of call.
Escaping doesn't happen when using other forms.

This means that, the following examples don't get escaped:
Example A:

const result: number = await prisma.executeRaw('UPDATE User SET active = $1 WHERE email = $2;', true, '[email protected]; DROP ALL TABLES')
// Will result in an injection 😱 

Example B:

let evilmail: string = '[email protected]; DROP ALL TABLES'
const result: number = await prisma.executeRaw(`UPDATE User SET active = ${true} WHERE email = ${evilmail};`)
// Will result in an injection 😱 

But this one will get escaped:
Example C:

let evilmail: string = '[email protected]; DROP ALL TABLES'
const result: number = prisma.executeRaw`UPDATE User SET active = ${true} WHERE email = ${evilmail};`
// Will be properly escaped 🥰 

I created an issue to ensure we escape parameters to cope with Example A above as I believe that this behavior doesn't match the expected standard. (cf. details in the issue #755)

About Example B, while this can indeed be confusing, we can not really expect to differentiate usages where queries are created using some custom complex logic (concatenating strings within a function using multiple conditional statements or loops for example) from simply using template literals. This comes from the TS language to a degree as well.
Executing unparametrized raw queries is necessary to support even if it involves a risk depending on how the query is composed.

The option you're proposing is meant as a strong mean to raise awareness about the risk. The problem with that breaking change is that it adds friction to upgrade, and still requires to know how you should proceed with an escaped query execution, which is not dangerous to execute (defeating the new name of the function).

The need here is to ensure we explain (if not educate) about the risk and explain how it should be done.
I will thus create another issues in the docs for that purpose and we'll see how much more would be needed there.

[Sytten]

This should also be safe.

const result: number = await prisma.executeRaw('UPDATE User SET active = $1 WHERE email = $2;', [true, '[email protected]; DROP ALL TABLES'])

Looking at the code https://github.com/prisma/prisma/blob/master/src/packages/client/src/runtime/getPrismaClient.ts#L361 I don't see how example B can work / which code path it uses.

[thebiglabasky]

I did a few more tests and it turns out the the examples I had provided did work with proper escaping indeed.
I'll dig deeper into how this is all handled and post an update, but the general proposal for this specific issue is to mostly update our docs, for what I've opened a separate issue here.

[thebiglabasky]

After a few more tests and checking with the team, there are two main behaviors:

• queryRaw(sql) and executeRaw(sql) will only be executing a single statement, which means some injections won't work (the one with multiple queries trying to drop tables for example) but some will (unveiling additional data using UNION for example)
• queryRaw(sql, params) and executeRaw(sql, params) are translated to prepared statements which are inherently secure
• queryRaw`sql` and executeRaw`sql` are translated to prepared statements the same way and are also inherently secure

We need to keep the ability to execute SQL that is prepared elsewhere, leaving users the responsibility to sanitize those, as there are plenty of cases where custom logic can be used to generate an SQL query, or take them from another place without getting the parameters map details which would allow for preparing a statement.

Renaming the method being a breaking change, we will only update the docs to make it clear.

I'll thus close this issue.

[Sytten]

What needs to be added in the doc is the usage of the now exposed sql, join, etc that dev can use to create sanitized queries outside the templated method.