antonniklasson / eslint-plugin-knex Goto Github PK

Lint rule(s) for Knex.js

JavaScript 100.00%

eslint eslint-plugin eslint-rules knexjs knex eslint-plugin-knex

eslint-plugin-knex's Introduction

eslint-plugin-knex

Installation

npm install -D eslint-plugin-knex
yarn add -D eslint-plugin-knex

Usage

In your eslint config file:

{
  "plugins": ["knex"],
  "rules": {
    "knex/avoid-injections": "error"
  }
}

Settings

You can configure what names you intend to use for the knex client. Make sure to include the library itself (knex), but also transaction variables (trx, transaction).

{
  "settings": {
    "knex": {
      "builderName": "^(knex|transaction)$"
    }
  }
}

Rules

`knex/avoid-injections`

Avoid some issues related to SQL injection by disallowing plain strings as the query argument to the raw queries. Check out the tests to get a sense for what is valid and not.

eslint-plugin-knex's People

Contributors

Stargazers

Watchers

Forkers

freshollie mahcloud gradedjestrisk renarsvilnis exodusmovement sekhmet 1024pix octo-topi

eslint-plugin-knex's Issues

knex/avoid-injections throws if file contains knex.schema.raw

Hi,
I'm trying to use the plugin to test DB migration files that use knex.schema.raw(...). And the plugin fails and eslint doesn't execute further.

Looked into the avoid-injections.test.js file and saw that the rule don't cover that case at all.

Debugged a bit and found out it happens when I have settings.knex.builderName = ... set .eslintrc as that runs conditional code - https://github.com/AntonNiklasson/eslint-plugin-knex/blob/master/rules/avoid-injections.js#L19

Error message:

TypeError: Cannot read properties of undefined (reading 'name')
Occurred while linting /my-project/db/migrations/20210413124138_rename_enum_to_pascal_case.js:2
Rule: "knex/avoid-injections"
    at CallExpression[callee.property.name=/^(raw|whereRaw|joinRaw)$/][arguments.0.type!='Literal'] (/my-project/node_modules/eslint-plugin-knex/rules/avoid-injections.js:24:62)
    at ruleErrorHandler (/my-project/node_modules/eslint/lib/linter/linter.js:1076:28)
    at /my-project/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/my-project/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/my-project/node_modules/eslint/lib/linter/node-event-generator.js:297:26)
    at NodeEventGenerator.applySelectors (/my-project/node_modules/eslint/lib/linter/node-event-generator.js:326:22)
    at NodeEventGenerator.enterNode (/my-project/node_modules/eslint/lib/linter/node-event-generator.js:340:14)
    at CodePathAnalyzer.enterNode (/my-project/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:795:23)
    at /my-project/node_modules/eslint/lib/linter/linter.js:1107:32

Update

Added a PR #17 that adds the test cases for knex.schema.raw function to aid you in fixing the issue.

Avoid analysing non-knex code if possible

As seen in #5 any kind of code using .raw(), or the other raw queries searched for in avoid-injections are hitting this rule. It would be great if the subject could for sure be either knex "root object" or a transaction. I'm not sure if it's possible without doing some back referencing gymnastics. But perhaps worth looking into at least.

The point is, returning early if !queryNode is great, but I can see pretty simple cases that would give false negatives on that rule.

`this.knex` is not identified as knex client and cause parsing to fail

I set up knex client as specified in the doc

Lint fails on this file

I strip down the file to a maximum and still gets the following message

TypeError: Cannot read properties of undefined (reading 'name')
Occurred while linting /home/octo-topi/Documents/Octo/Missions/Pix/repositories/pix/api/db/database-builder/database-builder.js:9
Rule: "knex/avoid-injections

Here is the stripped version

class Test {
  constructor({ knex}) {
    this.knex = knex;
  }

  async query() {
    const query = 'select ? from users';
    const bindingParameters = ['email'];
    await this.knex.raw(query, bindingParameters);
  }
}

export { Test };

.eslintrc.cjs is below - With no settings, lint pass

  settings: {
    knex: {
      builderName: "^(knex)$"
    }
  }

Look like the callee structrure is quite different with this

So the node.callee.object.name does not exists

        if (context.settings && context.settings.knex) {
          const builder = node.callee.object;
          const builderName = builder.name || builder.callee.name;

Handling of more complex knexRaw queries

~~Description in progress.~~

Issue fixed by refactoring code.

Add a single transaction rule

🦄 Problem

Knex implements explicit transactions, but allow to mix

a query in an explicit transaction
a query in the default transaction

Such a code would not be implemented purposefully, and may end up exhausting the connection pool

//  Take a connection A from the pool
//  Initiate a transaction on connection A

knex.transaction((trx) => {

   //  Take a connection B from the pool
   //  Initiate a transaction on connection B
  
  // Bug here, the code should be trx.update()
  knex.update();

   //  Validate transaction in connection B 
   //  Release connection B to the pool

}) 
//  Validate transaction in connection A 
//  Release connection A to the pool

Such an issue has been raised in knex repo

🤖 Solution

Implement a rule that does not allow mixing transactions

🌈 Remarks

Such a behavior occurred in production in this repository yesterday, blocking hundred of thousand of users

FYI, the original fix is here

Bug: Query string from another scope triggers error

There is a bug when the string used for queries are located in another scope.

const END_TIME_QUERY = `now() + interval '123 seconds'`

export default async function updateEndTime() {
  await await db('foo').update({
    ends_at: knex.raw(END_TIME_QUERY),
  })
}

This triggers an error in eslint-plugin-knex/rules/avoid-injections.js:44:56

      .getScope(queryNode)
      .variables.find((v) => v.name === queryNode.name).defs[0].node;
                                                         // ^ Here

Cannot read property 'type' of undefined

I'm getting a type of undefined on this line
node_modules/eslint-plugin-knex/rules/avoid-injections.js:30:15

code triggering the issue

const sharp = require('sharp');

sharp(input).raw();

Optionally add havingRaw, orderByRaw, groupByRaw

Have you considered other Raw methods to be added (probably as separate rules)?
Any ideas? :)

Critical security issue on npm audit

Hi! We're using this plugin as a part of our ESlint config, but there is a critical security issue thrown by npm audit that would require eslint-plugin-knex to update the eslint-remote-tester to the latest version to be fixed.

# npm audit report

simple-git  <=3.15.1
Severity: critical
Command injection in simple-git - https://github.com/advisories/GHSA-3f95-r44v-8mrg
Remote code execution in simple-git - https://github.com/advisories/GHSA-9w5j-4mwv-2wj8
Command injection in simple-git - https://github.com/advisories/GHSA-28xr-mwxg-3qc8
simple-git vulnerable to Remote Code Execution when enabling the ext transport protocol - https://github.com/advisories/GHSA-9p95-fxvg-qgq2
fix available via `npm audit fix --force`
Will install [...]/eslint-config-[...]@4.1.0, which is a breaking change
node_modules/simple-git
  eslint-remote-tester  <=2.1.1
  Depends on vulnerable versions of simple-git
  node_modules/eslint-remote-tester
    eslint-plugin-knex  >=0.2.0
    Depends on vulnerable versions of eslint-remote-tester
    node_modules/eslint-plugin-knex
      [...]/eslint-config-[...] >=4.1.1
      Depends on vulnerable versions of eslint-plugin-knex
      node_modules/[...]/eslint-config-[...]