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

BREAKING CHANGE: drop support for executing raw SQL strings in execute() methods for all dialects #3761

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dmitrysteblyuk
Copy link

@dmitrysteblyuk dmitrysteblyuk commented Dec 13, 2024

Issue reference: #3598

Description of changes

Support for execution of raw SQL strings is removed.

With this change, one needs to wrap an SQL string in sql tagged function (i.e. sql`<string>`) or in sql.raw(string) (in case the SQL string isn't static) before passing it to .execute().

Why?

Unlike raw strings, sql tagged templates are by design safe from SQL injections.
In case one cannot use a tagged template, then wrapping an SQL string in sql.raw() still makes the intent more clear and visible (e.g. for code reviewers) that one needs to pay extra attention to what is being executed.

Currently, accidentally missing/removing sql when using .execute() method, could lead to an SQL injection vulnerability. For example, these 2 snippets look similar:

await db.execute(sql`select * from users where id = ${id}`);

vs

await db.execute(`select * from users where id = ${id}`);

@L-Mario564
Copy link
Collaborator

I think passing a plain string can be useful for raw queries that don't have any user input. I agree with removing it from execute, and I think it'd be a nice idea to add the plain string behavior in something like executeRaw or executeUnsafe, that way, it's easy to find and review queries that won't be parameterized.

@dmitrysteblyuk
Copy link
Author

dmitrysteblyuk commented Dec 13, 2024

it'd be a nice idea to add the plain string behavior in something like executeRaw or executeUnsafe

I didn't introduce a new method, because there was already sql.raw().

passing a plain string can be useful for raw queries that don't have any user input

Personally, I'd still use sql`<query>` syntax even if there is no parameters (e.g. sql`ROLLBACK`). It will do the same thing as sql.raw('<query>') (or executeUnsafe('<query>')).
The reason for this, is that sql`<query>` is by design safe (almost) from SQL injections (with params or without).
And there's really only few scenarios, when a developer cannot use it (e.g. sql is loaded from a file).

Basically, as I see it:

  • sql.raw(query) - an exceptional use case that requires paying extra attention
  • sql`<query>` - all other cases (no need to worry about SQL injections)

@L-Mario564
Copy link
Collaborator

it'd be a nice idea to add the plain string behavior in something like executeRaw or executeUnsafe

I didn't introduce a new method, because there was already sql.raw().

passing a plain string can be useful for raw queries that don't have any user input

Personally, I'd still use sql<query> syntax even if there is no parameters (e.g. sqlROLLBACK). It will do the same thing as sql.raw('<query>') (or executeUnsafe('<query>')). The reason for this, is that sql<query> is by design safe (almost) from SQL injections (with params or without). And there's really only few scenarios, when a developer cannot use it (e.g. sql is loaded from a file).

Basically, as I see it:

* `sql.raw(query)` - an exceptional use case that requires paying extra attention

* sql`<query>` - all other cases (no need to worry about SQL injections)

I actually agree with you. The only small annoying thing is having to import the sql operator every time you want to execute a raw query. I think this can be mitigated by db.execute accepting a callback that exposes sql for you. Not saying you should implement this right here right now, as the core team members are the ones to make the call on this, but just throwing that suggestion out there regardless.

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

Successfully merging this pull request may close these issues.

2 participants