-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
refactor: Removes the deprecated ENABLE_EXPLORE_JSON_CSRF_PROTECTION feature flag #26344
refactor: Removes the deprecated ENABLE_EXPLORE_JSON_CSRF_PROTECTION feature flag #26344
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #26344 +/- ##
==========================================
- Coverage 69.07% 69.06% -0.02%
==========================================
Files 1930 1936 +6
Lines 75278 75375 +97
Branches 8429 8432 +3
==========================================
+ Hits 51998 52057 +59
- Misses 21133 21160 +27
- Partials 2147 2158 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -239,8 +239,6 @@ def explore_json_data(self, cache_key: str) -> FlaskResponse: | |||
return json_error_response(utils.error_msg_from_exception(ex), 400) | |||
|
|||
EXPLORE_JSON_METHODS = ["POST"] | |||
if not is_feature_enabled("ENABLE_EXPLORE_JSON_CSRF_PROTECTION"): |
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.
As this was False
by default, shouldn't we keep the EXPLORE_JSON_METHODS.append("GET")
?
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.
Nice catch! @dpgaspar can you confirm the expected behavior?
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.
@geido Confirmed and added the GET
method by default as you suggested.
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.
To my understanding this feature flag would allow explore_json
to accept HTTP GETs but still imposing CSRF protection. So when False
this endpoint would only accept HTTP POSTs and so would be automatically protected to CSRF. Shouldn't we keep the same behaviour and just allow for POST and be always protected?
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.
To my understanding this feature flag would allow explore_json to accept HTTP GETs but still imposing CSRF protection.
This feature flag was False
by default, allowing GET
. There's no other control than excluding GET
when this feature flag is True
.
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.
missed the not
on the feature flag check. Makes sense
8d380f9
to
1ed5013
Compare
@@ -239,8 +239,6 @@ def explore_json_data(self, cache_key: str) -> FlaskResponse: | |||
return json_error_response(utils.error_msg_from_exception(ex), 400) | |||
|
|||
EXPLORE_JSON_METHODS = ["POST"] | |||
if not is_feature_enabled("ENABLE_EXPLORE_JSON_CSRF_PROTECTION"): |
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.
missed the not
on the feature flag check. Makes sense
SUMMARY
As part of the 4.0 approved initiatives, this PR removes the
ENABLE_EXPLORE_JSON_CSRF_PROTECTION
feature flag.The previous value of the feature flag was
False
and now the feature is permanently removed.TESTING INSTRUCTIONS
CI should be sufficient for merging this PR. We'll do a complete testing of 4.0 after all approved proposals are merged.
ADDITIONAL INFORMATION