codeant-ai-for-open-source[bot] commented on code in PR #36933: URL: https://github.com/apache/superset/pull/36933#discussion_r2695173806
########## superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx: ########## @@ -0,0 +1,291 @@ +/** + * 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 { useCallback, useEffect, useState } from 'react'; +import { logging, makeApi, SupersetApiError, t } from '@superset-ui/core'; +import { styled, css, Alert } from '@apache-superset/core/ui'; +import { + Button, + FormItem, + InfoTooltip, + Input, + Modal, + Loading, + Form, + Space, +} from '@superset-ui/core/components'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import { Typography } from '@superset-ui/core/components/Typography'; +import { ModalTitleWithIcon } from 'src/components/ModalTitleWithIcon'; + +type Props = { + chartId: number; + formData: Record<string, unknown>; + show: boolean; + onHide: () => void; +}; + +type EmbeddedChart = { + uuid: string; + allowed_domains: string[]; + chart_id: number; + changed_on: string; +}; + +type EmbeddedApiPayload = { allowed_domains: string[] }; + +const stringToList = (stringyList: string): string[] => + stringyList.split(/(?:\s|,)+/).filter(x => x); + +const ButtonRow = styled.div` + display: flex; + flex-direction: row; + justify-content: flex-end; +`; + +export const ChartEmbedControls = ({ chartId, onHide }: Props) => { + const { addInfoToast, addDangerToast } = useToasts(); + const [ready, setReady] = useState(true); Review Comment: **Suggestion:** The component initializes the `ready` state to true, so the UI renders as if data is loaded before the GET request completes; this causes a flash of the main UI and bypasses the intended loading indicator. Initialize `ready` to false so the Loading spinner is shown until the fetch finishes. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Embed Chart modal flashes uninitialized content. - ⚠️ Visual flicker when opening Embed Chart modal. ``` </details> ```suggestion const [ready, setReady] = useState(false); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the Dashboard UI and click the chart "Embed chart" action that mounts the EmbeddedChartModal component (UI flow described in PR). The modal renders the ChartEmbedControls component defined at `superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx:61`. 2. On the initial render of ChartEmbedControls (component start at line 61), the code at `index.tsx:63` initializes ready with `useState(true)`. Because ready is true, the component proceeds to render the main UI instead of the Loading indicator. 3. Immediately after mount the useEffect at `index.tsx:129` runs and calls `setReady(false)` to start the GET request for `/api/v1/chart/${chartId}/embedded` (see lines 129-134). This causes a visible flash: the main UI is shown briefly, then the component re-renders into the Loading state (`if (!ready) { return <Loading />; }` at lines 151-153). 4. Expected behavior: Loading should show until the GET completes. The root cause is the initial value at `index.tsx:63`. Initializing ready to false prevents the UI flash. If you believe this pattern is intentional, note that the effect already sets ready false on mount, but that still allows an initial render with ready=true and causes the flash. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx **Line:** 63:63 **Comment:** *Logic Error: The component initializes the `ready` state to true, so the UI renders as if data is loaded before the GET request completes; this causes a flash of the main UI and bypasses the intended loading indicator. Initialize `ready` to false so the Loading spinner is shown until the fetch finishes. 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,338 @@ +/** + * 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 } from 'react'; +import ReactDOM from 'react-dom'; +import { + makeApi, + t, + logging, + QueryFormData, + 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;">${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) { + const { data } = event; + if (data == null || typeof data !== 'object' || data.type !== MESSAGE_TYPE) { Review Comment: **Suggestion:** Logic/compatibility bug: `validateMessageEvent` currently rejects any message that does not have `data.type === MESSAGE_TYPE`, which will also reject legitimate direct guest-token or handshake messages that omit `type`. As a result, valid messages carrying `guestToken` or `handshake` may be ignored. Relax validation to allow messages that carry `guestToken` or `handshake` in addition to the typed messages. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Embed auth initialization fails; guest token ignored. - ❌ Embedded charts fail to load without handshake. ``` </details> ```suggestion // Accept messages that explicitly use our MESSAGE_TYPE, or messages that // carry a guestToken or handshake for backward compatibility. if ( data == null || typeof data !== 'object' || (data.type !== MESSAGE_TYPE && !('guestToken' in data) && !('handshake' in data)) ) { ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Host an iframe pointing to the embeddable page (superset-frontend/src/embeddedChart/index.tsx). 2. From the parent page, send a postMessage to the iframe using window.postMessage({ guestToken: 'abc' }, targetOrigin) without a `type` field. 3. The embedded page receives the message and the listener at line 287 (window.addEventListener) immediately calls validateMessageEvent at lines 129-134. 4. validateMessageEvent enforces `data.type === MESSAGE_TYPE` and throws because `type` is absent; the listener's try/catch (lines 287-293) catches this and logs "ignoring message unrelated to embedded comms" and returns, so the guestToken handling branches (lines 296-307 / Switchboard flow at 310+) never run. 5. Observed outcome: guest token is ignored and the embedded app does not initialize authentication or render charts. Note: This is a concrete failure when parent code posts guestToken without adding a `type` property (a likely integration pattern). ``` </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:** 131:131 **Comment:** *Logic Error: Logic/compatibility bug: `validateMessageEvent` currently rejects any message that does not have `data.type === MESSAGE_TYPE`, which will also reject legitimate direct guest-token or handshake messages that omit `type`. As a result, valid messages carrying `guestToken` or `handshake` may be ignored. Relax validation to allow messages that carry `guestToken` or `handshake` in addition to the typed messages. 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,304 @@ +# 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 datetime import datetime, timedelta, timezone +from typing import Any + +from flask import g, request, Response +from flask_appbuilder.api import expose, protect, safe + +from superset.commands.explore.permalink.create import CreateExplorePermalinkCommand +from superset.daos.key_value import KeyValueDAO +from superset.embedded_chart.exceptions import ( + EmbeddedChartAccessDeniedError, + EmbeddedChartPermalinkNotFoundError, +) +from superset.explore.permalink.schemas import ExplorePermalinkSchema +from superset.extensions import event_logger, security_manager +from superset.key_value.shared_entries import get_permalink_salt +from superset.key_value.types import ( + KeyValueResource, + MarshmallowKeyValueCodec, + SharedKey, +) +from superset.key_value.utils import decode_permalink_id +from superset.security.guest_token import ( + GuestTokenResource, + GuestTokenResourceType, + GuestTokenRlsRule, + GuestTokenUser, + GuestUser, +) +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" + + def _validate_guest_token_access(self, permalink_key: str) -> bool: + """ + Validate that the guest token grants access to this permalink. + + Guest tokens contain a list of resources the user can access. + For embedded charts, we check that the permalink_key is in that list. + """ + user = g.user + if not isinstance(user, GuestUser): + return False + + for resource in user.resources: + if ( + resource.get("type") == GuestTokenResourceType.CHART_PERMALINK.value + and str(resource.get("id")) == permalink_key + ): + return True + return False + + def _get_permalink_value(self, permalink_key: str) -> dict[str, Any] | None: + """ + Get permalink value without access checks. + + For embedded charts, access is controlled via guest token validation, + so we skip the normal dataset/chart access checks. + """ + # Use the same salt, resource, and codec as the explore permalink command + salt = get_permalink_salt(SharedKey.EXPLORE_PERMALINK_SALT) + codec = MarshmallowKeyValueCodec(ExplorePermalinkSchema()) + key = decode_permalink_id(permalink_key, salt=salt) + return KeyValueDAO.get_value( + KeyValueResource.EXPLORE_PERMALINK, + key, + codec, + ) + + @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 permalink state + content: + application/json: + schema: + type: object + properties: + state: + type: object + properties: + formData: + type: object + description: The chart configuration formData + allowedDomains: + type: array + items: + type: string + description: Domains allowed to embed this chart + 401: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + """ + try: + # Validate guest token grants access to this permalink + if not self._validate_guest_token_access(permalink_key): + raise EmbeddedChartAccessDeniedError() + + # Get permalink value without access checks (guest token already validated) + permalink_value = self._get_permalink_value(permalink_key) + if not permalink_value: + raise EmbeddedChartPermalinkNotFoundError() + + # Return state in the format expected by the frontend: + # { state: { formData: {...}, allowedDomains: [...] } } + state = permalink_value.get("state", {}) + + return self.response( + 200, + state=state, + ) + except EmbeddedChartAccessDeniedError: + return self.response_401() + except EmbeddedChartPermalinkNotFoundError: + return self.response_404() + except Exception: + logger.exception("Error fetching embedded chart") + return self.response_500() + + @expose("/", methods=("POST",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.post", + log_to_statsd=False, + ) + def post(self) -> Response: + """Create an embeddable chart with guest token. + --- + post: + summary: Create embeddable chart + description: >- + Creates an embeddable chart configuration with a guest token. + The returned iframe_url and guest_token can be used to embed + the chart in external applications. + requestBody: + required: true + content: + application/json: + schema: + type: object + required: + - form_data + properties: + form_data: + type: object + description: Chart form_data configuration + allowed_domains: + type: array + items: + type: string + description: Domains allowed to embed this chart + ttl_minutes: + type: integer + default: 60 + description: Time-to-live for the embed in minutes + responses: + 200: + description: Embeddable chart created + content: + application/json: + schema: + type: object + properties: + iframe_url: + type: string + description: URL to use in iframe src + guest_token: + type: string + description: Guest token for authentication + permalink_key: + type: string + description: Permalink key for the chart + expires_at: + type: string + format: date-time + description: When the embed expires + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 500: + $ref: '#/components/responses/500' + """ + try: + body = request.json or {} + form_data = body.get("form_data", {}) + allowed_domains: list[str] = body.get("allowed_domains", []) + ttl_minutes: int = body.get("ttl_minutes", 60) + + if not form_data: + return self.response_400(message="form_data is required") + + # Validate required form_data structure + if not form_data.get("datasource"): + return self.response_400( + message="form_data must include 'datasource' field" + ) + + # Create permalink with the form_data + state = { + "formData": form_data, + "allowedDomains": allowed_domains, + } + permalink_key = CreateExplorePermalinkCommand(state).run() + + # Calculate expiration + expires_at = datetime.now(timezone.utc) + timedelta(minutes=ttl_minutes) + + # Generate guest token + username = g.user.username if hasattr(g, "user") and g.user else "anonymous" + guest_user: GuestTokenUser = { + "username": f"embed_{username}", + "first_name": "Embed", + "last_name": "User", + } + + resources: list[GuestTokenResource] = [ + { Review Comment: **Suggestion:** The resource `type` is passed as an enum object when creating the guest token resources, but later code (and token payloads) expect the string value; this can cause a mismatch during serialization or when validating the guest token (access will be denied because the types won't match). Change the resource `type` to use the enum's `.value` so a string is stored. [type error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ POST /api/v1/embedded_chart/ creation fails. - ❌ Generated guest token rejected by access checks. - ⚠️ UI \"Embed chart\" flow returns errors. - ⚠️ MCP tool embed creation may error. ``` </details> ```suggestion "type": GuestTokenResourceType.CHART_PERMALINK.value, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Call the embeddable-chart creation API: POST /api/v1/embedded_chart/ which maps to EmbeddedChartRestApi.post in superset/embedded_chart/api.py (post method defined around lines 177-301 in the PR). 2. Execution reaches the guest-token creation block at superset/embedded_chart/api.py:lines 261-283 (guest_user and resources construction), specifically the resources dict at lines 267-272 is built with GuestTokenResourceType.CHART_PERMALINK (enum instance). 3. The code calls security_manager.create_guest_access_token(...) at superset/embedded_chart/api.py:lines 276-281. The guest token creation serializes the resources payload into a JWT JSON body. 4. Because the resource \"type\" contains an Enum object (GuestTokenResourceType.CHART_PERMALINK) rather than a primitive string, JSON serialization or downstream validation either (a) raises a serialization error or (b) produces a payload that does not match the string-based checks in _validate_guest_token_access (superset/embedded_chart/api.py:_validate_guest_token_access, lines ~58-75). The POST request therefore either returns HTTP 500 (serialization error) or issues a guest token that will fail access checks when used to fetch the embed (leading to 401). ``` </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:** 269:269 **Comment:** *Type Error: The resource `type` is passed as an enum object when creating the guest token resources, but later code (and token payloads) expect the string value; this can cause a mismatch during serialization or when validating the guest token (access will be denied because the types won't match). Change the resource `type` to use the enum's `.value` so a string is stored. 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/dashboard/components/SliceHeaderControls/index.tsx: ########## @@ -603,6 +618,13 @@ const SliceHeaderControls = ( dataset={datasetWithVerboseMap} /> + <EmbeddedChartModal + chartId={slice.slice_id} + formData={props.formData} + show={embedModalIsOpen} + onHide={() => setEmbedModalIsOpen(false)} Review Comment: **Suggestion:** Prop-name mismatch on the rendered modal: other modals in this file use `showModal` / `onHideModal` naming (e.g. `DrillDetailModal`), but the new `EmbeddedChartModal` is rendered with `show` and `onHide`. If `EmbeddedChartModal` follows the same API as other dashboard modals, passing `show`/`onHide` will do nothing and the modal won't open/close as expected. Change the prop names to match the modal API used elsewhere (use `showModal` and `onHideModal`) so the component receives the visible state and close callback. [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Dashboard "Embed chart" modal fails to open. - ⚠️ Dashboard embed UI unusable for sharable charts. ``` </details> ```suggestion showModal={embedModalIsOpen} onHideModal={() => setEmbedModalIsOpen(false)} ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open a dashboard and open a chart's dropdown. The dropdown Menu is rendered with onClick={handleMenuClick} in this file (SliceHeaderControls). The handleMenuClick switch includes the EmbedChart branch at src/dashboard/components/SliceHeaderControls/index.tsx lines ~292-299 where it calls setEmbedModalIsOpen(true). 2. Select the "Embed chart" menu entry (MenuKeys.EmbedChart). The case MenuKeys.EmbedChart in handleMenuClick at index.tsx: ~294-298 executes setEmbedModalIsOpen(true). 3. The component tree includes <EmbeddedChartModal ... /> rendered at index.tsx:621-626. That component instance receives props show and onHide (lines 623-625), not the showModal/onHideModal shape used by other modals in this file (for example DrillDetailModal in the same component uses showModal and onHideModal). 4. Because EmbeddedChartModal is passed show/onHide instead of showModal/onHideModal, the modal's visibility API is not driven by the local state. Result: clicking "Embed chart" sets embedModalIsOpen to true (verified at handleMenuClick: index.tsx:~294-298) but the EmbeddedChartModal does not open. This reproduces the broken UI where the embed modal never appears when the menu item is used. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx **Line:** 624:625 **Comment:** *Possible Bug: Prop-name mismatch on the rendered modal: other modals in this file use `showModal` / `onHideModal` naming (e.g. `DrillDetailModal`), but the new `EmbeddedChartModal` is rendered with `show` and `onHide`. If `EmbeddedChartModal` follows the same API as other dashboard modals, passing `show`/`onHide` will do nothing and the modal won't open/close as expected. Change the prop names to match the modal API used elsewhere (use `showModal` and `onHideModal`) so the component receives the visible state and close callback. 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,304 @@ +# 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 datetime import datetime, timedelta, timezone +from typing import Any + +from flask import g, request, Response +from flask_appbuilder.api import expose, protect, safe + +from superset.commands.explore.permalink.create import CreateExplorePermalinkCommand +from superset.daos.key_value import KeyValueDAO +from superset.embedded_chart.exceptions import ( + EmbeddedChartAccessDeniedError, + EmbeddedChartPermalinkNotFoundError, +) +from superset.explore.permalink.schemas import ExplorePermalinkSchema +from superset.extensions import event_logger, security_manager +from superset.key_value.shared_entries import get_permalink_salt +from superset.key_value.types import ( + KeyValueResource, + MarshmallowKeyValueCodec, + SharedKey, +) +from superset.key_value.utils import decode_permalink_id +from superset.security.guest_token import ( + GuestTokenResource, + GuestTokenResourceType, + GuestTokenRlsRule, + GuestTokenUser, + GuestUser, +) +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" + + def _validate_guest_token_access(self, permalink_key: str) -> bool: + """ + Validate that the guest token grants access to this permalink. + + Guest tokens contain a list of resources the user can access. + For embedded charts, we check that the permalink_key is in that list. + """ + user = g.user + if not isinstance(user, GuestUser): + return False + + for resource in user.resources: + if ( + resource.get("type") == GuestTokenResourceType.CHART_PERMALINK.value + and str(resource.get("id")) == permalink_key + ): + return True + return False + + def _get_permalink_value(self, permalink_key: str) -> dict[str, Any] | None: + """ + Get permalink value without access checks. + + For embedded charts, access is controlled via guest token validation, + so we skip the normal dataset/chart access checks. + """ + # Use the same salt, resource, and codec as the explore permalink command + salt = get_permalink_salt(SharedKey.EXPLORE_PERMALINK_SALT) + codec = MarshmallowKeyValueCodec(ExplorePermalinkSchema()) + key = decode_permalink_id(permalink_key, salt=salt) + return KeyValueDAO.get_value( + KeyValueResource.EXPLORE_PERMALINK, + key, + codec, + ) + + @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 permalink state + content: + application/json: + schema: + type: object + properties: + state: + type: object + properties: + formData: + type: object + description: The chart configuration formData + allowedDomains: + type: array + items: + type: string + description: Domains allowed to embed this chart + 401: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + """ + try: + # Validate guest token grants access to this permalink + if not self._validate_guest_token_access(permalink_key): + raise EmbeddedChartAccessDeniedError() + + # Get permalink value without access checks (guest token already validated) + permalink_value = self._get_permalink_value(permalink_key) + if not permalink_value: + raise EmbeddedChartPermalinkNotFoundError() + + # Return state in the format expected by the frontend: + # { state: { formData: {...}, allowedDomains: [...] } } + state = permalink_value.get("state", {}) + + return self.response( + 200, + state=state, + ) + except EmbeddedChartAccessDeniedError: + return self.response_401() + except EmbeddedChartPermalinkNotFoundError: + return self.response_404() + except Exception: + logger.exception("Error fetching embedded chart") + return self.response_500() + + @expose("/", methods=("POST",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.post", + log_to_statsd=False, + ) + def post(self) -> Response: + """Create an embeddable chart with guest token. + --- + post: + summary: Create embeddable chart + description: >- + Creates an embeddable chart configuration with a guest token. + The returned iframe_url and guest_token can be used to embed + the chart in external applications. + requestBody: + required: true + content: + application/json: + schema: + type: object + required: + - form_data + properties: + form_data: + type: object + description: Chart form_data configuration + allowed_domains: + type: array + items: + type: string + description: Domains allowed to embed this chart + ttl_minutes: + type: integer + default: 60 + description: Time-to-live for the embed in minutes + responses: + 200: + description: Embeddable chart created + content: + application/json: + schema: + type: object + properties: + iframe_url: + type: string + description: URL to use in iframe src + guest_token: + type: string + description: Guest token for authentication + permalink_key: + type: string + description: Permalink key for the chart + expires_at: + type: string + format: date-time + description: When the embed expires + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 500: + $ref: '#/components/responses/500' + """ + try: + body = request.json or {} + form_data = body.get("form_data", {}) + allowed_domains: list[str] = body.get("allowed_domains", []) + ttl_minutes: int = body.get("ttl_minutes", 60) + + if not form_data: + return self.response_400(message="form_data is required") + + # Validate required form_data structure + if not form_data.get("datasource"): + return self.response_400( + message="form_data must include 'datasource' field" + ) + + # Create permalink with the form_data + state = { + "formData": form_data, + "allowedDomains": allowed_domains, + } + permalink_key = CreateExplorePermalinkCommand(state).run() + + # Calculate expiration + expires_at = datetime.now(timezone.utc) + timedelta(minutes=ttl_minutes) + + # Generate guest token + username = g.user.username if hasattr(g, "user") and g.user else "anonymous" + guest_user: GuestTokenUser = { + "username": f"embed_{username}", + "first_name": "Embed", + "last_name": "User", + } + + resources: list[GuestTokenResource] = [ + { + "type": GuestTokenResourceType.CHART_PERMALINK, + "id": permalink_key, + } + ] + + rls_rules: list[GuestTokenRlsRule] = [] + + guest_token_result = security_manager.create_guest_access_token( + user=guest_user, + resources=resources, + rls=rls_rules, + ) + + # Handle both bytes (older PyJWT) and string (PyJWT 2.0+) + guest_token = ( + guest_token_result.decode("utf-8") + if isinstance(guest_token_result, bytes) + else guest_token_result + ) + + # Build iframe URL using request host + base_url = request.host_url.rstrip("/") + iframe_url = f"{base_url}/embedded/chart/?permalink_key={permalink_key}" + Review Comment: **Suggestion:** The GET endpoint expects the permalink as a path parameter, but the generated `iframe_url` places the permalink as a query parameter; this mismatch means the iframe URL will not match the API route and the iframe will fail to load the embed. Construct the iframe URL using the path-style route (embed path + permalink) to match the GET route. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Embedded iframe fails to load (404). - ⚠️ Dashboard UI \"Embed chart\" unusable. - ⚠️ Third-party embeds get invalid URLs. ``` </details> ```suggestion iframe_url = f"{base_url}/embedded/chart/{permalink_key}" ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create an embed by calling POST /api/v1/embedded_chart/ (EmbeddedChartRestApi.post in superset/embedded_chart/api.py, method body lines ~177-301). The response includes the iframe_url built at superset/embedded_chart/api.py:lines 291-292. 2. Take the returned iframe_url (example: https://host/embedded/chart/?permalink_key=XYZ) and load it in a browser iframe. 3. Browser requests GET https://host/embedded/chart/ without a path parameter; the GET handler is defined as @expose("/<permalink_key>", methods=("GET",)) in superset/embedded_chart/api.py (get method at lines ~94-166). Because the path parameter is missing, Flask routing does not match the GET endpoint for the permalink and the iframe request yields 404/invalid route instead of returning the chart state. 4. Result: embedded iframe fails to load chart configuration; frontend cannot render embed. The mismatch is reproducible using the API response from step 1 and loading that iframe_url. ``` </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:** 293:293 **Comment:** *Logic Error: The GET endpoint expects the permalink as a path parameter, but the generated `iframe_url` places the permalink as a query parameter; this mismatch means the iframe URL will not match the API route and the iframe will fail to load the embed. Construct the iframe URL using the path-style route (embed path + permalink) to match the GET route. 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/commands/chart/delete.py: ########## @@ -68,3 +69,16 @@ def validate(self) -> None: security_manager.raise_for_ownership(model) except SupersetSecurityException as ex: raise ChartForbiddenError() from ex + + +class DeleteEmbeddedChartCommand(BaseCommand): + def __init__(self, chart: Slice): + self._chart = chart + + @transaction(on_error=partial(on_error, reraise=ChartDeleteEmbeddedFailedError)) + def run(self) -> None: + self.validate() + return EmbeddedChartDAO.delete(self._chart.embedded) + + def validate(self) -> None: + pass Review Comment: **Suggestion:** The `validate` method is a no-op, allowing deletion to proceed without verifying the embedded chart exists, whether there are associated reports, or whether the caller has ownership; implement the same existence, report-association, and ownership checks used in `DeleteChartCommand.validate` to prevent unauthorized or invalid deletes. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Embedded deletion bypasses ownership checks. - ❌ Deletion can proceed despite associated reports. - ❌ TypeError when embedded is missing. - ⚠️ Affects command layer: superset/commands/chart/delete.py. ``` </details> ```suggestion # Ensure chart and embedded object exist if not self._chart or getattr(self._chart, "embedded", None) is None: raise ChartNotFoundError() # Prevent deletion if there are associated ReportSchedules if reports := ReportScheduleDAO.find_by_chart_ids([self._chart.id]): report_names = [report.name for report in reports] raise ChartDeleteFailedReportsExistError( _( "There are associated alerts or reports: %(report_names)s", report_names=",".join(report_names), ) ) # Check ownership try: security_manager.raise_for_ownership(self._chart) except SupersetSecurityException as ex: raise ChartForbiddenError() from ex ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Call DeleteEmbeddedChartCommand.run(): - File: superset/commands/chart/delete.py - run() is defined at lines 78-81 and calls self.validate() then proceeds. - Current validate() at lines 83-84 is a no-op. 2. Create a chart that has no embedded object (embedded is None) and call run(): - Because validate() is a no-op, run() will call EmbeddedChartDAO.delete(None), which then raises TypeError in superset/daos/base.py:423-440 when iterating None. 3. Create a chart that has associated reports and call run(): - The DAO ReportSchedule check exists in DeleteChartCommand.validate (lines 52-71), but not here. ReportScheduleDAO.find_by_chart_ids is implemented at superset/daos/report.py:54-59. - Because validate() is empty, deletion will proceed even if reports exist, bypassing the safeguards that prevent deletion in DeleteChartCommand. 4. Call run() as a non-owner: - DeleteChartCommand.validate enforces ownership via security_manager.raise_for_ownership (superset/commands/chart/delete.py lines 52-71). The empty validate() here omits that check, allowing potential unauthorized deletion if callers invoke this command. Explanation: The no-op validate effectively omits existence, report-association, and ownership checks that are present for regular chart deletion and therefore is a real security/consistency gap. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/commands/chart/delete.py **Line:** 84:84 **Comment:** *Security: The `validate` method is a no-op, allowing deletion to proceed without verifying the embedded chart exists, whether there are associated reports, or whether the caller has ownership; implement the same existence, report-association, and ownership checks used in `DeleteChartCommand.validate` to prevent unauthorized or invalid deletes. 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 the "Embed code" and "Embed chart" options in the Share menu + "EMBEDDABLE_CHARTS": True, Review Comment: **Suggestion:** Security risk: enabling embeddable charts by default exposes the feature while the guest token secret in this repository is the insecure default, which can allow unauthorized embedded access. Disable the feature by default so operators must explicitly opt-in after configuring a strong `GUEST_TOKEN_JWT_SECRET`. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Embeddable chart creation endpoint exposed by default. - ❌ Guest tokens signed with insecure default secret. - ⚠️ Many deployments use shipped defaults unchanged. ``` </details> ```suggestion "EMBEDDABLE_CHARTS": False, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start Superset with the repository default configuration (no superset_config override). The DEFAULT_FEATURE_FLAGS dict in superset/config.py includes the lines at `superset/config.py:562-563` showing `"EMBEDDABLE_CHARTS": True`. 2. Verify embedded configuration values present in same file: the default guest secret `GUEST_TOKEN_JWT_SECRET` is set to the insecure default (`"test-guest-secret-change-me"`) in superset/config.py (Embedded config options section). 3. Use the UI flow described in the PR (Dashboard → Chart dropdown → "Embed chart" → POST /api/v1/embedded_chart/) which will be enabled when EMBEDDABLE_CHARTS is True; the endpoint will generate a guest token signed with the configured `GUEST_TOKEN_JWT_SECRET`. 4. Because EMBEDDABLE_CHARTS defaults to True and the guest secret is the insecure default, a deployment that hasn't overridden the secret will issue guest tokens signed with the known default, enabling unauthorized embeddable access. This reproduces the risk without any codepath modifications. ``` </details> <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:** *Security: Security risk: enabling embeddable charts by default exposes the feature while the guest token secret in this repository is the insecure default, which can allow unauthorized embedded access. Disable the feature by default so operators must explicitly opt-in after configuring a strong `GUEST_TOKEN_JWT_SECRET`. 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]
