codeant-ai-for-open-source[bot] commented on code in PR #42087:
URL: https://github.com/apache/superset/pull/42087#discussion_r3609208436
##########
tests/unit_tests/connectors/sqla/models_test.py:
##########
@@ -1177,3 +1179,86 @@ def
test_validate_stored_expression_rejects_subquery_around_jinja(
None,
"(SELECT password FROM ab_user LIMIT 1) {# x #}",
)
+
+
+def test_get_sqla_col_validates_stored_expression_at_query_time(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A stored calculated-column expression must be validated at the query sink,
+ not only at save time. ``get_sqla_col`` routes the expression through
+ ``validate_adhoc_subquery`` so a disallowed sub-query is rejected even when
+ it reaches the query with the save-time check bypassed (templating, the
+ create path, or older data). Locks in the query-time gate.
+ """
+ tc = TableColumn(
+ column_name="leak",
+ expression="(SELECT password FROM ab_user LIMIT 1)",
+ )
+ tc.table = mocker.MagicMock()
+ tc.table.database.backend = "sqlite"
+ spy = mocker.patch(
+ "superset.models.helpers.validate_adhoc_subquery",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ message="Sub-queries are not allowed in stored expressions.",
+ error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ ),
+ )
+ with pytest.raises(SupersetSecurityException):
+ tc.get_sqla_col()
+ spy.assert_called_once()
+
+
+def test_get_timestamp_expression_validates_stored_expression_at_query_time(
+ mocker: MockerFixture,
+) -> None:
+ """
+ The timestamp-expression sink must enforce the same query-time gate as
+ ``get_sqla_col``: a stored datetime column expression is routed through
+ ``validate_adhoc_subquery`` before it reaches ``literal_column``, so a
+ disallowed sub-query is rejected on the time-grained query path too.
+ """
+ tc = TableColumn(
+ column_name="ds",
+ expression="(SELECT ts FROM ab_user LIMIT 1)",
+ )
+ tc.table = mocker.MagicMock()
+ tc.table.database.backend = "sqlite"
+ spy = mocker.patch(
+ "superset.models.helpers.validate_adhoc_subquery",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ message="Sub-queries are not allowed in stored expressions.",
+ error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ ),
+ )
+ with pytest.raises(SupersetSecurityException):
+ tc.get_timestamp_expression(time_grain=None)
+ spy.assert_called_once()
+
+
+def test_get_sqla_col_falls_back_when_stored_expression_unparseable(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A stored expression using dialect-specific syntax that sqlglot cannot parse
+ (e.g. ``DATE_ADD(ds, 1)`` on MySQL) pre-dates the query-time gate and went
+ to the query unparsed. A parse failure must fall back to the raw expression
+ rather than break the query; a genuine sub-query still parses and is
caught.
+ """
+ tc = TableColumn(column_name="ds", expression="DATE_ADD(ds, 1)")
+ tc.table = mocker.MagicMock()
+ tc.table.database.backend = "mysql"
+ mocker.patch(
+ "superset.models.helpers.validate_adhoc_subquery",
+ side_effect=SupersetParseError("DATE_ADD(ds, 1)", "mysql"),
+ )
+ literal = mocker.patch("superset.connectors.sqla.models.literal_column")
+ tc.get_sqla_col()
+ # The raw expression reaches ``literal_column`` unchanged; no exception.
+ assert literal.call_args.args[0] == "DATE_ADD(ds, 1)"
Review Comment:
**Suggestion:** In this fallback test, `validate_adhoc_subquery` is patched
to raise `SupersetParseError`, but the test never verifies that the patched
validator was actually called. As written, the test can still pass if
query-time validation is accidentally removed entirely (because
`literal_column` would still receive the raw expression), so it does not
reliably lock in the intended behavior. Capture the patch as a spy and assert
it was called (ideally with expected arguments) before asserting the fallback
result. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Query-time validation gate can be removed without detection.
⚠️ Security regression tests for stored expressions less trustworthy.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `tests/unit_tests/connectors/sqla/models_test.py` and locate
`test_get_sqla_col_falls_back_when_stored_expression_unparseable` at lines
1245–1264 (per
PR diff and BulkRead output, function body shown at snippet lines 46–65).
2. Observe that within this test,
`mocker.patch("superset.models.helpers.validate_adhoc_subquery",
side_effect=SupersetParseError("DATE_ADD(ds, 1)", "mysql"))` is called at
lines 1257–1260,
but the result of `mocker.patch(...)` is not captured into a variable and
there is no
`assert_called_*` on the patched validator.
3. Compare with the neighboring tests
`test_get_sqla_col_validates_stored_expression_at_query_time` and
`test_get_timestamp_expression_validates_stored_expression_at_query_time` in
the same file
(snippet lines 1–43, actual lines 1184–1243), where the patch to
`superset.models.helpers.validate_adhoc_subquery` is stored in `spy` and
`spy.assert_called_once()` is asserted after calling
`TableColumn.get_sqla_col()` or
`get_timestamp_expression(...)`, explicitly locking in that the query-time
gate runs.
4. Inspect `superset/connectors/sqla/models.py` around lines 1095–1121 (Read
offset 1060,
snippet lines 36–62), where `TableColumn.get_sqla_col` calls
`self._validate_stored_expression(expression)` and then
`literal_column(expression,
type_=type_)`. If a future refactor erroneously removes
`self._validate_stored_expression(...)` (and thus the underlying
`validate_stored_expression_at_query_time`/`validate_adhoc_subquery` gate),
`tc.get_sqla_col()` would still call `literal_column` with the raw
`"DATE_ADD(ds, 1)"`
expression, causing this fallback test to continue passing: the patched
`validate_adhoc_subquery` is never invoked, yet `literal.call_args.args[0]`
still equals
`"DATE_ADD(ds, 1)"`. Because the test does not assert that the validator spy
was called,
it cannot detect that the query-time validation gate has been removed,
making the test
ineffective at locking in the intended behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7c11665147024655a16e983c97e3789a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7c11665147024655a16e983c97e3789a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/connectors/sqla/models_test.py
**Line:** 1257:1264
**Comment:**
*Incomplete Implementation: In this fallback test,
`validate_adhoc_subquery` is patched to raise `SupersetParseError`, but the
test never verifies that the patched validator was actually called. As written,
the test can still pass if query-time validation is accidentally removed
entirely (because `literal_column` would still receive the raw expression), so
it does not reliably lock in the intended behavior. Capture the patch as a spy
and assert it was called (ideally with expected arguments) before asserting the
fallback result.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42087&comment_hash=9063014e849fcb962eb559e1b67acb517532e315077d7489b27bf8c2c43f9686&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42087&comment_hash=9063014e849fcb962eb559e1b67acb517532e315077d7489b27bf8c2c43f9686&reaction=dislike'>👎</a>
--
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]
