codeant-ai-for-open-source[bot] commented on code in PR #40409:
URL: https://github.com/apache/superset/pull/40409#discussion_r3325860493
##########
tests/unit_tests/sql/parse_tests.py:
##########
@@ -1164,6 +1164,29 @@ def test_has_mutation(engine: str, sql: str, expected:
bool) -> None:
assert SQLScript(sql, engine).has_mutation() == expected
[email protected](
+ "engine, sql, expected",
+ [
+ # Plain SELECT parses to a proper AST node.
+ ("postgresql", "SELECT * FROM foo", False),
+ # CALL parses to ``exp.Command`` on Postgres.
+ ("postgresql", "CALL my_proc(1);", True),
+ # A script that mixes a parseable statement with an unparseable one
+ # is still flagged so strict scoping can refuse the whole script.
+ ("postgresql", "SELECT 1; CALL my_proc();", True),
+ # Kusto KQL statements aren't ``SQLStatement`` instances and must be
+ # skipped, not falsely flagged.
+ ("kustokql", "print 1", False),
Review Comment:
**Suggestion:** This test encodes a fail-open security behavior for strict
dataset matching: Kusto statements are currently treated as "parseable"
(`False`) even though they are not modeled by sqlglot and table extraction is
unsupported. Under `force_dataset_match=True`, that lets opaque Kusto queries
bypass the unparseable-statement guard. Update this expectation to fail closed
(or remove this case and add a strict-scoping denial test) so
unsupported/opaque statements are rejected instead of silently authorized.
[security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ SQL Lab Kusto queries bypass strict dataset-level authorization.
- ⚠️ Data access roles not enforced for Kusto engine queries.
- ⚠️ Test cements fail-open behavior for Kusto strict scoping.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure an Azure Data Explorer (Kusto) database using the
`KustoKqlEngineSpec` with
engine `"kustokql"` as defined in
`superset/db_engine_specs/kusto.py:160-176`. Create a
SQL Lab query record (`superset/models/sql_lab.py`, class `Query`) pointing
at this
database with a simple KQL statement like `print 1`.
2. When SQL Lab performs the execute pre-flight,
`CanAccessQueryValidatorImpl.validate` in
`superset/sqllab/validators.py:10-18` is called. This method unconditionally
invokes
`security_manager.raise_for_access(query=query, template_params=...,
force_dataset_match=True)`, putting the request on the strict-scoping path
for any engine,
including `"kustokql"`.
3. Inside `SupersetSecurityManager.raise_for_access` in
`superset/security/manager.py:2938-2953`, the code calls
`process_jinja_sql(query.sql,
database, template_params)`, which constructs a `SQLScript` in
`superset/sql/parse.py:1569-1568`. For a Kusto database,
`database.db_engine_spec.engine`
is `"kustokql"`, so `SQLScript.__init__` at `superset/sql/parse.py:20-29`
uses
`statement_class = self.special_engines.get(engine, SQLStatement)` and
creates
`KustoKQLStatement` instances (`superset/sql/parse.py:1111-1143`) for each
statement in
the script.
4. The strict unparseable-statement guard in
`SupersetSecurityManager.raise_for_access`
checks `parse_result.script.has_unparseable_statement` at
`superset/security/manager.py:2949-2952`.
`SQLScript.has_unparseable_statement` (defined
at `superset/sql/parse.py:1368-1376`) iterates `self.statements` and *skips*
any statement
that is not an instance of `SQLStatement`, per the comment `# Non-sqlglot
engines (e.g.
KQL) are handled separately.` Since all Kusto statements are
`KustoKQLStatement` instances
(not `SQLStatement`), the property always returns `False` for KQL scripts,
so the guard
that would deny unparseable/dynamic SQL never fires for Kusto.
5. For Kusto, `_extract_tables_from_statement` in `KustoKQLStatement`
(superset/sql/parse.py:1188-1198) logs a warning `"Kusto KQL doesn't support
table
extraction. This means that data access roles will not be enforced by
Superset in the
database."` and returns an empty table set. Consequently,
`process_jinja_sql` returns
`parse_result.tables = set()`, and the strict dataset-match loop in
`SupersetSecurityManager.raise_for_access` at
`superset/security/manager.py:2950-2976`
sees `tables` empty and never denies anything, even under
`force_dataset_match=True`. The
new unit test `test_has_unparseable_statement` in
`tests/unit_tests/sql/parse_tests.py:1182-1187` explicitly asserts that
`SQLScript("print
1", "kustokql").has_unparseable_statement is False` for the Kusto case
(lines 1177-1179),
locking in this behavior where opaque Kusto scripts bypass the
unparseable-statement
strict-scoping guard and any per-table dataset matching.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f952f2b14b9343d4b4565db76e2231dc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f952f2b14b9343d4b4565db76e2231dc&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/sql/parse_tests.py
**Line:** 1177:1179
**Comment:**
*Security: This test encodes a fail-open security behavior for strict
dataset matching: Kusto statements are currently treated as "parseable"
(`False`) even though they are not modeled by sqlglot and table extraction is
unsupported. Under `force_dataset_match=True`, that lets opaque Kusto queries
bypass the unparseable-statement guard. Update this expectation to fail closed
(or remove this case and add a strict-scoping denial test) so
unsupported/opaque statements are rejected instead of silently authorized.
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%2F40409&comment_hash=b157f747cbb332536113c0a83ad41fe6d4e254537a614873722608c82a8f3b90&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40409&comment_hash=b157f747cbb332536113c0a83ad41fe6d4e254537a614873722608c82a8f3b90&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]
