-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
[feature flag] Enforce csrf protection on explore_json endpoint #7935
[feature flag] Enforce csrf protection on explore_json endpoint #7935
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT
8e33b5b
to
45f11af
Compare
Codecov Report
@@ Coverage Diff @@
## master #7935 +/- ##
==========================================
+ Coverage 65.97% 65.97% +<.01%
==========================================
Files 468 468
Lines 22297 22304 +7
Branches 2429 2429
==========================================
+ Hits 14710 14715 +5
- Misses 7466 7468 +2
Partials 121 121
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one comment, otherwise lgtm
@@ -204,7 +204,8 @@ | |||
# will result in combined feature flags of { 'FOO': True, 'BAR': True, 'BAZ': True } | |||
DEFAULT_FEATURE_FLAGS = { | |||
# Experimental feature introducing a client (browser) cache | |||
"CLIENT_CACHE": False | |||
"CLIENT_CACHE": False, | |||
"ENABLE_EXPLORE_JSON_CSRF_PROTECTION": False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a comment explaining this flag too?
45f11af
to
85dc23b
Compare
also added a section for featured flags in http://superset.incubator.apache.org/installation.html
I added a section in http://superset.incubator.apache.org/installation.html for featured flags: |
85dc23b
to
41d2305
Compare
CATEGORY
Choose one
SUMMARY
This PR is to resume the work in #7449. For some security concerns, we need to enforce CSRF protection on query request to
explore_json
endpoint.So I want to add a new feature flag:
ENABLE_EXPLORE_JSON_CSRF_PROTECTION
. WhenENABLE_EXPLORE_JSON_CSRF_PROTECTION
is set to true, user cannot make GET request toexplore_json
.The default value for this feature
False
(current behavior), explore_json accepts both GET and POST request.TEST PLAN
send GET request to
explore_json
, you will get405 Method Not Allowed
exception.REVIEWERS
@DiggidyDave @betodealmeida @john-bodley @mistercrunch