Skip to content

Commit

Permalink
chore: Un-revert enabling CSP by default (apache#24543)
Browse files Browse the repository at this point in the history
  • Loading branch information
kgabryje authored Jun 29, 2023
1 parent bb1db9e commit 38df1a8
Show file tree
Hide file tree
Showing 15 changed files with 120 additions and 45 deletions.
1 change: 1 addition & 0 deletions UPDATING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/...`,
Expand Down
48 changes: 24 additions & 24 deletions docs/docs/security.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()`.

```
<script nonce="{{ csp_nonce() }}">
/* my script */
</script>
```

- Some dashboards load images using data URIs and require `data:` in their `img-src`
Expand All @@ -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

Expand All @@ -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": { ...
```

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
38 changes: 34 additions & 4 deletions superset/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}

#
Expand Down
6 changes: 5 additions & 1 deletion superset/initialization/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
3 changes: 2 additions & 1 deletion superset/templates/appbuilder/general/widgets/base_list.html
Original file line number Diff line number Diff line change
Expand Up @@ -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) %}
Expand Down Expand Up @@ -56,7 +57,7 @@
{{ lib.render_pagination(page, page_size, count, modelview_name) }}
{{ lib.render_set_page_size(page, page_size, count, modelview_name) }}
</div>
<script language="javascript">
<script language="javascript" nonce="{{ macros.get_nonce() }}">
var modelActions = new AdminActions();
</script>

Expand Down
3 changes: 2 additions & 1 deletion superset/templates/appbuilder/general/widgets/search.html
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
under the License.
#}
{% import 'appbuilder/general/lib.html' as lib %}
{% import "superset/macros.html" as macros %}

<div class="list-search-container">
<form id="filter_form" class="form-search" method="get">
Expand Down Expand Up @@ -44,7 +45,7 @@
</form>
</div>

<script>
<script nonce="{{ macros.get_nonce() }}">
(function($) {
function checkSearchButton() {
var hasFilter = $('.filters tr').length;
Expand Down
3 changes: 2 additions & 1 deletion superset/templates/superset/export_dashboards.html
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
specific language governing permissions and limitations
under the License.
#}
<script>
{% import "superset/macros.html" as macros %}
<script nonce="{{ macros.get_nonce() }}">
window.onload = function() {

// See issue #7353, window.open fails
Expand Down
4 changes: 3 additions & 1 deletion superset/templates/superset/form_view/csv_scripts.html
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
specific language governing permissions and limitations
under the License.
#}
<script>
{% import "superset/macros.html" as macros %}

<script nonce="{{ macros.get_nonce() }}">
$('#delimiter').on('change', function () {
var delimiterOptions = $('#delimiter').val();
if (delimiterOptions?.includes("other")) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#}
{% extends "appbuilder/base.html" %}
{% import 'appbuilder/general/lib.html' as lib %}
{% import "superset/macros.html" as macros %}
{% set begin_sep_label = '<td class="col-sm-2" style="border-left: 0; border-top: 0;">' %}
{% set end_sep_label = '</td>' %}
{% set begin_sep_field = '<td style="border-right: 0; border-top: 0;">' %}
Expand Down Expand Up @@ -132,7 +133,7 @@
</div>
{% endblock %}
{% block add_tail_js %}
<script src="{{url_for('appbuilder.static',filename='js/ab_keep_tab.js')}}"></script>
<script src="{{url_for('appbuilder.static',filename='js/ab_keep_tab.js')}}" nonce="{{ macros.get_nonce() }}"></script>
{% endblock %}
{% block tail_js %}
{{ super() }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
specific language governing permissions and limitations
under the License.
#}
<script>
{% import "superset/macros.html" as macros %}
<script nonce="{{ macros.get_nonce() }}">
var db = $("#database");
var schema = $("#schema");

Expand Down
23 changes: 23 additions & 0 deletions superset/templates/superset/macros.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{#
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
#}
{% macro get_nonce() %}
{% if csp_nonce is defined %}
{{ csp_nonce() }}
{% endif %}
{% endmacro %}
9 changes: 5 additions & 4 deletions superset/templates/superset/models/database/macros.html
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@
specific language governing permissions and limitations
under the License.
#}
{% import "superset/macros.html" as macros %}
{% macro testconn() %}
<script>
<script nonce="{{ macros.get_nonce() }}">
$("#sqlalchemy_uri").parent()
.append('<button id="testconn" class="btn btn-sm btn-primary">{{ _("Test Connection") }}</button>');
$("#testconn").click(function(e) {
Expand Down Expand Up @@ -72,19 +73,19 @@
{% endmacro %}

{% macro expand_extra_textarea() %}
<script>
<script nonce="{{ macros.get_nonce() }}">
$('#extra').attr('rows', '5');
</script>
{% endmacro %}

{% macro expand_encrypted_extra_textarea() %}
<script>
<script nonce="{{ macros.get_nonce() }}">
$('#encrypted_extra').attr('rows', '5');
</script>
{% endmacro %}

{% macro expand_server_cert_textarea() %}
<script>
<script nonce="{{ macros.get_nonce() }}">
$('#server_cert').attr('rows', '5');
</script>
{% endmacro %}
3 changes: 2 additions & 1 deletion superset/templates/superset/partials/asset_bundle.html
Original file line number Diff line number Diff line change
Expand Up @@ -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 #}
<!-- Bundle js {{ filename }} START -->
{% for entry in js_manifest(filename) %}
<script src="{{ entry }}" async></script>
<script src="{{ entry }}" async nonce="{{ macros.get_nonce() }}"></script>
{% endfor %}
<!-- Bundle js {{ filename }} END -->
{% endmacro %}
Expand Down
15 changes: 12 additions & 3 deletions superset/templates/superset/theme.html
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
under the License.
#}
{% extends "superset/basic.html" %}
{% import "superset/macros.html" as macros %}

{% block body %}
<body>
Expand Down Expand Up @@ -1340,7 +1341,15 @@ <h4 class="modal-title">Source Code</h4>
{% endblock %}
{% block tail_js %}
{{ super() }}
<script src="https://code.jquery.com/jquery-1.10.2.min.js"></script>
<script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.min.js"></script>
<script src="{{ assets_prefix }}/static/assets/stylesheets/less/cosmo/cosmoTheme.js"></script>
<script
src="https://code.jquery.com/jquery-1.10.2.min.js"
integrity="sha256-C6CB9UYIS9UJeqinPHWTHVqh/E1uhG5Twh+Y5qFQmYg="
crossorigin="anonymous"
nonce="{{ macros.get_nonce() }}"></script>
<script
src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.min.js"
integrity="sha384-Tc5IQib027qvyjSMfHjOMaLkfuWVxZxUPnCJA7l2mCWNIpG9mGCD8wGNIcPD7Txa"
crossorigin="anonymous"
nonce="{{ macros.get_nonce() }}"></script>
<script src="{{ assets_prefix }}/static/assets/stylesheets/less/cosmo/cosmoTheme.js" nonce="{{ macros.get_nonce() }}"></script>
{% endblock %}

0 comments on commit 38df1a8

Please sign in to comment.