bito-code-review[bot] commented on code in PR #36933: URL: https://github.com/apache/superset/pull/36933#discussion_r2908608016
########## .rat-excludes: ########## @@ -86,3 +86,4 @@ CLAUDE.md CURSOR.md GEMINI.md GPT.md +embed-demo.html Review Comment: <div> <div id="suggestion"> <div id="issue"><b>License Header Exclusion Inconsistency</b></div> <div id="fix"> Excluding embed-demo.html from license checks appears inconsistent with the project's standards in .cursor/rules/dev-standard.mdc and AGENTS.md, which permit exclusions only for LLM instruction files like LLMS.md or CLAUDE.md. If this file requires a license header, it should not be excluded; consider removing this entry. </div> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/44c9c94/.cursor/rules/dev-standard.mdc#L48";>dev-standard.mdc:48</a> </li> </ul> </details> <small><i>Code Review Run #2ff7b5</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,320 @@ +# 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.shared_entries import get_permalink_salt +from superset.key_value.types import ( + KeyValueResource, + MarshmallowKeyValueCodec, + SharedKey, +) +from superset.key_value.utils import decode_permalink_id Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing exception import for permalink validation</b></div> <div id="fix"> The _get_permalink_value method calls decode_permalink_id, which can raise KeyValueParseKeyError for invalid keys, but this exception isn't imported or caught, potentially leading to unhandled 500 errors instead of proper 404 responses. It looks like similar permalink APIs catch this explicitly. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion from superset.key_value.utils import decode_permalink_id from superset.key_value.exceptions import KeyValueParseKeyError ```` </div> </details> </div> <small><i>Code Review Run #2ff7b5</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,320 @@ +# 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.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, + ) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Unhandled exception in permalink decoding</b></div> <div id="fix"> Without catching KeyValueParseKeyError here, invalid permalink keys would propagate up and cause 500 errors rather than returning None for 404 handling. Other permalink getters in the codebase handle this similarly. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion try: key = decode_permalink_id(permalink_key, salt=salt) except KeyValueParseKeyError: return None return KeyValueDAO.get_value( KeyValueResource.EXPLORE_PERMALINK, key, codec, ) ```` </div> </details> </div> <small><i>Code Review Run #2ff7b5</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]
