diff --git a/UPDATING.md b/UPDATING.md index 64787158583f..450f6881ca73 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -31,6 +31,7 @@ The unqualified `DatabaseRestApi.oauth2` StatsD counter has been replaced with `DatabaseRestApi.oauth2.error`. Update monitoring rules and dashboards that consume the old counter to use the outcome-specific replacements. +- [42930](https://github.com/apache/superset/pull/42930): Dataset import data-URI fetches no longer honor an HTTP(S) proxy when `DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS` is `False` (the default): the connection is now made directly to the destination so the peer-address check validates the real target instead of a proxy's. Deployments that require an egress proxy to reach legitimate external data URLs for dataset import should set `DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS = True` or otherwise ensure those URLs resolve without one. - [42935](https://github.com/apache/superset/pull/42935): The MCP service now refuses to start (`MCPAuthConfigError`) when `MCP_JWT_ISSUER` trusts more than one issuer and no `MCP_USER_RESOLVER` is configured, instead of only logging a warning. This was already a documented misconfiguration (the default resolver isn't issuer-scoped, so distinct trusted issuers minting the same username/email would resolve to the same Superset user); deployments trusting multiple issuers must configure an `MCP_USER_RESOLVER` that derives its identity from the token's `iss` claim before upgrading. Single-issuer deployments are unaffected. - [42393](https://github.com/apache/superset/pull/42393): Exported dataset YAML now carries a `uuid` for each metric and column so that custom folder assignments (which reference metrics/columns by UUID) survive an import into another workspace. This affects any export bundle that contains datasets, not just a dataset export: chart, dashboard, database and full-asset exports all embed the same dataset YAML, so a dashboard exported from this release also fails to import into an older one even though no dataset was exported directly. As with `folders` and `currency_code_column`, the affected `datasets/` files fail schema validation (`Unknown field: uuid`) when imported into Superset releases that predate this change; regenerate or hand-edit exports for older targets in mixed-version fleets. - [42300](https://github.com/apache/superset/pull/42300): Timeseries charts (line/area/bar) with a Y-axis bound in effect — either an explicit `yAxisBounds` or one derived from `truncateYAxis` — now clamp out-of-range data points to that bound instead of letting ECharts drop the point (and the line segments around it) entirely. Any existing chart with a configured Y-axis bound and data outside it will look different after upgrading: a gap becomes a point pinned to the boundary. The clamp also rewrites the value ECharts reads for that point's tooltip and data label, so the displayed value is the bound rather than the true observation. diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.test.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.test.tsx index f353fd2532dd..3025164dd640 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.test.tsx @@ -16,9 +16,13 @@ * specific language governing permissions and limitations * under the License. */ -import { render, waitFor } from '../../../../spec/helpers/testing-library'; import type { EChartsCoreOption } from 'echarts/core'; -import Echart, { isReportScreenshotMode } from './Echart'; +import { render, waitFor } from '../../../../spec/helpers/testing-library'; +import Echart, { + ECHARTS_HOST_CLASS, + ECHARTS_RENDER_FINISHED_CLASS, + isReportScreenshotMode, +} from './Echart'; import type { EchartsProps } from '../types'; type Handler = (params: unknown) => void; @@ -272,3 +276,31 @@ test('keeps animation enabled when not in report screenshot mode', async () => { const lastOptions = mockChart.setOption.mock.calls.at(-1)?.[0]; expect(lastOptions.animation).not.toBe(false); }); + +test('tags the ECharts canvas host with the readiness-gate class', async () => { + const { container } = render(renderEchart(), { + initialState, + useRedux: true, + }); + await waitFor(() => expect(mockChart.setOption).toHaveBeenCalled()); + expect(container.querySelector(`.${ECHARTS_HOST_CLASS}`)).not.toBeNull(); +}); + +test('marks the host painted only on the ECharts `finished` event', async () => { + const { container } = render(renderEchart(), { + initialState, + useRedux: true, + }); + await waitFor(() => expect(mockChart.setOption).toHaveBeenCalled()); + + const host = container.querySelector(`.${ECHARTS_HOST_CLASS}`) as HTMLElement; + expect(host).not.toBeNull(); + + // `setOption` ran during mount, which clears the marker; `finished` has not + // fired yet, so the host must NOT be flagged as painted. + expect(host).not.toHaveClass(ECHARTS_RENDER_FINISHED_CLASS); + + // Simulate ECharts completing its draw -> the host is flagged painted. + trigger('finished'); + expect(host).toHaveClass(ECHARTS_RENDER_FINISHED_CLASS); +}); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx index 496882780fa0..3306f0a4e84e 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx @@ -138,6 +138,15 @@ export function isReportScreenshotMode(): boolean { } } +// Report-screenshot readiness contract (see superset/utils/screenshot_utils.py). +// `echarts-host` marks the canvas host element; `echarts-render-finished` is +// toggled OFF before each setOption and ON in the ECharts `finished` event -- +// the only signal that the canvas is fully painted (chartStatus/onRenderSuccess +// both fire pre-paint). The readiness gate treats a host that lacks +// `echarts-render-finished` as not-yet-painted so it never captures a blank chart. +export const ECHARTS_HOST_CLASS = 'echarts-host'; +export const ECHARTS_RENDER_FINISHED_CLASS = 'echarts-render-finished'; + function Echart( { width, @@ -201,6 +210,11 @@ function Echart( width, height, }); + // Paint marker for the report-screenshot readiness gate. `finished` + // is the only event that guarantees the canvas is fully drawn. + chartRef.current.on('finished', () => { + divRef.current?.classList.add(ECHARTS_RENDER_FINISHED_CLASS); + }); } // did mount handleSizeChange({ width, height }); @@ -321,6 +335,9 @@ function Echart( } )?.dataZoom : undefined; + // Clear the paint marker before (re)drawing; the `finished` handler + // re-adds it once the new frame is fully rendered. + divRef.current?.classList.remove(ECHARTS_RENDER_FINISHED_CLASS); chartRef.current?.setOption(themedEchartOptions, { notMerge, replaceMerge: notMerge ? undefined : ['series'], @@ -412,7 +429,14 @@ function Echart( handleSizeChange({ width, height }); }, [width, height, handleSizeChange]); - return ; + return ( + + ); } export default forwardRef(Echart); diff --git a/superset/commands/database/oauth2.py b/superset/commands/database/oauth2.py index 89e6a9d4be5c..1d09f5cc76f7 100644 --- a/superset/commands/database/oauth2.py +++ b/superset/commands/database/oauth2.py @@ -21,7 +21,7 @@ from typing import cast from uuid import UUID -from superset import db +from superset import db, security_manager from superset.commands.base import BaseCommand from superset.commands.database.exceptions import DatabaseNotFoundError from superset.daos.database import DatabaseUserOAuth2TokensDAO @@ -31,6 +31,7 @@ from superset.key_value.types import JsonKeyValueCodec, KeyValueResource from superset.models.core import Database, DatabaseUserOAuth2Tokens from superset.superset_typing import OAuth2State +from superset.utils.core import get_user_id from superset.utils.decorators import on_error, transaction from superset.utils.oauth2 import decode_oauth2_state @@ -121,6 +122,14 @@ def validate(self) -> None: self._state = decode_oauth2_state(self._parameters["state"]) + # Bind the callback to the current session: require an authenticated, + # non-guest user whose id matches the one carried in the state. + user_id = get_user_id() + if user_id is None or security_manager.is_guest_user(): + raise OAuth2Error("The OAuth2 callback requires an authenticated user") + if user_id != self._state["user_id"]: + raise OAuth2Error("The OAuth2 state belongs to a different user") + if database := DatabaseUserOAuth2TokensDAO.get_database( self._state["database_id"] ): diff --git a/superset/commands/dataset/importers/v1/utils.py b/superset/commands/dataset/importers/v1/utils.py index 266f14956770..b23c1183761a 100644 --- a/superset/commands/dataset/importers/v1/utils.py +++ b/superset/commands/dataset/importers/v1/utils.py @@ -15,9 +15,12 @@ # specific language governing permissions and limitations # under the License. import gzip +import ipaddress import logging import os import re +import socket +from http.client import HTTPConnection, HTTPResponse, HTTPSConnection from typing import Any from urllib import request from urllib.parse import urljoin, urlparse @@ -47,7 +50,7 @@ from superset.sql.parse import Table from superset.utils import json from superset.utils.core import get_user -from superset.utils.network import is_safe_host +from superset.utils.network import is_safe_host, is_safe_ip logger = logging.getLogger(__name__) @@ -76,6 +79,47 @@ def redirect_request( return super().redirect_request(req, fp, code, msg, headers, newurl) +def _raise_for_unsafe_peer(sock: socket.socket) -> None: + """ + Validate that an established connection's actual peer is publicly + routable, so the address reached matches the policy applied to the host. + """ + peer = sock.getpeername()[0] + if not is_safe_ip(ipaddress.ip_address(peer)): + raise DatasetForbiddenDataURI() + + +class _PeerValidatingHTTPConnection(HTTPConnection): + """HTTP connection that validates the peer address on connect.""" + + def connect(self) -> None: + super().connect() + _raise_for_unsafe_peer(self.sock) + + +class _PeerValidatingHTTPSConnection(HTTPSConnection): + """HTTPS connection that validates the peer address after the handshake.""" + + def connect(self) -> None: + super().connect() + _raise_for_unsafe_peer(self.sock) + + +class _PeerValidatingHTTPHandler(request.HTTPHandler): + """Opens HTTP connections through the peer-validating connection class.""" + + def http_open(self, req: request.Request) -> HTTPResponse: + return self.do_open(_PeerValidatingHTTPConnection, req) + + +class _PeerValidatingHTTPSHandler(request.HTTPSHandler): + """Opens HTTPS connections through the peer-validating connection class.""" + + def https_open(self, req: request.Request) -> HTTPResponse: + context = self._context # type: ignore[attr-defined] + return self.do_open(_PeerValidatingHTTPSConnection, req, context=context) + + CHUNKSIZE = 512 VARCHAR = re.compile(r"VARCHAR\((\d+)\)", re.IGNORECASE) @@ -581,7 +625,17 @@ def load_data(data_uri: str, dataset: SqlaTable, database: Database) -> None: validate_data_uri(data_uri) logger.info("Downloading data from %s", data_uri) - opener = request.build_opener(_ValidatingRedirectHandler) + handlers: list[request.BaseHandler | type[request.BaseHandler]] = [ + _ValidatingRedirectHandler + ] + if not app.config["DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS"]: + # Also enforce the policy at the socket layer: re-check the peer of + # every connection, including each redirect hop. Disable proxies so the + # connection is made directly to the destination and the peer check + # validates the destination address rather than a proxy's. + handlers.append(request.ProxyHandler({})) + handlers.extend([_PeerValidatingHTTPHandler, _PeerValidatingHTTPSHandler]) + opener = request.build_opener(*handlers) data = opener.open(data_uri) # pylint: disable=consider-using-with # noqa: S310 if data_uri.endswith(".gz"): data = gzip.open(data) diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index 0e6b3ff34a06..c87f571612c8 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -137,6 +137,29 @@ def resolve_executor_user(model: ReportSchedule) -> tuple["User", str]: return user, username +def _should_build_execution_context(model: ReportSchedule) -> bool: + """ + Whether an execution should run under a :class:`ReportExecutionContext`. + + Reports always do — their behavior is unchanged. Alerts join them only when + they deliver a rendered PNG/PDF screenshot to recipients, which happens when + ``ALERTS_ATTACH_REPORTS`` is enabled. Delivered screenshots must fail closed: + the context selects the fail-closed readiness predicate and disables + partial-tile fallback, so a blank or incomplete capture raises instead of + being delivered. + + CSV/text alerts, alerts without the attach flag, the non-delivered + query-context capture, and UI thumbnails are deliberately excluded and keep + their lenient capture contract. + """ + if model.type == ReportScheduleType.REPORT: + return True + return model.report_format in ( + ReportDataFormat.PNG, + ReportDataFormat.PDF, + ) and feature_flag_manager.is_feature_enabled("ALERTS_ATTACH_REPORTS") + + def log_report_delivery_phase( report_context: ReportExecutionContext | None, recipient_type: ReportRecipientType | None, @@ -1972,13 +1995,13 @@ def next(self) -> None: # noqa: C901 try: self.send() - except Exception as ex: # pylint: disable=broad-except - if self._handle_retry_or_error(str(ex), ex): + except Exception as first_ex: # pylint: disable=broad-except + if self._handle_retry_or_error(str(first_ex), first_ex): return # retry scheduled — exit cleanly try: self.update_report_schedule_and_log( - ReportState.ERROR, error_message=str(ex) + ReportState.ERROR, error_message=str(first_ex) ) except (ReportScheduleUnexpectedError, SQLAlchemyError) as logging_ex: # Logging failed (likely StaleDataError), but we still want to @@ -1991,7 +2014,45 @@ def next(self) -> None: # noqa: C901 exc_info=True, ) # Re-raise the original exception, not the logging failure - raise ex from logging_ex + raise first_ex from logging_ex + + # A delivery failure from the Success/Grace path must notify the + # owner just like the first-run path (ReportNotTriggeredErrorState). + # Without this, a schedule whose previous run succeeded would fail + # silently — e.g. once a screenshot capture starts failing closed. + # The error grace period still throttles repeated notifications. + if not self.is_in_error_grace_period(): + second_error_message = REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER + try: + self.send_error( + f"Error occurred for {self._report_schedule.type}:" + f" {self._report_schedule.name}", + str(first_ex), + ) + except SupersetErrorsException as second_ex: + second_error_message = ";".join( + [error.message for error in second_ex.errors] + ) + except ReportScheduleUnexpectedError: + # send_error failed due to logging issue; log and continue + # to raise the original error + logger.warning( + "Failed to send error notification due to database issue", + exc_info=True, + ) + except Exception as second_ex: # pylint: disable=broad-except + second_error_message = str(second_ex) + finally: + try: + self.update_report_schedule_and_log( + ReportState.ERROR, error_message=second_error_message + ) + except ReportScheduleUnexpectedError: + # Logging failed again; log it but don't hide first_ex + logger.warning( + "Failed to log final error state due to database issue", + exc_info=True, + ) raise # send() succeeded — clear retry state and log success. Any execution @@ -2058,13 +2119,18 @@ def run(self) -> None: if not self._model: raise ReportScheduleExecuteUnexpectedError() - if self._model.type == ReportScheduleType.REPORT: + # Reports always run under an execution context; alerts join them + # only when they deliver a rendered screenshot, so a blank/partial + # capture fails closed instead of being delivered. Ownership and + # terminal-error persistence remain report-only recovery semantics. + if _should_build_execution_context(self._model): # An invocation that enters on WORKING is a duplicate or stale # recovery, not the owner that created the active row. Its state # handler may terminalize a stale execution, but the command # boundary must never infer ownership from a replayed UUID. owns_report_working_state = ( - self._model.last_state != ReportState.WORKING + self._model.type == ReportScheduleType.REPORT + and self._model.last_state != ReportState.WORKING ) total_seconds = resolve_report_execution_budget_seconds( app.config, diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py index d8150956e6d9..17852041edbc 100644 --- a/superset/common/query_context_processor.py +++ b/superset/common/query_context_processor.py @@ -58,6 +58,7 @@ get_column_name, get_column_names_from_columns, get_column_names_from_metrics, + get_user_id, is_adhoc_column, is_adhoc_metric, ) @@ -270,6 +271,11 @@ def query_cache_key(self, query_obj: QueryObject, **kwargs: Any) -> str | None: datasource = self._qc_datasource extra_cache_keys = datasource.get_extra_cache_keys(query_obj.to_dict()) + # Annotation data is cached on the same entry as the dataframe, so the + # key must also bind the annotation sources' security context. + if query_obj and query_obj.annotation_layers: + kwargs["annotation_context"] = self._annotation_cache_context(query_obj) + cache_key = ( query_obj.cache_key( datasource=datasource.uid, @@ -283,6 +289,32 @@ def query_cache_key(self, query_obj: QueryObject, **kwargs: Any) -> str | None: ) return cache_key + def _annotation_cache_context(self, query_obj: QueryObject) -> dict[str, Any]: + """ + Cache-key material binding cached annotation data to its security + context. + + Annotation payloads are fetched per requesting user and stored on the + same cache entry as the dataframe, so the key also binds the requesting + user and, for chart-backed layers, the RLS clauses of the referenced + chart's datasource. + """ + source_rls: dict[str, list[str] | None] = {} + for layer in query_obj.annotation_layers: + if layer.get("sourceType") not in ("line", "table"): + continue + layer_value = layer.get("value") + chart = ( + ChartDAO.find_by_id(layer_value) if layer_value is not None else None + ) + annotation_datasource = chart.datasource if chart else None + source_rls[str(layer.get("value"))] = ( + security_manager.get_rls_cache_key(annotation_datasource) + if annotation_datasource + else None + ) + return {"user_id": get_user_id(), "source_rls": source_rls} + def get_query_result(self, query_object: QueryObject) -> QueryResult: """ Returns a pandas dataframe based on the query object. @@ -636,6 +668,11 @@ def get_native_annotation_data(query_obj: QueryObject) -> dict[str, Any]: if layer["sourceType"] == "NATIVE" ] layer_ids = [layer["value"] for layer in annotation_layers] + # Enforce the annotation read permission before returning layer records. + if layer_ids and not security_manager.can_access("can_read", "Annotation"): + raise QueryObjectValidationError( + _("You don't have access to annotation layers") + ) layer_objects = { layer_object.id: layer_object for layer_object in AnnotationLayerDAO.find_by_ids(layer_ids) @@ -645,6 +682,15 @@ def get_native_annotation_data(query_obj: QueryObject) -> dict[str, Any]: for layer in annotation_layers: layer_id = layer["value"] layer_name = layer["name"] + # A request may reference a layer id that does not exist; treat it + # as a validation error rather than failing on the missing key. + if (layer_object := layer_objects.get(layer_id)) is None: + raise QueryObjectValidationError( + _( + "Annotation layer with ID %(layer_id)s was not found", + layer_id=layer_id, + ) + ) columns = [ "start_dttm", "end_dttm", @@ -652,7 +698,6 @@ def get_native_annotation_data(query_obj: QueryObject) -> dict[str, Any]: "long_descr", "json_metadata", ] - layer_object = layer_objects[layer_id] records = [ {column: getattr(annotation, column) for column in columns} for annotation in layer_object.annotation diff --git a/superset/extensions/cache_middleware.py b/superset/extensions/cache_middleware.py index c688bbe8d9fb..d754c44a9f6c 100644 --- a/superset/extensions/cache_middleware.py +++ b/superset/extensions/cache_middleware.py @@ -26,8 +26,12 @@ # Matches only the static asset endpoint: # /api/v1/extensions///, where the file portion may # contain nested segments (worker / WASM / chunk subfolders). -# Does not match the list (/), get (//), or info (/_info) endpoints. -_ASSET_PATH_RE: re.Pattern[str] = re.compile(r"^/api/v1/extensions/[^/]+/[^/]+/.+$") +# Does not match the list (/), get (//), or info (/_info) +# endpoints, nor the per-user storage endpoints under +# ///storage/, whose responses must keep ``Vary: Cookie``. +_ASSET_PATH_RE: re.Pattern[str] = re.compile( + r"^/api/v1/extensions/[^/]+/[^/]+/(?!storage/).+$" +) class ExtensionCacheMiddleware: diff --git a/superset/extensions/storage/api.py b/superset/extensions/storage/api.py index 7ba8e7be5632..340825ffe9e5 100644 --- a/superset/extensions/storage/api.py +++ b/superset/extensions/storage/api.py @@ -92,10 +92,16 @@ class ExtensionStorageRestApi(BaseApi): route_base = "/api/v1/extensions" def response(self, status_code: int, **kwargs: Any) -> Response: - """Helper method to create JSON responses.""" + """Helper method to create JSON responses. + + Stored values are scoped to the requesting user, so responses are + marked non-cacheable. + """ from flask import jsonify - return jsonify(kwargs), status_code + response = jsonify(kwargs) + response.cache_control.no_store = True + return response, status_code def response_404(self, message: str = "Not found") -> Response: """Helper method to create 404 responses.""" diff --git a/superset/jinja_context.py b/superset/jinja_context.py index b902f184d146..96b3756a68f4 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -1272,6 +1272,34 @@ def get_dataset_id_from_context(metric_key: str) -> int: raise SupersetTemplateException(exc_message) +def guest_user_can_access_dataset(dataset: SqlaTable) -> bool: + """ + Whether the current guest (embedded) user may read the given dataset. + + Guest access is granted per dashboard, so the dataset must back at least + one chart on a dashboard the guest token covers; a ``datasets`` allowlist + on the token further restricts the reachable IDs. + + :param dataset: a dataset resolved without the DAO base filter. + :returns: whether the guest user may read the dataset. + """ + guest_user = security_manager.get_current_guest_user_if_guest() + if not guest_user: + return False + + allowed_datasets: list[int] | None = guest_user.guest_token.get("datasets") + if allowed_datasets is not None and ( + not isinstance(allowed_datasets, list) or dataset.id not in allowed_datasets + ): + return False + + return any( + security_manager.has_guest_access(dashboard) + for slc in dataset.slices + for dashboard in slc.dashboards + ) + + def metric_macro( env: Environment, context: dict[str, Any], @@ -1294,8 +1322,9 @@ def metric_macro( if not dataset_id: dataset_id = get_dataset_id_from_context(metric_key) - # Embedded user access is validated at the dashboard level, so we bypass - # the regular DAO filter for them + # Embedded (guest) user access is validated at the dashboard level, so the + # regular DAO filter is bypassed for them and dashboard-level scope is + # enforced explicitly below. dataset = DatasetDAO.find_by_id( dataset_id, skip_base_filter=security_manager.is_guest_user(), @@ -1303,6 +1332,11 @@ def metric_macro( if not dataset: raise DatasetNotFoundError(f"Dataset ID {dataset_id} not found.") + # With the base filter skipped, scope a guest to datasets reachable through + # a dashboard their token grants; reuse the not-found error for consistency. + if security_manager.is_guest_user() and not guest_user_can_access_dataset(dataset): + raise DatasetNotFoundError(f"Dataset ID {dataset_id} not found.") + metrics: dict[str, str] = { metric.metric_name: metric.expression for metric in dataset.metrics } diff --git a/superset/security/manager.py b/superset/security/manager.py index a7a23f6c47a3..9f87380f20e8 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -754,15 +754,24 @@ def _native_filter_query_modified( query: Any, allowed_columns: set[str], allowed_metrics: set[str] ) -> bool: """Whether a single query in a native-filter request reads beyond its targets.""" - # Columns and group-by may only reference target column(s); adhoc (free-form - # SQL) columns cannot be validated, so reject them. - for key in ("columns", "groupby"): + # Columns, group-by, and series columns may only reference target column(s); + # adhoc (free-form SQL) columns cannot be validated, so reject them. + for key in ("columns", "groupby", "series_columns"): for col in getattr(query, key, None) or []: if not isinstance(col, str) or col not in allowed_columns: return True for metric in getattr(query, "metrics", None) or []: if not _native_filter_term_allowed(metric, allowed_columns, allowed_metrics): return True + # A series-limit metric ranks the top-N groups in the inner query, so it is + # a value-returning term and is validated like a metric. ``QueryObject`` + # renames the deprecated ``timeseries_limit_metric`` payload key onto this + # attribute, so both spellings are covered. + series_limit_metric = getattr(query, "series_limit_metric", None) + if series_limit_metric and not _native_filter_term_allowed( + series_limit_metric, allowed_columns, allowed_metrics + ): + return True # order-by entries are ``(expression, asc)`` pairs. for order in getattr(query, "orderby", None) or []: expr = order[0] if isinstance(order, (list, tuple)) and order else order @@ -784,8 +793,9 @@ def _native_filter_request_modified(query_context: "QueryContext") -> bool: A native filter may only read the column(s) it targets on the dashboard it belongs to. The request is treated as modified (and therefore rejected for guest users) when it cannot be tied to a native filter on the requesting - dashboard, or when any value-returning term (column, group-by, metric, or - order-by) references something other than a target column, a simple + dashboard, or when any value-returning term (column, group-by, series + column, metric, series-limit metric, or order-by) references something + other than a target column, a simple aggregate over a target column, or the filter's configured sort metric. Free-form SQL terms and saved metrics other than the configured sort metric are rejected. Row-restricting clauses (``filter``/``extras``) are not @@ -4297,6 +4307,15 @@ def has_promiscuous_chart_access() -> bool: child_slice_id=slice_id, parent_slice=parent_slc, ) + # Bind the request to the child + # chart's own datasource, mirroring + # the direct-chart leg above. + and ( + child_slc := self.session.query(Slice) + .filter(Slice.id == slice_id) + .one_or_none() + ) + and child_slc.datasource == datasource ) ) ) diff --git a/superset/tasks/context.py b/superset/tasks/context.py index 07f95589acfc..76c8a01b8550 100644 --- a/superset/tasks/context.py +++ b/superset/tasks/context.py @@ -129,7 +129,12 @@ def _refresh_task(self) -> "Task": """ from superset.daos.tasks import TaskDAO - fresh_task = TaskDAO.find_one_or_none(uuid=self._task_uuid) + # Internal executor path: load the running task itself, keyed on a + # UUID this instance already holds, not a user-requested lookup; + # see TaskFilter for the request-scoped vs. internal-plumbing split. + fresh_task = TaskDAO.find_one_or_none( + uuid=self._task_uuid, skip_base_filter=True + ) if not fresh_task: raise ValueError(f"Task {self._task_uuid} not found") diff --git a/superset/tasks/decorators.py b/superset/tasks/decorators.py index 1cd7ff6497c2..bc43353145d1 100644 --- a/superset/tasks/decorators.py +++ b/superset/tasks/decorators.py @@ -167,6 +167,12 @@ class TaskWrapper(Generic[P]): return value is discarded. Direct calls execute synchronously, .schedule() runs async via Celery. + + The status-refresh reads below pass ``skip_base_filter=True`` to + ``TaskDAO.find_one_or_none`` because they read back the task this + executor itself submitted, keyed on the UUID it already holds -- not + a task requested by a user. See ``TaskFilter`` for the request-scoped + vs. internal-plumbing split. """ def __init__( @@ -378,7 +384,7 @@ def _wait_for_existing_task(self, task: "Task", timeout: int | None) -> "Task": task.uuid, ) # Return task in current state (caller can check status) - refreshed = TaskDAO.find_one_or_none(uuid=task.uuid) + refreshed = TaskDAO.find_one_or_none(uuid=task.uuid, skip_base_filter=True) return refreshed if refreshed else task def _execute_inline( @@ -422,7 +428,7 @@ def _execute_inline( set_ended_at=True, ).run() # Refresh to get updated task - refreshed = TaskDAO.find_one_or_none(uuid=task.uuid) + refreshed = TaskDAO.find_one_or_none(uuid=task.uuid, skip_base_filter=True) return refreshed if refreshed else task # Atomic transition: PENDING → IN_PROGRESS (set started_at for duration @@ -441,7 +447,7 @@ def _execute_inline( self.name, task_uuid, ) - refreshed = TaskDAO.find_one_or_none(uuid=task_uuid) + refreshed = TaskDAO.find_one_or_none(uuid=task_uuid, skip_base_filter=True) return refreshed if refreshed else task # Update cached status (no DB read needed - we just wrote IN_PROGRESS) @@ -520,7 +526,7 @@ def _execute_inline( ) # Refresh once at end to return current state - final_task = TaskDAO.find_one_or_none(uuid=task_uuid) + final_task = TaskDAO.find_one_or_none(uuid=task_uuid, skip_base_filter=True) return final_task if final_task else task except Exception as ex: @@ -542,7 +548,7 @@ def _execute_inline( ) # Refresh once at end to return current state - final_task = TaskDAO.find_one_or_none(uuid=task_uuid) + final_task = TaskDAO.find_one_or_none(uuid=task_uuid, skip_base_filter=True) return final_task if final_task else task finally: @@ -552,7 +558,9 @@ def _execute_inline( # Publish completion notification for any waiters # Use final_task if set by try/except, otherwise refresh (fallback) if final_task is None: - final_task = TaskDAO.find_one_or_none(uuid=task_uuid) + final_task = TaskDAO.find_one_or_none( + uuid=task_uuid, skip_base_filter=True + ) if final_task and final_task.status in TERMINAL_STATES: TaskManager.publish_completion(task_uuid, final_task.status) diff --git a/superset/tasks/filters.py b/superset/tasks/filters.py index f08619c4dfe2..9159a465969a 100644 --- a/superset/tasks/filters.py +++ b/superset/tasks/filters.py @@ -33,20 +33,35 @@ class TaskFilter(BaseFilter): # pylint: disable=too-few-public-methods owned and shared tasks. Unsubscribing removes visibility. Admins see all tasks without filtering. + + This filter applies to request-scoped reads only -- the REST API and + the MCP task tools -- where a task's visibility to the requesting + principal matters. Internal task-executor and scheduler code that + reads back the state of a task it already owns (e.g. polling for the + terminal status of the task it is currently executing) calls the DAO + with ``skip_base_filter=True`` instead: that code isn't presenting + task data to a user, and the UUID it operates on is never + caller-supplied, so the visibility check doesn't apply. """ def apply(self, query: Query, value: Any) -> Query: """Apply the filter to the query.""" - from sqlalchemy import and_, select + from flask import has_request_context + from sqlalchemy import and_, false, select from superset import security_manager from superset.models.task_subscribers import TaskSubscriber from superset.models.tasks import Task - # If user is admin or no user_id, return unfiltered query. - # This typically applies to background tasks and system operations user_id = get_user_id() - if not user_id or security_manager.is_admin(): + if not user_id: + # Within a request, a principal without a user id gets no tasks; + # background jobs run outside a request context and are unfiltered. + if has_request_context(): + return query.filter(false()) + return query + + if security_manager.is_admin(): return query is_subscribed = ( diff --git a/superset/tasks/manager.py b/superset/tasks/manager.py index 6778ea031812..08adb639a9ce 100644 --- a/superset/tasks/manager.py +++ b/superset/tasks/manager.py @@ -259,10 +259,15 @@ def time_remaining() -> float | None: return remaining if remaining > 0 else 0 def get_task() -> "Task | None": + # Reads back the task named by the caller's own task_uuid, not + # a user-requested lookup; see TaskFilter for the + # request-scoped vs. internal-plumbing split. if app and not has_app_context(): with app.app_context(): - return TaskDAO.find_one_or_none(uuid=task_uuid) - return TaskDAO.find_one_or_none(uuid=task_uuid) + return TaskDAO.find_one_or_none( + uuid=task_uuid, skip_base_filter=True + ) + return TaskDAO.find_one_or_none(uuid=task_uuid, skip_base_filter=True) # Check current state first task = get_task() @@ -478,7 +483,9 @@ def _check_abort_status(cls, task_uuid: UUID) -> bool: """ from superset.daos.tasks import TaskDAO - task = TaskDAO.find_one_or_none(uuid=task_uuid) + # Internal control-flow check on the task the executor is already + # running, not a user-facing lookup; see TaskFilter. + task = TaskDAO.find_one_or_none(uuid=task_uuid, skip_base_filter=True) return task is not None and task.status in ABORT_STATES @classmethod diff --git a/superset/tasks/scheduler.py b/superset/tasks/scheduler.py index 2a65ba1de9c4..5a7485497f3c 100644 --- a/superset/tasks/scheduler.py +++ b/superset/tasks/scheduler.py @@ -311,7 +311,11 @@ def execute_task( # noqa: C901 # Convert string UUID to native UUID (Celery deserializes as string) native_uuid = UUID(task_uuid) - task = TaskDAO.find_one_or_none(uuid=native_uuid) + # Internal executor path: load the task Celery was dispatched to run, + # keyed on the UUID passed at enqueue time, not a user-requested + # lookup; see TaskFilter for the request-scoped vs. internal-plumbing + # split. The refreshes below load the same task for the same reason. + task = TaskDAO.find_one_or_none(uuid=native_uuid, skip_base_filter=True) if not task: logger.error("Task %s not found in metastore", task_uuid) return {"status": "error", "message": "Task not found"} @@ -346,7 +350,7 @@ def execute_task( # noqa: C901 task_type, task_uuid, ) - refreshed = TaskDAO.find_one_or_none(uuid=native_uuid) + refreshed = TaskDAO.find_one_or_none(uuid=native_uuid, skip_base_filter=True) return { "status": refreshed.status if refreshed else "unknown", "task_uuid": task_uuid, @@ -489,7 +493,7 @@ def execute_task( # noqa: C901 ) # Refresh to get final status for return value and completion notification - refreshed = TaskDAO.find_one_or_none(uuid=native_uuid) + refreshed = TaskDAO.find_one_or_none(uuid=native_uuid, skip_base_filter=True) final_status = refreshed.status if refreshed else "unknown" # Publish completion notification for any waiters (e.g., sync callers) diff --git a/superset/utils/link_redirect.py b/superset/utils/link_redirect.py index 8707ae28ec0f..ed511f256d88 100644 --- a/superset/utils/link_redirect.py +++ b/superset/utils/link_redirect.py @@ -140,11 +140,17 @@ def is_safe_redirect_url(url: str) -> bool: # following a Location header). stripped = _URL_STRIPPED_CONTROL_CHARS.sub("", url.strip()) - # Block protocol-relative URLs - if stripped.startswith("//") or stripped.startswith("\\\\"): + # WHATWG URL parsers treat backslashes as forward slashes in special + # schemes, while urllib does not. Normalize backslashes to slashes before + # every structural check, mirroring Django's + # ``url_has_allowed_host_and_scheme``. + normalized = stripped.replace("\\", "/") + + # Block protocol-relative URLs (any leading mix of slash and backslash) + if normalized.startswith("//"): return False - parsed = urlparse(stripped) + parsed = urlparse(normalized) # Relative paths are safe if not parsed.scheme and not parsed.netloc: diff --git a/superset/utils/network.py b/superset/utils/network.py index 71fd946dbacd..819a5ffbf258 100644 --- a/superset/utils/network.py +++ b/superset/utils/network.py @@ -44,6 +44,19 @@ PING_TIMEOUT = 5 +def is_safe_ip(ip: ipaddress.IPv4Address | ipaddress.IPv6Address) -> bool: + """ + Return True if a single IP address is public and globally routable. + + IPv4-mapped IPv6 addresses (e.g. ``::ffff:127.0.0.1``) are unwrapped so + they are checked against the IPv4 unsafe networks rather than bypassing + them. + """ + if isinstance(ip, ipaddress.IPv6Address) and ip.ipv4_mapped: + ip = ip.ipv4_mapped + return ip.is_global and not any(ip in net for net in _SSRF_UNSAFE_NETWORKS) + + def is_safe_host(host: str) -> bool: """ Return True if ``host`` resolves exclusively to public, globally-routable @@ -52,6 +65,10 @@ def is_safe_host(host: str) -> bool: Returns False if any resolved address falls within a private, loopback, link-local, or otherwise non-routable range. An unresolvable host also returns False. + + Name resolution here is independent of the resolution performed when a + connection is later opened, so callers that go on to fetch from ``host`` + should also validate the connected peer address (see ``is_safe_ip``). """ try: results = socket.getaddrinfo(host, None) @@ -64,11 +81,7 @@ def is_safe_host(host: str) -> bool: ip = ipaddress.ip_address(sockaddr[0]) except ValueError: return False - # Unwrap IPv4-mapped IPv6 addresses (e.g. ::ffff:127.0.0.1) so they - # are checked against the IPv4 unsafe networks rather than bypassing. - if isinstance(ip, ipaddress.IPv6Address) and ip.ipv4_mapped: - ip = ip.ipv4_mapped - if not ip.is_global or any(ip in net for net in _SSRF_UNSAFE_NETWORKS): + if not is_safe_ip(ip): return False return True diff --git a/superset/utils/screenshot_utils.py b/superset/utils/screenshot_utils.py index 121fc6b04439..365ff5b56246 100644 --- a/superset/utils/screenshot_utils.py +++ b/superset/utils/screenshot_utils.py @@ -144,6 +144,21 @@ class TiledScreenshotBudgetExceededError(ScreenshotTaskBudgetExceededError): ) CHART_ID_CLASS_PATTERN = r"\bdashboard-chart-id-(\d+)\b" +# ECharts paint marker. The frontend +# (plugins/plugin-chart-echarts/src/components/Echart.tsx) tags the canvas host +# ``.echarts-host`` and adds ``.echarts-render-finished`` only in the ECharts +# ``finished`` event -- the sole signal that the canvas is fully painted. +# ``.slice_container`` alone is a pre-paint signal (it mounts when data arrives, +# before the canvas is drawn; chartStatus/onRenderSuccess fire pre-paint too), so +# a holder that still contains an unpainted host is treated as not-yet-rendered and +# the report screenshot waits for it instead of capturing a blank chart. Only +# ECharts hosts are gated; DOM/SVG vizzes paint on commit and non-ECharts canvas +# vizzes (deck.gl/mapbox/etc.) have no ``.echarts-host`` so they are unaffected. +ECHARTS_UNPAINTED_HOST_SELECTOR = r".echarts-host:not(.echarts-render-finished)" +CHART_ERROR_OR_EMPTY_SELECTOR = ( + f"{ALERT_SELECTOR}, {EMPTY_SELECTOR}, {MISSING_CHART_SELECTOR}" +) + # Shared body for holder readiness and timeout diagnostics. A holder is ready # only after a terminal marker appears and its loading marker disappears. UNREADY_CHART_HOLDERS_JS_BODY = f""" @@ -158,8 +173,19 @@ class TiledScreenshotBudgetExceededError(ScreenshotTaskBudgetExceededError): '{SLICE_CONTAINER_SELECTOR}' ) !== null; const stillLoading = holder.querySelector('{LOADING_SELECTOR}') !== null; - const isReady = holder.querySelector('{TERMINAL_MARKER_SELECTOR}') !== null; - if (stillLoading || !isReady) {{ + const hasErrorOrEmpty = holder.querySelector( + '{CHART_ERROR_OR_EMPTY_SELECTOR}' + ) !== null; + const hasUnpaintedEchart = holder.querySelector( + '{ECHARTS_UNPAINTED_HOST_SELECTOR}' + ) !== null; + // Ready = a settled error/empty/missing state, or a slice container + // whose ECharts canvas has finished painting. An unpainted ECharts host + // keeps the holder unready so a blank chart is never captured. + const isReady = !stillLoading && ( + hasErrorOrEmpty || (hasSliceContainer && !hasUnpaintedEchart) + ); + if (!isReady) {{ const chartIdMatch = holder.className.match(/{CHART_ID_CLASS_PATTERN}/); const chartId = chartIdMatch ? chartIdMatch[1] : null; let state; @@ -167,6 +193,8 @@ class TiledScreenshotBudgetExceededError(ScreenshotTaskBudgetExceededError): state = 'spinner_mounted'; }} else if (stillLoading) {{ state = 'waiting_on_database'; + }} else if (hasSliceContainer && hasUnpaintedEchart) {{ + state = 'mounted_unpainted'; }} else {{ state = 'nothing_mounted'; }} @@ -208,6 +236,11 @@ class TiledScreenshotBudgetExceededError(ScreenshotTaskBudgetExceededError): ) !== null) {{ return {{ chartId, state: 'empty' }}; }} + if (hasSliceContainer && holder.querySelector( + '{ECHARTS_UNPAINTED_HOST_SELECTOR}' + ) !== null) {{ + return {{ chartId, state: 'mounted_unpainted' }}; + }} if (hasSliceContainer) {{ return {{ chartId, state: 'rendered' }}; }} @@ -237,7 +270,8 @@ class TiledScreenshotBudgetExceededError(ScreenshotTaskBudgetExceededError): const chart = document.querySelector('.chart-container'); return chart !== null && chart.querySelector('{LOADING_SELECTOR}') === null - && chart.querySelector('{TERMINAL_MARKER_SELECTOR}') !== null; + && chart.querySelector('{TERMINAL_MARKER_SELECTOR}') !== null + && chart.querySelector('{ECHARTS_UNPAINTED_HOST_SELECTOR}') === null; }} """ @@ -249,6 +283,9 @@ class TiledScreenshotBudgetExceededError(ScreenshotTaskBudgetExceededError): const chart = document.querySelector('.chart-container'); if (chart === null) {{ return 'missing'; }} if (chart.querySelector('{LOADING_SELECTOR}') !== null) {{ return 'loading'; }} + if (chart.querySelector('{ECHARTS_UNPAINTED_HOST_SELECTOR}') !== null) {{ + return 'mounted_unpainted'; + }} if (chart.querySelector('{TERMINAL_MARKER_SELECTOR}') !== null) {{ return 'terminal'; }} diff --git a/tests/unit_tests/commands/databases/oauth2_test.py b/tests/unit_tests/commands/databases/oauth2_test.py index c0c11dcdcc27..6958b90d82bb 100644 --- a/tests/unit_tests/commands/databases/oauth2_test.py +++ b/tests/unit_tests/commands/databases/oauth2_test.py @@ -74,6 +74,7 @@ def test_validate_success( mock_parameters: OAuth2ProviderResponseSchema, ) -> None: mocker.patch("superset.utils.oauth2.decode_oauth2_state", return_value=mock_state) + mocker.patch("superset.commands.database.oauth2.get_user_id", return_value=1) mocker.patch.object( DatabaseUserOAuth2TokensDAO, "get_database", @@ -95,6 +96,7 @@ def test_validate_database_not_found( "superset.utils.oauth2.decode_oauth2_state", return_value={"database_id": 999}, ) + mocker.patch("superset.commands.database.oauth2.get_user_id", return_value=1) mocker.patch.object(DatabaseUserOAuth2TokensDAO, "get_database", return_value=None) command = OAuth2StoreTokenCommand(mock_parameters) @@ -120,6 +122,7 @@ def test_run_success( "get_database", return_value=mock_database, ) + mocker.patch("superset.commands.database.oauth2.get_user_id", return_value=1) mocker.patch.object( DatabaseUserOAuth2TokensDAO, "find_one_or_none", @@ -155,6 +158,7 @@ def test_run_logs_token_exchange_failure( "get_database", return_value=mock_database, ) + mocker.patch("superset.commands.database.oauth2.get_user_id", return_value=1) mock_database.db_engine_spec.get_oauth2_token.side_effect = HTTPError( "provider-payload-sentinel" ) @@ -188,6 +192,7 @@ def test_run_existing_token( "get_database", return_value=mock_database, ) + mocker.patch("superset.commands.database.oauth2.get_user_id", return_value=1) existing_token = MagicMock() mocker.patch.object( DatabaseUserOAuth2TokensDAO, @@ -208,3 +213,23 @@ def test_run_existing_token( assert result == "new_token" mock_delete.assert_called_once_with([existing_token]) mock_create.assert_called_once() + + +def test_validate_rejects_state_not_bound_to_session( + mocker: MockerFixture, + mock_parameters: OAuth2ProviderResponseSchema, +) -> None: + """ + The callback must only store tokens for the user who initiated the + dance: a state minted for another user, or presented without an + authenticated session, is rejected before any token exchange. + """ + command = OAuth2StoreTokenCommand(mock_parameters) + + mocker.patch("superset.commands.database.oauth2.get_user_id", return_value=2) + with pytest.raises(OAuth2Error): + command.validate() + + mocker.patch("superset.commands.database.oauth2.get_user_id", return_value=None) + with pytest.raises(OAuth2Error): + command.validate() diff --git a/tests/unit_tests/commands/report/execute_test.py b/tests/unit_tests/commands/report/execute_test.py index 4120481d6388..d6f6a7dee6f8 100644 --- a/tests/unit_tests/commands/report/execute_test.py +++ b/tests/unit_tests/commands/report/execute_test.py @@ -46,6 +46,7 @@ ReportScheduleXlsxFailedError, ) from superset.commands.report.execute import ( + _should_build_execution_context, BaseReportState, log_report_delivery_phase, persist_owned_report_execution_terminal_error, @@ -3747,6 +3748,8 @@ def test_success_state_send_error_logs_and_reraises( mocker, ReportSuccessState, schedule_type=ReportScheduleType.REPORT ) mocker.patch.object(state, "send", side_effect=RuntimeError("send boom")) + mocker.patch.object(state, "is_in_error_grace_period", return_value=False) + mocker.patch.object(state, "send_error") mocker.patch.object(state, "update_report_schedule_and_log") with pytest.raises(RuntimeError, match="send boom"): @@ -3808,6 +3811,46 @@ def test_get_notification_content_alert_no_flag_skips_attachment( assert content.text is None +@pytest.mark.parametrize( + ("schedule_type", "report_format", "attach_flag", "expected"), + [ + # Reports always run under an execution context, regardless of format. + (ReportScheduleType.REPORT, ReportDataFormat.PNG, False, True), + (ReportScheduleType.REPORT, ReportDataFormat.PDF, False, True), + (ReportScheduleType.REPORT, ReportDataFormat.CSV, False, True), + (ReportScheduleType.REPORT, ReportDataFormat.TEXT, False, True), + # Alerts that deliver a rendered screenshot fail closed only when the + # ALERTS_ATTACH_REPORTS flag is on (otherwise no artifact is attached). + (ReportScheduleType.ALERT, ReportDataFormat.PNG, True, True), + (ReportScheduleType.ALERT, ReportDataFormat.PDF, True, True), + (ReportScheduleType.ALERT, ReportDataFormat.PNG, False, False), + (ReportScheduleType.ALERT, ReportDataFormat.PDF, False, False), + # CSV/text/xlsx alerts never deliver a rendered screenshot; they stay + # lenient even with the attach flag on. + (ReportScheduleType.ALERT, ReportDataFormat.CSV, True, False), + (ReportScheduleType.ALERT, ReportDataFormat.TEXT, True, False), + (ReportScheduleType.ALERT, ReportDataFormat.XLSX, True, False), + ], +) +@patch("superset.commands.report.execute.feature_flag_manager") +def test_should_build_execution_context( + mock_ff: MagicMock, + mocker: MockerFixture, + schedule_type: ReportScheduleType, + report_format: ReportDataFormat, + attach_flag: bool, + expected: bool, +) -> None: + """Only reports and rendered-screenshot alerts run fail closed under a + ReportExecutionContext; CSV/text alerts and flag-off alerts stay lenient.""" + mock_ff.is_feature_enabled.return_value = attach_flag + model = mocker.Mock(spec=ReportSchedule) + model.type = schedule_type + model.report_format = report_format + + assert _should_build_execution_context(model) is expected + + def test_create_log_success_commits(mocker: MockerFixture) -> None: """Successful create_log creates a log entry and commits.""" schedule = mocker.Mock(spec=ReportSchedule) @@ -4217,6 +4260,139 @@ def test_success_state_error_logged_when_send_error_raises( assert ReportState.ERROR in states +@pytest.mark.parametrize( + "schedule_type", + [ReportScheduleType.REPORT, ReportScheduleType.ALERT], +) +def test_success_state_send_failure_notifies_owner( + mocker: MockerFixture, + schedule_type: ReportScheduleType, +) -> None: + """A delivery failure from the Success/Grace path must notify the owner, + mirroring the first-run (ReportNotTriggeredErrorState) path — otherwise a + previously-successful schedule fails silently (e.g. once a screenshot + capture starts failing closed).""" + state = _make_state_instance( + mocker, ReportSuccessState, schedule_type=schedule_type + ) + # No retries configured (the default), so _handle_retry_or_error returns + # False immediately without sending anything. + mocker.patch.object(state, "is_in_grace_period", return_value=False) + mocker.patch.object(state, "is_in_error_grace_period", return_value=False) + mock_update = mocker.patch.object(state, "update_report_schedule_and_log") + mock_send_error = mocker.patch.object(state, "send_error") + if schedule_type == ReportScheduleType.ALERT: + mocker.patch( + "superset.commands.report.execute.AlertCommand" + ).return_value.run.return_value = (True, "triggered") + mocker.patch.object( + state, + "send", + side_effect=ReportScheduleScreenshotFailedError("blank screenshot"), + ) + + with pytest.raises(ReportScheduleScreenshotFailedError, match="blank screenshot"): + state.next() + + mock_send_error.assert_called_once() + # The owner-notification path must also persist a terminal ERROR state, + # not leave the schedule stuck in WORKING (mirrors how the grace-period + # sibling test asserts the recorded terminal state). + assert mock_update.call_args_list[-1].args[0] == ReportState.ERROR + + +def test_success_state_send_failure_skips_notification_in_error_grace( + mocker: MockerFixture, +) -> None: + """When inside the error grace period, the Success/Grace path logs ERROR + but suppresses the (throttled) error notification.""" + state = _make_state_instance( + mocker, ReportSuccessState, schedule_type=ReportScheduleType.REPORT + ) + mocker.patch.object(state, "is_in_error_grace_period", return_value=True) + mock_update = mocker.patch.object(state, "update_report_schedule_and_log") + mock_send_error = mocker.patch.object(state, "send_error") + mocker.patch.object( + state, + "send", + side_effect=ReportScheduleScreenshotFailedError("blank screenshot"), + ) + + with pytest.raises(ReportScheduleScreenshotFailedError): + state.next() + + mock_send_error.assert_not_called() + states = [call.args[0] for call in mock_update.call_args_list] + assert ReportState.ERROR in states + + +@pytest.mark.parametrize( + ("failure_kind", "expected_message"), + [ + ("superset_errors", "smtp down;retry failed"), + ("generic", "smtp down"), + ], +) +def test_success_state_send_error_failure_overwrites_marker( + mocker: MockerFixture, + failure_kind: str, + expected_message: str, +) -> None: + """When the Success/Grace path's own error notification fails, the + placeholder marker is overwritten with the real failure message before + ERROR is logged -- mirroring the first-run (ReportNotTriggeredErrorState) + path. A SupersetErrorsException contributes its joined error messages; any + other exception contributes its ``str()``.""" + from superset.errors import ErrorLevel, SupersetError, SupersetErrorType + from superset.exceptions import SupersetErrorsException + + if failure_kind == "superset_errors": + send_error_exc: Exception = SupersetErrorsException( + [ + SupersetError( + message="smtp down", + error_type=SupersetErrorType.REPORT_NOTIFICATION_ERROR, + level=ErrorLevel.ERROR, + ), + SupersetError( + message="retry failed", + error_type=SupersetErrorType.REPORT_NOTIFICATION_ERROR, + level=ErrorLevel.ERROR, + ), + ] + ) + else: + send_error_exc = RuntimeError("smtp down") + + state = _make_state_instance( + mocker, ReportSuccessState, schedule_type=ReportScheduleType.REPORT + ) + mocker.patch.object(state, "is_in_error_grace_period", return_value=False) + mock_update = mocker.patch.object(state, "update_report_schedule_and_log") + mock_send_error = mocker.patch.object( + state, "send_error", side_effect=send_error_exc + ) + mocker.patch.object( + state, + "send", + side_effect=ReportScheduleScreenshotFailedError("blank screenshot"), + ) + + with pytest.raises(ReportScheduleScreenshotFailedError, match="blank screenshot"): + state.next() + + mock_send_error.assert_called_once() + # The placeholder marker must be replaced by the real notification failure + # before the terminal ERROR row is written. + final_call = mock_update.call_args_list[-1] + assert final_call.args[0] == ReportState.ERROR + assert final_call.kwargs.get("error_message") == expected_message + assert ( + final_call.kwargs.get("error_message") + != REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER + ) + + def test_get_url_for_csv_uses_post_processed_type( app: SupersetApp, mocker: MockerFixture, diff --git a/tests/unit_tests/common/test_query_context_processor.py b/tests/unit_tests/common/test_query_context_processor.py index d81dd495e3ae..dc50853c039c 100644 --- a/tests/unit_tests/common/test_query_context_processor.py +++ b/tests/unit_tests/common/test_query_context_processor.py @@ -27,6 +27,7 @@ from superset.common.chart_data_timing import QueryDataResult, QueryTiming from superset.common.db_query_status import QueryStatus from superset.common.query_context_processor import QueryContextProcessor +from superset.exceptions import QueryObjectValidationError from superset.utils.core import GenericDataType from superset.utils.date_parser import get_past_or_future @@ -98,6 +99,25 @@ def processor(mock_query_context): return processor +def test_query_cache_key_binds_annotation_data_to_requesting_user(processor): + """The cache key for annotated queries must differ per requesting user.""" + query_obj = MagicMock() + query_obj.annotation_layers = [{"sourceType": "NATIVE", "name": "a", "value": 1}] + with ( + patch( + "superset.common.query_context_processor.get_user_id", + side_effect=[1, 2], + ), + patch("superset.common.query_context_processor.security_manager"), + ): + processor.query_cache_key(query_obj) + processor.query_cache_key(query_obj) + contexts = [ + call.kwargs["annotation_context"] for call in query_obj.cache_key.call_args_list + ] + assert contexts[0] != contexts[1] + + def test_get_data_table_like(processor, mock_query_context): df = pd.DataFrame({"col1": [1, 2, 3], "col2": ["a", "b", "c"]}) coltypes = [GenericDataType.NUMERIC, GenericDataType.STRING] @@ -2377,3 +2397,26 @@ def fake_query(dct: dict[str, Any]) -> MagicMock: # for #40501. Without the fix, inner_from/to_dttm == shifted dates. assert captured[0]["inner_from_dttm"] == pd.Timestamp("2026-05-01") assert captured[0]["inner_to_dttm"] == pd.Timestamp("2026-05-28") + + +def test_get_native_annotation_data_requires_annotation_read_access(): + """Native annotation layers are only served to users who can read them.""" + query_obj = MagicMock() + query_obj.annotation_layers = [{"sourceType": "NATIVE", "name": "a", "value": 1}] + with ( + patch( + "superset.common.query_context_processor.security_manager" + ) as security_manager_mock, + patch( + "superset.common.query_context_processor.AnnotationLayerDAO.find_by_ids", + return_value=[], + ) as find_by_ids_mock, + ): + # ``can_access`` is synchronous; force a plain Mock so the patched + # manager doesn't hand back a truthy coroutine that slips past the + # ``not can_access(...)`` guard. + security_manager_mock.can_access = MagicMock(return_value=False) + with pytest.raises(QueryObjectValidationError): + QueryContextProcessor.get_native_annotation_data(query_obj) + security_manager_mock.can_access.assert_called_once_with("can_read", "Annotation") + find_by_ids_mock.assert_not_called() diff --git a/tests/unit_tests/daos/test_tasks.py b/tests/unit_tests/daos/test_tasks.py index 8a5d77c69ede..a24ad870fd5e 100644 --- a/tests/unit_tests/daos/test_tasks.py +++ b/tests/unit_tests/daos/test_tasks.py @@ -19,6 +19,7 @@ from uuid import UUID import pytest +from pytest_mock import MockerFixture from sqlalchemy.orm.session import Session from superset_core.tasks.types import TaskProperties, TaskScope, TaskStatus @@ -395,9 +396,15 @@ def test_remove_subscriber_not_subscribed(session_with_task: Session) -> None: assert result is None -def test_get_status(session_with_task: Session) -> None: +def test_get_status(session_with_task: Session, mocker: MockerFixture) -> None: """Test get_status returns status string when task found by UUID""" from superset.daos.tasks import TaskDAO + from superset.models.task_subscribers import TaskSubscriber + + # get_status enforces the TaskFilter, so the polling user must be + # authenticated and subscribed to see the task. + mocker.patch("superset.tasks.filters.get_user_id", return_value=TEST_USER_ID) + mocker.patch("superset.security_manager.is_admin", return_value=False) task = create_task( session_with_task, @@ -405,6 +412,8 @@ def test_get_status(session_with_task: Session) -> None: task_key="status-task", status=TaskStatus.IN_PROGRESS, ) + session_with_task.add(TaskSubscriber(task_id=task.id, user_id=TEST_USER_ID)) + session_with_task.flush() result = TaskDAO.get_status(task.uuid) diff --git a/tests/unit_tests/databases/api_test.py b/tests/unit_tests/databases/api_test.py index 2ebb88f1ee14..cedf0b15b03c 100644 --- a/tests/unit_tests/databases/api_test.py +++ b/tests/unit_tests/databases/api_test.py @@ -710,6 +710,10 @@ def test_oauth2_happy_path( return_value=None, ) + mocker.patch( + "superset.commands.database.oauth2.get_user_id", + return_value=1, + ) state: OAuth2State = { "user_id": 1, "database_id": 1, @@ -786,6 +790,10 @@ def test_oauth2_permissions( return_value=None, ) + mocker.patch( + "superset.commands.database.oauth2.get_user_id", + return_value=1, + ) state: OAuth2State = { "user_id": 1, "database_id": 1, @@ -867,6 +875,10 @@ def test_oauth2_multiple_tokens( return_value=None, ) + mocker.patch( + "superset.commands.database.oauth2.get_user_id", + return_value=1, + ) state: OAuth2State = { "user_id": 1, "database_id": 1, diff --git a/tests/unit_tests/datasets/commands/importers/v1/import_test.py b/tests/unit_tests/datasets/commands/importers/v1/import_test.py index 85c026050d84..764a0be3b21a 100644 --- a/tests/unit_tests/datasets/commands/importers/v1/import_test.py +++ b/tests/unit_tests/datasets/commands/importers/v1/import_test.py @@ -2239,3 +2239,79 @@ def test_import_restore_blocked_by_active_twin_at_incoming_identity( assert "another active dataset" in str(excinfo.value) # Check-before-mutate: the failed import leaves the row soft-deleted. assert existing.deleted_at is not None + + +def test_peer_validating_connection_blocks_rebound_peer() -> None: + """ + The import fetch validates the connected peer address, so a hostname that + passes ``is_safe_host`` and then re-resolves to an internal address (DNS + rebinding) is rejected before any request bytes are sent. + """ + from http.client import HTTPConnection + from unittest.mock import MagicMock, patch + + from superset.commands.dataset.exceptions import DatasetForbiddenDataURI + from superset.commands.dataset.importers.v1.utils 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(DatasetForbiddenDataURI): + conn.connect() + + +def test_load_data_disables_proxy_when_internal_urls_disallowed( + mocker: MockerFixture, +) -> None: + """ + ``load_data`` builds its opener with an explicit no-proxy handler when + internal data URLs are disallowed, so a configured HTTP(S) proxy can't + intercept the connection the peer check validates. + """ + from superset.commands.dataset.importers.v1.utils import load_data + + current_app.config["DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS"] = False + + mocker.patch("superset.commands.dataset.importers.v1.utils.validate_data_uri") + mocker.patch( + "superset.examples.helpers.normalize_example_data_url", + side_effect=lambda uri: uri, + ) + mocker.patch( + "superset.commands.dataset.importers.v1.utils._convert_temporal_columns" + ) + mocker.patch("superset.commands.dataset.importers.v1.utils.db.session.connection") + mock_df = Mock() + mock_df.keys.return_value = [] + mocker.patch( + "superset.commands.dataset.importers.v1.utils.pd.read_csv", + return_value=mock_df, + ) + mock_opener = Mock() + mock_opener.open.return_value = io.BytesIO(b"") + mock_build_opener = mocker.patch( + "superset.commands.dataset.importers.v1.utils.request.build_opener", + return_value=mock_opener, + ) + + dataset = Mock(spec=SqlaTable) + dataset.columns = [] + dataset.table_name = "my_table" + dataset.schema = None + + database = Mock(spec=Database) + database.sqlalchemy_uri = current_app.config["SQLALCHEMY_DATABASE_URI"] + + load_data("https://example.org/data.csv", dataset, database) + + handlers = mock_build_opener.call_args.args + assert any( + isinstance(handler, request.ProxyHandler) and not handler.proxies # type: ignore[attr-defined] + for handler in handlers + ) diff --git a/tests/unit_tests/extensions/storage/test_api.py b/tests/unit_tests/extensions/storage/test_api.py index c5f81547e7d0..e835fb8443d1 100644 --- a/tests/unit_tests/extensions/storage/test_api.py +++ b/tests/unit_tests/extensions/storage/test_api.py @@ -78,6 +78,32 @@ def test_ephemeral_get_delegates_to_dao( ) +@patch("superset.extensions.storage.api.ExtensionEphemeralDAO") +@patch("superset.extensions.storage.utils.get_extensions") +def test_ephemeral_get_response_is_marked_no_store( + mock_get_ext: MagicMock, mock_dao: MagicMock, app: Flask +) -> None: + """Stored values are scoped to the requesting user, so responses built via + `response()` must never be cached (e.g. by a shared/CDN cache).""" + mock_get_ext.return_value = {"acme.dashboard": MagicMock()} + Babel(app) + app.appbuilder = MagicMock() + app.appbuilder.sm.is_item_public.return_value = True + mock_dao.get_raw.return_value = (get_codec("json").encode({"data": 42}), "json") + + with app.test_request_context( + "/api/v1/extensions/acme/dashboard/storage/ephemeral/my-key" + ): + g.user = MagicMock(id=7) + + body, status_code = ExtensionStorageRestApi().get_ephemeral( + "acme", "dashboard", "my-key" + ) + + assert status_code == 200 + assert body.cache_control.no_store is True + + @patch("superset.extensions.storage.api.ExtensionEphemeralDAO") @patch("superset.extensions.storage.utils.get_extensions") def test_ephemeral_get_returns_none_when_entry_missing( diff --git a/tests/unit_tests/extensions/test_cache_middleware.py b/tests/unit_tests/extensions/test_cache_middleware.py index e9398032d68c..7a5c0281943b 100644 --- a/tests/unit_tests/extensions/test_cache_middleware.py +++ b/tests/unit_tests/extensions/test_cache_middleware.py @@ -103,6 +103,17 @@ def test_unrelated_path_is_not_intercepted() -> None: assert headers == upstream +def test_storage_endpoints_are_not_intercepted() -> None: + """Per-user storage responses must keep Vary: Cookie for shared caches.""" + upstream = [("Vary", "Accept-Encoding, Cookie")] + for path in ( + "/api/v1/extensions/acme/my-ext/storage/ephemeral/some-key", + "/api/v1/extensions/acme/my-ext/storage/persistent/some-key", + ): + headers = call_middleware(path, upstream) + assert headers == upstream + + # --- Vary stripping logic --- diff --git a/tests/unit_tests/jinja_context_test.py b/tests/unit_tests/jinja_context_test.py index 4b067ddfb29e..2b50b9d6a847 100644 --- a/tests/unit_tests/jinja_context_test.py +++ b/tests/unit_tests/jinja_context_test.py @@ -1096,6 +1096,34 @@ def test_metric_macro_with_dataset_id(mocker: MockerFixture) -> None: mock_get_form_data.assert_not_called() +def test_metric_macro_guest_user_dataset_out_of_scope(mocker: MockerFixture) -> None: + """ + Test that ``metric_macro`` denies a guest user a dataset that is not + reachable through any dashboard their guest token grants. + """ + mocker.patch("superset.security_manager.is_guest_user", return_value=True) + guest_user = mocker.MagicMock() + guest_user.guest_token = {} + mocker.patch( + "superset.security_manager.get_current_guest_user_if_guest", + return_value=guest_user, + ) + DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 + DatasetDAO.find_by_id.return_value = SqlaTable( + id=1, + table_name="test_dataset", + metrics=[ + SqlMetric(metric_name="count", expression="COUNT(*)"), + ], + database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"), + schema="my_schema", + sql=None, + ) + env = SandboxedEnvironment(undefined=DebugUndefined) + with pytest.raises(DatasetNotFoundError): + metric_macro(env, {}, "count", 1) + + def test_metric_macro_recursive(mocker: MockerFixture) -> None: """ Test the ``metric_macro`` when the definition is recursive. @@ -1732,6 +1760,13 @@ def test_metric_macro_embedded_user_skips_base_filter(mocker: MockerFixture) -> mock_is_guest_user = mocker.patch("superset.security_manager.is_guest_user") mock_is_guest_user.return_value = True + # Dashboard-level guest scope is asserted separately; here the dataset is + # in scope so the test can focus on the base-filter bypass. + mocker.patch( + "superset.jinja_context.guest_user_can_access_dataset", + return_value=True, + ) + DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 DatasetDAO.find_by_id.return_value = SqlaTable( table_name="test_dataset", diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index fb9a3fca790d..5f19477ddedc 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -224,6 +224,69 @@ def test_raise_for_access_guest_user_ok_subset( sm.raise_for_access(query_context=query_context) +def test_raise_for_access_guest_user_deck_multi_child_requires_child_datasource( + mocker: MockerFixture, + app_context: None, +) -> None: + """ + The deck.gl multi-layer child leg must bind the requested datasource to + the child chart: a valid parent/child pair does not authorize querying + an arbitrary dataset. + """ + sm = SupersetSecurityManager(appbuilder) + mocker.patch.object(sm, "is_guest_user", return_value=True) + mocker.patch.object(sm, "can_access", return_value=False) + mocker.patch.object(sm, "can_access_schema", return_value=False) + mocker.patch.object(sm, "is_editor", return_value=False) + mocker.patch.object(sm, "can_access_dashboard", return_value=True) + mocker.patch.object(sm, "get_current_guest_user_if_guest", return_value=None) + mocker.patch( + "superset.is_feature_enabled", + side_effect=lambda feature: feature == "EMBEDDED_SUPERSET", + ) + mocker.patch( + "superset.security.manager.query_context_modified", + return_value=False, + ) + + child_datasource = mocker.MagicMock() + other_datasource = mocker.MagicMock() + + parent_slc = mocker.MagicMock() + parent_slc.params = json.dumps({"viz_type": "deck_multi", "deck_slices": [42]}) + child_slc = mocker.MagicMock() + child_slc.datasource = child_datasource + + dashboard = mocker.MagicMock() + dashboard.slices = [parent_slc] + + query_mock = mocker.patch.object(sm.session, "query") + query_mock.return_value.filter.return_value.one_or_none.side_effect = [ + dashboard, + parent_slc, + child_slc, + dashboard, + parent_slc, + child_slc, + ] + + query_context = mocker.MagicMock() + query_context.form_data = { + "dashboardId": 10, + "slice_id": 42, + "parent_slice_id": 41, + } + + # Requesting the child's own datasource is allowed. + query_context.datasource = child_datasource + sm.raise_for_access(query_context=query_context) + + # The same chart context with any other datasource is rejected. + query_context.datasource = other_datasource + with pytest.raises(SupersetSecurityException): + sm.raise_for_access(query_context=query_context) + + def test_raise_for_access_guest_user_tampered_id( mocker: MockerFixture, app_context: None, @@ -1542,6 +1605,32 @@ def test_query_context_modified_native_filter_arbitrary_saved_metric_blocked( assert query_context_modified(qc) +def test_query_context_modified_native_filter_series_limit_terms_blocked( + mocker: MockerFixture, +) -> None: + """A series-limit metric or series column beyond the target is modified.""" + query = SimpleNamespace( + columns=["region"], + metrics=[], + groupby=[], + series_columns=["region"], + series_limit=5, + series_limit_metric={ + "expressionType": "SIMPLE", + "column": {"column_name": "salary"}, + "aggregate": "MAX", + }, + ) + qc = _native_filter_ctx(mocker, [query]) + assert query_context_modified(qc) + + query = SimpleNamespace( + columns=["region"], metrics=[], groupby=[], series_columns=["ssn"] + ) + qc = _native_filter_ctx(mocker, [query]) + assert query_context_modified(qc) + + def test_query_context_modified_native_filter_orderby_arbitrary_column_blocked( mocker: MockerFixture, ) -> None: diff --git a/tests/unit_tests/tasks/test_filters.py b/tests/unit_tests/tasks/test_filters.py new file mode 100644 index 000000000000..8ea33a19903c --- /dev/null +++ b/tests/unit_tests/tasks/test_filters.py @@ -0,0 +1,44 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from unittest.mock import MagicMock + +from flask import current_app +from pytest_mock import MockerFixture +from sqlalchemy import false + +from superset.tasks.filters import TaskFilter + + +def test_task_filter_fails_closed_for_request_without_user_id( + mocker: MockerFixture, + app_context: None, +) -> None: + """ + A request-bound principal without a user id (anonymous or guest user) + must not receive the unfiltered task list. + """ + mocker.patch("superset.tasks.filters.get_user_id", return_value=None) + task_filter = TaskFilter("id", MagicMock()) + query = MagicMock() + + with current_app.test_request_context("/api/v1/task/"): + filtered = task_filter.apply(query, None) + + assert filtered is not query + query.filter.assert_called_once() + (predicate,) = query.filter.call_args.args + assert str(predicate) == str(false()) diff --git a/tests/unit_tests/utils/test_link_redirect.py b/tests/unit_tests/utils/test_link_redirect.py index bad2658d59ac..5549b90345c6 100644 --- a/tests/unit_tests/utils/test_link_redirect.py +++ b/tests/unit_tests/utils/test_link_redirect.py @@ -170,3 +170,21 @@ def test_safe_path_with_tab_in_internal_segment(app: Flask) -> None: """A tab inside a regular path segment is still a relative URL after stripping; it must not flip the result to safe-then-unsafe.""" assert is_safe_redirect_url("/dashboard/1?from=tab%09inside") + + +@pytest.mark.parametrize( + "url", + [ + "/\\evil.com", # slash-backslash + "\\/evil.com", # backslash-slash + "\\\\evil.com", # double backslash + "/\\/evil.com", # slash-backslash-slash + "/%09/\\evil.com", # browser-stripped TAB then slash-backslash + "https:/\\evil.com", # backslash inside an absolute URL + ], +) +def test_unsafe_backslash_protocol_relative(app: Flask, url: str) -> None: + """WHATWG URL parsers treat backslashes as forward slashes in special + schemes, so any leading mix of slash and backslash is navigated as a + protocol-relative URL and must be rejected.""" + assert not is_safe_redirect_url(url) diff --git a/tests/unit_tests/utils/test_network.py b/tests/unit_tests/utils/test_network.py index afb557279e4a..2db44f8ba487 100644 --- a/tests/unit_tests/utils/test_network.py +++ b/tests/unit_tests/utils/test_network.py @@ -14,11 +14,44 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +import ipaddress from unittest.mock import patch import pytest -from superset.utils.network import is_safe_host +from superset.utils.network import is_safe_host, is_safe_ip + + +@pytest.mark.parametrize( + ("ip", "expected"), + [ + # Public → safe + ("93.184.216.34", True), + ("8.8.8.8", True), + ("2606:2800:220:1:248:1893:25c8:1946", True), + # Loopback → unsafe + ("127.0.0.1", False), + ("::1", False), + # RFC-1918 private ranges → unsafe + ("10.0.0.1", False), + ("172.16.0.1", False), + ("192.168.0.1", False), + # Link-local / IMDS → unsafe + ("169.254.169.254", False), + # CGNAT (RFC 6598) → unsafe + ("100.100.100.200", False), + # Multicast → unsafe, despite ip.is_global being True for these + ("224.0.0.1", False), + ("ff02::1", False), + # IPv4-mapped IPv6 → unwrapped and checked against IPv4 ranges + ("::ffff:127.0.0.1", False), + ("::ffff:8.8.8.8", True), + ], +) +def test_is_safe_ip(ip: str, expected: bool) -> None: + """`is_safe_ip` must classify individual addresses directly, independent + of hostname resolution.""" + assert is_safe_ip(ipaddress.ip_address(ip)) is expected @pytest.mark.parametrize( diff --git a/tests/unit_tests/utils/test_screenshot_utils.py b/tests/unit_tests/utils/test_screenshot_utils.py index cb772e45d78a..13ecb2d62881 100644 --- a/tests/unit_tests/utils/test_screenshot_utils.py +++ b/tests/unit_tests/utils/test_screenshot_utils.py @@ -1148,3 +1148,21 @@ def test_per_tile_timing_debug_line_logged(self, mock_page): assert args[1] == i + 1 # tile index assert args[2] == 3 # total tiles assert args[-1] == " [cache_key=xyz]" + + +def test_readiness_predicates_gate_on_unpainted_echarts_hosts() -> None: + """The report gate, the single-chart gate, and the diagnostics query all + key on the ECharts paint marker so a pre-paint canvas is never captured.""" + from superset.utils.screenshot_utils import ( + CHART_CONTAINER_READY_JS, + ECHARTS_UNPAINTED_HOST_SELECTOR, + FIND_CHART_HOLDER_STATES_JS, + REPORT_CHART_HOLDERS_READY_JS, + ) + + assert ( + ECHARTS_UNPAINTED_HOST_SELECTOR == ".echarts-host:not(.echarts-render-finished)" + ) + assert ECHARTS_UNPAINTED_HOST_SELECTOR in REPORT_CHART_HOLDERS_READY_JS + assert ECHARTS_UNPAINTED_HOST_SELECTOR in CHART_CONTAINER_READY_JS + assert "mounted_unpainted" in FIND_CHART_HOLDER_STATES_JS