aminghadersohi commented on code in PR #39915:
URL: https://github.com/apache/superset/pull/39915#discussion_r3236188160
##########
tests/unit_tests/mcp_service/chart/test_chart_schemas.py:
##########
@@ -778,3 +778,137 @@ def
test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "chart_name" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestColumnRefNameRelaxedPattern:
+ """ColumnRef.name no longer enforces a strict regex pattern.
+
+ Many valid database column names were previously rejected:
+ - Names starting with a digit (e.g. "1Q_revenue")
+ - Names with locale-specific characters
+ The field_validator sanitize_name() still blocks XSS and SQL injection.
+ """
+
+ def test_digit_prefixed_name_accepted(self) -> None:
+ """Column names starting with a digit must now be accepted."""
+ col = ColumnRef(name="1Q_revenue")
+ assert col.name == "1Q_revenue"
+
+ def test_name_with_hyphen_accepted(self) -> None:
+ col = ColumnRef(name="order-date")
+ assert col.name == "order-date"
Review Comment:
Fixed in 65f73aade5: added docstring '"""Hyphenated column names (e.g.
'order-date') must be accepted."""'.
##########
tests/unit_tests/mcp_service/chart/test_chart_schemas.py:
##########
@@ -778,3 +778,137 @@ def
test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "chart_name" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestColumnRefNameRelaxedPattern:
+ """ColumnRef.name no longer enforces a strict regex pattern.
+
+ Many valid database column names were previously rejected:
+ - Names starting with a digit (e.g. "1Q_revenue")
+ - Names with locale-specific characters
+ The field_validator sanitize_name() still blocks XSS and SQL injection.
+ """
+
+ def test_digit_prefixed_name_accepted(self) -> None:
+ """Column names starting with a digit must now be accepted."""
+ col = ColumnRef(name="1Q_revenue")
+ assert col.name == "1Q_revenue"
+
+ def test_name_with_hyphen_accepted(self) -> None:
+ col = ColumnRef(name="order-date")
+ assert col.name == "order-date"
+
+ def test_name_with_dot_accepted(self) -> None:
+ col = ColumnRef(name="schema.column")
+ assert col.name == "schema.column"
Review Comment:
Fixed in 65f73aade5: added docstring '"""Dot-qualified names (e.g.
'schema.column') must be accepted."""'.
##########
tests/unit_tests/mcp_service/chart/test_chart_schemas.py:
##########
@@ -778,3 +778,137 @@ def
test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "chart_name" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestColumnRefNameRelaxedPattern:
+ """ColumnRef.name no longer enforces a strict regex pattern.
+
+ Many valid database column names were previously rejected:
+ - Names starting with a digit (e.g. "1Q_revenue")
+ - Names with locale-specific characters
+ The field_validator sanitize_name() still blocks XSS and SQL injection.
+ """
+
+ def test_digit_prefixed_name_accepted(self) -> None:
+ """Column names starting with a digit must now be accepted."""
+ col = ColumnRef(name="1Q_revenue")
+ assert col.name == "1Q_revenue"
+
+ def test_name_with_hyphen_accepted(self) -> None:
+ col = ColumnRef(name="order-date")
+ assert col.name == "order-date"
+
+ def test_name_with_dot_accepted(self) -> None:
+ col = ColumnRef(name="schema.column")
+ assert col.name == "schema.column"
+
+ def test_name_with_spaces_accepted(self) -> None:
+ col = ColumnRef(name="Total Revenue")
+ assert col.name == "Total Revenue"
Review Comment:
Fixed in 65f73aade5: added docstring '"""Column names with spaces (e.g.
'Total Revenue') must be accepted."""'.
##########
tests/unit_tests/mcp_service/chart/test_chart_schemas.py:
##########
@@ -778,3 +778,137 @@ def
test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "chart_name" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestColumnRefNameRelaxedPattern:
+ """ColumnRef.name no longer enforces a strict regex pattern.
+
+ Many valid database column names were previously rejected:
+ - Names starting with a digit (e.g. "1Q_revenue")
+ - Names with locale-specific characters
+ The field_validator sanitize_name() still blocks XSS and SQL injection.
+ """
+
+ def test_digit_prefixed_name_accepted(self) -> None:
+ """Column names starting with a digit must now be accepted."""
+ col = ColumnRef(name="1Q_revenue")
+ assert col.name == "1Q_revenue"
+
+ def test_name_with_hyphen_accepted(self) -> None:
+ col = ColumnRef(name="order-date")
+ assert col.name == "order-date"
+
+ def test_name_with_dot_accepted(self) -> None:
+ col = ColumnRef(name="schema.column")
+ assert col.name == "schema.column"
+
+ def test_name_with_spaces_accepted(self) -> None:
+ col = ColumnRef(name="Total Revenue")
+ assert col.name == "Total Revenue"
+
+ def test_script_tag_neutralized(self) -> None:
+ """sanitize_name() neutralizes script-tag XSS via nh3.
+ Depending on nh3 version, nh3 either strips the entire script element
+ including its content (leaving empty → ValidationError) or strips only
+ the tag delimiters (leaving 'alert(1)'). Either way, no raw <script>
+ tag is stored in the column name."""
+ try:
+ col = ColumnRef(name="<script>alert(1)</script>")
+ assert "<script>" not in col.name
+ except ValidationError as exc:
+ # nh3 stripped entire element; empty-value guard raises with this
message
+ assert "cannot be empty" in str(exc) # noqa: PT017
+
+ def test_event_handler_injection_blocked(self) -> None:
+ """sanitize_name() rejects event-handler injection patterns
(on...=)."""
+ with pytest.raises(ValidationError):
+ ColumnRef(name="col onclick=alert(1)")
+
+ def test_sql_keyword_blocked(self) -> None:
+ """check_sql_keywords=True still blocks pure SQL statements."""
+ with pytest.raises(ValidationError):
+ ColumnRef(name="1; DROP TABLE users; --")
+
+ def test_empty_name_blocked(self) -> None:
+ with pytest.raises(ValidationError):
+ ColumnRef(name="")
+
+ def test_table_chart_with_digit_prefixed_column(self) -> None:
+ """End-to-end: digit-prefixed column passes through
GenerateChartRequest."""
+ req = GenerateChartRequest(
+ dataset_id=1,
+ config={
+ "chart_type": "table",
+ "columns": [
+ {"name": "1Q_revenue"},
+ {"name": "product_name"},
+ ],
+ },
+ )
+ assert req.config.chart_type == "table"
+
+ def test_xy_chart_with_hyphenated_column(self) -> None:
+ req = GenerateChartRequest(
+ dataset_id=1,
+ config={
+ "chart_type": "xy",
+ "x": {"name": "order-date"},
+ "y": [{"name": "1Q-revenue", "aggregate": "SUM"}],
+ },
+ )
+ assert req.config.chart_type == "xy"
Review Comment:
Fixed in 65f73aade5: added docstring '"""Hyphenated column names must pass
through GenerateChartRequest for XY charts."""'.
--
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]
