fix(mcp): reject unsafe dashboard layout replacements - #43476
fix(mcp): reject unsafe dashboard layout replacements#43476aminghadersohi wants to merge 4 commits into
Conversation
dosubot
Bot
added
change:backend
Code Review Agent Run #bb4f3fActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if isinstance(node, dict) | ||
| and node.get("type") == "CHART" | ||
| and (node.get("meta") or {}).get("chartId") in (chart_id, str(chart_id)) | ||
| and normalize_chart_id((node.get("meta") or {}).get("chartId")) == chart_id |
There was a problem hiding this comment.
Suggestion: normalize_chart_id accepts non-canonical decimal strings such as 001, so this line removes the matching layout chart even though _clean_json_metadata only removes the canonical forms 1 and "1". The chart can therefore be detached while stale 001 references remain in timed-refresh, immune-slice, filter-scope, expanded-slice, or default-filter metadata. Reject leading-zero strings in normalization or make metadata cleanup use the same normalization. [logic error]
Severity Level: Minor 🧹
- ⚠️ Chart removal leaves stale expanded-slice metadata.
- ⚠️ Timed-refresh metadata can retain detached chart IDs.
- ⚠️ Filter and default-filter references may target removed charts.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py
**Line:** 68:68
**Comment:**
*Logic Error: `normalize_chart_id` accepts non-canonical decimal strings such as `001`, so this line removes the matching layout chart even though `_clean_json_metadata` only removes the canonical forms `1` and `"1"`. The chart can therefore be detached while stale `001` references remain in timed-refresh, immune-slice, filter-scope, expanded-slice, or default-filter metadata. Reject leading-zero strings in normalization or make metadata cleanup use the same normalization.
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 fixThere was a problem hiding this comment.
Fixed in 7669bba. normalize_chart_id now rejects non-canonical decimal strings, so "001" no longer matches a chart during layout removal. That keeps it consistent with _clean_json_metadata, which keys off str(chart_id) — the two paths can no longer disagree about whether a reference matches, so a chart cannot be detached while stale 001 entries survive in expanded_slices, timed_refresh_immune_slices, filter_scopes, or default_filters. Added test_rejects_leading_zero_string_chart_id, which fails without the change.
|
The flagged issue is correct. The To resolve this, you should update def normalize_chart_id(value: Any) -> int | None:
"""Normalize an integer or canonical decimal-string chart ID."""
if isinstance(value, bool):
return None
if isinstance(value, int):
return value if value > 0 else None
if isinstance(value, str) and value.isascii() and value.isdecimal():
if value != "0" and value.startswith("0"):
return None
normalized = int(value)
return normalized if normalized > 0 else None
return NoneWould you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset/mcp_service/dashboard/layout_validation.py |
| if isinstance(value, str) and value.isascii() and value.isdecimal(): | ||
| normalized = int(value) | ||
| return normalized if normalized > 0 else None |
There was a problem hiding this comment.
Suggestion: int(value) can raise ValueError for an excessively long decimal string when Python's integer string conversion limit is enabled. Because this value comes from the MCP payload and is not caught, a malformed chart ID causes the validator to escape instead of returning InvalidDashboardLayout; bound the string length or catch the conversion error and return None. [type error]
Severity Level: Major ⚠️
- ❌ Malformed layout requests escape structured MCP validation.
- ⚠️ Clients receive an unexpected tool exception.
- ⚠️ Invalid updates may not produce consistent error types.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/layout_validation.py
**Line:** 84:86
**Comment:**
*Type Error: `int(value)` can raise `ValueError` for an excessively long decimal string when Python's integer string conversion limit is enabled. Because this value comes from the MCP payload and is not caught, a malformed chart ID causes the validator to escape instead of returning `InvalidDashboardLayout`; bound the string length or catch the conversion error and return `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 fixThere was a problem hiding this comment.
Fixed in 7669bba. The decimal-string length is now bounded before int(), so an oversized value cannot hit CPython's integer string conversion limit (confirmed: 4300 digits by default) and raise ValueError out of the validator. Malformed IDs return None and surface as a structured InvalidDashboardLayout like every other invalid input. Added test_rejects_oversized_string_chart_id, which fails without the change.
| continue | ||
| stack: list[tuple[str, bool]] = [(start_id, False)] |
There was a problem hiding this comment.
Suggestion: The validator does not enforce the frontend's maximum nesting depths from parentMaxDepthLookup. An MCP replacement can therefore persist ROW/COLUMN nesting that the dashboard editor explicitly rejects, producing layouts outside the supported parent/depth contract. Track the effective depth during iterative traversal and reject edges exceeding the corresponding frontend limit. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ MCP can persist layouts outside frontend depth limits.
- ⚠️ Dashboard editor operations reject persisted deep nesting.
- ⚠️ Users may need manual layout repair before editing.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/layout_validation.py
**Line:** 168:169
**Comment:**
*Incomplete Implementation: The validator does not enforce the frontend's maximum nesting depths from `parentMaxDepthLookup`. An MCP replacement can therefore persist ROW/COLUMN nesting that the dashboard editor explicitly rejects, producing layouts outside the supported parent/depth contract. Track the effective depth during iterative traversal and reject edges exceeding the corresponding frontend limit.
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 fixThere was a problem hiding this comment.
Fixed in 7669bba. Fair point — the validator was mirroring the type half of isValidChild.ts but not the depth half, which is inconsistent since _ALLOWED_CHILD_TYPES was itself derived from parentMaxDepthLookup's shape.
The allowed-child-type table is now derived directly from the depth table (_PARENT_MAX_DEPTH), so the two cannot drift apart. Depth is tracked on the existing iterative reachability walk rather than recursively, and it is derived from the validated children edges rather than trusted from the payload — same reasoning as parents in 037c7ea. TABS and TAB pass their depth through to children, matching the worked examples in the header comment of isValidChild.ts.
I checked this against the regression class from the earlier parents discussion before committing: all 9 shipped example dashboards still validate clean under the depth check, so this does not reintroduce false rejection of real saved layouts.
Tests added: test_rejects_nesting_beyond_frontend_depth_limit (fails without the change), plus test_accepts_maximum_supported_nesting_depth and test_accepts_tabs_without_consuming_depth to pin the upper bound and the tab pass-through so a future tightening can't silently over-reject.
| if request.position_json is not None: | ||
| chart_ids = [chart.id for chart in dashboard.slices] | ||
| if error := validate_dashboard_layout(request.position_json, chart_ids): | ||
| return DashboardError( |
There was a problem hiding this comment.
Suggestion: The chart set is snapshotted from dashboard.slices before the later mutation and commit, with no optimistic-lock or revalidation. If another operation adds a chart after this snapshot but before this commit, this replacement can be accepted without that chart and leave the newly associated chart unreachable; if a chart is removed concurrently, a valid replacement can instead be rejected. Validate and persist against the same locked/versioned dashboard state, or recheck the associations immediately before commit. [race condition]
Severity Level: Major ⚠️
- ⚠️ Concurrent chart additions can become unreachable.
- ⚠️ Concurrent removals can leave stale chart nodes.
- ❌ Dashboard layout and chart associations become inconsistent.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/update_dashboard.py
**Line:** 241:244
**Comment:**
*Race Condition: The chart set is snapshotted from `dashboard.slices` before the later mutation and commit, with no optimistic-lock or revalidation. If another operation adds a chart after this snapshot but before this commit, this replacement can be accepted without that chart and leave the newly associated chart unreachable; if a chart is removed concurrently, a valid replacement can instead be rejected. Validate and persist against the same locked/versioned dashboard state, or recheck the associations immediately before commit.
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 fixThere was a problem hiding this comment.
Not changing this one, but I traced the consequence rather than waving it off.
The window is real — dashboard.slices is read in _validate_update_request and the commit happens later in the same request — but both directions of the race degrade into states the frontend already handles, and neither is the blank-dashboard class this PR exists to prevent:
- Chart added concurrently (missing from the committed layout): hydration has no
CHARTnode for it at all, sochartIdToLayoutId[key]is unset andhydrate.tsauto-places it into a new row (superset-frontend/src/dashboard/actions/hydrate.ts, theif (!chartIdToLayoutId[key] && layout[parentId])branch). This is the self-healing "associated but unplaced" state. - Chart removed concurrently (stale node in the committed layout): the node is reachable, so it renders through the normal missing-chart placeholder path.
The failure this PR targets is specifically present but unreachable — a CHART node that hydration indexes (suppressing the auto-place fallback) while the renderer never reaches it. The validator guarantees every node in the committed tree is reachable from ROOT_ID, and a concurrently-added chart has no node at all rather than an unreachable one, so this race cannot produce that state.
Closing the window properly would need SELECT ... FOR UPDATE or a version column on the dashboard, which is a broader change than this PR and would be inconsistent with the REST UpdateDashboardCommand path, which has the same read-then-commit shape today. Re-reading dashboard.slices just before commit would narrow the window without closing it while adding a query, so it buys correctness theater rather than correctness. Happy to file it separately if you think dashboard-level optimistic locking is worth doing across both paths.
…7987] Addresses review feedback on the dashboard layout validator. Reject non-canonical decimal-string chart IDs. `normalize_chart_id` accepted leading-zero forms such as "001", while the json_metadata cleanup in remove_chart_from_dashboard keys off `str(chart_id)`. A chart could therefore be detached from the layout while stale "001" references survived in expanded_slices, timed_refresh_immune_slices, filter_scopes, and default_filters. Normalization now accepts only canonical forms, so both paths agree on whether a reference matches. Bound the decimal-string length before `int()`. An oversized value from the MCP payload hit CPython's integer string conversion limit and raised ValueError out of the validator, escaping the structured InvalidDashboardLayout contract instead of returning it. Enforce the frontend's `parentMaxDepthLookup` nesting limits. The validator already mirrored the type half of isValidChild.ts but not the depth half, so MCP could persist ROW/COLUMN nesting the dashboard editor rejects. The allowed-child-type table is now derived from the depth table so the two cannot drift, and depth is tracked on the existing iterative reachability walk. Depth is derived rather than trusted, and TABS/TAB pass their depth through to children as they do in the frontend. All 9 shipped example dashboards still validate clean, so this does not reintroduce the class of false rejection fixed in 037c7ea.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43476 +/- ##
===========================================
+ Coverage 66.81% 78.88% +12.07%
===========================================
Files 2876 2878 +2
Lines 164454 164976 +522
Branches 37960 38072 +112
===========================================
+ Hits 109873 130135 +20262
+ Misses 52388 32383 -20005
- Partials 2193 2458 +265
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #c11deaActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
1 participant
SUMMARY
Reject unsafe full
position_jsonreplacements in the MCPupdate_dashboardtool before they overwrite a valid saved layout.Superset renders only layout nodes reachable from
ROOT_ID, but dashboard hydration indexes everyCHARTnode inposition_json. An unreachable chart therefore prevents the normal missing-chart fallback while remaining invisible, which can leave a dashboard blank even though the chart nodes and slice associations still exist.This change validates renderer-required component shape, the v2 marker, graph references, parent/child type compatibility, nesting depth, cycles, component IDs, reachability, and root/top-level-tab shape. Traversal is iterative, imported decimal-string chart IDs are normalized through a helper shared with chart removal, and
DYNAMICnodes are rejected because the backend cannot validate the frontend registry. Reachable chart IDs must exactly match the charts associated with the dashboard.Invalid updates return
InvalidDashboardLayoutbefore any field mutation or commit, preserving the previous layout. The validator handles Superset's two reserved exceptions: detachedHEADER_ID, and the empty detachedGRID_IDretained by top-level tab layouts.Topology is derived rather than trusted. The frontend treats both
parentsand depth as derived metadata and recomputes them during hydration, so saved layouts can legitimately carry stale or missingparents; the validatedchildrenedges are authoritative for both.The MCP schema now explicitly states that
get_dashboard_layoutis a summary rather than a round-trippable raw tree, so callers are not directed into unsafe incremental full replacements.Tracking: SC-117987
[sc-117987]Related: #43133 contains generate-only fallback validation as part of native AI authoring. This PR provides the stronger shared validator for that path to reuse after rebase while keeping update rejection separate from generate fallback behavior.
(Recreated after the original #43367 was closed automatically when its source fork was deleted; review history from that PR is preserved there.)
TESTING INSTRUCTIONS
pytest tests/unit_tests/mcp_service/dashboard/test_layout_validation.py tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.pypre-commit run --files superset/mcp_service/dashboard/layout_validation.py superset/mcp_service/dashboard/schemas.py superset/mcp_service/dashboard/tool/update_dashboard.py superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py tests/unit_tests/mcp_service/dashboard/test_layout_validation.py tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.pyruff checkandruff format --checkpass for all changed Python files.git diff --checksuperset/examples/*/dashboard.yamlvalidate clean, guarding against false rejection of real saved layouts.ADDITIONAL INFORMATION