bito-code-review[bot] commented on code in PR #44800: URL: https://github.com/apache/superset/pull/44800#discussion_r4152332553
########## superset/mcp_service/dashboard_scope.py: ########## @@ -0,0 +1,1355 @@ +# 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. + +"""Dashboard filter scope for MCP tool calls. + +A client answering questions about a dashboard the user is looking at (an AI +assistant embedded next to it, for example) sends that dashboard's active +filters in the ``X-Superset-Dashboard-Scope`` request header. The header rides +every HTTP request of the conversation turn, so the model driving the tool +calls can neither see nor remove it, and a retried call carries it again. + +Scope is a *correctness* control: answers must reflect the filtered view the +user sees. It is not an authorization boundary. RBAC, dataset permissions and +row-level security are applied by the normal execution paths exactly as +without a scope, and nothing here relaxes or replaces them. + +Enforcement happens once per tool call in ``mcp_auth_hook`` (see +``apply_call_dashboard_scope``), before the tool runs. Every tool is in exactly +one class while a scope is active: + +* rewritten: the tool's request is rewritten so the scope is AND-composed with + whatever the model supplied. The rewrite happens before the tool builds a + query, so every internal path and every cache key sees the scoped request. +* gated: allowed unless the particular call would return data the scope cannot + be applied to, in which case it is refused. +* neutral: returns no dataset rows (metadata, links, mutations), unchanged. +* anything else, including extension tools and tools added later: refused. + +A refusal is always explicit. Scope that cannot be applied is never dropped. +""" + +from __future__ import annotations + +import base64 +import binascii +import logging +import zlib +from collections.abc import Callable, Mapping, Sequence +from copy import deepcopy +from dataclasses import dataclass +from datetime import datetime +from typing import Any, TYPE_CHECKING, TypeGuard + +from fastmcp.exceptions import ToolError + +from superset.constants import ( + EXTRA_FORM_DATA_APPEND_KEYS, + EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS, + NO_TIME_RANGE, +) +from superset.utils import json + +if TYPE_CHECKING: + import inspect + + from superset.models.slice import Slice + +logger = logging.getLogger(__name__) + +HEADER_NAME = "X-Superset-Dashboard-Scope" +SCOPE_VERSION = 1 +# The header is compressed by the sender; these bound what a request can make +# the server allocate before any scope logic runs. +MAX_HEADER_LENGTH = 64 * 1024 +MAX_PAYLOAD_BYTES = 1024 * 1024 + +REFUSAL_PREFIX = "Dashboard filter scope refused this call:" +_NO_QUERY = "No query was run." + +# extra_form_data keys whose value decides which rows a query includes. A model +# may repeat the scope's value but never replace it. +ROW_OVERRIDE_KEYS = frozenset( + {"time_range", "granularity_sqla", "time_column", "relative_start", "relative_end"} +) +# Keys that choose the column a time range applies to. Setting one while the +# scope carries filters would move the dashboard window to another column, and +# Superset's granularity handling drops existing filters on the chosen column. +TIME_TARGET_KEYS = frozenset({"granularity_sqla", "time_column"}) +# Keys that only change presentation (bucketing, interactivity), never which +# rows are included. The model's value wins. +PRESENTATION_KEYS = frozenset( + set(EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS) - ROW_OVERRIDE_KEYS +) | (set(EXTRA_FORM_DATA_APPEND_KEYS) - {"filters", "adhoc_filters"}) + +# Filter operators the dataset and SQL paths know how to apply. Anything else +# a dashboard sends is refused on those paths rather than guessed at. +SUPPORTED_OPERATORS = frozenset( + { + "==", + "!=", + ">", + "<", + ">=", + "<=", + "IN", + "NOT IN", + "IS NULL", + "IS NOT NULL", + "LIKE", + "ILIKE", + } +) +TEMPORAL_RANGE = "TEMPORAL_RANGE" + +# Preview formats that carry query results rather than a link. +DATA_PREVIEW_FORMATS = frozenset({"ascii", "table", "vega_lite"}) + + +class MCPDashboardScopeError(ToolError): + """A tool call refused because the dashboard scope cannot be applied to it. + + Subclasses ``ToolError`` so the message reaches the caller verbatim and the + model can explain the limitation instead of retrying blindly. + """ + + def __init__(self, reason: str, guidance: str = "") -> None: + message = f"{REFUSAL_PREFIX} {reason} {_NO_QUERY}" + if guidance: + message = f"{message} {guidance}" + super().__init__(message) + + +_USE_CHART_TOOLS = ( + "Use get_chart_data or get_dashboard_data on the dashboard's charts, which " + "apply the filters each chart shows, or ask the user to change the " + "dashboard filters." +) +_ASK_USER = ( + "Explain that the active dashboard filters cannot be applied to this " + "request, and do not present unfiltered results as the filtered answer." +) + + +@dataclass(frozen=True) +class DashboardScope: + """The active filters of the dashboard a request is scoped to.""" + + dashboard_id: int + # Chart id -> the extra_form_data the dashboard applies to that chart. + # Charts on the dashboard without an entry have no active filters. + chart_filters: Mapping[int, Mapping[str, Any]] + + @property + def has_constraints(self) -> bool: + """True when any chart's filters restrict which rows are included.""" + return any(_restricts_rows(efd) for efd in self.chart_filters.values()) + + +@dataclass(frozen=True) +class DashboardConstraints: + """Constraints for queries not tied to one dashboard chart. + + Derived from the queried dataset's charts, or all charts for other datasets. + + ``clauses`` are ``{"col", "op", "val"}`` dicts AND-ed into the query. + ``time_range`` is the dashboard time window; ``time_column`` names the + column it applies to. A time window without a resolved column must be + refused by dataset and SQL queries. + """ + + clauses: tuple[dict[str, Any], ...] + time_range: str | None + time_column: str | None + + @property + def is_empty(self) -> bool: + return not self.clauses and self.time_range is None + + @property + def columns(self) -> set[str]: + return {clause["col"] for clause in self.clauses} + + +def _restricts_rows(extra_form_data: Mapping[str, Any]) -> bool: + return bool( + extra_form_data.get("filters") + or extra_form_data.get("adhoc_filters") + or _time_range(extra_form_data) is not None + ) + + +def _time_range(extra_form_data: Mapping[str, Any]) -> str | None: + value = extra_form_data.get("time_range") + if value is None or value == NO_TIME_RANGE: + return None + return value + + +# --------------------------------------------------------------------------- +# Header decoding +# --------------------------------------------------------------------------- + + +def _malformed(detail: str) -> MCPDashboardScopeError: + return MCPDashboardScopeError( + f"the {HEADER_NAME} request header is malformed ({detail}).", + "This is a client integration error; the request cannot be answered " + "until the client sends a valid dashboard scope.", + ) + + +def decode_dashboard_scope(value: str) -> DashboardScope: + """Decode ``base64url(zlib(JSON))`` into a validated ``DashboardScope``. + + Any deviation is an error rather than "no scope": a caller that sent a + scope expects it applied, so a garbled one must not silently widen answers. + """ + if len(value) > MAX_HEADER_LENGTH: + raise _malformed("too large") + try: + compressed = base64.b64decode( + value.strip() + "=" * (-len(value.strip()) % 4), + altchars=b"-_", + validate=True, + ) + except (binascii.Error, ValueError) as ex: + raise _malformed("not base64url") from ex + + decompressor = zlib.decompressobj() + try: + raw = decompressor.decompress(compressed, MAX_PAYLOAD_BYTES + 1) + except zlib.error as ex: + raise _malformed("not zlib-compressed") from ex + if len(raw) > MAX_PAYLOAD_BYTES or decompressor.unconsumed_tail: + raise _malformed("payload too large") + if not decompressor.eof: + raise _malformed("truncated payload") + + try: + payload = json.loads(raw.decode("utf-8")) + except (UnicodeDecodeError, ValueError) as ex: + raise _malformed("not JSON") from ex + return _parse_payload(payload) + + +def _parse_payload(payload: Any) -> DashboardScope: + if not isinstance(payload, dict): + raise _malformed("payload must be a JSON object") + if payload.get("version") != SCOPE_VERSION: + raise _malformed(f"unsupported version {payload.get('version')!r}") + dashboard_id = payload.get("dashboard_id") + if not _is_positive_int(dashboard_id): + raise _malformed("dashboard_id must be a positive integer") + return DashboardScope( + dashboard_id=dashboard_id, + chart_filters=_parse_chart_filters(payload.get("chart_filters")), + ) + + +def _parse_chart_filters(raw_filters: Any) -> dict[int, Mapping[str, Any]]: + if raw_filters is None: + return {} + if not isinstance(raw_filters, dict): + raise _malformed("chart_filters must be an object") + chart_filters: dict[int, Mapping[str, Any]] = {} + for key, extra_form_data in raw_filters.items(): + chart_id = _chart_id(key) + if chart_id is None: + raise _malformed(f"chart id {key!r} is not a positive integer") + if not isinstance(extra_form_data, dict): + raise _malformed(f"filters for chart {chart_id} must be an object") + # Checked by type, not truthiness: a falsy non-list such as ``false`` + # must not read as "no filters" and silently widen answers. + for list_key in ("filters", "adhoc_filters"): + if not isinstance(extra_form_data.get(list_key, []), (list, type(None))): + raise _malformed(f"{list_key} for chart {chart_id} must be a list") + for str_key in sorted(ROW_OVERRIDE_KEYS): + if not isinstance(extra_form_data.get(str_key), (str, type(None))): + raise _malformed(f"{str_key} for chart {chart_id} must be a string") + chart_filters[chart_id] = extra_form_data + return chart_filters + + +def _is_positive_int(value: Any) -> TypeGuard[int]: + return isinstance(value, int) and not isinstance(value, bool) and value > 0 + + +def _chart_id(value: Any) -> int | None: + if _is_positive_int(value): + return value + if isinstance(value, str) and value.isdecimal() and int(value) > 0: + return int(value) + return None + + +def _scope_header_values() -> list[str]: + """Every value of the scope header on the current MCP HTTP request.""" + from fastmcp.server.dependencies import get_http_request + + try: + request = get_http_request() + except RuntimeError: + # stdio / in-process transports carry no HTTP headers. + return [] + return request.headers.getlist(HEADER_NAME) + + +def has_dashboard_scope_header() -> bool: + """True when the current request carries a scope header, valid or not.""" + return bool(_scope_header_values()) + + +def get_request_dashboard_scope() -> DashboardScope | None: + """The scope of the current request, or None when it carries none.""" + values = _scope_header_values() + if not values: + return None + if len(values) > 1: + raise _malformed("sent more than once") + return decode_dashboard_scope(values[0]) + + +# --------------------------------------------------------------------------- +# Per-chart composition +# --------------------------------------------------------------------------- + + +def _clause_key(clause: Any) -> str: + return json.dumps(clause, sort_keys=True, default=str) + + +def _dedupe(clauses: list[Any]) -> list[Any]: + seen: set[str] = set() + result = [] + for clause in clauses: + key = _clause_key(clause) + if key not in seen: + seen.add(key) + result.append(clause) + return result + + +def _column_names(column: Any) -> set[str]: + """Names a filter column can be matched by: the name itself, or an adhoc + column's label and SQL expression.""" + if isinstance(column, str): + return {column} + if isinstance(column, dict): + return { + value + for value in (column.get("label"), column.get("sqlExpression")) + if isinstance(value, str) + } + return set() + + +def _clauses(extra_form_data: Mapping[str, Any]) -> list[tuple[Any, Any]]: + """(column, operator) for every filter and adhoc filter clause.""" + clauses = [ + (clause.get("col"), clause.get("op")) + for clause in extra_form_data.get("filters") or [] + if isinstance(clause, dict) + ] + clauses += [ + (clause.get("subject"), clause.get("operator")) + for clause in extra_form_data.get("adhoc_filters") or [] + if isinstance(clause, dict) + ] + return clauses + + +def _clause_columns(extra_form_data: Mapping[str, Any]) -> set[str]: + return { + name + for column, _ in _clauses(extra_form_data) + for name in _column_names(column) + } + + +def _constrains_time(extra_form_data: Mapping[str, Any]) -> bool: + """True when the scope limits the time window: a time range, or a + TEMPORAL_RANGE clause such as a cross-filter on a time-series axis.""" + return _time_range(extra_form_data) is not None or any( + op == TEMPORAL_RANGE for _, op in _clauses(extra_form_data) + ) + + +def _check_model_temporal_filters( + scope_efd: Mapping[str, Any], model_efd: Mapping[str, Any] +) -> None: + """Refuse model TEMPORAL_RANGE clauses that Superset could apply in place + of the scope's filters. + + Query construction keeps one time window per granularity column and drops + other filters on that column, so a model temporal clause may replace the + dashboard's window or remove a dashboard filter on the same column. + """ + # Clauses the scope already carries (e.g. a client re-sending the chart's + # filters) add nothing and are ignored. + scope_keys = { + _clause_key(clause) + for key in ("filters", "adhoc_filters") + for clause in scope_efd.get(key) or [] + } + added = { + key: [ + clause + for clause in model_efd.get(key) or [] + if _clause_key(clause) not in scope_keys + ] + for key in ("filters", "adhoc_filters") + } + temporal = [column for column, op in _clauses(added) if op == TEMPORAL_RANGE] + if not temporal or not _restricts_rows(scope_efd): + return + if _constrains_time(scope_efd): + raise MCPDashboardScopeError( + "a TEMPORAL_RANGE filter cannot be added while the dashboard limits " + "the time range for this chart.", + _ASK_USER, + ) + scope_columns = _clause_columns(scope_efd) + for column in temporal: + if not isinstance(column, str) or column in scope_columns: + raise MCPDashboardScopeError( + f"a TEMPORAL_RANGE filter on {column!r} would replace the " + "dashboard's filter on that column.", + _ASK_USER, + ) + + +def compose_extra_form_data( + scope_efd: Mapping[str, Any] | None, + model_efd: Mapping[str, Any] | None, +) -> dict[str, Any]: + """AND-compose a chart's dashboard filters with model-supplied ones. + + The scope is a floor the model cannot lower: filter lists are + concatenated, and a row-constraining override the scope sets cannot be + changed. An empty or absent model payload yields the scope unchanged. + """ + scope_efd = dict(scope_efd or {}) + if model_efd is None: + return scope_efd + if not isinstance(model_efd, Mapping): + raise MCPDashboardScopeError( + "extra_form_data must be an object while dashboard filters apply." + ) + + _check_model_temporal_filters(scope_efd, model_efd) + + composed = dict(scope_efd) + for key, value in model_efd.items(): + _merge_model_value(composed, scope_efd, key, value) + return composed + + +def _merge_model_value( + composed: dict[str, Any], scope_efd: Mapping[str, Any], key: str, value: Any +) -> None: + """Merge one model-supplied extra_form_data key into ``composed``.""" + if key in ("filters", "adhoc_filters"): + if not isinstance(value, list): + raise MCPDashboardScopeError(f"extra_form_data.{key} must be a list.") + composed[key] = _dedupe([*(scope_efd.get(key) or []), *value]) + elif key in ROW_OVERRIDE_KEYS: + _check_row_override(scope_efd, key, value) + composed[key] = value + elif key in PRESENTATION_KEYS: + current = composed.get(key) + if isinstance(current, list) and isinstance(value, list): + composed[key] = [*current, *value] + elif isinstance(current, dict) and isinstance(value, dict): + composed[key] = {**current, **value} + else: + composed[key] = value + elif key in scope_efd and value != scope_efd[key]: + raise MCPDashboardScopeError( + f"extra_form_data.{key} is set by the dashboard and cannot be changed.", + _ASK_USER, + ) + else: + composed[key] = value + + +def _check_row_override(scope_efd: Mapping[str, Any], key: str, value: Any) -> None: + """Refuse a model override that would change which rows the scope keeps.""" + if key in scope_efd: + if value != scope_efd[key]: + raise MCPDashboardScopeError( + f"the dashboard sets {key}={scope_efd[key]!r} for this chart and " + f"it cannot be changed to {value!r}.", + _ASK_USER, + ) + elif key in TIME_TARGET_KEYS and _restricts_rows(scope_efd): + raise MCPDashboardScopeError( + f"{key} cannot be set while dashboard filters apply, because it " + "changes which column the dashboard's filters and time range " + "constrain.", + _ASK_USER, + ) + elif key not in TIME_TARGET_KEYS and _constrains_time(scope_efd): + # time_range or a relative anchor would move a window the dashboard + # set through a temporal cross-filter rather than a time range. + raise MCPDashboardScopeError( + f"{key} cannot be set while the dashboard limits the time range for " + "this chart.", + _ASK_USER, + ) + + +# --------------------------------------------------------------------------- +# Dashboard-wide constraints (dataset and SQL paths) +# --------------------------------------------------------------------------- + + +def _normalize_filter_clause(clause: Any, chart_id: int) -> dict[str, Any]: + if not isinstance(clause, dict) or not isinstance(clause.get("col"), str): + raise MCPDashboardScopeError( + f"a dashboard filter on chart {chart_id} targets an expression " + "rather than a column, which can only be applied through that chart.", + _USE_CHART_TOOLS, + ) + return {"col": clause["col"], "op": clause.get("op"), "val": clause.get("val")} + + +def _normalize_adhoc_clause(clause: Any, chart_id: int) -> dict[str, Any]: + if ( + not isinstance(clause, dict) + or clause.get("expressionType") != "SIMPLE" + or str(clause.get("clause") or "WHERE").upper() != "WHERE" + or not isinstance(clause.get("subject"), str) + ): + raise MCPDashboardScopeError( + f"a dashboard filter on chart {chart_id} is a custom SQL or HAVING " + "filter, which can only be applied through that chart.", + _USE_CHART_TOOLS, + ) + return { + "col": clause["subject"], + "op": clause.get("operator"), + "val": clause.get("comparator"), + } + + +def _validate_clause(clause: dict[str, Any]) -> None: + if (op := clause["op"]) not in SUPPORTED_OPERATORS: + raise MCPDashboardScopeError( + f"the dashboard filter operator {op!r} on column {clause['col']!r} " + "can only be applied through the dashboard's charts.", + _USE_CHART_TOOLS, + ) + values = clause["val"] if isinstance(clause["val"], list) else [clause["val"]] + for value in values: + if value is not None and not isinstance(value, (str, int, float, bool)): + raise MCPDashboardScopeError( + f"the dashboard filter on column {clause['col']!r} has a value " + "that can only be applied through the dashboard's charts.", + _USE_CHART_TOOLS, + ) + + +def _dataset_chart_filters( + scope: DashboardScope, dataset_ids: set[int], charts: list[Slice] +) -> Mapping[int, Mapping[str, Any]]: + """Select filters for charts on these datasets, or all charts if none match.""" + chart_ids = { + chart.id + for chart in charts + if chart.datasource_type == "table" and chart.datasource_id in dataset_ids + } + if not chart_ids: + return scope.chart_filters + return { + chart_id: efd + for chart_id, efd in scope.chart_filters.items() + if chart_id in chart_ids + } + + +def _validate_saved_chart_filters( + form_data: Mapping[str, Any], *, chart_id: int, append_filters: bool +) -> None: + """Refuse malformed saved filter lists used by the dashboard override merge.""" + # The merge reads adhoc_filters and appends dashboard filters to every + # saved adhoc_filter* list. Validate those lists before it touches them. + for key, saved_filters in form_data.items(): + if key == "adhoc_filters" or ( + append_filters and key.startswith("adhoc_filter") + ): + if not isinstance(saved_filters, list) or any( + not isinstance(clause, dict) for clause in saved_filters Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Chart overrides overwritten</b></div> <div id="fix"> `form_data["extra_form_data"] = deepcopy(dict(efd))` replaces the chart's own `extra_form_data` before `merge_extra_form_data` runs, so the chart's own overrides (e.g. its `time_range`) are silently dropped. `Slice.form_data` (models/slice.py) includes the chart's stored extra_form_data, and `merge_extra_form_data` (superset/utils/core.py:1145) merges whatever this key holds. Copy the chart's own extra_form_data first, then overlay `efd`. </div> </div> <small><i>Code Review Run #613573</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
