You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/08/21 16:11:33 UTC
(superset) 03/05: fix(dashboards): resolve permalink keys as the caller and bind cache entries to the requesting object
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch fix/report-webhook-executor-and-cache-hardening
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 7f1b74e363b7b9d793761576024c3b9cffeeb1de
Author: Superset Dev <de...@superset.apache.org>
AuthorDate: Fri Aug 21 01:46:03 2026 -0700
fix(dashboards): resolve permalink keys as the caller and bind cache entries to the requesting object
cache_dashboard_screenshot embedded a caller-supplied permalinkKey verbatim
into the URL rendered by the (potentially more-privileged) screenshot
executor, without ever resolving the key as the calling user. A caller with
access to one dashboard but only the permalink key of another could get the
second dashboard rendered under the executor's identity and read back
through the accessible dashboard's screenshot endpoint.
Caller-supplied permalink keys are now resolved via
GetDashboardPermalinkCommand, which access-checks the target dashboard as
the caller, and are rejected unless they resolve to the dashboard the
request is for.
Separately, cache_key/digest was the only thing gating the screenshot GET
endpoints, and cache entries are shared across every dashboard and chart in
the same cache backend, so any digest learned for one object could be
replayed to fetch another's cached render through a different, merely-
accessible pk. ScreenshotCachePayload entries now carry a scope
("dashboard:<id>" / "chart:<id>") stamped when the screenshot is computed,
and both dashboard and chart screenshot GET endpoints check it against the
requested pk before serving.
---
superset/charts/api.py | 6 +++
superset/dashboards/api.py | 57 ++++++++++++++++++--
superset/tasks/thumbnails.py | 3 ++
superset/utils/screenshots.py | 27 ++++++++++
tests/integration_tests/dashboards/api_tests.py | 69 ++++++++++++++++++++-----
tests/unit_tests/utils/screenshot_test.py | 54 +++++++++++++++++++
6 files changed, 198 insertions(+), 18 deletions(-)
diff --git a/superset/charts/api.py b/superset/charts/api.py
index 1e3626bd77d..232cad8a5c7 100644
--- a/superset/charts/api.py
+++ b/superset/charts/api.py
@@ -1112,6 +1112,12 @@ class ChartRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi):
return self.response_404()
if cache_payload := ChartScreenshot.get_from_cache_key(digest):
+ # The digest is caller-supplied and cache entries are shared
+ # across every chart (and, via the same backend, dashboards) --
+ # without this check any cache_key learned for one chart would
+ # serve its image under a different, merely-accessible `pk`.
+ if cache_payload.get_scope() != f"chart:{chart.id}":
+ return self.response_404()
if cache_payload.status == StatusValues.UPDATED:
try:
image = cache_payload.get_image()
diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py
index cbde7183106..8084975ff89 100644
--- a/superset/dashboards/api.py
+++ b/superset/dashboards/api.py
@@ -74,6 +74,7 @@ from superset.commands.dashboard.export_example import ExportExampleCommand
from superset.commands.dashboard.fave import AddFavoriteDashboardCommand
from superset.commands.dashboard.importers.dispatcher import ImportDashboardsCommand
from superset.commands.dashboard.permalink.create import CreateDashboardPermalinkCommand
+from superset.commands.dashboard.permalink.get import GetDashboardPermalinkCommand
from superset.commands.dashboard.restore import RestoreDashboardCommand
from superset.commands.dashboard.unfave import DelFavoriteDashboardCommand
from superset.commands.dashboard.update import (
@@ -105,6 +106,7 @@ from superset.dashboards.filters import (
DashboardTagNameFilter,
DashboardTitleOrSlugFilter,
)
+from superset.dashboards.permalink.exceptions import DashboardPermalinkGetFailedError
from superset.dashboards.permalink.types import DashboardPermalinkState
from superset.dashboards.schemas import (
CacheScreenshotSchema,
@@ -1842,6 +1844,43 @@ class DashboardRestApi(
raise
return self.response(202, job_id=job_id)
+ def _validate_permalink_for_dashboard(
+ self, permalink_key: str, dashboard: Dashboard
+ ) -> WerkzeugResponse | None:
+ """
+ Resolve (and access-check, as the calling user) a caller-supplied
+ permalink key, and confirm it belongs to `dashboard`.
+
+ A permalink key is resolved as the calling user, before it's ever
+ handed to the (potentially more-privileged) screenshot executor --
+ otherwise a caller with access only to `dashboard` could pass the
+ permalink key of a dashboard they can't access and have it rendered
+ under the executor's identity.
+
+ :returns: An error response if the key doesn't resolve, isn't
+ accessible to the caller, or belongs to a different dashboard;
+ ``None`` if it's valid for `dashboard`.
+ """
+ try:
+ permalink_value = GetDashboardPermalinkCommand(permalink_key).run()
+ except DashboardPermalinkGetFailedError:
+ return self.response_404()
+ except DashboardAccessDeniedError:
+ return self.response_403()
+ if not permalink_value:
+ return self.response_404()
+ try:
+ permalink_dashboard = DashboardDAO.get_by_id_or_slug(
+ permalink_value["dashboardId"]
+ )
+ except DashboardAccessDeniedError:
+ return self.response_403()
+ except DashboardNotFoundError:
+ return self.response_404()
+ if permalink_dashboard.id != dashboard.id:
+ return self.response_403()
+ return None
+
@expose("/<pk>/cache_dashboard_screenshot/", methods=("POST",))
@validate_feature_flags(["THUMBNAILS", "ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS"])
@protect()
@@ -1912,13 +1951,17 @@ class DashboardRestApi(
# if the permalink key is provided, dashboard_state will be ignored
# else, create a permalink key from the dashboard_state
- permalink_key = (
- payload.get("permalinkKey", None)
- or CreateDashboardPermalinkCommand(
+ permalink_key = payload.get("permalinkKey", None)
+ if permalink_key:
+ if error_response := self._validate_permalink_for_dashboard(
+ permalink_key, dashboard
+ ):
+ return error_response
+ else:
+ permalink_key = CreateDashboardPermalinkCommand(
dashboard_id=str(dashboard.id),
state=dashboard_state,
).run()
- )
dashboard_url = get_url_path("Superset.dashboard_permalink", key=permalink_key)
screenshot_obj = DashboardScreenshot(dashboard_url, dashboard.digest)
@@ -2019,6 +2062,12 @@ class DashboardRestApi(
# fetch the dashboard screenshot using the current user and cache if set
if cache_payload := DashboardScreenshot.get_from_cache_key(digest):
+ # The digest is caller-supplied and cache entries are shared across
+ # every dashboard (and, via the same backend, charts) -- without
+ # this check any cache_key learned for one dashboard would serve
+ # its image under a different, merely-accessible `pk`.
+ if cache_payload.get_scope() != f"dashboard:{dashboard.id}":
+ return self.response_404()
try:
image = cache_payload.get_image()
except ScreenshotImageNotAvailableException:
diff --git a/superset/tasks/thumbnails.py b/superset/tasks/thumbnails.py
index c80630e3114..cd03cabd53e 100644
--- a/superset/tasks/thumbnails.py
+++ b/superset/tasks/thumbnails.py
@@ -65,6 +65,7 @@ def cache_chart_thumbnail(
user = security_manager.find_user(username)
with override_user(user):
screenshot = ChartScreenshot(url, chart.digest)
+ screenshot.cache_scope = f"chart:{chart.id}"
screenshot.compute_and_cache(
user=user,
window_size=window_size,
@@ -102,6 +103,7 @@ def cache_dashboard_thumbnail(
user = security_manager.find_user(username)
with override_user(user):
screenshot = DashboardScreenshot(url, dashboard.digest)
+ screenshot.cache_scope = f"dashboard:{dashboard.id}"
resolved_cache_key = cache_key or screenshot.get_cache_key(
window_size, thumb_size
)
@@ -149,6 +151,7 @@ def cache_dashboard_screenshot( # pylint: disable=too-many-arguments
with override_user(current_user):
screenshot = DashboardScreenshot(dashboard_url, dashboard.digest)
+ screenshot.cache_scope = f"dashboard:{dashboard.id}"
screenshot.compute_and_cache(
user=current_user,
window_size=window_size,
diff --git a/superset/utils/screenshots.py b/superset/utils/screenshots.py
index 4fcd58c9c85..189b1c0d978 100644
--- a/superset/utils/screenshots.py
+++ b/superset/utils/screenshots.py
@@ -73,6 +73,13 @@ class ScreenshotCachePayloadType(TypedDict):
image: str | None
timestamp: str
status: str
+ # Identifies which object (e.g. "dashboard:<id>" or "chart:<id>") this
+ # entry was rendered for. Cache entries written before this field existed
+ # (or by a not-yet-upgraded worker during a rolling deploy) have no scope
+ # and are treated as belonging to no object -- read access is fail-closed
+ # until the entry is recomputed. Optional at the type level via `.get()`
+ # in `from_dict` for that reason.
+ scope: str | None
# Magic bytes for a cheap image sanity check. This is intentionally not a full
@@ -101,10 +108,12 @@ class ScreenshotCachePayload:
image: bytes | None = None,
status: StatusValues = StatusValues.PENDING,
timestamp: str = "",
+ scope: str | None = None,
):
self._image = image
self._timestamp = timestamp or datetime.now().isoformat()
self.status = StatusValues.UPDATED if image else status
+ self._scope = scope
@classmethod
def from_dict(cls, payload: ScreenshotCachePayloadType) -> ScreenshotCachePayload:
@@ -112,6 +121,9 @@ class ScreenshotCachePayload:
image=base64.b64decode(payload["image"]) if payload["image"] else None,
status=StatusValues(payload["status"]),
timestamp=payload["timestamp"],
+ # `.get` rather than `payload["scope"]`: entries cached before this
+ # field existed won't have the key.
+ scope=payload.get("scope"),
)
def to_dict(self) -> ScreenshotCachePayloadType:
@@ -121,8 +133,15 @@ class ScreenshotCachePayload:
else None,
"timestamp": self._timestamp,
"status": self.status.value,
+ "scope": self._scope,
}
+ def get_scope(self) -> str | None:
+ return self._scope
+
+ def set_scope(self, scope: str | None) -> None:
+ self._scope = scope
+
def update_timestamp(self) -> None:
self._timestamp = datetime.now().isoformat()
@@ -196,6 +215,13 @@ class BaseScreenshot:
window_size: WindowSize = DEFAULT_SCREENSHOT_WINDOW_SIZE
thumb_size: WindowSize = DEFAULT_SCREENSHOT_THUMBNAIL_SIZE
cache: Cache = thumbnail_cache
+ # Set by the caller (e.g. "dashboard:<id>" or "chart:<id>") before
+ # compute_and_cache() so the resulting cache entry records which object it
+ # was rendered for -- callers that later serve a cache entry by a
+ # caller-supplied digest/cache_key must check this against the object
+ # they're authorizing, since the same cache backend is shared across
+ # every dashboard and chart.
+ cache_scope: str | None = None
def __init__(self, url: str, digest: str | None):
self.digest = digest
@@ -315,6 +341,7 @@ class BaseScreenshot:
window_size = window_size or self.window_size
thumb_size = thumb_size or self.thumb_size
logger.info("Processing url for thumbnail: %s", cache_key)
+ cache_payload.set_scope(self.cache_scope)
cache_payload.computing()
self.cache.set(cache_key, cache_payload.to_dict())
image = None
diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py
index 52857d5f572..560fab4ce26 100644
--- a/tests/integration_tests/dashboards/api_tests.py
+++ b/tests/integration_tests/dashboards/api_tests.py
@@ -31,6 +31,7 @@ import yaml
from freezegun import freeze_time
from sqlalchemy import and_
from superset import db, security_manager # noqa: F401
+from superset.commands.dashboard.permalink.create import CreateDashboardPermalinkCommand
from superset.exceptions import LockAlreadyHeldException
from superset.models.dashboard import Dashboard
from superset.models.core import FavStar, FavStarClassName
@@ -4086,9 +4087,47 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
.filter(Dashboard.dashboard_title == "dash with tag")
.first()
)
- response = self._cache_screenshot(dashboard.id, {"permalinkKey": "1234"})
+ # A permalink key must resolve (and access-check) to `dashboard.id` --
+ # an arbitrary/unresolvable key is now rejected rather than accepted
+ # verbatim.
+ permalink_key = CreateDashboardPermalinkCommand(
+ dashboard_id=str(dashboard.id),
+ state={"dataMask": {}, "activeTabs": [], "anchor": "", "urlParams": []},
+ ).run()
+ response = self._cache_screenshot(dashboard.id, {"permalinkKey": permalink_key})
assert response.status_code == 202
+ @with_feature_flags(THUMBNAILS=True, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
+ @pytest.mark.usefixtures("create_dashboard_with_tag")
+ def test_cache_dashboard_screenshot_rejects_unresolvable_permalink(self):
+ self.login(ADMIN_USERNAME)
+ dashboard = (
+ db.session.query(Dashboard)
+ .filter(Dashboard.dashboard_title == "dash with tag")
+ .first()
+ )
+ response = self._cache_screenshot(dashboard.id, {"permalinkKey": "1234"})
+ assert response.status_code == 404
+
+ @with_feature_flags(THUMBNAILS=True, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
+ def test_cache_dashboard_screenshot_rejects_permalink_for_other_dashboard(self):
+ """
+ A permalink key that resolves to a *different* dashboard than `pk` must
+ be rejected -- otherwise a caller with access to `pk` could pass a
+ permalink key for a dashboard they don't have access to and have it
+ rendered by the (potentially more-privileged) screenshot executor.
+ """
+ self.login(ADMIN_USERNAME)
+ dashboard_a_id, dashboard_b_id = get_dashboards_ids(["world_health", "births"])
+ permalink_key = CreateDashboardPermalinkCommand(
+ dashboard_id=str(dashboard_b_id),
+ state={"dataMask": {}, "activeTabs": [], "anchor": "", "urlParams": []},
+ ).run()
+ response = self._cache_screenshot(
+ dashboard_a_id, {"permalinkKey": permalink_key}
+ )
+ assert response.status_code == 403
+
@with_feature_flags(THUMBNAILS=True, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
@pytest.mark.usefixtures("create_dashboard_with_tag")
def test_cache_dashboard_screenshot_dashboard_validation(self):
@@ -4124,15 +4163,15 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
"""
self.login(ADMIN_USERNAME)
mock_cache_task.return_value = None
- mock_get_from_cache_key.return_value = ScreenshotCachePayload(
- b"fake image data"
- )
dashboard = (
db.session.query(Dashboard)
.filter(Dashboard.dashboard_title == "dash with tag")
.first()
)
+ mock_get_from_cache_key.return_value = ScreenshotCachePayload(
+ b"fake image data", scope=f"dashboard:{dashboard.id}"
+ )
cache_resp = self._cache_screenshot(dashboard.id)
assert cache_resp.status_code == 200
cache_key = json.loads(cache_resp.data.decode("utf-8"))["cache_key"]
@@ -4162,9 +4201,6 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
"""
self.login(ADMIN_USERNAME)
mock_cache_task.return_value = None
- mock_get_from_cache_key.return_value = ScreenshotCachePayload(
- b"fake image data"
- )
mock_build_pdf.return_value = b"fake pdf data"
dashboard = (
@@ -4172,6 +4208,9 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
.filter(Dashboard.dashboard_title == "dash with tag")
.first()
)
+ mock_get_from_cache_key.return_value = ScreenshotCachePayload(
+ b"fake image data", scope=f"dashboard:{dashboard.id}"
+ )
cache_resp = self._cache_screenshot(dashboard.id)
assert cache_resp.status_code == 200
cache_key = json.loads(cache_resp.data.decode("utf-8"))["cache_key"]
@@ -4222,13 +4261,15 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
):
self.login(ADMIN_USERNAME)
mock_cache_task.return_value = None
- mock_get_from_cache_key.return_value = ScreenshotCachePayload(b"fake png data")
dashboard = (
db.session.query(Dashboard)
.filter(Dashboard.dashboard_title == "dash with tag")
.first()
)
+ mock_get_from_cache_key.return_value = ScreenshotCachePayload(
+ b"fake png data", scope=f"dashboard:{dashboard.id}"
+ )
cache_resp = self._cache_screenshot(dashboard.id)
assert cache_resp.status_code == 200
@@ -4254,9 +4295,6 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
"""
self.login(ADMIN_USERNAME)
mock_cache_task.return_value = None
- mock_get_from_cache_key.return_value = ScreenshotCachePayload(
- b"fake image data"
- )
mock_build_pdf.return_value = b"fake pdf data"
dashboard = (
@@ -4264,6 +4302,9 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
.filter(Dashboard.dashboard_title == "dash with tag")
.first()
)
+ mock_get_from_cache_key.return_value = ScreenshotCachePayload(
+ b"fake image data", scope=f"dashboard:{dashboard.id}"
+ )
cache_resp = self._cache_screenshot(dashboard.id)
assert cache_resp.status_code == 200
@@ -4292,14 +4333,14 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
"""
self.login(ADMIN_USERNAME)
mock_cache_task.return_value = None
- mock_get_from_cache_key.return_value = ScreenshotCachePayload(
- b"fake image data"
- )
mock_build_pdf.return_value = b"fake pdf data"
dashboard = Dashboard()
db.session.add(dashboard)
db.session.commit()
+ mock_get_from_cache_key.return_value = ScreenshotCachePayload(
+ b"fake image data", scope=f"dashboard:{dashboard.id}"
+ )
cache_resp = self._cache_screenshot(dashboard.id)
assert cache_resp.status_code == 200
diff --git a/tests/unit_tests/utils/screenshot_test.py b/tests/unit_tests/utils/screenshot_test.py
index d6a26b21625..8505579feb4 100644
--- a/tests/unit_tests/utils/screenshot_test.py
+++ b/tests/unit_tests/utils/screenshot_test.py
@@ -129,6 +129,24 @@ class TestComputeAndCache:
cache_payload: ScreenshotCachePayloadType = screenshot_obj.cache.get("key")
assert cache_payload["status"] == "Updated"
+ def test_stamps_cache_scope_when_set(self, mocker: MockerFixture, screenshot_obj):
+ """A caller (the thumbnail Celery tasks) sets `cache_scope` before
+ calling compute_and_cache so the persisted entry can later be checked
+ against the object a caller-supplied digest is being used to access."""
+ self._setup_compute_and_cache(mocker, screenshot_obj)
+ screenshot_obj.cache_scope = "dashboard:5"
+ screenshot_obj.compute_and_cache(force=False)
+ cache_payload: ScreenshotCachePayloadType = screenshot_obj.cache.get("key")
+ assert cache_payload["scope"] == "dashboard:5"
+
+ def test_scope_unset_when_caller_never_sets_it(
+ self, mocker: MockerFixture, screenshot_obj
+ ):
+ self._setup_compute_and_cache(mocker, screenshot_obj)
+ screenshot_obj.compute_and_cache(force=False)
+ cache_payload: ScreenshotCachePayloadType = screenshot_obj.cache.get("key")
+ assert cache_payload["scope"] is None
+
def test_passes_cache_key_log_context_to_capture(
self, mocker: MockerFixture, screenshot_obj
):
@@ -267,6 +285,42 @@ class TestScreenshotCachePayloadGetImage:
assert result1 is not result2
+class TestScreenshotCachePayloadScope:
+ """
+ Cache entries are shared across every dashboard and chart in the same
+ cache backend. `scope` records which object (e.g. "dashboard:5") an
+ entry was actually rendered for, so a caller-supplied digest/cache_key
+ can be checked against the object it's being used to authorize before
+ serving the image.
+ """
+
+ def test_scope_defaults_to_none(self):
+ payload = ScreenshotCachePayload(image=b"data")
+ assert payload.get_scope() is None
+
+ def test_set_scope(self):
+ payload = ScreenshotCachePayload(image=b"data")
+ payload.set_scope("dashboard:5")
+ assert payload.get_scope() == "dashboard:5"
+
+ def test_scope_round_trips_through_to_dict_from_dict(self):
+ payload = ScreenshotCachePayload(image=b"data", scope="dashboard:5")
+ restored = ScreenshotCachePayload.from_dict(payload.to_dict())
+ assert restored.get_scope() == "dashboard:5"
+
+ def test_legacy_dict_without_scope_key_tolerated(self):
+ """A cache entry written before `scope` existed has no "scope" key at
+ all -- from_dict must not raise and must report no scope rather than
+ matching any caller-supplied scope."""
+ legacy_dict: ScreenshotCachePayloadType = {
+ "image": None,
+ "timestamp": "2024-01-01T00:00:00",
+ "status": "Updated",
+ } # type: ignore[typeddict-item]
+ restored = ScreenshotCachePayload.from_dict(legacy_dict)
+ assert restored.get_scope() is None
+
+
class TestBaseScreenshotDriverFallback:
"""Test BaseScreenshot.driver() fallback logic for Playwright migration."""