codeant-ai-for-open-source[bot] commented on code in PR #39171: URL: https://github.com/apache/superset/pull/39171#discussion_r3624206011
########## superset-core/src/superset_core/extensions/context.py: ########## @@ -0,0 +1,136 @@ +# 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. + +""" +Extension Context API for superset-core extensions. + +Provides access to the current extension's context, including metadata +and scoped resources like storage. Extensions call `get_context()` to +access their context during execution. + +The context is set by the host (Superset) during extension loading and +is only available within extension code. + +Usage: + from superset_core.extensions.context import get_context + + def setup(): + ctx = get_context() + + # Access extension metadata + print(f"Running {ctx.extension.displayName} v{ctx.extension.version}") + + # Access extension-scoped storage + ctx.storage.ephemeral.set("lastRun", time.time(), EphemeralSetOptions(ttl=3600)) + data = ctx.storage.ephemeral.get("cachedData") +""" + +from __future__ import annotations + +from typing import Any, Protocol, TYPE_CHECKING + +if TYPE_CHECKING: + from superset_core.extensions.types import BaseExtension + + +class StorageAccessor(Protocol): + """Protocol for storage access with user-scoped and shared modes.""" + + def get(self, key: str) -> Any: + """Get a value from storage.""" + ... + + def set(self, key: str, value: Any, ttl: int = 3600) -> None: + """Set a value in storage with optional TTL.""" + ... Review Comment: **Suggestion:** The `StorageAccessor.set` contract uses a `ttl` integer argument, but the Tier 2 implementation in this PR expects an `EphemeralSetOptions` object; this API mismatch will let callers pass the wrong argument shape and can trigger runtime `TypeError` when the concrete accessor is invoked. Split the protocol so ephemeral and persistent setters use their real signatures. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Backend extensions using Python ephemeral storage can crash. - ⚠️ Ephemeral writes fail, losing short-lived extension state. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In a backend extension, import `get_context()` from `superset-core/src/superset_core/extensions/context.py:104` and obtain `ctx = get_context()`. 2. Use the `ExtensionStorage.ephemeral` accessor, which is typed as `StorageAccessor` in `superset-core/src/superset_core/extensions/context.py:71-76`, and follow its documented signature `set(self, key, value, ttl=3600)` from `context.py:57-58`. 3. Implement code such as `ctx.storage.ephemeral.set("job_progress", {"pct": 42}, ttl=300)` based on that protocol. 4. At runtime the concrete Tier 2 implementation for `ephemeral` follows `EphemeralStateAccessor.set(self, key, value, options: EphemeralSetOptions)` from `superset-core/src/superset_core/extensions/storage/ephemeral.py:69-76`, attempts to treat the third argument as an `EphemeralSetOptions` (accessing `options.ttl` / `options.codec`), and crashes with an `AttributeError`/`TypeError` because an `int` was passed instead of the expected dataclass. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=25d1bb1147b3457b88546bffce0ba4b8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=25d1bb1147b3457b88546bffce0ba4b8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-core/src/superset_core/extensions/context.py **Line:** 57:59 **Comment:** *Possible Bug: The `StorageAccessor.set` contract uses a `ttl` integer argument, but the Tier 2 implementation in this PR expects an `EphemeralSetOptions` object; this API mismatch will let callers pass the wrong argument shape and can trigger runtime `TypeError` when the concrete accessor is invoked. Split the protocol so ephemeral and persistent setters use their real signatures. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=b5a68826a5a7c81ddc3892ed95b040cf044c8dc7fe8230458a9528303d7ccac4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=b5a68826a5a7c81ddc3892ed95b040cf044c8dc7fe8230458a9528303d7ccac4&reaction=dislike'>👎</a> ########## superset/extensions/storage/persistent.py: ########## @@ -0,0 +1,244 @@ +# 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. + +""" +Host implementation for Persistent State (Tier 3 Storage). + +Provides the concrete database-backed implementation that is injected into +superset_core.extensions.storage.persistent at startup. +""" + +from __future__ import annotations + +from typing import Any, ClassVar + +from superset_core.extensions.storage.persistent import ( + PersistentListEntry, + PersistentListOptions, + PersistentListResult, + PersistentSetOptions, + PersistentState as CorePersistentState, +) + +from superset.extensions.storage.codecs import DEFAULT_CODEC, get_codec +from superset.extensions.storage.persistent_dao import ExtensionStorageDAO +from superset.extensions.storage.utils import ( + get_current_extension_id, + get_current_user_id, +) +from superset.utils.decorators import transaction + + +def _get_extension_id() -> str: + """Get the current extension ID from context.""" + return get_current_extension_id("persistent_state") + + +def _list( + extension_id: str, + user_fk: int | None, + options: PersistentListOptions, +) -> PersistentListResult: + """Shared `list` implementation for both scopes. + + Unlike the REST API, ambient backend code has no SAFE_CODECS + restriction — every entry's value is decoded unconditionally, same as + `get_decoded_value()`. + """ + entries, count = ExtensionStorageDAO.list_entries( + extension_id, + user_fk=user_fk, + resource_type=options.resource_type, + resource_uuid=options.resource_uuid, + page=options.page, + page_size=options.page_size, + ) + return PersistentListResult( + entries=[ + PersistentListEntry( + key=entry.key, + value=( + get_codec(entry.codec).decode(entry.value) + if entry.value is not None + else None + ), Review Comment: **Suggestion:** Listing decodes stored bytes directly with the codec but never decrypts entries marked as encrypted, so encrypted values will fail to decode or return invalid payloads. The list path needs to apply the same decrypt-then-decode behavior as single-item reads. [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Listing encrypted persistent entries fails or returns garbage. - ⚠️ Extensions cannot reliably paginate encrypted persistent state. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In a Superset extension frontend, call `ctx.storage.persistent.set('secret', payload, { encrypt: true })` via `superset-frontend/packages/superset-core/src/storage/index.ts`, which routes to core `PersistentState.set` (`superset-core/src/superset_core/extensions/storage/persistent.py:144`) and then to host `PersistentState.set` in `superset/extensions/storage/persistent.py:185-211`. 2. Host `PersistentState.set` passes `encrypt=True` and the current user id from `get_current_user_id('persistent_state')` (`superset/extensions/storage/utils.py:40-100`) into `ExtensionStorageDAO.set` (`superset/extensions/storage/persistent_dao.py:504`), which stores an encrypted blob and marks `ExtensionStorage.is_encrypted = True` (column defined in `superset/extensions/storage/persistent_model.py:101-103`). 3. Later, the extension calls `ctx.storage.persistent.list(options)` to page through keys; this invokes host `PersistentState.list` (`superset/extensions/storage/persistent.py:214-227`), which delegates to `_list` at `superset/extensions/storage/persistent.py:50-84`. 4. `_list` iterates the entries from `ExtensionStorageDAO.list_entries` (`superset/extensions/storage/persistent_dao.py:428`) and constructs `PersistentListEntry` objects, decoding each `entry.value` via `get_codec(entry.codec).decode(entry.value)` at lines 74-78. For rows where `is_encrypted` is True, `entry.value` contains ciphertext, so decoding without first decrypting causes codec-specific errors (e.g. JSON parse failures) or garbage values, breaking list responses for encrypted data and potentially returning 500 errors to the extension. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c66cf479ad5f4ebca5a26132d6ff1af8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c66cf479ad5f4ebca5a26132d6ff1af8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/extensions/storage/persistent.py **Line:** 74:78 **Comment:** *Possible Bug: Listing decodes stored bytes directly with the codec but never decrypts entries marked as encrypted, so encrypted values will fail to decode or return invalid payloads. The list path needs to apply the same decrypt-then-decode behavior as single-item reads. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=34ddc2555f5dfaa24b33c16df95de7fd0ce29c844110388f2f83fc89c4bfd8a4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=34ddc2555f5dfaa24b33c16df95de7fd0ce29c844110388f2f83fc89c4bfd8a4&reaction=dislike'>👎</a> ########## superset/extensions/storage/persistent_model.py: ########## @@ -0,0 +1,136 @@ +# 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 uuid as uuid_module + +from flask_appbuilder import Model +from sqlalchemy import ( + Boolean, + Column, + ForeignKey, + Index, + Integer, + LargeBinary, + String, + UniqueConstraint, +) +from sqlalchemy_utils import UUIDType +from superset_core.extensions.storage.models import ( + ExtensionStorageEntry as CoreExtensionStorageEntry, +) + +from superset.models.helpers import AuditMixinNullable + + +class ExtensionStorage(CoreExtensionStorageEntry, AuditMixinNullable, Model): + """Generic persistent key-value storage for extensions (Tier 3). + + Each row is identified by (extension_id, user_fk, resource_type, + resource_uuid, key): + + * Global scope — user_fk IS NULL, resource_type IS NULL + * User scope — user_fk set, resource_type IS NULL + * Resource scope — resource_type + resource_uuid set (user_fk optional) + + The payload is stored as raw bytes (value) alongside the identifier of + the codec used to encode it (codec), so the same bytes can be decoded + back into a value on read. When is_encrypted is True the value has been + encrypted at the DAO layer using Fernet and must be decrypted before + decoding. + """ + + __tablename__ = "extension_storage" + + id = Column(Integer, primary_key=True, autoincrement=True) + uuid = Column( + UUIDType(binary=True), + default=uuid_module.uuid4, + unique=True, + nullable=False, + ) + + # Extension identity + extension_id = Column(String(255), nullable=False) + + # Scope discriminators — all nullable; NULLs define the scope (see docstring). + # No relationship() is declared for user_fk: extension storage rows are + # not deleted when their owning user is (ondelete="SET NULL" demotes them + # to global scope instead), and nothing reads through such a relationship, + # so there's no ORM object here for a future change to attach a cascade to. + user_fk = Column( + Integer, + ForeignKey( + "ab_user.id", + ondelete="SET NULL", + name="fk_extension_storage_user_fk_ab_user", + ), + nullable=True, + ) Review Comment: **Suggestion:** Using `ondelete="SET NULL"` on the user foreign key silently moves user-scoped rows into shared scope when a user is deleted, which can expose previously private per-user data to all users through shared access. User-owned rows should be deleted or otherwise kept inaccessible instead of being demoted to global visibility. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Deleted users' persistent data can become globally readable. - ⚠️ Privacy risk for extensions storing user-specific information. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. An extension stores per-user data via `ctx.storage.persistent.set('prefs', value)` in the frontend (`superset-frontend/packages/superset-core/src/storage/index.ts`), which calls core `PersistentState.set` (`superset-core/src/superset_core/extensions/storage/persistent.py:144`) and host `PersistentState.set` in `superset/extensions/storage/persistent.py:185-211`. 2. Host `PersistentState.set` obtains the current user id from `get_current_user_id('persistent_state')` (`superset/extensions/storage/utils.py:40-100`) and passes it as `user_fk` into `ExtensionStorageDAO.set` (`superset/extensions/storage/persistent_dao.py:504`), creating an `ExtensionStorage` row whose `user_fk` column (defined at `superset/extensions/storage/persistent_model.py:75-82`) points to the owning user in `ab_user`. 3. An administrator later deletes that user from the `ab_user` table; because the foreign key on `ExtensionStorage.user_fk` is declared with `ondelete="SET NULL"` and `nullable=True` at `superset/extensions/storage/persistent_model.py:78-82`, the database automatically sets `user_fk` to `NULL`, effectively turning the row into a global (shared) entry. 4. Another user calls `ctx.storage.persistent.shared.get('prefs')`, which routes to `SharedPersistentStateAccessor.get` in `superset/extensions/storage/persistent.py:95-105`; this passes `user_fk=None` to `ExtensionStorageDAO.get_decoded_value`, causing the demoted row to be returned as shared state and exposing what was originally user-specific data to all users of the extension. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=211df7eff2814c499a1a28d8eed9aae7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=211df7eff2814c499a1a28d8eed9aae7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/extensions/storage/persistent_model.py **Line:** 75:82 **Comment:** *Security: Using `ondelete="SET NULL"` on the user foreign key silently moves user-scoped rows into shared scope when a user is deleted, which can expose previously private per-user data to all users through shared access. User-owned rows should be deleted or otherwise kept inaccessible instead of being demoted to global visibility. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=78e408aeeccd23f4f49ddfad47f27cef819d9316a78050158f371e531f2cc351&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=78e408aeeccd23f4f49ddfad47f27cef819d9316a78050158f371e531f2cc351&reaction=dislike'>👎</a> -- 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]
