Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(helm): remove config overrides for CSRF #22716

Merged

Conversation

reidab
Copy link
Contributor

@reidab reidab commented Jan 13, 2023

SUMMARY

Fixes #22715

This PR removes the WTF_ CSRF-related overrides from the configuration file generated by the Helm chart, allowing the defaults from config.py to be used.

superset/superset/config.py

Lines 249 to 257 in 2ccdb72

# Flask-WTF flag for CSRF
WTF_CSRF_ENABLED = True
# Add endpoints that need to be exempt from CSRF protection
WTF_CSRF_EXEMPT_LIST = [
"superset.views.core.log",
"superset.views.core.explore_json",
"superset.charts.data.api.data",
]

As noted in #22715, there may be better ways to handle the CSRF exclusion for these three routes outside of the config file to prevent this same mistake from being made in other context. This is just the simplest solution that solves the issue with the Helm chart clearing the list.

TESTING INSTRUCTIONS

  1. Deploy a copy of Superset using the Helm chart
  2. Navigate around the UI
  3. Confirm that calls to /superset/log are returning 200 instead of 302 redirects to /login

ADDITIONAL INFORMATION

  • Has associated issue: Helm chart overrides default WTF_CSRF_EXEMPT_LIST with blank array #22715
  • 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

Copy link
Member

@craig-rueda craig-rueda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to re-generate the helm README as the version bump requires this

This prevents the configuration generated by the helm chart from
overriding the default WTF_ configuration values in config.py.

Without these default values, calls to three logging and chart data
endpoints will fail with CSRF errors.
@reidab reidab force-pushed the rm-wtf-overrides-from-helm-config branch from 4a6cfaf to de3407d Compare January 13, 2023 18:26
@reidab
Copy link
Contributor Author

reidab commented Jan 13, 2023

You need to re-generate the helm README as the version bump requires this

@craig-rueda just re-generated the README and updated the branch.

@craig-rueda craig-rueda merged commit 85da86d into apache:master Jan 13, 2023
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm chart overrides default WTF_CSRF_EXEMPT_LIST with blank array
3 participants