feat(templating): Safer Jinja template processing #11704

robdiciuccio · 2020-11-14T02:54:00Z

SUMMARY

Prevent passing potentially unsafe modules/methods to the Jinja template context:

Removes default exposed modules such as datetime and timedelta
Proxies exposed methods such as current_user_id and url_param through a method that enforces static return values

There are no changes to Jijna templating syntax with this update, other than the default modules (such as datetime) are no longer available by default.

Adding custom Jinja context items is still possible via JINJA_CONTEXT_ADDONS as is overriding the template processor on a per-database engine basis via CUSTOM_TEMPLATE_PROCESSORS.

Additional context/discussion in #11617

TODO

Write tests

TEST PLAN

Test Jinja templating in SQL Lab and in virtual datasets

ADDITIONAL INFORMATION

@mistercrunch @ktmud @dpgaspar @villebro

codecov-io · 2020-11-14T03:04:25Z

Codecov Report

Merging #11704 (f7fbd59) into master (c241c6f) will decrease coverage by 7.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #11704      +/-   ##
==========================================
- Coverage   63.04%   56.00%   -7.05%     
==========================================
  Files         895      408     -487     
  Lines       43315    14437   -28878     
  Branches     4015     3716     -299     
==========================================
- Hits        27308     8085   -19223     
+ Misses      15829     6352    -9477     
+ Partials      178        0     -178

Flag	Coverage Δ
cypress	`56.00% <ø> (?)`
javascript	`?`
python	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...uperset-frontend/src/dashboard/util/dnd-reorder.js	`0.00% <0.00%> (-100.00%)`	⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js	`0.00% <0.00%> (-100.00%)`	⬇️
...dashboard/components/resizable/ResizableHandle.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js	`0.00% <0.00%> (-93.48%)`	⬇️
...src/dashboard/components/gridComponents/Header.jsx	`10.52% <0.00%> (-86.85%)`	⬇️
...rc/dashboard/components/gridComponents/Divider.jsx	`13.33% <0.00%> (-86.67%)`	⬇️
...set-frontend/src/views/CRUD/welcome/EmptyState.tsx	`5.71% <0.00%> (-82.10%)`	⬇️
...et-frontend/src/components/ListView/ActionsBar.tsx	`11.11% <0.00%> (-81.20%)`	⬇️
...et-frontend/src/components/Menu/LanguagePicker.tsx	`0.00% <0.00%> (-80.00%)`	⬇️
superset-frontend/src/components/Link.tsx	`7.69% <0.00%> (-79.81%)`	⬇️
... and 797 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c241c6f...f7fbd59. Read the comment docs.

superset/jinja_context.py

mistercrunch · 2020-11-15T02:07:23Z

superset/config.py

@@ -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.


We may not want to make JINJA_CONTEXT_ADDONS mutually exclusive with SAFE_JINJA_PROCESSING. Someone may want to add a safe function to their environment without having to fully pivot into the legacy/more risky approach. It should be easy to support this, but we should highlight the caveats.

"""
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 you 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.
"""

This was unintentional. Fixing to allow JINJA_CONTEXT_ADDONS to coexist with SAFE_JINJA_PROCESSING

mistercrunch · 2020-11-15T02:22:49Z

superset/jinja_context.py

+    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),


Wasn't familiar with partial, but I'm guessing that the intent is to clean up other methods/context that could be attached to the callable (?)

The intent with partial is to wrap the callable with a method to enforce a safe return value.

villebro

A good step forward for securing Jinja. Would be curious to hear other community thoughts on this, especially if someone is extensively using datetime, random, uuid etc from the current base context. Perhaps kick off a DISCUSS on the mailing list?

villebro · 2020-11-16T06:54:46Z

superset/config.py

@@ -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


Would it make sense to introduce a TEMPLATE_PROCESSOR parameter that accepts TemplateProcessorEnum values, something like

TemplateProcessorEnum(enum.Enum): SafeJinja = 1 LegacyJinja = 2 Chevron = 3 Custom = 4

In this approach, LegacyJinja would include the old datetime, random etc base context, and SafeJinja would have a more limited set.

I'm not sure we want to support so many different modes. To me it's more important to find a "paved path" of safe and flexible templating solution that makes the most sense. Every feature flag we added here is more like a temporary solution for compatibility rather than something we want to support in the long-term.

@ktmud I agree, I think we should push safety and (potentially unsafe) customizability as a path forward.

superset/jinja_context.py

villebro · 2020-11-16T06:58:49Z

superset/jinja_context.py

+        none_type,
+        "bool",
+        "str",
+        "unicode",


nit: I'm pretty sure unicode has been replaced by str in Python 3.

villebro · 2020-11-16T07:01:57Z

superset/jinja_context.py

+    return_value = func(*args, **kwargs)
+    value_type = type(return_value).__name__
+    if value_type not in allowed_types:
+        raise SupersetTemplateException("Unsafe template value")


Would be nice to get a more verbose error here, something like

raise SupersetTemplateException(__("Unsafe return type for function %(func)s: %(value_type)s", func=func.__name__, value_type=value_type)

superset/config.py

robdiciuccio · 2020-11-17T01:40:40Z

Taking @ktmud 's feedback, making this less configurable and moving towards sane defaults which can be overridden via existing JINJA_CONTEXT_ADDONS and CUSTOM_TEMPLATE_PROCESSORS configuration.

ktmud · 2020-11-17T07:36:37Z

UPDATING.md

@@ -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`


Maybe we should prevent DEFAULT_FEATURE_FLAGS from being overridden by custom config files/modules.

dpgaspar

Looks great! but some tests are still failing, left a couple of non blocking comments and a question regarding presto context

dpgaspar · 2020-11-17T09:34:12Z

UPDATING.md

 - [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


This seems like a merge conflict, while your at it can you fix turned off be default to turned off by default

dpgaspar · 2020-11-17T09:55:14Z

superset/jinja_context.py

+        self._context[self.engine] = {
+            "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),


nit: Not totally related with this PR but it's strange that def latest_sub_partition(self, table_name: str, **kwargs: Any) -> Any: returns Any should probably be Optional[str]

superset/jinja_context.py

villebro

LGTM, non-blocking comment on naming of function.

superset/jinja_context.py

mistercrunch

LGTM

ktmud

LGTM with one small question

ktmud · 2020-11-17T18:05:38Z

superset/jinja_context.py

+COLLECTION_TYPES = ("list", "dict", "tuple", "set")
+
+
+@memoized


What's the benefit of memoization here? Is current_app proxy very expensive?

The value doesn't change over time, so just trying to save some processing cycles. Probably not much gain here, but attempting not to repeat initialization logic.

* 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

Enable safer Jinja template processing

7e45c67

superset-github-bot bot added the preset-io label Nov 14, 2020

pull-request-size bot added the size/L label Nov 14, 2020

robdiciuccio mentioned this pull request Nov 14, 2020

feat: support 'chevron' library for templating as jinja alternative #11617

Closed

mistercrunch requested review from dpgaspar and villebro November 14, 2020 17:43

mistercrunch reviewed Nov 15, 2020

View reviewed changes

superset/jinja_context.py Outdated Show resolved Hide resolved

mistercrunch reviewed Nov 15, 2020

View reviewed changes

superset/jinja_context.py Show resolved Hide resolved

mistercrunch reviewed Nov 15, 2020

View reviewed changes

superset/jinja_context.py Outdated Show resolved Hide resolved

mistercrunch reviewed Nov 15, 2020

View reviewed changes

villebro reviewed Nov 16, 2020

View reviewed changes

robdiciuccio added 2 commits November 16, 2020 13:12

Allow JINJA_CONTEXT_ADDONS with SAFE_JINJA_PROCESSING

e6ff845

Make template processor initialization less magical, refactor classes

cb2fd94

ktmud reviewed Nov 17, 2020

View reviewed changes

superset/config.py Show resolved Hide resolved

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

cce6d2b

robdiciuccio added 6 commits November 16, 2020 22:12

Restore previous ENABLE_TEMPLATE_PROCESSING default

c11ac0a

Add recursive type checking, update tests

e37b20e

remove erroneous config file

aa2bb5d

Remove TableColumn models from template context

44a084e

pylint refactoring

69ddd6d

Add entry to UPDATING.md

e1ff657

pull-request-size bot added size/XL and removed size/L labels Nov 17, 2020

Merge branch master

c0e65ce

ktmud reviewed Nov 17, 2020

View reviewed changes

dpgaspar reviewed Nov 17, 2020

View reviewed changes

villebro reviewed Nov 17, 2020

View reviewed changes

superset/jinja_context.py Outdated Show resolved Hide resolved

Resolve botched merge conflict

bc2fc01

mistercrunch approved these changes Nov 17, 2020

View reviewed changes

ktmud reviewed Nov 17, 2020

View reviewed changes

robdiciuccio added 2 commits November 17, 2020 11:00

Update docs on running single python test

a9461e4

Refactor template context checking to support engine-specific methods

f7fbd59

robdiciuccio force-pushed the rd/safe-jinja branch from 184c6b4 to f7fbd59 Compare November 17, 2020 19:18

craig-rueda approved these changes Nov 17, 2020

View reviewed changes

craig-rueda merged commit 01d15f5 into apache:master Nov 17, 2020

craig-rueda deleted the rd/safe-jinja branch November 17, 2020 19:55

mistercrunch added the rush! Requires immediate attention label Nov 18, 2020

junlincc removed the rush! Requires immediate attention label Mar 18, 2021

m-ajay mentioned this pull request Jun 24, 2023

[Snyk] Fix for 1 vulnerabilities m-ajay/superset#144

Open

m-ajay mentioned this pull request Jan 9, 2024

[Snyk] Security upgrade webpack from 4.46.0 to 5.1.1 m-ajay/superset#240

Open

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(templating): Safer Jinja template processing #11704

feat(templating): Safer Jinja template processing #11704

robdiciuccio commented Nov 14, 2020 •

edited

Loading

codecov-io commented Nov 14, 2020 •

edited

Loading

mistercrunch Nov 15, 2020 •

edited

Loading

robdiciuccio Nov 16, 2020

mistercrunch Nov 15, 2020

robdiciuccio Nov 16, 2020

villebro left a comment

villebro Nov 16, 2020

ktmud Nov 16, 2020

robdiciuccio Nov 17, 2020

villebro Nov 16, 2020

villebro Nov 16, 2020

robdiciuccio commented Nov 17, 2020

ktmud Nov 17, 2020

dpgaspar left a comment

dpgaspar Nov 17, 2020

dpgaspar Nov 17, 2020

villebro left a comment

mistercrunch left a comment

ktmud left a comment

ktmud Nov 17, 2020

robdiciuccio Nov 17, 2020

		- [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

		COLLECTION_TYPES = ("list", "dict", "tuple", "set")


		@memoized

feat(templating): Safer Jinja template processing #11704

feat(templating): Safer Jinja template processing #11704

Conversation

robdiciuccio commented Nov 14, 2020 • edited Loading

SUMMARY

TODO

TEST PLAN

ADDITIONAL INFORMATION

codecov-io commented Nov 14, 2020 • edited Loading

Codecov Report

mistercrunch Nov 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robdiciuccio commented Nov 17, 2020

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

mistercrunch left a comment

Choose a reason for hiding this comment

ktmud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robdiciuccio commented Nov 14, 2020 •

edited

Loading

codecov-io commented Nov 14, 2020 •

edited

Loading

mistercrunch Nov 15, 2020 •

edited

Loading