You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/08/09 03:00:37 UTC
[PR] fix(dataset): improve dataset, report, and expression validation [superset]
rusackas opened a new pull request, #42929:
URL: https://github.com/apache/superset/pull/42929
### SUMMARY
Improves validation in the dataset-update and report/alert command paths and in stored/rendered SQL expression handling.
- Repointing a physical dataset at a different table (or converting virtual→physical) now runs the same data-access check the create path enforces; editorship alone is not treated as data access.
- `fetch_values_predicate` is routed through the same parser-based validator as stored column and metric expressions, at save time and again when it is embedded at query time.
- Alert queries are validated when saved and when run: single-statement only, read-only unless the database allows DML, and authorized for the saving/executing user; templated SQL defers the static checks to execution time.
- Rendered Jinja column/metric expressions are re-validated before being embedded, since save-time validation only sees the pre-render skeleton.
- When row-level-security predicates apply to a virtual dataset but cannot be injected, the query is aborted instead of running unfiltered, and an unresolved RLS state scopes the cache key to the current user.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable (backend behavior).
### TESTING INSTRUCTIONS
`pytest tests/unit_tests/commands/dataset/ tests/unit_tests/commands/report/ tests/unit_tests/connectors/sqla/models_test.py tests/unit_tests/models/test_virtual_dataset_format.py`
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3763263328
##########
superset/commands/report/base.py:
##########
@@ -58,6 +68,52 @@ def run(self) -> Any:
def validate(self) -> None:
pass
+ def validate_alert_query(
+ self,
+ database: Database,
+ sql: str,
+ exceptions: list[ValidationError],
+ ) -> None:
+ """
+ Validate alert SQL at save time: it must parse as a single statement,
+ must not mutate state unless the database allows DML, and the saving
+ user must be authorized for the tables it reads. Templated SQL that
+ only parses after rendering is validated at execution time on the
+ rendered query.
+ """
+ contains_jinja = bool(_JINJA_BLOCK_RE.search(sql))
+ try:
+ script = SQLScript(sql, engine=database.backend)
+ except SupersetParseError as ex:
+ if not contains_jinja:
+ exceptions.append(
+ ValidationError(
+ _("Invalid SQL: %(error)s", error=ex.error.message),
+ field_name="sql",
+ )
+ )
+ return
+ if len(script.statements) != 1:
+ exceptions.append(AlertQueryMultipleStatementsValidationError())
Review Comment:
This only skips the save-time check on a parse failure, which is the case that actually matters: Jinja confined to a string literal (e.g. a templated date bound) still parses as valid SQL, and the statement-count/DML/access checks on that literal text are still correct and worth running early. Blanket-skipping every query that contains `{{ }}` would waive save-time validation for that common case too, which is a step back, not forward -- and the rendered SQL is re-validated at execution time regardless (_validate_rendered_sql in alert.py).
##########
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:
Fixed. `validate_rendered_expression` now converts every failure mode (parse, set operation, disallowed sub-query, sanitization) to `QueryObjectValidationError`, matching `validate_stored_expression_at_query_time`, instead of letting `SupersetSecurityException` escape uncaught from the call sites.
##########
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:
Same fix as the models.py thread: `validate_rendered_expression` now always converts to `QueryObjectValidationError`, so this call site gets consistent semantics without needing its own conversion handler.
##########
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:
Fixed. The unresolved-predicates cache-key fallback now keys guest users by a hash of their token's RLS rules and resources instead of `get_user_id()`, which is `None` for every guest and was collapsing all of them onto the same cache entry.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #42929:
URL: https://github.com/apache/superset/pull/42929#issuecomment-5229497574
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42929?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `15.00000%` with `102 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.07%. Comparing base ([`3b164e4`](https://app.codecov.io/gh/apache/superset/commit/3b164e4270860ac07223d4df1a60ca7b56312362?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0b60db8`](https://app.codecov.io/gh/apache/superset/commit/0b60db8e6d390940ed079f75aa1ff7d10205dcb5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/42929?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/models/helpers.py](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&filepath=superset%2Fmodels%2Fhelpers.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbW9kZWxzL2hlbHBlcnMucHk=) | 7.40% | [25 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/dataset/update.py](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&filepath=superset%2Fcommands%2Fdataset%2Fupdate.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGF0YXNldC91cGRhdGUucHk=) | 4.34% | [22 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/report/base.py](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&filepath=superset%2Fcommands%2Freport%2Fbase.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvcmVwb3J0L2Jhc2UucHk=) | 23.07% | [20 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/report/alert.py](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&filepath=superset%2Fcommands%2Freport%2Falert.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvcmVwb3J0L2FsZXJ0LnB5) | 18.75% | [13 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/connectors/sqla/models.py](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&filepath=superset%2Fconnectors%2Fsqla%2Fmodels.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | 0.00% | [9 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/report/update.py](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&filepath=superset%2Fcommands%2Freport%2Fupdate.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvcmVwb3J0L3VwZGF0ZS5weQ==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/report/exceptions.py](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&filepath=superset%2Fcommands%2Freport%2Fexceptions.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvcmVwb3J0L2V4Y2VwdGlvbnMucHk=) | 66.66% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/report/create.py](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&filepath=superset%2Fcommands%2Freport%2Fcreate.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvcmVwb3J0L2NyZWF0ZS5weQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/rls.py](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&filepath=superset%2Futils%2Frls.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvcmxzLnB5) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42929?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (3b164e4) and HEAD (0b60db8). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (3b164e4) | HEAD (0b60db8) |
>|------|------|------|
>|python|47|1|
>|presto|8|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #42929 +/- ##
==========================================
- Coverage 66.41% 57.07% -9.35%
==========================================
Files 2858 2858
Lines 161446 161558 +112
Branches 37190 37207 +17
==========================================
- Hits 107222 92206 -15016
- Misses 52187 68495 +16308
+ Partials 2037 857 -1180
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/42929/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/42929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/42929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/42929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/42929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.14% <15.00%> (-0.04%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/42929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.14% <15.00%> (-19.08%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/42929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/42929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/42929?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #42929:
URL: https://github.com/apache/superset/pull/42929
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #42929:
URL: https://github.com/apache/superset/pull/42929#issuecomment-5260026606
@sha174n thanks for the read-through. Rebased over #42936 and dropped the duplicate database-access check it added in `_validate_dataset_source` - the existing `_validate_sql_access`/physical-repoint checks already covered repointing, so the two were double-calling `raise_for_access` and breaking #42936's own tests.
On the fail-closed question: yes, intentional. `validate_rendered_expression` only runs on Jinja output rendered at request time, so failing closed there is the safer default. The fallback in the stored-expression check exists for legacy dialect-specific syntax already sitting in the DB, which doesn't apply to freshly rendered SQL.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42929:
URL: https://github.com/apache/superset/pull/42929#issuecomment-5261398942
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3742447985
##########
superset/commands/report/alert.py:
##########
@@ -181,6 +183,18 @@ def _get_alert_metadata_from_object(self) -> dict[str, Any]:
"execution_id": self._execution_id,
}
+ def _validate_rendered_sql(self, rendered_sql: str) -> None:
+ """
+ Enforce SQL-level constraints on the rendered alert query: a single
+ statement, and no mutations unless the database allows DML.
+ """
+ database = self._report_schedule.database
+ script = SQLScript(rendered_sql, engine=database.backend)
+ if len(script.statements) > 1:
+ raise AlertQueryError(message=_("Alert query must be a single statement"))
+ if script.has_mutation() and not database.allow_dml:
Review Comment:
**Suggestion:** The validation only rejects more than one statement, so an empty or comment-only alert query with zero parsed statements passes both save-time and execution-time validation. `apply_limit_to_sql` then indexes the last statement and raises an error when the alert runs instead of returning a field validation error. Require exactly one statement by rejecting zero statements as well. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Invalid alerts fail only when scheduled.
- ⚠️ Users receive generic execution errors.
- ⚠️ Alert configuration validation permits unusable schedules.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9fbe1ef6b4c14b82aa06f2cd1284b165&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=9fbe1ef6b4c14b82aa06f2cd1284b165&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:** superset/commands/report/alert.py
**Line:** 193:195
**Comment:**
*Incorrect Condition Logic: The validation only rejects more than one statement, so an empty or comment-only alert query with zero parsed statements passes both save-time and execution-time validation. `apply_limit_to_sql` then indexes the last statement and raises an error when the alert runs instead of returning a field validation error. Require exactly one statement by rejecting zero statements as well.
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=206211c032d0a43e444f7e1163e7d7131c28bfcdfe194304234ae63eea314d83&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42929&comment_hash=206211c032d0a43e444f7e1163e7d7131c28bfcdfe194304234ae63eea314d83&reaction=dislike'>👎</a>
##########
superset/commands/report/alert.py:
##########
@@ -196,6 +210,7 @@ def _execute_query(self) -> pd.DataFrame:
try:
rendered_sql = sql_template.process_template(self._report_schedule.sql)
Review Comment:
**Suggestion:** The authorization and read-only checks run against `rendered_sql`, but execution uses `limited_rendered_sql` after the configurable SQL mutator has transformed it. A mutator can add statements, change referenced tables, or introduce mutations after these checks, so the database may execute SQL that was never authorized. Validate the final SQL immediately before execution, or ensure the authorization and mutation checks cover the exact SQL sent to the database. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Mutated alert SQL can bypass matching authorization checks.
- ⚠️ Configured SQL mutators can introduce execution-time query failures.
- ⚠️ Alert execution differs from validated SQL behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3ad932bc6f194760a294e936fbcc88be&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=3ad932bc6f194760a294e936fbcc88be&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:** superset/commands/report/alert.py
**Line:** 212:223
**Comment:**
*Security: The authorization and read-only checks run against `rendered_sql`, but execution uses `limited_rendered_sql` after the configurable SQL mutator has transformed it. A mutator can add statements, change referenced tables, or introduce mutations after these checks, so the database may execute SQL that was never authorized. Validate the final SQL immediately before execution, or ensure the authorization and mutation checks cover the exact SQL sent to the database.
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=82cc22958e160932cb522b9aad6ba809067a0129a706b42a4fc65832d3cf7269&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42929&comment_hash=82cc22958e160932cb522b9aad6ba809067a0129a706b42a4fc65832d3cf7269&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42929:
URL: https://github.com/apache/superset/pull/42929#issuecomment-5230197078
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3742517879
##########
superset/commands/report/alert.py:
##########
@@ -196,6 +210,7 @@ def _execute_query(self) -> pd.DataFrame:
try:
rendered_sql = sql_template.process_template(self._report_schedule.sql)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require re-authorization after trusted, operator-controlled SQL mutators or limit wrappers in alert query execution; these transformations are within the established trust boundary.
**Applied to:**
- `superset/commands/report/alert.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3763264557
##########
superset/commands/report/base.py:
##########
@@ -58,6 +68,52 @@ def run(self) -> Any:
def validate(self) -> None:
pass
+ def validate_alert_query(
+ self,
+ database: Database,
+ sql: str,
+ exceptions: list[ValidationError],
+ ) -> None:
+ """
+ Validate alert SQL at save time: it must parse as a single statement,
+ must not mutate state unless the database allows DML, and the saving
+ user must be authorized for the tables it reads. Templated SQL that
+ only parses after rendering is validated at execution time on the
+ rendered query.
+ """
+ contains_jinja = bool(_JINJA_BLOCK_RE.search(sql))
+ try:
+ script = SQLScript(sql, engine=database.backend)
+ except SupersetParseError as ex:
+ if not contains_jinja:
+ exceptions.append(
+ ValidationError(
+ _("Invalid SQL: %(error)s", error=ex.error.message),
+ field_name="sql",
+ )
+ )
+ return
+ if len(script.statements) != 1:
+ exceptions.append(AlertQueryMultipleStatementsValidationError())
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> For templated SQL that parses successfully, retain save-time statement-count, DML, and access validation; only defer validation when parsing fails due to unresolved Jinja.
**Applied to:**
- `superset/commands/report/base.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3762478438
##########
superset/commands/dataset/update.py:
##########
@@ -183,21 +192,33 @@ def _validate_dataset_source(self, exceptions: list[ValidationError]) -> None:
):
exceptions.append(DatasetExistsValidationError(table))
- self._validate_sql_access(db, catalog, schema, exceptions)
+ # Repointing a physical dataset (or converting a virtual dataset to a
+ # physical one) runs the same data-access check as the create path.
+ sql = self._properties.get("sql", self._model.sql)
+ if not sql and (
+ source_changed or ("sql" in self._properties and self._model.sql)
+ ):
+ try:
+ security_manager.raise_for_access(database=db, table=table)
+ except SupersetSecurityException as ex:
+ exceptions.append(DatasetDataAccessIsNotAllowed(ex.error.message))
+
Review Comment:
**Suggestion:** The database-repoint path already calls `raise_for_access` for the target table above, but this new block runs the same check again whenever `database_changed` is true. If access is denied, the same validation error is appended twice, producing duplicate errors in the update response. Avoid repeating the table-level check when the database-repoint check has already covered it. [logic error]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Dataset updates to unauthorized replacement databases return duplicate validation errors.
- ⚠️ API clients receive redundant `DatasetDataAccessIsNotAllowed` entries.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b5f09afd0de7491196cfb87cb6194d6f&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=b5f09afd0de7491196cfb87cb6194d6f&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/commands/dataset/update.py
**Line:** 195:205
**Comment:**
*Logic Error: The database-repoint path already calls `raise_for_access` for the target table above, but this new block runs the same check again whenever `database_changed` is true. If access is denied, the same validation error is appended twice, producing duplicate errors in the update response. Avoid repeating the table-level check when the database-repoint check has already covered it.
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=f8061338afbe0f58bad2d70af77dacbf61b2a8ecf14e4bcc067ab53af0d77bfb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42929&comment_hash=f8061338afbe0f58bad2d70af77dacbf61b2a8ecf14e4bcc067ab53af0d77bfb&reaction=dislike'>👎</a>
##########
superset/commands/report/base.py:
##########
@@ -58,6 +68,52 @@ def run(self) -> Any:
def validate(self) -> None:
pass
+ def validate_alert_query(
+ self,
+ database: Database,
+ sql: str,
+ exceptions: list[ValidationError],
+ ) -> None:
+ """
+ Validate alert SQL at save time: it must parse as a single statement,
+ must not mutate state unless the database allows DML, and the saving
+ user must be authorized for the tables it reads. Templated SQL that
+ only parses after rendering is validated at execution time on the
+ rendered query.
+ """
+ contains_jinja = bool(_JINJA_BLOCK_RE.search(sql))
+ try:
+ script = SQLScript(sql, engine=database.backend)
+ except SupersetParseError as ex:
+ if not contains_jinja:
+ exceptions.append(
+ ValidationError(
+ _("Invalid SQL: %(error)s", error=ex.error.message),
+ field_name="sql",
+ )
+ )
+ return
+ if len(script.statements) != 1:
+ exceptions.append(AlertQueryMultipleStatementsValidationError())
Review Comment:
**Suggestion:** Templated SQL is supposed to defer static validation until execution, but `contains_jinja` is only consulted when parsing raises an exception. If the unresolved template happens to parse as a single SQL statement, this code still applies statement-count, DML, and authorization checks to the unrendered SQL and can reject valid templates based on placeholder-shaped table references. Skip all save-time static checks whenever Jinja is present; the rendered execution path already performs the required validation and authorization. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Valid Jinja-based alerts can fail during create or update validation.
- ⚠️ Dynamic table references are checked against unresolved SQL.
- ⚠️ Alert execution never reaches rendered-query validation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0ea0dac794994b66aac03f185fe0d3a3&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=0ea0dac794994b66aac03f185fe0d3a3&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/commands/report/base.py
**Line:** 84:97
**Comment:**
*Api Mismatch: Templated SQL is supposed to defer static validation until execution, but `contains_jinja` is only consulted when parsing raises an exception. If the unresolved template happens to parse as a single SQL statement, this code still applies statement-count, DML, and authorization checks to the unrendered SQL and can reject valid templates based on placeholder-shaped table references. Skip all save-time static checks whenever Jinja is present; the rendered execution path already performs the required validation and authorization.
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=d002c91052eb845d08f037b1df23ec4c5abafb532edf48fd25110a867cb3fab5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42929&comment_hash=d002c91052eb845d08f037b1df23ec4c5abafb532edf48fd25110a867cb3fab5&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3764176393
##########
superset/models/helpers.py:
##########
@@ -329,6 +329,62 @@ def validate_stored_expression_at_query_time(
return expression
+def validate_rendered_expression(
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to add unit tests for `validate_rendered_expression` is appropriate, as it ensures the new public function is properly covered, matching the testing pattern used for `validate_stored_expression_at_query_time`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42929:
URL: https://github.com/apache/superset/pull/42929#issuecomment-5229485621
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3763243807
##########
superset/commands/dataset/update.py:
##########
@@ -183,21 +192,33 @@ def _validate_dataset_source(self, exceptions: list[ValidationError]) -> None:
):
exceptions.append(DatasetExistsValidationError(table))
- self._validate_sql_access(db, catalog, schema, exceptions)
+ # Repointing a physical dataset (or converting a virtual dataset to a
+ # physical one) runs the same data-access check as the create path.
+ sql = self._properties.get("sql", self._model.sql)
+ if not sql and (
+ source_changed or ("sql" in self._properties and self._model.sql)
+ ):
+ try:
+ security_manager.raise_for_access(database=db, table=table)
+ except SupersetSecurityException as ex:
+ exceptions.append(DatasetDataAccessIsNotAllowed(ex.error.message))
+
Review Comment:
Good catch. Removed the redundant checks: the physical-repoint table check now only runs when the database itself did not change (the repoint check above already covers that pair), and the SQL-access re-check no longer fires on a bare `database_changed` without an actual SQL change.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42929:
URL: https://github.com/apache/superset/pull/42929#issuecomment-5229506054
<!-- Bito Reply -->
The security concern is valid. The current implementation of `security_manager.raise_for_access` with `sql=rendered_sql` may rely on default behaviors that are too permissive for raw SQL queries, potentially allowing access to tables not explicitly granted via dataset permissions. To resolve this, you should ensure that the authorization check strictly validates the tables referenced in the rendered SQL against the user's authorized datasets, rather than relying on broad database-level permissions.
To implement this, you can update the authorization logic in `superset/commands/report/alert.py` to extract the tables from the rendered SQL and verify each one against the user's permissions, similar to how SQL Lab enforces strict scoping.
**superset/commands/report/alert.py**
```
# Run table-level authorization as the executing user against
# the rendered SQL.
try:
# Use a stricter validator that parses the SQL and checks
# each referenced table against authorized datasets.
security_manager.raise_for_access(
database=self._report_schedule.database,
sql=rendered_sql,
strict_dataset_matching=True,
)
except SupersetSecurityException as ex:
raise AlertQueryError(
message=_("Alert query failed the authorization check")
) from ex
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3742516981
##########
superset/commands/report/alert.py:
##########
@@ -220,6 +235,17 @@ def _execute_query(self) -> pd.DataFrame:
raise ReportScheduleExecutorNotFoundError(username)
with override_user(user):
+ # Run table-level authorization as the executing user against
+ # the rendered SQL.
+ try:
+ security_manager.raise_for_access(
+ database=self._report_schedule.database,
+ sql=rendered_sql,
+ )
Review Comment:
Agreed. Added force_dataset_match=True to both the save-time and execution-time checks so alert SQL is scoped the same way SQL Lab is.
##########
superset/commands/report/alert.py:
##########
@@ -181,6 +183,18 @@ def _get_alert_metadata_from_object(self) -> dict[str, Any]:
"execution_id": self._execution_id,
}
+ def _validate_rendered_sql(self, rendered_sql: str) -> None:
+ """
+ Enforce SQL-level constraints on the rendered alert query: a single
+ statement, and no mutations unless the database allows DML.
+ """
+ database = self._report_schedule.database
+ script = SQLScript(rendered_sql, engine=database.backend)
+ if len(script.statements) > 1:
+ raise AlertQueryError(message=_("Alert query must be a single statement"))
+ if script.has_mutation() and not database.allow_dml:
Review Comment:
Good point. Changed it to require exactly one statement, so an empty or comment-only query gets a proper validation error instead of failing later at run time.
##########
superset/commands/report/alert.py:
##########
@@ -196,6 +210,7 @@ def _execute_query(self) -> pd.DataFrame:
try:
rendered_sql = sql_template.process_template(self._report_schedule.sql)
Review Comment:
The SQL mutator and the limit wrapper are operator-controlled, so they sit inside the trust boundary described in SECURITY.md, and apply_limit only wraps the query with a LIMIT. Keeping the check on the rendered SQL rather than re-authorizing after trusted config runs.
##########
superset/models/helpers.py:
##########
@@ -3020,9 +3080,40 @@ def get_from_clause(
if rls_applied:
from_sql = parsed_script.format()
- except Exception as ex:
- # Log the error but don't fail - RLS application is best-effort
- logger.warning("Failed to apply RLS to virtual dataset SQL: %s", ex)
+ except Exception as ex: # pylint: disable=broad-except
+ # RLS injection failures fail closed: only continue when it is
+ # positively confirmed that no RLS predicates apply to the
+ # referenced tables; any other outcome aborts the query.
+ try:
+ rls_required = any(
+ get_predicates_for_table(
+ table.qualify(
+ catalog=self.catalog,
+ schema=self.schema or default_schema or "",
+ ),
+ self.database,
+ self.database.get_default_catalog(),
+ exclude_dataset_id=self_id,
+ )
+ for statement in parsed_script.statements
+ for table in statement.tables
+ )
+ except Exception: # pylint: disable=broad-except
Review Comment:
This one is intentional. It's a fail-closed path, so we want to catch anything and fall back to the safe behavior rather than narrow it to specific exception types.
##########
superset/commands/report/exceptions.py:
##########
@@ -40,6 +40,38 @@ def __init__(self) -> None:
super().__init__(_("Database does not exist"), field_name="database")
+class AlertQueryMultipleStatementsValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL containing multiple statements
+ """
+
+ def __init__(self) -> None:
+ super().__init__(
+ _("Alert query must be a single statement"),
+ field_name="sql",
+ )
+
+
+class AlertQueryDMLNotAllowedValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL that mutates state on a
+ database that does not allow DML
+ """
+
+ def __init__(self) -> None:
+ super().__init__(_("Alert query must be read-only"), field_name="sql")
+
+
+class AlertQueryDataAccessValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL referencing tables the user
+ is not authorized to query
+ """
+
+ def __init__(self, message: str) -> None:
+ super().__init__(message, field_name="sql")
+
Review Comment:
Added unit tests for the DML-not-allowed and data-access cases in base_test.py.
##########
tests/unit_tests/connectors/sqla/models_test.py:
##########
@@ -1486,6 +1486,33 @@ def test_validate_stored_expression_rejects_subquery_around_jinja(
)
+def test_get_sqla_col_revalidates_rendered_jinja_expression(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A Jinja block that renders into a sub-query must be rejected at query
+ time: save-time validation only sees the block as a placeholder, so the
+ rendered expression is re-validated before it is embedded via
+ ``literal_column``.
+ """
+ # A real Database (not a MagicMock) so the ORM relationship assignment on
+ # SqlaTable has a valid instance state; sqlite gives a concrete backend.
+ database = Database(database_name="t", sqlalchemy_uri="sqlite://")
+ mocker.patch("superset.models.helpers.is_feature_enabled", return_value=False)
Review Comment:
The validator resolves is_feature_enabled from the helpers module namespace, so patching it there is correct. The test passes and exercises the intended path.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3742441611
##########
superset/commands/report/alert.py:
##########
@@ -220,6 +235,17 @@ def _execute_query(self) -> pd.DataFrame:
raise ReportScheduleExecutorNotFoundError(username)
with override_user(user):
+ # Run table-level authorization as the executing user against
+ # the rendered SQL.
+ try:
+ security_manager.raise_for_access(
+ database=self._report_schedule.database,
+ sql=rendered_sql,
+ )
Review Comment:
**Suggestion:** Use strict dataset matching when authorizing the raw rendered alert SQL. The default `raise_for_access` behavior permits catalog or schema permissions and can authorize references to unregistered tables, allowing an executor with broad database-level access to run alert queries against tables for which no datasource access has been granted. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Executor can read unregistered database tables.
- ❌ Alert results can expose unauthorized datasource data.
- ⚠️ Authorization differs from SQL Lab strict scoping.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=46e444fe6d9f4342be193fb5b62a15f9&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=46e444fe6d9f4342be193fb5b62a15f9&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:** superset/commands/report/alert.py
**Line:** 241:244
**Comment:**
*Security: Use strict dataset matching when authorizing the raw rendered alert SQL. The default `raise_for_access` behavior permits catalog or schema permissions and can authorize references to unregistered tables, allowing an executor with broad database-level access to run alert queries against tables for which no datasource access has been granted.
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=604c703278b1eda45254f35b7a74efa573827b68357563235551581d21afc09e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42929&comment_hash=604c703278b1eda45254f35b7a74efa573827b68357563235551581d21afc09e&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3763972468
##########
superset/models/helpers.py:
##########
@@ -329,6 +329,62 @@ def validate_stored_expression_at_query_time(
return expression
+def validate_rendered_expression(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test coverage</b></div>
<div id="fix">
A new public function `validate_rendered_expression` has been added with no corresponding test coverage. The `validate_stored_expression_at_query_time` function above (line 270) has tests in `tests/unit_tests/models/helpers_test.py`; the new function should receive equivalent coverage.
</div>
</div>
<small><i>Code Review Run #5a1d2b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/models/helpers.py:
##########
@@ -3020,9 +3080,40 @@ def get_from_clause(
if rls_applied:
from_sql = parsed_script.format()
- except Exception as ex:
- # Log the error but don't fail - RLS application is best-effort
- logger.warning("Failed to apply RLS to virtual dataset SQL: %s", ex)
+ except Exception as ex: # pylint: disable=broad-except
+ # RLS injection failures fail closed: only continue when it is
+ # positively confirmed that no RLS predicates apply to the
+ # referenced tables; any other outcome aborts the query.
+ try:
+ rls_required = any(
+ get_predicates_for_table(
+ table.qualify(
+ catalog=self.catalog,
+ schema=self.schema or default_schema or "",
+ ),
+ self.database,
+ self.database.get_default_catalog(),
+ exclude_dataset_id=self_id,
+ )
+ for statement in parsed_script.statements
+ for table in statement.tables
+ )
+ except Exception: # pylint: disable=broad-except
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Broad exception clause detected</b></div>
<div id="fix">
Catching broad `Exception` is discouraged. Consider catching specific exceptions like `ValueError` or using `ExceptionGroup` to preserve exception context.
</div>
</div>
<small><i>Code Review Run #2ce454</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Avoid catching generic Exception</b></div>
<div id="fix">
Replace the broad `Exception` catch with more specific exception types to avoid silently masking unexpected errors. Other similar exception handlers exist at lines 3160 and 3163-3164 in this function.
</div>
</div>
<small><i>Code Review Run #5a1d2b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42929:
URL: https://github.com/apache/superset/pull/42929#issuecomment-5262942321
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3742451331
##########
superset/models/helpers.py:
##########
@@ -3020,9 +3080,40 @@ def get_from_clause(
if rls_applied:
from_sql = parsed_script.format()
- except Exception as ex:
- # Log the error but don't fail - RLS application is best-effort
- logger.warning("Failed to apply RLS to virtual dataset SQL: %s", ex)
+ except Exception as ex: # pylint: disable=broad-except
+ # RLS injection failures fail closed: only continue when it is
+ # positively confirmed that no RLS predicates apply to the
+ # referenced tables; any other outcome aborts the query.
+ try:
+ rls_required = any(
+ get_predicates_for_table(
+ table.qualify(
+ catalog=self.catalog,
+ schema=self.schema or default_schema or "",
+ ),
+ self.database,
+ self.database.get_default_catalog(),
+ exclude_dataset_id=self_id,
+ )
+ for statement in parsed_script.statements
+ for table in statement.tables
+ )
+ except Exception: # pylint: disable=broad-except
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Broad exception clause detected</b></div>
<div id="fix">
Catching broad `Exception` is discouraged. Consider catching specific exceptions like `ValueError` or using `ExceptionGroup` to preserve exception context.
</div>
</div>
<small><i>Code Review Run #2ce454</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/commands/report/exceptions.py:
##########
@@ -40,6 +40,38 @@ def __init__(self) -> None:
super().__init__(_("Database does not exist"), field_name="database")
+class AlertQueryMultipleStatementsValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL containing multiple statements
+ """
+
+ def __init__(self) -> None:
+ super().__init__(
+ _("Alert query must be a single statement"),
+ field_name="sql",
+ )
+
+
+class AlertQueryDMLNotAllowedValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL that mutates state on a
+ database that does not allow DML
+ """
+
+ def __init__(self) -> None:
+ super().__init__(_("Alert query must be read-only"), field_name="sql")
+
+
+class AlertQueryDataAccessValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL referencing tables the user
+ is not authorized to query
+ """
+
+ def __init__(self, message: str) -> None:
+ super().__init__(message, field_name="sql")
+
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test coverage for exceptions</b></div>
<div id="fix">
The new exception classes `AlertQueryDMLNotAllowedValidationError` (line 55) and `AlertQueryDataAccessValidationError` (line 65) are used in `validate_alert_query` (base.py:99-105) but have no unit test coverage. Add tests for: (1) DML statement on database with `allow_dml=False`, (2) SQL accessing unauthorized tables via `security_manager.raise_for_access`.
</div>
</div>
<small><i>Code Review Run #2ce454</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/connectors/sqla/models_test.py:
##########
@@ -1486,6 +1486,33 @@ def test_validate_stored_expression_rejects_subquery_around_jinja(
)
+def test_get_sqla_col_revalidates_rendered_jinja_expression(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A Jinja block that renders into a sub-query must be rejected at query
+ time: save-time validation only sees the block as a placeholder, so the
+ rendered expression is re-validated before it is embedded via
+ ``literal_column``.
+ """
+ # A real Database (not a MagicMock) so the ORM relationship assignment on
+ # SqlaTable has a valid instance state; sqlite gives a concrete backend.
+ database = Database(database_name="t", sqlalchemy_uri="sqlite://")
+ mocker.patch("superset.models.helpers.is_feature_enabled", return_value=False)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Wrong mock target in security test</b></div>
<div id="fix">
The mock at line 1501 patches `superset.models.helpers.is_feature_enabled`, but `validate_rendered_expression` reads `is_feature_enabled` via `superset.connectors.sqla.models` (module-level import). The mock is a dead intercept and does not suppress `ALLOW_ADHOC_SUBQUERY` — the test will silently pass without raising `SupersetSecurityException` even when the flag is False.
</div>
</div>
<small><i>Code Review Run #2ce454</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3742517424
##########
superset/models/helpers.py:
##########
@@ -3020,9 +3080,40 @@ def get_from_clause(
if rls_applied:
from_sql = parsed_script.format()
- except Exception as ex:
- # Log the error but don't fail - RLS application is best-effort
- logger.warning("Failed to apply RLS to virtual dataset SQL: %s", ex)
+ except Exception as ex: # pylint: disable=broad-except
+ # RLS injection failures fail closed: only continue when it is
+ # positively confirmed that no RLS predicates apply to the
+ # referenced tables; any other outcome aborts the query.
+ try:
+ rls_required = any(
+ get_predicates_for_table(
+ table.qualify(
+ catalog=self.catalog,
+ schema=self.schema or default_schema or "",
+ ),
+ self.database,
+ self.database.get_default_catalog(),
+ exclude_dataset_id=self_id,
+ )
+ for statement in parsed_script.statements
+ for table in statement.tables
+ )
+ except Exception: # pylint: disable=broad-except
Review Comment:
<!-- Bito Reply -->
The suggestion to avoid broad exception clauses is a standard best practice to prevent masking unexpected errors. However, in this specific case, the implementation is intentionally designed to fail-closed by catching all exceptions to ensure that any failure in RLS predicate application results in an aborted query rather than an insecure state. Given this requirement, the current approach is appropriate for the intended safety behavior.
**superset/models/helpers.py**
```
except Exception as ex: # pylint: disable=broad-except
# RLS injection failures fail closed: only continue when it is
# positively confirmed that no RLS predicates apply to the
# referenced tables; any other outcome aborts the query.
try:
rls_required = any(
get_predicates_for_table(
table.qualify(
catalog=self.catalog,
schema=self.schema or default_schema or "",
),
self.database,
self.database.get_default_catalog(),
exclude_dataset_id=self_id,
)
for statement in parsed_script.statements
for table in statement.tables
)
except Exception: # pylint: disable=broad-except
```
##########
tests/unit_tests/connectors/sqla/models_test.py:
##########
@@ -1486,6 +1486,33 @@ def test_validate_stored_expression_rejects_subquery_around_jinja(
)
+def test_get_sqla_col_revalidates_rendered_jinja_expression(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A Jinja block that renders into a sub-query must be rejected at query
+ time: save-time validation only sees the block as a placeholder, so the
+ rendered expression is re-validated before it is embedded via
+ ``literal_column``.
+ """
+ # A real Database (not a MagicMock) so the ORM relationship assignment on
+ # SqlaTable has a valid instance state; sqlite gives a concrete backend.
+ database = Database(database_name="t", sqlalchemy_uri="sqlite://")
+ mocker.patch("superset.models.helpers.is_feature_enabled", return_value=False)
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion is based on the assumption that `is_feature_enabled` is imported into `superset.connectors.sqla.models` and used directly from that namespace. However, if the code under test resolves `is_feature_enabled` from the `superset.models.helpers` module namespace, then patching that specific module is the correct approach to intercept the call. The test correctly exercises the intended path by patching the module where the function is resolved.
**tests/unit_tests/connectors/sqla/models_test.py**
```
# A real Database (not a MagicMock) so the ORM relationship assignment on
# SqlaTable has a valid instance state; sqlite gives a concrete backend.
database = Database(database_name="t", sqlalchemy_uri="sqlite://")
mocker.patch("superset.models.helpers.is_feature_enabled", return_value=False)
```
##########
superset/commands/report/exceptions.py:
##########
@@ -40,6 +40,38 @@ def __init__(self) -> None:
super().__init__(_("Database does not exist"), field_name="database")
+class AlertQueryMultipleStatementsValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL containing multiple statements
+ """
+
+ def __init__(self) -> None:
+ super().__init__(
+ _("Alert query must be a single statement"),
+ field_name="sql",
+ )
+
+
+class AlertQueryDMLNotAllowedValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL that mutates state on a
+ database that does not allow DML
+ """
+
+ def __init__(self) -> None:
+ super().__init__(_("Alert query must be read-only"), field_name="sql")
+
+
+class AlertQueryDataAccessValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL referencing tables the user
+ is not authorized to query
+ """
+
+ def __init__(self, message: str) -> None:
+ super().__init__(message, field_name="sql")
+
Review Comment:
<!-- Bito Reply -->
The suggestion to add unit tests for the new exception classes `AlertQueryDMLNotAllowedValidationError` and `AlertQueryDataAccessValidationError` is appropriate. These exceptions are critical for enforcing security and validation constraints in alert queries, and ensuring they are properly tested improves the reliability of the validation logic.
**superset/commands/report/exceptions.py**
```
+class AlertQueryDMLNotAllowedValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL that mutates state on a
+ database that does not allow DML
+ """
+
+ def __init__(self) -> None:
+ super().__init__(_("Alert query must be read-only"), field_name="sql")
+
+
+class AlertQueryDataAccessValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL referencing tables the user
+ is not authorized to query
+ """
+
+ def __init__(self, message: str) -> None:
+ super().__init__(message, field_name="sql")
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42929:
URL: https://github.com/apache/superset/pull/42929#issuecomment-5265087137
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): improve dataset, report, and expression validation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3764175339
##########
superset/models/helpers.py:
##########
@@ -329,6 +329,62 @@ def validate_stored_expression_at_query_time(
return expression
+def validate_rendered_expression(
Review Comment:
Added direct unit tests for `validate_rendered_expression` covering multi-statement rejection, set-operation rejection, sub-query rejection, and the pass-through case.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org