Is escaping SQL queries like this safe?

I am currently working on a NodeJS backend script that parses incoming HTTP requests to write to and read from a MySQL database for work. I tried to protect it against SQL injections by using a kind of two-layer protection.

To write to the database the user needs to provide valid JSON. This is how the JSON’s keys need to be for the script to write to the DB. If they aren’t, the request will be cancelled.

{
    "uuid": "1234-5678-abcd-efgh",
    "product_id": 6,
    "product_extras": "color=red",
    "product_count": 2,
    "buyer_name": "X Y",
    "shipping_address": "XY Street 5"
}

The first layer that checks this input is a very basic blacklist. Here the USER_INPUT variable is the JSON above that has already gone through the validation process.

let BLACKLIST = ["DROP ", "DELETE ", "INSERT ", "UPDATE ", "SELECT ", "WHERE ", "ALTER "];
let dont_execute = false;

for(var i = 0; i < BLACKLIST.length; i++) {   
    if(JSON.stringify(USER_INPUT).toUpperCase().includes(BLACKLIST[i].toUpperCase())) {
        console.log("x1b[31mreceived blacklisted command - aborting");
        dont_execute = true;
        return false;
    }
}

After that validation, if dont_execute is still false, the second layer will be called and so the query will be escaped and sent just like this:

// setting.sqlconnection.table_name is equal to "orders" in this case. Also this variable can't be changed or specified by the user. It's pulled from a settings.json file

sql.sendQuery("INSERT INTO " + setting.sqlconnection.table_name + " (uuid, productid, orderid, productextras, productcount, buyername, shippingaddress) VALUES (" + sqlconnection.escape(uuid) + ", " + sqlconnection.escape(parseddata.product_id) + ", " + null + ", " + sqlconnection.escape(parseddata.product_extras) + ", " + sqlconnection.escape(parseddata.product_count) + ", " + sqlconnection.escape(parseddata.buyer_name) + ", " + sqlconnection.escape(parseddata.shipping_address) + ")");

I have tried many different injections like this one:

{
    "uuid": "1234-5678-abcd-efgh",
    "product_id": 6,
    "product_extras": "color=red",
    "product_count": 2,
    "buyer_name": "X Y",
    "shipping_address": "');DROP orders;--"
}

And just like I expected, it didn’t work because first of all it was cought in the first blacklist layer. But after disabling it just to test things out the whole SQL query was just interpreted like a string as the apostrophe was escaped (' became '), which is just what I wanted. The query that will be sent with the SQLI-infected JSON will then as far as I know look like this in the end:

INSERT INTO orders (guid, productid, orderid, productextras, productcount, buyername, shippingaddress) VALUES ("1234-5678-abcd-efgh", 6, null, "color=red", 2, "X Y", "');DROP orders--")

But the problem is, after showing this to my boss he said it wouldn’t be safe and it’s still vulnerable to SQLI. So my question is if he is right and if so, what I could do to improve it.

Additional information:
I am using the npm package mysql for the database connection
I am using XAMPP with MySQL to host the database locally

I’m sorry if I couldn’t provide more information but I’m pretty sure that I am not allowed to post more stuff than this.

Answer

The denylist approach is bound to miss some cases. You’d need to study a lot more about how queries are formed, and you should write thorough unit tests for your code so anyone who reviews your code can see which cases you’ve tested.

The denylist approach is also going to get false positives. It appears that you cannot insert any data that includes the word “DROP” for example. That’s going to block some legitimate data values.

Both of these problems are solved by using parameterized queries, as the comments above have suggested. You said you’ll “take a look” at using parameters, but you should think of it as the primary solution for protecting against SQL injection, not any sort of optional or advanced usage.

However, parameters are only useful in place of scalar values (strings, numbers, dates). You can’t use parameters for table names or column names or other identifiers, or SQL expressions, or SQL keywords. Those cases may be less common, but you’ve got at least one example in your dynamic table name.

Harmful input doesn’t come only from users. It can come from files, from web services, from JSON documents. It can even come from your own database! Any content that has a chance of containing weird characters can cause SQL injection.

SQL injection isn’t necessarily malicious. It can just be a mistake that is more likely to cause your SQL query to be invalid, instead of causing a data breach.

The solution to supplement parameterized queries is often allowlisting. For example, if you wonder if the table name from your config file is legitimate, check it against a list of known table names. Some apps keep this as in a constant array. Some apps query the INFORMATION_SCHEMA to get the up-to-date list of tables.

Ever see a table in an e-commerce database named ORDER? That would cause SQL to become confused, because ORDER is a reserved word. You should delimit table names in back-ticks just in case they are SQL reserved words or contain punctuation or whitespace.

Here’s an example of adding back-ticks around the table name:

sql.sendQuery("INSERT INTO `" + setting.sqlconnection.table_name + "` (uuid, ...