codeant-ai-for-open-source[bot] commented on code in PR #43111:
URL: https://github.com/apache/superset/pull/43111#discussion_r3775552452
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -3702,3 +3703,238 @@ def
test_validate_guest_token_resources_accepts_embedded_int_id(
sm.validate_guest_token_resources(
[{"type": GuestTokenResourceType.DASHBOARD, "id": 5}]
)
+
+
+# ---------------------------------------------------------------------------
+# _sql_filters_modified – block custom SQL injection by guest users
+# ---------------------------------------------------------------------------
+
+
+def test_sql_filters_extras_where_injected_blocked(
+ mocker: MockerFixture,
+) -> None:
+ """Injecting extras.where when the chart has no SQL filters is blocked."""
+ query_context = mocker.MagicMock()
+ stored_chart = mocker.MagicMock()
+ stored_chart.params_dict = {"metrics": ["count"]}
+
+ query = QueryObject(extras={"where": "1=1"})
+ query_context.queries = [query]
Review Comment:
**Suggestion:** This test only supplies one query, so it cannot detect a
cross-query authorization regression where a SQL predicate saved for one query
is accepted when moved to another query. Add a multi-query case with distinct
stored predicates and assert that each request query may use only its
corresponding predicate, rather than relying on a global allowed set. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Guest SQL predicates can transfer between chart queries.
- ⚠️ Multi-query embedded chart authorization lacks isolation coverage.
- ❌ `raise_for_access` may accept a predicate on the wrong query.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ac784103658844e387aa7fa10d6ecc21&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=ac784103658844e387aa7fa10d6ecc21&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/manager_test.py
**Line:** 3721:3722
**Comment:**
*Security: This test only supplies one query, so it cannot detect a
cross-query authorization regression where a SQL predicate saved for one query
is accepted when moved to another query. Add a multi-query case with distinct
stored predicates and assert that each request query may use only its
corresponding predicate, rather than relying on a global allowed set.
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%2F43111&comment_hash=efc8e3912560534737400185486d9ddedd41482ebca80581b1c8bf3d54be8320&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43111&comment_hash=efc8e3912560534737400185486d9ddedd41482ebca80581b1c8bf3d54be8320&reaction=dislike'>👎</a>
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -3702,3 +3703,238 @@ def
test_validate_guest_token_resources_accepts_embedded_int_id(
sm.validate_guest_token_resources(
[{"type": GuestTokenResourceType.DASHBOARD, "id": 5}]
)
+
+
+# ---------------------------------------------------------------------------
+# _sql_filters_modified – block custom SQL injection by guest users
+# ---------------------------------------------------------------------------
+
+
+def test_sql_filters_extras_where_injected_blocked(
+ mocker: MockerFixture,
+) -> None:
+ """Injecting extras.where when the chart has no SQL filters is blocked."""
+ query_context = mocker.MagicMock()
+ stored_chart = mocker.MagicMock()
+ stored_chart.params_dict = {"metrics": ["count"]}
+
+ query = QueryObject(extras={"where": "1=1"})
+ query_context.queries = [query]
+
+ form_data: dict[str, Any] = {"slice_id": 1}
+
+ assert _sql_filters_modified(query_context, form_data, stored_chart, None)
+
+
+def test_sql_filters_extras_having_injected_blocked(
+ mocker: MockerFixture,
+) -> None:
+ """Injecting extras.having when the chart has no SQL filters is blocked."""
+ query_context = mocker.MagicMock()
+ stored_chart = mocker.MagicMock()
+ stored_chart.params_dict = {}
+
+ query = QueryObject(extras={"having": "COUNT(*) > 0"})
+ query_context.queries = [query]
+
+ form_data: dict[str, Any] = {"slice_id": 1}
+
+ assert _sql_filters_modified(query_context, form_data, stored_chart, None)
+
+
+def test_sql_filters_extras_where_replay_allowed(
+ mocker: MockerFixture,
+) -> None:
+ """Replaying the chart's own SQL WHERE filter is allowed."""
+ sql_filter = {
+ "expressionType": "SQL",
+ "sqlExpression": "region = 'EMEA'",
+ "clause": "WHERE",
+ }
+ query_context = mocker.MagicMock()
+ stored_chart = mocker.MagicMock()
+ stored_chart.params_dict = {"adhoc_filters": [sql_filter]}
+
+ # freeform_where_having wraps each clause in parens
+ query = QueryObject(extras={"where": "(region = 'EMEA')"})
+ query_context.queries = [query]
+
+ form_data: dict[str, Any] = {"slice_id": 1}
+
+ assert not _sql_filters_modified(query_context, form_data, stored_chart,
None)
+
+
+def test_sql_filters_extras_having_replay_allowed(
+ mocker: MockerFixture,
+) -> None:
+ """Replaying the chart's own SQL HAVING filter is allowed."""
+ sql_filter = {
+ "expressionType": "SQL",
+ "sqlExpression": "SUM(sales) > 100",
+ "clause": "HAVING",
+ }
+ query_context = mocker.MagicMock()
+ stored_chart = mocker.MagicMock()
+ stored_chart.params_dict = {"adhoc_filters": [sql_filter]}
+
+ query = QueryObject(extras={"having": "(SUM(sales) > 100)"})
+ query_context.queries = [query]
+
+ form_data: dict[str, Any] = {"slice_id": 1}
+
+ assert not _sql_filters_modified(query_context, form_data, stored_chart,
None)
+
+
+def test_sql_filters_adhoc_sql_filter_injected_blocked(
+ mocker: MockerFixture,
+) -> None:
+ """Injecting a new SQL adhoc filter not on the stored chart is blocked."""
+ query_context = mocker.MagicMock()
+ stored_chart = mocker.MagicMock()
+ stored_chart.params_dict = {}
+
+ query = QueryObject()
+ query_context.queries = [query]
+
+ injected_filter = {
+ "expressionType": "SQL",
+ "sqlExpression": "1=1",
+ "clause": "WHERE",
+ }
+ form_data: dict[str, Any] = {"slice_id": 1, "adhoc_filters":
[injected_filter]}
+
+ assert _sql_filters_modified(query_context, form_data, stored_chart, None)
+
+
+def test_sql_filters_adhoc_sql_filter_replay_allowed(
+ mocker: MockerFixture,
+) -> None:
+ """Replaying the exact stored SQL adhoc filter is allowed."""
+ sql_filter = {
+ "expressionType": "SQL",
+ "sqlExpression": "region = 'EMEA'",
+ "clause": "WHERE",
+ }
+ query_context = mocker.MagicMock()
+ stored_chart = mocker.MagicMock()
+ stored_chart.params_dict = {"adhoc_filters": [sql_filter]}
+
+ query = QueryObject()
+ query_context.queries = [query]
+
+ form_data: dict[str, Any] = {"slice_id": 1, "adhoc_filters": [sql_filter]}
+
+ assert not _sql_filters_modified(query_context, form_data, stored_chart,
None)
+
+
+def test_sql_filters_empty_extras_always_allowed(
+ mocker: MockerFixture,
+) -> None:
+ """No SQL in extras is always allowed, even when the chart has SQL
filters."""
+ sql_filter = {
+ "expressionType": "SQL",
+ "sqlExpression": "region = 'EMEA'",
+ "clause": "WHERE",
+ }
+ query_context = mocker.MagicMock()
+ stored_chart = mocker.MagicMock()
+ stored_chart.params_dict = {"adhoc_filters": [sql_filter]}
+
+ query = QueryObject()
+ query_context.queries = [query]
+
+ form_data: dict[str, Any] = {"slice_id": 1}
+
+ assert not _sql_filters_modified(query_context, form_data, stored_chart,
None)
+
+
+def test_sql_filters_from_stored_qc_allowed(
+ mocker: MockerFixture,
+) -> None:
+ """extras.where from stored query_context is allowed."""
+ query_context = mocker.MagicMock()
+ stored_chart = mocker.MagicMock()
+ stored_chart.params_dict = {}
+
+ stored_qc = {
+ "queries": [{"extras": {"where": "(col > 5)"}}],
+ }
+
+ query = QueryObject(extras={"where": "(col > 5)"})
+ query_context.queries = [query]
+
+ form_data: dict[str, Any] = {"slice_id": 1}
+
+ assert not _sql_filters_modified(query_context, form_data, stored_chart,
stored_qc)
+
+
+def test_sql_filters_different_sql_blocked(
+ mocker: MockerFixture,
+) -> None:
+ """Modified SQL (appending extra predicates) is blocked."""
+ sql_filter = {
+ "expressionType": "SQL",
+ "sqlExpression": "col > 5",
+ "clause": "WHERE",
+ }
+ query_context = mocker.MagicMock()
+ stored_chart = mocker.MagicMock()
+ stored_chart.params_dict = {"adhoc_filters": [sql_filter]}
+
+ # Attacker appends extra predicate
+ query = QueryObject(
+ extras={"where": "(col > 5) AND (1=1)"},
+ )
+ query_context.queries = [query]
+
+ form_data: dict[str, Any] = {"slice_id": 1}
+
+ assert _sql_filters_modified(query_context, form_data, stored_chart, None)
+
+
+def test_sql_filters_simple_filters_not_blocked(
+ mocker: MockerFixture,
+) -> None:
+ """SIMPLE structured filters (from dashboard native filters) are not
blocked."""
+ query_context = mocker.MagicMock()
+ stored_chart = mocker.MagicMock()
+ stored_chart.params_dict = {}
+
+ query = QueryObject(
+ filters=[{"col": "country", "op": "==", "val": "US"}],
+ )
+ query_context.queries = [query]
+
+ simple_adhoc_filter = {
+ "expressionType": "SIMPLE",
+ "subject": "country",
+ "operator": "==",
+ "comparator": "US",
+ "clause": "WHERE",
+ }
+ form_data: dict[str, Any] = {
+ "slice_id": 1,
+ "adhoc_filters": [simple_adhoc_filter],
+ }
+
+ assert not _sql_filters_modified(query_context, form_data, stored_chart,
None)
+
+
+def test_query_context_modified_sql_filter_injection_blocked(
+ mocker: MockerFixture,
+) -> None:
+ """End-to-end: query_context_modified rejects injected SQL filters."""
+ query_context = mocker.MagicMock()
Review Comment:
**Suggestion:** The test is labeled end-to-end but invokes
`query_context_modified` directly, so it does not exercise the guest-user check
or the exception path in `raise_for_access`. A regression that removes or
bypasses this predicate from the guest request flow would still pass this test;
invoke the real access-checking path with a guest user and assert that the
security exception is raised. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Direct helper testing cannot detect authorization wiring regressions.
- ⚠️ Embedded chart requests could bypass SQL tampering rejection.
- ⚠️ Exception behavior remains unverified at the security boundary.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cace13421fd84a77a3a9a76f64163e5c&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=cace13421fd84a77a3a9a76f64163e5c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/manager_test.py
**Line:** 3926:3927
**Comment:**
*Api Mismatch: The test is labeled end-to-end but invokes
`query_context_modified` directly, so it does not exercise the guest-user check
or the exception path in `raise_for_access`. A regression that removes or
bypasses this predicate from the guest request flow would still pass this test;
invoke the real access-checking path with a guest user and assert that the
security exception is raised.
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%2F43111&comment_hash=d121d7f1e860a9e82ca19f1d6fd6a222eb615ea2a401bf6ffe27fec592440949&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43111&comment_hash=d121d7f1e860a9e82ca19f1d6fd6a222eb615ea2a401bf6ffe27fec592440949&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]
