forwards-long-jump / discotron

Modular Discord bot supporting plugins hosted on git repositories
MIT License
3 stars 5 forks source link

Ensure database operations are safe #31

Closed RedMser closed 4 years ago

RedMser commented 4 years ago

In database-crud.js (and probably other places), raw SQL strings are specified and concatenated with unsanitized values. Uncareful handling of user input can cause problems with the SQL string handling.

Solution Use parameterization like so:

var stmt = db.prepare("INSERT INTO lorem VALUES (?)");
for (var i = 0; i < 10; i++) {
      stmt.run("Ipsum " + i);
}
stmt.finalize();
Blatoy commented 4 years ago

I've double-checked database-crud.js and I see why it seems dodgy but it actually already uses parameterization.

If we take a look at this code:

sql += valuesText + " WHERE " + whereText;

it seems really fishy because if whereText is a user input, the query is compromised, but if you look a few lines higher you will find:

        for (let key in where) {
            whereText += key + "=? AND ";
            params.push(where[key]);
        }

So the final query may look like select * from table WHERE username=?. table and username are effectively not parametrized, but if the user have access to table / fields name we have other issues.

If you find any security issue though I'll be really grateful