aminghadersohi commented on code in PR #45575:
URL: https://github.com/apache/superset/pull/45575#discussion_r4362993943
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -6377,15 +6367,18 @@ def _detect_chart_name_sanitization(cls, data: Any) ->
Any:
return data
with field_validation_error("chart_name"):
sanitized, was_modified = sanitize_user_input_with_changes(
- raw, "Chart name", max_length=255, allow_empty=True
+ raw, "Chart name", max_length=250, allow_empty=True
Review Comment:
You're right: removed markup should not consume the stored-name budget.
Fixed in 2ae7a389d2a298b45cbba8a388d99d94c50eb7c8, with byte-string coercion
coverage in 335dbaf204c7684af3e990505949a35c61cee635. The shared user-text
sanitizer now has a separate raw-input resource bound of 10,000 characters (or
the field cap if larger), while the existing final-text limit still runs after
sanitization. Scalar sibling field validators also sanitize before Pydantic's
length check.
Regression tests added by this change:
- `test_markup_title_length_uses_sanitized_text`: the exact 245-character
bold chart name, the 250-character boundary, all chart aliases,
generate/update, and the corresponding dashboard generate/update/duplicate
paths. It also checks that sanitized over-limit values still fail and that
successful stripping reports a warning.
- `test_sibling_sanitized_length_budget`: every remaining caller of the
shared sanitizer at its final text limit and one character over it.
- `test_sanitized_length_budget`: tag removal, removable Unicode, and entity
decoding, through both sanitizer entry points.
- `test_raw_sanitization_input_is_bounded_before_html_parsing`: oversized
raw markup is rejected before parsing.
With the original source restored, these tests produced 104 failures and 37
passes; the passing cases are the unchanged over-limit rejection guards. The
updated source passes all 141 cases. The byte-string follow-up adds 20 cases
that fail on the initial ordering change and pass at the final head. Docker
pytest passes both touched modules (1,180 tests) plus the
utility/tool/normalization sibling selection (1,144 tests; overlapping utility
coverage). The three Bullet/Rose Jest suites pass all 32 tests. Scoped
pre-commit passes, including mypy, Ruff and Pylint.
Siblings checked:
- superset/mcp_service/chart/schemas.py:6396 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (shared generate/update chart-name
mixin and all four aliases)
- superset/mcp_service/dashboard/schemas.py:884 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (generate title and aliases)
- superset/mcp_service/dashboard/schemas.py:1050 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (update title and aliases)
- superset/mcp_service/dashboard/schemas.py:1539 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (duplicate title and aliases)
- superset/mcp_service/dashboard/schemas.py:1397 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (certified_by)
- superset/mcp_service/dashboard/schemas.py:1422 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (certification_details)
- superset/mcp_service/chart/schemas.py:883 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (temporal column)
- superset/mcp_service/chart/schemas.py:1009 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (column name)
- superset/mcp_service/chart/schemas.py:1021 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (display label)
- superset/mcp_service/chart/schemas.py:1144 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (filter column)
- superset/mcp_service/chart/schemas.py:1227 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (Gantt sort column)
- superset/mcp_service/chart/schemas.py:2781 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (Sunburst time range)
- superset/mcp_service/chart/schemas.py:3177 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (pivot comparison period)
- superset/mcp_service/chart/schemas.py:4650 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (Bullet range/marker/marker-line
labels)
- superset/mcp_service/chart/schemas.py:4664 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (Bullet time range)
- superset/mcp_service/utils/sanitization.py:233 — fixed in
335dbaf204c7684af3e990505949a35c61cee635 (warning wrapper delegates to shared
sanitizer)
- superset/mcp_service/chart/tool/generate_chart.py:255 — not affected:
revalidation carries over the already-sanitized name; no raw-length check there
- superset/mcp_service/chart/validation/pipeline.py:326 — not affected:
normalized requests carry over the already-sanitized name
- superset/mcp_service/dataset/schemas.py — not affected: names are not
HTML-stripped; markup is literal and counts toward storage length
- superset/mcp_service/sql_lab/schemas.py — not affected: labels and tab
titles are not HTML-stripped
- superset/mcp_service/theme/schemas.py — not affected: theme names are not
HTML-stripped
- superset/mcp_service/utils/sanitization.py:365 — not affected: chart-name
cap does not apply; filter values use a separate sanitizer with their own
1,000-character raw-input contract, not this shared user-text sanitizer
- superset/mcp_service/utils/sanitization.py:467 — not affected: SQL
expressions reject HTML tags rather than strip them
- superset/mcp_service/utils/error_builder.py:54 — not affected: separate
error-display truncation helper, not schema input sanitization
I searched with `rg -n
'sanitize_user_input|sanitize_filter_value|sanitize_sql_expression'
superset/mcp_service --glob '*.py'` and checked the dataset, SQL Lab, and theme
name validators too.
--
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]
