From 38df1a873f3bbfcdc5a7af6aaf7b17f8c7a9e08c Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Thu, 29 Jun 2023 17:38:17 +0200 Subject: [PATCH] chore: Un-revert enabling CSP by default (#24543) --- UPDATING.md | 1 + docs/docs/security.mdx | 48 +++++++++---------- .../src/models/ExtensibleFunction.ts | 3 +- superset/config.py | 38 +++++++++++++-- superset/initialization/__init__.py | 6 ++- .../appbuilder/general/widgets/base_list.html | 3 +- .../appbuilder/general/widgets/search.html | 3 +- .../templates/superset/export_dashboards.html | 3 +- .../superset/form_view/csv_scripts.html | 4 +- .../form_view/csv_to_database_view/edit.html | 3 +- .../form_view/database_schemas_selector.html | 3 +- superset/templates/superset/macros.html | 23 +++++++++ .../superset/models/database/macros.html | 9 ++-- .../superset/partials/asset_bundle.html | 3 +- superset/templates/superset/theme.html | 15 ++++-- 15 files changed, 120 insertions(+), 45 deletions(-) create mode 100644 superset/templates/superset/macros.html diff --git a/UPDATING.md b/UPDATING.md index b95cedc1e99fc..f55102d15e071 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -34,6 +34,7 @@ assists people when migrating to a new version. ### Breaking Changes +- [24262](https://github.com/apache/superset/pull/24262): Enabled `TALISMAN_ENABLED` flag by default and provided stricter default Content Security Policy - [24415](https://github.com/apache/superset/pull/24415): Removed the obsolete Druid NoSQL REGEX operator. - [24423](https://github.com/apache/superset/pull/24423): Removed deprecated APIs `/superset/slice_json/...`, `/superset/annotation_json/...` - [24400](https://github.com/apache/superset/pull/24400): Removed deprecated APIs `/superset/recent_activity/...`, `/superset/fave_dashboards_by_username/...`, `/superset/fave_dashboards/...`, `/superset/created_dashboards/...`, `/superset/user_slices/`, `/superset/created_slices/...`, `/superset/fave_slices/...`, `/superset/favstar/...`, diff --git a/docs/docs/security.mdx b/docs/docs/security.mdx index a81149afb5004..ab6d41e895f40 100644 --- a/docs/docs/security.mdx +++ b/docs/docs/security.mdx @@ -176,9 +176,9 @@ a certain resource type or policy area. You can check possible directives It's extremely important to correctly configure a Content Security Policy when deploying Superset to prevent many types of attacks. Superset provides two variables in `config.py` for deploying a CSP: -- `TALISMAN_ENABLED` defaults to `False`; set this to `True` in order to implement a CSP -- `TALISMAN_CONFIG` holds the actual the policy definition (_see example below_) as well as any - other arguments to be passed to Talisman. +- `TALISMAN_ENABLED` defaults to `True`; set this to `False` in order to disable CSP +- `TALISMAN_CONFIG` holds the actual the policy definition (*see example below*) as well as any +other arguments to be passed to Talisman. When running in production mode, Superset will check at startup for the presence of a CSP. If one is not found, it will issue a warning with the security risks. For environments @@ -187,10 +187,20 @@ this warning using the `CONTENT_SECURITY_POLICY_WARNING` key in `config.py`. #### CSP Requirements -- Superset needs both the `'unsafe-eval'` and `'unsafe-inline'` CSP keywords in order to operate. +* Superset needs the `style-src unsafe-inline` CSP directive in order to operate. ``` - default-src 'self' 'unsafe-eval' 'unsafe-inline' + style-src 'self' 'unsafe-inline' + ``` + +* Only scripts marked with a [nonce](https://content-security-policy.com/nonce/) can be loaded and executed. +Nonce is a random string automatically generated by Talisman on each page load. +You can get current nonce value by calling jinja macro `csp_nonce()`. + + ``` + ``` - Some dashboards load images using data URIs and require `data:` in their `img-src` @@ -206,21 +216,11 @@ this warning using the `CONTENT_SECURITY_POLICY_WARNING` key in `config.py`. connect-src 'self' https://api.mapbox.com https://events.mapbox.com ``` -This is a basic example `TALISMAN_CONFIG` that implements the above requirements, uses `'self'` to -limit content to the same origin as the Superset server, and disallows outdated HTML elements by -setting `object-src` to `'none'`. +* Other CSP directives default to `'self'` to limit content to the same origin as the Superset server. + +In order to adjust provided CSP configuration to your needs, follow the instructions and examples provided in +[Content Security Policy Reference](https://content-security-policy.com/) -```python -TALISMAN_CONFIG = { - "content_security_policy": { - "default-src": ["'self'", "'unsafe-inline'", "'unsafe-eval'"], - "img-src": ["'self'", "data:"], - "worker-src": ["'self'", "blob:"], - "connect-src": ["'self'", "https://api.mapbox.com", "https://events.mapbox.com"], - "object-src": "'none'", - } -} -``` #### Other Talisman security considerations @@ -229,15 +229,15 @@ of which `content_security_policy` is only one. Those can be found in the [Talisman documentation](https://pypi.org/project/flask-talisman/) under _Options_. These generally improve security, but administrators should be aware of their existence. -In particular, the default option of `force_https = True` may break Superset's Alerts & Reports +In particular, the option of `force_https = True` (`False` by default) may break Superset's Alerts & Reports if workers are configured to access charts via a `WEBDRIVER_BASEURL` beginning -with `http://`. As long as a Superset deployment enforces https upstream, e.g., -through a loader balancer or application gateway, it should be acceptable to set this -option to `False`, like this: +with `http://`. As long as a Superset deployment enforces https upstream, e.g., +through a loader balancer or application gateway, it should be acceptable to keep this +option disabled. Otherwise, you may want to enable `force_https` like this: ```python TALISMAN_CONFIG = { - "force_https": False, + "force_https": True, "content_security_policy": { ... ``` diff --git a/superset-frontend/packages/superset-ui-core/src/models/ExtensibleFunction.ts b/superset-frontend/packages/superset-ui-core/src/models/ExtensibleFunction.ts index 78901dd565bc0..5a247d751ed7c 100644 --- a/superset-frontend/packages/superset-ui-core/src/models/ExtensibleFunction.ts +++ b/superset-frontend/packages/superset-ui-core/src/models/ExtensibleFunction.ts @@ -22,9 +22,8 @@ */ export default class ExtensibleFunction extends Function { + // @ts-ignore constructor(fn: Function) { - super(); - // eslint-disable-next-line @typescript-eslint/no-unsafe-return, no-constructor-return return Object.setPrototypeOf(fn, new.target.prototype); } diff --git a/superset/config.py b/superset/config.py index abb73e9f56ba6..4ac95925ec694 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1365,12 +1365,42 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument CONTENT_SECURITY_POLICY_WARNING = True # Do you want Talisman enabled? -TALISMAN_ENABLED = False +TALISMAN_ENABLED = True # If you want Talisman, how do you want it configured?? TALISMAN_CONFIG = { - "content_security_policy": None, - "force_https": True, - "force_https_permanent": False, + "content_security_policy": { + "default-src": ["'self'"], + "img-src": ["'self'", "data:"], + "worker-src": ["'self'", "blob:"], + "connect-src": [ + "'self'", + "https://api.mapbox.com", + "https://events.mapbox.com", + ], + "object-src": "'none'", + "style-src": ["'self'", "'unsafe-inline'"], + "script-src": ["'self'", "'strict-dynamic'"], + }, + "content_security_policy_nonce_in": ["script-src"], + "force_https": False, +} +# React requires `eval` to work correctly in dev mode +TALISMAN_DEV_CONFIG = { + "content_security_policy": { + "default-src": ["'self'"], + "img-src": ["'self'", "data:"], + "worker-src": ["'self'", "blob:"], + "connect-src": [ + "'self'", + "https://api.mapbox.com", + "https://events.mapbox.com", + ], + "object-src": "'none'", + "style-src": ["'self'", "'unsafe-inline'"], + "script-src": ["'self'", "'unsafe-inline'", "'unsafe-eval'"], + }, + "content_security_policy_nonce_in": ["script-src"], + "force_https": False, } # diff --git a/superset/initialization/__init__.py b/superset/initialization/__init__.py index 63546060068fb..94a506b83ac2c 100644 --- a/superset/initialization/__init__.py +++ b/superset/initialization/__init__.py @@ -606,7 +606,11 @@ def __call__( # Talisman talisman_enabled = self.config["TALISMAN_ENABLED"] - talisman_config = self.config["TALISMAN_CONFIG"] + talisman_config = ( + self.config["TALISMAN_DEV_CONFIG"] + if self.superset_app.debug + else self.config["TALISMAN_CONFIG"] + ) csp_warning = self.config["CONTENT_SECURITY_POLICY_WARNING"] if talisman_enabled: diff --git a/superset/templates/appbuilder/general/widgets/base_list.html b/superset/templates/appbuilder/general/widgets/base_list.html index 3b376d243b124..4842511c5fbdf 100644 --- a/superset/templates/appbuilder/general/widgets/base_list.html +++ b/superset/templates/appbuilder/general/widgets/base_list.html @@ -17,6 +17,7 @@ under the License. #} {% import 'appbuilder/general/lib.html' as lib %} +{% import "superset/macros.html" as macros %} {% set can_add = "can_add" | is_item_visible(modelview_name) %} {% set can_show = "can_show" | is_item_visible(modelview_name) %} @@ -56,7 +57,7 @@ {{ lib.render_pagination(page, page_size, count, modelview_name) }} {{ lib.render_set_page_size(page, page_size, count, modelview_name) }} - diff --git a/superset/templates/appbuilder/general/widgets/search.html b/superset/templates/appbuilder/general/widgets/search.html index 22c6b629c182d..e59b96848e490 100644 --- a/superset/templates/appbuilder/general/widgets/search.html +++ b/superset/templates/appbuilder/general/widgets/search.html @@ -17,6 +17,7 @@ under the License. #} {% import 'appbuilder/general/lib.html' as lib %} +{% import "superset/macros.html" as macros %}
- + {% endblock %} {% block tail_js %} {{ super() }} diff --git a/superset/templates/superset/form_view/database_schemas_selector.html b/superset/templates/superset/form_view/database_schemas_selector.html index ac827c1330a10..a02ea46098579 100644 --- a/superset/templates/superset/form_view/database_schemas_selector.html +++ b/superset/templates/superset/form_view/database_schemas_selector.html @@ -16,7 +16,8 @@ specific language governing permissions and limitations under the License. #} - {% endmacro %} {% macro expand_encrypted_extra_textarea() %} - {% endmacro %} {% macro expand_server_cert_textarea() %} - {% endmacro %} diff --git a/superset/templates/superset/partials/asset_bundle.html b/superset/templates/superset/partials/asset_bundle.html index 066c9f64e6d0a..41b6d7cf8a00f 100644 --- a/superset/templates/superset/partials/asset_bundle.html +++ b/superset/templates/superset/partials/asset_bundle.html @@ -16,12 +16,13 @@ specific language governing permissions and limitations under the License. #} +{% import "superset/macros.html" as macros %} {% macro js_bundle(filename) %} {# HTML comment is needed for webpack-dev-server to replace assets with development version #} {% for entry in js_manifest(filename) %} - + {% endfor %} {% endmacro %} diff --git a/superset/templates/superset/theme.html b/superset/templates/superset/theme.html index 856796a4c4b21..3f6c8fb0745fb 100644 --- a/superset/templates/superset/theme.html +++ b/superset/templates/superset/theme.html @@ -17,6 +17,7 @@ under the License. #} {% extends "superset/basic.html" %} +{% import "superset/macros.html" as macros %} {% block body %} @@ -1340,7 +1341,15 @@

Source Code

{% endblock %} {% block tail_js %} {{ super() }} - - - + + + {% endblock %}