diff --git a/superset/mcp_service/chart/schemas.py b/superset/mcp_service/chart/schemas.py index 55e2cf0b7fd..c3b22bfb772 100644 --- a/superset/mcp_service/chart/schemas.py +++ b/superset/mcp_service/chart/schemas.py @@ -1274,10 +1274,15 @@ class ListChartsRequest(MetadataCacheControl): # The tool input models class GenerateChartRequest(QueryCacheControl): + model_config = ConfigDict(populate_by_name=True) + dataset_id: int | str = Field(..., description="Dataset identifier (ID, UUID)") config: Dict[str, Any] = Field(..., description=_CHART_CONFIG_DESCRIPTION) chart_name: str | None = Field( - None, description="Auto-generates if omitted", max_length=255 + None, + description="Auto-generates if omitted", + max_length=255, + validation_alias=AliasChoices("chart_name", "name", "title", "slice_name"), ) save_chart: bool = Field(default=False, description="Save permanently in Superset") generate_preview: bool = True @@ -1331,6 +1336,8 @@ class GenerateExploreLinkRequest(FormDataCacheControl): class UpdateChartRequest(QueryCacheControl): + model_config = ConfigDict(populate_by_name=True) + identifier: int | str = Field(..., description="Chart ID or UUID") config: Dict[str, Any] | None = Field( None, @@ -1339,7 +1346,10 @@ class UpdateChartRequest(QueryCacheControl): ), ) chart_name: str | None = Field( - None, description="Auto-generates if omitted", max_length=255 + None, + description="Auto-generates if omitted", + max_length=255, + validation_alias=AliasChoices("chart_name", "name", "title", "slice_name"), ) generate_preview: bool = Field( default=True, diff --git a/superset/mcp_service/chart/tool/generate_chart.py b/superset/mcp_service/chart/tool/generate_chart.py index 1be64f041d0..eb9e0e446bc 100644 --- a/superset/mcp_service/chart/tool/generate_chart.py +++ b/superset/mcp_service/chart/tool/generate_chart.py @@ -281,7 +281,34 @@ async def generate_chart( # noqa: C901 ) # Parse the raw config dict into a typed ChartConfig for downstream use - config = parse_chart_config(request.config) + try: + config = parse_chart_config(request.config) + except (ValueError, TypeError) as e: + from superset.mcp_service.utils.error_sanitization import ( + _sanitize_validation_error, + ) + + sanitized = _sanitize_validation_error(e) + execution_time = int((time.time() - start_time) * 1000) + return GenerateChartResponse.model_validate( + { + "chart": None, + "error": { + "error_type": "validation_error", + "message": f"Invalid chart configuration: {sanitized}", + "details": sanitized, + "error_code": "INVALID_CHART_CONFIG", + }, + "performance": { + "query_duration_ms": execution_time, + "cache_status": "error", + "optimization_suggestions": [], + }, + "success": False, + "schema_version": "2.0", + "api_version": "v1", + } + ) # Map the simplified config to Superset's form_data format # Pass dataset_id to enable column type checking for proper viz_type selection diff --git a/superset/mcp_service/chart/tool/update_chart.py b/superset/mcp_service/chart/tool/update_chart.py index ecb7d178240..c2f4216b90b 100644 --- a/superset/mcp_service/chart/tool/update_chart.py +++ b/superset/mcp_service/chart/tool/update_chart.py @@ -97,22 +97,23 @@ def _missing_config_or_name_error() -> GenerateChartResponse: def _build_update_payload( request: UpdateChartRequest, chart: Any, + parsed_config: Any = None, ) -> dict[str, Any] | GenerateChartResponse: """Build the update payload for a chart update. Returns a dict payload on success, or a GenerateChartResponse error when neither config nor chart_name is provided. + ``parsed_config`` is the pre-parsed chart config from the caller. """ - if request.config is not None: - config = parse_chart_config(request.config) + if parsed_config is not None: dataset_id = chart.datasource_id if chart.datasource_id else None - new_form_data = map_config_to_form_data(config, dataset_id=dataset_id) + new_form_data = map_config_to_form_data(parsed_config, dataset_id=dataset_id) new_form_data.pop("_mcp_warnings", None) chart_name = ( request.chart_name if request.chart_name - else chart.slice_name or generate_chart_name(config) + else chart.slice_name or generate_chart_name(parsed_config) ) return { @@ -130,12 +131,14 @@ def _build_update_payload( def _build_preview_form_data( request: UpdateChartRequest, chart: Any, + parsed_config: Any = None, ) -> dict[str, Any] | GenerateChartResponse: """Merge the existing chart's form_data with the requested changes. Used by the preview-first flow so the user can review edits in Explore before clicking Save. Returns the merged form_data dict on success, or a GenerateChartResponse error when neither config nor chart_name is given. + ``parsed_config`` is the pre-parsed chart config from the caller. """ existing_form_data: dict[str, Any] = {} if getattr(chart, "params", None): @@ -147,10 +150,9 @@ def _build_preview_form_data( ) existing_form_data = {} - if request.config is not None: - config = parse_chart_config(request.config) + if parsed_config is not None: dataset_id = chart.datasource_id if chart.datasource_id else None - new_form_data = map_config_to_form_data(config, dataset_id=dataset_id) + new_form_data = map_config_to_form_data(parsed_config, dataset_id=dataset_id) new_form_data.pop("_mcp_warnings", None) merged = {**existing_form_data, **new_form_data} else: @@ -319,10 +321,41 @@ async def update_chart( # noqa: C901 warnings: list[str] = [] saved = False + # Parse config once upfront so helpers and analysis can reuse it. + parsed_config = None + if request.config is not None: + try: + parsed_config = parse_chart_config(request.config) + except (ValueError, TypeError) as e: + from superset.mcp_service.utils.error_sanitization import ( + _sanitize_validation_error, + ) + + sanitized = _sanitize_validation_error(e) + return GenerateChartResponse.model_validate( + { + "chart": None, + "error": { + "error_type": "validation_error", + "message": f"Invalid chart configuration: {sanitized}", + "details": sanitized, + "error_code": "INVALID_CHART_CONFIG", + }, + "performance": { + "query_duration_ms": int((time.time() - start_time) * 1000), + "cache_status": "error", + "optimization_suggestions": [], + }, + "success": False, + "schema_version": "2.0", + "api_version": "v1", + } + ) + if not request.generate_preview: from superset.commands.chart.update import UpdateChartCommand - payload_or_error = _build_update_payload(request, chart) + payload_or_error = _build_update_payload(request, chart, parsed_config) if isinstance(payload_or_error, GenerateChartResponse): return payload_or_error @@ -334,7 +367,7 @@ async def update_chart( # noqa: C901 f"{get_superset_base_url()}/explore/?slice_id={updated_chart.id}" ) else: - preview_or_error = _build_preview_form_data(request, chart) + preview_or_error = _build_preview_form_data(request, chart, parsed_config) if isinstance(preview_or_error, GenerateChartResponse): return preview_or_error @@ -343,12 +376,9 @@ async def update_chart( # noqa: C901 chart, preview_or_error ) - # Parse config for analysis (may be None for name-only updates) - config = parse_chart_config(request.config) if request.config else None - chart_for_analysis = updated_chart if saved else chart - capabilities = analyze_chart_capabilities(chart_for_analysis, config) - semantics = analyze_chart_semantics(chart_for_analysis, config) + capabilities = analyze_chart_capabilities(chart_for_analysis, parsed_config) + semantics = analyze_chart_semantics(chart_for_analysis, parsed_config) execution_time = int((time.time() - start_time) * 1000) performance = PerformanceMetadata( @@ -363,7 +393,11 @@ async def update_chart( # noqa: C901 else ( request.chart_name or (chart.slice_name if hasattr(chart, "slice_name") else None) - or (generate_chart_name(config) if config else "Updated chart") + or ( + generate_chart_name(parsed_config) + if parsed_config + else "Updated chart" + ) ) ) accessibility = AccessibilityMetadata( diff --git a/superset/mcp_service/chart/tool/update_chart_preview.py b/superset/mcp_service/chart/tool/update_chart_preview.py index 75de06c23fc..ed6a6d53769 100644 --- a/superset/mcp_service/chart/tool/update_chart_preview.py +++ b/superset/mcp_service/chart/tool/update_chart_preview.py @@ -103,7 +103,31 @@ def update_chart_preview( try: # Parse the raw config dict into a typed ChartConfig - config = parse_chart_config(request.config) + try: + config = parse_chart_config(request.config) + except (ValueError, TypeError) as e: + from superset.mcp_service.utils.error_sanitization import ( + _sanitize_validation_error, + ) + + sanitized = _sanitize_validation_error(e) + return { + "chart": None, + "error": { + "error_type": "validation_error", + "message": f"Invalid chart configuration: {sanitized}", + "details": sanitized, + "error_code": "INVALID_CHART_CONFIG", + }, + "performance": { + "query_duration_ms": int((time.time() - start_time) * 1000), + "cache_status": "error", + "optimization_suggestions": [], + }, + "success": False, + "schema_version": "2.0", + "api_version": "v1", + } with event_logger.log_context(action="mcp.update_chart_preview.form_data"): # Map the new config to form_data format diff --git a/superset/mcp_service/chart/validation/pipeline.py b/superset/mcp_service/chart/validation/pipeline.py index 6923de0bd0b..ff01a5cabf3 100644 --- a/superset/mcp_service/chart/validation/pipeline.py +++ b/superset/mcp_service/chart/validation/pipeline.py @@ -35,74 +35,14 @@ from superset.mcp_service.common.error_schemas import ( logger = logging.getLogger(__name__) - -def _redact_sql_select(error_str: str, error_str_upper: str) -> str: - """Redact SELECT...FROM clause content to prevent data disclosure.""" - if "SELECT" in error_str_upper and "FROM" in error_str_upper: - select_idx = error_str_upper.find("SELECT") - from_idx = error_str_upper.find("FROM", select_idx) - if select_idx != -1 and from_idx != -1: - return error_str[: select_idx + 7] + " [REDACTED] " + error_str[from_idx:] - return error_str - - -def _redact_sql_where(error_str: str, error_str_upper: str) -> str: - """Redact WHERE clause content to prevent data disclosure.""" - if "WHERE" not in error_str_upper: - return error_str - - where_idx = error_str_upper.find("WHERE") - terminators = ["ORDER", "GROUP", "LIMIT", "UNION", "EXCEPT", "INTERSECT"] - term_idx = len(error_str) - for term in terminators: - idx = error_str_upper.find(term, where_idx) - if idx != -1 and idx < term_idx: - term_idx = idx - return error_str[: where_idx + 6] + " [REDACTED]" + error_str[term_idx:] - - -def _get_generic_error_message(error_str: str) -> str | None: - """Return generic message for common error types, or None.""" - error_lower = error_str.lower() - if "permission" in error_lower or "access" in error_lower: - return "Validation failed due to access restrictions" - if "database" in error_lower or "connection" in error_lower: - return "Validation failed due to database connectivity" - if "timeout" in error_lower: - return "Validation timed out" - return None - - -def _sanitize_validation_error(error: Exception) -> str: - """SECURITY FIX: Sanitize validation errors to prevent disclosure.""" - error_str = str(error) - - # SECURITY FIX: Limit length FIRST to prevent ReDoS attacks - if len(error_str) > 200: - error_str = error_str[:200] + "...[truncated]" - - # Remove potentially sensitive schema information - import re - - sensitive_patterns = [ - (r'\btable\s+[\'"`]?(\w+)[\'"`]?', "table [REDACTED]"), - (r'\bcolumn\s+[\'"`]?(\w+)[\'"`]?', "column [REDACTED]"), - (r'\bdatabase\s+[\'"`]?(\w+)[\'"`]?', "database [REDACTED]"), - (r'\bschema\s+[\'"`]?(\w+)[\'"`]?', "schema [REDACTED]"), - ] - for pattern, replacement in sensitive_patterns: - error_str = re.sub(pattern, replacement, error_str, flags=re.IGNORECASE) - - # SECURITY FIX: SQL sanitization without ReDoS-vulnerable patterns - error_str_upper = error_str.upper() - error_str = _redact_sql_select(error_str, error_str_upper) - error_str = _redact_sql_where(error_str, error_str_upper) - - # Return generic message for common error types - if generic := _get_generic_error_message(error_str): - return generic - - return error_str +# Re-export from shared utility so existing ``from pipeline import ...`` +# callers continue to work without changes. +from superset.mcp_service.utils.error_sanitization import ( # noqa: E402, F401 + _get_generic_error_message, + _redact_sql_select, + _redact_sql_where, + _sanitize_validation_error, +) class ValidationResult: @@ -174,7 +114,21 @@ class ValidationPipeline: # Parse the raw config dict into a typed ChartConfig for # downstream validators that need typed access. - typed_config = parse_chart_config(request.config) + try: + typed_config = parse_chart_config(request.config) + except (ValueError, TypeError) as e: + from superset.mcp_service.utils.error_builder import ( + ChartErrorBuilder, + ) + + sanitized_reason = _sanitize_validation_error(e) + error = ChartErrorBuilder.build_error( + error_type="validation_error", + template_key="validation_error", + template_vars={"reason": sanitized_reason}, + error_code="INVALID_CHART_CONFIG", + ) + return ValidationResult(is_valid=False, request=request, error=error) # Fetch dataset context once and reuse across validation layers dataset_context = ValidationPipeline._get_dataset_context( diff --git a/superset/mcp_service/dashboard/schemas.py b/superset/mcp_service/dashboard/schemas.py index e197b25aa44..31033842050 100644 --- a/superset/mcp_service/dashboard/schemas.py +++ b/superset/mcp_service/dashboard/schemas.py @@ -70,6 +70,7 @@ from datetime import datetime from typing import Annotated, Any, Dict, List, Literal, TYPE_CHECKING from pydantic import ( + AliasChoices, BaseModel, ConfigDict, Field, @@ -497,6 +498,8 @@ class AddChartToDashboardResponse(BaseModel): class GenerateDashboardRequest(BaseModel): """Request schema for generating a dashboard.""" + model_config = ConfigDict(populate_by_name=True) + chart_ids: List[int] = Field( ..., description="List of chart IDs to include in the dashboard", min_length=1 ) @@ -506,6 +509,7 @@ class GenerateDashboardRequest(BaseModel): "Title for the new dashboard. When omitted a descriptive title " "is generated from the included chart names." ), + validation_alias=AliasChoices("dashboard_title", "title", "name"), ) description: str | None = Field(None, description="Description for the dashboard") published: bool = Field( diff --git a/superset/mcp_service/explore/tool/generate_explore_link.py b/superset/mcp_service/explore/tool/generate_explore_link.py index 5a7360665c1..d6eeb88c4c0 100644 --- a/superset/mcp_service/explore/tool/generate_explore_link.py +++ b/superset/mcp_service/explore/tool/generate_explore_link.py @@ -101,7 +101,21 @@ async def generate_explore_link( try: # Parse the raw config dict into a typed ChartConfig - config = parse_chart_config(request.config) + try: + config = parse_chart_config(request.config) + except (ValueError, TypeError) as e: + from superset.mcp_service.utils.error_sanitization import ( + _sanitize_validation_error, + ) + + sanitized = _sanitize_validation_error(e) + await ctx.error(f"Invalid chart configuration: {sanitized}") + return { + "url": "", + "form_data": {}, + "form_data_key": None, + "error": f"Invalid chart configuration: {sanitized}", + } await ctx.report_progress(1, 4, "Validating dataset exists") with event_logger.log_context(action="mcp.generate_explore_link.dataset_check"): diff --git a/superset/mcp_service/utils/error_sanitization.py b/superset/mcp_service/utils/error_sanitization.py new file mode 100644 index 00000000000..332a3aa7111 --- /dev/null +++ b/superset/mcp_service/utils/error_sanitization.py @@ -0,0 +1,109 @@ +# 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. + +""" +Shared error sanitization utilities for MCP service. + +SECURITY: These functions sanitize validation errors to prevent information +disclosure (e.g., SQL fragments, schema names, table names) while preserving +actionable error messages for LLM callers. +""" + +import re + + +def _redact_sql_select(error_str: str, error_str_upper: str) -> str: + """Redact SELECT...FROM clause content to prevent data disclosure.""" + if "SELECT" in error_str_upper and "FROM" in error_str_upper: + select_idx = error_str_upper.find("SELECT") + from_idx = error_str_upper.find("FROM", select_idx) + if select_idx != -1 and from_idx != -1: + return error_str[: select_idx + 7] + " [REDACTED] " + error_str[from_idx:] + return error_str + + +def _redact_sql_where(error_str: str, error_str_upper: str) -> str: + """Redact WHERE clause content to prevent data disclosure.""" + if "WHERE" not in error_str_upper: + return error_str + + where_idx = error_str_upper.find("WHERE") + terminators = ["ORDER", "GROUP", "LIMIT", "UNION", "EXCEPT", "INTERSECT"] + term_idx = len(error_str) + for term in terminators: + idx = error_str_upper.find(term, where_idx) + if idx != -1 and idx < term_idx: + term_idx = idx + return error_str[: where_idx + 6] + " [REDACTED]" + error_str[term_idx:] + + +def _get_generic_error_message(error_str: str) -> str | None: + """Return generic message for common error types, or None.""" + error_lower = error_str.lower() + if "permission" in error_lower or "access" in error_lower: + return "Validation failed due to access restrictions" + if "database" in error_lower or "connection" in error_lower: + return "Validation failed due to database connectivity" + if "timeout" in error_lower: + return "Validation timed out" + return None + + +def _sanitize_validation_error(error: Exception) -> str: + """SECURITY FIX: Sanitize validation errors to prevent disclosure.""" + error_str = str(error) + + # Pydantic tagged-union errors prefix the message with a long + # ``1 validation error for tagged-union[...]`` header before the + # per-field body (e.g. ``Value error, ...``, ``Field required``, + # ``Input should be ...``). The body always lives on a line indented + # by exactly two spaces — pull it out so the 200-char truncation + # below doesn't swallow the actionable part. The pydantic footer + # ``\n For further information ...`` uses four-space indent and + # is dropped here. + if "tagged-union[" in error_str: + body_match = re.search(r"\n (?! )", error_str) + if body_match: + idx = body_match.end() + footer_idx = error_str.find("\n For further information", idx) + end = footer_idx if footer_idx != -1 else len(error_str) + error_str = error_str[idx:end].strip() + + # SECURITY FIX: Limit length FIRST to prevent ReDoS attacks + if len(error_str) > 200: + error_str = error_str[:200] + "...[truncated]" + + # Remove potentially sensitive schema information + sensitive_patterns = [ + (r'\btable\s+[\'"`]?(\w+)[\'"`]?', "table [REDACTED]"), + (r'\bcolumn\s+[\'"`]?(\w+)[\'"`]?', "column [REDACTED]"), + (r'\bdatabase\s+[\'"`]?(\w+)[\'"`]?', "database [REDACTED]"), + (r'\bschema\s+[\'"`]?(\w+)[\'"`]?', "schema [REDACTED]"), + ] + for pattern, replacement in sensitive_patterns: + error_str = re.sub(pattern, replacement, error_str, flags=re.IGNORECASE) + + # SECURITY FIX: SQL sanitization without ReDoS-vulnerable patterns + error_str_upper = error_str.upper() + error_str = _redact_sql_select(error_str, error_str_upper) + error_str = _redact_sql_where(error_str, error_str_upper) + + # Return generic message for common error types + if generic := _get_generic_error_message(error_str): + return generic + + return error_str diff --git a/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py b/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py index 6322818fbac..e69d919f202 100644 --- a/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py +++ b/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py @@ -609,7 +609,7 @@ class TestBuildUpdatePayload: chart = Mock() chart.datasource_id = None # Avoid dataset lookup - result = _build_update_payload(request, chart) + result = _build_update_payload(request, chart, parsed_config=config) assert isinstance(result, dict) assert result["slice_name"] == "My Custom Name" @@ -627,7 +627,7 @@ class TestBuildUpdatePayload: chart.datasource_id = None chart.slice_name = "Existing Name" - result = _build_update_payload(request, chart) + result = _build_update_payload(request, chart, parsed_config=config) assert isinstance(result, dict) assert result["slice_name"] == "Existing Name" @@ -861,7 +861,7 @@ class TestBuildPreviewFormData: chart.slice_name = "Existing" chart.params = '{"viz_type": "line", "custom_flag": true}' - result = _build_preview_form_data(request, chart) + result = _build_preview_form_data(request, chart, parsed_config=config) assert isinstance(result, dict) # Existing keys not touched by the new config are preserved @@ -918,7 +918,7 @@ class TestBuildPreviewFormData: chart.slice_name = "Broken" chart.params = "not-json" - result = _build_preview_form_data(request, chart) + result = _build_preview_form_data(request, chart, parsed_config=config) assert isinstance(result, dict) assert result["slice_id"] == 9