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/21 16:11:15 UTC
[PR] fix(sqllab): re-validate access against rendered SQL and tighten cache/permalink scoping [superset]
rusackas opened a new pull request, #43394:
URL: https://github.com/apache/superset/pull/43394
### SUMMARY
- SQL Lab query execution now re-authorizes against the literal rendered SQL immediately before running it, instead of authorizing one render and executing a second, independent render — closing the gap where a nondeterministic Jinja template could authorize against one table and execute against another.
- The SQL executor's result cache is now scoped by effective user identity for connections where that identity varies per user (impersonation/OAuth2), instead of being shared across all callers of the same database connection.
- The legacy `kv:`-prefixed SQL Lab permalink resolution path (a raw sequential-ID lookup with no ownership check) has been removed; only the salted-hashid permalink store remains.
- Jinja's `escaped_val`/`url_param` now render escaped list/dict values through a wrapper that doesn't reintroduce Python's own `repr()`-added quote delimiters around already-escaped elements.
### TESTING INSTRUCTIONS
`pytest tests/unit_tests/jinja_context_test.py tests/unit_tests/sql/execution/test_executor.py tests/unit_tests/commands/sql_lab/` — new/extended tests per change, each verified to fail pre-fix and pass post-fix.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing 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(sqllab): re-validate access against rendered SQL and tighten cache/permalink scoping [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43394:
URL: https://github.com/apache/superset/pull/43394#issuecomment-5372444325
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | c9a8188896b822dbfedd701fdd6ddc86094725e7 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a8878a20929b8000897512f |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43394--superset-docs-preview.netlify.app](https://deploy-preview-43394--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcvBDYMwDADAXfyu1TQS4HSDjuE4piAFJw19tELsTn73uQNqK1v9whPgBrGxydI9r7_7_smZIza1pA2LiSJbQmFZFHcpdbV3P0lrLv9X6mtkoonYu-BDJOcchWl4-BnOCw) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43394--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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(sqllab): re-validate access against rendered SQL and tighten cache/permalink scoping [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43394:
URL: https://github.com/apache/superset/pull/43394#issuecomment-5374836406
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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(sqllab): re-validate access against rendered SQL and tighten cache/permalink scoping [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43394:
URL: https://github.com/apache/superset/pull/43394#issuecomment-5372353767
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(sqllab): re-validate access against rendered SQL and tighten cache/permalink scoping [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43394:
URL: https://github.com/apache/superset/pull/43394#discussion_r3832184274
##########
superset/jinja_context.py:
##########
@@ -118,6 +118,48 @@ class TimeFilter:
time_range: str | None
+class SQLSafeList(list[Any]): # noqa: FURB189
+ """
+ A list of dialect-escaped values whose *whole-container* string
+ rendering cannot re-introduce raw quote characters.
+
+ Rendering a plain Python list in a Jinja template goes through
+ ``str()``/``repr()``, which wraps every string element in fresh quote
+ delimiters (and switches to double-quote delimiters when the element
+ contains a single quote, emitting that single quote raw). Either way
+ the rendered text can contain quote characters that were never
+ escaped for SQL, so a template interpolating the list inside its own
+ quotes -- e.g. ``LIKE '{{ filter.get('escaped_val') }}'`` -- could be
+ broken out of even though every string leaf was individually escaped.
+ This subclass renders as its (already-escaped) elements joined with
+ ``", "``, with no additional delimiters, so every quote in the output
+ is one the dialect's literal processor already escaped.
+ """
+
+ def __str__(self) -> str:
+ return ", ".join(str(element) for element in self)
+
+ __repr__ = __str__
+
+
+class SQLSafeDict(dict[Any, Any]): # noqa: FURB189
+ """
+ A dict of dialect-escaped values whose *whole-container* string
+ rendering cannot re-introduce raw quote characters. Mirrors
+ :class:`SQLSafeList` for the mapping case.
+
+ Keys are rendered as-is: they are used for member lookups (for
+ example ``{{ get_guest_user_attribute('tenant').id }}``), not
+ interpolated into SQL, and are never passed through
+ ``ExtraCache._escape_value``.
+ """
+
+ def __str__(self) -> str:
Review Comment:
Good catch, keys are now escaped the same way values are in 4dce236. Whole-dict rendering now carries the same guarantee as SQLSafeList already did for values.
##########
superset/commands/sql_lab/execute.py:
##########
@@ -151,6 +151,15 @@ def _run_sql_json_exec_from_scratch(self) -> SqlJsonExecutionStatus:
self._validate_access(query, self._execution_context.template_params)
self._execution_context.set_query(query)
rendered_query = self._sql_query_render.render(self._execution_context)
+ # The check above authorizes a render of query.sql + template_params
+ # performed before rendering, so that macros with side effects are
+ # gated before they run. self._sql_query_render.render() above is an
+ # independent second render of the same source; for a
+ # nondeterministic template (e.g. one using Jinja's `random` filter
+ # to pick a table) the two renders can diverge, letting a query
+ # read a table the first check never saw. Re-validate the literal
+ # rendered text that is about to execute.
+ self._validate_rendered_access(query, rendered_query)
Review Comment:
This one's out of scope: SQL_QUERY_MUTATOR is an operator-configured hook (set in superset_config.py), not something an unprivileged caller can influence. If an operator writes a mutator that rewrites table references, that's their own code running with their own trust, same as any other custom mutator behavior. The re-validation here targets the actual gap: Jinja template rendering itself diverging between the pre-check and the executed 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(sqllab): re-validate access against rendered SQL and tighten cache/permalink scoping [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43394:
URL: https://github.com/apache/superset/pull/43394#issuecomment-5372413284
<!-- Bito Reply -->
The flagged issue is valid. The `SQLSafeDict` implementation in `superset/jinja_context.py` currently emits keys directly without escaping, which could allow an attacker to inject SQL delimiters if a key contains malicious characters. To resolve this, you should escape the keys in the `__str__` method of `SQLSafeDict` using the same dialect-aware escaping mechanism used for values.
Here is a concise fix for `superset/jinja_context.py`:
```python
def __str__(self) -> str:
# Assuming self.dialect is accessible or available in context
return ", ".join(f"{self._escape_key(key)}: {value}" for key, value in self.items())
```
(Note: You will need to ensure `_escape_key` is defined to handle the dialect-specific escaping for keys.)
Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well?
**superset/jinja_context.py**
```
def __str__(self) -> str:
return ", ".join(f"{self._escape_key(key)}: {value}" for key, value in self.items())
```
--
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(sqllab): re-validate access against rendered SQL and tighten cache/permalink scoping [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #43394:
URL: https://github.com/apache/superset/pull/43394
--
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(sqllab): re-validate access against rendered SQL and tighten cache/permalink scoping [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 #43394:
URL: https://github.com/apache/superset/pull/43394#discussion_r3831853671
##########
superset/jinja_context.py:
##########
@@ -118,6 +118,48 @@ class TimeFilter:
time_range: str | None
+class SQLSafeList(list[Any]): # noqa: FURB189
+ """
+ A list of dialect-escaped values whose *whole-container* string
+ rendering cannot re-introduce raw quote characters.
+
+ Rendering a plain Python list in a Jinja template goes through
+ ``str()``/``repr()``, which wraps every string element in fresh quote
+ delimiters (and switches to double-quote delimiters when the element
+ contains a single quote, emitting that single quote raw). Either way
+ the rendered text can contain quote characters that were never
+ escaped for SQL, so a template interpolating the list inside its own
+ quotes -- e.g. ``LIKE '{{ filter.get('escaped_val') }}'`` -- could be
+ broken out of even though every string leaf was individually escaped.
+ This subclass renders as its (already-escaped) elements joined with
+ ``", "``, with no additional delimiters, so every quote in the output
+ is one the dialect's literal processor already escaped.
+ """
+
+ def __str__(self) -> str:
+ return ", ".join(str(element) for element in self)
+
+ __repr__ = __str__
+
+
+class SQLSafeDict(dict[Any, Any]): # noqa: FURB189
+ """
+ A dict of dialect-escaped values whose *whole-container* string
+ rendering cannot re-introduce raw quote characters. Mirrors
+ :class:`SQLSafeList` for the mapping case.
+
+ Keys are rendered as-is: they are used for member lookups (for
+ example ``{{ get_guest_user_attribute('tenant').id }}``), not
+ interpolated into SQL, and are never passed through
+ ``ExtraCache._escape_value``.
+ """
+
+ def __str__(self) -> str:
Review Comment:
**Suggestion:** Dictionary keys are emitted without dialect escaping, even though guest-token attribute keys are externally supplied and the entire dictionary is rendered as SQL text by Jinja. A key containing a quote or SQL expression can therefore reintroduce an injection delimiter and contradict the class's guarantee that whole-container rendering is safe. Either escape keys before rendering or prevent whole-dictionary interpolation rather than emitting them directly. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Guest-token attributes can alter whole-dictionary SQL interpolation.
- ⚠️ Affected templates use `get_guest_user_attribute()` object rendering.
- ❌ Rendered queries may bypass intended tenant filtering.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/jinja_context.py
**Line:** 157:157
**Comment:**
*Security: Dictionary keys are emitted without dialect escaping, even though guest-token attribute keys are externally supplied and the entire dictionary is rendered as SQL text by Jinja. A key containing a quote or SQL expression can therefore reintroduce an injection delimiter and contradict the class's guarantee that whole-container rendering is safe. Either escape keys before rendering or prevent whole-dictionary interpolation rather than emitting them directly.
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%2F43394&comment_hash=30a2763f218e8a86bd44445bb83429c138e087a3aa9334bbd4b153666e56d790&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43394&comment_hash=30a2763f218e8a86bd44445bb83429c138e087a3aa9334bbd4b153666e56d790&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(sqllab): re-validate access against rendered SQL and tighten cache/permalink scoping [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 #43394:
URL: https://github.com/apache/superset/pull/43394#discussion_r3831857239
##########
superset/commands/sql_lab/execute.py:
##########
@@ -151,6 +151,15 @@ def _run_sql_json_exec_from_scratch(self) -> SqlJsonExecutionStatus:
self._validate_access(query, self._execution_context.template_params)
self._execution_context.set_query(query)
rendered_query = self._sql_query_render.render(self._execution_context)
+ # The check above authorizes a render of query.sql + template_params
+ # performed before rendering, so that macros with side effects are
+ # gated before they run. self._sql_query_render.render() above is an
+ # independent second render of the same source; for a
+ # nondeterministic template (e.g. one using Jinja's `random` filter
+ # to pick a table) the two renders can diverge, letting a query
+ # read a table the first check never saw. Re-validate the literal
+ # rendered text that is about to execute.
+ self._validate_rendered_access(query, rendered_query)
Review Comment:
**Suggestion:** The re-validation covers `rendered_query`, but SQL Lab later passes each block through `database.mutate_sql_based_on_config` before execution. Because `SQL_QUERY_MUTATOR` can rewrite the statement, the SQL reaching the database can reference different tables or statements than the text authorized here. Validate the post-mutation SQL immediately before execution, or ensure the exact authorized text is what the executor runs. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Custom SQL mutators can execute unauthorized table references.
- ⚠️ SQL Lab authorization covers pre-mutation SQL only.
- ⚠️ Both synchronous and asynchronous execution paths are affected.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/sql_lab/execute.py
**Line:** 162:162
**Comment:**
*Security: The re-validation covers `rendered_query`, but SQL Lab later passes each block through `database.mutate_sql_based_on_config` before execution. Because `SQL_QUERY_MUTATOR` can rewrite the statement, the SQL reaching the database can reference different tables or statements than the text authorized here. Validate the post-mutation SQL immediately before execution, or ensure the exact authorized text is what the executor runs.
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%2F43394&comment_hash=9592a299bfb984c22bb3e0b04cd3604ee88ace0e3beb2009e2ff2b3ec46f5f9a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43394&comment_hash=9592a299bfb984c22bb3e0b04cd3604ee88ace0e3beb2009e2ff2b3ec46f5f9a&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(sqllab): re-validate access against rendered SQL and tighten cache/permalink scoping [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43394:
URL: https://github.com/apache/superset/pull/43394#issuecomment-5372407770
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43394?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 `42.42424%` with `19 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.33%. Comparing base ([`f7d505e`](https://app.codecov.io/gh/apache/superset/commit/f7d505e1fd0b4d0fbac4fe90637815343c867628?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c9a8188`](https://app.codecov.io/gh/apache/superset/commit/c9a8188896b822dbfedd701fdd6ddc86094725e7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 9 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/43394?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/sql/execution/executor.py](https://app.codecov.io/gh/apache/superset/pull/43394?src=pr&el=tree&filepath=superset%2Fsql%2Fexecution%2Fexecutor.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc3FsL2V4ZWN1dGlvbi9leGVjdXRvci5weQ==) | 11.76% | [15 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43394?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/jinja\_context.py](https://app.codecov.io/gh/apache/superset/pull/43394?src=pr&el=tree&filepath=superset%2Fjinja_context.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvamluamFfY29udGV4dC5weQ==) | 60.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43394?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 (f7d505e) and HEAD (c9a8188). Click for more details.
>
> <details><summary>HEAD has 82 uploads less than BASE</summary>
>
>| Flag | BASE (f7d505e) | HEAD (c9a8188) |
>|------|------|------|
>|python|72|1|
>|presto|12|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43394 +/- ##
==========================================
- Coverage 66.84% 57.33% -9.51%
==========================================
Files 2876 2876
Lines 164066 164245 +179
Branches 37863 37912 +49
==========================================
- Hits 109666 94177 -15489
- Misses 52226 69174 +16948
+ Partials 2174 894 -1280
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43394/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/43394/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/43394/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/43394/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/43394/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.03% <42.42%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/43394/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.03% <42.42%> (-19.26%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43394/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/43394/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/43394?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(sqllab): re-validate access against rendered SQL and tighten cache/permalink scoping [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 #43394:
URL: https://github.com/apache/superset/pull/43394#discussion_r3832187696
##########
superset/commands/sql_lab/execute.py:
##########
@@ -151,6 +151,15 @@ def _run_sql_json_exec_from_scratch(self) -> SqlJsonExecutionStatus:
self._validate_access(query, self._execution_context.template_params)
self._execution_context.set_query(query)
rendered_query = self._sql_query_render.render(self._execution_context)
+ # The check above authorizes a render of query.sql + template_params
+ # performed before rendering, so that macros with side effects are
+ # gated before they run. self._sql_query_render.render() above is an
+ # independent second render of the same source; for a
+ # nondeterministic template (e.g. one using Jinja's `random` filter
+ # to pick a table) the two renders can diverge, letting a query
+ # read a table the first check never saw. Re-validate the literal
+ # rendered text that is about to execute.
+ self._validate_rendered_access(query, rendered_query)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag SQL_QUERY_MUTATOR rewrites as authorization gaps; treat operator-configured mutators as trusted code and focus validation findings on untrusted template-rendering behavior.
**Applied to:**
- `superset/commands/sql_lab/execute.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