From 7e45c6737531d14dc8328cb9159407a73bff5445 Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Fri, 13 Nov 2020 18:30:11 -0800 Subject: [PATCH 01/13] Enable safer Jinja template processing --- superset/config.py | 6 ++- superset/extensions.py | 24 +++++----- superset/jinja_context.py | 93 ++++++++++++++++++++++++++++++++------- 3 files changed, 94 insertions(+), 29 deletions(-) diff --git a/superset/config.py b/superset/config.py index 04cb70fd1745f..16b9d35bbd855 100644 --- a/superset/config.py +++ b/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": False, + "ENABLE_TEMPLATE_PROCESSING": True, "KV_STORE": False, "PRESTO_EXPAND_DATA": False, # Exposes API endpoint to compute thumbnails @@ -677,6 +677,10 @@ class CeleryConfig: # pylint: disable=too-few-public-methods # language. This allows you to define custom logic to process macro template. CUSTOM_TEMPLATE_PROCESSORS: Dict[str, Type[BaseTemplateProcessor]] = {} +# Prevent access to classes/objects and proxy methods in the default Jinja context, +# unless explicitly overridden by JINJA_CONTEXT_ADDONS or CUSTOM_TEMPLATE_PROCESSORS. +SAFE_JINJA_PROCESSING: bool = True + # Roles that are controlled by the API / Superset and should not be changes # by humans. ROBOT_PERMISSION_ROLES = ["Public", "Gamma", "Alpha", "Admin", "sql_lab"] diff --git a/superset/extensions.py b/superset/extensions.py index 9be0c37bb937a..5b980641cd395 100644 --- a/superset/extensions.py +++ b/superset/extensions.py @@ -42,20 +42,22 @@ 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._base_context: Dict[str, Any] = {} self._template_processors: Dict[str, Type["BaseTemplateProcessor"]] = {} def init_app(self, app: Flask) -> None: + if not app.config["SAFE_JINJA_PROCESSING"]: + 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._base_context.update(app.config["JINJA_CONTEXT_ADDONS"]) self._template_processors.update(app.config["CUSTOM_TEMPLATE_PROCESSORS"]) diff --git a/superset/jinja_context.py b/superset/jinja_context.py index 988aea89bdb2a..010415bddad4f 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -17,12 +17,14 @@ """Defines the templating context for SQL Lab""" import inspect import re -from typing import Any, cast, List, Optional, Tuple, TYPE_CHECKING +from functools import partial +from typing import Any, Callable, cast, Dict, List, Optional, Tuple, TYPE_CHECKING -from flask import g, request -from jinja2.sandbox import SandboxedEnvironment +from flask import current_app, g, request +from jinja2.sandbox import ImmutableSandboxedEnvironment, SandboxedEnvironment from superset import jinja_base_context +from superset.exceptions import SupersetTemplateException from superset.extensions import feature_flag_manager, jinja_context_manager from superset.utils.core import convert_legacy_filters_into_adhoc, merge_extra_filters @@ -151,7 +153,7 @@ def cache_key_wrapper(self, key: Any) -> Any: def url_param( self, param: str, default: Optional[str] = None, add_to_cache_keys: bool = True - ) -> Optional[Any]: + ) -> Optional[str]: """ Read a url or post parameter and use it in your SQL Lab query. @@ -186,6 +188,28 @@ def url_param( return result +def safe_proxy(func: Callable[..., Any], *args: Any, **kwargs: Any) -> Any: + none_type = type(None).__name__ + allowed_types = [ + none_type, + "bool", + "str", + "unicode", + "int", + "long", + "float", + "list", + "dict", + "tuple", + ] + return_value = func(*args, **kwargs) + value_type = type(return_value).__name__ + if value_type not in allowed_types: + raise SupersetTemplateException("Unsafe template value") + + return return_value + + class BaseTemplateProcessor: # pylint: disable=too-few-public-methods """Base class for database-specific jinja context @@ -218,9 +242,33 @@ def __init__( self._schema = query.schema elif table: self._schema = table.schema + self._extra_cache_keys = extra_cache_keys + self._context: Dict[str, Any] = {} + self.set_env() + self.set_context(**kwargs) + + def set_env(self) -> None: + self._env = SandboxedEnvironment() - extra_cache = ExtraCache(extra_cache_keys) + def set_context(self, **kwargs: Any) -> None: + self._context.update(kwargs) + self._context.update(jinja_base_context) + + def process_template(self, sql: str, **kwargs: Any) -> str: + """Processes a sql template + + >>> sql = "SELECT '{{ datetime(2017, 1, 1).isoformat() }}'" + >>> process_template(sql) + "SELECT '2017-01-01T00:00:00'" + """ + template = self._env.from_string(sql) + kwargs.update(self._context) + return template.render(kwargs) + +class JinjaTemplateProcessor(BaseTemplateProcessor): + def set_context(self, **kwargs: Any) -> None: + extra_cache = ExtraCache(self._extra_cache_keys) self._context = { "url_param": extra_cache.url_param, "current_user_id": extra_cache.current_user_id, @@ -229,22 +277,28 @@ def __init__( "filter_values": filter_values, "form_data": {}, } + self._context.update(kwargs) self._context.update(jinja_base_context) + if self.engine: self._context[self.engine] = self - self._env = SandboxedEnvironment() - def process_template(self, sql: str, **kwargs: Any) -> str: - """Processes a sql template - >>> sql = "SELECT '{{ datetime(2017, 1, 1).isoformat() }}'" - >>> process_template(sql) - "SELECT '2017-01-01T00:00:00'" - """ - template = self._env.from_string(sql) - kwargs.update(self._context) - return template.render(kwargs) +class SafeJinjaTemplateProcessor(BaseTemplateProcessor): + def set_context(self, **kwargs: Any) -> None: + extra_cache = ExtraCache(self._extra_cache_keys) + self._context = { + "url_param": partial(safe_proxy, extra_cache.url_param), + "current_user_id": partial(safe_proxy, extra_cache.current_user_id), + "current_username": partial(safe_proxy, extra_cache.current_username), + "cache_key_wrapper": partial(safe_proxy, extra_cache.cache_key_wrapper), + "filter_values": partial(safe_proxy, filter_values), + "form_data": {}, + } + + def set_env(self) -> None: + self._env = ImmutableSandboxedEnvironment() class NoOpTemplateProcessor( @@ -257,7 +311,7 @@ def process_template(self, sql: str, **kwargs: Any) -> str: return sql -class PrestoTemplateProcessor(BaseTemplateProcessor): +class PrestoTemplateProcessor(JinjaTemplateProcessor): """Presto Jinja context The methods described here are namespaced under ``presto`` in the @@ -335,8 +389,13 @@ def get_template_processor( **kwargs: Any, ) -> BaseTemplateProcessor: if feature_flag_manager.is_feature_enabled("ENABLE_TEMPLATE_PROCESSING"): + default_processor = ( + SafeJinjaTemplateProcessor + if current_app.config["SAFE_JINJA_PROCESSING"] + else JinjaTemplateProcessor + ) template_processor = template_processors.get( - database.backend, BaseTemplateProcessor + database.backend, default_processor ) else: template_processor = NoOpTemplateProcessor From e6ff845f470333ee7ca6441b8d825e6c2aa9b13c Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Mon, 16 Nov 2020 13:12:02 -0800 Subject: [PATCH 02/13] Allow JINJA_CONTEXT_ADDONS with SAFE_JINJA_PROCESSING --- superset/config.py | 6 ++- superset/jinja_context.py | 78 ++++++++++++++++++++++----------------- 2 files changed, 49 insertions(+), 35 deletions(-) diff --git a/superset/config.py b/superset/config.py index 16b9d35bbd855..f514c0c2c1ae6 100644 --- a/superset/config.py +++ b/superset/config.py @@ -667,7 +667,11 @@ 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 diff --git a/superset/jinja_context.py b/superset/jinja_context.py index 010415bddad4f..49af685f4b054 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -21,6 +21,7 @@ from typing import Any, Callable, cast, Dict, List, Optional, Tuple, TYPE_CHECKING from flask import current_app, g, request +from flask_babel import gettext as _ from jinja2.sandbox import ImmutableSandboxedEnvironment, SandboxedEnvironment from superset import jinja_base_context @@ -188,24 +189,32 @@ def url_param( return result +NONE_TYPE = type(None).__name__ +ALLOWED_TYPES = [ + NONE_TYPE, + "bool", + "str", + "unicode", + "int", + "long", + "float", + "list", + "dict", + "tuple", +] + + def safe_proxy(func: Callable[..., Any], *args: Any, **kwargs: Any) -> Any: - none_type = type(None).__name__ - allowed_types = [ - none_type, - "bool", - "str", - "unicode", - "int", - "long", - "float", - "list", - "dict", - "tuple", - ] return_value = func(*args, **kwargs) value_type = type(return_value).__name__ - if value_type not in allowed_types: - raise SupersetTemplateException("Unsafe template value") + if value_type not in ALLOWED_TYPES: + raise SupersetTemplateException( + _( + "Unsafe return type for function %(func)s: %(value_type)s", + func=func.__name__, + value_type=value_type, + ) + ) return return_value @@ -268,18 +277,17 @@ def process_template(self, sql: str, **kwargs: Any) -> str: class JinjaTemplateProcessor(BaseTemplateProcessor): def set_context(self, **kwargs: Any) -> None: + super().set_context(**kwargs) extra_cache = ExtraCache(self._extra_cache_keys) - self._context = { - "url_param": extra_cache.url_param, - "current_user_id": extra_cache.current_user_id, - "current_username": extra_cache.current_username, - "cache_key_wrapper": extra_cache.cache_key_wrapper, - "filter_values": filter_values, - "form_data": {}, - } - - self._context.update(kwargs) - self._context.update(jinja_base_context) + self._context.update( + { + "url_param": extra_cache.url_param, + "current_user_id": extra_cache.current_user_id, + "current_username": extra_cache.current_username, + "cache_key_wrapper": extra_cache.cache_key_wrapper, + "filter_values": filter_values, + } + ) if self.engine: self._context[self.engine] = self @@ -287,15 +295,17 @@ def set_context(self, **kwargs: Any) -> None: class SafeJinjaTemplateProcessor(BaseTemplateProcessor): def set_context(self, **kwargs: Any) -> None: + super().set_context(**kwargs) extra_cache = ExtraCache(self._extra_cache_keys) - self._context = { - "url_param": partial(safe_proxy, extra_cache.url_param), - "current_user_id": partial(safe_proxy, extra_cache.current_user_id), - "current_username": partial(safe_proxy, extra_cache.current_username), - "cache_key_wrapper": partial(safe_proxy, extra_cache.cache_key_wrapper), - "filter_values": partial(safe_proxy, filter_values), - "form_data": {}, - } + self._context.update( + { + "url_param": partial(safe_proxy, extra_cache.url_param), + "current_user_id": partial(safe_proxy, extra_cache.current_user_id), + "current_username": partial(safe_proxy, extra_cache.current_username), + "cache_key_wrapper": partial(safe_proxy, extra_cache.cache_key_wrapper), + "filter_values": partial(safe_proxy, filter_values), + } + ) def set_env(self) -> None: self._env = ImmutableSandboxedEnvironment() From cb2fd94be728bc6ec4d83a85bf0d4ea647965c6f Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Mon, 16 Nov 2020 14:11:53 -0800 Subject: [PATCH 03/13] Make template processor initialization less magical, refactor classes --- superset/config.py | 7 +-- superset/jinja_context.py | 90 +++++++++++++++++---------------------- 2 files changed, 42 insertions(+), 55 deletions(-) diff --git a/superset/config.py b/superset/config.py index f514c0c2c1ae6..2b3c5e73e87be 100644 --- a/superset/config.py +++ b/superset/config.py @@ -674,11 +674,12 @@ class CeleryConfig: # pylint: disable=too-few-public-methods # 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]] = {} # Prevent access to classes/objects and proxy methods in the default Jinja context, diff --git a/superset/jinja_context.py b/superset/jinja_context.py index 49af685f4b054..68232707ce78e 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -34,6 +34,20 @@ from superset.models.core import Database from superset.models.sql_lab import Query +NONE_TYPE = type(None).__name__ +ALLOWED_RETURN_TYPES = [ + NONE_TYPE, + "bool", + "str", + "unicode", + "int", + "long", + "float", + "list", + "dict", + "tuple", +] + def filter_values(column: str, default: Optional[str] = None) -> List[str]: """ Gets a values for a particular filter as a list @@ -189,25 +203,13 @@ def url_param( return result -NONE_TYPE = type(None).__name__ -ALLOWED_TYPES = [ - NONE_TYPE, - "bool", - "str", - "unicode", - "int", - "long", - "float", - "list", - "dict", - "tuple", -] - - def safe_proxy(func: Callable[..., Any], *args: Any, **kwargs: Any) -> Any: return_value = func(*args, **kwargs) + if not current_app.config["SAFE_JINJA_PROCESSING"]: + return return_value + value_type = type(return_value).__name__ - if value_type not in ALLOWED_TYPES: + if value_type not in ALLOWED_RETURN_TYPES: raise SupersetTemplateException( _( "Unsafe return type for function %(func)s: %(value_type)s", @@ -253,12 +255,13 @@ def __init__( self._schema = table.schema self._extra_cache_keys = extra_cache_keys self._context: Dict[str, Any] = {} - self.set_env() + self._env = ( + ImmutableSandboxedEnvironment() + if current_app.config["SAFE_JINJA_PROCESSING"] + else SandboxedEnvironment() + ) self.set_context(**kwargs) - def set_env(self) -> None: - self._env = SandboxedEnvironment() - def set_context(self, **kwargs: Any) -> None: self._context.update(kwargs) self._context.update(jinja_base_context) @@ -276,24 +279,6 @@ def process_template(self, sql: str, **kwargs: Any) -> str: class JinjaTemplateProcessor(BaseTemplateProcessor): - def set_context(self, **kwargs: Any) -> None: - super().set_context(**kwargs) - extra_cache = ExtraCache(self._extra_cache_keys) - self._context.update( - { - "url_param": extra_cache.url_param, - "current_user_id": extra_cache.current_user_id, - "current_username": extra_cache.current_username, - "cache_key_wrapper": extra_cache.cache_key_wrapper, - "filter_values": filter_values, - } - ) - - if self.engine: - self._context[self.engine] = self - - -class SafeJinjaTemplateProcessor(BaseTemplateProcessor): def set_context(self, **kwargs: Any) -> None: super().set_context(**kwargs) extra_cache = ExtraCache(self._extra_cache_keys) @@ -307,9 +292,6 @@ def set_context(self, **kwargs: Any) -> None: } ) - def set_env(self) -> None: - self._env = ImmutableSandboxedEnvironment() - class NoOpTemplateProcessor( BaseTemplateProcessor @@ -330,6 +312,15 @@ class PrestoTemplateProcessor(JinjaTemplateProcessor): engine = "presto" + def set_context(self, **kwargs: Any) -> None: + super().set_context(**kwargs) + self._context[self.engine] = { + "first_latest_partition": self.first_latest_partition, + "latest_partitions": self.latest_partitions, + "latest_sub_partition": self.latest_sub_partition, + "latest_partition": self.latest_partition, + } + @staticmethod def _schema_table( table_name: str, schema: Optional[str] @@ -385,11 +376,11 @@ class HiveTemplateProcessor(PrestoTemplateProcessor): # The global template processors from Jinja context manager. template_processors = jinja_context_manager.template_processors -keys = tuple(globals().keys()) -for k in keys: - o = globals()[k] - if o and inspect.isclass(o) and issubclass(o, BaseTemplateProcessor): - template_processors[o.engine] = o +default_processors = {"presto": PrestoTemplateProcessor, "hive": HiveTemplateProcessor} +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] def get_template_processor( @@ -399,13 +390,8 @@ def get_template_processor( **kwargs: Any, ) -> BaseTemplateProcessor: if feature_flag_manager.is_feature_enabled("ENABLE_TEMPLATE_PROCESSING"): - default_processor = ( - SafeJinjaTemplateProcessor - if current_app.config["SAFE_JINJA_PROCESSING"] - else JinjaTemplateProcessor - ) template_processor = template_processors.get( - database.backend, default_processor + database.backend, JinjaTemplateProcessor ) else: template_processor = NoOpTemplateProcessor From cce6d2b11b1b3709b1131261e7f4649d5e38a2f3 Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Mon, 16 Nov 2020 17:30:27 -0800 Subject: [PATCH 04/13] Consolidat Jinja logic, remove config flag in favor of sane defaults --- superset/__init__.py | 2 - superset/app.py | 5 -- superset/config.py | 4 -- superset/connectors/sqla/models.py | 4 +- superset/extensions.py | 39 ------------- superset/jinja_context.py | 94 +++++++++++++++++------------- superset/superset_config.py | 0 7 files changed, 55 insertions(+), 93 deletions(-) create mode 100755 superset/superset_config.py diff --git a/superset/__init__.py b/superset/__init__.py index 4dbee10f2aa1e..dc8a80b7f021c 100644 --- a/superset/__init__.py +++ b/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 diff --git a/superset/app.py b/superset/app.py index 806dbfd482179..acc2f2612747b 100644 --- a/superset/app.py +++ b/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 diff --git a/superset/config.py b/superset/config.py index 2b3c5e73e87be..821fdb0594942 100644 --- a/superset/config.py +++ b/superset/config.py @@ -682,10 +682,6 @@ class CeleryConfig: # pylint: disable=too-few-public-methods # basis. Example value = `{"presto": CustomPrestoTemplateProcessor}` CUSTOM_TEMPLATE_PROCESSORS: Dict[str, Type[BaseTemplateProcessor]] = {} -# Prevent access to classes/objects and proxy methods in the default Jinja context, -# unless explicitly overridden by JINJA_CONTEXT_ADDONS or CUSTOM_TEMPLATE_PROCESSORS. -SAFE_JINJA_PROCESSING: bool = True - # Roles that are controlled by the API / Superset and should not be changes # by humans. ROBOT_PERMISSION_ROLES = ["Public", "Gamma", "Alpha", "Admin", "sql_lab"] diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 34961f0b4b6e9..e3ba4a330aeba 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -875,12 +875,12 @@ 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}, } diff --git a/superset/extensions.py b/superset/extensions.py index 5b980641cd395..c018565130231 100644 --- a/superset/extensions.py +++ b/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 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,39 +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: Dict[str, Any] = {} - self._template_processors: Dict[str, Type["BaseTemplateProcessor"]] = {} - - def init_app(self, app: Flask) -> None: - if not app.config["SAFE_JINJA_PROCESSING"]: - 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._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: @@ -142,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() diff --git a/superset/jinja_context.py b/superset/jinja_context.py index 68232707ce78e..a9766b2ed41ef 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -15,19 +15,21 @@ # specific language governing permissions and limitations # under the License. """Defines the templating context for SQL Lab""" -import inspect import re from functools import partial from typing import Any, Callable, cast, Dict, List, Optional, Tuple, TYPE_CHECKING from flask import current_app, g, request from flask_babel import gettext as _ -from jinja2.sandbox import ImmutableSandboxedEnvironment, SandboxedEnvironment +from jinja2.sandbox import SandboxedEnvironment -from superset import jinja_base_context from superset.exceptions import SupersetTemplateException -from superset.extensions import feature_flag_manager, jinja_context_manager -from superset.utils.core import convert_legacy_filters_into_adhoc, merge_extra_filters +from superset.extensions import feature_flag_manager +from superset.utils.core import ( + convert_legacy_filters_into_adhoc, + memoized, + merge_extra_filters, +) if TYPE_CHECKING: from superset.connectors.sqla.models import SqlaTable @@ -35,7 +37,7 @@ from superset.models.sql_lab import Query NONE_TYPE = type(None).__name__ -ALLOWED_RETURN_TYPES = [ +ALLOWED_TYPES = ( NONE_TYPE, "bool", "str", @@ -46,7 +48,12 @@ "list", "dict", "tuple", -] +) + + +@memoized +def context_addons() -> Dict[str, Any]: + return current_app.config.get("JINJA_CONTEXT_ADDONS", {}) def filter_values(column: str, default: Optional[str] = None) -> List[str]: @@ -205,11 +212,8 @@ def url_param( def safe_proxy(func: Callable[..., Any], *args: Any, **kwargs: Any) -> Any: return_value = func(*args, **kwargs) - if not current_app.config["SAFE_JINJA_PROCESSING"]: - return return_value - value_type = type(return_value).__name__ - if value_type not in ALLOWED_RETURN_TYPES: + if value_type not in ALLOWED_TYPES: raise SupersetTemplateException( _( "Unsafe return type for function %(func)s: %(value_type)s", @@ -222,18 +226,8 @@ def safe_proxy(func: Callable[..., Any], *args: Any, **kwargs: Any) -> Any: class BaseTemplateProcessor: # pylint: disable=too-few-public-methods - """Base class for database-specific jinja context - - There's this bit of magic in ``process_template`` that instantiates only - the database context for the active database as a ``models.Database`` - object binds it to the context object, so that object methods - have access to - that context. This way, {{ hive.latest_partition('mytable') }} just - knows about the database it is operating in. - - This means that object methods are only available for the active database - and are given access to the ``models.Database`` object and schema - name. For globally available methods use ``@classmethod``. + """ + Base class for database-specific jinja context """ engine: Optional[str] = None @@ -255,16 +249,27 @@ def __init__( self._schema = table.schema self._extra_cache_keys = extra_cache_keys self._context: Dict[str, Any] = {} - self._env = ( - ImmutableSandboxedEnvironment() - if current_app.config["SAFE_JINJA_PROCESSING"] - else SandboxedEnvironment() - ) + self._env = SandboxedEnvironment() self.set_context(**kwargs) def set_context(self, **kwargs: Any) -> None: self._context.update(kwargs) - self._context.update(jinja_base_context) + 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 @@ -275,6 +280,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) @@ -315,10 +322,10 @@ class PrestoTemplateProcessor(JinjaTemplateProcessor): def set_context(self, **kwargs: Any) -> None: super().set_context(**kwargs) self._context[self.engine] = { - "first_latest_partition": self.first_latest_partition, - "latest_partitions": self.latest_partitions, - "latest_sub_partition": self.latest_sub_partition, - "latest_partition": self.latest_partition, + "first_latest_partition": partial(safe_proxy, self.first_latest_partition), + "latest_partitions": partial(safe_proxy, self.latest_partitions), + "latest_sub_partition": partial(safe_proxy, self.latest_sub_partition), + "latest_partition": partial(safe_proxy, self.latest_partition), } @staticmethod @@ -374,13 +381,18 @@ class HiveTemplateProcessor(PrestoTemplateProcessor): engine = "hive" -# The global template processors from Jinja context manager. -template_processors = jinja_context_manager.template_processors -default_processors = {"presto": PrestoTemplateProcessor, "hive": HiveTemplateProcessor} -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] +DEFAULT_PROCESSORS = {"presto": PrestoTemplateProcessor, "hive": HiveTemplateProcessor} + + +@memoized +def template_processors() -> Dict[str, Any]: + template_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] + + return template_processors def get_template_processor( @@ -390,7 +402,7 @@ def get_template_processor( **kwargs: Any, ) -> BaseTemplateProcessor: if feature_flag_manager.is_feature_enabled("ENABLE_TEMPLATE_PROCESSING"): - template_processor = template_processors.get( + template_processor = template_processors().get( database.backend, JinjaTemplateProcessor ) else: diff --git a/superset/superset_config.py b/superset/superset_config.py new file mode 100755 index 0000000000000..e69de29bb2d1d From c11ac0a7cc83f89e81e010277a9e03042b90af22 Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Mon, 16 Nov 2020 22:12:23 -0800 Subject: [PATCH 05/13] Restore previous ENABLE_TEMPLATE_PROCESSING default --- superset/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/config.py b/superset/config.py index 821fdb0594942..a1699ead93ece 100644 --- a/superset/config.py +++ b/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 From e37b20eae53ca4f7129e62d5d59d7e6aaabc506b Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Mon, 16 Nov 2020 22:17:03 -0800 Subject: [PATCH 06/13] Add recursive type checking, update tests --- superset/jinja_context.py | 26 ++++- tests/core_tests.py | 93 +---------------- tests/jinja_context_tests.py | 187 ++++++++++++++++++++++++++++++++++- 3 files changed, 210 insertions(+), 96 deletions(-) diff --git a/superset/jinja_context.py b/superset/jinja_context.py index a9766b2ed41ef..5d37e5659b7f6 100644 --- a/superset/jinja_context.py +++ b/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,6 +224,13 @@ 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 @@ -256,13 +266,12 @@ 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: + def validate_template_context(self, 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", @@ -270,6 +279,15 @@ def validate_template_context(self, context: Dict[str, Any]) -> None: 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 def process_template(self, sql: str, **kwargs: Any) -> str: """Processes a sql template @@ -281,8 +299,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 = self.validate_template_context(kwargs) + return template.render(context) class JinjaTemplateProcessor(BaseTemplateProcessor): diff --git a/tests/core_tests.py b/tests/core_tests.py index 64abd94fdf810..87b698f4d8097 100644 --- a/tests/core_tests.py +++ b/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") diff --git a/tests/jinja_context_tests.py b/tests/jinja_context_tests.py index 349e13593aa8a..e191cd0095c12 100644 --- a/tests/jinja_context_tests.py +++ b/tests/jinja_context_tests.py @@ -15,10 +15,22 @@ # specific language governing permissions and limitations # under the License. import json +from datetime import datetime +from typing import Any +from unittest import mock + +import pytest import tests.test_app from superset import app -from superset.jinja_context import ExtraCache, filter_values +from superset.exceptions import SupersetTemplateException +from superset.jinja_context import ( + ExtraCache, + filter_values, + get_template_processor, + safe_proxy, +) +from superset.utils import core as utils from tests.base_tests import SupersetTestCase @@ -97,3 +109,176 @@ def test_url_param_form_data(self) -> None: query_string={"form_data": json.dumps({"url_params": {"foo": "bar"}})} ): self.assertEqual(ExtraCache().url_param("foo"), "bar") + + def test_safe_proxy_primitive(self) -> None: + def func(input: Any) -> Any: + return input + + return_value = safe_proxy(func, "foo") + self.assertEqual("foo", return_value) + + def test_safe_proxy_dict(self) -> None: + def func(input: Any) -> Any: + return input + + return_value = safe_proxy(func, {"foo": "bar"}) + self.assertEqual({"foo": "bar"}, return_value) + + def test_safe_proxy_lambda(self) -> None: + def func(input: Any) -> Any: + return input + + with pytest.raises(SupersetTemplateException): + safe_proxy(func, lambda: "bar") + + def test_safe_proxy_nested_lambda(self) -> None: + def func(input: Any) -> Any: + return input + + with pytest.raises(SupersetTemplateException): + safe_proxy(func, {"foo": lambda: "bar"}) + + def test_process_template(self) -> None: + maindb = utils.get_example_database() + sql = "SELECT '{{ 1+1 }}'" + tp = get_template_processor(database=maindb) + rendered = tp.process_template(sql) + self.assertEqual("SELECT '2'", rendered) + + def test_get_template_kwarg(self) -> None: + maindb = utils.get_example_database() + s = "{{ foo }}" + tp = get_template_processor(database=maindb, foo="bar") + rendered = tp.process_template(s) + self.assertEqual("bar", rendered) + + def test_template_kwarg(self) -> None: + maindb = utils.get_example_database() + s = "{{ foo }}" + tp = get_template_processor(database=maindb) + rendered = tp.process_template(s, foo="bar") + self.assertEqual("bar", rendered) + + def test_get_template_kwarg_dict(self) -> None: + maindb = utils.get_example_database() + s = "{{ foo.bar }}" + tp = get_template_processor(database=maindb, foo={"bar": "baz"}) + rendered = tp.process_template(s) + self.assertEqual("baz", rendered) + + def test_template_kwarg_dict(self) -> None: + maindb = utils.get_example_database() + s = "{{ foo.bar }}" + tp = get_template_processor(database=maindb) + rendered = tp.process_template(s, foo={"bar": "baz"}) + self.assertEqual("baz", rendered) + + def test_get_template_kwarg_lambda(self) -> None: + maindb = utils.get_example_database() + s = "{{ foo() }}" + tp = get_template_processor(database=maindb, foo=lambda: "bar") + with pytest.raises(SupersetTemplateException): + tp.process_template(s) + + def test_template_kwarg_lambda(self) -> None: + maindb = utils.get_example_database() + s = "{{ foo() }}" + tp = get_template_processor(database=maindb) + with pytest.raises(SupersetTemplateException): + tp.process_template(s, foo=lambda: "bar") + + def test_get_template_kwarg_module(self) -> None: + maindb = utils.get_example_database() + s = "{{ dt(2017, 1, 1).isoformat() }}" + tp = get_template_processor(database=maindb, dt=datetime) + with pytest.raises(SupersetTemplateException): + tp.process_template(s) + + def test_template_kwarg_module(self) -> None: + maindb = utils.get_example_database() + s = "{{ dt(2017, 1, 1).isoformat() }}" + tp = get_template_processor(database=maindb) + with pytest.raises(SupersetTemplateException): + tp.process_template(s, dt=datetime) + + def test_get_template_kwarg_nested_module(self) -> None: + maindb = utils.get_example_database() + s = "{{ foo.dt }}" + tp = get_template_processor(database=maindb, foo={"dt": datetime}) + with pytest.raises(SupersetTemplateException): + tp.process_template(s) + + def test_template_kwarg_nested_module(self) -> None: + maindb = utils.get_example_database() + s = "{{ foo.dt }}" + tp = get_template_processor(database=maindb) + with pytest.raises(SupersetTemplateException): + tp.process_template(s, foo={"bar": datetime}) + + @mock.patch("superset.jinja_context.context_addons") + def test_template_context_addons(self, addons_mock) -> None: + addons_mock.return_value = {"datetime": datetime} + maindb = utils.get_example_database() + s = "SELECT '{{ datetime(2017, 1, 1).isoformat() }}'" + tp = get_template_processor(database=maindb) + rendered = tp.process_template(s) + self.assertEqual("SELECT '2017-01-01T00:00:00'", rendered) + + @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(1970, 1, 1)) + db = mock.Mock() + db.backend = "db_for_macros_testing" + tp = 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) -> None: + """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 = 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 = 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 = 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 = get_template_processor(database=maindb) + rendered = tp.process_template(sql) + assert sql == rendered From aa2bb5d44bd2b772ae781d4cd83b8fe8397c4998 Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Mon, 16 Nov 2020 22:17:55 -0800 Subject: [PATCH 07/13] remove erroneous config file --- superset/superset_config.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100755 superset/superset_config.py diff --git a/superset/superset_config.py b/superset/superset_config.py deleted file mode 100755 index e69de29bb2d1d..0000000000000 From 44a084e6e3f54a785826fc7626f305a083653c40 Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Mon, 16 Nov 2020 22:52:25 -0800 Subject: [PATCH 08/13] Remove TableColumn models from template context --- superset/connectors/sqla/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index e3ba4a330aeba..f2ed5f1f4c0be 100644 --- a/superset/connectors/sqla/models.py +++ b/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) From 69ddd6d3c095be8b4487c48647ef4f61b36f9047 Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Mon, 16 Nov 2020 23:08:32 -0800 Subject: [PATCH 09/13] pylint refactoring --- superset/extensions.py | 2 +- superset/jinja_context.py | 57 ++++++++++++++++++++------------------- 2 files changed, 30 insertions(+), 29 deletions(-) diff --git a/superset/extensions.py b/superset/extensions.py index c018565130231..7011bb237d636 100644 --- a/superset/extensions.py +++ b/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 diff --git a/superset/jinja_context.py b/superset/jinja_context.py index 5d37e5659b7f6..a158e2ff302f4 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -235,6 +235,30 @@ def safe_proxy(func: Callable[..., Any], *args: Any, **kwargs: Any) -> Any: 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 @@ -266,29 +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]) -> 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 - def process_template(self, sql: str, **kwargs: Any) -> str: """Processes a sql template @@ -299,7 +300,7 @@ def process_template(self, sql: str, **kwargs: Any) -> str: template = self._env.from_string(sql) kwargs.update(self._context) - context = self.validate_template_context(kwargs) + context = validate_template_context(kwargs) return template.render(context) @@ -404,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( From e1ff6578568511bd59ff17fbcdfc43cb3051a072 Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Mon, 16 Nov 2020 23:15:00 -0800 Subject: [PATCH 10/13] Add entry to UPDATING.md --- UPDATING.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/UPDATING.md b/UPDATING.md index e56deff7a14f5..dfcd7521ff149 100644 --- a/UPDATING.md +++ b/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. From bc2fc0123fe7754b6358448107b7a1f4b5301816 Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Tue, 17 Nov 2020 09:41:17 -0800 Subject: [PATCH 11/13] Resolve botched merge conflict --- UPDATING.md | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 3a65cf6bb66f2..b4c686338804a 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -40,11 +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 -<<<<<<< HEAD -* [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` -======= -- [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` ->>>>>>> master +- [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. From a9461e47a0c28ac5cb5e8198ee4892979e208121 Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Tue, 17 Nov 2020 11:00:36 -0800 Subject: [PATCH 12/13] Update docs on running single python test --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cffbedc74d039..2e7d79e405303 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -607,7 +607,7 @@ tox -e -- tests/test_file.py or for a specific test via, ```bash -tox -e -- tests/test_file.py:TestClassName.test_method_name +tox -e -- tests/test_file.py::TestClassName::test_method_name ``` Note that the test environment uses a temporary directory for defining the From f7fbd592fdda45936a9ca7fb5ec7c0a824347ce8 Mon Sep 17 00:00:00 2001 From: Rob DiCiuccio Date: Tue, 17 Nov 2020 11:09:59 -0800 Subject: [PATCH 13/13] Refactor template context checking to support engine-specific methods --- superset/jinja_context.py | 23 ++++++++++++++++++----- tests/jinja_context_tests.py | 10 ++++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/superset/jinja_context.py b/superset/jinja_context.py index a158e2ff302f4..400b9d60df7e0 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -235,7 +235,7 @@ def safe_proxy(func: Callable[..., Any], *args: Any, **kwargs: Any) -> Any: return return_value -def validate_template_context(context: Dict[str, Any]) -> Dict[str, Any]: +def validate_context_types(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(): @@ -253,12 +253,25 @@ def validate_template_context(context: Dict[str, Any]) -> Dict[str, Any]: context[key] = json.loads(json.dumps(context[key])) except TypeError: raise SupersetTemplateException( - _("Unsupported template value for key %(key)s", key=key,) + _("Unsupported template value for key %(key)s", key=key) ) return context +def validate_template_context( + engine: Optional[str], context: Dict[str, Any] +) -> Dict[str, Any]: + if engine and engine in context: + # validate engine context separately to allow for engine-specific methods + engine_context = validate_context_types(context.pop(engine)) + valid_context = validate_context_types(context) + valid_context[engine] = engine_context + return valid_context + + return validate_context_types(context) + + class BaseTemplateProcessor: # pylint: disable=too-few-public-methods """ Base class for database-specific jinja context @@ -300,7 +313,7 @@ def process_template(self, sql: str, **kwargs: Any) -> str: template = self._env.from_string(sql) kwargs.update(self._context) - context = validate_template_context(kwargs) + context = validate_template_context(self.engine, kwargs) return template.render(context) @@ -404,7 +417,7 @@ class HiveTemplateProcessor(PrestoTemplateProcessor): @memoized -def template_processors() -> Dict[str, Any]: +def get_template_processors() -> Dict[str, Any]: processors = current_app.config.get("CUSTOM_TEMPLATE_PROCESSORS", {}) for engine in DEFAULT_PROCESSORS: # do not overwrite engine-specific CUSTOM_TEMPLATE_PROCESSORS @@ -421,7 +434,7 @@ def get_template_processor( **kwargs: Any, ) -> BaseTemplateProcessor: if feature_flag_manager.is_feature_enabled("ENABLE_TEMPLATE_PROCESSING"): - template_processor = template_processors().get( + template_processor = get_template_processors().get( database.backend, JinjaTemplateProcessor ) else: diff --git a/tests/jinja_context_tests.py b/tests/jinja_context_tests.py index e191cd0095c12..a3314d05ea25c 100644 --- a/tests/jinja_context_tests.py +++ b/tests/jinja_context_tests.py @@ -215,6 +215,16 @@ def test_template_kwarg_nested_module(self) -> None: with pytest.raises(SupersetTemplateException): tp.process_template(s, foo={"bar": datetime}) + @mock.patch("superset.jinja_context.HiveTemplateProcessor.latest_partition") + def test_template_hive(self, lp_mock) -> None: + lp_mock.return_value = "the_latest" + db = mock.Mock() + db.backend = "hive" + s = "{{ hive.latest_partition('my_table') }}" + tp = get_template_processor(database=db) + rendered = tp.process_template(s) + self.assertEqual("the_latest", rendered) + @mock.patch("superset.jinja_context.context_addons") def test_template_context_addons(self, addons_mock) -> None: addons_mock.return_value = {"datetime": datetime}