bito-code-review[bot] commented on code in PR #36933:
URL: https://github.com/apache/superset/pull/36933#discussion_r2931179284
##########
superset/security/manager.py:
##########
@@ -3109,6 +3131,28 @@ def has_guest_access(self, dashboard: "Dashboard") ->
bool:
return True
return False
+ def has_guest_chart_permalink_access(self, datasource: Any) -> bool:
+ """
+ Check if a guest user has access to a datasource via a chart permalink
resource.
+
+ For embedded charts, the guest token contains chart_permalink
resources.
+ This grants datasource access when the user holds at least one
+ chart_permalink resource. The embedded chart page constrains the actual
+ query to the datasource specified in the permalink's formData.
+
+ TODO: For stricter security, resolve each permalink and compare the
+ datasource in its formData against the requested datasource. This would
+ prevent a guest user from crafting requests to other datasources.
+ """
+ user = self.get_current_guest_user_if_guest()
+ if not user or not isinstance(user, GuestUser):
+ return False
+
+ return any(
+ r.get("type") == GuestTokenResourceType.CHART_PERMALINK.value
+ for r in user.resources
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Potential security risk in datasource access</b></div>
<div id="fix">
The has_guest_chart_permalink_access method grants datasource access if the
guest user has any chart_permalink resource, without checking if that permalink
corresponds to the requested datasource. This could allow unauthorized access
to datasources from other permalinks. Consider implementing stricter validation
as noted in the TODO.
</div>
</div>
<small><i>Code Review Run #1f05a6</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/pages/EmbeddedChartsList/index.tsx:
##########
@@ -0,0 +1,254 @@
+/**
+ * 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, useMemo, useState } from 'react';
+import { Link } from 'react-router-dom';
+import { SupersetClient } from '@superset-ui/core';
+import { styled, css } from '@apache-superset/core/theme';
+import { t } from '@apache-superset/core/translation';
+import { DeleteModal, Tooltip } from '@superset-ui/core/components';
+import { Icons } from '@superset-ui/core/components/Icons';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import { ListView } from 'src/components';
+import SubMenu, { SubMenuProps } from 'src/features/home/SubMenu';
+
+const PAGE_SIZE = 25;
+
+interface EmbeddedChart {
+ uuid: string;
+ chart_id: number;
+ chart_name: string | null;
+ allowed_domains: string[];
+ changed_on: string | null;
+}
+
+interface EmbeddedChartsListProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+}
+
+const ActionsWrapper = styled.div`
+ display: flex;
+ gap: 8px;
+`;
+
+const ActionButton = styled.button`
+ ${({ theme }) => css`
+ background: none;
+ border: none;
+ cursor: pointer;
+ padding: 4px;
+ color: ${theme.colorTextSecondary};
+ &:hover {
+ color: ${theme.colorPrimary};
+ }
+ `}
+`;
+
+function EmbeddedChartsList({
+ addDangerToast,
+ addSuccessToast,
+}: EmbeddedChartsListProps) {
+ const [embeddedCharts, setEmbeddedCharts] = useState<EmbeddedChart[]>([]);
+ const [loading, setLoading] = useState(true);
+ const [chartToDelete, setChartToDelete] = useState<EmbeddedChart | null>(
+ null,
+ );
+
+ const fetchEmbeddedCharts = useCallback(async () => {
+ setLoading(true);
+ try {
+ const response = await SupersetClient.get({
+ endpoint: '/api/v1/chart/embedded',
+ });
+ setEmbeddedCharts(response.json.result || []);
+ } catch (error) {
+ addDangerToast(t('Error loading embedded charts'));
+ } finally {
+ setLoading(false);
+ }
+ }, [addDangerToast]);
+
+ useEffect(() => {
+ fetchEmbeddedCharts();
+ }, [fetchEmbeddedCharts]);
+
+ const handleDelete = useCallback(
+ async (chart: EmbeddedChart) => {
+ try {
+ await SupersetClient.delete({
+ endpoint: `/api/v1/chart/${chart.chart_id}/embedded`,
+ });
+ addSuccessToast(t('Embedding disabled for %s', chart.chart_name));
+ fetchEmbeddedCharts();
+ } catch (error) {
+ addDangerToast(t('Error disabling embedding for %s',
chart.chart_name));
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Null chart name in delete toasts</b></div>
<div id="fix">
The success and error toasts in handleDelete use chart.chart_name directly,
but since chart_name can be null (as defined in the EmbeddedChart interface),
this will display 'null' in the message. Use a fallback like chart.chart_name
|| t('Untitled') to match the pattern used elsewhere in the component for
consistency.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- const handleDelete = useCallback(
- async (chart: EmbeddedChart) => {
- try {
- await SupersetClient.delete({
- endpoint: `/api/v1/chart/${chart.chart_id}/embedded`,
- });
- addSuccessToast(t('Embedding disabled for %s', chart.chart_name));
- fetchEmbeddedCharts();
- } catch (error) {
- addDangerToast(t('Error disabling embedding for %s',
chart.chart_name));
- } finally {
- setChartToDelete(null);
- }
- },
- [addDangerToast, addSuccessToast, fetchEmbeddedCharts],
- );
+ const handleDelete = useCallback(
+ async (chart: EmbeddedChart) => {
- try {
- await SupersetClient.delete({
- endpoint: `/api/v1/chart/${chart.chart_id}/embedded`,
- });
- addSuccessToast(t('Embedding disabled for %s', chart.chart_name
|| t('Untitled')));
- fetchEmbeddedCharts();
- } catch (error) {
- addDangerToast(t('Error disabling embedding for %s',
chart.chart_name || t('Untitled')));
- } finally {
- setChartToDelete(null);
- }
- },
- [addDangerToast, addSuccessToast, fetchEmbeddedCharts],
- );
```
</div>
</details>
</div>
<small><i>Code Review Run #1f05a6</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/embedded_chart/api.py:
##########
@@ -0,0 +1,321 @@
+# 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.exceptions import CommandException
+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.exceptions import KeyValueParseKeyError
+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 (ValueError, KeyError, KeyValueParseKeyError) as ex:
+ logger.warning("Error fetching embedded chart: %s", ex)
+ 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")
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing i18n for error messages</b></div>
<div id="fix">
Import `gettext` as `_` from `flask_babel` at the top of the file and wrap
all user-facing error message strings in calls to `_()`, e.g.,
`self.response_400(message=_('form_data is required'))`.
</div>
</div>
<small><i>Code Review Run #1f05a6</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/charts/api.py:
##########
@@ -1184,3 +1196,253 @@ def import_(self) -> Response:
)
command.run()
return self.response(200, message="OK")
+
+ @expose("/<id_or_uuid>/embedded", methods=("GET",))
+ @protect()
+ @safe
+ @permission_name("read")
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.get_embedded",
+ log_to_statsd=False,
+ )
+ def get_embedded(self, id_or_uuid: str) -> Response:
+ """Get the chart's embedded configuration.
+ ---
+ get:
+ summary: Get the chart's embedded configuration
+ parameters:
+ - in: path
+ schema:
+ type: string
+ name: id_or_uuid
+ description: The chart id or uuid
+ responses:
+ 200:
+ description: Result contains the embedded chart config
+ 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()
+
+ if not chart.embedded:
+ return self.response(404)
+ embedded: EmbeddedChart = chart.embedded[0]
+ result = self.embedded_response_schema.dump(embedded)
+ return self.response(200, result=result)
+
+ @expose("/<id_or_uuid>/embedded", methods=["POST", "PUT"])
+ @protect()
+ @safe
+ @permission_name("set_embedded")
+ @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)
+
+ @expose("/<id_or_uuid>/embedded", methods=("DELETE",))
+ @protect()
+ @safe
+ @permission_name("set_embedded")
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self,
+ *args,
+ **kwargs: f"{self.__class__.__name__}.delete_embedded",
+ log_to_statsd=False,
+ )
+ def delete_embedded(self, id_or_uuid: str) -> Response:
+ """Delete a chart's embedded configuration.
+ ---
+ delete:
+ summary: Delete a chart's embedded configuration
+ parameters:
+ - in: path
+ schema:
+ type: string
+ name: id_or_uuid
+ description: The chart id or uuid
+ responses:
+ 200:
+ description: Successfully removed the configuration
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ 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()
+
+ DeleteEmbeddedChartCommand(chart).run()
+ return self.response(200, message="OK")
+
+ @expose("/embedded", methods=("GET",))
+ @protect()
+ @safe
+ @permission_name("read")
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.list_embedded",
+ log_to_statsd=False,
+ )
+ def list_embedded(self) -> Response:
+ """List all embedded chart configurations.
+ ---
+ get:
+ summary: List all embedded chart configurations
+ responses:
+ 200:
+ description: List of embedded charts
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ type: array
+ items:
+ type: object
+ properties:
+ uuid:
+ type: string
+ chart_id:
+ type: integer
+ chart_name:
+ type: string
+ allowed_domains:
+ type: array
+ items:
+ type: string
+ changed_on:
+ type: string
+ 401:
+ $ref: '#/components/responses/401'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ embedded_charts = (
+ db.session.query(EmbeddedChart)
+ .join(Slice, EmbeddedChart.chart_id == Slice.id)
+ .all()
+ )
+ result = [
+ {
+ "uuid": str(ec.uuid),
+ "chart_id": ec.chart_id,
+ "chart_name": ec.chart.slice_name if ec.chart else None,
+ "allowed_domains": ec.allowed_domains,
+ "changed_on": ec.changed_on.isoformat() if ec.changed_on else
None,
+ }
+ for ec in embedded_charts
+ ]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Security: Missing permission filters in list
endpoint</b></div>
<div id="fix">
The list_embedded endpoint queries all EmbeddedChart records without
applying user permission filters, potentially exposing embedded configuration
for charts the user cannot access. Other chart endpoints like get_embedded use
ChartDAO.get_by_id_or_uuid, which applies ChartFilter for permissions, but this
list method does not. This could lead to information disclosure if embedded
charts contain sensitive data.
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Performance: N+1 query in list endpoint</b></div>
<div id="fix">
The query in list_embedded joins Slice but does not eagerly load the
EmbeddedChart.chart relationship, causing N+1 queries when accessing
ec.chart.slice_name for each embedded chart. This can degrade performance with
many records. Use joinedload to populate the relationship in the initial query.
</div>
</div>
<small><i>Code Review Run #1f05a6</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [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]
