feat(templating): Safer Jinja template processing (apache#11704)

* Enable safer Jinja template processing

* Allow JINJA_CONTEXT_ADDONS with SAFE_JINJA_PROCESSING

* Make template processor initialization less magical, refactor classes

* Consolidat Jinja logic, remove config flag in favor of sane defaults

* Restore previous ENABLE_TEMPLATE_PROCESSING default

* Add recursive type checking, update tests

* remove erroneous config file

* Remove TableColumn models from template context

* pylint refactoring

* Add entry to UPDATING.md

* Resolve botched merge conflict

* Update docs on running single python test

* Refactor template context checking to support engine-specific methods

CONTRIBUTING.md

@@ -607,7 +607,7 @@ tox -e <environment> -- tests/test_file.py
 or for a specific test via,
 
 ```bash
-tox -e <environment> -- tests/test_file.py:TestClassName.test_method_name
+tox -e <environment> -- tests/test_file.py::TestClassName::test_method_name
 ```
 
 Note that the test environment uses a temporary directory for defining the

UPDATING.md

@@ -24,6 +24,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`.
+
 - [11509](https://github.com/apache/incubator-superset/pull/11509): Config value `TABLE_NAMES_CACHE_CONFIG` has been renamed to `DATA_CACHE_CONFIG`, which will now also hold query results cache from connected datasources (previously held in `CACHE_CONFIG`), in addition to the table names. If you will set `DATA_CACHE_CONFIG` to a new cache backend different than your previous `CACHE_CONFIG`, plan for additional cache warmup to avoid degrading charting performance for the end users.
 
 - [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`.
@@ -38,7 +40,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 by default. To turn it on set `ENABLE_TEMPLATE_PROCESSING` to True on `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/__init__.py

@@ -26,7 +26,6 @@
     db,
     event_logger,
     feature_flag_manager,
-    jinja_context_manager,
     manifest_processor,
     results_backend_manager,
     security_manager,
@@ -44,7 +43,6 @@
 get_feature_flags = feature_flag_manager.get_feature_flags
 get_manifest_files = manifest_processor.get_manifest_files
 is_feature_enabled = feature_flag_manager.is_feature_enabled
-jinja_base_context = jinja_context_manager.base_context
 results_backend = LocalProxy(lambda: results_backend_manager.results_backend)
 results_backend_use_msgpack = LocalProxy(
     lambda: results_backend_manager.should_use_msgpack

superset/app.py

@@ -35,7 +35,6 @@
     csrf,
     db,
     feature_flag_manager,
-    jinja_context_manager,
     machine_auth_provider_factory,
     manifest_processor,
     migrate,
@@ -515,7 +514,6 @@ def init_app(self) -> None:
         self.configure_logging()
         self.configure_middlewares()
         self.configure_cache()
-        self.configure_jinja_context()
 
         with self.flask_app.app_context():  # type: ignore
             self.init_app_in_ctx()
@@ -573,9 +571,6 @@ def configure_url_map_converters(self) -> None:
         self.flask_app.url_map.converters["regex"] = RegexConverter
         self.flask_app.url_map.converters["object_type"] = ObjectTypeConverter
 
-    def configure_jinja_context(self) -> None:
-        jinja_context_manager.init_app(self.flask_app)
-
     def configure_middlewares(self) -> None:
         if self.config["ENABLE_CORS"]:
             from flask_cors import CORS

superset/config.py

@@ -672,14 +672,19 @@ class CeleryConfig:  # pylint: disable=too-few-public-methods
 # A dictionary of items that gets merged into the Jinja context for
 # SQL Lab. The existing context gets updated with this dictionary,
 # meaning values for existing keys get overwritten by the content of this
-# dictionary.
+# dictionary. Exposing functionality through JINJA_CONTEXT_ADDONS has security
+# implications as it opens a window for a user to execute untrusted code.
+# It's important to make sure that the objects exposed (as well as objects attached
+# to those objets) are harmless. We recommend only exposing simple/pure functions that
+# return native types.
 JINJA_CONTEXT_ADDONS: Dict[str, Callable[..., Any]] = {}
 
-# A dictionary of macro template processors that gets merged into global
+# A dictionary of macro template processors (by engine) that gets merged into global
 # template processors. The existing template processors get updated with this
 # dictionary, which means the existing keys get overwritten by the content of this
-# dictionary. The customized addons don't necessarily need to use jinjia templating
-# language. This allows you to define custom logic to process macro template.
+# dictionary. The customized addons don't necessarily need to use Jinja templating
+# language. This allows you to define custom logic to process templates on a per-engine
+# basis. Example value = `{"presto": CustomPrestoTemplateProcessor}`
 CUSTOM_TEMPLATE_PROCESSORS: Dict[str, Type[BaseTemplateProcessor]] = {}
 
 # Roles that are controlled by the API / Superset and should not be changes

superset/connectors/sqla/models.py

@@ -875,14 +875,14 @@ def get_sqla_query(  # pylint: disable=too-many-arguments,too-many-locals,too-ma
     ) -> SqlaQuery:
         """Querying any sqla table from this common interface"""
         template_kwargs = {
-            "from_dttm": from_dttm,
+            "from_dttm": from_dttm.isoformat() if from_dttm else None,
             "groupby": groupby,
             "metrics": metrics,
             "row_limit": row_limit,
             "row_offset": row_offset,
-            "to_dttm": to_dttm,
+            "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,15 +16,10 @@
 # under the License.
 import json
 import os
-import random
-import time
-import uuid
-from datetime import datetime, timedelta
-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
-from dateutil.relativedelta import relativedelta
 from flask import Flask
 from flask_appbuilder import AppBuilder, SQLA
 from flask_migrate import Migrate
@@ -36,37 +31,6 @@
 from superset.utils.feature_flag_manager import FeatureFlagManager
 from superset.utils.machine_auth import MachineAuthProviderFactory
 
-if TYPE_CHECKING:
-    from superset.jinja_context import BaseTemplateProcessor
-
-
-class JinjaContextManager:
-    def __init__(self) -> None:
-        self._base_context = {
-            "datetime": datetime,
-            "random": random,
-            "relativedelta": relativedelta,
-            "time": time,
-            "timedelta": timedelta,
-            "uuid1": uuid.uuid1,
-            "uuid3": uuid.uuid3,
-            "uuid4": uuid.uuid4,
-            "uuid5": uuid.uuid5,
-        }
-        self._template_processors: Dict[str, Type["BaseTemplateProcessor"]] = {}
-
-    def init_app(self, app: Flask) -> None:
-        self._base_context.update(app.config["JINJA_CONTEXT_ADDONS"])
-        self._template_processors.update(app.config["CUSTOM_TEMPLATE_PROCESSORS"])
-
-    @property
-    def base_context(self) -> Dict[str, Any]:
-        return self._base_context
-
-    @property
-    def template_processors(self) -> Dict[str, Type["BaseTemplateProcessor"]]:
-        return self._template_processors
-
 
 class ResultsBackendManager:
     def __init__(self) -> None:
@@ -140,7 +104,6 @@ def get_manifest_files(self, bundle: str, asset_type: str) -> List[str]:
 _event_logger: Dict[str, Any] = {}
 event_logger = LocalProxy(lambda: _event_logger.get("event_logger"))
 feature_flag_manager = FeatureFlagManager()
-jinja_context_manager = JinjaContextManager()
 machine_auth_provider_factory = MachineAuthProviderFactory()
 manifest_processor = UIManifestProcessor(APP_DIR)
 migrate = Migrate()