[REF] sql-injection: Consider SQL again

pylint_odoo/checkers/no_modules.py

@@ -431,6 +431,15 @@ def colon_list_to_dict(self, colon_list):
         """
         return dict(item.split(":") for item in colon_list)
 
+    def _sqli_allowable(self, node):
+        if isinstance(node, astroid.Call):
+            node = node.func
+        # self._thing is OK (mostly self._table), self._thing() also because
+        # it's a common pattern of reports (self._select, self._group_by, ...)
+        return (isinstance(node, astroid.Attribute)
+                and isinstance(node.expr, astroid.Name)
+                and node.attrname.startswith('_'))
+
     def _is_psycopg2_sql(self, node):
         if isinstance(node, astroid.Name):
             for assignation_node in self._get_assignation_nodes(node):
@@ -454,15 +463,6 @@ def _is_psycopg2_sql(self, node):
         if 'psycopg2' in package_names:
             return True
 
-    def _sqli_allowable(self, node):
-        if isinstance(node, astroid.Call):
-            node = node.func
-        # self._thing is OK (mostly self._table), self._thing() also because
-        # it's a common pattern of reports (self._select, self._group_by, ...)
-        return (isinstance(node, astroid.Attribute)
-                and isinstance(node.expr, astroid.Name)
-                and node.attrname.startswith('_'))
-
     def _check_node_for_sqli_risk(self, node):
         if isinstance(node, astroid.BinOp) and node.op in ('%', '+'):
             if isinstance(node.right, astroid.Tuple):
@@ -481,9 +481,16 @@ def _check_node_for_sqli_risk(self, node):
         # ignore sql.SQL().format
         if isinstance(node, astroid.Call) \
                 and isinstance(node.func, astroid.Attribute) \
-                and isinstance(node.func.expr, astroid.Const) \
                 and node.func.attrname == 'format':
 
+            # exclude sql.SQL or sql.Identifier
+            is_psycopg2 = (
+                list(map(self._is_psycopg2_sql, node.args)) +
+                [self._is_psycopg2_sql(keyword.value)
+                 for keyword in (node.keywords or [])])
+            if is_psycopg2 and all(is_psycopg2):
+                return False
+
             if not all(map(self._sqli_allowable, node.args or [])):
                 return True
 

pylint_odoo/test/main.py

@@ -56,7 +56,7 @@
     'print-used': 1,
     'redundant-modulename-xml': 1,
     'rst-syntax-error': 2,
-    'sql-injection': 16,
+    'sql-injection': 17,
     'str-format-used': 3,
     'translation-field': 2,
     'translation-required': 15,

pylint_odoo/test_repo/broken_module/models/broken_model.py

@@ -442,6 +442,10 @@ def sql_injection_format(self, ids, cr):
         self.cr.execute(
             'SELECT name FROM account WHERE id IN {}'.format(ids))
 
+        var = 'SELECT name FROM account WHERE id IN {}'
+        values = (1, 2, 3)
+        self._cr.execute(var.format(values))
+
     def sql_injection_plus_operator(self, ids, cr):
         # Use of +: risky
         self.cr.execute(