codeant-ai-for-open-source[bot] commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3762477711
##########
superset/models/helpers.py:
##########
@@ -329,6 +329,65 @@ def validate_stored_expression_at_query_time(
return expression
+def validate_rendered_expression(
+ expression: str,
+ database: Database,
+ catalog: str | None,
+ schema: str | None,
+) -> str:
+ """
+ Apply the stored-expression validation policy to a rendered expression.
+
+ Query-time counterpart to ``validate_stored_expression``: it runs on the
+ already-rendered expression that is embedded via ``literal_column`` and
+ applies the same policy, failing closed on unparseable results.
+
+ :param expression: the rendered expression
+ :returns: the expression to embed, possibly rewritten with RLS predicates
+ :raises SupersetSecurityException: on multi-statement, set-operation, or
+ disallowed sub-query expressions
+ :raises QueryObjectValidationError: if the clause fails sanitization
+ """
+ engine = database.backend
+ wrapped = f"SELECT {expression}"
+
+ try:
+ parsed = SQLStatement(wrapped, engine)
+ except SupersetParseError as ex:
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
+ message=_(
+ "Custom SQL fields cannot be parsed as a single SQL
statement."
+ ),
+ level=ErrorLevel.ERROR,
+ )
+ ) from ex
+
+ if parsed.is_set_operation():
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
+ message=_("Custom SQL fields cannot contain set operations."),
+ level=ErrorLevel.ERROR,
+ )
+ )
+
Review Comment:
**Suggestion:** The rendered-expression path raises
`SupersetSecurityException` directly for parse, set-operation, and
disallowed-subquery cases, unlike `validate_stored_expression_at_query_time`,
which converts these failures to `QueryObjectValidationError`. Callers invoke
this helper while constructing chart queries and do not perform that
conversion, so templated expressions can take a different error/HTTP path than
equivalent stored expressions. Convert these security failures to
`QueryObjectValidationError` before returning them to callers. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Templated chart expressions return 403 instead of validation errors.
- ⚠️ Chart query clients receive inconsistent failure semantics.
- ⚠️ Query construction may bypass normal 400 error handling.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7f7f8acae7344b3bae8c01b369eabe8b&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=7f7f8acae7344b3bae8c01b369eabe8b&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:** superset/models/helpers.py
**Line:** 356:375
**Comment:**
*Api Mismatch: The rendered-expression path raises
`SupersetSecurityException` directly for parse, set-operation, and
disallowed-subquery cases, unlike `validate_stored_expression_at_query_time`,
which converts these failures to `QueryObjectValidationError`. Callers invoke
this helper while constructing chart queries and do not perform that
conversion, so templated expressions can take a different error/HTTP path than
equivalent stored expressions. Convert these security failures to
`QueryObjectValidationError` before returning them to callers.
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%2F42929&comment_hash=bafee31cda210ba19ea4418d9bc457ab837f1e77500e5d419b55d23340998a59&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42929&comment_hash=bafee31cda210ba19ea4418d9bc457ab837f1e77500e5d419b55d23340998a59&reaction=dislike'>👎</a>
##########
superset/utils/rls.py:
##########
@@ -204,6 +204,10 @@ def collect_rls_predicates_for_sql(
}
)
except Exception:
- # If we can't parse the SQL, return empty list
- # This ensures RLS application failure doesn't break caching
- return []
+ # The applicable predicates could not be determined (e.g. the SQL
+ # could not be parsed); scope the cache key to the current user.
+ from superset.utils.core import (
+ get_user_id, # pylint: disable=import-outside-toplevel
+ )
+
+ return [f"rls-predicates-unresolved:{get_user_id()}"]
Review Comment:
**Suggestion:** The unresolved fallback is not isolated for guest-token
users: `GuestUser` has no normal database user ID, so `get_user_id()` returns
`None` for every guest token. If two guest tokens have different RLS clauses or
guest attributes, they both receive `rls-predicates-unresolved:None` and can
reuse each other’s virtual-dataset cache entries. Include a per-token identity
or a deterministic representation of the guest token’s RLS context in this
fallback key. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Embedded guests can reuse cached results across RLS contexts.
- ❌ Virtual-dataset responses may expose another guest's permitted rows.
- ⚠️ Affects unparseable or templated virtual-dataset SQL.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2615002176f945d382ec9f0971262260&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=2615002176f945d382ec9f0971262260&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:** superset/utils/rls.py
**Line:** 209:213
**Comment:**
*Security: The unresolved fallback is not isolated for guest-token
users: `GuestUser` has no normal database user ID, so `get_user_id()` returns
`None` for every guest token. If two guest tokens have different RLS clauses or
guest attributes, they both receive `rls-predicates-unresolved:None` and can
reuse each other’s virtual-dataset cache entries. Include a per-token identity
or a deterministic representation of the guest token’s RLS context in this
fallback key.
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%2F42929&comment_hash=542e096d311e7f58e89e29bba01b56c5f031fbe864cd83587f6708967377e535&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42929&comment_hash=542e096d311e7f58e89e29bba01b56c5f031fbe864cd83587f6708967377e535&reaction=dislike'>👎</a>
##########
superset/connectors/sqla/models.py:
##########
@@ -1215,6 +1217,14 @@ def get_sqla_col(
msg=msg,
)
) from ex
+ if expression != self.expression:
+ # Re-check the rendered expression before embedding it.
+ expression = validate_rendered_expression(
+ expression,
+ self.database,
+ self.table.catalog if self.table else None,
+ self.table.schema if self.table else None,
+ )
Review Comment:
**Suggestion:** The rendered-expression validator raises
`SupersetSecurityException` directly for parse, set-operation, and
disallowed-subquery failures, while the existing `_validate_stored_expression`
path converts these failures into `QueryObjectValidationError`. Because this
new call is outside a conversion handler, a Jinja-rendered invalid expression
can escape as a raw 403/security exception instead of following the established
query-object validation error path, causing inconsistent API behavior across
stored and rendered expressions. Convert the validation exception to the same
`QueryObjectValidationError` contract used by the existing validator (and apply
the same handling in the timestamp and metric call sites). [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Templated column expressions return inconsistent HTTP statuses.
- ⚠️ Templated metric expressions bypass query validation handling.
- ⚠️ Clients receive security errors instead of chart validation errors.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cd0a6c1335da4188a579fa9dc1ac6362&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=cd0a6c1335da4188a579fa9dc1ac6362&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:** superset/connectors/sqla/models.py
**Line:** 1220:1227
**Comment:**
*Api Mismatch: The rendered-expression validator raises
`SupersetSecurityException` directly for parse, set-operation, and
disallowed-subquery failures, while the existing `_validate_stored_expression`
path converts these failures into `QueryObjectValidationError`. Because this
new call is outside a conversion handler, a Jinja-rendered invalid expression
can escape as a raw 403/security exception instead of following the established
query-object validation error path, causing inconsistent API behavior across
stored and rendered expressions. Convert the validation exception to the same
`QueryObjectValidationError` contract used by the existing validator (and apply
the same handling in the timestamp and metric call sites).
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%2F42929&comment_hash=73487075d6b86c3e19873d742b3ea58e0a748f5826cbe4204ec7755f94108032&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42929&comment_hash=73487075d6b86c3e19873d742b3ea58e0a748f5826cbe4204ec7755f94108032&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]
