This is an automated email from the ASF dual-hosted git repository. EnxDev pushed a commit to branch test/chatbot-local in repository https://gitbox.apache.org/repos/asf/superset.git
commit 7f6f805ffa830023c4c096833f42ebec21c314d0 Author: Enzo Martellucci <[email protected]> AuthorDate: Thu May 28 11:21:30 2026 +0200 fix(extensions): sync second-round review fixes to chatbot-local branch - Validate manifest.id segments in POST endpoint before building dest_file path - Add hostile manifest.id test (../../tmp/evil → 400) - Add sqlite-backed round-trip tests for settings.py upsert logic - Add HTTP tests for GET/PUT /api/v1/extensions/settings endpoints - Wrap handleDelete in useCallback; already in columns useMemo deps - Fix MySQL comment drift in _upsert_settings_row (read-then-update, not merge) - Add intentional no-admin-gate comment to get_settings endpoint Co-Authored-By: Claude Sonnet 4.6 <[email protected]> --- .../src/extensions/ExtensionsList.tsx | 33 ++-- superset/extensions/api.py | 32 +++- superset/extensions/settings.py | 6 +- tests/unit_tests/extensions/test_api.py | 27 +++ tests/unit_tests/extensions/test_settings.py | 207 +++++++++++++++++++++ 5 files changed, 283 insertions(+), 22 deletions(-) diff --git a/superset-frontend/src/extensions/ExtensionsList.tsx b/superset-frontend/src/extensions/ExtensionsList.tsx index 3902fd1ee19..de2d3f27531 100644 --- a/superset-frontend/src/extensions/ExtensionsList.tsx +++ b/superset-frontend/src/extensions/ExtensionsList.tsx @@ -193,22 +193,25 @@ const ExtensionsList: FunctionComponent<ExtensionsListProps> = ({ }); }; - const handleDelete = (extension: Extension) => { - const { publisher, name } = extension; - SupersetClient.delete({ - endpoint: `/api/v1/extensions/${publisher}/${name}`, - }).then( - () => { - addSuccessToast(t('Deleted: %s', extension.name)); - refreshData(); - }, - createErrorHandler(errMsg => - addDangerToast( - t('There was an issue deleting %s: %s', extension.name, errMsg), + const handleDelete = useCallback( + (extension: Extension) => { + const { publisher, name } = extension; + SupersetClient.delete({ + endpoint: `/api/v1/extensions/${publisher}/${name}`, + }).then( + () => { + addSuccessToast(t('Deleted: %s', extension.name)); + refreshData(); + }, + createErrorHandler(errMsg => + addDangerToast( + t('There was an issue deleting %s: %s', extension.name, errMsg), + ), ), - ), - ); - }; + ); + }, + [addDangerToast, addSuccessToast, refreshData], + ); const columns = useMemo( () => [ diff --git a/superset/extensions/api.py b/superset/extensions/api.py index cf841e86aaf..d7bc73cc02e 100644 --- a/superset/extensions/api.py +++ b/superset/extensions/api.py @@ -289,25 +289,44 @@ class ExtensionsRestApi(BaseApi): except Exception as ex: # pylint: disable=broad-except return self.response(400, message=f"Invalid extension bundle: {ex}") + # Validate the manifest id before using it as a filename component. + # The id is publisher.name (e.g. "acme.chatbot"); each segment must pass + # _validate_segment so a crafted bundle cannot write outside EXTENSIONS_PATH + # even though the admin is trusted — defence-in-depth against third-party + # bundles the admin did not author. + manifest_id: str = extension.manifest.id + id_parts = manifest_id.split(".", 1) + if len(id_parts) != 2 or not all( # noqa: PLR2004 + _validate_segment(p) for p in id_parts + ): + return self.response( + 400, + message=( + f"Invalid extension id '{manifest_id}' in manifest. " + "Expected '<publisher>.<name>' with alphanumeric, hyphen, " + "or underscore characters only." + ), + ) + # Reject bundles whose manifest id collides with a LOCAL_EXTENSIONS entry. local_ids = { Path(p).name for p in current_app.config.get("LOCAL_EXTENSIONS", []) } - if extension.manifest.id in local_ids: + if manifest_id in local_ids: return self.response( 409, message=( - f"Extension '{extension.manifest.id}' is already installed as a " + f"Extension '{manifest_id}' is already installed as a " "local extension. Remove it from LOCAL_EXTENSIONS before uploading." ), ) # Persist to EXTENSIONS_PATH so the extension survives restarts. - # Destination path is derived from the validated manifest id, not the - # uploaded filename, so the upload filename cannot escape EXTENSIONS_PATH. + # Destination filename is built from the validated manifest id, not from the + # uploaded filename, so neither can escape EXTENSIONS_PATH. dest_dir = Path(extensions_path) dest_dir.mkdir(parents=True, exist_ok=True) - dest_file = dest_dir / f"{extension.manifest.id}.supx" + dest_file = dest_dir / f"{manifest_id}.supx" stream.seek(0) dest_file.write_bytes(stream.read()) @@ -389,6 +408,9 @@ class ExtensionsRestApi(BaseApi): @expose("/settings", methods=("GET",)) def get_settings(self, **kwargs: Any) -> Response: """Get global extension admin settings. + + No admin gate here by design: authenticated non-admin users need these + settings so the ChatbotMount can read active_chatbot_id on every page. --- get: summary: Get extension admin settings (active chatbot, enabled flags). diff --git a/superset/extensions/settings.py b/superset/extensions/settings.py index ecce0accf19..c6993221763 100644 --- a/superset/extensions/settings.py +++ b/superset/extensions/settings.py @@ -64,7 +64,9 @@ def _upsert_settings_row( ) db.session.execute(stmt) else: - # MySQL/MariaDB: session.merge handles INSERT-or-UPDATE without rollback. + # MySQL/MariaDB: read-then-update (no native INSERT-OR-REPLACE in SQLAlchemy + # core that is safe under concurrent writes, so we rely on the @transaction + # decorator for serialisation). obj = db.session.get(ExtensionSettings, _SETTINGS_ROW_ID) if obj is None: obj = ExtensionSettings( @@ -99,7 +101,7 @@ def _upsert_enabled_flag(extension_id: str, enabled: bool) -> None: ) db.session.execute(stmt) else: - # MySQL/MariaDB: read-then-update without rollback. + # MySQL/MariaDB: read-then-update, serialised by the @transaction decorator. obj = ( db.session.query(ExtensionEnabled) .filter_by(extension_id=extension_id) diff --git a/tests/unit_tests/extensions/test_api.py b/tests/unit_tests/extensions/test_api.py index c78f3953e95..e182056062c 100644 --- a/tests/unit_tests/extensions/test_api.py +++ b/tests/unit_tests/extensions/test_api.py @@ -234,6 +234,33 @@ class TestPostEndpoint: assert resp.status_code == 409 assert "local extension" in resp.json["message"] + def test_hostile_manifest_id_rejected( + self, client: Any, full_api_access: None, mocker: Any, tmp_path: Path + ) -> None: + """A crafted manifest.id with path traversal must not escape EXTENSIONS_PATH.""" + mocker.patch( + "superset.extensions.api.security_manager.is_admin", return_value=True + ) + mocker.patch.dict( + "flask.current_app.config", + {"EXTENSIONS_PATH": str(tmp_path), "LOCAL_EXTENSIONS": []}, + ) + fake_ext = _make_fake_extension("../../tmp/evil") + mocker.patch( + "superset.extensions.api.get_bundle_files_from_zip", return_value=[] + ) + mocker.patch( + "superset.extensions.api.get_loaded_extension", return_value=fake_ext + ) + supx = _make_supx("../../tmp/evil") + resp = client.post( + "/api/v1/extensions/", + data={"bundle": (io.BytesIO(supx), "ext.supx")}, + content_type="multipart/form-data", + ) + assert resp.status_code == 400 + assert "Invalid extension id" in resp.json["message"] + def test_happy_path_returns_201( self, client: Any, full_api_access: None, mocker: Any, tmp_path: Path ) -> None: diff --git a/tests/unit_tests/extensions/test_settings.py b/tests/unit_tests/extensions/test_settings.py new file mode 100644 index 00000000000..22466ab072e --- /dev/null +++ b/tests/unit_tests/extensions/test_settings.py @@ -0,0 +1,207 @@ +# 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. + +"""Unit tests for extension settings persistence and the settings API endpoints.""" + +from __future__ import annotations + +from typing import Any + +# --------------------------------------------------------------------------- +# Settings persistence (settings.py) — sqlite-backed round-trip tests +# --------------------------------------------------------------------------- + + +class TestGetExtensionSettings: + def test_returns_defaults_when_no_rows(self, app_context: Any) -> None: + from superset.extensions.settings import get_extension_settings + + result = get_extension_settings() + assert result["active_chatbot_id"] is None + assert result["enabled"] == {} + + def test_round_trips_active_chatbot_id(self, app_context: Any) -> None: + from superset.extensions.settings import ( + get_extension_settings, + update_extension_settings, + ) + + update_extension_settings({"active_chatbot_id": "acme.chatbot"}) + result = get_extension_settings() + assert result["active_chatbot_id"] == "acme.chatbot" + + def test_round_trips_enabled_flags(self, app_context: Any) -> None: + from superset.extensions.settings import ( + get_extension_settings, + update_extension_settings, + ) + + update_extension_settings( + {"enabled": {"acme.chatbot": True, "acme.widget": False}} + ) + result = get_extension_settings() + assert result["enabled"]["acme.chatbot"] is True + assert result["enabled"]["acme.widget"] is False + + +class TestUpdateExtensionSettings: + def test_empty_string_active_chatbot_id_stored_as_none( + self, app_context: Any + ) -> None: + from superset.extensions.settings import ( + get_extension_settings, + update_extension_settings, + ) + + # First set a value, then clear it via empty string. + update_extension_settings({"active_chatbot_id": "acme.chatbot"}) + update_extension_settings({"active_chatbot_id": ""}) + assert get_extension_settings()["active_chatbot_id"] is None + + def test_non_bool_enabled_value_is_skipped(self, app_context: Any) -> None: + from superset.extensions.settings import ( + get_extension_settings, + update_extension_settings, + ) + + update_extension_settings({"enabled": {"acme.chatbot": "yes"}}) + # "yes" is not a bool — the row should not have been written. + result = get_extension_settings() + assert "acme.chatbot" not in result["enabled"] + + def test_upsert_overwrites_existing_chatbot(self, app_context: Any) -> None: + from superset.extensions.settings import ( + get_extension_settings, + update_extension_settings, + ) + + update_extension_settings({"active_chatbot_id": "acme.chatbot"}) + update_extension_settings({"active_chatbot_id": "vendor.bot"}) + assert get_extension_settings()["active_chatbot_id"] == "vendor.bot" + + def test_upsert_overwrites_existing_enabled_flag(self, app_context: Any) -> None: + from superset.extensions.settings import ( + get_extension_settings, + update_extension_settings, + ) + + update_extension_settings({"enabled": {"acme.chatbot": True}}) + update_extension_settings({"enabled": {"acme.chatbot": False}}) + assert get_extension_settings()["enabled"]["acme.chatbot"] is False + + def test_partial_update_leaves_other_keys_intact(self, app_context: Any) -> None: + from superset.extensions.settings import ( + get_extension_settings, + update_extension_settings, + ) + + update_extension_settings( + {"active_chatbot_id": "acme.chatbot", "enabled": {"acme.widget": True}} + ) + # Update only enabled — active_chatbot_id must survive. + update_extension_settings({"enabled": {"acme.widget": False}}) + result = get_extension_settings() + assert result["active_chatbot_id"] == "acme.chatbot" + assert result["enabled"]["acme.widget"] is False + + def test_returns_current_state(self, app_context: Any) -> None: + from superset.extensions.settings import update_extension_settings + + result = update_extension_settings({"active_chatbot_id": "acme.chatbot"}) + assert result["active_chatbot_id"] == "acme.chatbot" + + +# --------------------------------------------------------------------------- +# GET /api/v1/extensions/settings +# --------------------------------------------------------------------------- + + +class TestGetSettingsEndpoint: + def test_authenticated_user_can_read( + self, client: Any, full_api_access: None, mocker: Any + ) -> None: + mocker.patch( + "superset.extensions.api.get_extension_settings", + return_value={"active_chatbot_id": None, "enabled": {}}, + ) + resp = client.get("/api/v1/extensions/settings") + assert resp.status_code == 200 + assert resp.json["result"]["active_chatbot_id"] is None + + def test_returns_active_chatbot_and_enabled_map( + self, client: Any, full_api_access: None, mocker: Any + ) -> None: + mocker.patch( + "superset.extensions.api.get_extension_settings", + return_value={ + "active_chatbot_id": "acme.chatbot", + "enabled": {"acme.chatbot": True}, + }, + ) + resp = client.get("/api/v1/extensions/settings") + assert resp.status_code == 200 + assert resp.json["result"]["active_chatbot_id"] == "acme.chatbot" + assert resp.json["result"]["enabled"]["acme.chatbot"] is True + + +# --------------------------------------------------------------------------- +# PUT /api/v1/extensions/settings +# --------------------------------------------------------------------------- + + +class TestPutSettingsEndpoint: + def test_non_admin_rejected( + self, client: Any, full_api_access: None, mocker: Any + ) -> None: + mocker.patch( + "superset.extensions.api.security_manager.is_admin", return_value=False + ) + resp = client.put( + "/api/v1/extensions/settings", + json={"active_chatbot_id": "acme.chatbot"}, + ) + assert resp.status_code == 403 + + def test_admin_can_update_active_chatbot( + self, client: Any, full_api_access: None, mocker: Any + ) -> None: + mocker.patch( + "superset.extensions.api.security_manager.is_admin", return_value=True + ) + mocker.patch( + "superset.extensions.api.update_extension_settings", + return_value={"active_chatbot_id": "acme.chatbot", "enabled": {}}, + ) + resp = client.put( + "/api/v1/extensions/settings", + json={"active_chatbot_id": "acme.chatbot"}, + ) + assert resp.status_code == 200 + assert resp.json["result"]["active_chatbot_id"] == "acme.chatbot" + + def test_empty_body_is_accepted( + self, client: Any, full_api_access: None, mocker: Any + ) -> None: + mocker.patch( + "superset.extensions.api.security_manager.is_admin", return_value=True + ) + mocker.patch( + "superset.extensions.api.update_extension_settings", + return_value={"active_chatbot_id": None, "enabled": {}}, + ) + resp = client.put("/api/v1/extensions/settings", json={}) + assert resp.status_code == 200
