S3649 FP with Knex.js

Product: sonarcloud

Getting false positive for “Database queries should not be vulnerable to injection attacks” in very specific uses of Knex.js query builder. I’ve provided some minimal compliant and noncompliant examples below:

Positional bindings:

import { db } from './knex'

function (req, res) {
  const {id} = req.query
  
  const query = db('books')
    .select('title')
    .where(db.raw('id = ?', [1]))

  console.log(query.toSQL().toNative())
  // { sql: 'select "title" from "books" where id = $1', bindings: [ 1 ] }
}

Correctly generates parameterized query. Should not be flagged as error.

Named bindings:

import { db } from './knex'

function (req, res) {
  const {id} = req.query
  
  const query = db('books')
    .select('title')
    .where(db.raw('id = :id', {id: 1}))

  console.log(query.toSQL().toNative())
  // { sql: 'select "title" from "books" where id = $1', bindings: [ 1 ] }
}

Should not be flagged as error.

Builder methods like where():

import { db } from './knex'

function (req, res) {
  const {id} = req.query
  
  const query = db('books')
    .select('title')
    .where({id})
    // .where('id', id) // alternative syntax
    // .where('id', '=', id) // alternative syntax
    // .where('id', '=', `${id}`) // contrived example

  console.log(query.toSQL().toNative()
  // { sql: 'select "title" from "books" where "id" = $1', bindings: [ 1 ] }
}

Using where and other similar builder methods should not be flagged as error.

Template strings inside raw:

import { db } from './knex'

function (req, res) {
  const {id} = req.query
  
  const query = db('books')
    .select('title')
    .where(db.raw(`id = ${id}`))

  console.log(query.toSQL().toNative()
  // { sql: 'select "title" from "books" where id = 1', bindings: [] } // Potential for SQL injection!
}

This is a valid error :+1:

Useful resources I came across in researching this topic:

Let me know if I can add anything else to help!

2 Likes

Hello Sean and welcome to the community!

Thanks a lot for the detailed test cases as well! We have an internal ticket to track this issue, so we will not forget about it.

1 Like

Hi Hendrik, thank you!

That’s good to hear. I look forward to future updates on this.

1 Like

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.