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 19:34:45 UTC
(superset) branch master updated: fix: tighten SSRF validation, executor resolution, and cache scoping across reports/thumbnails (#43395)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 2b6044b624a fix: tighten SSRF validation, executor resolution, and cache scoping across reports/thumbnails (#43395)
2b6044b624a is described below
commit 2b6044b624a95b4894ee574bd6adea036bb368b3
Author: Evan Rusackas <ev...@preset.io>
AuthorDate: Fri Aug 21 12:34:37 2026 -0700
fix: tighten SSRF validation, executor resolution, and cache scoping across reports/thumbnails (#43395)
Co-authored-by: Superset Dev <de...@superset.apache.org>
Co-authored-by: Claude Sonnet 5 <no...@anthropic.com>
---
superset-websocket/spec/index.test.ts | 36 ++++
superset-websocket/src/index.ts | 14 +-
superset/charts/api.py | 8 +-
superset/commands/report/create.py | 2 +-
superset/daos/report.py | 16 +-
superset/dashboards/api.py | 61 ++++++-
superset/reports/notifications/webhook.py | 140 ++++++++++++++-
superset/tasks/thumbnails.py | 3 +
superset/tasks/types.py | 7 +-
superset/tasks/utils.py | 95 ++--------
superset/thumbnails/digest.py | 13 +-
superset/utils/screenshots.py | 51 +++++-
tests/integration_tests/dashboards/api_tests.py | 79 ++++++--
tests/integration_tests/reports/alert_tests.py | 6 +-
tests/integration_tests/reports/utils.py | 11 +-
tests/unit_tests/daos/test_report_dao.py | 57 +++++-
.../reports/notifications/webhook_tests.py | 199 +++++++++++++++++++++
tests/unit_tests/tasks/test_utils.py | 97 +++++-----
tests/unit_tests/thumbnails/test_digest.py | 88 ++++++++-
tests/unit_tests/utils/screenshot_test.py | 107 +++++++++++
20 files changed, 917 insertions(+), 173 deletions(-)
diff --git a/superset-websocket/spec/index.test.ts b/superset-websocket/spec/index.test.ts
index acae9981700..13e36ad31a6 100644
--- a/superset-websocket/spec/index.test.ts
+++ b/superset-websocket/spec/index.test.ts
@@ -625,6 +625,42 @@ describe('server', () => {
);
expect(wsEventMock).toHaveBeenCalledWith('pong', expect.any(Function));
});
+
+ test('unsolicited pong payload cannot pollute Object.prototype', async () => {
+ const validToken = jwt.sign({ channel: channelId }, config.jwtSecret);
+ const request = getRequest(validToken, 'http://localhost');
+
+ server.wsConnection(ws, request);
+
+ // Extract the handler registered for the 'pong' event, the same way
+ // the underlying `ws` library would invoke it on a raw pong frame.
+ const pongCall = wsEventMock.mock.calls.find(call => call[0] === 'pong');
+ expect(pongCall).toBeDefined();
+ const pongHandler = pongCall![1] as (data: Buffer) => void;
+
+ // An unsolicited pong with a payload matching an inherited key must not
+ // resolve through the prototype chain and must not write through to
+ // Object.prototype.
+ pongHandler(Buffer.from('__proto__'));
+ pongHandler(Buffer.from('constructor'));
+ pongHandler(Buffer.from('hasOwnProperty'));
+
+ // eslint-disable-next-line no-prototype-builtins
+ expect(Object.prototype.hasOwnProperty('pongTs')).toBe(false);
+ expect(({} as Record<string, unknown>).pongTs).toBeUndefined();
+
+ // A genuine socket id must still record its pong normally.
+ const socketId = server.channels[channelId].sockets[0];
+ const beforePongTs = server.sockets[socketId].pongTs;
+ dateNowSpy.mockImplementation(() =>
+ new Date('2021-03-10T11:02:58.135Z').valueOf(),
+ );
+ pongHandler(Buffer.from(socketId));
+ expect(server.sockets[socketId].pongTs).not.toBe(beforePongTs);
+ expect(server.sockets[socketId].pongTs).toBe(
+ new Date('2021-03-10T11:02:58.135Z').valueOf(),
+ );
+ });
});
describe('connection limits', () => {
diff --git a/superset-websocket/src/index.ts b/superset-websocket/src/index.ts
index 9177f9d8dbd..4e206b5141d 100644
--- a/superset-websocket/src/index.ts
+++ b/superset-websocket/src/index.ts
@@ -475,12 +475,18 @@ export const wsConnection = (ws: WebSocket, request: http.IncomingMessage) => {
// init event handler for `pong` events (connection management)
ws.on('pong', function pong(data: Buffer) {
const socketId = data.toString();
- const socketInstance = sockets[socketId];
- if (!socketInstance) {
+ // `sockets` is a plain object, so an unsolicited pong carrying an
+ // inherited key ('__proto__', 'constructor', 'hasOwnProperty', ...) as
+ // its payload would otherwise resolve through the prototype chain
+ // instead of missing outright, letting a client write an enumerable
+ // `pongTs` onto Object.prototype (tripped over by the for...in loops in
+ // checkSockets/cleanChannel on every GC pass). Guarding with an
+ // own-property check rejects every such key in one place.
+ if (!Object.prototype.hasOwnProperty.call(sockets, socketId)) {
logger.warn(`pong received for nonexistent socket ${socketId}`);
- } else {
- socketInstance.pongTs = Date.now();
+ return;
}
+ sockets[socketId].pongTs = Date.now();
});
};
diff --git a/superset/charts/api.py b/superset/charts/api.py
index 1e3626bd77d..5636cc50012 100644
--- a/superset/charts/api.py
+++ b/superset/charts/api.py
@@ -1050,7 +1050,7 @@ class ChartRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi):
task_status=cache_payload.get_status(),
)
- if cache_payload.should_trigger_task(force):
+ if cache_payload.should_trigger_task(force, expected_scope=f"chart:{chart.id}"):
logger.info("Triggering screenshot ASYNC")
screenshot_obj.cache.set(cache_key, ScreenshotCachePayload().to_dict())
cache_chart_thumbnail.delay(
@@ -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/commands/report/create.py b/superset/commands/report/create.py
index eaaebebac29..b2c84031909 100644
--- a/superset/commands/report/create.py
+++ b/superset/commands/report/create.py
@@ -154,7 +154,7 @@ class CreateReportScheduleCommand(CreateMixin, BaseReportScheduleCommand):
if (
creation_method != ReportCreationMethod.ALERTS_REPORTS
and not ReportScheduleDAO.validate_unique_creation_method(
- dashboard_id, chart_id
+ dashboard_id, chart_id, creation_method
)
):
raise ReportScheduleCreationMethodUniquenessValidationError()
diff --git a/superset/daos/report.py b/superset/daos/report.py
index 28f999a009f..34643ff6ef3 100644
--- a/superset/daos/report.py
+++ b/superset/daos/report.py
@@ -180,17 +180,27 @@ class ReportScheduleDAO(BaseDAO[ReportSchedule]):
@staticmethod
def validate_unique_creation_method(
- dashboard_id: int | None = None, chart_id: int | None = None
+ dashboard_id: int | None = None,
+ chart_id: int | None = None,
+ creation_method: str | None = None,
) -> bool:
"""
- Validate if the user already has a chart or dashboard
- with a report attached form the self subscribe reports
+ Validate if the user already has a chart or dashboard with a report
+ attached that was created via the same creation method as the one
+ being validated. Only reports created through the same method (e.g.
+ two "charts"-sourced reports) compete for the one-per-object slot --
+ an unrelated self-subscribed alert/report (creation method
+ "alerts_reports") on the same chart or dashboard doesn't count
+ against it.
"""
query = db.session.query(ReportSchedule).filter_by(created_by_fk=get_user_id())
if dashboard_id is not None:
query = query.filter(ReportSchedule.dashboard_id == dashboard_id)
+ if creation_method is not None:
+ query = query.filter(ReportSchedule.creation_method == creation_method)
+
if chart_id is not None:
query = query.filter(ReportSchedule.chart_id == chart_id)
diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py
index cbde7183106..32578c49805 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)
@@ -1940,7 +1983,9 @@ class DashboardRestApi(
task_status=cache_payload.get_status(),
)
- if cache_payload.should_trigger_task(force):
+ if cache_payload.should_trigger_task(
+ force, expected_scope=f"dashboard:{dashboard.id}"
+ ):
logger.info("Triggering screenshot ASYNC")
cache_dashboard_screenshot.delay(
username=get_current_user(),
@@ -2019,6 +2064,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/reports/notifications/webhook.py b/superset/reports/notifications/webhook.py
index b8fc3f57930..52e5183797d 100644
--- a/superset/reports/notifications/webhook.py
+++ b/superset/reports/notifications/webhook.py
@@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
+import ipaddress
import logging
from typing import Any
from urllib.parse import urlparse
@@ -22,6 +23,9 @@ from urllib.parse import urlparse
import backoff
import requests
from flask import current_app
+from requests.adapters import HTTPAdapter
+from urllib3.connection import HTTPConnection, HTTPSConnection
+from urllib3.connectionpool import HTTPConnectionPool, HTTPSConnectionPool
from superset import feature_flag_manager
from superset.reports.models import ReportRecipientType
@@ -32,10 +36,116 @@ from superset.reports.notifications.exceptions import (
)
from superset.utils import json
from superset.utils.decorators import statsd_gauge
-from superset.utils.network import is_safe_host
+from superset.utils.network import is_safe_host, is_safe_ip
logger = logging.getLogger(__name__)
+# Number of characters of a failing response body kept in the server-side log
+# line. Response bodies are never folded into the exception message raised
+# back to the caller -- that message is persisted verbatim as
+# ``ReportExecutionLog.error_message`` and readable via the execution log
+# API, which would otherwise turn the webhook target into a readback oracle
+# for whatever it chooses to return (including an internal host reached via
+# DNS rebinding).
+_LOGGED_RESPONSE_BODY_LIMIT = 500
+
+
+def _sanitize_for_log(text: str) -> str:
+ """
+ Escape newlines and other control characters in text that gets embedded
+ in a log line. The webhook target controls the response body verbatim,
+ so logging it unescaped would let it forge additional log records or
+ corrupt line-oriented log ingestion.
+ """
+ return text.translate(
+ {c: f"\\x{c:02x}" for c in [*range(0x20), 0x7F] if c not in (0x09,)}
+ )
+
+
+def _raise_for_unsafe_peer(conn: HTTPConnection) -> None:
+ """
+ Validate that a connection's actual peer is publicly routable.
+
+ ``_validate_webhook_url`` resolves and checks the hostname once, ahead of
+ time; the connection opened here is resolved independently and may reach
+ a different address (DNS rebinding via a low-TTL record), so the check
+ has to be repeated against the address actually connected to.
+ """
+ sock = conn.sock
+ if sock is None:
+ return
+ peer = sock.getpeername()[0]
+ if not is_safe_ip(ipaddress.ip_address(peer)):
+ raise NotificationParamException("Webhook URL target host is not allowed.")
+
+
+class _PeerValidatingHTTPConnection(HTTPConnection):
+ """HTTP connection that validates the peer address on connect."""
+
+ def connect(self) -> None:
+ super().connect()
+ _raise_for_unsafe_peer(self)
+
+
+class _PeerValidatingHTTPSConnection(HTTPSConnection):
+ """HTTPS connection that validates the peer address after the handshake."""
+
+ def connect(self) -> None:
+ super().connect()
+ _raise_for_unsafe_peer(self)
+
+
+class _PeerValidatingHTTPConnectionPool(HTTPConnectionPool):
+ ConnectionCls = _PeerValidatingHTTPConnection
+
+
+class _PeerValidatingHTTPSConnectionPool(HTTPSConnectionPool):
+ ConnectionCls = _PeerValidatingHTTPSConnection
+
+
+class _PeerValidatingHTTPAdapter(HTTPAdapter):
+ """
+ Transport adapter that routes requests through connection classes which
+ validate the connected peer address, closing the TOCTOU window between
+ the hostname check in ``_validate_webhook_url`` and the connection that
+ ``send()`` actually opens.
+
+ Mirrors the peer-validation approach used for dataset-import data URIs
+ (``superset.commands.dataset.importers.v1.utils``), adapted to
+ ``requests``/``urllib3`` connection pooling instead of ``urllib``.
+ """
+
+ def init_poolmanager(self, *args: Any, **kwargs: Any) -> None:
+ super().init_poolmanager(*args, **kwargs)
+ # Assign a new dict rather than mutating the manager's dict in
+ # place -- the attribute otherwise aliases urllib3's module-global
+ # default scheme-to-pool-class mapping.
+ self.poolmanager.pool_classes_by_scheme = {
+ "http": _PeerValidatingHTTPConnectionPool,
+ "https": _PeerValidatingHTTPSConnectionPool,
+ }
+
+
+def _get_requester() -> Any:
+ """
+ Return the object used to issue the webhook POST -- either the
+ ``requests`` module itself or a ``requests.Session``, both of which
+ expose a compatible ``.post(url, ...)``.
+
+ Operators who explicitly opt into internal webhook targets via
+ ``ALERT_REPORTS_WEBHOOK_ALLOW_INTERNAL_HOSTS`` get the plain ``requests``
+ module (no peer pinning -- internal hosts are the intended destination).
+ Otherwise a session is returned whose transport pins the connection to
+ the address that was actually validated.
+ """
+ if current_app.config["ALERT_REPORTS_WEBHOOK_ALLOW_INTERNAL_HOSTS"]:
+ return requests
+ session = requests.Session()
+ adapter = _PeerValidatingHTTPAdapter()
+ session.mount("http://", adapter)
+ session.mount("https://", adapter)
+ return session
+
class WebhookNotification(BaseNotification):
"""
@@ -174,6 +284,7 @@ class WebhookNotification(BaseNotification):
payload = self._get_req_payload()
files = self._get_files()
timeout = current_app.config["ALERT_REPORTS_WEBHOOK_TIMEOUT"]
+ requester = _get_requester()
try:
if files:
@@ -184,7 +295,7 @@ class WebhookNotification(BaseNotification):
else:
data[key] = value
- response = requests.post(
+ response = requester.post(
wh_url,
data=data,
files=files,
@@ -192,7 +303,7 @@ class WebhookNotification(BaseNotification):
allow_redirects=False,
)
else:
- response = requests.post(
+ response = requester.post(
wh_url, json=payload, timeout=timeout, allow_redirects=False
)
@@ -201,14 +312,29 @@ class WebhookNotification(BaseNotification):
)
if response.status_code >= 500 or response.status_code == 429:
+ # The response body is logged server-side only (and
+ # truncated) -- it must not be folded into the exception
+ # message, which is persisted as the report execution log's
+ # error message and surfaced back to whoever can read that
+ # log, turning the webhook target into a readback oracle.
+ logger.warning(
+ "Webhook to %s failed with status code %s: %s",
+ wh_url,
+ response.status_code,
+ _sanitize_for_log(response.text[:_LOGGED_RESPONSE_BODY_LIMIT]),
+ )
raise NotificationUnprocessableException(
- f"Webhook failed with status code {response.status_code}: \
- {response.text}"
+ f"Webhook failed with status code {response.status_code}"
)
if response.status_code >= 400:
+ logger.warning(
+ "Webhook to %s failed with status code %s: %s",
+ wh_url,
+ response.status_code,
+ _sanitize_for_log(response.text[:_LOGGED_RESPONSE_BODY_LIMIT]),
+ )
raise NotificationParamException(
- f"Webhook failed with status code {response.status_code}: \
- {response.text}"
+ f"Webhook failed with status code {response.status_code}"
)
if response.status_code >= 300:
# Redirects are intentionally not followed (allow_redirects=False),
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/tasks/types.py b/superset/tasks/types.py
index d2cd488067a..e35d6566795 100644
--- a/superset/tasks/types.py
+++ b/superset/tasks/types.py
@@ -48,9 +48,10 @@ class ExecutorType(StrEnum):
# The last modifier of the model, if they are an editor directly (user-type subject)
# or indirectly (their role or group is an editor subject)
MODIFIER_EDITOR = "modifier_editor"
- # An editor of the model. Resolves to a user who is an editor either directly
- # or through role/group membership. Prioritizes: modifier -> creator -> first
- # direct user-type editor -> deterministic user from role/group editors.
+ # Whoever authored the model's current state, provided they are also an editor
+ # (directly, or through role/group membership). Prioritizes: modifier -> creator.
+ # Never resolves to any other attached editor -- only the user who actually wrote
+ # the state being executed.
EDITOR = "editor"
diff --git a/superset/tasks/utils.py b/superset/tasks/utils.py
index 6ffefed7933..611eceab9c4 100644
--- a/superset/tasks/utils.py
+++ b/superset/tasks/utils.py
@@ -20,13 +20,12 @@ from __future__ import annotations
import logging
import traceback
from http.client import HTTPResponse
-from typing import Any, cast, TYPE_CHECKING
+from typing import cast, TYPE_CHECKING
from urllib import request
from uuid import UUID, uuid4
from celery.utils.log import get_task_logger
from flask import g
-from sqlalchemy import or_, select
from superset_core.tasks.types import TaskProperties, TaskScope
from superset.tasks.exceptions import ExecutorNotFoundError, InvalidExecutorError
@@ -41,8 +40,6 @@ from superset.utils.hashing import hash_from_str
from superset.utils.urls import get_url_path
if TYPE_CHECKING:
- from flask_appbuilder.security.sqla.models import User
-
from superset.models.dashboard import Dashboard
from superset.models.slice import Slice
from superset.reports.models import ReportSchedule
@@ -52,55 +49,6 @@ logger = get_task_logger(__name__)
logger.setLevel(logging.INFO)
-def _get_indirect_editor_user(editors: list[Any]) -> User | None:
- """Return a deterministic user represented by role/group editor subjects."""
- from flask_appbuilder.security.sqla.models import (
- assoc_user_group,
- assoc_user_role,
- User,
- )
-
- from superset import db
- from superset.subjects.types import SubjectType
-
- role_ids = [
- editor.role_id
- for editor in editors
- if editor.type == SubjectType.ROLE and editor.role_id
- ]
- group_ids = [
- editor.group_id
- for editor in editors
- if editor.type == SubjectType.GROUP and editor.group_id
- ]
- conditions = []
- if role_ids:
- conditions.append(
- User.id.in_(
- select(assoc_user_role.c.user_id).where(
- assoc_user_role.c.role_id.in_(role_ids)
- )
- )
- )
- if group_ids:
- conditions.append(
- User.id.in_(
- select(assoc_user_group.c.user_id).where(
- assoc_user_group.c.group_id.in_(group_ids)
- )
- )
- )
- if not conditions:
- return None
-
- return (
- db.session.query(User)
- .filter(User.active.is_(True), or_(*conditions))
- .order_by(User.id)
- .first()
- )
-
-
# pylint: disable=too-many-branches
def get_executor( # noqa: C901
executors: list[Executor],
@@ -112,9 +60,14 @@ def get_executor( # noqa: C901
types extract the user from the underlying object (e.g. CREATOR), a fixed user
account, or the user that initiated the request.
- The EDITOR, CREATOR_EDITOR, and MODIFIER_EDITOR types resolve users from the model's
- editors (subjects). These check both direct user-type subjects and indirect
- membership through role/group subjects.
+ The CREATOR_EDITOR, MODIFIER_EDITOR, and EDITOR types additionally require the
+ resolved user (the model's creator or modifier) to be an editor of the model,
+ directly (user-type subject) or indirectly (through a role/group subject). They
+ never resolve to any *other* attached editor: editor subjects can be attached to
+ a model by whoever creates or edits it with no consent from the attached user, so
+ resolving to an arbitrary attached editor would let a low-privileged creator or
+ modifier arrange for the task to execute as a different, potentially
+ higher-privileged, user.
:param executors: The requested executor in descending order. When the
first user is found it is returned.
@@ -128,7 +81,6 @@ def get_executor( # noqa: C901
:raises ExecutorNotFoundError: If no users were found in after
iterating through all entries in `executors`
"""
- from superset.subjects.types import SubjectType
from superset.subjects.utils import get_user_subject_ids
# Build set of all subject IDs that are editors of this model
@@ -140,13 +92,6 @@ def get_executor( # noqa: C901
return False
return bool(set(get_user_subject_ids(user_id)) & editor_subject_ids)
- # Direct user-type editors (for EDITOR fallback resolution)
- editor_users = [
- e.user
- for e in getattr(model, "editors", [])
- if e.type == SubjectType.USER and e.user is not None
- ]
-
for executor in executors:
if isinstance(executor, FixedExecutor):
return ExecutorType.FIXED_USER, executor.username
@@ -167,11 +112,15 @@ def get_executor( # noqa: C901
if (user := model.changed_by) and user.is_active:
return executor, user.username
if executor == ExecutorType.EDITOR:
- # Priority: modifier → creator → direct user editor → indirect editor.
- # Inactive users are skipped at every step so that scheduling can
- # fall through to another active owner/editor instead of failing
- # outright (see: ExecutorNotFoundError only once no active
- # candidate remains).
+ # Priority: modifier -> creator. Resolves only to whoever authored
+ # the model's current state -- changed_by/created_by are set by the
+ # framework from the authenticated session on write, so a caller
+ # who edits the object becomes changed_by themselves and cannot
+ # point this at a victim. Deliberately does NOT fall through to an
+ # arbitrary other attached editor (direct or via role/group
+ # membership): that would let a low-privileged creator/modifier
+ # attach a higher-privileged user as an editor with no consent and
+ # have the task execute with that victim's credentials.
if (
(modifier := model.changed_by)
and modifier.is_active
@@ -184,14 +133,6 @@ def get_executor( # noqa: C901
and _is_editor(creator.id)
):
return executor, creator.username
- if active_editor_user := next(
- (user for user in editor_users if user.is_active), None
- ):
- return executor, active_editor_user.username
- if indirect_editor := _get_indirect_editor_user(
- getattr(model, "editors", [])
- ):
- return executor, indirect_editor.username
raise ExecutorNotFoundError()
diff --git a/superset/thumbnails/digest.py b/superset/thumbnails/digest.py
index 9a18dc64fd6..5cea4c9ddd5 100644
--- a/superset/thumbnails/digest.py
+++ b/superset/thumbnails/digest.py
@@ -62,9 +62,18 @@ def _adjust_string_with_rls(
Add the RLS filters to the unique string based on current executor.
"""
+ # Prefer the ambient guest user (the actual requesting principal) over a
+ # DB-user lookup by username: for guest requests `executor` is the
+ # token-supplied username, which can collide with a real DB username. If
+ # find_user() were tried first, a collision would compute RLS under the
+ # unrelated DB user's identity, and the token's own per-token rls claims
+ # (surfaced via get_guest_rls_filters(), which reads the ambient guest
+ # user installed by override_user() below) would never enter the digest --
+ # letting two guest tokens with the same username but different rls
+ # collide on one cache entry.
user = (
- security_manager.find_user(executor)
- or security_manager.get_current_guest_user_if_guest()
+ security_manager.get_current_guest_user_if_guest()
+ or security_manager.find_user(executor)
)
if user:
diff --git a/superset/utils/screenshots.py b/superset/utils/screenshots.py
index 4fcd58c9c85..10b41388c22 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()
@@ -177,13 +196,31 @@ class ScreenshotCachePayload:
datetime.now() - datetime.fromisoformat(self.get_timestamp())
).total_seconds() >= computing_ttl
- def should_trigger_task(self, force: bool = False) -> bool:
+ def should_trigger_task(
+ self, force: bool = False, expected_scope: str | None = None
+ ) -> bool:
+ """
+ :param expected_scope: The scope (e.g. "dashboard:<id>") the caller
+ requires this entry to carry. Entries written before scope
+ tracking existed -- or by a stale/mismatched caller -- deserialize
+ with no scope (or a different one) and are otherwise
+ indistinguishable from a fresh, valid ``UPDATED`` entry, which
+ would leave them permanently un-refreshed: the scope check at
+ read time rejects them, but nothing ever re-triggers computation.
+ Treat a scope mismatch on an ``UPDATED`` entry as a cache miss so
+ it gets recomputed and re-scoped.
+ """
return (
force
or self.status == StatusValues.PENDING
or (self.status == StatusValues.ERROR and self.is_error_cache_ttl_expired())
or (self.status == StatusValues.COMPUTING and self.is_computing_stale())
or (self.status == StatusValues.UPDATED and self._image is None)
+ or (
+ self.status == StatusValues.UPDATED
+ and expected_scope is not None
+ and self._scope != expected_scope
+ )
)
@@ -196,6 +233,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
@@ -305,7 +349,9 @@ class BaseScreenshot:
cache_payload = (
self.get_from_cache_key(cache_key) or ScreenshotCachePayload()
)
- if not cache_payload.should_trigger_task(force=force):
+ if not cache_payload.should_trigger_task(
+ force=force, expected_scope=self.cache_scope
+ ):
logger.info(
"Skipping compute - already processed for thumbnail: %s",
cache_key,
@@ -315,6 +361,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..3432571b187 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,51 @@ 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)
+ @pytest.mark.usefixtures(
+ "load_world_bank_dashboard_with_slices",
+ "load_birth_names_dashboard_with_slices",
+ )
+ 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 +4167,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 +4205,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 +4212,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 +4265,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
@@ -4238,6 +4283,12 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
cache_resp = self._cache_screenshot(dashboard.id)
assert cache_resp.status_code == 202
+ # Restore a valid, correctly-scoped payload so the request below
+ # actually reaches the download_format validation instead of
+ # failing the earlier cache-scope check.
+ mock_get_from_cache_key.return_value = ScreenshotCachePayload(
+ b"fake png data", scope=f"dashboard:{dashboard.id}"
+ )
response = self._get_screenshot(dashboard.id, cache_key, "invalid")
assert response.status_code == 404
@@ -4254,9 +4305,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 +4312,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 +4343,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/integration_tests/reports/alert_tests.py b/tests/integration_tests/reports/alert_tests.py
index bb8c163e615..b16dfa6b15b 100644
--- a/tests/integration_tests/reports/alert_tests.py
+++ b/tests/integration_tests/reports/alert_tests.py
@@ -51,7 +51,11 @@ def _get_user_subjects(users):
"editor_names,creator_name,config,expected_result",
[
(["gamma"], None, [FixedExecutor("admin")], "admin"),
- (["gamma"], None, [ExecutorType.EDITOR], "gamma"),
+ # EDITOR only resolves to whoever authored the model's current state
+ # (changed_by, then created_by); a bare attached editor with no
+ # authorship on the model is not enough, so gamma must be the creator
+ # here for EDITOR to resolve to them.
+ (["gamma"], "gamma", [ExecutorType.EDITOR], "gamma"),
(
["alpha", "gamma"],
"gamma",
diff --git a/tests/integration_tests/reports/utils.py b/tests/integration_tests/reports/utils.py
index 44b739e132c..900f38a3378 100644
--- a/tests/integration_tests/reports/utils.py
+++ b/tests/integration_tests/reports/utils.py
@@ -97,6 +97,13 @@ def insert_report_schedule(
logs = logs or []
last_state = last_state or ReportState.NOOP
+ # created_by/changed_by are populated by AuditMixin's column defaults, which
+ # are only evaluated when the row is actually flushed. The override_user
+ # context therefore has to remain active through add()/commit() (not just
+ # construction) so the flush sees the intended editor as the ambient user --
+ # otherwise the row ends up authored by whoever (if anyone) is ambiently
+ # logged in, and EDITOR executor resolution -- which now only trusts
+ # changed_by/created_by -- can't resolve it back to an attached editor.
with override_user(editor_users[0] if editor_users else None):
report_schedule = ReportSchedule(
type=type,
@@ -125,8 +132,8 @@ def insert_report_schedule(
retry_notify_owners=retry_notify_owners,
retry_notify_recipients=retry_notify_recipients,
)
- db.session.add(report_schedule)
- db.session.commit()
+ db.session.add(report_schedule)
+ db.session.commit()
return report_schedule
diff --git a/tests/unit_tests/daos/test_report_dao.py b/tests/unit_tests/daos/test_report_dao.py
index 3fa883ce00e..3d00859e2b8 100644
--- a/tests/unit_tests/daos/test_report_dao.py
+++ b/tests/unit_tests/daos/test_report_dao.py
@@ -16,11 +16,17 @@
# under the License.
from __future__ import annotations
+from unittest.mock import patch
+
import pytest
from sqlalchemy.orm.session import Session
from superset.daos.report import ReportScheduleDAO
-from superset.reports.models import ReportSchedule, ReportScheduleType
+from superset.reports.models import (
+ ReportCreationMethod,
+ ReportSchedule,
+ ReportScheduleType,
+)
from superset.utils import json
@@ -127,3 +133,52 @@ def test_find_by_native_filter_id_escapes_underscore_wildcard(
assert len(results) == 1
assert results[0].name == "with-underscore"
+
+
+@patch("superset.daos.report.get_user_id", return_value=1)
+def test_validate_unique_creation_method_ignores_other_creation_methods(
+ mock_user_id, session: Session
+) -> None:
+ """A self-subscribed alert (creation method "alerts_reports") attached to
+ a chart doesn't compete with a "charts"-sourced report for that same
+ chart's one-report-per-creation-method slot."""
+ existing = ReportSchedule(
+ name="existing-alert",
+ type=ReportScheduleType.ALERT,
+ crontab="* * * * *",
+ chart_id=1,
+ creation_method=ReportCreationMethod.ALERTS_REPORTS,
+ created_by_fk=1,
+ )
+ session.add(existing)
+ session.flush()
+
+ assert (
+ ReportScheduleDAO.validate_unique_creation_method(
+ chart_id=1, creation_method=ReportCreationMethod.CHARTS
+ )
+ is True
+ )
+
+
+@patch("superset.daos.report.get_user_id", return_value=1)
+def test_validate_unique_creation_method_conflicts_on_same_creation_method(
+ mock_user_id, session: Session
+) -> None:
+ existing = ReportSchedule(
+ name="existing-report",
+ type=ReportScheduleType.REPORT,
+ crontab="* * * * *",
+ chart_id=1,
+ creation_method=ReportCreationMethod.CHARTS,
+ created_by_fk=1,
+ )
+ session.add(existing)
+ session.flush()
+
+ assert (
+ ReportScheduleDAO.validate_unique_creation_method(
+ chart_id=1, creation_method=ReportCreationMethod.CHARTS
+ )
+ is False
+ )
diff --git a/tests/unit_tests/reports/notifications/webhook_tests.py b/tests/unit_tests/reports/notifications/webhook_tests.py
index 89c3ecd6dba..49f2c75c8f3 100644
--- a/tests/unit_tests/reports/notifications/webhook_tests.py
+++ b/tests/unit_tests/reports/notifications/webhook_tests.py
@@ -509,3 +509,202 @@ def test_send_max_time_does_not_abandon_recovering_target(
# would pin elapsed at 0 and pass at any ``max_time``, including a broken
# ``max_time=1`` — this non-zero tick keeps the guard real.
assert len(post_calls) == 3
+
+
+def test_peer_validating_connection_blocks_rebound_peer() -> None:
+ """
+ The webhook POST validates the connected peer address, so a hostname that
+ passes ``is_safe_host`` at validation time and then re-resolves to an
+ internal address by the time the connection is opened (DNS rebinding) is
+ rejected before any request bytes are sent -- mirrors the equivalent test
+ for the dataset-import data-URI fetch path.
+ """
+ from unittest.mock import MagicMock, patch
+
+ from urllib3.connection import HTTPConnection
+
+ from superset.reports.notifications.exceptions import NotificationParamException
+ from superset.reports.notifications.webhook import _PeerValidatingHTTPConnection
+
+ sock = MagicMock()
+ sock.getpeername.return_value = ("169.254.169.254", 80)
+
+ with patch.object(
+ HTTPConnection, "connect", lambda self: setattr(self, "sock", sock)
+ ):
+ conn = _PeerValidatingHTTPConnection("rebinder.example.com")
+ with pytest.raises(NotificationParamException):
+ conn.connect()
+
+
+def test_peer_validating_connection_allows_public_peer() -> None:
+ """A connection whose actual peer resolves to a public address is allowed
+ through unmodified."""
+ from unittest.mock import MagicMock, patch
+
+ from urllib3.connection import HTTPConnection
+
+ from superset.reports.notifications.webhook import _PeerValidatingHTTPConnection
+
+ sock = MagicMock()
+ sock.getpeername.return_value = ("93.184.216.34", 80) # example.com, public
+
+ with patch.object(
+ HTTPConnection, "connect", lambda self: setattr(self, "sock", sock)
+ ):
+ conn = _PeerValidatingHTTPConnection("example.com")
+ conn.connect() # should not raise
+
+
+def test_get_requester_returns_plain_requests_when_internal_hosts_allowed(
+ monkeypatch,
+) -> None:
+ """
+ When the operator opts into internal webhook targets via
+ ``ALERT_REPORTS_WEBHOOK_ALLOW_INTERNAL_HOSTS``, ``_get_requester`` must
+ return the plain ``requests`` module (no peer pinning), preserving the
+ documented escape hatch for internal automation targets.
+ """
+ import requests
+
+ from superset.reports.notifications.webhook import _get_requester
+
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.current_app", _allow_internal_app()
+ )
+
+ assert _get_requester() is requests
+
+
+def test_get_requester_pins_peer_when_internal_hosts_disallowed(monkeypatch) -> None:
+ """
+ By default (``ALERT_REPORTS_WEBHOOK_ALLOW_INTERNAL_HOSTS`` unset/False),
+ ``_get_requester`` returns a session whose adapters validate the
+ connected peer address rather than the plain ``requests`` module.
+ """
+ from superset.reports.notifications.webhook import (
+ _get_requester,
+ _PeerValidatingHTTPAdapter,
+ )
+
+ class MockCurrentApp:
+ config = {"ALERT_REPORTS_WEBHOOK_ALLOW_INTERNAL_HOSTS": False}
+
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.current_app", MockCurrentApp
+ )
+
+ requester = _get_requester()
+ assert isinstance(requester.get_adapter("http://x/"), _PeerValidatingHTTPAdapter)
+ assert isinstance(requester.get_adapter("https://x/"), _PeerValidatingHTTPAdapter)
+
+
+def test_send_rejects_rebound_peer_despite_passed_hostname_check(
+ monkeypatch, mock_header_data
+) -> None:
+ """
+ End-to-end regression for the DNS-rebinding TOCTOU: the hostname check in
+ ``_validate_webhook_url`` runs once, ahead of time, and is simulated here
+ as having passed (as it would for a low-TTL record that resolves publicly
+ at that moment) via monkeypatching ``is_safe_host``. The actual POST
+ still connects to a loopback test server. Before the fix this reaches the
+ server and succeeds; after the fix the connected peer is validated
+ independently and the request is rejected regardless of what the
+ hostname check concluded earlier.
+ """
+ import http.server
+ import threading
+
+ from superset.reports.models import ReportRecipients, ReportRecipientType
+ from superset.reports.notifications.base import NotificationContent
+
+ class Handler(http.server.BaseHTTPRequestHandler):
+ def do_POST(self) -> None: # noqa: N802
+ self.send_response(200)
+ self.end_headers()
+ self.wfile.write(b"ok")
+
+ def log_message(self, *_args: object) -> None:
+ pass
+
+ server = http.server.HTTPServer(("127.0.0.1", 0), Handler)
+ port = server.server_port
+ thread = threading.Thread(target=server.serve_forever, daemon=True)
+ thread.start()
+ try:
+ content = NotificationContent(
+ name="test alert",
+ header_data=mock_header_data,
+ description="Test description",
+ )
+ webhook_notification = WebhookNotification(
+ recipient=ReportRecipients(
+ type=ReportRecipientType.WEBHOOK,
+ recipient_config_json=(f'{{"target": "http://127.0.0.1:{port}/"}}'),
+ ),
+ content=content,
+ )
+
+ class MockCurrentApp:
+ config = {
+ "ALERT_REPORTS_WEBHOOK_HTTPS_ONLY": False,
+ "ALERT_REPORTS_WEBHOOK_ALLOW_INTERNAL_HOSTS": False,
+ "ALERT_REPORTS_WEBHOOK_TIMEOUT": 5,
+ }
+
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.current_app", MockCurrentApp
+ )
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.feature_flag_manager."
+ "is_feature_enabled",
+ lambda flag: True,
+ )
+ # Simulate the hostname check having passed at validation time (a
+ # low-TTL DNS record resolving publicly at that instant).
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.is_safe_host",
+ lambda host: True,
+ )
+
+ with pytest.raises(
+ (NotificationParamException, NotificationUnprocessableException)
+ ):
+ webhook_notification.send()
+ finally:
+ server.shutdown()
+
+
+def test_send_error_message_omits_response_body(monkeypatch, mock_header_data) -> None:
+ """
+ A failing response's body must not be interpolated into the raised
+ exception message: that message is persisted verbatim as the report
+ execution log's error message and readable via the logs API, which would
+ otherwise turn the webhook target into a readback oracle for whatever
+ it returns (e.g. a cloud metadata service reached via DNS rebinding).
+ """
+ webhook_notification = _make_webhook(mock_header_data)
+ leaked_response_content = "internal-metadata-service-response-body"
+
+ class _LeakyResponse:
+ status_code = 502
+ text = leaked_response_content
+
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.current_app", _allow_internal_app()
+ )
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.feature_flag_manager.is_feature_enabled",
+ lambda flag: True,
+ )
+ monkeypatch.setattr(
+ "superset.reports.notifications.webhook.requests.post",
+ lambda *args, **kwargs: _LeakyResponse(),
+ )
+ monkeypatch.setattr("time.sleep", lambda *a, **k: None)
+
+ with pytest.raises(NotificationUnprocessableException) as excinfo:
+ webhook_notification.send()
+
+ assert leaked_response_content not in str(excinfo.value)
+ assert "502" in str(excinfo.value)
diff --git a/tests/unit_tests/tasks/test_utils.py b/tests/unit_tests/tasks/test_utils.py
index 2c423e4f3fd..b4732e4f092 100644
--- a/tests/unit_tests/tasks/test_utils.py
+++ b/tests/unit_tests/tasks/test_utils.py
@@ -127,24 +127,6 @@ class ModelConfig:
user_groups: dict[int, list[int]] = field(default_factory=dict)
-def _get_mock_indirect_editor_user(
- editors: list[MagicMock],
- model_config: ModelConfig,
-) -> User | None:
- editor_role_ids = {
- editor.role_id for editor in editors if getattr(editor, "role_id", None)
- }
- editor_group_ids = {
- editor.group_id for editor in editors if getattr(editor, "group_id", None)
- }
- for user_id in sorted(set(model_config.user_roles) | set(model_config.user_groups)):
- if editor_role_ids & set(model_config.user_roles.get(user_id, [])):
- return User(id=user_id, username=str(user_id))
- if editor_group_ids & set(model_config.user_groups.get(user_id, [])):
- return User(id=user_id, username=str(user_id))
- return None
-
-
class ModelType(int, Enum):
DASHBOARD = 1
CHART = 2
@@ -205,7 +187,10 @@ class ModelType(int, Enum):
],
ModelConfig(editors=EditorSpec(user_ids=[2]), modifier=1),
None,
- (ExecutorType.EDITOR, 2),
+ # Modifier (1) is not an editor and there is no creator, so EDITOR
+ # no longer falls through to the unrelated attached editor (2) --
+ # it falls through to the next executor in the chain instead.
+ (ExecutorType.MODIFIER, 1),
),
(
ModelType.REPORT_SCHEDULE,
@@ -420,7 +405,8 @@ class ModelType(int, Enum):
None,
(ExecutorType.FIXED_USER, FIXED_USER_ID),
),
- # Role editor fallback resolves a deterministic physical user.
+ # No modifier/creator set: EDITOR no longer falls through to an
+ # arbitrary user reachable via a role editor subject.
(
ModelType.REPORT_SCHEDULE,
[ExecutorType.EDITOR],
@@ -429,9 +415,10 @@ class ModelType(int, Enum):
user_roles={8: [10], 6: [10]},
),
None,
- (ExecutorType.EDITOR, 6),
+ ExecutorNotFoundError(),
),
- # Group editor fallback resolves a deterministic physical user.
+ # No modifier/creator set: EDITOR no longer falls through to an
+ # arbitrary user reachable via a group editor subject.
(
ModelType.REPORT_SCHEDULE,
[ExecutorType.EDITOR],
@@ -440,9 +427,11 @@ class ModelType(int, Enum):
user_groups={9: [20], 7: [20]},
),
None,
- (ExecutorType.EDITOR, 7),
+ ExecutorNotFoundError(),
),
- # Direct user editors are preferred over role/group fallback users.
+ # No modifier/creator set: EDITOR no longer falls through to an
+ # unrelated directly-attached user editor either -- resolution is
+ # limited to whoever authored the model (modifier/creator).
(
ModelType.REPORT_SCHEDULE,
[ExecutorType.EDITOR],
@@ -451,7 +440,22 @@ class ModelType(int, Enum):
user_roles={4: [10]},
),
None,
- (ExecutorType.EDITOR, 5),
+ ExecutorNotFoundError(),
+ ),
+ # Attacker scenario: a low-privileged creator/modifier (99) attaches a
+ # victim (5) as an editor with no consent. EDITOR must not resolve to
+ # the victim -- 99 is not itself an editor, so scheduling fails
+ # instead of silently executing as user 5.
+ (
+ ModelType.REPORT_SCHEDULE,
+ [ExecutorType.EDITOR],
+ ModelConfig(
+ editors=EditorSpec(user_ids=[5]),
+ creator=99,
+ modifier=99,
+ ),
+ None,
+ ExecutorNotFoundError(),
),
# Mixed editors with MODIFIER_EDITOR: modifier is a user-type editor
(
@@ -587,11 +591,12 @@ class ModelType(int, Enum):
None,
ExecutorNotFoundError(),
),
- # EDITOR: creator (the only editor) is inactive, so scheduling falls
- # through to the still-active direct-user editor instead of failing.
- # This is the scenario from #33584: the original report creator has
- # gone inactive but a currently-active owner/editor should still be
- # usable as the executor.
+ # EDITOR: creator (the only author) is inactive, so scheduling fails
+ # rather than falling through to an unrelated attached editor. This is
+ # a deliberate behavior break from the prior #33584 fallback: EDITOR
+ # only ever resolves to whoever authored the model's state, so an
+ # inactive author is no longer patched over by scheduling the task
+ # under a different, merely-attached editor's credentials.
(
ModelType.REPORT_SCHEDULE,
[ExecutorType.EDITOR],
@@ -601,11 +606,11 @@ class ModelType(int, Enum):
creator_active=False,
),
None,
- (ExecutorType.EDITOR, 2),
+ ExecutorNotFoundError(),
),
- # EDITOR: both modifier and creator are inactive editors, and the only
- # direct-user editor is also inactive → falls through to the indirect
- # (role/group) editor resolution.
+ # EDITOR: both modifier and creator are inactive, so scheduling fails
+ # rather than falling through to an indirect (role/group) editor that
+ # never authored the model.
(
ModelType.REPORT_SCHEDULE,
[ExecutorType.EDITOR],
@@ -620,7 +625,7 @@ class ModelType(int, Enum):
user_roles={6: [10]},
),
None,
- (ExecutorType.EDITOR, 6),
+ ExecutorNotFoundError(),
),
# EDITOR: modifier is an inactive editor, creator is an active editor →
# resolves to the creator instead of failing outright.
@@ -701,20 +706,14 @@ def test_get_executor(
"superset.subjects.utils.get_user_subject_ids",
side_effect=mock_get_user_subject_ids,
):
- with patch(
- "superset.tasks.utils._get_indirect_editor_user",
- side_effect=lambda editors: _get_mock_indirect_editor_user(
- editors, model_config
- ),
- ):
- with cm:
- executor_type, executor = get_executor(
- executors=executors,
- model=obj,
- current_user=str(current_user) if current_user else None,
- )
- assert executor_type == expected_executor_type
- assert executor == expected_executor
+ with cm:
+ executor_type, executor = get_executor(
+ executors=executors,
+ model=obj,
+ current_user=str(current_user) if current_user else None,
+ )
+ assert executor_type == expected_executor_type
+ assert executor == expected_executor
@pytest.mark.parametrize(
diff --git a/tests/unit_tests/thumbnails/test_digest.py b/tests/unit_tests/thumbnails/test_digest.py
index d6d7f7e2c0d..02e1316e4d8 100644
--- a/tests/unit_tests/thumbnails/test_digest.py
+++ b/tests/unit_tests/thumbnails/test_digest.py
@@ -21,7 +21,7 @@ from typing import Any, TYPE_CHECKING
from unittest.mock import MagicMock, patch, PropertyMock
import pytest
-from flask import current_app
+from flask import current_app, g
from flask_appbuilder.security.sqla.models import User
from superset.connectors.sqla.models import BaseDatasource, SqlaTable
@@ -494,6 +494,92 @@ def test_dashboard_digest_deterministic_datasource_order(
assert digests[0] is not None
+def test_dashboard_digest_uses_guest_token_rls(
+ app_context: None,
+) -> None:
+ """
+ Two guest tokens whose username collides with the same real DB username,
+ but which carry different per-token rls claims, must produce different
+ digests. If the DB-user lookup were preferred on a username collision,
+ both tokens would compute RLS under the unrelated DB user's identity and
+ collide on the same cache entry -- serving tenant B the screenshot
+ rendered under tenant A's RLS scope.
+ """
+ from superset import security_manager
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+ from superset.thumbnails.digest import get_dashboard_digest
+
+ kwargs = {**_DEFAULT_DASHBOARD_KWARGS}
+ slices = [Slice(**slice_kwargs) for slice_kwargs in kwargs.pop("slices")]
+ dashboard = Dashboard(**kwargs, slices=slices)
+
+ # A real DB user whose username collides with the guest token's username.
+ colliding_db_user = User(id=99, username="guest_alice")
+
+ class FakeGuestUser:
+ """
+ Minimal guest-user stand-in. Two instances differ only by their
+ per-token `guest_rls` claim, mirroring how two guest tokens can share
+ a username while carrying different rls claims.
+ """
+
+ is_anonymous = False
+ id = None
+
+ def __init__(self, username: str, guest_rls: str) -> None:
+ self.username = username
+ self.guest_rls = guest_rls
+
+ def make_datasource() -> MagicMock:
+ ds = MagicMock(spec=BaseDatasource)
+ ds.id = 1
+ ds.is_rls_supported = True
+
+ # Simulate the real get_sqla_row_level_filters(): in production this
+ # resolves RLS (including get_guest_rls_filters()) against whichever
+ # identity override_user() installed as the ambient g.user.
+ def _filters() -> list[str]:
+ current = getattr(g, "user", None)
+ guest_rls = getattr(current, "guest_rls", None)
+ return [guest_rls] if guest_rls else []
+
+ ds.get_sqla_row_level_filters = MagicMock(side_effect=_filters)
+ return ds
+
+ digests = []
+ for guest_rls in ("tenant_a_filter", "tenant_b_filter"):
+ guest_user = FakeGuestUser("guest_alice", guest_rls)
+ with (
+ patch.dict(
+ current_app.config,
+ {
+ "THUMBNAIL_EXECUTORS": [ExecutorType.CURRENT_USER],
+ "THUMBNAIL_DASHBOARD_DIGEST_FUNC": None,
+ },
+ ),
+ patch.object(
+ type(dashboard),
+ "datasources",
+ new_callable=PropertyMock,
+ return_value=[make_datasource()],
+ ),
+ patch.object(security_manager, "find_user", return_value=colliding_db_user),
+ patch.object(
+ security_manager,
+ "get_current_guest_user_if_guest",
+ return_value=guest_user,
+ ),
+ patch.object(security_manager, "prefetch_rls_filters", return_value=None),
+ override_user(guest_user),
+ ):
+ digests.append(get_dashboard_digest(dashboard=dashboard))
+
+ assert digests[0] is not None
+ assert digests[1] is not None
+ assert digests[0] != digests[1]
+
+
def test_dashboard_digest_prefetches_rls_filters(
app_context: None,
) -> None:
diff --git a/tests/unit_tests/utils/screenshot_test.py b/tests/unit_tests/utils/screenshot_test.py
index d6a26b21625..fe64f7d69a7 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
):
@@ -206,6 +224,29 @@ class TestComputeAndCache:
cache_payload: ScreenshotCachePayloadType = screenshot_obj.cache.get("key")
assert cache_payload["image"] != b"initial_value"
+ def test_recomputes_updated_entry_with_mismatched_scope(
+ self, mocker: MockerFixture, screenshot_obj
+ ):
+ """A cache entry that already has a valid image but a scope that
+ doesn't match this screenshot object's `cache_scope` (e.g. an entry
+ written before scope tracking existed, or written for a different
+ object) must be treated as a cache miss and recomputed -- otherwise
+ it can never be re-stamped with the right scope and stays
+ unservable forever."""
+ mocks = self._setup_compute_and_cache(mocker, screenshot_obj)
+ cached_value = ScreenshotCachePayload(image=b"initial_value")
+ get_from_cache_key = mocks.get("get_from_cache_key")
+ get_from_cache_key.return_value = cached_value
+
+ screenshot_obj.cache_scope = "dashboard:5"
+ screenshot_obj.compute_and_cache(force=False)
+
+ get_screenshot = mocks.get("get_screenshot")
+ get_screenshot.assert_called_once()
+ cache_payload: ScreenshotCachePayloadType = screenshot_obj.cache.get("key")
+ assert cache_payload["image"] != b"initial_value"
+ assert cache_payload["scope"] == "dashboard:5"
+
def test_resize(self, mocker: MockerFixture, screenshot_obj):
mocks = self._setup_compute_and_cache(mocker, screenshot_obj)
window_size = thumb_size = (10, 10)
@@ -267,6 +308,72 @@ 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
+
+ def test_should_trigger_task_ignores_scope_by_default(self):
+ """Without an `expected_scope`, an updated entry with a real image is
+ never re-triggered -- existing (scope-agnostic) callers keep their
+ current behavior."""
+ payload = ScreenshotCachePayload(image=b"data", scope="dashboard:5")
+ assert payload.should_trigger_task(force=False) is False
+
+ def test_should_trigger_task_true_on_scope_mismatch(self):
+ """An updated entry whose scope doesn't match what the caller expects
+ (including a legacy entry with no scope at all) must be treated as a
+ cache miss so it gets recomputed and re-scoped."""
+ payload = ScreenshotCachePayload(image=b"data", scope=None)
+ assert (
+ payload.should_trigger_task(force=False, expected_scope="dashboard:5")
+ is True
+ )
+
+ mismatched = ScreenshotCachePayload(image=b"data", scope="dashboard:6")
+ assert (
+ mismatched.should_trigger_task(force=False, expected_scope="dashboard:5")
+ is True
+ )
+
+ def test_should_trigger_task_false_on_scope_match(self):
+ payload = ScreenshotCachePayload(image=b"data", scope="dashboard:5")
+ assert (
+ payload.should_trigger_task(force=False, expected_scope="dashboard:5")
+ is False
+ )
+
+
class TestBaseScreenshotDriverFallback:
"""Test BaseScreenshot.driver() fallback logic for Playwright migration."""