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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jan 18, 2024
Merged

refactor: Removes the deprecated ENABLE_EXPLORE_JSON_CSRF_PROTECTION feature flag #26344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a3b8293
refactor: Removes the ENABLE_EXPLORE_JSON_CSRF_PROTECTION feature flag
michael-s-molina Dec 22, 2023
4199b35
Fixes tests
michael-s-molina Dec 26, 2023
a00ac27
Fixes tests 2
michael-s-molina Dec 26, 2023
5ab9a3e
Update UPDATING.md
michael-s-molina Jan 5, 2024
1ed5013
Adds GET
michael-s-molina Jan 8, 2024
423996b
Merge branch 'master' into remove-csrf-protection-ff
michael-s-molina Jan 16, 2024
f3a1ccd
Merge branch 'master' into remove-csrf-protection-ff
michael-s-molina Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion RESOURCES/FEATURE_FLAGS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ These features flags currently default to True and **will be removed in a future
- DASHBOARD_CROSS_FILTERS
- DASHBOARD_FILTERS_EXPERIMENTAL
- DASHBOARD_NATIVE_FILTERS
- ENABLE_EXPLORE_JSON_CSRF_PROTECTION
- ENABLE_JAVASCRIPT_CONTROLS
- GENERIC_CHART_AXES
- KV_STORE
Expand Down
1 change: 1 addition & 0 deletions UPDATING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ assists people when migrating to a new version.

### Breaking Changes

- [26344](https://github.com/apache/superset/issues/26344): Removes the deprecated `ENABLE_EXPLORE_JSON_CSRF_PROTECTION` feature flag. The previous value of the feature flag was `False` and now the feature is permanently removed.
- [26345](https://github.com/apache/superset/issues/26345): Removes the deprecated `ENABLE_TEMPLATE_REMOVE_FILTERS` feature flag. The previous value of the feature flag was `True` and now the feature is permanently enabled.
- [26346](https://github.com/apache/superset/issues/26346): Removes the deprecated `REMOVE_SLICE_LEVEL_LABEL_COLORS` feature flag. The previous value of the feature flag was `False` and now the feature is permanently removed.
- [26348](https://github.com/apache/superset/issues/26348): Removes the deprecated `CLIENT_CACHE` feature flag. The previous value of the feature flag was `False` and now the feature is permanently removed.
Expand Down
1 change: 0 additions & 1 deletion docs/docs/installation/configuring-superset.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,6 @@ You can enable or disable features with flag from `superset_config.py`:

```python
FEATURE_FLAGS = {
'ENABLE_EXPLORE_JSON_CSRF_PROTECTION': False,
'PRESTO_EXPAND_DATA': False,
}
```
Expand Down
8 changes: 0 additions & 8 deletions superset/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,14 +409,6 @@ class D3Format(TypedDict, total=False):
# editor no longer shows. Currently this is set to false so that the editor
# option does show, but we will be depreciating it.
"DISABLE_LEGACY_DATASOURCE_EDITOR": True,
# For some security concerns, you may need to enforce CSRF protection on
# all query request to explore_json endpoint. In Superset, we use
# `flask-csrf <https://sjl.bitbucket.io/flask-csrf/>`_ add csrf protection
# for all POST requests, but this protection doesn't apply to GET method.
# When ENABLE_EXPLORE_JSON_CSRF_PROTECTION is set to true, your users cannot
# make GET request to explore_json. explore_json accepts both GET and POST request.
# See `PR 7935 <https://github.com/apache/superset/pull/7935>`_ for more details.
"ENABLE_EXPLORE_JSON_CSRF_PROTECTION": False, # deprecated
"ENABLE_TEMPLATE_PROCESSING": False,
# Allow for javascript controls components
# this enables programmers to customize certain charts (like the
Expand Down
18 changes: 12 additions & 6 deletions superset/views/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
# pylint: disable=invalid-name
# pylint: disable=too-many-lines
from __future__ import annotations

import contextlib
Expand Down Expand Up @@ -238,19 +239,24 @@ def explore_json_data(self, cache_key: str) -> FlaskResponse:
except SupersetException as ex:
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"):
Copy link
Member

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") ?

Copy link
Member Author

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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

EXPLORE_JSON_METHODS.append("GET")

@api
@has_access_api
@handle_api_exception
@event_logger.log_this
@expose(
"/explore_json/<datasource_type>/<int:datasource_id>/",
methods=EXPLORE_JSON_METHODS,
methods=(
"GET",
"POST",
),
)
@expose(
"/explore_json/",
methods=(
"GET",
"POST",
),
)
@expose("/explore_json/", methods=EXPLORE_JSON_METHODS)
@etag_cache()
@check_resource_permissions(check_datasource_perms)
@deprecated(eol_version="4.0.0")
Expand Down
9 changes: 8 additions & 1 deletion tests/integration_tests/core_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,8 +559,15 @@ def test_comments_in_sqlatable_query(self):
self.assertEqual(clean_query, rendered_query)

def test_slice_payload_no_datasource(self):
form_data = {
"viz_type": "dist_bar",
}
self.login(username="admin")
data = self.get_json_resp("/superset/explore_json/", raise_on_error=False)
rv = self.client.post(
"/superset/explore_json/",
data={"form_data": json.dumps(form_data)},
)
data = json.loads(rv.data.decode("utf-8"))

self.assertEqual(
data["errors"][0]["message"],
Expand Down
Loading