chore: Enable CSP by default #24262

kgabryje · 2023-06-01T11:49:13Z

SUMMARY

This PR enables TALISMAN_ENABLED by default and provides a default content security policy config.
Please keep in mind that this is a breaking change. If your Superset deployment loads additional scripts, loads images from external domains, performs HTTP requests to external domains, you need to adjust the default CSP config by adding external origins to appropriate CSP directives and/or mark the scripts with nonce as described in the updated docs.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

kgabryje · 2023-06-01T11:54:09Z

superset-frontend/packages/superset-ui-core/src/models/ExtensibleFunction.ts

  constructor(fn: Function) {
-    super();


Calling new Function() breaks the CSP rule that disallows using eval in scripts.
Extending Function is a nice hack that let's us make a class instance (in most cases it's a formatter) callable - for example instead of calling formatter.format(value) we call formatter(value). Removing super() does not break that behaviour while also letting us avoid calling Function constructor.

superset/templates/superset/theme.html

codecov · 2023-06-01T11:58:23Z

Codecov Report

Merging #24262 (857950f) into master (3e76736) will increase coverage by 0.05%.
The diff coverage is 87.80%.

❗ Current head 857950f differs from pull request most recent head 37698f2. Consider uploading reports for the commit 37698f2 to get more accurate results

@@            Coverage Diff             @@
##           master   #24262      +/-   ##
==========================================
+ Coverage   68.91%   68.97%   +0.05%     
==========================================
  Files        1899     1901       +2     
  Lines       73843    73969     +126     
  Branches     8119     8119              
==========================================
+ Hits        50892    51019     +127     
+ Misses      20840    20839       -1     
  Partials     2111     2111

Flag	Coverage Δ
hive	`53.90% <47.56%> (+<0.01%)`	⬆️
javascript	`55.65% <ø> (-0.01%)`	⬇️
mysql	`?`
postgres	`79.48% <87.80%> (+0.16%)`	⬆️
presto	`53.81% <47.56%> (+<0.01%)`	⬆️
python	`83.37% <87.80%> (+0.06%)`	⬆️
sqlite	`?`
unit	`54.59% <43.29%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
.../superset-ui-core/src/models/ExtensibleFunction.ts	`100.00% <ø> (ø)`
superset/tasks/cache.py	`59.34% <48.64%> (-1.81%)`	⬇️
superset/charts/commands/warm_up_cache.py	`97.50% <97.50%> (ø)`
superset/charts/api.py	`87.78% <100.00%> (+0.70%)`	⬆️
superset/charts/commands/exceptions.py	`94.00% <100.00%> (+0.38%)`	⬆️
superset/charts/schemas.py	`99.38% <100.00%> (+0.01%)`	⬆️
superset/config.py	`92.00% <100.00%> (+0.02%)`	⬆️
superset/datasets/api.py	`88.65% <100.00%> (+0.70%)`	⬆️
superset/datasets/commands/exceptions.py	`94.36% <100.00%> (+0.24%)`	⬆️
superset/datasets/commands/warm_up_cache.py	`100.00% <100.00%> (ø)`
... and 2 more

... and 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sfirke · 2023-06-01T14:49:29Z

Two things re: docs changes:

Typo: "Superset needs both style-src unsafe-inline CSP directive in order to operate." should be "Superset needs the ..."
Changing force_https to now default to False seems good to me. But it means that this section on the Security page "Other Talisman security considerations" needs to be updated. For instance right now there's text talking about how to override the default value of force_https = True.

kgabryje · 2023-06-01T16:58:23Z

Well spotted @sfirke, thank you!

dpgaspar

LGTM

eschutho · 2023-06-16T00:10:01Z

superset/config.py

+        "script-src": ["'self'", "'strict-dynamic'"],
+    },
+    "content_security_policy_nonce_in": ["script-src"],
+    "force_https": False,


@kgabryje curious why this force_https change?

I wanted to limit the scope of the changes in this PR just to CSP - and force_https was disabled by default so far (since Talisman was disabled by default)

I agree with keeping this disabled by default - I would wager that by far the majority of prod Superset deployments terminate SSL/TLS on the LB.

eschutho

LGTM, i just left one question. Thanks @kgabryje!

villebro

Looks great! One honest question and one potential improvement idea, LMKWYT?

villebro · 2023-06-20T12:00:59Z

superset/config.py

+        "script-src": ["'self'", "'strict-dynamic'"],
+    },
+    "content_security_policy_nonce_in": ["script-src"],
+    "force_https": False,


I agree with keeping this disabled by default - I would wager that by far the majority of prod Superset deployments terminate SSL/TLS on the LB.

villebro · 2023-06-20T12:34:06Z

superset/config.py

+# 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"],
+}


shouldn't we set force_https: False here, too? AFAIK it defaults to True, right?

Yup nice catch!

villebro · 2023-06-20T12:35:50Z

superset/initialization/__init__.py

+        talisman_config = (
+            self.config["TALISMAN_DEV_CONFIG"]
+            if self.superset_app.debug
+            else self.config["TALISMAN_CONFIG"]
+        )


I'm wondering if it's a good idea to have separate dev and non-dev configs. Should these maybe be TALISMAN_DEFAULT_DEV_CONFIG and TALISMAN_DEFAULT_PROD_CONFIG, and only if TALISMAN_CONFIG is undefined would we fall back to the default. Thoughts?

We must have "unsafe-eval" in dev mode - React and Webpack use it and there's no way around it

villebro

LGTM

This reverts commit c3b5d72.

kgabryje added risk:breaking-change Issues or PRs that will introduce breaking changes v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch labels Jun 1, 2023

kgabryje requested review from dpgaspar, eschutho and michael-s-molina June 1, 2023 11:49

pull-request-size bot added the size/L label Jun 1, 2023

kgabryje commented Jun 1, 2023

View reviewed changes

kgabryje requested a review from villebro June 1, 2023 11:54

github-advanced-security bot found potential problems Jun 1, 2023

View reviewed changes

superset/templates/superset/theme.html Fixed Show fixed Hide fixed

kgabryje force-pushed the chore/csp branch from 34cb843 to 8164a99 Compare June 1, 2023 12:47

kgabryje force-pushed the chore/csp branch from e022ffb to dd01887 Compare June 2, 2023 11:26

dpgaspar approved these changes Jun 15, 2023

View reviewed changes

eschutho reviewed Jun 16, 2023

View reviewed changes

eschutho approved these changes Jun 16, 2023

View reviewed changes

kgabryje force-pushed the chore/csp branch from dd01887 to b1876c9 Compare June 19, 2023 13:33

kgabryje added 6 commits June 19, 2023 15:33

chore: Enable CSP by default

d049b27

Update docs

bb6511a

Add integrity check

f4fb6dc

Add integrity check

ce51a8a

Update docs

8a7f720

Update UPDATING.md

ba3b963

kgabryje force-pushed the chore/csp branch from b1876c9 to ba3b963 Compare June 19, 2023 13:34

villebro reviewed Jun 20, 2023

View reviewed changes

Set force_https to false in dev mode

37698f2

villebro approved these changes Jun 20, 2023

View reviewed changes

kgabryje merged commit c3b5d72 into apache:master Jun 20, 2023
31 checks passed

kgabryje added a commit to kgabryje/incubator-superset that referenced this pull request Jun 21, 2023

Revert "chore: Enable CSP by default (apache#24262)"

f96e032

This reverts commit c3b5d72.

hughhhh added a commit to preset-io/superset that referenced this pull request Jun 21, 2023

Revert "chore: Enable CSP by default (apache#24262)"

cc9d013

This reverts commit c3b5d72.

michael-s-molina removed the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jun 26, 2023

kgabryje mentioned this pull request Jun 28, 2023

chore: Un-revert enabling CSP by default #24543

Merged

9 tasks

Jobisworking mentioned this pull request Aug 6, 2023

HTTP_HEADERS DEAFAULT_HTTP_HEADERS and OVERRIDE_HTTP_HEADERS seem not to have any effect #24785

Open

Jaszkowic mentioned this pull request Aug 10, 2023

CSP directive needs 'unsafe-eval' to evaluate Deck.gl "Javascript Data Interceptor" #24947

Open

3 tasks

ramki88 mentioned this pull request Sep 23, 2023

Superset 3.0 not able to login, cookies are being set as secure even though the site is http. #25374

Closed

This was referenced Oct 16, 2023

New version 2.1.1, 3.0.1 stackabletech/superset-operator#415

Merged

Configure Content Security Policy (CSP) properly stackabletech/superset-operator#416

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Enable CSP by default #24262

chore: Enable CSP by default #24262

kgabryje commented Jun 1, 2023

kgabryje Jun 1, 2023

codecov bot commented Jun 1, 2023 •

edited

sfirke commented Jun 1, 2023

kgabryje commented Jun 1, 2023

dpgaspar left a comment

eschutho Jun 16, 2023

kgabryje Jun 19, 2023

villebro Jun 20, 2023

eschutho left a comment

villebro left a comment

villebro Jun 20, 2023

villebro Jun 20, 2023

kgabryje Jun 20, 2023

villebro Jun 20, 2023

kgabryje Jun 20, 2023

villebro left a comment

chore: Enable CSP by default #24262

chore: Enable CSP by default #24262

Conversation

kgabryje commented Jun 1, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

codecov bot commented Jun 1, 2023 • edited

Codecov Report

sfirke commented Jun 1, 2023

kgabryje commented Jun 1, 2023

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

Choose a reason for hiding this comment

eschutho left a 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

villebro left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 1, 2023 •

edited