Add API for unquoted escaped strings

[nickshanks]

My first attempt to use Node's mysql package went like this [paraphrasing code]:

const mysql = require('mysql');
const result = mysql.query('SELECT * FROM table LIMIT 3');
console.log(result);

This worked fine. My second attempt to use Node's mysql package went somewhat downhill:

const mysql = require('mysql');
const result = mysql.query('SELECT * FROM table WHERE description LIKE "%?%" LIMIT 3', [req.query.q]);
console.log(result);

I traced the problem back to this package. It can be characterised as one of two things:

1. The question mark substitution is context-unaware. It should not wrap quote marks around strings if there already are quote marks around the question mark (i.e. it is preceded by an uneven number of any quote character). Or,
2. There needs to be a second API exposed via node's mysql package which lets programmers perform an unquoted escape (and yes, I've seen Why does SqlString.escapeString return values in single quotes? #19). This will allow programmers to put escaped, user-sourced values within larger strings without having to strip off the quote marks.

As an aside, I looked at the code of this package and all three points of return from escapeString() duplicitously wrap the escaped string in quote characters, à la return "'" + val + "'"; — you ought to refactor to have this happen in a single place, e.g.:

function escapeString(val) {
  return "'" + escapeUnquotedString(val) + "'";
}

function escapeUnquotedString(val) {
  var chunkIndex = CHARS_GLOBAL_REGEXP.lastIndex = 0;
  var escapedVal = '';
  var match;

  while ((match = CHARS_GLOBAL_REGEXP.exec(val))) {
    escapedVal += val.slice(chunkIndex, match.index) + CHARS_ESCAPE_MAP[match[0]];
    chunkIndex = CHARS_GLOBAL_REGEXP.lastIndex;
  }

  if (chunkIndex === 0) {
    // Nothing was escaped
    return val;
  }

  if (chunkIndex < val.length) {
    return escapedVal + val.slice(chunkIndex);
  }

  return escapedVal;
}

This removes duplication and reduces the chances of bugs being introduced (e.g. a new return point being added but forgetting to wrap in quotes).

[dougwilson]

There needs to be a second API exposed via node's mysql package which lets programmers perform an unquoted escape

No, we cannot do that. This module did that a long time ago and a security CVE was filed against it and we had to remove it.

You need to include the % inside the string you want to escape:

const result = mysql.query('SELECT * FROM table WHERE description LIKE ? LIMIT 3', [`%${req.query.q}%`]);

The question mark substitution is context-unaware.

Pull request #29 is working towards this, so if that works for you, then we are already working towards it, so this would be a duplicate of that.

And since the mysql module takes just plan queries, you can also alternatively not use this module at all and instead use any escaping module you like, for example https://hiddentao.com/squel/

[nickshanks]

Hello Doug. Much thanks for addressing my questions.

No, we cannot do that. This module did that a long time ago and a security CVE was filed against it and we had to remove it.

Do you know the CVE # off-hand? I'm curious as to how what is essentially a labour-saving utility routine can have a security issue, and whether that applies to how I'm doing it too (assuming the people calling it understood what it did and were using it correctly—if CVEs can be filed because users are abusing the tools they have, we need to file one against the concept of a memory address!).

You need to include the % inside the string you want to escape

I would have assumed (had I thought of doing it this way) that given its special meaning, percent would be one of the characters that got escaped, and so this technique would not work. I see now that it is not escaped. It seems this library is not aware of whether the string to be escaped is intended for use as a plain string, or LIKE or RLIKE value (seems similar to the escape() vs. escapeId() distinction), and always treats values as plain strings, not escaping characters destined for pattern matching specifiers. That's fair enough as long as the next level up (the mysqljs/mysql repo) knows this and sends pattern matching strings to a different escaper function. At the moment it sends everything to this library.

you can [] use any escaping module you like

Good point :-) But I would rather help improve the defaults if I can.
Since I now know that using question mark characters with LIKE and RLIKE is not safe, I'll do as you suggest and only use pre-escaped SQL with npm's sql module.

Pull request #29 is working towards this

That issue is quite long and I don't have time today to read through it, so I can't yet confirm if it is a duplicate.

[dougwilson]

It seems this library is not aware of whether the string to be escaped is intended for use as a plain string, or LIKE or RLIKE value (seems similar to the escape() vs. escapeId() distinction), and always treats values as plain strings, not escaping characters destined for pattern matching specifiers. That's fair enough as long as the next level up (the mysqljs/mysql repo) knows this and sends pattern matching strings to a different escaper function. At the moment it sends everything to this library.

That's right, because this library is not a full sql constructor like https://hiddentao.com/squel/ and is not designed to be; one already exists and it is https://hiddentao.com/squel/ :)

The only difference is at the sql langauge level and at that level there are only two things: values and identifiers. The LIKE etc. takes a value, and even characters like the % can be altered within your sql statement, so without this library fully building out your entire statement, then it would not have any idea what the wildcard chars are for that particular string.

I think, based on what you're describing, you would be much happier using the https://hiddentao.com/squel/ module over this module anyway, as it is a higher level abstraction, which seems to me what you're desiring here.

[dougwilson]

Another thing to note is that this module is also made to function with modules like squel. For example their toParam() method https://hiddentao.com/squel/#parameters produces the two values to pass into this module format. It includes the % character in the array and not in the sql as well, so a change here would break a lot of integration with mysql module and all the various sql formatting / orm libraries.

I'm not saying there cannot be a change here, but ideally they shouldn't all break all of a sudden when they are very useful libs. Probably should open a dialog with all these module as part of initial seeking to determine how to keep them all working together.

[nickshanks]

Parties talking to each other never created new problems :-)
I was suggesting a new API rather than breaking backwards compat.
And thanks for the pointer to the squel (pronounced: squeel?) library. Will investigate.