codeant-ai-for-open-source[bot] commented on code in PR #36933: URL: https://github.com/apache/superset/pull/36933#discussion_r2666503443
########## superset-frontend/src/embeddedChart/index.tsx: ########## @@ -0,0 +1,303 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import 'src/public-path'; + +import { useState, useEffect, useCallback } from 'react'; +import ReactDOM from 'react-dom'; +import { makeApi, t, logging, QueryFormData } from '@superset-ui/core'; +import { StatefulChart } from '@superset-ui/core'; +import Switchboard from '@superset-ui/switchboard'; +import getBootstrapData, { applicationRoot } from 'src/utils/getBootstrapData'; +import setupClient from 'src/setup/setupClient'; +import setupPlugins from 'src/setup/setupPlugins'; +import { store, USER_LOADED } from 'src/views/store'; +import { Loading } from '@superset-ui/core/components'; +import { ErrorBoundary, DynamicPluginProvider } from 'src/components'; +import { addDangerToast } from 'src/components/MessageToasts/actions'; +import ToastContainer from 'src/components/MessageToasts/ToastContainer'; +import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { SupersetThemeProvider } from 'src/theme/ThemeProvider'; +import { ThemeController } from 'src/theme/ThemeController'; +import type { ThemeStorage } from '@apache-superset/core/ui'; +import { Provider as ReduxProvider } from 'react-redux'; + +setupPlugins(); + +const debugMode = process.env.WEBPACK_MODE === 'development'; +const bootstrapData = getBootstrapData(); + +function log(...info: unknown[]) { + if (debugMode) logging.debug(`[superset-embedded-chart]`, ...info); +} + +/** + * In-memory implementation of ThemeStorage interface for embedded contexts. + */ +class ThemeMemoryStorageAdapter implements ThemeStorage { + private storage = new Map<string, string>(); + + getItem(key: string): string | null { + return this.storage.get(key) || null; + } + + setItem(key: string, value: string): void { + this.storage.set(key, value); + } + + removeItem(key: string): void { + this.storage.delete(key); + } +} + +const themeController = new ThemeController({ + storage: new ThemeMemoryStorageAdapter(), +}); + +interface PermalinkState { + formData: QueryFormData; +} + +const appMountPoint = document.getElementById('app')!; +const MESSAGE_TYPE = '__embedded_comms__'; + +function showFailureMessage(message: string) { + appMountPoint.innerHTML = `<div style="display: flex; align-items: center; justify-content: center; height: 100vh; font-family: sans-serif; color: #666;">${message}</div>`; +} + +if (!window.parent || window.parent === window) { + showFailureMessage( + t( + 'This page is intended to be embedded in an iframe, but it looks like that is not the case.', + ), + ); +} + +let displayedUnauthorizedToast = false; + +/** + * Handle unauthorized errors from the API. + */ +function guestUnauthorizedHandler() { + if (displayedUnauthorizedToast) return; + displayedUnauthorizedToast = true; + store.dispatch( + addDangerToast( + t( + 'This session has encountered an interruption. The embedded chart may not load correctly.', + ), + { + duration: -1, + noDuplicate: true, + }, + ), + ); +} + +/** + * Configures SupersetClient with the correct settings for the embedded chart page. + */ +function setupGuestClient(guestToken: string) { + setupClient({ + appRoot: applicationRoot(), + guestToken, + guestTokenHeaderName: bootstrapData.config?.GUEST_TOKEN_HEADER_NAME, + unauthorizedHandler: guestUnauthorizedHandler, + }); +} + +function validateMessageEvent(event: MessageEvent) { + if (typeof event.data !== 'object' || event.data.type !== MESSAGE_TYPE) { Review Comment: **Suggestion:** `validateMessageEvent` accesses `event.data.type` without guarding against `event.data` being null; because `typeof null === 'object'` the function will throw a TypeError when `event.data` is null (instead of throwing the intended validation error), leading to unexpected exceptions. [null pointer] **Severity Level:** Minor ⚠️ ```suggestion const data = event.data; if (data == null || typeof data !== 'object' || (data as any).type !== MESSAGE_TYPE) { ``` <details> <summary><b>Why it matters? ⭐ </b></summary> The current code will throw a runtime TypeError when event.data is null because typeof null === 'object' and the second operand accesses .type. The improved code guards for null/undefined first, which fixes a real bug in message validation. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/embeddedChart/index.tsx **Line:** 125:125 **Comment:** *Null Pointer: `validateMessageEvent` accesses `event.data.type` without guarding against `event.data` being null; because `typeof null === 'object'` the function will throw a TypeError when `event.data` is null (instead of throwing the intended validation error), leading to unexpected exceptions. 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. ``` </details> ########## superset/embedded_chart/view.py: ########## @@ -0,0 +1,72 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from typing import Callable + +from flask import abort, current_app +from flask_appbuilder import expose +from flask_login import AnonymousUserMixin, login_user + +from superset import event_logger, is_feature_enabled +from superset.superset_typing import FlaskResponse +from superset.utils import json +from superset.views.base import BaseSupersetView, common_bootstrap_payload + + +class EmbeddedChartView(BaseSupersetView): + """Server-side rendering for embedded chart pages.""" + + route_base = "/embedded/chart" + + @expose("/") + @event_logger.log_this_with_extra_payload + def embedded_chart( + self, + add_extra_log_payload: Callable[..., None] = lambda **kwargs: None, + ) -> FlaskResponse: + """ + Server side rendering for the embedded chart page. + Expects ?permalink_key=xxx query parameter. + """ + if not is_feature_enabled("EMBEDDABLE_CHARTS_MCP"): + abort(404) + + # Log in as anonymous user for page rendering + # This view needs to be visible to all users, + # and building the page fails if g.user and/or ctx.user aren't present. + login_user(AnonymousUserMixin(), force=True) + + add_extra_log_payload( + embedded_type="chart", + ) + + bootstrap_data = { + "config": { + "GUEST_TOKEN_HEADER_NAME": current_app.config[ + "GUEST_TOKEN_HEADER_NAME" + ], Review Comment: **Suggestion:** Runtime error (KeyError): accessing `current_app.config["GUEST_TOKEN_HEADER_NAME"]` will raise a KeyError if the configuration key is not present, causing the request to 500. Use .get with a sensible default or validate/abort with a clear error. [possible bug] **Severity Level:** Critical 🚨 ```suggestion "GUEST_TOKEN_HEADER_NAME": current_app.config.get( "GUEST_TOKEN_HEADER_NAME", "" ), ``` <details> <summary><b>Why it matters? ⭐ </b></summary> This is a legitimate runtime risk: current_app.config[...] will raise KeyError if the key isn't set and crash the request. Using .get with a sensible default (or explicitly validating and aborting) avoids a 500. The suggested improved_code is a small, safe change that prevents crashes; it's relevant to the diff and fixes a real potential production bug. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/embedded_chart/view.py **Line:** 58:60 **Comment:** *Possible Bug: Runtime error (KeyError): accessing `current_app.config["GUEST_TOKEN_HEADER_NAME"]` will raise a KeyError if the configuration key is not present, causing the request to 500. Use .get with a sensible default or validate/abort with a clear error. 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. ``` </details> ########## superset/security/manager.py: ########## @@ -2834,6 +2838,15 @@ def validate_guest_token_resources(resources: GuestTokenResources) -> None: embedded = EmbeddedDashboardDAO.find_by_id(str(resource["id"])) if not embedded: raise EmbeddedDashboardNotFoundError() + elif resource["type"] == GuestTokenResourceType.CHART_PERMALINK.value: + # Validate that the chart permalink exists + permalink_key = str(resource["id"]) + try: + permalink_value = GetExplorePermalinkCommand(permalink_key).run() + if not permalink_value: + raise EmbeddedChartPermalinkNotFoundError() + except Exception as ex: + raise EmbeddedChartPermalinkNotFoundError() from ex Review Comment: **Suggestion:** Re-raising EmbeddedChartPermalinkNotFoundError using "from ex" attaches the original exception as the cause, which can leak internal error details into logs or responses; if the intention is to normalize to a not-found error, re-raise without chaining or re-raise the original unexpected exception. [security] **Severity Level:** Critical 🚨 ```suggestion except Exception: # Normalize missing permalink errors but avoid chaining unexpected internals raise EmbeddedChartPermalinkNotFoundError() ``` <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/security/manager.py **Line:** 2848:2849 **Comment:** *Security: Re-raising EmbeddedChartPermalinkNotFoundError using "from ex" attaches the original exception as the cause, which can leak internal error details into logs or responses; if the intention is to normalize to a not-found error, re-raise without chaining or re-raise the original unexpected exception. 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. ``` </details> ########## superset/mcp_service/embedded_chart/tool/__init__.py: ########## @@ -0,0 +1,19 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from .get_embeddable_chart import get_embeddable_chart + +__all__ = ["get_embeddable_chart"] Review Comment: **Suggestion:** Eager top-level import can create a circular import if `get_embeddable_chart` imports from the `tool` package (or other sibling modules); this will raise ImportError at runtime when importing the package. Replace the direct import with a lazy loader (module-level __getattr__) so the function is imported only when first accessed, avoiding circular import problems. [possible bug] **Severity Level:** Critical 🚨 ```suggestion from importlib import import_module __all__ = ["get_embeddable_chart"] def __getattr__(name): if name == "get_embeddable_chart": mod = import_module(".get_embeddable_chart", __package__) return getattr(mod, "get_embeddable_chart") raise AttributeError(f"module {__name__} has no attribute {name}") ``` <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/embedded_chart/tool/__init__.py **Line:** 17:19 **Comment:** *Possible Bug: Eager top-level import can create a circular import if `get_embeddable_chart` imports from the `tool` package (or other sibling modules); this will raise ImportError at runtime when importing the package. Replace the direct import with a lazy loader (module-level __getattr__) so the function is imported only when first accessed, avoiding circular import problems. 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. ``` </details> ########## superset/config.py: ########## @@ -559,6 +559,8 @@ class D3TimeFormat(TypedDict, total=False): # This feature flag is stil in beta and is not recommended for production use. "GLOBAL_ASYNC_QUERIES": False, "EMBEDDED_SUPERSET": False, + # Enables MCP tool for creating embeddable chart iframes with guest tokens + "EMBEDDABLE_CHARTS_MCP": False, Review Comment: **Suggestion:** Logic/consistency issue: there's already a feature flag `EMBEDDABLE_CHARTS` set to True earlier in the file; adding `EMBEDDABLE_CHARTS_MCP` defaulting to False creates an ambiguous/conflicting default behavior (one embedding-related flag True, the MCP-specific flag False) which can lead to surprising runtime behavior if code paths check the wrong flag. Align the default or explicitly document the relationship; the minimal fix is to set the MCP flag to the same default as the generic embedding flag. [logic error] **Severity Level:** Minor ⚠️ ```suggestion "EMBEDDABLE_CHARTS_MCP": True, ``` <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/config.py **Line:** 563:563 **Comment:** *Logic Error: Logic/consistency issue: there's already a feature flag `EMBEDDABLE_CHARTS` set to True earlier in the file; adding `EMBEDDABLE_CHARTS_MCP` defaulting to False creates an ambiguous/conflicting default behavior (one embedding-related flag True, the MCP-specific flag False) which can lead to surprising runtime behavior if code paths check the wrong flag. Align the default or explicitly document the relationship; the minimal fix is to set the MCP flag to the same default as the generic embedding flag. 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. ``` </details> ########## superset-frontend/src/embeddedChart/index.tsx: ########## @@ -0,0 +1,303 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import 'src/public-path'; + +import { useState, useEffect, useCallback } from 'react'; +import ReactDOM from 'react-dom'; +import { makeApi, t, logging, QueryFormData } from '@superset-ui/core'; +import { StatefulChart } from '@superset-ui/core'; +import Switchboard from '@superset-ui/switchboard'; +import getBootstrapData, { applicationRoot } from 'src/utils/getBootstrapData'; +import setupClient from 'src/setup/setupClient'; +import setupPlugins from 'src/setup/setupPlugins'; +import { store, USER_LOADED } from 'src/views/store'; +import { Loading } from '@superset-ui/core/components'; +import { ErrorBoundary, DynamicPluginProvider } from 'src/components'; +import { addDangerToast } from 'src/components/MessageToasts/actions'; +import ToastContainer from 'src/components/MessageToasts/ToastContainer'; +import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { SupersetThemeProvider } from 'src/theme/ThemeProvider'; +import { ThemeController } from 'src/theme/ThemeController'; +import type { ThemeStorage } from '@apache-superset/core/ui'; +import { Provider as ReduxProvider } from 'react-redux'; + +setupPlugins(); + +const debugMode = process.env.WEBPACK_MODE === 'development'; +const bootstrapData = getBootstrapData(); + +function log(...info: unknown[]) { + if (debugMode) logging.debug(`[superset-embedded-chart]`, ...info); +} + +/** + * In-memory implementation of ThemeStorage interface for embedded contexts. + */ +class ThemeMemoryStorageAdapter implements ThemeStorage { + private storage = new Map<string, string>(); + + getItem(key: string): string | null { + return this.storage.get(key) || null; + } + + setItem(key: string, value: string): void { + this.storage.set(key, value); + } + + removeItem(key: string): void { + this.storage.delete(key); + } +} + +const themeController = new ThemeController({ + storage: new ThemeMemoryStorageAdapter(), +}); + +interface PermalinkState { + formData: QueryFormData; +} + +const appMountPoint = document.getElementById('app')!; +const MESSAGE_TYPE = '__embedded_comms__'; + +function showFailureMessage(message: string) { + appMountPoint.innerHTML = `<div style="display: flex; align-items: center; justify-content: center; height: 100vh; font-family: sans-serif; color: #666;">${message}</div>`; +} + +if (!window.parent || window.parent === window) { + showFailureMessage( + t( + 'This page is intended to be embedded in an iframe, but it looks like that is not the case.', + ), + ); +} + +let displayedUnauthorizedToast = false; + +/** + * Handle unauthorized errors from the API. + */ +function guestUnauthorizedHandler() { + if (displayedUnauthorizedToast) return; + displayedUnauthorizedToast = true; + store.dispatch( + addDangerToast( + t( + 'This session has encountered an interruption. The embedded chart may not load correctly.', + ), + { + duration: -1, + noDuplicate: true, + }, + ), + ); +} + +/** + * Configures SupersetClient with the correct settings for the embedded chart page. + */ +function setupGuestClient(guestToken: string) { + setupClient({ + appRoot: applicationRoot(), + guestToken, + guestTokenHeaderName: bootstrapData.config?.GUEST_TOKEN_HEADER_NAME, + unauthorizedHandler: guestUnauthorizedHandler, + }); +} + +function validateMessageEvent(event: MessageEvent) { + if (typeof event.data !== 'object' || event.data.type !== MESSAGE_TYPE) { + throw new Error(`Message type does not match type used for embedded comms`); + } +} + +/** + * Embedded Chart component that fetches formData from permalink and renders StatefulChart. + */ +function EmbeddedChartApp() { + const [formData, setFormData] = useState<QueryFormData | null>(null); + const [error, setError] = useState<string | null>(null); + const [loading, setLoading] = useState(true); + + const fetchPermalinkData = useCallback(async () => { + const urlParams = new URLSearchParams(window.location.search); + const permalinkKey = urlParams.get('permalink_key'); + + if (!permalinkKey) { + setError(t('Missing permalink_key parameter')); + setLoading(false); + return; + } + + try { + const getPermalinkData = makeApi<void, { state: PermalinkState }>({ + method: 'GET', + endpoint: `/api/v1/embedded_chart/${permalinkKey}`, + }); + + const response = await getPermalinkData(); + const { state } = response; + + if (!state?.formData) { + setError(t('Invalid permalink data: missing formData')); + setLoading(false); + return; + } + + log('Loaded formData from permalink:', state.formData); + setFormData(state.formData); + setLoading(false); + } catch (err) { + logging.error('Failed to load permalink data:', err); + setError( + t('Failed to load chart data. The permalink may have expired.'), + ); + setLoading(false); + } + }, []); + + useEffect(() => { + fetchPermalinkData(); + }, [fetchPermalinkData]); + + if (loading) { + return ( + <div + style={{ width: '100%', height: '100vh', position: 'relative' }} + > + <Loading position="floating" /> + </div> + ); + } + + if (error) { + return ( + <div + style={{ + display: 'flex', + alignItems: 'center', + justifyContent: 'center', + height: '100vh', + fontFamily: 'sans-serif', + color: '#666', + padding: '20px', + textAlign: 'center', + }} + > + {error} + </div> + ); + } + + if (!formData) { + return null; + } + + return ( + <div style={{ width: '100%', height: '100vh' }}> + <StatefulChart + formData={formData} + width="100%" + height="100%" + showLoading + enableNoResults + /> + </div> + ); +} + +/** + * Context providers wrapper for the embedded chart. + */ +function EmbeddedChartWithProviders() { + return ( + <SupersetThemeProvider themeController={themeController}> + <ReduxProvider store={store}> + <DynamicPluginProvider> + <ErrorBoundary> + <EmbeddedChartApp /> + </ErrorBoundary> + <ToastContainer position="top" /> + </DynamicPluginProvider> + </ReduxProvider> + </SupersetThemeProvider> + ); +} + +function start() { + const getMeWithRole = makeApi<void, { result: UserWithPermissionsAndRoles }>({ + method: 'GET', + endpoint: '/api/v1/me/roles/', + }); + return getMeWithRole().then( + ({ result }) => { + bootstrapData.user = result; + store.dispatch({ + type: USER_LOADED, + user: result, + }); + ReactDOM.render(<EmbeddedChartWithProviders />, appMountPoint); + }, + err => { + logging.error(err); + showFailureMessage( + t( + 'Something went wrong with embedded authentication. Check the dev console for details.', + ), + ); + }, + ); +} + +window.addEventListener('message', function embeddedChartInitializer(event) { + try { + validateMessageEvent(event); + } catch (err) { + log('ignoring message unrelated to embedded comms', err, event); + return; + } + + const port = event.ports?.[0]; + if (event.data.handshake === 'port transfer' && port) { + log('message port received', event); + + Switchboard.init({ + port, + name: 'superset-embedded-chart', + debug: debugMode, + }); + + let started = false; + + Switchboard.defineMethod( + 'guestToken', + ({ guestToken }: { guestToken: string }) => { + setupGuestClient(guestToken); + if (!started) { + start(); + started = true; Review Comment: **Suggestion:** The `started` flag is set immediately after calling `start()`; if `start()` fails, `started` remains true and the app will not retry on later `guestToken` messages. Set `started` only after `start()` completes successfully (or reset it on failure). [race condition] **Severity Level:** Minor ⚠️ ```suggestion // call start and set started to true only after a successful start start() .then(() => { started = true; }) .catch(err => { logging.error('Failed to start after receiving guestToken', err); }); ``` <details> <summary><b>Why it matters? ⭐ </b></summary> start() returns a promise; setting started synchronously means a failure leaves started true and blocks subsequent retries. Setting started only after start resolves (or resetting on failure) fixes a real initialization/retry bug. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/embeddedChart/index.tsx **Line:** 294:294 **Comment:** *Race Condition: The `started` flag is set immediately after calling `start()`; if `start()` fails, `started` remains true and the app will not retry on later `guestToken` messages. Set `started` only after `start()` completes successfully (or reset it on failure). 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. ``` </details> ########## superset/embedded_chart/api.py: ########## @@ -0,0 +1,119 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +import logging + +from flask import Response +from flask_appbuilder.api import expose, safe +from flask_appbuilder.hooks import before_request + +from superset import is_feature_enabled +from superset.commands.explore.permalink.get import GetExplorePermalinkCommand +from superset.embedded_chart.exceptions import EmbeddedChartPermalinkNotFoundError +from superset.extensions import event_logger +from superset.views.base_api import BaseSupersetApi, statsd_metrics + +logger = logging.getLogger(__name__) + + +class EmbeddedChartRestApi(BaseSupersetApi): + """REST API for embedded chart data retrieval.""" + + resource_name = "embedded_chart" + allow_browser_login = True + openapi_spec_tag = "Embedded Chart" + + @before_request + def ensure_feature_enabled(self) -> Response | None: + if not is_feature_enabled("EMBEDDABLE_CHARTS_MCP"): + return self.response_404() + return None + + @expose("/<permalink_key>", methods=("GET",)) + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get", + log_to_statsd=False, + ) + def get(self, permalink_key: str) -> Response: + """Get chart form_data from permalink key. + --- + get: + summary: Get embedded chart configuration + description: >- + Retrieves the form_data for rendering an embedded chart. + This endpoint is used by the embedded chart iframe to load + the chart configuration. + parameters: + - in: path + schema: + type: string + name: permalink_key + description: The chart permalink key + required: true + responses: + 200: + description: Chart form_data + content: + application/json: + schema: + type: object + properties: + result: + type: object + properties: + form_data: + type: object + description: The chart configuration form_data + datasource_id: + type: integer + description: The datasource ID + datasource_type: + type: string + description: The datasource type + chart_id: + type: integer + description: Optional chart ID if based on saved chart + 401: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + """ + try: + permalink_value = GetExplorePermalinkCommand(permalink_key).run() + if not permalink_value: + raise EmbeddedChartPermalinkNotFoundError() + + state = permalink_value.get("state", {}) + form_data = state.get("formData", {}) + + return self.response( + 200, + result={ + "form_data": form_data, + "datasource_id": permalink_value.get("datasourceId"), + "datasource_type": permalink_value.get("datasourceType"), + "chart_id": permalink_value.get("chartId"), + }, + ) + except EmbeddedChartPermalinkNotFoundError: + return self.response_404() + except Exception as ex: + logger.exception("Error fetching embedded chart: %s", ex) + return self.response_500(message=str(ex)) Review Comment: **Suggestion:** Security: the code returns the raw exception message to API clients in a 500 response (message=str(ex)), which can leak internal details and sensitive information. Return a generic 500 response and log the exception server-side instead. [security] **Severity Level:** Critical 🚨 ```suggestion except Exception: logger.exception("Error fetching embedded chart", exc_info=True) return self.response_500() ``` <details> <summary><b>Why it matters? ⭐ </b></summary> This is a valid security concern: returning str(ex) in a 500 response can leak internal state / sensitive details. Replacing it with a generic response and logging the full exception server-side fixes a real information-leak. It's low-risk and localized to this handler. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/embedded_chart/api.py **Line:** 115:119 **Comment:** *Security: Security: the code returns the raw exception message to API clients in a 500 response (message=str(ex)), which can leak internal details and sensitive information. Return a generic 500 response and log the exception server-side instead. 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. ``` </details> ########## superset-frontend/webpack.config.js: ########## @@ -300,6 +300,7 @@ const config = { menu: addPreamble('src/views/menu.tsx'), spa: addPreamble('/src/views/index.tsx'), embedded: addPreamble('/src/embedded/index.tsx'), + embeddedChart: addPreamble('/src/embeddedChart/index.tsx'), Review Comment: **Suggestion:** Logic bug: the entry path is passed with a leading slash to `addPreamble`, which calls `path.join(APP_DIR, entry)`. When `entry` starts with '/', `path.join` treats it as an absolute path and discards `APP_DIR`, producing an incorrect path; remove the leading slash so the path is joined relative to `APP_DIR`. [logic error] **Severity Level:** Minor ⚠️ ```suggestion embeddedChart: addPreamble('src/embeddedChart/index.tsx'), ``` <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/webpack.config.js **Line:** 303:303 **Comment:** *Logic Error: Logic bug: the entry path is passed with a leading slash to `addPreamble`, which calls `path.join(APP_DIR, entry)`. When `entry` starts with '/', `path.join` treats it as an absolute path and discards `APP_DIR`, producing an incorrect path; remove the leading slash so the path is joined relative to `APP_DIR`. 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. ``` </details> ########## superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py: ########## @@ -0,0 +1,228 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +MCP tool: get_embeddable_chart + +Creates an embeddable chart iframe with guest token authentication. +This enables AI agents to generate charts that can be displayed in +external applications via iframe. +""" + +import logging +from datetime import datetime, timedelta, timezone + +from fastmcp import Context +from flask import g +from superset_core.mcp import tool + +from superset import is_feature_enabled +from superset.mcp_service.auth import has_dataset_access +from superset.mcp_service.embedded_chart.schemas import ( + GetEmbeddableChartRequest, + GetEmbeddableChartResponse, +) +from superset.mcp_service.utils.schema_utils import parse_request +from superset.mcp_service.utils.url_utils import get_superset_base_url + +logger = logging.getLogger(__name__) + + +@tool(tags=["core"]) +@parse_request(GetEmbeddableChartRequest) +async def get_embeddable_chart( + request: GetEmbeddableChartRequest, + ctx: Context, +) -> GetEmbeddableChartResponse: + """Create an embeddable chart iframe URL with guest token authentication. + + This tool creates an ephemeral chart visualization that can be embedded + in external applications via iframe. The chart is configured via form_data + and stored as a permalink with TTL. + + IMPORTANT: + - Requires EMBEDDABLE_CHARTS_MCP feature flag to be enabled + - The iframe_html can be directly embedded in web pages + - Guest token must be passed to the iframe for authentication + - Chart expires after ttl_minutes (default: 60 minutes) + + Example usage: + ```json + { + "datasource_id": 123, + "viz_type": "echarts_timeseries_line", + "form_data": { + "metrics": ["count"], + "groupby": ["category"], + "time_range": "Last 7 days" + }, + "ttl_minutes": 120, + "height": 500 + } + ``` + + Returns iframe_url, guest_token, and ready-to-use iframe_html snippet. + """ + await ctx.info( + f"Creating embeddable chart: datasource_id={request.datasource_id}, " + f"viz_type={request.viz_type}" + ) + + # Check feature flag + if not is_feature_enabled("EMBEDDABLE_CHARTS_MCP"): + await ctx.error("EMBEDDABLE_CHARTS_MCP feature flag is not enabled") + return GetEmbeddableChartResponse( + success=False, + error="EMBEDDABLE_CHARTS_MCP feature flag is not enabled", + ) + + try: + # Import here to avoid circular imports + from superset.commands.explore.permalink.create import ( + CreateExplorePermalinkCommand, + ) + from superset.daos.dataset import DatasetDAO + from superset.extensions import security_manager + from superset.security.guest_token import GuestTokenResourceType + + # Resolve dataset + dataset = None + if isinstance(request.datasource_id, int) or ( + isinstance(request.datasource_id, str) and request.datasource_id.isdigit() + ): + dataset_id = ( + int(request.datasource_id) + if isinstance(request.datasource_id, str) + else request.datasource_id + ) + dataset = DatasetDAO.find_by_id(dataset_id) + else: + # Try UUID lookup + dataset = DatasetDAO.find_by_id(request.datasource_id, id_column="uuid") + + if not dataset: + await ctx.error(f"Dataset not found: {request.datasource_id}") + return GetEmbeddableChartResponse( + success=False, + error=f"Dataset not found: {request.datasource_id}", + ) + + # Check access + if not has_dataset_access(dataset): + await ctx.error("Access denied to dataset") + return GetEmbeddableChartResponse( + success=False, + error="Access denied to dataset", + ) + + # Build complete form_data + form_data = { + **request.form_data, + "viz_type": request.viz_type, + "datasource": f"{dataset.id}__table", + } + + # Apply overrides if provided + if request.form_data_overrides: + form_data.update(request.form_data_overrides) + + # Create permalink + state = {"formData": form_data} + permalink_key = CreateExplorePermalinkCommand(state).run() + + await ctx.debug(f"Created permalink: {permalink_key}") + + # Calculate expiration + expires_at = datetime.now(timezone.utc) + timedelta(minutes=request.ttl_minutes) Review Comment: **Suggestion:** TypeError when `ttl_minutes` is None: `timedelta(minutes=request.ttl_minutes)` will raise if `request.ttl_minutes` is None; ensure a default is used (e.g., 60) before constructing the timedelta. [type error] **Severity Level:** Minor ⚠️ ```suggestion # Calculate expiration (use a safe default if ttl_minutes is missing) ttl_minutes = ( request.ttl_minutes if getattr(request, "ttl_minutes", None) is not None else 60 ) expires_at = datetime.now(timezone.utc) + timedelta(minutes=ttl_minutes) ``` <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py **Line:** 148:149 **Comment:** *Type Error: TypeError when `ttl_minutes` is None: `timedelta(minutes=request.ttl_minutes)` will raise if `request.ttl_minutes` is None; ensure a default is used (e.g., 60) before constructing the timedelta. 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. ``` </details> ########## superset/mcp_service/app.py: ########## @@ -324,6 +324,9 @@ def create_mcp_app( get_instance_info, health_check, ) +from superset.mcp_service.embedded_chart.tool import ( # noqa: F401, E402 + get_embeddable_chart, +) Review Comment: **Suggestion:** The import is used only for its decorator side-effects (tool registration) and the symbol is otherwise unused; importing the module (not the symbol) makes that intent explicit and avoids suggesting the symbol is referenced in this file. [code quality] **Severity Level:** Minor ⚠️ ```suggestion # Import module for side-effects (registers tools via decorators) — symbol is not used directly here import superset.mcp_service.embedded_chart.tool # noqa: F401 ``` <details> <summary><b>Why it matters? ⭐ </b></summary> This is a good, minimal-quality suggestion: importing the module (rather than a symbol) makes the intent of importing for side-effects explicit and avoids implying the symbol is used in this file. It reduces accidental future removal by other maintainers and is safe here because the symbol isn't referenced elsewhere in this module. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/app.py **Line:** 327:329 **Comment:** *Code Quality: The import is used only for its decorator side-effects (tool registration) and the symbol is otherwise unused; importing the module (not the symbol) makes that intent explicit and avoids suggesting the symbol is referenced in this file. 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. ``` </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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
