Improve unsafe query detection

package.json

@@ -21,9 +21,6 @@
     "type": "git",
     "url": "git+ssh://git@github.com/uphold/eslint-plugin-sql-template.git"
   },
-  "dependencies": {
-    "sql-parse": "^0.1.5"
-  },
   "engines": {
     "node": ">=20"
   },

rules/no-unsafe-query.js

@@ -1,77 +1,108 @@
 'use strict';
 
 /**
- * Module dependencies.
+ * Helper function to check if an expression contains a variable.
  */
 
-const parser = require('sql-parse');
+function containsVariableExpression(expression) {
+  if (!expression) return false;
 
-/**
- * Check if `literal` is an SQL query.
- */
-
-function isSqlQuery(literal) {
-  if (!literal) {
-    return false;
+  if (['Identifier', 'CallExpression', 'MemberExpression'].includes(expression.type)) {
+    return true;
   }
 
-  try {
-    parser.parse(literal);
+  if (expression.type === 'ConditionalExpression') {
+    return containsVariableExpression(expression.consequent) || containsVariableExpression(expression.alternate);
+  }
 
-    // eslint-disable-next-line no-unused-vars
-  } catch (error) {
-    return false;
+  if (expression.type === 'TemplateLiteral') {
+    return expression.expressions.some(containsVariableExpression);
   }
 
-  return true;
+  return false;
 }
 
 /**
- * Validate node.
+ * Helper function to check if a node has a parent that is a template literal.
  */
 
-function validate(node, context) {
-  if (!node) {
-    return;
-  }
+function hasParentTemplateLiteral(node) {
+  if (!node?.parent) return false;
 
-  if (node.type === 'TaggedTemplateExpression' && node.tag.name !== 'sql') {
-    node = node.quasi;
+  if (node.parent.type === 'TemplateLiteral') {
+    return true;
   }
 
-  if (node.type === 'TemplateLiteral' && node.expressions.length) {
-    const literal = node.quasis.map(quasi => quasi.value.raw).join('x');
-
-    if (isSqlQuery(literal)) {
-      context.report({
-        node,
-        message: 'Use the `sql` tagged template literal for raw queries'
-      });
-    }
-  }
+  return hasParentTemplateLiteral(node.parent);
 }
 
 /**
- * Export `no-unsafe-query`.
+ * SQL starting keywords to detect inside the template literal.
+ */
+
+const sqlKeywords = /^`\s*(SELECT|INSERT\s+INTO|UPDATE|DELETE\s+FROM|WITH|GRANT|BEGIN|DROP)\s/i;
+
+/**
+ * Rule definition.
  */
 
 module.exports = {
   meta: {
     type: 'suggestion',
+    hasSuggestions: true,
+    fixable: 'code',
     docs: {
-      description: 'disallow unsafe SQL queries',
+      description: 'Enforce safe SQL query handling using tagged templates',
       recommended: false,
       url: 'https://github.com/uphold/eslint-plugin-sql-template#rules'
     },
-    schema: [] // no options
+    messages: {
+      missingSqlTag: 'Use the `sql` tagged template literal for raw queries'
+    },
+    schema: []
   },
   create(context) {
     return {
-      CallExpression(node) {
-        node.arguments.forEach(argument => validate(argument, context));
-      },
-      VariableDeclaration(node) {
-        node.declarations.forEach(declaration => validate(declaration.init, context));
+      TemplateLiteral(node) {
+        // Only check interpolated template literals.
+        if (node?.type !== 'TemplateLiteral' || node.expressions.length === 0) {
+          return;
+        }
+
+        // Skip if the template literal has in it's chain a parent that is a TemplateLiteral.
+        if (hasParentTemplateLiteral(node)) {
+          return;
+        }
+
+        // Skip if the template literal is already tagged with `sql`.
+        if (node.parent.type === 'TaggedTemplateExpression' && node.parent.tag.name === 'sql') {
+          return;
+        }
+
+        // Check if the template literal has SQL.
+        const hasSQL = sqlKeywords.test(context.sourceCode.getText(node));
+
+        // Recursively check if any expression is a variable (Identifier, MemberExpression, or nested TemplateLiteral)
+        const hasVariableExpression = node.expressions.some(containsVariableExpression);
+
+        if (hasSQL && hasVariableExpression) {
+          context.report({
+            node,
+            messageId: 'missingSqlTag',
+            suggest: [
+              {
+                desc: 'Wrap with sql tag',
+                fix(fixer) {
+                  if (node.parent?.type === 'TaggedTemplateExpression') {
+                    return fixer.replaceText(node.parent.tag, 'sql');
+                  }
+
+                  return fixer.insertTextBefore(node, 'sql');
+                }
+              }
+            ]
+          });
+        }
       }
     };
   }

test/rules/no-unsafe-query_test.js

@@ -7,9 +7,14 @@
 const { RuleTester } = require('eslint');
 const rule = require('../../rules/no-unsafe-query');
 
+/**
+ * Configure the rule tester.
+ */
+
 RuleTester.setDefaultConfig({
   languageOptions: {
-    ecmaVersion: 2022
+    ecmaVersion: 2020,
+    sourceType: 'module'
   }
 });
 
@@ -26,7 +31,13 @@ ruleTester.run('no-unsafe-query', rule, {
       errors: [
         {
           message: 'Use the `sql` tagged template literal for raw queries',
-          type: 'TemplateLiteral'
+          type: 'TemplateLiteral',
+          suggestions: [
+            {
+              desc: 'Wrap with sql tag',
+              output: 'const column = "*"; foo.query(sql`SELECT ${column} FROM foobar`);'
+            }
+          ]
         }
       ]
     },
@@ -35,7 +46,13 @@ ruleTester.run('no-unsafe-query', rule, {
       errors: [
         {
           message: 'Use the `sql` tagged template literal for raw queries',
-          type: 'TemplateLiteral'
+          type: 'TemplateLiteral',
+          suggestions: [
+            {
+              desc: 'Wrap with sql tag',
+              output: 'const column = "*"; const query = sql`SELECT ${column} FROM foobar`; foo.query(query);'
+            }
+          ]
         }
       ]
     },
@@ -44,7 +61,13 @@ ruleTester.run('no-unsafe-query', rule, {
       errors: [
         {
           message: 'Use the `sql` tagged template literal for raw queries',
-          type: 'TemplateLiteral'
+          type: 'TemplateLiteral',
+          suggestions: [
+            {
+              desc: 'Wrap with sql tag',
+              output: 'const column = "*"; foo.query(sql`SELECT ${column} FROM foobar`);'
+            }
+          ]
         }
       ]
     },
@@ -53,19 +76,110 @@ ruleTester.run('no-unsafe-query', rule, {
       errors: [
         {
           message: 'Use the `sql` tagged template literal for raw queries',
-          type: 'TemplateLiteral'
+          type: 'TemplateLiteral',
+          suggestions: [
+            {
+              desc: 'Wrap with sql tag',
+              output: 'const column = "*"; const query = sql`SELECT ${column} FROM foobar`; foo.query(query);'
+            }
+          ]
+        }
+      ]
+    },
+    {
+      code: 'const foo = { id: 123 }; fooManager.query(`UPDATE "Foo" SET "setAt" = NULL WHERE id = \'${foo.id}\'`, { transaction });',
+      errors: [
+        {
+          message: 'Use the `sql` tagged template literal for raw queries',
+          type: 'TemplateLiteral',
+          suggestions: [
+            {
+              desc: 'Wrap with sql tag',
+              output:
+                'const foo = { id: 123 }; fooManager.query(sql`UPDATE "Foo" SET "setAt" = NULL WHERE id = \'${foo.id}\'`, { transaction });'
+            }
+          ]
+        }
+      ]
+    },
+    {
+      code: 'const batchSize = 100; const query = `WITH selected AS ( SELECT id FROM "foobar" WHERE "default" IS NULL LIMIT ${batchSize} FOR NO KEY UPDATE SKIP LOCKED )`;',
+      errors: [
+        {
+          message: 'Use the `sql` tagged template literal for raw queries',
+          type: 'TemplateLiteral',
+          suggestions: [
+            {
+              desc: 'Wrap with sql tag',
+              output:
+                'const batchSize = 100; const query = sql`WITH selected AS ( SELECT id FROM "foobar" WHERE "default" IS NULL LIMIT ${batchSize} FOR NO KEY UPDATE SKIP LOCKED )`;'
+            }
+          ]
+        }
+      ]
+    },
+    {
+      code: `const skipped = ['abc']; const query = \` SELECT count(*) as total FROM "foobar" WHERE "bizbaz" = 'foobiz' AND foo - 'bar' - 'biz' - 'baz' = '{}' \${skipped.length > 0 ? \`AND biz NOT IN (\${skipped.map(foo => \`'\${foo}'\`).join(',')}))\` : ''}\``,
+      errors: [
+        {
+          message: 'Use the `sql` tagged template literal for raw queries',
+          type: 'TemplateLiteral',
+          suggestions: [
+            {
+              desc: 'Wrap with sql tag',
+              output: `const skipped = ['abc']; const query = sql\` SELECT count(*) as total FROM "foobar" WHERE "bizbaz" = 'foobiz' AND foo - 'bar' - 'biz' - 'baz' = '{}' \${skipped.length > 0 ? \`AND biz NOT IN (\${skipped.map(foo => \`'\${foo}'\`).join(',')}))\` : ''}\``
+            }
+          ]
+        }
+      ]
+    },
+    {
+      code: `async function updateRecords() { const [updated] = await queryInterface.sequelize.query(\` WITH selected AS ( SELECT id FROM "Foobiz" WHERE "default" IS NULL LIMIT \${batchSize} FOR NO KEY UPDATE SKIP LOCKED ), "foobar" AS ( SELECT up.id FROM "FooBarBiz" up, selected s WHERE up.id = s.id AND up.main IS TRUE AND up."deletedAt" IS NULL ) UPDATE "Foobiz" am SET "default" = CASE WHEN fooo.id IS NULL THEN FALSE ELSE TRUE END FROM selected s LEFT JOIN "foobar" fooo ON s.id = fooo.id WHERE am.id = s.id RETURNING am.id \`, { transaction }); }`,
+      errors: [
+        {
+          message: 'Use the `sql` tagged template literal for raw queries',
+          type: 'TemplateLiteral',
+          suggestions: [
+            {
+              desc: 'Wrap with sql tag',
+              output: `async function updateRecords() { const [updated] = await queryInterface.sequelize.query(sql\` WITH selected AS ( SELECT id FROM "Foobiz" WHERE "default" IS NULL LIMIT \${batchSize} FOR NO KEY UPDATE SKIP LOCKED ), "foobar" AS ( SELECT up.id FROM "FooBarBiz" up, selected s WHERE up.id = s.id AND up.main IS TRUE AND up."deletedAt" IS NULL ) UPDATE "Foobiz" am SET "default" = CASE WHEN fooo.id IS NULL THEN FALSE ELSE TRUE END FROM selected s LEFT JOIN "foobar" fooo ON s.id = fooo.id WHERE am.id = s.id RETURNING am.id \`, { transaction }); }`
+            }
+          ]
+        }
+      ]
+    },
+    {
+      code: 'const totalQuery = () => `SELECT COUNT(*) as total FROM "foo" u WHERE NOT EXISTS (SELECT 1 FROM "fooBar" ue WHERE ue."FooId" = u.id LIMIT 1) ${foobiz()};`;',
+      errors: [
+        {
+          message: 'Use the `sql` tagged template literal for raw queries',
+          type: 'TemplateLiteral',
+          suggestions: [
+            {
+              desc: 'Wrap with sql tag',
+              output:
+                'const totalQuery = () => sql`SELECT COUNT(*) as total FROM "foo" u WHERE NOT EXISTS (SELECT 1 FROM "fooBar" ue WHERE ue."FooId" = u.id LIMIT 1) ${foobiz()};`;'
+            }
+          ]
         }
       ]
     }
   ],
   valid: [
-    'const column = "*"; foo.query(sql`SELECT ${column} FROM foobar`);',
-    'const column = "*"; const query = sql`SELECT ${column} FROM foobar`; foo.query(query);',
-    'foo.query(`SELECT column FROM foobar`);',
-    'const query = `SELECT column FROM foobar`; foo.query(query);',
-    'const foo = "bar"; baz.greet(`hello ${foo}`);',
-    'const foo = "bar"; const baz = `hello ${foo}`; qux.greet(baz);',
-    'foo.greet(`hello`);',
-    'const foo = `bar`; baz.greet(foo);'
+    { code: 'const column = "*"; foo.query(sql`SELECT ${column} FROM foobar`);' },
+    { code: 'const column = "*"; const query = sql`SELECT ${column} FROM foobar`; foo.query(query);' },
+    { code: 'const query = sql`SELECT column FROM foobar`; foo.query(query);' },
+    { code: 'foo.query(sql`SELECT ${column} FROM foobar`);' },
+    { code: 'const query = sql`SELECT ${column} FROM foobar`; foo.query(query);' },
+    { code: 'const foo = "bar"; baz.greet(`hello ${foo}`);' },
+    { code: 'const foo = "bar"; const baz = `hello ${foo}`; qux.greet(baz);' },
+    { code: 'foo.greet(`hello`);' },
+    { code: 'const foo = `bar`; baz.greet(foo);' },
+    { code: 'db.query(`SELECT foo FROM bar`);' }, // Raw SQL without interpolation is valid
+    { code: 'const query = `SELECT * FROM users WHERE active = true`;' }, // Also valid
+    { code: 'foo.query(`SELECT * FROM table WHERE id = 1`);' }, // Also valid
+    { code: 'db.query(`SELECT foo FROM bar WHERE biz = ${"foo"}`);' }, // Raw SQL with interpolation of a literal is valid
+    { code: 'log.info(`This will update ${total} records`)' },
+    { code: 'const token = crypto.generateToken(32); redis.set(sha1`password-reset:token:${token}`);' }
   ]
 });