You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sha174n (via GitHub)" <gi...@apache.org> on 2026/09/08 12:41:00 UTC
[PR] fix(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
sha174n opened a new pull request, #43998:
URL: https://github.com/apache/superset/pull/43998
The MCP `execute_sql` tool validated access to the database connection but did not apply the dataset/table-level access validation that the SQL Lab execution path performs (`raise_for_access(..., force_dataset_match=True)`). As a result the two execution surfaces could authorize the tables a query references differently.
This adds the same table-access validation to `execute_sql`, called before the query runs, so a query that references tables the user is not granted is rejected consistently with SQL Lab. It uses the canonical `security_manager.raise_for_access` entry point (passing `database`/`sql`/`catalog`/`schema`/`template_params`), returns a `TABLE_SECURITY_ACCESS_ERROR` response on denial, and leaves the destructive-DDL check and execution steps unchanged.
Adds a unit test asserting a query referencing an unauthorized table is denied and never executed.
--
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(mcp): align execute_sql table-access checks with the SQL Lab path [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 #43998:
URL: https://github.com/apache/superset/pull/43998#discussion_r3958031878
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -171,6 +175,29 @@ async def execute_sql(request: ExecuteSqlRequest, ctx: Context) -> ExecuteSqlRes
error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR.value,
)
+ # 1b. Enforce dataset/table-level access, matching the SQL Lab
+ # execution path (which calls raise_for_access(..., force_dataset_match=
+ # True)). Access to the database connection alone does not authorize
+ # every table on it: the query must resolve to datasets the user is
+ # granted, so the tables it references are validated here before it runs.
+ with event_logger.log_context(action="mcp.execute_sql.table_access_validation"):
+ try:
+ security_manager.raise_for_access(
+ database=database,
+ sql=request.sql,
+ catalog=request.catalog,
+ schema=request.schema_name,
+ template_params=request.template_params,
+ force_dataset_match=True,
+ )
Review Comment:
**Suggestion:** `raise_for_access` returns immediately for users with database access, so this call never reaches the strict table loop and unauthorized tables can still execute. [security]
**Assessment:** ๐ด `Critical` ยท ๐ `Occurrence: Often`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fc4b23ec5e2a4fb38df58dd6794c21c3&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=fc4b23ec5e2a4fb38df58dd6794c21c3&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/mcp_service/sql_lab/tool/execute_sql.py
**Line:** 185:192
**Comment:**
*Security: `raise_for_access` returns immediately for users with database access, so this call never reaches the strict table loop and unauthorized tables can still execute.
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%2F43998&comment_hash=97b8f8571e7b3253b62fcb7f025ed06c5d59fe9750cff0201da588d9cc2f2a27&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43998&comment_hash=97b8f8571e7b3253b62fcb7f025ed06c5d59fe9750cff0201da588d9cc2f2a27&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(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43998:
URL: https://github.com/apache/superset/pull/43998#issuecomment-5594045800
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 an incremental AI Review.
- `/review full` - 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(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43998:
URL: https://github.com/apache/superset/pull/43998#discussion_r3960607698
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -171,6 +175,29 @@ async def execute_sql(request: ExecuteSqlRequest, ctx: Context) -> ExecuteSqlRes
error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR.value,
)
+ # 1b. Enforce dataset/table-level access, matching the SQL Lab
+ # execution path (which calls raise_for_access(..., force_dataset_match=
+ # True)). Access to the database connection alone does not authorize
+ # every table on it: the query must resolve to datasets the user is
+ # granted, so the tables it references are validated here before it runs.
+ with event_logger.log_context(action="mcp.execute_sql.table_access_validation"):
+ try:
+ security_manager.raise_for_access(
+ database=database,
+ sql=request.sql,
+ catalog=request.catalog,
+ schema=request.schema_name,
+ template_params=request.template_params,
+ force_dataset_match=True,
+ )
Review Comment:
<!-- Bito Reply -->
The suggestion is correct. The previous implementation was indeed short-circuited by the database-level access check, rendering the table-level validation ineffective. Removing the gate and relying on `raise_for_access` with `force_dataset_match=True` correctly ensures that table-level access is enforced for the specific tables resolved by the query.
**superset/mcp_service/sql_lab/tool/execute_sql.py**
```
with event_logger.log_context(action="mcp.execute_sql.table_access_validation"):
try:
security_manager.raise_for_access(
database=database,
sql=request.sql,
catalog=request.catalog,
schema=request.schema_name,
template_params=request.template_params,
force_dataset_match=True,
)
```
--
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(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43998:
URL: https://github.com/apache/superset/pull/43998#issuecomment-5585361076
<!-- Bito Reply -->
The security concern is valid. In the current implementation, `raise_for_access` with `force_dataset_match=True` is intended to validate table-level access, but if the security manager configuration or the specific `raise_for_access` implementation returns early for users with broad database-level access, the subsequent table-level checks may be bypassed.
To resolve this, ensure that `raise_for_access` is configured to strictly enforce table-level validation regardless of database-level permissions, or explicitly perform the table-level check separately if `raise_for_access` is insufficient for this requirement. Given the current implementation, you should verify if `force_dataset_match=True` is correctly triggering the expected validation logic in your security manager.
**superset/mcp_service/sql_lab/tool/execute_sql.py**
```
with event_logger.log_context(action="mcp.execute_sql.table_access_validation"):
try:
security_manager.raise_for_access(
database=database,
sql=request.sql,
catalog=request.catalog,
schema=request.schema_name,
template_params=request.template_params,
force_dataset_match=True,
)
```
--
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(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43998:
URL: https://github.com/apache/superset/pull/43998#discussion_r3958269625
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -171,6 +175,29 @@ async def execute_sql(request: ExecuteSqlRequest, ctx: Context) -> ExecuteSqlRes
error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR.value,
)
+ # 1b. Enforce dataset/table-level access, matching the SQL Lab
+ # execution path (which calls raise_for_access(..., force_dataset_match=
+ # True)). Access to the database connection alone does not authorize
+ # every table on it: the query must resolve to datasets the user is
+ # granted, so the tables it references are validated here before it runs.
+ with event_logger.log_context(action="mcp.execute_sql.table_access_validation"):
+ try:
+ security_manager.raise_for_access(
+ database=database,
+ sql=request.sql,
+ catalog=request.catalog,
+ schema=request.schema_name,
+ template_params=request.template_params,
+ force_dataset_match=True,
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-863: Table Access Check No-Op</b></div>
<div id="fix">
This check never runs: `raise_for_access(database=..., sql=...)` builds an ephemeral `Query`, enters the `if database and table or query:` branch, and returns early at manager.py:4529 because `can_access_database(database)` is already True (confirmed at line 168). So `force_dataset_match=True` is never enforced and any table on the database is reachable. Match the SQL Lab path (`sqllab/validators.py` passes `query=`) or add a check that bypasses the database-access short-circuit. ([CWE-863](https://cwe.mitre.org/data/definitions/863.html))
</div>
</div>
<small><i>Code Review Run #4069da</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(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43998:
URL: https://github.com/apache/superset/pull/43998#issuecomment-5585432235
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43998?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 `14.28571%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.54%. Comparing base ([`c4e145b`](https://app.codecov.io/gh/apache/superset/commit/c4e145b071f8ec708ca16485852de3dd8bec9117?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b4f4097`](https://app.codecov.io/gh/apache/superset/commit/b4f40974b412aba9f1b7748575e988c4e3edeca4?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/43998?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/mcp\_service/sql\_lab/tool/execute\_sql.py](https://app.codecov.io/gh/apache/superset/pull/43998?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fsql_lab%2Ftool%2Fexecute_sql.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2Uvc3FsX2xhYi90b29sL2V4ZWN1dGVfc3FsLnB5) | 14.28% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43998?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 (c4e145b) and HEAD (b4f4097). Click for more details.
>
> <details><summary>HEAD has 33 uploads less than BASE</summary>
>
>| Flag | BASE (c4e145b) | HEAD (b4f4097) |
>|------|------|------|
>|python|26|2|
>|presto|5|1|
>|hive|6|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43998 +/- ##
===========================================
- Coverage 79.81% 57.54% -22.27%
===========================================
Files 2914 2914
Lines 170473 170479 +6
Branches 39518 39518
===========================================
- Hits 136066 98106 -37960
- Misses 31885 71451 +39566
+ Partials 2522 922 -1600
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43998/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/43998/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.63% <14.28%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/43998/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/43998/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/43998/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.51% <14.28%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/43998/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.56% <14.28%> (-44.64%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43998/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/43998/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/43998?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(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n merged PR #43998:
URL: https://github.com/apache/superset/pull/43998
--
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(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43998:
URL: https://github.com/apache/superset/pull/43998#discussion_r3959156049
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -171,6 +175,29 @@ async def execute_sql(request: ExecuteSqlRequest, ctx: Context) -> ExecuteSqlRes
error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR.value,
)
+ # 1b. Enforce dataset/table-level access, matching the SQL Lab
+ # execution path (which calls raise_for_access(..., force_dataset_match=
+ # True)). Access to the database connection alone does not authorize
+ # every table on it: the query must resolve to datasets the user is
+ # granted, so the tables it references are validated here before it runs.
+ with event_logger.log_context(action="mcp.execute_sql.table_access_validation"):
+ try:
+ security_manager.raise_for_access(
+ database=database,
+ sql=request.sql,
+ catalog=request.catalog,
+ schema=request.schema_name,
+ template_params=request.template_params,
+ force_dataset_match=True,
+ )
Review Comment:
<!-- Bito Reply -->
The observation that the database-level gate short-circuits the check is correct. Since the current implementation relies on `raise_for_access` which returns early when database access is already granted, the `force_dataset_match=True` parameter is indeed not enforced for individual tables. Keeping the thread open is appropriate while the approach is being reworked to ensure table-level validation is correctly applied.
**superset/mcp_service/sql_lab/tool/execute_sql.py**
```
security_manager.raise_for_access(
database=database,
sql=request.sql,
catalog=request.catalog,
schema=request.schema_name,
template_params=request.template_params,
force_dataset_match=True,
)
```
--
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(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #43998:
URL: https://github.com/apache/superset/pull/43998#discussion_r3960605673
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -171,6 +175,29 @@ async def execute_sql(request: ExecuteSqlRequest, ctx: Context) -> ExecuteSqlRes
error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR.value,
)
+ # 1b. Enforce dataset/table-level access, matching the SQL Lab
+ # execution path (which calls raise_for_access(..., force_dataset_match=
+ # True)). Access to the database connection alone does not authorize
+ # every table on it: the query must resolve to datasets the user is
+ # granted, so the tables it references are validated here before it runs.
+ with event_logger.log_context(action="mcp.execute_sql.table_access_validation"):
+ try:
+ security_manager.raise_for_access(
+ database=database,
+ sql=request.sql,
+ catalog=request.catalog,
+ schema=request.schema_name,
+ template_params=request.template_params,
+ force_dataset_match=True,
+ )
Review Comment:
Correct, the call was unreachable behind the preceding `can_access_database` gate. Fixed in 23087bcc88: that gate is removed and the whole access decision now goes through `raise_for_access(..., force_dataset_match=True)`, matching `superset/sqllab/validators.py`.
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -171,6 +175,29 @@ async def execute_sql(request: ExecuteSqlRequest, ctx: Context) -> ExecuteSqlRes
error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR.value,
)
+ # 1b. Enforce dataset/table-level access, matching the SQL Lab
+ # execution path (which calls raise_for_access(..., force_dataset_match=
+ # True)). Access to the database connection alone does not authorize
+ # every table on it: the query must resolve to datasets the user is
+ # granted, so the tables it references are validated here before it runs.
+ with event_logger.log_context(action="mcp.execute_sql.table_access_validation"):
+ try:
+ security_manager.raise_for_access(
+ database=database,
+ sql=request.sql,
+ catalog=request.catalog,
+ schema=request.schema_name,
+ template_params=request.template_params,
+ force_dataset_match=True,
+ )
Review Comment:
Correct, the call was unreachable behind the preceding `can_access_database` gate. Fixed in 23087bcc88: that gate is removed and the whole access decision now goes through `raise_for_access(..., force_dataset_match=True)`, matching `superset/sqllab/validators.py`.
--
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(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43998:
URL: https://github.com/apache/superset/pull/43998#issuecomment-5585288759
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(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43998:
URL: https://github.com/apache/superset/pull/43998#issuecomment-5596014620
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 an incremental AI Review.
- `/review full` - 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(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #43998:
URL: https://github.com/apache/superset/pull/43998#discussion_r3959154123
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -171,6 +175,29 @@ async def execute_sql(request: ExecuteSqlRequest, ctx: Context) -> ExecuteSqlRes
error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR.value,
)
+ # 1b. Enforce dataset/table-level access, matching the SQL Lab
+ # execution path (which calls raise_for_access(..., force_dataset_match=
+ # True)). Access to the database connection alone does not authorize
+ # every table on it: the query must resolve to datasets the user is
+ # granted, so the tables it references are validated here before it runs.
+ with event_logger.log_context(action="mcp.execute_sql.table_access_validation"):
+ try:
+ security_manager.raise_for_access(
+ database=database,
+ sql=request.sql,
+ catalog=request.catalog,
+ schema=request.schema_name,
+ template_params=request.template_params,
+ force_dataset_match=True,
+ )
Review Comment:
Confirmed. With the database-level gate ahead of it, `raise_for_access` returns at its `can_access_database` shortcut and never reaches the table loop, so this call is a no-op as written.
Dropping that gate is not a correct substitute either, since the table loop only ranges over tables the statement actually resolves to. Reworking the approach, so leaving this thread open rather than resolving it.
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -171,6 +175,29 @@ async def execute_sql(request: ExecuteSqlRequest, ctx: Context) -> ExecuteSqlRes
error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR.value,
)
+ # 1b. Enforce dataset/table-level access, matching the SQL Lab
+ # execution path (which calls raise_for_access(..., force_dataset_match=
+ # True)). Access to the database connection alone does not authorize
+ # every table on it: the query must resolve to datasets the user is
+ # granted, so the tables it references are validated here before it runs.
+ with event_logger.log_context(action="mcp.execute_sql.table_access_validation"):
+ try:
+ security_manager.raise_for_access(
+ database=database,
+ sql=request.sql,
+ catalog=request.catalog,
+ schema=request.schema_name,
+ template_params=request.template_params,
+ force_dataset_match=True,
+ )
Review Comment:
Confirmed. With the database-level gate ahead of it, `raise_for_access` returns at its `can_access_database` shortcut and never reaches the table loop, so this call is a no-op as written.
Dropping that gate is not a correct substitute either, since the table loop only ranges over tables the statement actually resolves to. Reworking the approach, so leaving this thread open rather than resolving 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(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43998:
URL: https://github.com/apache/superset/pull/43998#issuecomment-5587042670
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | 8428e17e73a4e7e7197072faf7019df916cf53b7 |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa01fd80e0a9b0008050783 |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-43998--superset-docs-preview.netlify.app](https://deploy-preview-43998--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#DcpLDkAwEADQu8xaY0RQbuAY0-k0JD5DKyHi7rp7i_eCnvuqCQaAAtxJG0_ZYb7LldXILXwlMfFYjKdEUZIhZokxby-67M_o82-JsAreoiD1DhEtNtjZGr4f) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43998--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(mcp): align execute_sql table-access checks with the SQL Lab path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43998:
URL: https://github.com/apache/superset/pull/43998#issuecomment-5610586966
**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