codeant-ai-for-open-source[bot] commented on code in PR #38452:
URL: https://github.com/apache/superset/pull/38452#discussion_r2977869873
##########
superset/sql/parse.py:
##########
@@ -1414,6 +1476,22 @@ def extract_tables_from_statement(
for source in scope.sources.values()
if isinstance(source, exp.Table) and not is_cte(source, scope)
]
+ # traverse_scope omits the primary target table for DML statements
+ # (UPDATE/DELETE/INSERT); add it explicitly so RLS checks cover it.
+ if isinstance(statement, (exp.Update, exp.Delete)) and isinstance(
Review Comment:
**Suggestion:** Target-table extraction for DML statements still misses
`MERGE`, so mutating `MERGE INTO ...` statements can have an empty table set
and bypass RLS-protected-table mutation blocking in SQL Lab. Include
`exp.Merge` in the explicit DML target handling. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ MERGE can evade RLS protected-table mutation blocking.
- ❌ SQL Lab mutation guard misses protected target tables.
```
</details>
```suggestion
if isinstance(statement, (exp.Update, exp.Delete, exp.Merge)) and
isinstance(
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable SQL Lab RLS enforcement (`RLS_IN_SQLLAB`) and execute SQL through
`execute_sql_statements()` flow in `superset/sql_lab.py`.
2. Submit a mutating `MERGE INTO protected_table ...` statement;
`is_mutating()` returns
True because `exp.Merge` is in `mutating_nodes`
(`superset/sql/parse.py:713-717`).
3. Table extraction runs via `BaseSQLStatement.__init__`
(`superset/sql/parse.py:403`) ->
`extract_tables_from_statement()` (`superset/sql/parse.py:1439`).
4. In `extract_tables_from_statement()`, explicit target handling covers only
`Update/Delete/Insert` (`superset/sql/parse.py:1481-1494`), not `Merge`;
sqlglot scope
traversal omits the MERGE target table.
5. In `sql_lab.py:437-445`, RLS mutation-block check iterates `for table in
statement.tables`; when empty, no `get_predicates_for_table()` check happens
and no
`SupersetSecurityException` is raised.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/sql/parse.py
**Line:** 1481:1481
**Comment:**
*Security: Target-table extraction for DML statements still misses
`MERGE`, so mutating `MERGE INTO ...` statements can have an empty table set
and bypass RLS-protected-table mutation blocking in SQL Lab. Include
`exp.Merge` in the explicit DML target handling.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38452&comment_hash=b5397852990232908efa92dcc24396865732cc483fd040d9b8322305830842c3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38452&comment_hash=b5397852990232908efa92dcc24396865732cc483fd040d9b8322305830842c3&reaction=dislike'>👎</a>
##########
superset/sql/parse.py:
##########
@@ -684,14 +720,21 @@ def is_mutating(self) -> bool:
exp.Alter,
)
- for node_type in mutating_nodes:
- if self._parsed.find(node_type):
+ for node in self._parsed.walk():
+ if isinstance(node, mutating_nodes):
return True
- # depending on the dialect (Oracle, MS SQL) the `ALTER` is parsed as a
+ # depending on the dialect (Oracle, MS SQL) some DML is parsed as a
# command, not an expression - check at root level
- if isinstance(self._parsed, exp.Command) and self._parsed.name ==
"ALTER":
- return True # pragma: no cover
+ if isinstance(self._parsed, exp.Command) and self._parsed.name.upper()
in {
Review Comment:
**Suggestion:** `COPY` statements parsed as `exp.Copy` are not covered by
the `exp.Command` branch, so they are currently treated as non-mutating and can
bypass DML/RLS mutation guards. Add an explicit `exp.Copy` check before the
command-name check. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ COPY can bypass allow_dml mutation restriction.
- ❌ RLS mutation-only checks skipped for COPY statements.
```
</details>
```suggestion
if isinstance(self._parsed, exp.Copy):
return True
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Execute a PostgreSQL `COPY ...` statement through SQL Lab validation path
(`superset/sql_lab.py`).
2. sqlglot parses this as `exp.Copy` (dedicated node), not `exp.Command`;
current
`is_mutating()` checks `exp.Command` names for `"COPY"` only at
`superset/sql/parse.py:729-738`.
3. `is_mutating()` also misses `exp.Copy` in `mutating_nodes`
(`superset/sql/parse.py:713-721`), so it returns False.
4. `parsed_script.has_mutation()` (`superset/sql/parse.py:1384`) can
therefore return
False, bypassing `if parsed_script.has_mutation() and not
database.allow_dml` at
`superset/sql_lab.py:431`.
5. With `RLS_IN_SQLLAB` enabled, statement enters non-mutating branch
(`sql_lab.py:446-447`) instead of protected-table mutation blocking logic.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/sql/parse.py
**Line:** 729:729
**Comment:**
*Security: `COPY` statements parsed as `exp.Copy` are not covered by
the `exp.Command` branch, so they are currently treated as non-mutating and can
bypass DML/RLS mutation guards. Add an explicit `exp.Copy` check before the
command-name check.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38452&comment_hash=93dd60f0fdb6a0a5ff48c154fa23c4d71d4876236f8a9c987a7a6de515731bcc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38452&comment_hash=93dd60f0fdb6a0a5ff48c154fa23c4d71d4876236f8a9c987a7a6de515731bcc&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]
