codeant-ai-for-open-source[bot] commented on code in PR #40433: URL: https://github.com/apache/superset/pull/40433#discussion_r3303530099
########## superset/extensions/settings.py: ########## @@ -0,0 +1,55 @@ +# 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. + +"""Admin settings persistence for extensions (active chatbot, enable/disable).""" + +from typing import Any + +from superset import db +from superset.models.core import ExtensionEnabled, ExtensionSettings + +_SETTINGS_ROW_ID = 1 + + +def get_extension_settings() -> dict[str, Any]: + row = db.session.get(ExtensionSettings, _SETTINGS_ROW_ID) + enabled_rows = db.session.query(ExtensionEnabled).all() + return { + "active_chatbot_id": row.active_chatbot_id if row else None, + "enabled": {r.extension_id: r.enabled for r in enabled_rows}, + } + + +def update_extension_settings(body: dict[str, Any]) -> dict[str, Any]: + row = db.session.get(ExtensionSettings, _SETTINGS_ROW_ID) + if row is None: + row = ExtensionSettings(id=_SETTINGS_ROW_ID) + db.session.add(row) + + if "active_chatbot_id" in body: + row.active_chatbot_id = body["active_chatbot_id"] or None Review Comment: **Suggestion:** `active_chatbot_id` is assigned directly from request data without type checking; non-string JSON values (object/array) will fail at commit time against a string column and surface as a 500. Validate/cast this field to a string-or-null before persisting. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Non-string chatbot id payload crashes settings endpoint. - ⚠️ Chatbot settings update unreliable for mis-typed clients. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Issue a malformed request to the settings endpoint `PUT /api/v1/extensions/settings` (handled by `ExtensionsRestApi.put_settings` in `superset/extensions/api.py:71-56`) with a non-string `active_chatbot_id`, e.g. JSON `{"active_chatbot_id": {"id": "chatbot.one"}}`. 2. `put_settings` deserializes the JSON into `body={"active_chatbot_id": {"id": "chatbot.one"}}` and calls `update_extension_settings(body)` in `superset/extensions/settings.py:37-55`. 3. In `update_extension_settings`, the block at `settings.py:43-44` executes because `"active_chatbot_id" in body` is true, and assigns `row.active_chatbot_id = body["active_chatbot_id"] or None`, so `row.active_chatbot_id` becomes a Python `dict` instead of a string. 4. When `db.session.commit()` runs at `settings.py:54`, SQLAlchemy attempts to persist that dict into the `ExtensionSettings.active_chatbot_id` column declared as `String(250)` in `superset/models/core.py:12-18`, which can cause a database adapter error and surface as a 500 response on `/api/v1/extensions/settings` instead of rejecting the bad input up front. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4b5bb1fdc2984e77b706c32bd35c0d61&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4b5bb1fdc2984e77b706c32bd35c0d61&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/settings.py **Line:** 43:44 **Comment:** *Type Error: `active_chatbot_id` is assigned directly from request data without type checking; non-string JSON values (object/array) will fail at commit time against a string column and surface as a 500. Validate/cast this field to a string-or-null before persisting. 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%2F40433&comment_hash=e4e25a90cd9a63a0cd2fa2e5b696d21e0ec9e0b883c32d95b80752d670687ac2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40433&comment_hash=e4e25a90cd9a63a0cd2fa2e5b696d21e0ec9e0b883c32d95b80752d670687ac2&reaction=dislike'>👎</a> ########## superset/extensions/settings.py: ########## @@ -0,0 +1,55 @@ +# 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. + +"""Admin settings persistence for extensions (active chatbot, enable/disable).""" + +from typing import Any + +from superset import db +from superset.models.core import ExtensionEnabled, ExtensionSettings + +_SETTINGS_ROW_ID = 1 + + +def get_extension_settings() -> dict[str, Any]: + row = db.session.get(ExtensionSettings, _SETTINGS_ROW_ID) + enabled_rows = db.session.query(ExtensionEnabled).all() + return { + "active_chatbot_id": row.active_chatbot_id if row else None, + "enabled": {r.extension_id: r.enabled for r in enabled_rows}, + } + + +def update_extension_settings(body: dict[str, Any]) -> dict[str, Any]: + row = db.session.get(ExtensionSettings, _SETTINGS_ROW_ID) + if row is None: + row = ExtensionSettings(id=_SETTINGS_ROW_ID) + db.session.add(row) + + if "active_chatbot_id" in body: + row.active_chatbot_id = body["active_chatbot_id"] or None + + if "enabled" in body: + for extension_id, enabled in body["enabled"].items(): + flag = db.session.get(ExtensionEnabled, extension_id) + if flag is None: + flag = ExtensionEnabled(extension_id=extension_id) + db.session.add(flag) + flag.enabled = bool(enabled) Review Comment: **Suggestion:** Coercing `enabled` with `bool(enabled)` misinterprets non-boolean values (for example `"false"` becomes `True`), which can silently flip the stored setting to the wrong value. Require actual boolean input and reject or strictly parse other types. [incorrect condition logic] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Wrong enable flag stored for string boolean inputs. - ⚠️ Admin cannot reliably disable extensions via custom clients. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Call the settings update endpoint `PUT /api/v1/extensions/settings` handled by `ExtensionsRestApi.put_settings` in `superset/extensions/api.py:71-56`, but send a payload where `enabled` values are strings instead of booleans, e.g. `{"enabled": {"example.extension": "false"}}`. 2. `put_settings` parses the JSON into `body={"enabled": {"example.extension": "false"}}` and invokes `update_extension_settings(body)` in `superset/extensions/settings.py:37-55`. 3. In `update_extension_settings`, the loop at `settings.py:46-52` iterates `for extension_id, enabled in body["enabled"].items():` and eventually executes `flag.enabled = bool(enabled)` at line 52, where `enabled` is the string `"false"`. 4. Python truthiness rules treat any non-empty string as `True`, so `"false"` becomes `True` and is stored into the `ExtensionEnabled.enabled` Boolean column (`superset/models/core.py:20-25`), causing the extension to remain enabled even though the client attempted to disable it. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b3abdc21aef74c7f9d555b83b270b175&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b3abdc21aef74c7f9d555b83b270b175&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/settings.py **Line:** 52:52 **Comment:** *Incorrect Condition Logic: Coercing `enabled` with `bool(enabled)` misinterprets non-boolean values (for example `"false"` becomes `True`), which can silently flip the stored setting to the wrong value. Require actual boolean input and reject or strictly parse other types. 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%2F40433&comment_hash=f897bafe3f95e884fc2d466c78dffbd62fb4ea74d4ef5018d65c954e7df8d3ec&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40433&comment_hash=f897bafe3f95e884fc2d466c78dffbd62fb4ea74d4ef5018d65c954e7df8d3ec&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]
