mysqljs / sqlstring

Simple SQL escape and format for MySQL
MIT License
403 stars 78 forks source link

Escaping date sometimes results in empty string #67

Closed zefir-git closed 2 years ago

zefir-git commented 2 years ago

This is a very weird issue that I was not able to fully debug to find the cause.

Part of my project's code looks like this:

const mysql = require("mysql");
const db = mysql.createConnection(...);

When I run a query with {Date} objects, some of them resulted in a query like (demo) image image Running mysql.escape(new Date()) I get empty string.

I installed mysql and sqlstring on a new plain project and could not reproduce this. mysql.escape(...) seems to work with any other type.

Any ideas what could be the cause for this?

image

dougwilson commented 2 years ago

Hi @williamd5 sorry you are having this issue. I'm not sure off-hand, but I will try and comb through the code to see under what condition there can be an empty string returned for a Date object. My initial guess from looking at the code is that since the test is val instanceof Date, and Date objects have no enumerable properties, the Date object you are passing in is from a different v8 isolate from the sqlstring module, so gets treated as an empty object.

dougwilson commented 2 years ago

I'm going to mark this as a bug for now, as it should work for Date objects from different isolates, which it doesn't today. Of course, if that is your issue or not, I can't say for sure. If you are able to step through the buggy instance with a debugger, you can check if instanceof Date returns false for the object or not (or I guess edit in a console.log in your node_modules copy of sqlstring).

zefir-git commented 2 years ago

Thank you for the information. I will try to debug as you said and see if I can find anything.

zefir-git commented 2 years ago

In sqlstring lib I added debug to check val instanceof Date and it outputs false. According to this SO answer

you can use the instanceof operator, i.e. But it will return true for invalid dates too, e.g. new Date('random_string') is also instance of Date

date instanceof Date

This will fail if objects are passed across frame boundaries.

A work-around for this is to check the object's class via

Object.prototype.toString.call(date) === '[object Date]'

Here is what I debugged: image image

The recommended alternative in the SO answer seems to work. I will open a PR on sqlstring with a reference to this issue. image

dougwilson commented 2 years ago

Thank you! I am happy to make the fix, but if you want to do it and open a PR, please be sure to add a test case for this condition.

zefir-git commented 2 years ago

If you could make the fix that would be great

zefir-git commented 2 years ago

Any suggestion how to test cross-frame in the tests? I have started a branch on a fork cloudnode-pro/sqlstring@patch/67

dougwilson commented 2 years ago

Hi @williamd5 sorry I was away over the week; I just have a short amount of time today, so wanted to get this fixed for ya. I landed the change + test.