Skip to content

feat: escape single quotes in string literals.#1392

Merged
igalklebanov merged 1 commit intokysely-org:masterfrom
igalklebanov:sanitize-string-literals
Apr 11, 2025
Merged

feat: escape single quotes in string literals.#1392
igalklebanov merged 1 commit intokysely-org:masterfrom
igalklebanov:sanitize-string-literals

Conversation

@igalklebanov
Copy link
Copy Markdown
Member

@igalklebanov igalklebanov commented Mar 16, 2025

Hey 👋

closes #1124.

This PR introduces some string literal SQL injection denial in the form of escaping ' to deny the most common string literal attacks:

-- when sql.lit(`'; drop table users --`)
where column = ''; drop table users --'
-- when sql.lit(`' or 1=1 --`)
where column = '' or 1=1 --'

and make them:

-- when sql.lit(`'; drop table users --`)
where column = '''; drop table users --'
-- when sql.lit(`' or 1=1 --`)
where column = ''' or 1=1 --'

If you find any additional measures we could take, please submit an issue OR swing by the Discord.
Some things are out of bounds, like SQL parsing, semantic checks, etc.

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 16, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 6, 2025 9:19pm

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Mar 16, 2025

kysely_koa_example

npm i https://pkg.pr.new/kysely-org/kysely@1392

commit: 5a7952a

@igalklebanov igalklebanov marked this pull request as ready for review March 17, 2025 09:17
@igalklebanov igalklebanov requested a review from koskimas March 17, 2025 09:18
@igalklebanov igalklebanov changed the title sanitize string literals. feat: sanitize string literals. Mar 17, 2025
@igalklebanov igalklebanov marked this pull request as draft March 17, 2025 12:00
@igalklebanov
Copy link
Copy Markdown
Member Author

igalklebanov commented Mar 17, 2025

  • Need to handle strings with ' already being escaped.

  • Possibly find a way to opt-out of sanitation.

Copy link
Copy Markdown
Member

@koskimas koskimas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

As discussed in discord, check if already escaped strings could be supported.

@igalklebanov igalklebanov force-pushed the sanitize-string-literals branch from 878299e to f200786 Compare March 23, 2025 23:30
@igalklebanov igalklebanov changed the title feat: sanitize string literals. feat: escape quotes in string literals. Mar 24, 2025
@igalklebanov igalklebanov changed the title feat: escape quotes in string literals. feat: escape single quotes in string literals. Mar 24, 2025
@igalklebanov igalklebanov marked this pull request as ready for review March 24, 2025 09:34
@igalklebanov igalklebanov added the breaking change Includes breaking changes label Mar 24, 2025
@igalklebanov igalklebanov force-pushed the sanitize-string-literals branch from 37e8385 to cae9646 Compare March 30, 2025 12:28
failing test.

...

...
@igalklebanov
Copy link
Copy Markdown
Member Author

igalklebanov commented Apr 8, 2025

@koskimas I wonder if we should add an easy opt-out mechanism in case we did something wrong, and until we fix it. The cheapest (no API changes to dialects and compilers) would be a Kysely-specific environment variable check - but that might be inconvenient to Deno users (might make --allow-env a requirement suddenly). Should we break things and make it part of APIs?

I'm also unsure about the "already escaped literals" problem. Is it a problem? what if the literal is not escaped, just has a bunch of 's grouped together? Can we tell?

@koskimas
Copy link
Copy Markdown
Member

koskimas commented Apr 8, 2025

If it's not easy to avoid, let's just forget about it. It's really a niche problem that might not affect anyone.

Let's not do any opt-out or anything. We've done bigger changes with only the hope that it doesn't affect anyone 😅

@igalklebanov igalklebanov changed the base branch from v0.28 to master April 11, 2025 00:06
@igalklebanov igalklebanov merged commit 08487c4 into kysely-org:master Apr 11, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Includes breaking changes enhancement New feature or request internal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Escape string literals by default

2 participants