feat(mcp): add embeddable charts with guest token authentication #36933
+2,387
−41
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
✅ 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}
Contributor
|
CodeAnt AI finished reviewing your PR. |
Contributor
💡 Enhance Your PR ReviewsWe noticed that 3 feature(s) are not configured for this repository. Enabling these features can help improve your code quality and workflow: 🚦 Quality GatesStatus: Quality Gates are not enabled at the organization level 🎫 Jira Ticket ComplianceStatus: Jira credentials file not found. Please configure Jira integration in your settings ⚙️ Custom RulesStatus: No custom rules configured. Add rules via organization settings or .codeant/review.json in your repository Want to enable these features? Contact your organization admin or check our documentation for setup instructions. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #36933 +/- ##
===========================================
+ Coverage 0 67.72% +67.72%
===========================================
Files 0 650 +650
Lines 0 48261 +48261
Branches 0 5251 +5251
===========================================
+ Hits 0 32687 +32687
- Misses 0 14292 +14292
- Partials 0 1282 +1282
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #e877b0
Actionable Suggestions - 1
-
superset-frontend/webpack.config.js - 1
- Path format inconsistency · Line 300-304
Additional Suggestions - 8
-
superset/embedded_chart/api.py - 3
-
API Response Consistency · Line 110-111Ensure datasource_id and datasource_type are not null in the JSON response to match the OpenAPI schema, using defaults consistent with GetExplorePermalinkCommand logic.
-
Type Safety Improvement · Line 99-104Add type hints to local variables to comply with type safety standards for new Python code. This improves maintainability and aligns with MyPy requirements.
-
Logging Cleanup · Line 118-118Remove redundant %s ex from logger.exception, as the method already logs the full traceback and exception details.
-
-
superset/embedded_chart/exceptions.py - 4
-
Messages not internationalized · Line 21-21Exception messages appear to be user-facing and should be internationalized using flask_babel._() for consistency with other Superset exceptions like SupersetMarshmallowValidationError. This ensures messages can be translated if the application supports multiple languages.
-
Missing docstring · Line 20-20This new exception class lacks a docstring, which is required for all new classes per the project's coding standards. Consider adding a brief description like 'Raised when an embedded chart permalink is not found or has expired.'
Code suggestion
@@ -20,2 +20,3 @@ - class EmbeddedChartPermalinkNotFoundError(SupersetException): - message = "The embedded chart permalink could not be found or has expired." + class EmbeddedChartPermalinkNotFoundError(SupersetException): + """Raised when an embedded chart permalink is not found or has expired.""" + message = "The embedded chart permalink could not be found or has expired."
-
Missing docstring · Line 24-24This new exception class lacks a docstring, which is required for all new classes per the project's coding standards. Consider adding a brief description like 'Raised when access to an embedded chart is denied.'
Code suggestion
@@ -24,2 +24,4 @@ - class EmbeddedChartAccessDeniedError(SupersetException): - message = "Access to this embedded chart is denied." + class EmbeddedChartAccessDeniedError(SupersetException): + """Raised when access to an embedded chart is denied.""" + message = "Access to this embedded chart is denied."
-
Missing docstring · Line 28-28This new exception class lacks a docstring, which is required for all new classes per the project's coding standards. Consider adding a brief description like 'Raised when the embedded chart feature is disabled.'
-
-
superset-frontend/src/embeddedChart/index.tsx - 1
-
Inline styles usage · Line 80-80The code uses inline styles in several places (e.g., error messages, loading containers, chart wrapper), which may conflict with the guideline to 'avoid custom css and styles' and follow antd best practices whenever possible. While necessary for embedded viewport filling, consider refactoring to use antd's Flex or Space components for layout, or extract to a CSS module if custom styling is unavoidable.
-
Review Details
-
Files reviewed - 15 · Commit Range:
99b1a97..99b1a97- superset-frontend/src/embeddedChart/index.tsx
- superset-frontend/webpack.config.js
- superset/config.py
- superset/embedded_chart/__init__.py
- superset/embedded_chart/api.py
- superset/embedded_chart/exceptions.py
- superset/embedded_chart/view.py
- superset/initialization/__init__.py
- superset/mcp_service/app.py
- superset/mcp_service/embedded_chart/__init__.py
- superset/mcp_service/embedded_chart/schemas.py
- superset/mcp_service/embedded_chart/tool/__init__.py
- superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py
- superset/security/guest_token.py
- superset/security/manager.py
-
Files skipped - 0
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
Contributor
Author
Code Review Responses✅ Fixed Issues
❌ Not Applicable / Incorrect Suggestions
|
Contributor
There was a problem hiding this comment.
Code Review Agent Run #a3d995
Actionable Suggestions - 1
-
superset/security/manager.py - 1
- Blind exception catch too broad · Line 2850-2851
Additional Suggestions - 2
-
superset-frontend/src/embeddedChart/index.tsx - 2
-
Improved null safety · Line 125-126The added null check for event.data prevents potential TypeError if data is null/undefined, aligning with TypeScript best practices for MessageEvent handling.
-
Enhanced error handling · Line 292-298Wrapping start() in .catch() handles async failures gracefully, logging errors and resetting state, which improves reliability for embedded chart initialization.
-
Review Details
-
Files reviewed - 16 · Commit Range:
99b1a97..4be0466- superset-frontend/src/embeddedChart/index.tsx
- superset-frontend/webpack.config.js
- superset/config.py
- superset/embedded_chart/__init__.py
- superset/embedded_chart/api.py
- superset/embedded_chart/exceptions.py
- superset/embedded_chart/view.py
- superset/initialization/__init__.py
- superset/mcp_service/app.py
- superset/mcp_service/embedded_chart/__init__.py
- superset/mcp_service/embedded_chart/schemas.py
- superset/mcp_service/embedded_chart/tool/__init__.py
- superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py
- superset/security/guest_token.py
- superset/security/manager.py
- tests/integration_tests/security_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
Contributor
Code Review Agent Run #86accdActionable 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 |
Contributor
Code Review Agent Run #0b3e6dActionable 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 |
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
Code Review Agent Run #42298fActionable Suggestions - 0Additional Suggestions - 1
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 |
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
aminghadersohi
requested review from
betodealmeida,
craig-rueda,
dpgaspar,
eschutho,
geido,
kgabryje,
michael-s-molina,
mistercrunch,
nytai,
rusackas,
sadpandajoe and
villebro
as code owners
Contributor
Code Review Agent Run #ee1305Actionable 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 |
Contributor
Author
yeah you're both 100% right. let me get rid of it |
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
Code Review Agent Run #8c50b8Actionable Suggestions - 0Additional Suggestions - 1
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 |
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Add a new MCP tool `get_embeddable_chart` that enables AI agents to create ephemeral chart visualizations with guest token authentication. This allows charts to be embedded in external applications (Claude Desktop, ChatGPT, etc.) without persisting them to the database. Key changes: - Add CHART_PERMALINK resource type to GuestTokenResourceType enum - Add validation for chart_permalink resources in SecurityManager - Add EMBEDDABLE_CHARTS_MCP feature flag (disabled by default) - Create embedded_chart backend module with API and view - Create MCP tool with schemas for get_embeddable_chart - Create frontend embeddedChart entry using StatefulChart component The feature uses the existing permalink system for chart state storage with TTL support, and leverages StatefulChart for lightweight rendering that handles data fetching internally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix: address CI failures for embeddable charts PR - Remove literal color apache#666 from embeddedChart (use default text color) - Shorten long description lines in schemas.py - Fix import ordering in app.py - Add EmbeddedChartRestApi and EmbeddedChartView to security allowlist 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix: address code review feedback for embeddable charts - Fix null pointer in validateMessageEvent (typeof null === 'object') - Fix race condition with started flag - reset on failure - Fix security issue - don't leak exception message in 500 response - Add docstrings to exception classes - Remove redundant %s formatting in logger.exception 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix: remove exception chaining to prevent internal detail leakage Remove `from ex` in exception handling to prevent leaking internal exception details. Re-raise EmbeddedChartPermalinkNotFoundError directly if already raised, otherwise raise a new instance without chaining. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix: use 'from None' to satisfy ruff B904 rule Within an except clause, using 'from None' explicitly suppresses exception chaining which prevents internal error details from being exposed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> chore: format with ruff 0.9.7 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> chore: format embeddedChart with prettier 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix: add proper type annotations for mypy compliance - Import GuestTokenUser, GuestTokenResource, GuestTokenRlsRule types - Add type annotations to guest_user, resources, rls_rules - Use GuestTokenResourceType enum directly instead of .value - Handle bytes return type from create_guest_access_token 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix: use .get() for GUEST_TOKEN_HEADER_NAME config access Provides a default value to prevent potential KeyError. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> chore: re-trigger CI for flaky test The sharded-jest-tests (3) failure is a flaky test timeout (DatasourceEditor.test.tsx) unrelated to this PR's changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): add referrer validation and @Protect() decorator for embeddable charts Security improvements based on code review comparing with dashboard embedding: 1. Add optional allowed_domains parameter to get_embeddable_chart MCP tool - Stored in permalink state for later validation - Allows restricting which domains can embed the chart 2. Add referrer validation to EmbeddedChartView - Validates request.referrer against allowed_domains - Uses same same_origin() check as dashboard embedding - Returns 403 if referrer not in allowed domains list 3. Add @Protect() decorator to EmbeddedChartRestApi.get() - Consistent with EmbeddedDashboardRestApi pattern - Adds class_permission_name for FAB permissions 4. Include allowed_domains in API response - Useful for debugging and validation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> chore: re-trigger CI for flaky test The sharded-jest-tests (3) failure is a flaky test timeout (DatasourceEditor.test.tsx) unrelated to this PR's changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): fix API response format and PyJWT compatibility for embeddable charts 1. Fix API response format to match frontend expectations: - Frontend expects { state: { formData: {...} } } - Was returning { result: { form_data: {...} } } - Now returns state directly from permalink 2. Fix PyJWT compatibility: - PyJWT 2.0+ returns string, older versions return bytes - Handle both cases when decoding guest token 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): add guest token security for embedded chart data access - Add guest token validation in embedded_chart API to verify permalink access - Add has_guest_chart_permalink_access() to security manager to allow guest users with chart_permalink resources to access datasources - Add can_read CurrentUserRestApi to PUBLIC_ROLE_PERMISSIONS for frontend initialization in embedded views - Remove redundant referrer validation from view (now handled by guest token) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): remove EMBEDDABLE_CHARTS_MCP feature flag, use EMBEDDED_SUPERSET Per PR review feedback, avoid adding a new feature flag since MCP is already feature-flagged. Embedded charts now use the existing EMBEDDED_SUPERSET flag which is consistent with dashboard embedding. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): remove all feature flag checks from embedded charts Per PR review feedback, remove all feature flag usage. The embedded chart functionality is now always available - no feature flags needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): add isinstance check for GuestUser in has_guest_chart_permalink_access The method was failing for regular User objects that don't have a resources attribute. Added explicit isinstance(user, GuestUser) check to ensure we only access user.resources for actual GuestUser instances. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): fix ruff linting issues in embedded chart files - Fix import sorting in api.py - Remove unused imports in view.py (Any, cast, same_origin, GetExplorePermalinkCommand) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
aminghadersohi
force-pushed
the
amin/embeddable-charts-mcp
branch
from
e07f57d to
ccb0742
Compare
github-actions
bot
removed
doc
Contributor
There was a problem hiding this comment.
Code Review Agent Run #68cad7
Actionable Suggestions - 1
-
superset/security/manager.py - 1
- Blind exception catch too broad · Line 2854-2855
Additional Suggestions - 2
-
superset-frontend/src/embeddedChart/index.tsx - 2
-
Unchecked DOM element access · Line 76-76The code asserts appMountPoint is non-null with `!`, but if `document.getElementById('app')` returns null, this will cause a runtime TypeError. In an embedded context, the element should exist, but adding a check improves robustness.
Code suggestion
diff --git a/superset-frontend/src/embeddedChart/index.tsx b/superset-frontend/src/embeddedChart/index.tsx index 0000000..1234567 100644 --- a/superset-frontend/src/embeddedChart/index.tsx +++ b/superset-frontend/src/embeddedChart/index.tsx @@ -75,7 +75,9 @@ interface PermalinkState { formData: QueryFormData; } -const appMountPoint = document.getElementById('app')!; +const appMountPoint = document.getElementById('app'); +if (!appMountPoint) { + throw new Error('App mount point element not found'); +} const MESSAGE_TYPE = '__embedded_comms__'; function showFailureMessage(message: string) {
-
Potential XSS via innerHTML · Line 80-80Using innerHTML to inject content poses XSS risks, even if the message is from a translation function. Switch to creating DOM elements programmatically for safer content insertion.
-
Review Details
-
Files reviewed - 15 · Commit Range:
ccb0742..ccb0742- superset-frontend/src/embeddedChart/index.tsx
- superset-frontend/webpack.config.js
- superset/embedded_chart/__init__.py
- superset/embedded_chart/api.py
- superset/embedded_chart/exceptions.py
- superset/embedded_chart/view.py
- superset/initialization/__init__.py
- superset/mcp_service/app.py
- superset/mcp_service/embedded_chart/__init__.py
- superset/mcp_service/embedded_chart/schemas.py
- superset/mcp_service/embedded_chart/tool/__init__.py
- superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py
- superset/security/guest_token.py
- superset/security/manager.py
- tests/integration_tests/security_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
Comment on lines
+2854
to
+2855
| except Exception: | ||
| raise EmbeddedChartPermalinkNotFoundError() from None |
Contributor
There was a problem hiding this comment.
Blind exception catch too broad
Replace the broad except Exception on line 2854 with specific exception types. Catch only EmbeddedChartPermalinkNotFoundError or other expected exceptions to avoid masking unexpected errors.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| except Exception: | |
| raise EmbeddedChartPermalinkNotFoundError() from None | |
| except (EmbeddedChartPermalinkNotFoundError, ValueError, KeyError): | |
| raise EmbeddedChartPermalinkNotFoundError() from None |
Code Review Run #68cad7
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Add "Embed chart" option to chart header dropdown menu on dashboards. When clicked, opens a modal that generates embed code (iframe + guest token) using the existing permalink + guest token infrastructure. Changes: - Add POST /api/v1/embedded_chart/ endpoint to create embeddable charts - Add EmbeddedChartModal component with allowed domains and TTL settings - Add "Embed chart" menu item to SliceHeaderControls - Add EmbedChart to MenuKeys enum This brings UI parity with PR apache#33424's approach while using our simpler ephemeral permalink-based architecture (no database model needed). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
There was a problem hiding this comment.
Code Review Agent Run #e87f28
Actionable Suggestions - 1
-
superset/embedded_chart/api.py - 1
- Input Validation Gap · Line 241-241
Additional Suggestions - 2
-
superset/embedded_chart/api.py - 1
-
Unbounded Input · Line 239-239ttl_minutes lacks bounds checking, allowing negative values (past expiration) or very large ones. Add min/max validation to ensure reasonable embed lifetimes.
-
-
superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx - 1
-
Script Injection Risk · Line 119-120The inline script embeds guest_token and iframe_url directly in template literals, which could break if these values contain single quotes. Use JSON.stringify to safely escape them.
-
Review Details
-
Files reviewed - 18 · Commit Range:
ccb0742..2b4b4f8- superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx
- superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
- superset-frontend/src/dashboard/types.ts
- superset-frontend/src/embeddedChart/index.tsx
- superset-frontend/webpack.config.js
- superset/embedded_chart/__init__.py
- superset/embedded_chart/api.py
- superset/embedded_chart/exceptions.py
- superset/embedded_chart/view.py
- superset/initialization/__init__.py
- superset/mcp_service/app.py
- superset/mcp_service/embedded_chart/__init__.py
- superset/mcp_service/embedded_chart/schemas.py
- superset/mcp_service/embedded_chart/tool/__init__.py
- superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py
- superset/security/guest_token.py
- superset/security/manager.py
- tests/integration_tests/security_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
| allowed_domains: list[str] = body.get("allowed_domains", []) | ||
| ttl_minutes: int = body.get("ttl_minutes", 60) | ||
|
|
||
| if not form_data: |
Contributor
There was a problem hiding this comment.
Input Validation Gap
The code checks if form_data is truthy but doesn't validate its structure. Since CreateExplorePermalinkCommand expects 'datasource' in formData, invalid input could cause runtime exceptions. Add early validation to prevent this.
Code Review Run #e87f28
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Add CORS/referrer validation to embedded chart view, matching the dashboard embedding behavior. If allowed_domains is configured in the permalink state, validate that the request referrer matches one of the allowed domains before rendering. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
There was a problem hiding this comment.
Code Review Agent Run #b9e5b1
Actionable Suggestions - 1
-
superset/embedded_chart/view.py - 1
- Security Bug in Referrer Validation · Line 42-53
Review Details
-
Files reviewed - 18 · Commit Range:
2b4b4f8..1e6c4ee- superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx
- superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
- superset-frontend/src/dashboard/types.ts
- superset-frontend/src/embeddedChart/index.tsx
- superset-frontend/webpack.config.js
- superset/embedded_chart/__init__.py
- superset/embedded_chart/api.py
- superset/embedded_chart/exceptions.py
- superset/embedded_chart/view.py
- superset/initialization/__init__.py
- superset/mcp_service/app.py
- superset/mcp_service/embedded_chart/__init__.py
- superset/mcp_service/embedded_chart/schemas.py
- superset/mcp_service/embedded_chart/tool/__init__.py
- superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py
- superset/security/guest_token.py
- superset/security/manager.py
- tests/integration_tests/security_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
Comment on lines
+42
to
+53
| def same_origin(url1: str | None, url2: str | None) -> bool: | ||
| """Check if two URLs have the same origin (scheme + netloc).""" | ||
| if not url1 or not url2: | ||
| return False | ||
| parsed1 = urlparse(url1) | ||
| parsed2 = urlparse(url2) | ||
| # For domain matching, we just check if the host matches | ||
| # url2 might just be a domain like "example.com" | ||
| if not parsed2.scheme: | ||
| # url2 is just a domain, check if it matches url1's netloc | ||
| return parsed1.netloc == url2 or parsed1.netloc.endswith(f".{url2}") | ||
| return (parsed1.scheme, parsed1.netloc) == (parsed2.scheme, parsed2.netloc) |
Contributor
There was a problem hiding this comment.
Security Bug in Referrer Validation
The same_origin function incorrectly uses netloc (which includes ports) for domain matching, causing it to reject valid referrers with non-standard ports like 'https://example.com:8080' when 'example.com' is allowed. It should use hostname for host-only comparison and perform case-insensitive matching, as hosts are case-insensitive per RFC.
Code Review Run #b9e5b1
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
- Remove CodeBlock styled component using deprecated theme.colors - Change CodeOutlined icon to ExportOutlined (valid icon) - Add chartId prop to EmbeddedChartModal - Simplify EmbeddedChartModal to use existing embedded chart API - Fix ESLint issues (duplicate imports, destructuring, useEffect pattern) - Replace console.error with logging.error 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Add new embedded chart permissions to the expected permissions set in test_info_security_chart test. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
The Embed code option (simple iframe URL) was previously always available with EMBEDDABLE_CHARTS: True by default. Keep it always visible since it's a simple feature. Only gate the new Embed chart option (which uses guest tokens) behind the EMBEDDED_SUPERSET feature flag. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Revert to using EMBEDDABLE_CHARTS instead of EMBEDDED_SUPERSET for chart embedding features to maintain backwards compatibility. - EMBEDDABLE_CHARTS: True by default (for Embed code/chart options) - EMBEDDED_SUPERSET: False by default (for dashboard embedding with guest tokens) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
There was a problem hiding this comment.
Code Review Agent Run #4834de
Actionable Suggestions - 3
-
superset/config.py - 1
- Breaking change in stable feature flag · Line 594-595
-
superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx - 1
- Error Handling Issue · Line 142-146
-
superset/charts/api.py - 1
- Missing Permission Decorator · Line 1247-1339
Additional Suggestions - 9
-
tests/integration_tests/charts/api_tests.py - 1
-
Unrestricted embedded permissions · Line 309-312These embedded permissions are added to the expected set, but unlike "can_set_embedded", they are not restricted in the security manager. This could allow unauthorized access to embedded configurations. Consider adding them to ADMIN_ONLY_PERMISSIONS to match the pattern for admin-only features.
-
-
superset/charts/api.py - 1
-
Incomplete Transaction Handling · Line 1320-1339The try block only catches ValidationError from schema load, but if EmbeddedChartDAO.upsert raises an exception, the session won't rollback, risking partial DB commits.
-
-
superset/daos/chart.py - 1
-
Type Hint Error · Line 128-128The type hint for 'item' in the create method appears incorrect, as it references the DAO class rather than the model type expected by BaseDAO[EmbeddedChart]. This could cause MyPy type checking failures, though it won't affect runtime since the method raises NotImplementedError.
Code suggestion
@@ -128,1 +128,1 @@ -item: EmbeddedChartDAO | None = None, +item: EmbeddedChart | None = None,
-
-
superset/models/embedded_chart.py - 1
-
Property logic refinement · Line 53-58The `allowed_domains` property could include empty strings if `allow_domain_list` has consecutive commas or leading/trailing commas, which might cause unexpected behavior in embedding checks. Consider refining the logic to handle malformed input gracefully.
Code suggestion
@@ -53,6 +53,6 @@ @property def allowed_domains(self) -> list[str]: """ A list of domains which are allowed to embed the chart. An empty list means any domain can embed. """ - return self.allow_domain_list.split(",") if self.allow_domain_list else [] + return [domain.strip() for domain in self.allow_domain_list.split(",") if domain.strip()] if self.allow_domain_list else []
-
-
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx - 1
-
Icon choice for embed menu item · Line 501-501The icon for the 'Embed chart' menu item was changed to ExportOutlined, but embedding charts typically involves code generation, so CodeOutlined is more appropriate.
Code suggestion
@@ -501,1 +501,1 @@ - icon: <Icons.ExportOutlined css={dropdownIconsStyles} />, + icon: <Icons.CodeOutlined css={dropdownIconsStyles} />,
-
-
superset/views/embedded_charts.py - 1
-
Missing Documentation · Line 33-33The list method is missing a docstring. Per the project's coding standards in AGENTS.md and .cursor/rules/dev-standard.mdc, docstrings are required for new functions and methods to improve maintainability.
Code suggestion
@@ -31,6 +31,7 @@ @expose("/list/") @has_access def list(self) -> FlaskResponse: + """Render the embedded charts list page.""" return super().render_app_template()
-
-
superset/initialization/__init__.py - 1
-
Category Mismatch · Line 526-533The category "Security" may not fit for EmbeddedChartsView, as it's chart management rather than security. Other chart views like SliceModelView use no category. If embedding has security implications, keep as is.
-
-
superset/commands/chart/delete.py - 1
-
Code Structure Inconsistency · Line 81-81The run method here returns the result of EmbeddedChartDAO.delete, but the similar DeleteChartCommand.run method in the same file does not return. For consistency within the codebase, consider removing the return statement.
-
-
superset/charts/schemas.py - 1
-
Schema metadata suggestion · Line 348-356The new EmbeddedChartConfigSchema and EmbeddedChartResponseSchema classes are correctly defined with appropriate field types and requirements that match their usage in the charts API. However, consider adding metadata to the fields for better OpenAPI documentation, as seen in other schemas in this file.
-
Review Details
-
Files reviewed - 37 · Commit Range:
1e6c4ee..66ad889- superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx
- superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
- superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
- superset-frontend/src/dashboard/components/menu/ShareMenuItems/ShareMenuItems.test.tsx
- superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx
- superset-frontend/src/dashboard/types.ts
- superset-frontend/src/embeddedChart/index.tsx
- superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx
- superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
- superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx
- superset-frontend/src/pages/EmbeddedChartsList/index.tsx
- superset-frontend/src/views/routes.tsx
- superset-frontend/webpack.config.js
- superset/charts/api.py
- superset/charts/schemas.py
- superset/commands/chart/delete.py
- superset/commands/chart/exceptions.py
- superset/config.py
- superset/daos/chart.py
- superset/embedded_chart/__init__.py
- superset/embedded_chart/api.py
- superset/embedded_chart/exceptions.py
- superset/embedded_chart/view.py
- superset/initialization/__init__.py
- superset/mcp_service/app.py
- superset/mcp_service/embedded_chart/__init__.py
- superset/mcp_service/embedded_chart/schemas.py
- superset/mcp_service/embedded_chart/tool/__init__.py
- superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py
- superset/migrations/versions/2026-01-08_16-45_50aff65919a0_add_embedded_charts_table.py
- superset/models/embedded_chart.py
- superset/models/slice.py
- superset/security/guest_token.py
- superset/security/manager.py
- superset/views/embedded_charts.py
- tests/integration_tests/charts/api_tests.py
- tests/integration_tests/security_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
Comment on lines
594
to
595
| # Enable caching per user key for Superset cache (not database cache impersonation) | ||
| "CACHE_QUERY_BY_USER": False, |
Contributor
There was a problem hiding this comment.
Breaking change in stable feature flag
Removing the stable EMBEDDABLE_CHARTS feature flag from DEFAULT_FEATURE_FLAGS disables chart embedding by default, which is a breaking change for users relying on the default configuration. According to FEATURE_FLAGS.md and official docs, this flag is still stable and not deprecated. If this removal is intentional, consider adding a deprecation notice first.
Code Review Run #4834de
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Comment on lines
142
to
146
| .then(({ result }) => { | ||
| setReady(true); | ||
| setEmbedded(result); | ||
| setAllowedDomains(result ? result.allowed_domains.join(', ') : ''); | ||
| }); |
Contributor
There was a problem hiding this comment.
Error Handling Issue
The useEffect fails to set ready to true on API errors, potentially leaving the component stuck in loading state. Add a .finally block to ensure ready is always set.
Code suggestion
Check the AI-generated fix before applying
@@ -141,6 +141,7 @@
.then(({ result }) => {
setReady(true);
setEmbedded(result);
setAllowedDomains(result ? result.allowed_domains.join(', ') : '');
})
+ .finally(() => setReady(true));
Code Review Run #4834de
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Comment on lines
1247
to
1339
| @expose("/<id_or_uuid>/embedded", methods=["POST", "PUT"]) | ||
| @protect() | ||
| @safe | ||
| @statsd_metrics | ||
| @event_logger.log_this_with_context( | ||
| action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.set_embedded", | ||
| log_to_statsd=False, | ||
| ) | ||
| def set_embedded(self, id_or_uuid: str) -> Response: | ||
| """Set a chart's embedded configuration. | ||
| --- | ||
| post: | ||
| summary: Set a chart's embedded configuration | ||
| parameters: | ||
| - in: path | ||
| schema: | ||
| type: string | ||
| name: id_or_uuid | ||
| description: The chart id or uuid | ||
| requestBody: | ||
| description: The embedded configuration to set | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: EmbeddedChartConfigSchema | ||
| responses: | ||
| 200: | ||
| description: Successfully set the configuration | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| result: | ||
| $ref: '#/components/schemas/EmbeddedChartResponseSchema' | ||
| 401: | ||
| $ref: '#/components/responses/401' | ||
| 404: | ||
| $ref: '#/components/responses/404' | ||
| 500: | ||
| $ref: '#/components/responses/500' | ||
| put: | ||
| description: >- | ||
| Sets a chart's embedded configuration. | ||
| parameters: | ||
| - in: path | ||
| schema: | ||
| type: string | ||
| name: id_or_uuid | ||
| description: The chart id or uuid | ||
| requestBody: | ||
| description: The embedded configuration to set | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: EmbeddedChartConfigSchema | ||
| responses: | ||
| 200: | ||
| description: Successfully set the configuration | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| result: | ||
| $ref: '#/components/schemas/EmbeddedChartResponseSchema' | ||
| 401: | ||
| $ref: '#/components/responses/401' | ||
| 404: | ||
| $ref: '#/components/responses/404' | ||
| 500: | ||
| $ref: '#/components/responses/500' | ||
| """ | ||
| try: | ||
| chart = ChartDAO.get_by_id_or_uuid(id_or_uuid) | ||
| except ChartNotFoundError: | ||
| return self.response_404() | ||
|
|
||
| try: | ||
| body = self.embedded_config_schema.load(request.json) | ||
|
|
||
| embedded = EmbeddedChartDAO.upsert( | ||
| chart, | ||
| body["allowed_domains"], | ||
| ) | ||
| db.session.commit() # pylint: disable=consider-using-transaction | ||
|
|
||
| result = self.embedded_response_schema.dump(embedded) | ||
| return self.response(200, result=result) | ||
| except ValidationError as error: | ||
| db.session.rollback() # pylint: disable=consider-using-transaction | ||
| return self.response_400(message=error.messages) | ||
|
|
Contributor
There was a problem hiding this comment.
Missing Permission Decorator
The set_embedded endpoint lacks a @permission_name decorator, unlike get_embedded ('read') and delete_embedded ('set_embedded'). This could allow unauthorized config changes if permissions aren't enforced elsewhere.
Citations
- Rule Violated: AGENTS.md:85
Code Review Run #4834de
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Contributor
Code Review Agent Run #d77337Actionable Suggestions - 0Additional Suggestions - 5
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 |
- Add @permission_name("set_embedded") decorator to set_embedded endpoint
- Fix error handling in EmbeddedChartModal to always set ready state
- Add datasource validation in embedded chart API create endpoint
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
Code Review Agent Run #86e5b3Actionable 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Add embeddable charts with guest token authentication, accessible via both:
get_embeddable_chart) - For AI agents to create charts programmaticallyKey Features:
CHART_PERMALINKresource type for secure embeddingArchitecture:
Files Changed:
superset/security/guest_token.py- Add CHART_PERMALINK enumsuperset/security/manager.py- Add validation and access control logicsuperset/embedded_chart/- Backend module (api, view, exceptions)superset/mcp_service/embedded_chart/- MCP tool with schemassuperset-frontend/src/embeddedChart/- Frontend entry pointsuperset-frontend/src/dashboard/components/EmbeddedChartModal/- UI modalsuperset-frontend/src/dashboard/components/SliceHeaderControls/- Menu itemBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
New "Embed chart" menu option:
Embedded chart rendering:
TESTING INSTRUCTIONS
Testing via UI
Testing via MCP (for AI Agents)
1. List available charts:
2. Get chart details:
3. Create embeddable chart:
4. Create test HTML file with returned values:
5. Open HTML file in browser and verify chart renders
ADDITIONAL INFORMATION
Comparison with PR #33424:
This PR takes a simpler approach than #33424:
CHART_PERMALINKguest token infrastructure🤖 Generated with Claude Code