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

fix(Embedded): Skip CSRF validation for dashboard download endpoints #31798

Merged
merged 1 commit into from
Jan 13, 2025
Merged

fix(Embedded): Skip CSRF validation for dashboard download endpoints #31798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 6 additions & 15 deletions superset/dashboards/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,19 @@
import logging
from datetime import datetime
from io import BytesIO
from typing import Any, Callable, cast, Optional
from typing import Any, Callable, cast
from zipfile import is_zipfile, ZipFile

from flask import g, redirect, request, Response, send_file, url_for
from flask_appbuilder import permission_name
from flask_appbuilder.api import expose, protect, rison, safe
from flask_appbuilder.hooks import before_request
from flask_appbuilder.models.sqla.interface import SQLAInterface
from flask_babel import gettext, ngettext
from marshmallow import ValidationError
from werkzeug.wrappers import Response as WerkzeugResponse
from werkzeug.wsgi import FileWrapper

from superset import db, is_feature_enabled, thumbnail_cache
from superset import db, thumbnail_cache
from superset.charts.schemas import ChartEntityResponseSchema
from superset.commands.dashboard.copy import CopyDashboardCommand
from superset.commands.dashboard.create import CreateDashboardCommand
Expand Down Expand Up @@ -124,6 +123,7 @@
requires_form_data,
requires_json,
statsd_metrics,
validate_feature_flags,
)
from superset.views.error_handling import handle_api_exception
from superset.views.filters import (
Expand Down Expand Up @@ -160,18 +160,6 @@ def wraps(self: BaseSupersetModelRestApi, id_or_slug: str) -> Response:
class DashboardRestApi(BaseSupersetModelRestApi):
datamodel = SQLAInterface(Dashboard)

@before_request(only=["thumbnail", "cache_dashboard_screenshot", "screenshot"])
def ensure_thumbnails_enabled(self) -> Optional[Response]:
if not is_feature_enabled("THUMBNAILS"):
return self.response_404()
return None

@before_request(only=["cache_dashboard_screenshot", "screenshot"])
def ensure_screenshots_enabled(self) -> Optional[Response]:
if not is_feature_enabled("ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS"):
return self.response_404()
return None

include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | {
RouteMethod.EXPORT,
RouteMethod.IMPORT,
Expand Down Expand Up @@ -1038,6 +1026,7 @@ def export(self, **kwargs: Any) -> Response:
return response

@expose("/<pk>/thumbnail/<digest>/", methods=("GET",))
@validate_feature_flags(["THUMBNAILS"])
@protect()
@safe
@rison(thumbnail_query_schema)
Expand Down Expand Up @@ -1141,6 +1130,7 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse:
)

@expose("/<pk>/cache_dashboard_screenshot/", methods=("POST",))
@validate_feature_flags(["THUMBNAILS", "ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS"])
Copy link
Member

Choose a reason for hiding this comment

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

I am thinking if we really need the couple here at this point or ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS will suffice. What's your thought? I have been meaning to remove this dependency before and this could be a good chance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@geido if we remove the requirement to have THUMBNAILS enabled for this endpoint, but then we keep it at the digest endpoint, then at the end the process would still not work unless you have both enabled.

Also, currently when you trigger a "screenshot generation" it returns the digest link, which returns a 404 status code until it's being processed. If THUMBNAILS is disabled, it would return 404 forever and users wouldn't have an easy way to spot the issue.

I think we could either:

  • Remove the THUMBNAILS check for this endpoint, and then update the check on the digest endpoint to validate for either THUMBNAILS OR ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS. This way you can still download a dashboard screenshot without having THUMBNAILS FF enabled; or
  • Update the other endpoint to return a different status code if the THUMBNAILS FF is disabled. Also update the frontend (if that's not the case yet) to validate if both FFs are enabled to decide if frontend or backend processing should be used.

One potential issue with the first approach is that environments with ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS enabled but THUMBNAILS disabled would still expose the digest endpoint, so it's possible that disabling THUMBNAILS FF would no longer be sufficient to remove thumbnails from displaying in the card views.

Let me know your thoughts!

Copy link
Member

Choose a reason for hiding this comment

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

Alright. I think there is a case here to keep both. Maybe @fisjac can consider the above for the work that he is doing around screenshot generation for future improvements. Thanks for this!

@protect()
@rison(screenshot_query_schema)
@safe
Expand Down Expand Up @@ -1241,6 +1231,7 @@ def trigger_celery() -> WerkzeugResponse:
return trigger_celery()

@expose("/<pk>/screenshot/<digest>/", methods=("GET",))
@validate_feature_flags(["THUMBNAILS", "ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS"])
@protect()
@safe
@statsd_metrics
Expand Down
24 changes: 24 additions & 0 deletions superset/views/base_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
from sqlalchemy import and_, distinct, func
from sqlalchemy.orm.query import Query

from superset import is_feature_enabled
from superset.exceptions import InvalidPayloadFormatError
from superset.extensions import db, event_logger, security_manager, stats_logger_manager
from superset.models.core import FavStar
Expand Down Expand Up @@ -130,6 +131,29 @@ def wraps(self: BaseSupersetApiMixin, *args: Any, **kwargs: Any) -> Response:
return functools.update_wrapper(wraps, f)


def validate_feature_flags(
feature_flags: list[str],
) -> Callable[[Callable[..., Response]], Callable[..., Response]]:
"""
A decorator to check if all given feature flags are enabled.

:param feature_flags: List of feature flag names to be checked.
"""

def decorate(f: Callable[..., Response]) -> Callable[..., Response]:
@functools.wraps(f)
def wrapper(
self: BaseSupersetModelRestApi, *args: Any, **kwargs: Any
) -> Response:
if not all(is_feature_enabled(flag) for flag in feature_flags):
return self.response_404()
Comment on lines +148 to +149
Copy link

Choose a reason for hiding this comment

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

Incorrect HTTP Status Code for Disabled Features category Functionality

Tell me more
What is the issue?

The decorator returns a 404 Not Found response when feature flags are disabled, which is semantically incorrect and misleading.

Why this matters

A 404 response indicates a resource was not found, but in this case, the resource exists but is intentionally disabled. This could confuse API clients and monitoring systems trying to understand why the endpoint is not accessible.

Suggested change ∙ Feature Preview

Return a 403 Forbidden status code which better represents that the feature exists but access is not permitted:

if not all(is_feature_enabled(flag) for flag in feature_flags):
    return self.response_403()
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

return f(self, *args, **kwargs)

return wrapper

return decorate


class RelatedFieldFilter:
# data class to specify what filter to use on a /related endpoint
# pylint: disable=too-few-public-methods
Expand Down
84 changes: 51 additions & 33 deletions tests/integration_tests/dashboards/api_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@

from tests.integration_tests.base_api_tests import ApiOwnersTestCaseMixin
from tests.integration_tests.base_tests import SupersetTestCase
from tests.integration_tests.conftest import with_feature_flags
from tests.integration_tests.constants import (
ADMIN_USERNAME,
ALPHA_USERNAME,
Expand Down Expand Up @@ -3027,10 +3028,9 @@ def _get_screenshot(self, dashboard_id, cache_key, download_format):
uri = f"/api/v1/dashboard/{dashboard_id}/screenshot/{cache_key}/?download_format={download_format}" # noqa: E501
return self.client.get(uri)

@with_feature_flags(THUMBNAILS=True, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
@pytest.mark.usefixtures("create_dashboard_with_tag")
@patch("superset.dashboards.api.is_feature_enabled")
def test_cache_dashboard_screenshot_success(self, is_feature_enabled):
is_feature_enabled.return_value = True
def test_cache_dashboard_screenshot_success(self):
self.login(ADMIN_USERNAME)
dashboard = (
db.session.query(Dashboard)
Expand All @@ -3040,10 +3040,9 @@ def test_cache_dashboard_screenshot_success(self, is_feature_enabled):
response = self._cache_screenshot(dashboard.id)
assert response.status_code == 202

@with_feature_flags(THUMBNAILS=True, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
@pytest.mark.usefixtures("create_dashboard_with_tag")
@patch("superset.dashboards.api.is_feature_enabled")
def test_cache_dashboard_screenshot_dashboard_validation(self, is_feature_enabled):
is_feature_enabled.return_value = True
def test_cache_dashboard_screenshot_dashboard_validation(self):
self.login(ADMIN_USERNAME)
dashboard = (
db.session.query(Dashboard)
Expand All @@ -3059,25 +3058,21 @@ def test_cache_dashboard_screenshot_dashboard_validation(self, is_feature_enable
response = self._cache_screenshot(dashboard.id, invalid_payload)
assert response.status_code == 400

@patch("superset.dashboards.api.is_feature_enabled")
def test_cache_dashboard_screenshot_dashboard_not_found(self, is_feature_enabled):
is_feature_enabled.return_value = True
@with_feature_flags(THUMBNAILS=True, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
def test_cache_dashboard_screenshot_dashboard_not_found(self):
self.login(ADMIN_USERNAME)
non_existent_id = 999
response = self._cache_screenshot(non_existent_id)
assert response.status_code == 404

@with_feature_flags(THUMBNAILS=True, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
@pytest.mark.usefixtures("create_dashboard_with_tag")
@patch("superset.dashboards.api.cache_dashboard_screenshot")
@patch("superset.dashboards.api.DashboardScreenshot.get_from_cache_key")
@patch("superset.dashboards.api.is_feature_enabled")
def test_screenshot_success_png(
self, is_feature_enabled, mock_get_cache, mock_cache_task
):
def test_screenshot_success_png(self, mock_get_cache, mock_cache_task):
"""
Validate screenshot returns png
"""
is_feature_enabled.return_value = True
self.login(ADMIN_USERNAME)
mock_cache_task.return_value = None
mock_get_cache.return_value = BytesIO(b"fake image data")
Expand All @@ -3096,18 +3091,17 @@ def test_screenshot_success_png(
assert response.mimetype == "image/png"
assert response.data == b"fake image data"

@with_feature_flags(THUMBNAILS=True, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
@pytest.mark.usefixtures("create_dashboard_with_tag")
@patch("superset.dashboards.api.cache_dashboard_screenshot")
@patch("superset.dashboards.api.build_pdf_from_screenshots")
@patch("superset.dashboards.api.DashboardScreenshot.get_from_cache_key")
@patch("superset.dashboards.api.is_feature_enabled")
def test_screenshot_success_pdf(
self, is_feature_enabled, mock_get_from_cache, mock_build_pdf, mock_cache_task
self, mock_get_from_cache, mock_build_pdf, mock_cache_task
):
"""
Validate screenshot can return pdf.
"""
is_feature_enabled.return_value = True
self.login(ADMIN_USERNAME)
mock_cache_task.return_value = None
mock_get_from_cache.return_value = BytesIO(b"fake image data")
Expand All @@ -3127,14 +3121,11 @@ def test_screenshot_success_pdf(
assert response.mimetype == "application/pdf"
assert response.data == b"fake pdf data"

@with_feature_flags(THUMBNAILS=True, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
@pytest.mark.usefixtures("create_dashboard_with_tag")
@patch("superset.dashboards.api.cache_dashboard_screenshot")
@patch("superset.dashboards.api.DashboardScreenshot.get_from_cache_key")
@patch("superset.dashboards.api.is_feature_enabled")
def test_screenshot_not_in_cache(
self, is_feature_enabled, mock_get_cache, mock_cache_task
):
is_feature_enabled.return_value = True
def test_screenshot_not_in_cache(self, mock_get_cache, mock_cache_task):
self.login(ADMIN_USERNAME)
mock_cache_task.return_value = None
mock_get_cache.return_value = None
Expand All @@ -3151,22 +3142,18 @@ def test_screenshot_not_in_cache(
response = self._get_screenshot(dashboard.id, cache_key, "pdf")
assert response.status_code == 404

@patch("superset.dashboards.api.is_feature_enabled")
def test_screenshot_dashboard_not_found(self, is_feature_enabled):
is_feature_enabled.return_value = True
@with_feature_flags(THUMBNAILS=True, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
def test_screenshot_dashboard_not_found(self):
self.login(ADMIN_USERNAME)
non_existent_id = 999
response = self._get_screenshot(non_existent_id, "some_cache_key", "png")
assert response.status_code == 404

@with_feature_flags(THUMBNAILS=True, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
@pytest.mark.usefixtures("create_dashboard_with_tag")
@patch("superset.dashboards.api.cache_dashboard_screenshot")
@patch("superset.dashboards.api.DashboardScreenshot.get_from_cache_key")
@patch("superset.dashboards.api.is_feature_enabled")
def test_screenshot_invalid_download_format(
self, is_feature_enabled, mock_get_cache, mock_cache_task
):
is_feature_enabled.return_value = True
def test_screenshot_invalid_download_format(self, mock_get_cache, mock_cache_task):
self.login(ADMIN_USERNAME)
mock_cache_task.return_value = None
mock_get_cache.return_value = BytesIO(b"fake png data")
Expand All @@ -3184,10 +3171,41 @@ def test_screenshot_invalid_download_format(
response = self._get_screenshot(dashboard.id, cache_key, "invalid")
assert response.status_code == 404

@with_feature_flags(THUMBNAILS=False, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
@pytest.mark.usefixtures("create_dashboard_with_tag")
def test_cache_dashboard_screenshot_feature_thumbnails_ff_disabled(self):
self.login(ADMIN_USERNAME)

dashboard = (
db.session.query(Dashboard)
.filter(Dashboard.dashboard_title == "dash with tag")
.first()
)

assert dashboard is not None

response = self._cache_screenshot(dashboard.id)
assert response.status_code == 404

@with_feature_flags(THUMBNAILS=True, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=False)
@pytest.mark.usefixtures("create_dashboard_with_tag")
def test_cache_dashboard_screenshot_feature_screenshot_ff_disabled(self):
self.login(ADMIN_USERNAME)

dashboard = (
db.session.query(Dashboard)
.filter(Dashboard.dashboard_title == "dash with tag")
.first()
)

assert dashboard is not None

response = self._cache_screenshot(dashboard.id)
assert response.status_code == 404

@with_feature_flags(THUMBNAILS=False, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=False)
@pytest.mark.usefixtures("create_dashboard_with_tag")
@patch("superset.dashboards.api.is_feature_enabled")
def test_cache_dashboard_screenshot_feature_disabled(self, is_feature_enabled):
is_feature_enabled.return_value = False
def test_cache_dashboard_screenshot_feature_both_ff_disabled(self):
self.login(ADMIN_USERNAME)

dashboard = (
Expand Down
Loading