feat(templating): Safer Jinja template processing

UPDATING.md

@@ -23,6 +23,8 @@ assists people when migrating to a new version.
 
 ## Next
 
+* [11704](https://github.com/apache/incubator-superset/pull/11704) Breaking change: Jinja templating for SQL queries has been updated, removing default modules such as `datetime` and `random` and enforcing static template values. To restore or extend functionality, use `JINJA_CONTEXT_ADDONS` and `CUSTOM_TEMPLATE_PROCESSORS` in `superset_config.py`.
+
 * [11575](https://github.com/apache/incubator-superset/pull/11575) The Row Level Security (RLS) config flag has been moved to a feature flag. To migrate, add `ROW_LEVEL_SECURITY: True` to the `FEATURE_FLAGS` dict in `superset_config.py`.
 
 * NOTICE: config flag ENABLE_REACT_CRUD_VIEWS has been set to `True` by default, set to `False` if
@@ -36,7 +38,7 @@ assists people when migrating to a new version.
   and requires more work. You can easily turn on the languages you want
   to expose in your environment in superset_config.py
 
-* [11172](https://github.com/apache/incubator-superset/pull/11172): Breaking change: SQL templating is turned off be default. To turn it on set `ENABLE_TEMPLATE_PROCESSING` to True on `DEFAULT_FEATURE_FLAGS`
+* [11172](https://github.com/apache/incubator-superset/pull/11172): Breaking change: SQL templating is turned off be default. To turn it on set `ENABLE_TEMPLATE_PROCESSING` to True in `FEATURE_FLAGS`
 
 * [11155](https://github.com/apache/incubator-superset/pull/11155): The `FAB_UPDATE_PERMS` config parameter is no longer required as the Superset application correctly informs FAB under which context permissions should be updated.
 

superset/config.py

@@ -301,7 +301,7 @@ def _try_json_readsha(  # pylint: disable=unused-argument
     # Experimental feature introducing a client (browser) cache
     "CLIENT_CACHE": False,
     "ENABLE_EXPLORE_JSON_CSRF_PROTECTION": False,
-    "ENABLE_TEMPLATE_PROCESSING": True,
+    "ENABLE_TEMPLATE_PROCESSING": False,
     "KV_STORE": False,
     "PRESTO_EXPAND_DATA": False,
     # Exposes API endpoint to compute thumbnails

superset/connectors/sqla/models.py

@@ -882,7 +882,7 @@ def get_sqla_query(  # pylint: disable=too-many-arguments,too-many-locals,too-ma
             "row_offset": row_offset,
             "to_dttm": to_dttm.isoformat() if to_dttm else None,
             "filter": filter,
-            "columns": {col.column_name: col for col in self.columns},
+            "columns": [col.column_name for col in self.columns],
         }
         is_sip_38 = is_feature_enabled("SIP_38_VIZ_REARCHITECTURE")
         template_kwargs.update(self.template_params_dict)

superset/extensions.py

@@ -16,7 +16,7 @@
 # under the License.
 import json
 import os
-from typing import Any, Callable, Dict, List, Optional, Type, TYPE_CHECKING
+from typing import Any, Callable, Dict, List, Optional
 
 import celery
 from cachelib.base import BaseCache

superset/jinja_context.py

@@ -15,6 +15,7 @@
 # specific language governing permissions and limitations
 # under the License.
 """Defines the templating context for SQL Lab"""
+import json
 import re
 from functools import partial
 from typing import Any, Callable, cast, Dict, List, Optional, Tuple, TYPE_CHECKING
@@ -48,7 +49,9 @@
     "list",
     "dict",
     "tuple",
+    "set",
 )
+COLLECTION_TYPES = ("list", "dict", "tuple", "set")
 
 
 @memoized
@@ -221,10 +224,41 @@ def safe_proxy(func: Callable[..., Any], *args: Any, **kwargs: Any) -> Any:
                 value_type=value_type,
             )
         )
+    if value_type in COLLECTION_TYPES:
+        try:
+            return_value = json.loads(json.dumps(return_value))
+        except TypeError:
+            raise SupersetTemplateException(
+                _("Unsupported return value for method %(name)s", name=func.__name__,)
+            )
 
     return return_value
 
 
+def validate_template_context(context: Dict[str, Any]) -> Dict[str, Any]:
+    for key in context:
+        arg_type = type(context[key]).__name__
+        if arg_type not in ALLOWED_TYPES and key not in context_addons():
+            if arg_type == "partial" and context[key].func.__name__ == "safe_proxy":
+                continue
+            raise SupersetTemplateException(
+                _(
+                    "Unsafe template value for key %(key)s: %(value_type)s",
+                    key=key,
+                    value_type=arg_type,
+                )
+            )
+        if arg_type in COLLECTION_TYPES:
+            try:
+                context[key] = json.loads(json.dumps(context[key]))
+            except TypeError:
+                raise SupersetTemplateException(
+                    _("Unsupported template value for key %(key)s", key=key,)
+                )
+
+    return context
+
+
 class BaseTemplateProcessor:  # pylint: disable=too-few-public-methods
     """
     Base class for database-specific jinja context
@@ -256,21 +290,6 @@ def set_context(self, **kwargs: Any) -> None:
         self._context.update(kwargs)
         self._context.update(context_addons())
 
-    def validate_template_context(self, context: Dict[str, Any]) -> None:
-        for key in context:
-            arg_type = type(context[key]).__name__
-            if arg_type not in ALLOWED_TYPES and key not in context_addons():
-                if arg_type == "partial" and context[key].func.__name__ == "safe_proxy":
-                    continue
-
-                raise SupersetTemplateException(
-                    _(
-                        "Unsafe template value for key %(key)s: %(value_type)s",
-                        key=key,
-                        value_type=arg_type,
-                    )
-                )
-
     def process_template(self, sql: str, **kwargs: Any) -> str:
         """Processes a sql template
 
@@ -281,8 +300,8 @@ def process_template(self, sql: str, **kwargs: Any) -> str:
         template = self._env.from_string(sql)
         kwargs.update(self._context)
 
-        self.validate_template_context(kwargs)
-        return template.render(kwargs)
+        context = validate_template_context(kwargs)
+        return template.render(context)
 
 
 class JinjaTemplateProcessor(BaseTemplateProcessor):
@@ -386,13 +405,13 @@ class HiveTemplateProcessor(PrestoTemplateProcessor):
 
 @memoized
 def template_processors() -> Dict[str, Any]:
-    template_processors = current_app.config.get("CUSTOM_TEMPLATE_PROCESSORS", {})
+    processors = current_app.config.get("CUSTOM_TEMPLATE_PROCESSORS", {})
     for engine in DEFAULT_PROCESSORS:
         # do not overwrite engine-specific CUSTOM_TEMPLATE_PROCESSORS
-        if not engine in template_processors:
-            template_processors[engine] = DEFAULT_PROCESSORS[engine]
+        if not engine in processors:
+            processors[engine] = DEFAULT_PROCESSORS[engine]
 
-    return template_processors
+    return processors
 
 
 def get_template_processor(

tests/core_tests.py

@@ -669,103 +669,14 @@ def test_extra_table_metadata(self):
             f"/superset/extra_table_metadata/{example_db.id}/birth_names/{schema}/"
         )
 
-    def test_process_template(self):
-        maindb = utils.get_example_database()
-        if maindb.backend == "presto":
-            # TODO: make it work for presto
-            return
-        sql = "SELECT '{{ datetime(2017, 1, 1).isoformat() }}'"
-        tp = jinja_context.get_template_processor(database=maindb)
-        rendered = tp.process_template(sql)
-        self.assertEqual("SELECT '2017-01-01T00:00:00'", rendered)
-
-    def test_get_template_kwarg(self):
-        maindb = utils.get_example_database()
-        if maindb.backend == "presto":
-            # TODO: make it work for presto
-            return
-        s = "{{ foo }}"
-        tp = jinja_context.get_template_processor(database=maindb, foo="bar")
-        rendered = tp.process_template(s)
-        self.assertEqual("bar", rendered)
-
-    def test_template_kwarg(self):
-        maindb = utils.get_example_database()
-        if maindb.backend == "presto":
-            # TODO: make it work for presto
-            return
-        s = "{{ foo }}"
-        tp = jinja_context.get_template_processor(database=maindb)
-        rendered = tp.process_template(s, foo="bar")
-        self.assertEqual("bar", rendered)
-
     def test_templated_sql_json(self):
         if utils.get_example_database().backend == "presto":
             # TODO: make it work for presto
             return
         self.login()
-        sql = "SELECT '{{ datetime(2017, 1, 1).isoformat() }}' as test"
+        sql = "SELECT '{{ 1+1 }}' as test"
         data = self.run_sql(sql, "fdaklj3ws")
-        self.assertEqual(data["data"][0]["test"], "2017-01-01T00:00:00")
-
-    @mock.patch("tests.superset_test_custom_template_processors.datetime")
-    def test_custom_process_template(self, mock_dt) -> None:
-        """Test macro defined in custom template processor works."""
-        mock_dt.utcnow = mock.Mock(return_value=datetime.datetime(1970, 1, 1))
-        db = mock.Mock()
-        db.backend = "db_for_macros_testing"
-        tp = jinja_context.get_template_processor(database=db)
-
-        sql = "SELECT '$DATE()'"
-        rendered = tp.process_template(sql)
-        self.assertEqual("SELECT '{}'".format("1970-01-01"), rendered)
-
-        sql = "SELECT '$DATE(1, 2)'"
-        rendered = tp.process_template(sql)
-        self.assertEqual("SELECT '{}'".format("1970-01-02"), rendered)
-
-    def test_custom_get_template_kwarg(self):
-        """Test macro passed as kwargs when getting template processor
-        works in custom template processor."""
-        db = mock.Mock()
-        db.backend = "db_for_macros_testing"
-        s = "$foo()"
-        tp = jinja_context.get_template_processor(database=db, foo=lambda: "bar")
-        rendered = tp.process_template(s)
-        self.assertEqual("bar", rendered)
-
-    def test_custom_template_kwarg(self) -> None:
-        """Test macro passed as kwargs when processing template
-        works in custom template processor."""
-        db = mock.Mock()
-        db.backend = "db_for_macros_testing"
-        s = "$foo()"
-        tp = jinja_context.get_template_processor(database=db)
-        rendered = tp.process_template(s, foo=lambda: "bar")
-        self.assertEqual("bar", rendered)
-
-    def test_custom_template_processors_overwrite(self) -> None:
-        """Test template processor for presto gets overwritten by custom one."""
-        db = mock.Mock()
-        db.backend = "db_for_macros_testing"
-        tp = jinja_context.get_template_processor(database=db)
-
-        sql = "SELECT '{{ datetime(2017, 1, 1).isoformat() }}'"
-        rendered = tp.process_template(sql)
-        self.assertEqual(sql, rendered)
-
-        sql = "SELECT '{{ DATE(1, 2) }}'"
-        rendered = tp.process_template(sql)
-        self.assertEqual(sql, rendered)
-
-    def test_custom_template_processors_ignored(self) -> None:
-        """Test custom template processor is ignored for a difference backend
-        database."""
-        maindb = utils.get_example_database()
-        sql = "SELECT '$DATE()'"
-        tp = jinja_context.get_template_processor(database=maindb)
-        rendered = tp.process_template(sql)
-        assert sql == rendered
+        self.assertEqual(data["data"][0]["test"], "2")
 
     @mock.patch("tests.superset_test_custom_template_processors.datetime")
     @mock.patch("superset.sql_lab.get_sql_results")