(superset) 02/08: fix: DB-specific quoting in Jinja macro (#25779)

[el...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

elizabeth pushed a commit to branch 2.1
in repository https://gitbox.apache.org/repos/asf/superset.git

commit 34101594e284ab3acce692f41aff7759ccb4bf1d
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Mon Oct 30 09:50:44 2023 -0400

    fix: DB-specific quoting in Jinja macro (#25779)
---
 superset/jinja_context.py              | 45 +++++++++++++++++++++++-----------
 tests/unit_tests/jinja_context_test.py |  9 +++++--
 2 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/superset/jinja_context.py b/superset/jinja_context.py
index d9409e297b..ffbab4f9bc 100644
--- a/superset/jinja_context.py
+++ b/superset/jinja_context.py
@@ -35,6 +35,7 @@ from flask_babel import gettext as _
 from jinja2 import DebugUndefined
 from jinja2.sandbox import SandboxedEnvironment
 from sqlalchemy.engine.interfaces import Dialect
+from sqlalchemy.sql.expression import bindparam
 from sqlalchemy.types import String
 from typing_extensions import TypedDict
 
@@ -407,23 +408,39 @@ def validate_template_context(
     return validate_context_types(context)
 
 
-def where_in(values: List[Any], mark: str = "'") -> str:
-    """
-    Given a list of values, build a parenthesis list suitable for an IN expression.
+class WhereInMacro:  # pylint: disable=too-few-public-methods
+    def __init__(self, dialect: Dialect):
+        self.dialect = dialect
 
-        >>> where_in([1, "b", 3])
-        (1, 'b', 3)
+    def __call__(self, values: List[Any], mark: Optional[str] = None) -> str:
+        """
+        Given a list of values, build a parenthesis list suitable for an IN expression.
 
-    """
+            >>> from sqlalchemy.dialects import mysql
+            >>> where_in = WhereInMacro(dialect=mysql.dialect())
+            >>> where_in([1, "Joe's", 3])
+            (1, 'Joe''s', 3)
 
-    def quote(value: Any) -> str:
-        if isinstance(value, str):
-            value = value.replace(mark, mark * 2)
-            return f"{mark}{value}{mark}"
-        return str(value)
+        """
+        binds = [bindparam(f"value_{i}", value) for i, value in enumerate(values)]
+        string_representations = [
+            str(
+                bind.compile(
+                    dialect=self.dialect, compile_kwargs={"literal_binds": True}
+                )
+            )
+            for bind in binds
+        ]
+        joined_values = ", ".join(string_representations)
+        result = f"({joined_values})"
+
+        if mark:
+            result += (
+                "\n-- WARNING: the `mark` parameter was removed from the `where_in` "
+                "macro for security reasons\n"
+            )
 
-    joined_values = ", ".join(quote(value) for value in values)
-    return f"({joined_values})"
+        return result
 
 
 class BaseTemplateProcessor:
@@ -459,7 +476,7 @@ class BaseTemplateProcessor:
         self.set_context(**kwargs)
 
         # custom filters
-        self._env.filters["where_in"] = where_in
+        self._env.filters["where_in"] = WhereInMacro(database.get_dialect())
 
     def set_context(self, **kwargs: Any) -> None:
         self._context.update(kwargs)
diff --git a/tests/unit_tests/jinja_context_test.py b/tests/unit_tests/jinja_context_test.py
index 13b3ae9e9c..16b1420d08 100644
--- a/tests/unit_tests/jinja_context_test.py
+++ b/tests/unit_tests/jinja_context_test.py
@@ -20,17 +20,22 @@ import json
 
 import pytest
 from pytest_mock import MockFixture
+from sqlalchemy.dialects import mysql
 
 from superset.datasets.commands.exceptions import DatasetNotFoundError
-from superset.jinja_context import dataset_macro, where_in
+from superset.jinja_context import dataset_macro, WhereInMacro
 
 
 def test_where_in() -> None:
     """
     Test the ``where_in`` Jinja2 filter.
     """
+    where_in = WhereInMacro(mysql.dialect())
     assert where_in([1, "b", 3]) == "(1, 'b', 3)"
-    assert where_in([1, "b", 3], '"') == '(1, "b", 3)'
+    assert where_in([1, "b", 3], '"') == (
+        "(1, 'b', 3)\n-- WARNING: the `mark` parameter was removed from the "
+        "`where_in` macro for security reasons\n"
+    )
     assert where_in(["O'Malley's"]) == "('O''Malley''s')"