Result of query with comment is inconsistent and random

[kkrypt0nn]

There seem to be an inconsistent bug in the comments parsing method.

Having the following raw SQL query (making it a one liner doesn't change anything either):

const query = `SELECT *
						   FROM user_satellites_satellite
									FULL JOIN satellite ON user_satellites_satellite.satelliteId = satellite.id
						   WHERE user_satellites_satellite.userId = '${user.id}'
							 AND (satellite.name LIKE '%${searchQuery}%' OR satellite.description LIKE '%${searchQuery}%');`;

with searchQuery being (random can be anything):

random') UNION SELECT CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), tbl_name, CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR) FROM sqlite_master; --

And executing it with query()

await this.dataSource.query(query);

Will either return the correct and expected data or the error

The supplied SQL string contains more than one statement

Logging with NestJS shows the following query is getting executed (query logged being always the same):

The exact same query being executed multiple times over and over, e.g. by spamming, will make the query return correct data after some time, which makes the bug inconsistent:

bug.mp4

---

Switching the SQLite driver to the sqlite - being installed with npm install sqlite3 - driver makes the query always work and return the expected result, hence is not related to TypeORM itself.

[Prinzhorn]

Can you provide a self-contained reproduction? Preferably a single index.js that uses a :memory: database and can just be run standalone without anything other than better-sqlite3?

Alternatively, since you've seem to have narrowed down the possible cause, maybe you can contribute a failing test? But from what I understand you're saying it's not behaving consistently?

[Prinzhorn]

FWIW, what you're doing is not optimal. It allows SQL injections (including injecting comments) and prevents you from re-using prepared statements. I would do sth. like satellite.name LIKE ('%' || :searchQuery || '%') and pass it as a parameter.

[kkrypt0nn]

Can you provide a self-contained reproduction? Preferably a single index.js that uses a :memory: database and can just be run standalone without anything other than better-sqlite3?

Alternatively, since you've seem to have narrowed down the possible cause, maybe you can contribute a failing test? But from what I understand you're saying it's not behaving consistently?

What exactly do you mean with "self-contained reproduction"? I could provide a NestJS example which has the issue contained in it if needed.

FWIW, what you're doing is not optimal. It allows SQL injections (including injecting comments) and prevents you from re-using prepared statements. I would do sth. like satellite.name LIKE '%' || :searchQuery || '%' and pass it as a parameter.

Yep I am aware of that, the purpose of the application is to showcase it, hence why the tested input is actually an SQLi and why I am using the dataSource to make a raw query instead of making use of TypeORM's repositories.

[Prinzhorn]

What exactly do you mean with "self-contained reproduction"? I could provide a NestJS example which has the issue contained in it if needed.

import Database from 'better-sqlite3';

const db = new Database(':memory:');

// Your code here.

Yep I am aware of that, the purpose of the application is to showcase it, hence why the tested input is actually an SQLi

Ok, so the problem is not that you're getting "The supplied SQL string contains more than one statement" errors but that you claim for the same input better-sqlite3 sometimes errors and sometimes doesn't? Correct?

[kkrypt0nn]

Ok, so the problem is not that you're getting "The supplied SQL string contains more than one statement" errors but that you claim for the same input better-sqlite3 sometimes errors and sometimes doesn't? Correct?

Absolutely, when spamming the same query over and over it leads to a previously failed query returning the correct data - which is there the big inconsistency.

I will provide the reproduction as soon as possible 👍

[Prinzhorn]

I will provide the reproduction as soon as possible +1

I'm super curious to see. Right now I'm assuming the error is somewhere else in the chain and not in better-sqlite3 itself. But let's find out.

Absolutely, when spamming the same query over and over it leads to a previously failed query returning the correct data - which is there the big inconsistency.

This could very easily be a race condition (or other type of bug, caching etc.) in your server or client code, making it look like that when in reality it is actually consistent. Hence why we need a repro without any other dependency.

[kkrypt0nn]

Seems like when just using it in a standalone way it's fine.

I suppose NestJS's TypeORM module is doing some magic which makes it behave differently 🤔

[kkrypt0nn]

Seems like the issue is actually with the comments parsing here after all. When using template strings and new lines like in the example below, it will fail - though when using template strings and putting everything in a single line it succeeds perfectly. There is no reason it should fail for multiple statements as the query executed is

SELECT *
                                                   FROM user_satellites_satellite
                                                                        FULL JOIN satellite ON user_satellites_satellite.satelliteId = satellite.id
                                                   WHERE user_satellites_satellite.userId = '630633ba-030c-4967-acd4-d790bf26cfda'
                                                         AND (satellite.name LIKE '%ezrms7w8tzmjb3s02iu6') UNION SELECT tbl_name, CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR) FROM sqlite_master; --%' OR satellite.description LIKE '%ezrms7w8tzmjb3s02iu6') UNION SELECT tbl_name, CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR) FROM sqlite_master; --%');

Which has only one statement as the rest is commented out.

Source code used:

import Database from "better-sqlite3";

const db = new Database("db.sqlite");

let failed = 0;
let succeeded = 0;

for (let i = 0; i < 5000; i++) {
  const random = Array.from(Array(20), () =>
    Math.floor(Math.random() * 36).toString(36)
  ).join("");
  const searchQuery = `${random}') UNION SELECT tbl_name, CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR) FROM sqlite_master; --`;
  const query = `SELECT *
						   FROM user_satellites_satellite
									FULL JOIN satellite ON user_satellites_satellite.satelliteId = satellite.id
						   WHERE user_satellites_satellite.userId = '630633ba-030c-4967-acd4-d790bf26cfda'
							 AND (satellite.name LIKE '%${searchQuery}%' OR satellite.description LIKE '%${searchQuery}%');`;
  try {
    await db.prepare(query).all();
    succeeded++;
  } catch (_) {
    failed++;
  }
}

console.log(failed, succeeded); // Will never be consistent

The interesting part is that sometimes it works, sometimes it doesn't. You can also edit the code so that failed queries are executed again, some will work after some new attempts made.

Running the exact same code with sqlite3 works fine and will always log 0 5000:

const sqlite3 = require("sqlite3").verbose();
const db = new sqlite3.Database("db.sqlite");

async function main() {
  let failed = 0;
  let succeeded = 0;

  for (let i = 0; i < 5000; i++) {
    const random = Array.from(Array(20), () =>
      Math.floor(Math.random() * 36).toString(36)
    ).join("");
    const searchQuery = `${random}') UNION SELECT tbl_name, CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR) FROM sqlite_master; --`;
    const query = `SELECT *
						   FROM user_satellites_satellite
									FULL JOIN satellite ON user_satellites_satellite.satelliteId = satellite.id
						   WHERE user_satellites_satellite.userId = '630633ba-030c-4967-acd4-d790bf26cfda'
							 AND (satellite.name LIKE '%${searchQuery}%' OR satellite.description LIKE '%${searchQuery}%');`;
    try {
      await db.prepare(query).all();
      succeeded++;
    } catch (_) {
      failed++;
    }
  }

  console.log(failed, succeeded); // 0 5000
}

main();

The database used is available here: https://file.io/GaG5ThL0YbHs

[Prinzhorn]

I'm getting consistent 5000 0. Are you on the latest version of better-sqlite3? What Node.js version? Does this only happen locally or also on other machines? Can you provide a Dockerfile that demonstrates the inconsistency so that I can see it too? Also can you reproduce this with a consistent seeded random value (e.g. https://github.com/davidbau/seedrandom)? This should give you at least consistent numbers, albeit maybe not 5000 0. If not, then this contradicts everything I know about computers.

[Prinzhorn]

Are you sure it's actually inconsistent or are you expecting the following to not produce an error?

import Database from "better-sqlite3";

const db = new Database(":memory:");

db.prepare(`SELECT 1; --%test`);

[kkrypt0nn]

Are you on the latest version of better-sqlite3? What Node.js version? Does this only happen locally or also on other machines?

"better-sqlite3": "^8.2.0", Node.JS 19.8.1 (macOS) & 18.15.0 (Windows)

Are you sure it's actually inconsistent or are you expecting the following to not produce an error?

That is it output I am getting:

When making the query a one line query with template strings, without new lines or tabs, it works as it should:

It is very inconsistent, and the query executed should never produce an error (hence must always return 0 5000), as the query is a valid SQLite query with comment.

Can you provide a Dockerfile that demonstrates the inconsistency so that I can see it too?

Using that basic Dockerfile

FROM node:19.8.1

WORKDIR /app

COPY db.sqlite /app
COPY main.js /app
COPY package-lock.json /app
COPY package.json /app

RUN npm i

and getting an interactive shell with

docker run -it $(docker build -q .) bash

gives me sometimes fully failed, sometimes fully succeeded results:

When logging the error in the docker:

RangeError: The supplied SQL string contains more than one statement
    at Database.prepare (/app/node_modules/better-sqlite3/lib/methods/wrappers.js:5:21)
    at file:///app/main.js:19:14
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)

[kkrypt0nn]

If you're interested - those are the results when using sqlite3 along with the same database & query:

Locally on macOS:

Within a docker: