You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "gabotorresruiz (via GitHub)" <gi...@apache.org> on 2026/08/11 17:30:35 UTC
[PR] fix(mcp): harden the embedded-guest chart data-read path [superset]
gabotorresruiz opened a new pull request, #43066:
URL: https://github.com/apache/superset/pull/43066
### SUMMARY
Follow-up to the embedded-guest MCP data-read work (#41753). The max-effort review of that PR fixed the high-severity findings in-line and deferred a batch of lower-severity correctness, robustness, and coverage gaps; this PR clears that batch.
Two things up front, because these are intentionally small:
- **None is a security bypass.** Guest chart data stays gated by `raise_for_access` on every path. These are about *not returning a raw 500/opaque error* and about internal consistency, not about what a guest can access.
- **None changes behavior for non-guest principals.** Every change is either guest-only or a no-op for existing callers.
Grouped by area:
**Guest dashboard scoping (`utils/filters.py`, `dashboards/filters.py`)**
- A dashboard **slug** is a valid guest-token resource id (`Dashboard.get` accepts it), but it was routed to `Dashboard.id.in_([slug])`, which raises a cast error/500 on strict backends. It now routes to `Dashboard.slug`.
- The dashboard-list guest filter had its own copy of the scoping logic that used an either/or (uuid *or* int), so a token mixing uuid and int ids silently dropped the int-id dashboards β disagreeing with the chart-list filter. Both now use the shared `guest_embedded_dashboard_filter`, so visibility is consistent and the duplicate branch is gone.
**`get_chart_data` / `get_chart_preview` robustness**
- `row_limit` can arrive as a string (cached form_data). The no-context path already coerced it; the `using_unsaved_state` and `_query_from_form_data` paths did not, and a string could 500 in `apply_max_row_limit`. Now coerced on all paths.
- The `form_data_key`-only path had no guest handling: the unsaved-chart cache is not scoped to a guest's dashboards, so a guest failed opaquely downstream. Guests are now denied cleanly there.
- The dataset pre-check was skipped entirely for guests, so a *deleted* dataset surfaced a raw error instead of the clean `DatasetNotAccessible` contract. It now runs for guests with `check_access=False` β existence check kept (clean error), RBAC access check skipped (guests are authorized via the dashboard context, not dataset RBAC).
**MCP config / cleanup (`mcp_config.py`, `auth.py`)**
- `get_mcp_config` did not forward `MCP_RESTRICTED_TOOL_POLICY`, so a policy set at the `mcp_config` layer was dropped in standalone MCP mode. Now forwarded.
- The 7-tool guest allow-list was duplicated as two literals kept in sync by a comment (and a test). `auth.py` now derives it from the single `mcp_config` source; the now-redundant sync test is removed.
**Tests**
- Added tool-level guest tests for `get_chart_data` (the highest-value guest tool): the data query is pinned to the token's dashboard, the dataset check runs existence-only for guests, an out-of-scope chart is denied, and the `form_data_key`-only path is denied.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend/MCP correctness).
### TESTING INSTRUCTIONS
- `pytest tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py tests/unit_tests/mcp_service/test_guest_token_auth.py tests/unit_tests/mcp_service/test_guest_scope.py`
- `pytest tests/unit_tests/utils/filters_test.py tests/unit_tests/dashboards/`
- All green locally; `ruff` and `ruff format` clean.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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(mcp): harden the embedded-guest chart data-read path [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43066:
URL: https://github.com/apache/superset/pull/43066#issuecomment-5256620327
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43066?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 `19.23077%` with `21 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.21%. Comparing base ([`47adbe3`](https://app.codecov.io/gh/apache/superset/commit/47adbe31d5c3007adc5bd9b02c6634a8c13a2a31?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`cc2c4af`](https://app.codecov.io/gh/apache/superset/commit/cc2c4af99a9d79ed737f1690bb6b031ba5e8fa07?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 14 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/43066?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/chart/tool/get\_chart\_data.py](https://app.codecov.io/gh/apache/superset/pull/43066?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_data.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC9nZXRfY2hhcnRfZGF0YS5weQ==) | 0.00% | [11 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43066?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/filters.py](https://app.codecov.io/gh/apache/superset/pull/43066?src=pr&el=tree&filepath=superset%2Futils%2Ffilters.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvZmlsdGVycy5weQ==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43066?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/dashboards/filters.py](https://app.codecov.io/gh/apache/superset/pull/43066?src=pr&el=tree&filepath=superset%2Fdashboards%2Ffilters.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9maWx0ZXJzLnB5) | 33.33% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43066?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...perset/mcp\_service/chart/tool/get\_chart\_preview.py](https://app.codecov.io/gh/apache/superset/pull/43066?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_preview.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC9nZXRfY2hhcnRfcHJldmlldy5weQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43066?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43066 +/- ##
==========================================
- Coverage 66.55% 57.21% -9.34%
==========================================
Files 2864 2866 +2
Lines 161894 162227 +333
Branches 37305 37346 +41
==========================================
- Hits 107742 92826 -14916
- Misses 52106 68537 +16431
+ Partials 2046 864 -1182
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43066/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/43066/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/43066/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/43066/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/43066/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.13% <19.23%> (+0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/43066/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.13% <19.23%> (-18.99%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43066/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/43066/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/43066?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): harden the embedded-guest chart data-read path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3768912286
##########
superset/dashboards/filters.py:
##########
@@ -270,7 +236,6 @@ def _apply_legacy(self, query: Query) -> Query:
or_(
Dashboard.id.in_(editor_ids_query),
Dashboard.id.in_(datasource_perm_query),
- *feature_flagged_filters,
*extra_access_filters,
)
)
Review Comment:
<!-- Bito Reply -->
The removal of the dead method is appropriate. Since the conditional logic that previously invoked this method has been removed, the method is no longer reachable and its deletion correctly reduces code complexity and maintenance overhead.
**superset/dashboards/filters.py**
```
or_(
Dashboard.id.in_(editor_ids_query),
Dashboard.id.in_(datasource_perm_query),
- *feature_flagged_filters,
*extra_access_filters,
)
)
```
--
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): harden the embedded-guest chart data-read path [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3768910024
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -1742,3 +1743,214 @@ def test_non_integer_float_total_rows_is_truncated(self) -> None:
chart_data = _make_chart_data(total_rows=5.9)
assert chart_data.total_rows == 5
assert isinstance(chart_data.total_rows, int)
+
+
+class TestGuestScoping:
+ """Tool-level guest coverage for get_chart_data (the highest-value guest
+ tool): the data query is pinned to the token's dashboard, the dataset
+ existence check runs without the RBAC access check, and requests that a
+ guest cannot be scoped for are denied cleanly."""
+
+ @pytest.mark.asyncio
+ async def test_guest_query_pinned_to_dashboard_with_existence_only_check(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ chart = SimpleNamespace(
+ id=9,
+ slice_name="Sales",
+ viz_type="table",
+ datasource_id=1,
+ datasource_type="table",
+ query_context='{"queries": []}',
+ params=None,
+ )
+ validate_calls: dict[str, Any] = {}
+
+ def fake_validate(datasource_id: Any, check_access: bool = True) -> Any:
+ validate_calls["check_access"] = check_access
+ return SimpleNamespace(is_valid=True, warnings=[], error=None)
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {
+ "queries": [{"data": [{"a": 1}], "colnames": ["a"], "rowcount": 1}]
+ }
+
+ mock_authorize = MagicMock()
+ with (
+ patch.object(module, "find_chart_by_identifier", return_value=chart),
+ patch.object(module, "validate_chart_dataset", side_effect=fake_validate),
+ patch.object(module.guest_scope, "is_guest_read", return_value=True),
+ patch.object(module.guest_scope, "guest_dashboard_id", return_value=6),
+ patch.object(module.guest_scope, "authorize_query", mock_authorize),
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand",
+ _Command,
+ ),
+ patch(
+ "superset.charts.schemas.ChartDataQueryContextSchema.load",
+ lambda self, data: object(),
+ ),
+ ):
+ async with Client(mcp_server) as client:
+ await client.call_tool(
+ "get_chart_data", {"request": {"identifier": "9"}}
+ )
+
+ # F: the dataset existence check runs, but without the RBAC access check.
+ assert validate_calls["check_access"] is False
+ # Scoping: the data query is pinned to the token's dashboard, so
+ # raise_for_access authorizes against a dashboard the guest can see.
+ mock_authorize.assert_called_once()
+ assert mock_authorize.call_args.args[1] == 6
+
+ @pytest.mark.asyncio
+ async def test_guest_out_of_scope_chart_is_not_found(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ from superset.utils import json
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ # ChartFilter scopes an out-of-scope chart out, so the lookup returns None.
+ with (
+ patch.object(module, "find_chart_by_identifier", return_value=None),
+ patch.object(module.guest_scope, "is_guest_read", return_value=True),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_data", {"request": {"identifier": "123"}}
+ )
+
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "NotFound"
+
+ @pytest.mark.asyncio
+ async def test_guest_form_data_key_only_path_is_denied(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ from superset.utils import json
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ # A valid cached blob is available, so without the guest-denial branch
+ # the code would proceed to query rather than 404 on a cache miss. The
+ # NotFound here therefore pins the denial itself, not a cache miss.
+ with (
+ patch.object(module.guest_scope, "is_guest_read", return_value=True),
+ patch.object(
+ module,
+ "get_cached_form_data",
+ return_value='{"datasource_id": 1, "datasource_type": "table"}',
+ ),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_data", {"request": {"form_data_key": "cached-key"}}
+ )
+
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "NotFound"
+ assert "No accessible chart found for this request." in data["error"]
+
+
+@pytest.mark.asyncio
+async def test_query_from_form_data_zero_row_limit_falls_back_to_default(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """A falsy 0 row_limit resolves to ROW_LIMIT (0 means "no limit"), so the
+ row_limit coercion does not change behavior for a cached row_limit of 0."""
Review Comment:
Fixed the docstring. It now says a falsy int `0` hits the `or ROW_LIMIT` fallback and resolves to the configured default before coercion runs, which is what the assertion checks.
##########
superset/dashboards/filters.py:
##########
@@ -192,24 +193,13 @@ def _apply_viewers(self, query: Query) -> Query:
if user_id:
filters.append(Dashboard.id.in_(extra_dashboards_filter(user_id)))
- # (D) Embedded: preserved as-is
+ # Reuse the shared guest scoping so the dashboard list and chart list
+ # agree on visibility.
if is_feature_enabled("EMBEDDED_SUPERSET") and security_manager.is_guest_user(
g.user
):
- guest_user: GuestUser = g.user
- embedded_dashboard_ids = [
- r["id"]
- for r in guest_user.resources
- if r["type"] == GuestTokenResourceType.DASHBOARD.value
- ]
- condition = (
- Dashboard.embedded.any(
- EmbeddedDashboard.uuid.in_(embedded_dashboard_ids)
- )
- if any(is_uuid(id_) for id_ in embedded_dashboard_ids)
- else Dashboard.id.in_(embedded_dashboard_ids)
- )
- filters.append(condition)
+ if (guest_condition := guest_embedded_dashboard_filter()) is not None:
+ filters.append(guest_condition)
Review Comment:
Agreed. Reworked it so a guest returns the token scoping as the sole predicate via an early return in `apply()`, mirroring `ChartFilter`, instead of OR-ing it with the role paths. Also dropped the dead `_apply_legacy`, which carried the same pattern.
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -994,8 +1004,11 @@ async def _query_from_form_data(
error_type="InvalidFormData",
)
- row_limit = (
- request.limit or form_data.get("row_limit") or current_app.config["ROW_LIMIT"]
+ # row_limit may arrive as a str; coerce it. Keep the trailing fallback so a
+ # falsy 0 still resolves to ROW_LIMIT (as before).
+ row_limit = _coerce_row_limit(
+ request.limit or form_data.get("row_limit") or current_app.config["ROW_LIMIT"],
+ current_app.config["ROW_LIMIT"],
)
Review Comment:
Good catch. `_coerce_row_limit` now falls back to the default for any non-positive value, and I guarded the saved-context path that sets `row_limit` directly, so a negative can't reach `LIMIT -1` downstream. Added `-1` and `"-1"` cases to the coercion test.
##########
superset/dashboards/filters.py:
##########
@@ -270,7 +236,6 @@ def _apply_legacy(self, query: Query) -> Query:
or_(
Dashboard.id.in_(editor_ids_query),
Dashboard.id.in_(datasource_perm_query),
- *feature_flagged_filters,
*extra_access_filters,
)
)
Review Comment:
Right, it was dead. Nothing has called `_apply_legacy` since `apply()` started routing everything through `_apply_viewers`. Removed the method and trimmed the now-stale docstring section.
##########
superset/utils/filters.py:
##########
@@ -87,4 +92,6 @@ def guest_embedded_dashboard_filter() -> Optional[ColumnElement[bool]]:
# branch and has_guest_access); a guest is only ever scoped to embedded
# dashboards, never a plain internal id.
conditions.append(and_(Dashboard.id.in_(int_ids), Dashboard.embedded.any()))
+ if slug_ids:
+ conditions.append(and_(Dashboard.slug.in_(slug_ids), Dashboard.embedded.any()))
Review Comment:
Good catch, that was a half-state I introduced. `has_guest_access` only authorizes by dashboard id and embedded uuid, never slug, so a slug dashboard would list but its data would be denied. Removed the slug branch so the list filter matches the data path (slugs stay fail-closed).
--
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): harden the embedded-guest chart data-read path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43066:
URL: https://github.com/apache/superset/pull/43066#issuecomment-5297617938
**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(mcp): harden the embedded-guest chart data-read path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3762586477
##########
superset/dashboards/filters.py:
##########
@@ -270,7 +236,6 @@ def _apply_legacy(self, query: Query) -> Query:
or_(
Dashboard.id.in_(editor_ids_query),
Dashboard.id.in_(datasource_perm_query),
- *feature_flagged_filters,
*extra_access_filters,
)
)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead method post-refactor</b></div>
<div id="fix">
The `_apply_legacy` method is dead code. The conditional `is_feature_enabled("ENABLE_VIEWERS")` that was its only caller was removed by this diff. The method body now only accumulates complexity and maintenance burden with zero runtime benefit.
</div>
</div>
<small><i>Code Review Run #a1a7e2</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): harden the embedded-guest chart data-read path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43066:
URL: https://github.com/apache/superset/pull/43066#issuecomment-5287105502
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(mcp): harden the embedded-guest chart data-read path [superset]
Posted by "rebenitez1802 (via GitHub)" <gi...@apache.org>.
rebenitez1802 commented on code in PR #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3761530724
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1293,13 +1293,13 @@ def __init__(self, form_data: Dict[str, Any]):
logger.info("Generating preview for chart %s", getattr(chart, "id", "NO_ID"))
logger.info("Chart datasource_id: %s", getattr(chart, "datasource_id", "NONE"))
- # Skip the dataset pre-check for transient charts (no ID) and for guests
- # (authorized via the dashboard context, not dataset RBAC).
+ # Skip the pre-check only for transient charts (no ID). Guests keep the
+ # existence check but skip the RBAC access check (dashboard-authorized).
from superset.mcp_service import guest_scope
- if getattr(chart, "id", None) is not None and not guest_scope.is_guest_read():
+ if getattr(chart, "id", None) is not None:
validation_result = validate_chart_dataset(
- chart.datasource_id, check_access=True
+ chart.datasource_id, check_access=not guest_scope.is_guest_read()
Review Comment:
π΄ **Blocker β same root cause as `get_chart_data`**
`validate_chart_dataset(check_access=False)` still resolves the dataset via `DatasetDAO.find_by_id` β `DatasourceFilter`, which denies a guest β `DatasetNotAccessible` on every guest preview. Preferred fix is the `skip_base_filter=True` existence lookup in `chart_utils.py` (see the `get_chart_data` comment); the minimal in-diff fix restores the guest skip:
```suggestion
if getattr(chart, "id", None) is not None and not guest_scope.is_guest_read():
validation_result = validate_chart_dataset(
chart.datasource_id, check_access=True
```
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -1742,3 +1743,173 @@ def test_non_integer_float_total_rows_is_truncated(self) -> None:
chart_data = _make_chart_data(total_rows=5.9)
assert chart_data.total_rows == 5
assert isinstance(chart_data.total_rows, int)
+
+
+class TestGuestScoping:
+ """Tool-level guest coverage for get_chart_data (the highest-value guest
+ tool): the data query is pinned to the token's dashboard, the dataset
+ existence check runs without the RBAC access check, and requests that a
+ guest cannot be scoped for are denied cleanly."""
+
+ @pytest.mark.asyncio
+ async def test_guest_query_pinned_to_dashboard_with_existence_only_check(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ chart = SimpleNamespace(
+ id=9,
+ slice_name="Sales",
+ viz_type="table",
+ datasource_id=1,
+ datasource_type="table",
+ query_context='{"queries": []}',
+ params=None,
+ )
+ validate_calls: dict[str, Any] = {}
+
+ def fake_validate(datasource_id: Any, check_access: bool = True) -> Any:
+ validate_calls["check_access"] = check_access
+ return SimpleNamespace(is_valid=True, warnings=[], error=None)
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {
+ "queries": [{"data": [{"a": 1}], "colnames": ["a"], "rowcount": 1}]
+ }
+
+ mock_authorize = MagicMock()
+ with (
+ patch.object(module, "find_chart_by_identifier", return_value=chart),
+ patch.object(module, "validate_chart_dataset", side_effect=fake_validate),
Review Comment:
π‘ **Medium β this mock hides the blocker above**
Patching `validate_chart_dataset` to return `is_valid=True` means the real `DatasetDAO.find_by_id` β `DatasourceFilter` path is never exercised, so the guest-denial regression sails through CI. The rest of this test is sound (`check_access is False`, `authorize_query` pinned to dashboard `6`). Add a test that drives the *real* `validate_chart_dataset` for a guest against a live dataset and asserts the data query succeeds β that would catch the blocker.
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -1742,3 +1743,173 @@ def test_non_integer_float_total_rows_is_truncated(self) -> None:
chart_data = _make_chart_data(total_rows=5.9)
assert chart_data.total_rows == 5
assert isinstance(chart_data.total_rows, int)
+
+
+class TestGuestScoping:
+ """Tool-level guest coverage for get_chart_data (the highest-value guest
+ tool): the data query is pinned to the token's dashboard, the dataset
+ existence check runs without the RBAC access check, and requests that a
+ guest cannot be scoped for are denied cleanly."""
+
+ @pytest.mark.asyncio
+ async def test_guest_query_pinned_to_dashboard_with_existence_only_check(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ chart = SimpleNamespace(
+ id=9,
+ slice_name="Sales",
+ viz_type="table",
+ datasource_id=1,
+ datasource_type="table",
+ query_context='{"queries": []}',
+ params=None,
+ )
+ validate_calls: dict[str, Any] = {}
+
+ def fake_validate(datasource_id: Any, check_access: bool = True) -> Any:
+ validate_calls["check_access"] = check_access
+ return SimpleNamespace(is_valid=True, warnings=[], error=None)
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {
+ "queries": [{"data": [{"a": 1}], "colnames": ["a"], "rowcount": 1}]
+ }
+
+ mock_authorize = MagicMock()
+ with (
+ patch.object(module, "find_chart_by_identifier", return_value=chart),
+ patch.object(module, "validate_chart_dataset", side_effect=fake_validate),
+ patch.object(module.guest_scope, "is_guest_read", return_value=True),
+ patch.object(module.guest_scope, "guest_dashboard_id", return_value=6),
+ patch.object(module.guest_scope, "authorize_query", mock_authorize),
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand",
+ _Command,
+ ),
+ patch(
+ "superset.charts.schemas.ChartDataQueryContextSchema.load",
+ lambda self, data: object(),
+ ),
+ ):
+ async with Client(mcp_server) as client:
+ await client.call_tool(
+ "get_chart_data", {"request": {"identifier": "9"}}
+ )
+
+ # F: the dataset existence check runs, but without the RBAC access check.
+ assert validate_calls["check_access"] is False
+ # Scoping: the data query is pinned to the token's dashboard, so
+ # raise_for_access authorizes against a dashboard the guest can see.
+ mock_authorize.assert_called_once()
+ assert mock_authorize.call_args.args[1] == 6
+
+ @pytest.mark.asyncio
+ async def test_guest_out_of_scope_chart_is_not_found(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ from superset.utils import json
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ # ChartFilter scopes an out-of-scope chart out, so the lookup returns None.
+ with (
+ patch.object(module, "find_chart_by_identifier", return_value=None),
+ patch.object(module.guest_scope, "is_guest_read", return_value=True),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_data", {"request": {"identifier": "123"}}
+ )
+
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "NotFound"
+
+ @pytest.mark.asyncio
+ async def test_guest_form_data_key_only_path_is_denied(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ from superset.utils import json
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ # A valid cached blob is available, so without the guest-denial branch
+ # the code would proceed to query rather than 404 on a cache miss. The
+ # NotFound here therefore pins the denial itself, not a cache miss.
+ with (
+ patch.object(module.guest_scope, "is_guest_read", return_value=True),
+ patch.object(
+ module,
+ "get_cached_form_data",
+ return_value='{"datasource_id": 1, "datasource_type": "table"}',
+ ),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_data", {"request": {"form_data_key": "cached-key"}}
+ )
+
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "NotFound"
+ assert "No accessible chart found for this request." in data["error"]
+
+
+@pytest.mark.asyncio
+async def test_query_from_form_data_zero_row_limit_falls_back_to_default(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """A falsy 0 row_limit resolves to ROW_LIMIT (0 means "no limit"), so the
+ row_limit coercion does not change behavior for a cached row_limit of 0."""
+ from flask import current_app
+
+ module = importlib.import_module("superset.mcp_service.chart.tool.get_chart_data")
+ captured: dict[str, Any] = {}
+
+ def fake_build(form_data: Any, **kwargs: Any) -> Any:
+ captured["row_limit"] = kwargs.get("row_limit")
+ return object()
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {"queries": [{"data": [], "colnames": [], "rowcount": 0}]}
+
+ monkeypatch.setattr(module, "build_query_context_from_form_data", fake_build)
+ monkeypatch.setattr(
+ module,
+ "event_logger",
+ SimpleNamespace(log_context=lambda **kwargs: nullcontext()),
+ )
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ monkeypatch.setattr(get_data_command_module, "ChartDataCommand", _Command)
+
+ await _query_from_form_data(
+ {"datasource_id": 1, "datasource_type": "table", "row_limit": 0},
+ GetChartDataRequest(form_data_key="k"),
+ _AsyncContext(),
+ )
+
+ assert captured["row_limit"] == current_app.config["ROW_LIMIT"]
+ assert captured["row_limit"] != 0
Review Comment:
π‘ **Medium β this test doesn't exercise the coercion it's named for**
`row_limit=0` is a falsy int, so it's resolved by the pre-existing `... or ROW_LIMIT` chain *before* `_coerce_row_limit` coerces anything β deleting the `_coerce_row_limit` call would still pass this test. It also hides a divergence: a cached **string** `"0"` is truthy, bypasses the `or ROW_LIMIT` fallback, and yields `row_limit=0` ("no limit"), while int `0` falls back to `ROW_LIMIT`. So *"a falsy 0 still resolves to ROW_LIMIT (as before)"* holds only for int `0`. The `using_unsaved_state` coercion site (line 526) also has no test.
Add a test that actually exercises strβint coercion:
```python
@pytest.mark.asyncio
async def test_query_from_form_data_string_row_limit_is_coerced(
monkeypatch: pytest.MonkeyPatch,
) -> None:
module = importlib.import_module("superset.mcp_service.chart.tool.get_chart_data")
captured: dict[str, Any] = {}
def fake_build(form_data: Any, **kwargs: Any) -> Any:
captured["row_limit"] = kwargs.get("row_limit")
return object()
class _Command:
def __init__(self, query_context: Any) -> None: ...
def validate(self) -> None: ...
def run(self) -> dict[str, Any]:
return {"queries": [{"data": [], "colnames": [], "rowcount": 0}]}
monkeypatch.setattr(module, "build_query_context_from_form_data", fake_build)
monkeypatch.setattr(
module, "event_logger",
SimpleNamespace(log_context=lambda **kwargs: nullcontext()),
)
monkeypatch.setattr(
importlib.import_module("superset.commands.chart.data.get_data_command"),
"ChartDataCommand", _Command,
)
await _query_from_form_data(
{"datasource_id": 1, "datasource_type": "table", "row_limit": "250"},
GetChartDataRequest(form_data_key="k"),
_AsyncContext(),
)
assert captured["row_limit"] == 250
assert isinstance(captured["row_limit"], int)
```
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -433,30 +440,30 @@ async def get_chart_data( # noqa: C901
)
logger.info("Getting data for chart %s: %s", chart.id, chart.slice_name)
- # Skip the dataset RBAC pre-check for guests (see guest_scope.is_guest_read).
- if not guest_scope.is_guest_read():
- validation_result = validate_chart_dataset(
- chart.datasource_id, check_access=True
+ # Validate the dataset for everyone. Guests skip the RBAC access check
+ # (governed by authorize_query below) but keep the existence check, so a
+ # deleted dataset still returns the clean DatasetNotAccessible contract.
+ validation_result = validate_chart_dataset(
+ chart.datasource_id, check_access=not guest_scope.is_guest_read()
+ )
+ if not validation_result.is_valid:
+ await ctx.warning(
+ "Chart found but dataset is not accessible: %s"
+ % (validation_result.error,)
)
- if not validation_result.is_valid:
- await ctx.warning(
- "Chart found but dataset is not accessible: %s"
- % (validation_result.error,)
- )
- logger.warning(
- "get_chart_data: dataset not accessible for chart_id=%s: %s",
- chart.id,
- validation_result.error,
- )
- return ChartError(
- error=validation_result.error
- or "Chart's dataset is not accessible. "
- "Dataset may have been deleted.",
- error_type="DatasetNotAccessible",
- )
- # Log any warnings (e.g., virtual dataset warnings)
- for warning in validation_result.warnings:
- await ctx.warning("Dataset warning: %s" % (warning,))
+ logger.warning(
+ "get_chart_data: dataset not accessible for chart_id=%s: %s",
+ chart.id,
+ validation_result.error,
+ )
+ return ChartError(
+ error=validation_result.error
+ or "Chart's dataset is not accessible. Dataset may have been deleted.",
+ error_type="DatasetNotAccessible",
+ )
+ # Log any warnings (e.g., virtual dataset warnings)
+ for warning in validation_result.warnings:
+ await ctx.warning("Dataset warning: %s" % (warning,))
Review Comment:
π΄ **Blocker β this pre-check denies every guest, defeating the change**
For a guest this is not existence-only. `validate_chart_dataset` (`chart_utils.py`) calls `DatasetDAO.find_by_id(datasource_id)` with the default `skip_base_filter=False`; the `check_access` flag only gates the *separate* `has_dataset_access` call, not the lookup. `DatasetDAO.base_filter = DatasourceFilter`, which returns rows only under `can_access_all_datasources()` (False for a guest) or a matching DB/datasource/schema grant β an embedded guest has none. So `find_by_id` returns `None` β `is_valid=False` β `DatasetNotAccessible` for every in-scope, live chart. The sibling `get_chart_info` still skips this for guests on purpose (`get_chart_info.py:113-115`: *"Guests read via the dashboard context, not dataset RBAC; skip the perm-check"*). CI stays green only because the new test mocks `validate_chart_dataset` (see the test comment).
**Preferred fix (keeps the existence-check intent):** thread the flag through `validate_chart_dataset` so `check_access=False` resolves the dataset with `DatasetDAO.find_by_id(datasource_id, skip_base_filter=True)` β existence without RBAC. That file is not in this diff, so it can't be a one-click suggestion.
**Minimal in-diff fix** (matches `get_chart_info`; drops the guest existence check):
```suggestion
# Guests read via the dashboard context (authorize_query +
# raise_for_access below), not dataset RBAC, and validate_chart_dataset
# resolves the dataset through the DatasourceFilter base filter, which
# denies a guest outright -- so running it here returns
# DatasetNotAccessible for every guest. Skip it, as get_chart_info does.
if not guest_scope.is_guest_read():
validation_result = validate_chart_dataset(
chart.datasource_id, check_access=True
)
if not validation_result.is_valid:
await ctx.warning(
"Chart found but dataset is not accessible: %s"
% (validation_result.error,)
)
logger.warning(
"get_chart_data: dataset not accessible for chart_id=%s: %s",
chart.id,
validation_result.error,
)
return ChartError(
error=validation_result.error
or "Chart's dataset is not accessible. Dataset may have been deleted.",
error_type="DatasetNotAccessible",
)
# Log any warnings (e.g., virtual dataset warnings)
for warning in validation_result.warnings:
await ctx.warning("Dataset warning: %s" % (warning,))
```
##########
superset/utils/filters.py:
##########
@@ -87,4 +92,6 @@ def guest_embedded_dashboard_filter() -> Optional[ColumnElement[bool]]:
# branch and has_guest_access); a guest is only ever scoped to embedded
# dashboards, never a plain internal id.
conditions.append(and_(Dashboard.id.in_(int_ids), Dashboard.embedded.any()))
+ if slug_ids:
+ conditions.append(and_(Dashboard.slug.in_(slug_ids), Dashboard.embedded.any()))
Review Comment:
π‘ **Medium β the `slug_ids` branch ships with no test**
`tests/unit_tests/utils/filters_test.py` covers disabled / non-guest / no-resources / uuid / int / mixed, but has no slug case, so this new branch (and any wrong-column or cast regression in it) is uncovered. Add a `_guest_with_dashboards("my-slug")` case asserting the compiled SQL contains `dashboards.slug IN (...)` plus the `embedded` `EXISTS`, and a uuid+int+slug mixed case. Separately, the `DashboardAccessFilter` guest wiring in `dashboards/filters.py` is only covered via the extracted helper β `subjects/test_filters.py` mocks `_apply_viewers` out and sets `is_guest_user=False` β so a dropped `append` there wouldn't be caught either.
--
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): harden the embedded-guest chart data-read 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 #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3768443814
##########
superset/utils/filters.py:
##########
@@ -87,4 +92,6 @@ def guest_embedded_dashboard_filter() -> Optional[ColumnElement[bool]]:
# branch and has_guest_access); a guest is only ever scoped to embedded
# dashboards, never a plain internal id.
conditions.append(and_(Dashboard.id.in_(int_ids), Dashboard.embedded.any()))
+ if slug_ids:
+ conditions.append(and_(Dashboard.slug.in_(slug_ids), Dashboard.embedded.any()))
Review Comment:
**Suggestion:** The slug predicate can resolve a dashboard for a guest, but the downstream `security_manager.has_guest_access()` contract only compares token resources with `Dashboard.id` and the embedded dashboard UUID, not `Dashboard.slug`. Consequently, slug-based charts pass the SQL lookup but `guest_dashboard_id()` returns `None`, so the MCP data path does not attach the dashboard context and the request is denied later instead of successfully returning the authorized chart data. Resolve slugs to dashboard IDs before applying this filter or extend the guest-access check to recognize slugs. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Slug-based guest MCP chart reads fail authorization.
- β οΈ UUID- and numeric-resource tokens remain unaffected.
- β οΈ Guest users receive an access error instead of chart data.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ed791d23c0e2428d8da04720e76d282d&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=ed791d23c0e2428d8da04720e76d282d&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/filters.py
**Line:** 95:96
**Comment:**
*Api Mismatch: The slug predicate can resolve a dashboard for a guest, but the downstream `security_manager.has_guest_access()` contract only compares token resources with `Dashboard.id` and the embedded dashboard UUID, not `Dashboard.slug`. Consequently, slug-based charts pass the SQL lookup but `guest_dashboard_id()` returns `None`, so the MCP data path does not attach the dashboard context and the request is denied later instead of successfully returning the authorized chart data. Resolve slugs to dashboard IDs before applying this filter or extend the guest-access check to recognize slugs.
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%2F43066&comment_hash=a8fc76d36ffe5c808c1527eeff8e6ecf97c23ae8e28daa21acf1f62b909715c0&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43066&comment_hash=a8fc76d36ffe5c808c1527eeff8e6ecf97c23ae8e28daa21acf1f62b909715c0&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): harden the embedded-guest chart data-read path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43066:
URL: https://github.com/apache/superset/pull/43066#issuecomment-5274182181
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(mcp): harden the embedded-guest chart data-read path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43066:
URL: https://github.com/apache/superset/pull/43066#issuecomment-5272024599
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(mcp): harden the embedded-guest chart data-read 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 #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3761649167
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -1742,3 +1743,214 @@ def test_non_integer_float_total_rows_is_truncated(self) -> None:
chart_data = _make_chart_data(total_rows=5.9)
assert chart_data.total_rows == 5
assert isinstance(chart_data.total_rows, int)
+
+
+class TestGuestScoping:
+ """Tool-level guest coverage for get_chart_data (the highest-value guest
+ tool): the data query is pinned to the token's dashboard, the dataset
+ existence check runs without the RBAC access check, and requests that a
+ guest cannot be scoped for are denied cleanly."""
+
+ @pytest.mark.asyncio
+ async def test_guest_query_pinned_to_dashboard_with_existence_only_check(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ chart = SimpleNamespace(
+ id=9,
+ slice_name="Sales",
+ viz_type="table",
+ datasource_id=1,
+ datasource_type="table",
+ query_context='{"queries": []}',
+ params=None,
+ )
+ validate_calls: dict[str, Any] = {}
+
+ def fake_validate(datasource_id: Any, check_access: bool = True) -> Any:
+ validate_calls["check_access"] = check_access
+ return SimpleNamespace(is_valid=True, warnings=[], error=None)
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {
+ "queries": [{"data": [{"a": 1}], "colnames": ["a"], "rowcount": 1}]
+ }
+
+ mock_authorize = MagicMock()
+ with (
+ patch.object(module, "find_chart_by_identifier", return_value=chart),
+ patch.object(module, "validate_chart_dataset", side_effect=fake_validate),
+ patch.object(module.guest_scope, "is_guest_read", return_value=True),
+ patch.object(module.guest_scope, "guest_dashboard_id", return_value=6),
+ patch.object(module.guest_scope, "authorize_query", mock_authorize),
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand",
+ _Command,
+ ),
+ patch(
+ "superset.charts.schemas.ChartDataQueryContextSchema.load",
+ lambda self, data: object(),
+ ),
+ ):
+ async with Client(mcp_server) as client:
+ await client.call_tool(
+ "get_chart_data", {"request": {"identifier": "9"}}
+ )
+
+ # F: the dataset existence check runs, but without the RBAC access check.
+ assert validate_calls["check_access"] is False
+ # Scoping: the data query is pinned to the token's dashboard, so
+ # raise_for_access authorizes against a dashboard the guest can see.
+ mock_authorize.assert_called_once()
+ assert mock_authorize.call_args.args[1] == 6
+
+ @pytest.mark.asyncio
+ async def test_guest_out_of_scope_chart_is_not_found(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ from superset.utils import json
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ # ChartFilter scopes an out-of-scope chart out, so the lookup returns None.
+ with (
+ patch.object(module, "find_chart_by_identifier", return_value=None),
+ patch.object(module.guest_scope, "is_guest_read", return_value=True),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_data", {"request": {"identifier": "123"}}
+ )
+
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "NotFound"
+
+ @pytest.mark.asyncio
+ async def test_guest_form_data_key_only_path_is_denied(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ from superset.utils import json
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ # A valid cached blob is available, so without the guest-denial branch
+ # the code would proceed to query rather than 404 on a cache miss. The
+ # NotFound here therefore pins the denial itself, not a cache miss.
+ with (
+ patch.object(module.guest_scope, "is_guest_read", return_value=True),
+ patch.object(
+ module,
+ "get_cached_form_data",
+ return_value='{"datasource_id": 1, "datasource_type": "table"}',
+ ),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_data", {"request": {"form_data_key": "cached-key"}}
+ )
+
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "NotFound"
+ assert "No accessible chart found for this request." in data["error"]
+
+
+@pytest.mark.asyncio
+async def test_query_from_form_data_zero_row_limit_falls_back_to_default(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """A falsy 0 row_limit resolves to ROW_LIMIT (0 means "no limit"), so the
+ row_limit coercion does not change behavior for a cached row_limit of 0."""
Review Comment:
**Suggestion:** The test documentation contradicts the behavior it asserts: a cached `row_limit` of `0` is described as meaning βno limit,β but the assertion requires it to resolve to `ROW_LIMIT` instead. Update the docstring to state that zero falls back to the configured default, or change the expected behavior if zero is intended to mean unlimited. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor π§Ή</summary>
```mdx
- β οΈ Test documentation contradicts its verified assertions.
- β οΈ Misleads maintainers about zero row-limit semantics.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=48595ace0d644830a22295ae9471392f&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=48595ace0d644830a22295ae9471392f&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:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
**Line:** 1880:1881
**Comment:**
*Comment Mismatch: The test documentation contradicts the behavior it asserts: a cached `row_limit` of `0` is described as meaning βno limit,β but the assertion requires it to resolve to `ROW_LIMIT` instead. Update the docstring to state that zero falls back to the configured default, or change the expected behavior if zero is intended to mean unlimited.
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%2F43066&comment_hash=4cc8d2453fbd5b82d40cecf64156487e3f81a4496b4425db4751c80c763c468b&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43066&comment_hash=4cc8d2453fbd5b82d40cecf64156487e3f81a4496b4425db4751c80c763c468b&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): harden the embedded-guest chart data-read 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 #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3761671390
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -994,8 +1004,11 @@ async def _query_from_form_data(
error_type="InvalidFormData",
)
- row_limit = (
- request.limit or form_data.get("row_limit") or current_app.config["ROW_LIMIT"]
+ # row_limit may arrive as a str; coerce it. Keep the trailing fallback so a
+ # falsy 0 still resolves to ROW_LIMIT (as before).
+ row_limit = _coerce_row_limit(
+ request.limit or form_data.get("row_limit") or current_app.config["ROW_LIMIT"],
+ current_app.config["ROW_LIMIT"],
)
Review Comment:
**Suggestion:** `_coerce_row_limit` accepts negative numeric values, so cached form data or a request containing a value such as `"-1"` is converted successfully and passed downstream. `apply_max_row_limit` preserves that negative value, which can produce an invalid database limit or backend-specific unlimited-result behavior instead of a bounded query. Reject negative values or fall back to the configured default during coercion. [type error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ MCP chart queries can fail on negative limits.
- β οΈ Backend behavior may vary for negative SQL limits.
- β Unsaved chart data retrieval can return an internal error.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d9095397703e49d1aea45e810497155a&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=d9095397703e49d1aea45e810497155a&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/chart/tool/get_chart_data.py
**Line:** 1009:1012
**Comment:**
*Type Error: `_coerce_row_limit` accepts negative numeric values, so cached form data or a request containing a value such as `"-1"` is converted successfully and passed downstream. `apply_max_row_limit` preserves that negative value, which can produce an invalid database limit or backend-specific unlimited-result behavior instead of a bounded query. Reject negative values or fall back to the configured default during coercion.
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%2F43066&comment_hash=558bafa9ea2c216e4b5ba5ddf11dc66807d932dff65cc21d603e2f1fc2eb3f49&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43066&comment_hash=558bafa9ea2c216e4b5ba5ddf11dc66807d932dff65cc21d603e2f1fc2eb3f49&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): harden the embedded-guest chart data-read path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43066:
URL: https://github.com/apache/superset/pull/43066#issuecomment-5284332532
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(mcp): harden the embedded-guest chart data-read path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43066:
URL: https://github.com/apache/superset/pull/43066#issuecomment-5256256832
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): harden the embedded-guest chart data-read 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 #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3768464165
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1293,13 +1293,13 @@ def __init__(self, form_data: Dict[str, Any]):
logger.info("Generating preview for chart %s", getattr(chart, "id", "NO_ID"))
logger.info("Chart datasource_id: %s", getattr(chart, "datasource_id", "NONE"))
- # Skip the dataset pre-check for transient charts (no ID) and for guests
- # (authorized via the dashboard context, not dataset RBAC).
+ # Skip the pre-check only for transient charts (no ID). Guests keep the
+ # existence check but skip the RBAC access check (dashboard-authorized).
from superset.mcp_service import guest_scope
- if getattr(chart, "id", None) is not None and not guest_scope.is_guest_read():
+ if getattr(chart, "id", None) is not None:
validation_result = validate_chart_dataset(
- chart.datasource_id, check_access=True
+ chart.datasource_id, check_access=not guest_scope.is_guest_read()
)
Review Comment:
**Suggestion:** Transient charts created from a `form_data_key` use `id = 0`, not `None`. This condition therefore runs `validate_chart_dataset` for every unsaved preview, passing its datasource ID (which is `None` for the no-identifier transient path), causing validation to return an invalid-dataset error before the preview strategy can run. Check for a real persisted ID instead of only checking for `None`. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Unsaved identifier-based previews return DatasetNotAccessible errors.
- β οΈ Preview strategies never run for transient charts.
- β οΈ MCP callers cannot preview cached unsaved chart configurations.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b814fa3372784506b52194d3d69061d4&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=b814fa3372784506b52194d3d69061d4&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/chart/tool/get_chart_preview.py
**Line:** 1300:1303
**Comment:**
*Logic Error: Transient charts created from a `form_data_key` use `id = 0`, not `None`. This condition therefore runs `validate_chart_dataset` for every unsaved preview, passing its datasource ID (which is `None` for the no-identifier transient path), causing validation to return an invalid-dataset error before the preview strategy can run. Check for a real persisted ID instead of only checking for `None`.
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%2F43066&comment_hash=f7fce0ade536fb3f3edf69f77b019843846bfb6ae5630e3024261e16ae4f30ab&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43066&comment_hash=f7fce0ade536fb3f3edf69f77b019843846bfb6ae5630e3024261e16ae4f30ab&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): harden the embedded-guest chart data-read 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 #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3768469718
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -433,30 +440,30 @@ async def get_chart_data( # noqa: C901
)
logger.info("Getting data for chart %s: %s", chart.id, chart.slice_name)
- # Skip the dataset RBAC pre-check for guests (see guest_scope.is_guest_read).
- if not guest_scope.is_guest_read():
- validation_result = validate_chart_dataset(
- chart.datasource_id, check_access=True
+ # Validate the dataset for everyone. Guests skip the RBAC access check
+ # (governed by authorize_query below) but keep the existence check, so a
+ # deleted dataset still returns the clean DatasetNotAccessible contract.
+ validation_result = validate_chart_dataset(
+ chart.datasource_id, check_access=not guest_scope.is_guest_read()
+ )
Review Comment:
**Suggestion:** For guest requests this validates only the saved chart datasource with `check_access=False`, while a supplied `form_data_key` can replace the query context and its datasource before execution. `guest_scope.authorize_query` overwrites `dashboardId` and `slice_id` but does not pin the datasource to the resolved chart, so a cached form-data payload for another datasource can be executed whenever the guest principal has any matching datasource-level permission. Validate or reject the cached datasource and ensure it matches the resolved chart before bypassing RBAC. [security]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Guest chart reads can execute cached queries against unrelated datasources.
- β οΈ Dataset validation and guest dashboard scoping become inconsistent.
- β Sensitive data may be exposed through an unsaved form-data key.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=29ae3374cd754ae6a935ff247fda0f1d&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=29ae3374cd754ae6a935ff247fda0f1d&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/chart/tool/get_chart_data.py
**Line:** 446:448
**Comment:**
*Security: For guest requests this validates only the saved chart datasource with `check_access=False`, while a supplied `form_data_key` can replace the query context and its datasource before execution. `guest_scope.authorize_query` overwrites `dashboardId` and `slice_id` but does not pin the datasource to the resolved chart, so a cached form-data payload for another datasource can be executed whenever the guest principal has any matching datasource-level permission. Validate or reject the cached datasource and ensure it matches the resolved chart before bypassing RBAC.
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%2F43066&comment_hash=78282a39d31a05bdf43326dc2178c0023835398b5960fda35ca42f46d67fb43c&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43066&comment_hash=78282a39d31a05bdf43326dc2178c0023835398b5960fda35ca42f46d67fb43c&reaction=dislike'>π</a>
##########
superset/utils/filters.py:
##########
@@ -78,7 +78,12 @@ def guest_embedded_dashboard_filter() -> Optional[ColumnElement[bool]]:
# Route each id kind to its own column and OR them β a plain int sent to the
# uuid-typed column would raise a bind/type error.
uuid_ids = [id_ for id_ in ids if is_uuid(id_)]
- int_ids = [id_ for id_ in ids if not is_uuid(id_)]
+ # A non-uuid id is a numeric dashboard id or a slug; route slugs to the slug
+ # column instead of the int id column (which would raise a cast error). The
+ # data path (has_guest_access) stays the authorization gate.
+ non_uuid_ids = [id_ for id_ in ids if not is_uuid(id_)]
+ int_ids = [id_ for id_ in non_uuid_ids if str(id_).isdigit()]
+ slug_ids = [id_ for id_ in non_uuid_ids if not str(id_).isdigit()]
Review Comment:
**Suggestion:** Using `str(id_).isdigit()` does not match the identifier contract used by `Dashboard.get`, whose `is_int` accepts any value convertible by `int`, including whitespace-padded and explicitly signed numeric strings. The token schema accepts these values as strings, and validation therefore accepts a resource such as `+123` or ` 123`, but this code routes it to `Dashboard.slug` instead of `Dashboard.id`, so the guest loses access to the dashboard referenced by its otherwise valid token. Reuse the model's integer classification helper rather than `isdigit()`. [type error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Formatted numeric resource ids fail dashboard scoping.
- β οΈ Guest token validation and filtering classify ids differently.
- β οΈ Embedded chart reads return not-found or access errors.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=078ce764466c4a9ebc005d448964c3a0&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=078ce764466c4a9ebc005d448964c3a0&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/filters.py
**Line:** 84:86
**Comment:**
*Type Error: Using `str(id_).isdigit()` does not match the identifier contract used by `Dashboard.get`, whose `is_int` accepts any value convertible by `int`, including whitespace-padded and explicitly signed numeric strings. The token schema accepts these values as strings, and validation therefore accepts a resource such as `+123` or ` 123`, but this code routes it to `Dashboard.slug` instead of `Dashboard.id`, so the guest loses access to the dashboard referenced by its otherwise valid token. Reuse the model's integer classification helper rather than `isdigit()`.
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%2F43066&comment_hash=62a134938e9bb70e4059967f7c4b7093ec5b32395f55b06452bbd3792f2671a9&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43066&comment_hash=62a134938e9bb70e4059967f7c4b7093ec5b32395f55b06452bbd3792f2671a9&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): harden the embedded-guest chart data-read path [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on PR #43066:
URL: https://github.com/apache/superset/pull/43066#issuecomment-5258753015
Thanks for the thorough pass, genuinely useful. Fixed the blocker at the root in `validate_chart_dataset` (`skip_base_filter=not check_access`) and added tests that actually fail without the fix, for the guest existence path, the row_limit coercion, and the slug branch. Replies inline, all threads addressed.
--
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): harden the embedded-guest chart data-read path [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43066:
URL: https://github.com/apache/superset/pull/43066#issuecomment-5260119329
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(mcp): harden the embedded-guest chart data-read path [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz merged PR #43066:
URL: https://github.com/apache/superset/pull/43066
--
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): harden the embedded-guest chart data-read 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 #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3768915403
##########
superset/utils/filters.py:
##########
@@ -78,7 +78,12 @@ def guest_embedded_dashboard_filter() -> Optional[ColumnElement[bool]]:
# Route each id kind to its own column and OR them β a plain int sent to the
# uuid-typed column would raise a bind/type error.
uuid_ids = [id_ for id_ in ids if is_uuid(id_)]
- int_ids = [id_ for id_ in ids if not is_uuid(id_)]
+ # A non-uuid id is a numeric dashboard id or a slug; route slugs to the slug
+ # column instead of the int id column (which would raise a cast error). The
+ # data path (has_guest_access) stays the authorization gate.
+ non_uuid_ids = [id_ for id_ in ids if not is_uuid(id_)]
+ int_ids = [id_ for id_ in non_uuid_ids if str(id_).isdigit()]
+ slug_ids = [id_ for id_ in non_uuid_ids if not str(id_).isdigit()]
Review Comment:
β
**Customized review instruction saved!**
**Instruction:**
> Keep using canonical decimal-string classification for dashboard IDs in guest access filters; do not replace it with an integer-convertibility helper when authorization compares resource IDs by string equality.
**Applied to:**
- `superset/utils/filters.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(mcp): harden the embedded-guest chart data-read path [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3768911493
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1293,13 +1293,13 @@ def __init__(self, form_data: Dict[str, Any]):
logger.info("Generating preview for chart %s", getattr(chart, "id", "NO_ID"))
logger.info("Chart datasource_id: %s", getattr(chart, "datasource_id", "NONE"))
- # Skip the dataset pre-check for transient charts (no ID) and for guests
- # (authorized via the dashboard context, not dataset RBAC).
+ # Skip the pre-check only for transient charts (no ID). Guests keep the
+ # existence check but skip the RBAC access check (dashboard-authorized).
from superset.mcp_service import guest_scope
- if getattr(chart, "id", None) is not None and not guest_scope.is_guest_read():
+ if getattr(chart, "id", None) is not None:
validation_result = validate_chart_dataset(
- chart.datasource_id, check_access=True
+ chart.datasource_id, check_access=not guest_scope.is_guest_read()
)
Review Comment:
Correct, transient charts use id `0`, not `None`, so the pre-check was running for them. Switched the guard to a truthy check so a transient chart skips it as the comment intends.
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -433,30 +440,30 @@ async def get_chart_data( # noqa: C901
)
logger.info("Getting data for chart %s: %s", chart.id, chart.slice_name)
- # Skip the dataset RBAC pre-check for guests (see guest_scope.is_guest_read).
- if not guest_scope.is_guest_read():
- validation_result = validate_chart_dataset(
- chart.datasource_id, check_access=True
+ # Validate the dataset for everyone. Guests skip the RBAC access check
+ # (governed by authorize_query below) but keep the existence check, so a
+ # deleted dataset still returns the clean DatasetNotAccessible contract.
+ validation_result = validate_chart_dataset(
+ chart.datasource_id, check_access=not guest_scope.is_guest_read()
+ )
Review Comment:
Traced this end to end and it doesn't become a foreign read: `authorize_query` pins `slice_id` to the resolved chart, `query_context_modified` rejects a slice mismatch, and the guest datasource grant requires `slc.datasource == datasource`, so a swapped datasource is denied by `raise_for_access` before `run()`. That said, a guest has no business using the unsaved-state cache, so I gated `form_data_key` off for guests as defense in depth. Added a test that a guest with an identifier plus a foreign `form_data_key` never reads the cache.
##########
superset/utils/filters.py:
##########
@@ -78,7 +78,12 @@ def guest_embedded_dashboard_filter() -> Optional[ColumnElement[bool]]:
# Route each id kind to its own column and OR them β a plain int sent to the
# uuid-typed column would raise a bind/type error.
uuid_ids = [id_ for id_ in ids if is_uuid(id_)]
- int_ids = [id_ for id_ in ids if not is_uuid(id_)]
+ # A non-uuid id is a numeric dashboard id or a slug; route slugs to the slug
+ # column instead of the int id column (which would raise a cast error). The
+ # data path (has_guest_access) stays the authorization gate.
+ non_uuid_ids = [id_ for id_ in ids if not is_uuid(id_)]
+ int_ids = [id_ for id_ in non_uuid_ids if str(id_).isdigit()]
+ slug_ids = [id_ for id_ in non_uuid_ids if not str(id_).isdigit()]
Review Comment:
This one dissolves with the slug removal above. `has_guest_access` compares resources by string equality against `dashboard.id`, so `+123` or ` 123` never match a real id there either. `isdigit()` captures exactly the canonical decimal strings that can string-equal an id, so it mirrors the auth gate. Using `is_int` here would route `+123` into `Dashboard.id.in_(...)`, which Postgres coerces and matches, re-creating the list-yes/data-no mismatch in the other direction. So I kept `isdigit()` on purpose.
--
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): harden the embedded-guest chart data-read path [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43066:
URL: https://github.com/apache/superset/pull/43066#issuecomment-5256276164
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 393151dcc101ddcb1c1dd57a2b6a8450ac687c82 |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a7b54eacc7a1b0007ea3017 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-43066--superset-docs-preview.netlify.app](https://deploy-preview-43066--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#DcpLCoAgFAXQvbxxktFHaAct46rPCizFjIpo7zk7g_NSTGGLmUaiinTCbpZit971ZqKYTz6ysMgQiWGFw5Fd8D5cZVuOPjyTLX-A0n3HMEah0VJKxWhlo-j7AQ) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43066--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): harden the embedded-guest chart data-read path [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3761641996
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -433,30 +440,30 @@ async def get_chart_data( # noqa: C901
)
logger.info("Getting data for chart %s: %s", chart.id, chart.slice_name)
- # Skip the dataset RBAC pre-check for guests (see guest_scope.is_guest_read).
- if not guest_scope.is_guest_read():
- validation_result = validate_chart_dataset(
- chart.datasource_id, check_access=True
+ # Validate the dataset for everyone. Guests skip the RBAC access check
+ # (governed by authorize_query below) but keep the existence check, so a
+ # deleted dataset still returns the clean DatasetNotAccessible contract.
+ validation_result = validate_chart_dataset(
+ chart.datasource_id, check_access=not guest_scope.is_guest_read()
+ )
+ if not validation_result.is_valid:
+ await ctx.warning(
+ "Chart found but dataset is not accessible: %s"
+ % (validation_result.error,)
)
- if not validation_result.is_valid:
- await ctx.warning(
- "Chart found but dataset is not accessible: %s"
- % (validation_result.error,)
- )
- logger.warning(
- "get_chart_data: dataset not accessible for chart_id=%s: %s",
- chart.id,
- validation_result.error,
- )
- return ChartError(
- error=validation_result.error
- or "Chart's dataset is not accessible. "
- "Dataset may have been deleted.",
- error_type="DatasetNotAccessible",
- )
- # Log any warnings (e.g., virtual dataset warnings)
- for warning in validation_result.warnings:
- await ctx.warning("Dataset warning: %s" % (warning,))
+ logger.warning(
+ "get_chart_data: dataset not accessible for chart_id=%s: %s",
+ chart.id,
+ validation_result.error,
+ )
+ return ChartError(
+ error=validation_result.error
+ or "Chart's dataset is not accessible. Dataset may have been deleted.",
+ error_type="DatasetNotAccessible",
+ )
+ # Log any warnings (e.g., virtual dataset warnings)
+ for warning in validation_result.warnings:
+ await ctx.warning("Dataset warning: %s" % (warning,))
Review Comment:
Good catch, you're right. Traced it end to end: `check_access` only gates `has_dataset_access`, while `find_by_id` keeps applying `DatasourceFilter`, so every guest was getting denied. Went with your preferred fix and threaded `skip_base_filter` through `validate_chart_dataset`, so `check_access=False` is now a true existence-only lookup. That keeps the guest existence check instead of dropping it the way `get_chart_info` does.
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1293,13 +1293,13 @@ def __init__(self, form_data: Dict[str, Any]):
logger.info("Generating preview for chart %s", getattr(chart, "id", "NO_ID"))
logger.info("Chart datasource_id: %s", getattr(chart, "datasource_id", "NONE"))
- # Skip the dataset pre-check for transient charts (no ID) and for guests
- # (authorized via the dashboard context, not dataset RBAC).
+ # Skip the pre-check only for transient charts (no ID). Guests keep the
+ # existence check but skip the RBAC access check (dashboard-authorized).
from superset.mcp_service import guest_scope
- if getattr(chart, "id", None) is not None and not guest_scope.is_guest_read():
+ if getattr(chart, "id", None) is not None:
validation_result = validate_chart_dataset(
- chart.datasource_id, check_access=True
+ chart.datasource_id, check_access=not guest_scope.is_guest_read()
Review Comment:
Same call underneath, so the same fix covers it. With `validate_chart_dataset` skipping the base filter when `check_access` is False, the guest preview keeps its existence check without the RBAC denial.
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -1742,3 +1743,173 @@ def test_non_integer_float_total_rows_is_truncated(self) -> None:
chart_data = _make_chart_data(total_rows=5.9)
assert chart_data.total_rows == 5
assert isinstance(chart_data.total_rows, int)
+
+
+class TestGuestScoping:
+ """Tool-level guest coverage for get_chart_data (the highest-value guest
+ tool): the data query is pinned to the token's dashboard, the dataset
+ existence check runs without the RBAC access check, and requests that a
+ guest cannot be scoped for are denied cleanly."""
+
+ @pytest.mark.asyncio
+ async def test_guest_query_pinned_to_dashboard_with_existence_only_check(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ chart = SimpleNamespace(
+ id=9,
+ slice_name="Sales",
+ viz_type="table",
+ datasource_id=1,
+ datasource_type="table",
+ query_context='{"queries": []}',
+ params=None,
+ )
+ validate_calls: dict[str, Any] = {}
+
+ def fake_validate(datasource_id: Any, check_access: bool = True) -> Any:
+ validate_calls["check_access"] = check_access
+ return SimpleNamespace(is_valid=True, warnings=[], error=None)
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {
+ "queries": [{"data": [{"a": 1}], "colnames": ["a"], "rowcount": 1}]
+ }
+
+ mock_authorize = MagicMock()
+ with (
+ patch.object(module, "find_chart_by_identifier", return_value=chart),
+ patch.object(module, "validate_chart_dataset", side_effect=fake_validate),
Review Comment:
Agreed, that mock was hiding the bug. Moved the real coverage down to `test_chart_utils.py`: it drives `validate_chart_dataset` with `check_access=False` and asserts `find_by_id` gets `skip_base_filter=True` and `has_dataset_access` is never called. Verified it fails without the fix.
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -1742,3 +1743,173 @@ def test_non_integer_float_total_rows_is_truncated(self) -> None:
chart_data = _make_chart_data(total_rows=5.9)
assert chart_data.total_rows == 5
assert isinstance(chart_data.total_rows, int)
+
+
+class TestGuestScoping:
+ """Tool-level guest coverage for get_chart_data (the highest-value guest
+ tool): the data query is pinned to the token's dashboard, the dataset
+ existence check runs without the RBAC access check, and requests that a
+ guest cannot be scoped for are denied cleanly."""
+
+ @pytest.mark.asyncio
+ async def test_guest_query_pinned_to_dashboard_with_existence_only_check(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ chart = SimpleNamespace(
+ id=9,
+ slice_name="Sales",
+ viz_type="table",
+ datasource_id=1,
+ datasource_type="table",
+ query_context='{"queries": []}',
+ params=None,
+ )
+ validate_calls: dict[str, Any] = {}
+
+ def fake_validate(datasource_id: Any, check_access: bool = True) -> Any:
+ validate_calls["check_access"] = check_access
+ return SimpleNamespace(is_valid=True, warnings=[], error=None)
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {
+ "queries": [{"data": [{"a": 1}], "colnames": ["a"], "rowcount": 1}]
+ }
+
+ mock_authorize = MagicMock()
+ with (
+ patch.object(module, "find_chart_by_identifier", return_value=chart),
+ patch.object(module, "validate_chart_dataset", side_effect=fake_validate),
+ patch.object(module.guest_scope, "is_guest_read", return_value=True),
+ patch.object(module.guest_scope, "guest_dashboard_id", return_value=6),
+ patch.object(module.guest_scope, "authorize_query", mock_authorize),
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand",
+ _Command,
+ ),
+ patch(
+ "superset.charts.schemas.ChartDataQueryContextSchema.load",
+ lambda self, data: object(),
+ ),
+ ):
+ async with Client(mcp_server) as client:
+ await client.call_tool(
+ "get_chart_data", {"request": {"identifier": "9"}}
+ )
+
+ # F: the dataset existence check runs, but without the RBAC access check.
+ assert validate_calls["check_access"] is False
+ # Scoping: the data query is pinned to the token's dashboard, so
+ # raise_for_access authorizes against a dashboard the guest can see.
+ mock_authorize.assert_called_once()
+ assert mock_authorize.call_args.args[1] == 6
+
+ @pytest.mark.asyncio
+ async def test_guest_out_of_scope_chart_is_not_found(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ from superset.utils import json
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ # ChartFilter scopes an out-of-scope chart out, so the lookup returns None.
+ with (
+ patch.object(module, "find_chart_by_identifier", return_value=None),
+ patch.object(module.guest_scope, "is_guest_read", return_value=True),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_data", {"request": {"identifier": "123"}}
+ )
+
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "NotFound"
+
+ @pytest.mark.asyncio
+ async def test_guest_form_data_key_only_path_is_denied(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ from superset.utils import json
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ # A valid cached blob is available, so without the guest-denial branch
+ # the code would proceed to query rather than 404 on a cache miss. The
+ # NotFound here therefore pins the denial itself, not a cache miss.
+ with (
+ patch.object(module.guest_scope, "is_guest_read", return_value=True),
+ patch.object(
+ module,
+ "get_cached_form_data",
+ return_value='{"datasource_id": 1, "datasource_type": "table"}',
+ ),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_data", {"request": {"form_data_key": "cached-key"}}
+ )
+
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "NotFound"
+ assert "No accessible chart found for this request." in data["error"]
+
+
+@pytest.mark.asyncio
+async def test_query_from_form_data_zero_row_limit_falls_back_to_default(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """A falsy 0 row_limit resolves to ROW_LIMIT (0 means "no limit"), so the
+ row_limit coercion does not change behavior for a cached row_limit of 0."""
+ from flask import current_app
+
+ module = importlib.import_module("superset.mcp_service.chart.tool.get_chart_data")
+ captured: dict[str, Any] = {}
+
+ def fake_build(form_data: Any, **kwargs: Any) -> Any:
+ captured["row_limit"] = kwargs.get("row_limit")
+ return object()
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {"queries": [{"data": [], "colnames": [], "rowcount": 0}]}
+
+ monkeypatch.setattr(module, "build_query_context_from_form_data", fake_build)
+ monkeypatch.setattr(
+ module,
+ "event_logger",
+ SimpleNamespace(log_context=lambda **kwargs: nullcontext()),
+ )
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ monkeypatch.setattr(get_data_command_module, "ChartDataCommand", _Command)
+
+ await _query_from_form_data(
+ {"datasource_id": 1, "datasource_type": "table", "row_limit": 0},
+ GetChartDataRequest(form_data_key="k"),
+ _AsyncContext(),
+ )
+
+ assert captured["row_limit"] == current_app.config["ROW_LIMIT"]
+ assert captured["row_limit"] != 0
Review Comment:
Right, `0` is falsy so it short circuits before the coercion ever runs. Added a test that feeds `row_limit="250"` and checks it comes back as int `250`, which does break if you remove `_coerce_row_limit`. Kept the `0` case as a plain regression for the falsy fallback. Good call on the str `"0"` vs int `0` asymmetry too, noted.
##########
superset/utils/filters.py:
##########
@@ -87,4 +92,6 @@ def guest_embedded_dashboard_filter() -> Optional[ColumnElement[bool]]:
# branch and has_guest_access); a guest is only ever scoped to embedded
# dashboards, never a plain internal id.
conditions.append(and_(Dashboard.id.in_(int_ids), Dashboard.embedded.any()))
+ if slug_ids:
+ conditions.append(and_(Dashboard.slug.in_(slug_ids), Dashboard.embedded.any()))
Review Comment:
Added a slug case plus a uuid+int+slug mixed case in `filters_test.py`, asserting the compiled SQL hits `dashboards.slug` with the embedded `EXISTS`. On the `DashboardAccessFilter` wiring: fair, it's still only exercised through the helper. Left it there since the helper is the shared gate, but happy to add a focused test if you'd rather.
--
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): harden the embedded-guest chart data-read 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 #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3761651481
##########
superset/dashboards/filters.py:
##########
@@ -192,24 +193,13 @@ def _apply_viewers(self, query: Query) -> Query:
if user_id:
filters.append(Dashboard.id.in_(extra_dashboards_filter(user_id)))
- # (D) Embedded: preserved as-is
+ # Reuse the shared guest scoping so the dashboard list and chart list
+ # agree on visibility.
if is_feature_enabled("EMBEDDED_SUPERSET") and security_manager.is_guest_user(
g.user
):
- guest_user: GuestUser = g.user
- embedded_dashboard_ids = [
- r["id"]
- for r in guest_user.resources
- if r["type"] == GuestTokenResourceType.DASHBOARD.value
- ]
- condition = (
- Dashboard.embedded.any(
- EmbeddedDashboard.uuid.in_(embedded_dashboard_ids)
- )
- if any(is_uuid(id_) for id_ in embedded_dashboard_ids)
- else Dashboard.id.in_(embedded_dashboard_ids)
- )
- filters.append(condition)
+ if (guest_condition := guest_embedded_dashboard_filter()) is not None:
+ filters.append(guest_condition)
Review Comment:
**Suggestion:** Appending `guest_condition` to the existing access alternatives and later combining them with `or_` does not scope guests to their token dashboards. A guest who has ordinary viewer, editor, dataset, or extra-filter access can still receive dashboards absent from the guest token. Apply the guest condition as the sole access predicate for guest users, as `ChartFilter` does; the same issue exists in the legacy branch where it is added to `feature_flagged_filters`. [security]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ Guest dashboard listings can include dashboards absent from token resources.
- β οΈ Dashboard metadata visibility becomes broader than token scoping.
- β οΈ Legacy and viewer-enabled filters exhibit the same widening.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ef321cd7f2664609b0964d1270a46ffd&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=ef321cd7f2664609b0964d1270a46ffd&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/dashboards/filters.py
**Line:** 196:203
**Comment:**
*Security: Appending `guest_condition` to the existing access alternatives and later combining them with `or_` does not scope guests to their token dashboards. A guest who has ordinary viewer, editor, dataset, or extra-filter access can still receive dashboards absent from the guest token. Apply the guest condition as the sole access predicate for guest users, as `ChartFilter` does; the same issue exists in the legacy branch where it is added to `feature_flagged_filters`.
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%2F43066&comment_hash=a23645d2457324828b2fd136a838c48b67eb74176e64528df6950afa2e7e6872&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43066&comment_hash=a23645d2457324828b2fd136a838c48b67eb74176e64528df6950afa2e7e6872&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