Regression on placeholders

[CharlzVard]

Replace from placeholder on 3.17+, return "[object, object]"

const mysql = require('mysql2');
const data = { id: 1 };
console.log(mysql.format('?', [data]));

Return to 3.16 solve the issue and returns "id = 1"
3.17+ returnss "[object, object]"

[kwaschny]

Having a similar and probably related issue:

import mysql from 'mysql2/promise';

const data = { 'foo': JSON.stringify([ [ 'bar', 123 ] ]) };
const stmt = mysql.format('?', [ data ]);

console.log(stmt);

3.16

`foo` = '[[\"bar\",123]]'

3.17+

'[object Object]'

[mbaumgartl]

The change (#4054) was introduced in 3.17.0 to prevent SQL injections.

[kwaschny]

to prevent SQL injections

The exploits shown in the examples are only possible because the request payload is allowed to specify columns. Identifiers like tables and columns must never be user provided. And if you absolutely have to do it, then at least use a whitelist for your identifiers and construct the statement carefully. The whole point of query() has always been the ability to conveniently feed values(!) into a well-known schema instead of having to concatenate a bulky statement manually. It has never been about giving up control over the schema.

The whole change feels like an attempt to protect against rookie mistakes when teaching them about data binding in Prepared Statements would have done a better job.

[wellwelwel]

The whole point of query() has always been the ability to conveniently feed values(!) into a well-known schema instead of having to concatenate a bulky statement manually. It has never been about giving up control over the schema.

Bringing it from mysqljs/mysql:

In order to avoid SQL Injection attacks, you should always escape any user provided data before using it inside a SQL query. You can do so using the `mysql.escape()`, `connection.escape()` or `pool.escape()` methods:

var userId = 'some user provided value';
var sql    = 'SELECT * FROM users WHERE id = ' + connection.escape(userId);
connection.query(sql, function (error, results, fields) {
  if (error) throw error;
  // ...
});

But, in practice:

import { escape } from 'mysql';

const userId = { id: true };
const query = 'SELECT * FROM users WHERE id = ' + escape(userId);

console.log(query); // 🦠 SELECT * FROM users WHERE id = `id` = true

So the "In order to avoid SQL Injection attacks" is not exactly a well-knwon. This has been taught through documentation as a safe approach for years. However, I don't see this limitation as valid for this specific issue, since here the user has full responsibility for what will be generated by format, escape, etc., unlike what happens internally outside the user's control by default.

To address this, it would be possible to add an option/flag to the dependency that allows this protection to be disabled intentionally. Then for the unrestricted format (e.g, mysql.format('?', [ data ])), simply enable this flag in MySQL2 before exposing the helpers to the user, and the breaking change is resolved, while internally (for query), the "flag" would remain disabled as it is now.

[kwaschny]

This has been taught through documentation as a safe approach for years.

And this has never been good advise. Any type should only be escaped as flat value, not as multiple values, not as nested values. One does not expect a value to change the structure of a SQL statement. This behavior in node mysql, although being documented since the dawn of time, violates the principle of least surprise and thus contributes to the attacking surface. The new sql-escaper actually improves the situation by preventing plain objects from altering your statement. But it broke the existing assumption that objects can be utilized for bulking values safely as long as you understand the transformation.

Testing a bit more and looking at my broken unit tests, I realized 3.17+ makes an attempt to keep it backward compatible by looking at the statement. And yet:

.query(
  `UPDATE ... SET modified = UTC_TIMESTAMP(), ? WHERE id = ?`,
  [ { foo: 'bar' }, 123 ]
)

will end up as '[object Object]'.

Maybe this case can be solved via AST, but I would like to propose introducing an alternative method instead. Something like .queryUnsafe( to keep the "unsafe" transformation of objects and not requiring an AST at all. Let the developer decide how to build each statement.

[wellwelwel]

Something like .queryUnsafe( to keep the "unsafe" transformation of objects and not requiring an AST at all. Let the developer decide how to build each statement.

What about an explicit flag such as unsafeValues, for example?

// A
.query('', [], { unsafeValues: true });
// B
.query('', [], { unsafeFormat: true });
// C
.query('', [], { unsafeEscape: true });
// D
.query('', [], { unsafeStatements: true }); // I'm not sure this name fits in this context.
// ...

• The same option would be available to mysql.format.

[kwaschny]

What about an explicit flag?

Fine by me as long as one can adjust it for every query (i.e. don't force it to be constructor-only). I personally prefer option A unsafeValues as this option describes the behavior and fits for .format(…, { unsafeValues: true }) too.

[constb]

Hi, just to let you know that I also have a valid MariaDB query that got broken after 3.17:

pool.query({
  sql: "SET STATEMENT max_statement_time=15 FOR INSERT INTO ?? SET ?",
  values: ['domains', { id: 1, domain: 'test.com' }],
  timeout: 16000,
});

After the change in mysql2, this puts [object Object] into the query instead of the expected field list.

[kym6464]

I understand the desire to address a security vulnerability, but I would argue that #4054 should be a major version bump since it introduces a breaking change.

It forces myself (and likely others) to pin this library to v3.16.3, thereby missing out on future security patches.

If this were a major version upgrade, then it would give users time to properly test and update their code in the future while also allowing them to benefit from security patches to v3 in the meantime. If the goal is to maximize security for the community, then this may be more secure overall depending on what % of users are impacted by this issue.

That being said, I can appreciate the additional burden of maintaining two major versions and backporting security updates. I appreciate the efforts of the maintainers of this awesome library! Just wanted to share my 2 cents.

[sidorares]

@kwaschny agree, but sometimes we realize that after the release. the intent was a backwards compatible change. I guess now the only practical solution is to roll forward and fix incompatibilities as we find them