Knex.js and NodeJS - Dynamic Endpoints for Select, Insert, Delete, Update-CodePudding

Currently moving a web application over to using Knex to help with issues to do with SQL injection.

Knex has helped us to stop the issue of running different and new queries however we have a point of concern. Currently we have 4 endpoints which look like so:

router.all('/deleteTable', checkAuth, (req, res) => {
if (checkTableWhitelist(req.body.table) === true) {
knexMarsDb(req.body.table)
  .where(req.body.where)
  .del()
  .then((result) => {
    console.log(result);
    res.json({ success: true, message: 'deleted' });
  })
  .catch((error) => {
    res.json({ success: false, message: 'failure' });
    console.log(error);
  });
} else {
res.send(401, 'You are not authorised for this request');
console.log('Table not in whitelist');
}
});

What I understand is that it can be a threat to have this as it means people can delete whatever and how many rows and etc they want to.

Is there a way to make this safer (we have lots of queries running) or is it smarter to just move everything to a hard coded individual endpoint basis?

Thank you

CodePudding user response：

Yes, this is SQL injection by design. It looks like it accepts any arbitrary WHERE clause, so as long as checkTableWhitelist() returns true, they can delete whatever they want.

In general, it's inadvisable to accept request input and then use it verbatim as code. This applies to SQL and any other code that is parsed at runtime. For example, you shouldn't accept arbitrary Javascript code as input and just pass it to eval() to execute it, either.

I would code some conditions to interpret the input, make sure it is one of a set of prescribed types of deletions, and then form the query conditionally. Do NOT use the text of the input in the query, but only use the input to choose which DELETE statement to run. This allows you to be in control.

CodePudding user response：

Knex is a query builder (usually used as a building block by other high level libs like ORMs) it prevents SQL injection because implements placeholders under the hood.

so assuming that the function 'checkTableWhitelist' is working as expected (is not vulnerable) , you are simply giving endusers total control over those tables.

Assuming that this is the behavior you want to achieve, it is always inadvisable to expose the database directly externally.

You should implement your API in a more stringent way if security is a concern of yours.

Ex. Expose an endpoint for each operation you want to grant,

API/TABLE/FIELD/:ID/DELETE

api/users/1/delete

api/group/1/delete

...

so the controller logic will be:

knexMarsDb('users')
.where('id', req.params.id)
.del()