aminghadersohi commented on code in PR #45575:
URL: https://github.com/apache/superset/pull/45575#discussion_r4363520671
##########
superset/mcp_service/utils/sanitization.py:
##########
@@ -305,6 +339,19 @@ def sanitize_user_input(
value = _strip_html_tags(value)
value = _remove_dangerous_unicode(value)
+ # Reject rather than store tag-shaped text: a later sanitizer pass would
+ # decode and strip it, so the stored value would not be stable.
+ _check_stable_html(value, field_name)
+
+ # Conservative bracket escaping can expand text even after tags are
removed.
+ # Enforce the storage cap for callers without a Pydantic Field length
limit.
+ if len(value) > max_length:
Review Comment:
You're right. I fixed the ordering in
4818cc616fa9cc41abb782276c3d951cd033a978: the shared user-text sanitizer trims
after tag/Unicode removal, before checking the final text cap. The independent
raw-input resource bound and HTML/URL/event-handler/SQL safety checks remain in
place.
Regression tests expanded in this commit:
-
`tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py::test_markup_title_length_uses_sanitized_text`:
padded markup at the 250/500-character boundary, all title aliases, chart
generate/update and dashboard generate/update/duplicate. It asserts exact
trimmed values and warnings, while over-limit values still report
`string_too_long`.
-
`tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py::test_sibling_sanitized_length_budget`:
every sibling scalar/list field on the shared sanitize-plus-cap path,
including certification, columns, time ranges and Bullet labels.
-
`tests/unit_tests/mcp_service/utils/test_sanitization.py::test_sanitized_length_budget`:
both shared entry points, literal spaces, encoded spaces and NBSP at each
supported cap.
Before the source change, this expanded test selection produced 153 failures
and 323 passes; the failing padded boundary cases pass after the fix. Both
touched modules pass all 1,516 tests in Docker. Existing unpadded-markup and
over-limit guards still pass, preserving the earlier raw-versus-sanitized
length correction. The Bullet/Rose Jest suites pass 32 tests. Scoped pre-commit
passes, including mypy, Ruff and Pylint.
Siblings checked:
- superset/mcp_service/chart/schemas.py:883 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/chart/schemas.py:1009 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/chart/schemas.py:1021 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/chart/schemas.py:1144 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/chart/schemas.py:1227 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/chart/schemas.py:2781 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/chart/schemas.py:3177 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/chart/schemas.py:4650 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/chart/schemas.py:4664 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/chart/schemas.py:6396 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/dashboard/schemas.py:884 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/dashboard/schemas.py:1050 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/dashboard/schemas.py:1397 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/dashboard/schemas.py:1422 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/dashboard/schemas.py:1539 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/utils/sanitization.py:233 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/utils/sanitization.py:261 — fixed in
4818cc616fa9cc41abb782276c3d951cd033a978
- superset/mcp_service/chart/tool/generate_chart.py:255 — not affected:
schema revalidation carries over the already-trimmed chart name
- superset/mcp_service/chart/tool/update_chart.py:907 — not affected: the
shared UpdateChartRequest validates the name before this tool persists it
- superset/mcp_service/chart/validation/pipeline.py:326 — not affected:
normalization carries over the already-trimmed chart name
- superset/mcp_service/dataset/schemas.py — not affected: names do not use
HTML stripping; markup remains literal
- superset/mcp_service/sql_lab/schemas.py — not affected: labels and tab
titles do not use HTML stripping
- superset/mcp_service/theme/schemas.py — not affected: theme names
intentionally retain whitespace and do not use this sanitizer
- superset/mcp_service/utils/sanitization.py:368 — not affected: filter
values have a separate raw-input cap and preserve literal value whitespace
- superset/mcp_service/utils/sanitization.py:470 — not affected: SQL
expressions reject HTML rather than strip tags
- superset/mcp_service/utils/error_builder.py:54 — not affected: separate
display-context helper, not persisted input validation
Search: `rg -n
'sanitize_user_input|sanitize_filter_value|sanitize_sql_expression'
superset/mcp_service --glob '*.py'`; the repository-wide search finds no shared
user-text callers outside MCP.
--
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]
