kysely-org / kysely

A type-safe typescript SQL query builder
https://kysely.dev
MIT License
10.71k stars 271 forks source link

Alarming error output sql #110

Closed boehs closed 2 years ago

boehs commented 2 years ago

Apologies, I am probably doing something horribly wrong. Inserts are failing with

 code: 'ER_PARSE_ERROR',
  errno: 1064,
  sqlState: '42000',
  sqlMessage: "You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '' at line 1",

Confused, I pasted the code into an SQL editor, and found that syntax highlighting was also broken. It breaks

'<p>Omie,</p><p>We aren\\'t in contact
                        ^^
                        here

And the problem becomes obvious! Single quotes are being escaped like \\', when really they ought to be like ''.

I'm unclear where this escaping code is even coming from. I did a search for escape and found nothing. Interestingly, Connection.escape(value: any): string is also escaping in a faulty way:

'<p>Omie,</p><p>We aren\'t in contact

So it's pretty unclear to me if this issue is coming from kysely or mysql2. I was hoping you could provide some insight.

Some other things

  1. Regardless of the source of the problem, it's very concerning. Will a deliberate, more sophisticated attack be handled?
  2. I can't escape manually, I tried and it just results in exponential \\\\\\\\\
koskimas commented 2 years ago

You should never ever ever ever ever concatenate user input to SQL strings! Instead you should pass all user input as parameters. Then, there's no need to escape anything, as the value is never added to the SQL string. Could you share the code that's causing that and I can point out how it should be written?

boehs commented 2 years ago

The code is simple enough

    await db
        .insertInto('ltc')
        .values([{message: "<p>Omie,</p><p>We aren't in contact"}, {message: "<p>Very cute. Nice try</p>"}])
        .onDuplicateKeyUpdate({})
        .execute()
koskimas commented 2 years ago

There's absolutely no way that's causing the bug you reported, unless you are doing something crazy with plugins or using a third-party dialect.

koskimas commented 2 years ago

Here's one of the tens of tests I have for inserts and the SQL that's produced

      const query = ctx.db.insertInto('person').values({
        first_name: 'Foo',
        last_name: 'Barson',
        gender: 'other',
      })

      testSql(query, dialect, {
        postgres: {
          sql: 'insert into "person" ("first_name", "last_name", "gender") values ($1, $2, $3)',
          parameters: ['Foo', 'Barson', 'other'],
        },
        mysql: {
          sql: 'insert into `person` (`first_name`, `last_name`, `gender`) values (?, ?, ?)',
          parameters: ['Foo', 'Barson', 'other'],
        },
        sqlite: {
          sql: 'insert into "person" ("first_name", "last_name", "gender") values (?, ?, ?)',
          parameters: ['Foo', 'Barson', 'other'],
        },
      })

As you can see, the values are not concatenated to the SQL.

boehs commented 2 years ago

This is the full code I have written excluding type definitions


import { Generated, Kysely, MysqlDialect } from "kysely"
import { createPool } from 'mysql2'
import { readFile, readdir as readDir} from 'node:fs/promises'

let pool = createPool({
       // using normal settings
})

const db = new Kysely<Database>({
    dialect: new MysqlDialect({
        pool: pool
    })
})

const files = await readDir('./letter')

for (const file of files) {
    const contents = await readFile(`./letter/${file}`)
    const json: LtcJson[] = JSON.parse(contents.toString())

    // @ts-ignore
    const patchedJson: LtcTable[] = json.map(letter => {
        delete Object.assign(letter, {["id"]: letter["Id"]})["Id"]
        // @ts-ignore
        letter.letterPostDate = new Date(Number(letter.letterPostDate.split('/Date(')[1].split(')/')[0]))
        return letter
    })

    await db
        .insertInto('ltc')
        .values(patchedJson)
        .onDuplicateKeyUpdate({})
        .execute()
}

This is a fresh install of MySQL on a server I control. The only operations done to the MySQL database was the bare minimum to set up a user and a table

koskimas commented 2 years ago

Don't know what to tell you. I can't reproduce that. If you can provide me a script I can run without doing anything but ts-node script.ts, I can take a look.

boehs commented 2 years ago

Sure, I want to get to the bottom of this. One moment

koskimas commented 2 years ago

Actually that empty onDuplicateKeyUpdate is causing the query to fail but it has nothing to do with the input.

boehs commented 2 years ago

Oh interesting. Is it possible that when the query fails, it just dumps an incorrect output? (Still working on self contained script, almost there)

koskimas commented 2 years ago

Your error is

You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '' at line 1

I think the near '' would usually say something like near 'select' but in this case the error is at the end, and the error message is confusing. I think the error is simply that an empty on duplicate key update is not valid SQL. Does the query work if you remove the onDuplicateKeyUpdate() line?

boehs commented 2 years ago
The code ```ts #!/usr/bin/env ts-node import { Generated, Kysely, MysqlDialect } from "kysely" import { createPool } from 'mysql2' const letters = [ { "Id": 13, "letterMessage": "

Omie,

We aren't in contact - that's. Recently, I haven't. I. If, all.

One hope, right?

", "letterPostDate": "/Date(1650851725000)/", }, { "Id": 12, "letterMessage": "

Nothing special here. A few periods.", "letterPostDate": "/Date(1234567890000)/", } ] interface LtcBase { letterMessage: string; } interface LtcTable extends LtcBase { id: Generated letterPostDate: Date; } interface Database { test: LtcTable } let pool = createPool({ database: "your", host: "localhost", user: "creds", password: "here" }) const db = new Kysely({ dialect: new MysqlDialect({ pool: pool }) }) await db.schema .createTable('test') .addColumn('id', 'integer', (col) => col.autoIncrement().primaryKey()) .addColumn('letterPostDate', 'datetime') .addColumn('letterMessage', 'text') .execute() // @ts-ignore const patchedJson: LtcTable[] = letters.map(letter => { delete Object.assign(letter, { ["id"]: letter["Id"] })["Id"] // @ts-ignore letter.letterPostDate = new Date(Number(letter.letterPostDate.split('/Date(')[1].split(')/')[0])) return letter }) await db .insertInto('test') .values(patchedJson) .onDuplicateKeyUpdate({}) .execute() console.log('done') ```

You will need to add your credentials but otherwise should work fine.

Some observations

  1. Without the .onDuplicateKeyUpdate({}) the query does indeed work fine 🎉 🎉
  2. The error is very concerning with the \\'. I assume this is not actually the SQL being run?
  code: 'ER_PARSE_ERROR',
  errno: 1064,
  sqlState: '42000',
  sqlMessage: "You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '' at line 1",
  sql: "insert into `test` (`letterMessage`, `letterPostDate`, `id`) values ('<p>Omie,</p><p>We aren\\'t in contact - that\\'s. Recently, I haven\\'t. I. If, all.</p><p>One hope, right? </p>', '2022-04-24 21:55:25.000', 13), ('<p>Nothing special here. A few periods.', '2009-02-13 18:31:30.000', 12) on duplicate key update "
}
  1. Somehow this incorrect query worked 80 times before erroring (the for loop in the actual code posted above successfully iterated 80 times). How could this be?
koskimas commented 2 years ago

I think mariadb is just printing the whole thing as SQL for some odd reason, but the error is from the invalid SQL caused by the wrong use of onDuplicateKeyUpdate. That's not the SQL kysely sends to the mysql2 driver. Kysely uses parameters correctly and the query in this case is

insert into `test` (`letterMessage`, `letterPostDate`, `id`) 
values (?, ?, ?), (?, ?, ?) on duplicate key update

Feel free to debug or add console.logs to kysely code to see what kysely sends to the mysql2 driver. Kysely is definitely not doing something that stupid. And neither is mysql2.

The error in sqlMessage has nothing to do with the ' characters in the values.

koskimas commented 2 years ago

Here's the line where the query is sent to mysql2. You can edit that line in your node_modules to see what's happening.

boehs commented 2 years ago

Thank you! Sorry for that whole roller coaster. Is there some way I could buy you a coffee?

koskimas commented 2 years ago

There's no way or need to do that. Thanks for the offer though 😊