aminghadersohi commented on code in PR #45575:
URL: https://github.com/apache/superset/pull/45575#discussion_r4367066006
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -6396,16 +6416,24 @@ def _detect_chart_name_sanitization(cls, data: Any) ->
Any:
@field_validator("chart_name")
@classmethod
- def sanitize_chart_name(cls, v: str | None) -> str | None:
- """Sanitize chart name to prevent XSS attacks."""
- try:
- return sanitize_user_input(
- v, "Chart name", max_length=255, allow_empty=True
- )
- except ValueError as ex:
- raise PydanticCustomError(
- "unsafe_text", "{error}", {"error": str(ex)}
- ) from ex
+ def normalize_blank_chart_name(cls, v: str | None) -> str | None:
+ """Keep the optional-name fallback; nonblank names are sanitized
above."""
+ return v if v and v.strip() else None
Review Comment:
You're right: my earlier theme sibling entry was incorrect. Fixed in
0c80cb2f93830711849b06965c4e28a3f4ce2f00; final head
57260a2b4544fb3c71004775194a5d8ca897dfed (documentation/test alignment):
create_theme rejects blank theme_name before the 250-character cap, including
the reported 300-space input. I also gave
create_dataset_metric/update_dataset_metric metric_name and update_dataset
table_name the same blank-first behavior, rather than weakening the
documentation promise. Nonblank names retain their whitespace, length limits,
and malformed-input errors; omitted updates and explicit invalid null names
keep their existing behavior.
Regression tests added in
tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py:
- test_documented_blank_errors_precede_length systematically covers all
eight request fields with a documented blank-error promise:
update_dashboard.dashboard_title (500), duplicate_dashboard.dashboard_title
(500), create_virtual_dataset.dataset_name (250), create_theme.theme_name
(250), create_dataset_metric.metric_name (255),
update_dataset_metric.metric_name (255), update_dataset.table_name (250), and
save_sql_query.label (256). Each is tested with " " * (cap + 1) as str, bytes
and bytearray, plus Unicode whitespace. The four affected requests produce 24
failures before this commit and pass after; the other 24 cases are
already-passing sibling controls.
- test_required_name_over_cap_blank_wire_reason pins the exact MCP response,
including "Text must not be empty" at the corresponding name field, for cap+1
and cap+50 blanks. Eight cases fail before/pass after (including theme_name
with 300 spaces); four existing-behavior sibling controls pass both times.
- test_documented_name_nonblank_length_boundary,
test_literal_names_keep_nonblank_whitespace, and
test_literal_names_reject_malformed_inputs retain 84 passing controls for
exact/over-limit nonblank names, whitespace preservation, and native
field-located type/UTF-8 errors.
The same 144-case regression suite ran in Docker against untouched source at
705c6f0bc395800d6b5fee03d5bef0a9830b1852 (32 failed / 112 passed) and this
commit (144 passed). Command: `python -m pytest -q
tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py -k
'documented_blank_errors_precede_length or
documented_name_nonblank_length_boundary or
required_name_over_cap_blank_wire_reason or literal_names_'`.
Full Docker MCP run: 13,245 passed / 12 skipped with `python -m pytest -n 2
--dist loadfile tests/unit_tests/mcp_service -m 'not manual and not
llm_compliance' --cache-clear -o cache_dir=/tmp/pytest-cache --maxfail=20`. The
touched regression file passed all 1,737 tests. The updated
test_builtin_name_validation_documented_for_administrators also fails against
the original documentation and passes against the clarified documentation; its
whitespace-normalized assertion avoids depending on Markdown line wrapping.
Changed-file pre-commit and explicit `pre-commit run mypy` passed; the three
existing frontend Jest suites passed (32 tests). Real-LLM checks were excluded
(OpenRouter disabled).
I searched `rg -n 'max_length|empty_text' superset/mcp_service -g
'*schemas.py'`, inventoried all 140 explicit max_length field declarations
(including nested/response schemas), compared the documentation's named
blank-error contracts, and searched all callers of the changed request models.
The eight promised fields and adjacent paths are all listed below.
Siblings checked:
- superset/mcp_service/theme/schemas.py:138 — fixed in
0c80cb2f93830711849b06965c4e28a3f4ce2f00 (CreateThemeRequest.theme_name, cap
250).
- superset/mcp_service/dataset/schemas.py:701 — fixed in
0c80cb2f93830711849b06965c4e28a3f4ce2f00 (DatasetMetricProperties.metric_name,
shared blank-first validator, cap 255).
- superset/mcp_service/dataset/schemas.py:809 — fixed in
0c80cb2f93830711849b06965c4e28a3f4ce2f00
(UpdateDatasetMetricRequest.metric_name inherits that validator).
- superset/mcp_service/dataset/schemas.py:820 — fixed in
0c80cb2f93830711849b06965c4e28a3f4ce2f00
(CreateDatasetMetricRequest.metric_name inherits that validator despite
overriding the field).
- superset/mcp_service/dataset/schemas.py:1025 — fixed in
0c80cb2f93830711849b06965c4e28a3f4ce2f00 (UpdateDatasetRequest.table_name, cap
250).
- superset/mcp_service/dataset/schemas.py:580 — not affected:
CreateVirtualDatasetRequest.dataset_name (cap 250) already decodes bytes,
rejects blanks and trims before its cap; included in the same regression sweep.
- superset/mcp_service/dashboard/schemas.py:926 — not affected:
UpdateDashboardRequest.dashboard_title (cap 500) already rejects blanks before
its cap; included in the same regression sweep.
- superset/mcp_service/dashboard/schemas.py:1487 — not affected:
DuplicateDashboardRequest.dashboard_title (cap 500) already rejects blanks
before its cap; included in the same regression sweep.
- superset/mcp_service/sql_lab/schemas.py:233 — not affected:
SaveSqlQueryRequest.label (cap 256) already decodes bytes and rejects blanks
before its cap; included in the same regression sweep.
- superset/mcp_service/chart/schemas.py:6352 — not affected: ChartNameMixin
already normalizes blank chart_name to None before its cap for both
GenerateChartRequest and UpdateChartRequest;
test_blank_titles_normalize_before_length covers strings, bytes, bytearrays and
all four aliases.
- superset/mcp_service/dashboard/schemas.py:781 — not affected:
GenerateDashboardRequest uses its documented optional-title fallback rather
than an empty-text error; test_blank_titles_normalize_before_length covers
strings, bytes, bytearrays and aliases.
- superset/mcp_service/dataset/schemas.py:506 — not affected:
CreateDatasetRequest.table_name trims before its cap and uses its min_length
error for blanks, not the blank-error promise naming create_virtual_dataset and
update_dataset; schema/catalog blanks normalize to None before their caps.
- superset/mcp_service/dataset/schemas.py:458 — not affected: nested
CreateDatasetMetric.metric_name has no max_length and is not the
create_dataset_metric/update_dataset_metric tool request named in the
documentation.
- superset/mcp_service/sql_lab/schemas.py:301 — not affected:
OpenSqlLabRequest.title normalizes blanks to None before its cap, not the
blank-error contract for save_sql_query.label.
- superset/mcp_service/dashboard/schemas.py:1093 — not affected:
UpdateDashboardRequest.slug has a separate blank-clears-slug contract and
normalizes before its cap, not the dashboard_title blank-error promise.
- superset/mcp_service/theme/tool/create_theme.py:61 — not affected:
consumes the validated CreateThemeRequest; no separate raw-name normalization
path.
- superset/mcp_service/dataset/tool/create_dataset_metric.py:47 — not
affected: consumes the validated CreateDatasetMetricRequest and its updates(),
with no separate raw-name normalization path.
- superset/mcp_service/dataset/tool/update_dataset_metric.py:110 — not
affected: consumes the validated UpdateDatasetMetricRequest and its updates(),
with no separate raw-name normalization path.
- superset/mcp_service/dataset/tool/update_dataset.py:80 — not affected:
consumes the validated UpdateDatasetRequest and its updates(), with no separate
raw-name normalization path.
- superset/themes/schemas.py — not affected: the REST theme endpoint uses
separate Marshmallow validation; the documented promise here is for MCP
requests.
- superset/datasets/schemas.py — not affected: REST dataset/metric schemas
do not invoke these MCP validators; their validation contract is unchanged.
- docs/admin_docs/configuration/mcp-server.mdx:2457 — fixed in
57260a2b4544fb3c71004775194a5d8ca897dfed (blank-first ordering is explicit,
nonblank whitespace preservation is precise, and the comparison names virtual
datasets rather than physical table names).
- tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py:1376 —
fixed in 57260a2b4544fb3c71004775194a5d8ca897dfed (the documentation assertion
checks the clarified contract and ignores Markdown line wrapping).
--
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]
