fix(semantic_layers): restore fail-closed-masked fields on update, not just write-only ones

Once a GET fell back to masking every value (schema unavailable at read
time), the update path only restored write-only keys, so a name-only save
could persist the literal mask into non-secret fields like `account` once
the schema became available again.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

superset/commands/semantic_layer/update.py

@@ -34,6 +34,7 @@ from superset.commands.semantic_layer.exceptions import (
     SemanticViewUpdateFailedError,
 )
 from superset.commands.utils import current_user_can_modify_object
+from superset.constants import PASSWORD_MASK
 from superset.daos.semantic_layer import SemanticLayerDAO, SemanticViewDAO
 from superset.semantic_layers.models import SemanticLayer, SemanticView
 from superset.semantic_layers.registry import registry
@@ -43,6 +44,43 @@ from superset.utils.decorators import on_error, transaction
 logger = logging.getLogger(__name__)
 
 
+def _unmask_configuration(
+    existing_raw_configuration: str | None,
+    new_configuration: dict[str, Any],
+) -> dict[str, Any]:
+    """
+    Replace ``PASSWORD_MASK`` sentinels in an incoming update payload with
+    the value already stored.
+
+    The GET/list endpoints mask write-only configuration values (see
+    ``superset.semantic_layers.api._mask_configuration``), and fail closed by
+    masking every truthy value when the connector's schema can't be
+    determined. A client that round-trips that response back on an update
+    (e.g. a name-only edit) would otherwise overwrite the real stored
+    values -- secret or not -- with the literal mask string. Restore any key
+    whose incoming value is exactly the mask sentinel from the stored
+    configuration regardless of whether the schema currently marks it
+    write-only, since a client only ever sends the sentinel back for a value
+    it previously received masked (including a value masked by the
+    fail-closed fallback).
+    """
+    try:
+        existing_configuration = (
+            json.loads(existing_raw_configuration) if existing_raw_configuration else {}
+        )
+    except (TypeError, ValueError):
+        existing_configuration = {}
+
+    return {
+        key: (
+            existing_configuration[key]
+            if value == PASSWORD_MASK and key in existing_configuration
+            else value
+        )
+        for key, value in new_configuration.items()
+    }
+
+
 class UpdateSemanticViewCommand(BaseCommand):
     def __init__(self, model_id: int, data: dict[str, Any]):
         self._model_id = model_id
@@ -121,6 +159,12 @@ class UpdateSemanticLayerCommand(BaseCommand):
         if name and not SemanticLayerDAO.validate_update_uniqueness(self._uuid, name):
             raise SemanticLayerInvalidError(f"Name already exists: {name}")
 
+        if isinstance(self._properties.get("configuration"), dict):
+            self._properties["configuration"] = _unmask_configuration(
+                self._model.configuration,
+                self._properties["configuration"],
+            )
+
         if configuration := self._properties.get("configuration"):
             sl_type = self._model.type
             cls = registry[sl_type]

superset/semantic_layers/api.py

@@ -56,7 +56,7 @@ from superset.commands.semantic_layer.update import (
     UpdateSemanticLayerCommand,
     UpdateSemanticViewCommand,
 )
-from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP
+from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, PASSWORD_MASK
 from superset.daos.semantic_layer import SemanticLayerDAO
 from superset.datasets.schemas import get_delete_ids_schema
 from superset.exceptions import SupersetSecurityException
@@ -81,10 +81,53 @@ from superset.views.base_api import (
 logger = logging.getLogger(__name__)
 
 
+def _mask_configuration(layer: SemanticLayer, config: dict[str, Any]) -> dict[str, Any]:
+    """
+    Redact configuration values the connector's schema marks as write-only.
+
+    A connector publishes its configuration shape via ``get_configuration_schema``;
+    a property with ``"writeOnly": true`` (the standard JSON Schema way of
+    marking a field that's set but never echoed back, e.g. a password or API
+    key) is replaced with ``PASSWORD_MASK`` here rather than returned in the
+    clear.
+    """
+    schema: dict[str, Any] | None = None
+    if cls := registry.get(layer.type):
+        try:
+            schema = cls.get_configuration_schema()
+        except Exception:  # pylint: disable=broad-except
+            schema = None
+
+    if schema is None:
+        # Either the type isn't registered or its schema couldn't load, so we
+        # can't tell which fields are secret. Fail closed: mask every truthy
+        # value rather than risk echoing a credential back in the clear.
+        logger.warning(
+            "Could not determine the configuration schema for semantic layer "
+            "type %s; masking all configuration values.",
+            layer.type,
+        )
+        return {key: PASSWORD_MASK if value else value for key, value in config.items()}
+
+    secret_keys = {
+        key
+        for key, prop in schema.get("properties", {}).items()
+        if isinstance(prop, dict) and prop.get("writeOnly")
+    }
+    if not secret_keys:
+        return config
+
+    return {
+        key: PASSWORD_MASK if key in secret_keys and value else value
+        for key, value in config.items()
+    }
+
+
 def _serialize_layer(layer: SemanticLayer) -> dict[str, Any]:
     config = layer.configuration
     if isinstance(config, str):
         config = json.loads(config)
+    config = _mask_configuration(layer, config or {})
     return {
         "uuid": str(layer.uuid),
         "name": layer.name,
@@ -677,6 +720,9 @@ class SemanticLayerRestApi(BaseSupersetApi):
             404:
               $ref: '#/components/responses/404'
         """
+        if not is_feature_enabled("SEMANTIC_LAYERS"):
+            return self.response_404()
+
         layer = SemanticLayerDAO.find_by_uuid(uuid)
         if not layer:
             return self.response_404()
@@ -1158,6 +1204,9 @@ class SemanticLayerRestApi(BaseSupersetApi):
             401:
               $ref: '#/components/responses/401'
         """
+        if not is_feature_enabled("SEMANTIC_LAYERS"):
+            return self.response_404()
+
         layers = SemanticLayerDAO.find_all()
         result = [_serialize_layer(layer) for layer in layers]
         return self.response(200, result=result)
@@ -1186,6 +1235,9 @@ class SemanticLayerRestApi(BaseSupersetApi):
             404:
               $ref: '#/components/responses/404'
         """
+        if not is_feature_enabled("SEMANTIC_LAYERS"):
+            return self.response_404()
+
         layer = SemanticLayerDAO.find_by_uuid(uuid)
         if not layer:
             return self.response_404()

tests/unit_tests/commands/semantic_layer/update_test.py

@@ -28,10 +28,13 @@ from superset.commands.semantic_layer.exceptions import (
     SemanticViewNotFoundError,
 )
 from superset.commands.semantic_layer.update import (
+    _unmask_configuration,
     UpdateSemanticLayerCommand,
     UpdateSemanticViewCommand,
 )
+from superset.constants import PASSWORD_MASK
 from superset.exceptions import SupersetSecurityException
+from superset.utils import json
 
 
 def test_update_semantic_view_success(mocker: MockerFixture) -> None:
@@ -463,3 +466,110 @@ def test_update_uniqueness_same_config_same_name_fails(
         layer_uuid="layer-uuid-1",
         configuration={"schema": "prod"},
     )
+
+
+# =============================================================================
+# _unmask_configuration tests
+# =============================================================================
+
+
+def test_unmask_configuration_restores_masked_secret() -> None:
+    """A masked write-only field in the payload is replaced by the stored
+    value rather than overwriting the real credential with the mask."""
+    result = _unmask_configuration(
+        '{"account": "test", "password": "hunter2"}',
+        {"account": "test", "password": PASSWORD_MASK},
+    )
+
+    assert result == {"account": "test", "password": "hunter2"}
+
+
+def test_unmask_configuration_keeps_fresh_secret() -> None:
+    """A genuinely new secret value (not the mask sentinel) passes through
+    unchanged."""
+    result = _unmask_configuration(
+        '{"account": "test", "password": "old-secret"}',
+        {"account": "test", "password": "new-secret"},
+    )
+
+    assert result == {"account": "test", "password": "new-secret"}
+
+
+def test_unmask_configuration_restores_fail_closed_masked_fields() -> None:
+    """When the read path fell back to masking every value (schema
+    unavailable at GET time), the update path must restore all of them on
+    round-trip, not just write-only ones -- otherwise a name-only save
+    persists the literal mask into non-secret fields like ``account`` once
+    the schema becomes available again."""
+    result = _unmask_configuration(
+        '{"account": "test", "database": "prod", "password": "hunter2"}',
+        {
+            "account": PASSWORD_MASK,
+            "database": PASSWORD_MASK,
+            "password": PASSWORD_MASK,
+        },
+    )
+
+    assert result == {
+        "account": "test",
+        "database": "prod",
+        "password": "hunter2",
+    }
+
+
+def test_unmask_configuration_missing_existing_key() -> None:
+    """A masked field with no corresponding stored value passes through
+    unchanged rather than raising."""
+    result = _unmask_configuration(
+        '{"account": "test"}',
+        {"account": "test", "password": PASSWORD_MASK},
+    )
+
+    assert result == {"account": "test", "password": PASSWORD_MASK}
+
+
+def test_update_semantic_layer_preserves_masked_secret_end_to_end(
+    mocker: MockerFixture,
+) -> None:
+    """A name-only PUT that round-trips the masked GET response does not
+    overwrite the stored credential with the literal mask."""
+    mock_model = MagicMock()
+    mock_model.type = "snowflake"
+    mock_model.configuration = '{"account": "test", "password": "hunter2"}'
+
+    dao = mocker.patch(
+        "superset.commands.semantic_layer.update.SemanticLayerDAO",
+    )
+    dao.find_by_uuid.return_value = mock_model
+    dao.update.return_value = mock_model
+
+    mocker.patch(
+        "superset.commands.semantic_layer.update.current_user_can_modify_object",
+    )
+
+    mock_cls = MagicMock()
+    mock_cls.get_configuration_schema.return_value = {
+        "properties": {"password": {"type": "string", "writeOnly": True}}
+    }
+    mocker.patch.dict(
+        "superset.commands.semantic_layer.update.registry",
+        {"snowflake": mock_cls},
+        clear=True,
+    )
+
+    data = {
+        "name": "Renamed",
+        "configuration": {"account": "test", "password": PASSWORD_MASK},
+    }
+    UpdateSemanticLayerCommand("some-uuid", data).run()
+
+    mock_cls.from_configuration.assert_called_once_with(
+        {"account": "test", "password": "hunter2"}
+    )
+    dao.update.assert_called_once_with(
+        mock_model,
+        attributes={
+            "name": "Renamed",
+            "configuration": json.dumps({"account": "test", "password": "hunter2"}),
+        },
+    )

tests/unit_tests/semantic_layers/api_test.py

@@ -36,9 +36,14 @@ from superset.commands.semantic_layer.exceptions import (
     SemanticViewNotFoundError,
     SemanticViewUpdateFailedError,
 )
+from superset.constants import PASSWORD_MASK
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
 from superset.exceptions import SupersetSecurityException
-from superset.semantic_layers.api import SemanticLayerRestApi, SemanticViewRestApi
+from superset.semantic_layers.api import (
+    _mask_configuration,
+    SemanticLayerRestApi,
+    SemanticViewRestApi,
+)
 
 SEMANTIC_LAYERS_APP = pytest.mark.parametrize(
     "app",
@@ -618,6 +623,21 @@ def test_runtime_schema_exception(
     assert "Bad config" in response.json["message"]
 
 
+@pytest.mark.parametrize(
+    "app",
+    [{"FEATURE_FLAGS": {"SEMANTIC_LAYERS": False}}],
+    indirect=True,
+)
+def test_runtime_schema_flag_off_returns_404(
+    client: Any,
+    full_api_access: None,
+) -> None:
+    response = client.post(
+        f"/api/v1/semantic_layer/{uuid_lib.uuid4()}/schema/runtime",
+    )
+    assert response.status_code == 404
+
+
 @SEMANTIC_LAYERS_APP
 def test_post_semantic_layer(
     client: Any,
@@ -937,6 +957,11 @@ def test_get_list_semantic_layers(
     layer2.configuration = '{"account": "test"}'
     layer2.changed_on_delta_humanized.return_value = "2 hours ago"
 
+    mocker.patch.dict(
+        "superset.semantic_layers.api.registry",
+        {"snowflake": MagicMock(get_configuration_schema=lambda: {"properties": {}})},
+        clear=True,
+    )
     mock_dao = mocker.patch("superset.semantic_layers.api.SemanticLayerDAO")
     mock_dao.find_all.return_value = [layer1, layer2]
 
@@ -967,6 +992,19 @@ def test_get_list_semantic_layers_empty(
     assert response.json["result"] == []
 
 
+@pytest.mark.parametrize(
+    "app",
+    [{"FEATURE_FLAGS": {"SEMANTIC_LAYERS": False}}],
+    indirect=True,
+)
+def test_get_list_semantic_layers_flag_off_returns_404(
+    client: Any,
+    full_api_access: None,
+) -> None:
+    response = client.get("/api/v1/semantic_layer/")
+    assert response.status_code == 404
+
+
 @SEMANTIC_LAYERS_APP
 def test_get_semantic_layer(
     client: Any,
@@ -984,6 +1022,11 @@ def test_get_semantic_layer(
     layer.configuration = '{"account": "test"}'
     layer.changed_on_delta_humanized.return_value = "1 day ago"
 
+    mocker.patch.dict(
+        "superset.semantic_layers.api.registry",
+        {"snowflake": MagicMock(get_configuration_schema=lambda: {"properties": {}})},
+        clear=True,
+    )
     mock_dao = mocker.patch("superset.semantic_layers.api.SemanticLayerDAO")
     mock_dao.find_by_uuid.return_value = layer
 
@@ -1013,6 +1056,19 @@ def test_get_semantic_layer_not_found(
     assert response.status_code == 404
 
 
+@pytest.mark.parametrize(
+    "app",
+    [{"FEATURE_FLAGS": {"SEMANTIC_LAYERS": False}}],
+    indirect=True,
+)
+def test_get_semantic_layer_flag_off_returns_404(
+    client: Any,
+    full_api_access: None,
+) -> None:
+    response = client.get(f"/api/v1/semantic_layer/{uuid_lib.uuid4()}")
+    assert response.status_code == 404
+
+
 @SEMANTIC_LAYERS_APP
 def test_get_semantic_layer_forbidden(
     client: Any,
@@ -1056,6 +1112,11 @@ def test_serialize_layer_string_config(
     layer.configuration = '{"account": "test"}'
     layer.changed_on_delta_humanized.return_value = "1 day ago"
 
+    mocker.patch.dict(
+        "superset.semantic_layers.api.registry",
+        {"snowflake": MagicMock(get_configuration_schema=lambda: {"properties": {}})},
+        clear=True,
+    )
     mock_dao = mocker.patch("superset.semantic_layers.api.SemanticLayerDAO")
     mock_dao.find_by_uuid.return_value = layer
 
@@ -1081,6 +1142,11 @@ def test_serialize_layer_dict_config(
     layer.configuration = {"account": "test"}
     layer.changed_on_delta_humanized.return_value = "1 day ago"
 
+    mocker.patch.dict(
+        "superset.semantic_layers.api.registry",
+        {"snowflake": MagicMock(get_configuration_schema=lambda: {"properties": {}})},
+        clear=True,
+    )
     mock_dao = mocker.patch("superset.semantic_layers.api.SemanticLayerDAO")
     mock_dao.find_by_uuid.return_value = layer
 
@@ -1115,6 +1181,141 @@ def test_serialize_layer_none_config(
     assert response.json["result"]["configuration"] == {}
 
 
+def test_mask_configuration_redacts_write_only_fields(mocker: MockerFixture) -> None:
+    """Test _mask_configuration redacts properties the schema marks writeOnly."""
+    layer = MagicMock()
+    layer.type = "snowflake"
+
+    mock_cls = MagicMock()
+    mock_cls.get_configuration_schema.return_value = {
+        "properties": {
+            "account": {"type": "string"},
+            "password": {"type": "string", "writeOnly": True},
+        },
+    }
+    mocker.patch.dict(
+        "superset.semantic_layers.api.registry",
+        {"snowflake": mock_cls},
+        clear=True,
+    )
+
+    result = _mask_configuration(layer, {"account": "test", "password": "hunter2"})
+
+    assert result == {"account": "test", "password": PASSWORD_MASK}
+
+
+def test_mask_configuration_skips_falsy_secret_values(
+    mocker: MockerFixture,
+) -> None:
+    """Test _mask_configuration leaves an unset write-only field alone."""
+    layer = MagicMock()
+    layer.type = "snowflake"
+
+    mock_cls = MagicMock()
+    mock_cls.get_configuration_schema.return_value = {
+        "properties": {"password": {"type": "string", "writeOnly": True}},
+    }
+    mocker.patch.dict(
+        "superset.semantic_layers.api.registry",
+        {"snowflake": mock_cls},
+        clear=True,
+    )
+
+    result = _mask_configuration(layer, {"password": ""})
+
+    assert result == {"password": ""}
+
+
+def test_mask_configuration_no_write_only_properties(mocker: MockerFixture) -> None:
+    """Test _mask_configuration is a no-op when the schema has no writeOnly fields."""
+    layer = MagicMock()
+    layer.type = "snowflake"
+
+    mock_cls = MagicMock()
+    mock_cls.get_configuration_schema.return_value = {
+        "properties": {"account": {"type": "string"}},
+    }
+    mocker.patch.dict(
+        "superset.semantic_layers.api.registry",
+        {"snowflake": mock_cls},
+        clear=True,
+    )
+
+    config = {"account": "test"}
+    result = _mask_configuration(layer, config)
+
+    assert result is config
+
+
+def test_mask_configuration_no_registered_class(mocker: MockerFixture) -> None:
+    """Test _mask_configuration fails closed when the type has no connector."""
+    layer = MagicMock()
+    layer.type = "unregistered"
+
+    mocker.patch.dict("superset.semantic_layers.api.registry", {}, clear=True)
+
+    config = {"account": "test", "password": "hunter2"}
+    result = _mask_configuration(layer, config)
+
+    assert result == {"account": PASSWORD_MASK, "password": PASSWORD_MASK}
+
+
+def test_mask_configuration_schema_error(mocker: MockerFixture) -> None:
+    """Test _mask_configuration fails closed if the schema can't load."""
+    layer = MagicMock()
+    layer.type = "snowflake"
+
+    mock_cls = MagicMock()
+    mock_cls.get_configuration_schema.side_effect = ValueError("boom")
+    mocker.patch.dict(
+        "superset.semantic_layers.api.registry",
+        {"snowflake": mock_cls},
+        clear=True,
+    )
+
+    config = {"account": "test", "password": "hunter2"}
+    result = _mask_configuration(layer, config)
+
+    assert result == {"account": PASSWORD_MASK, "password": PASSWORD_MASK}
+
+
+@SEMANTIC_LAYERS_APP
+def test_get_semantic_layer_masks_write_only_configuration(
+    client: Any,
+    full_api_access: None,
+    mocker: MockerFixture,
+) -> None:
+    """Test GET /<uuid> redacts write-only configuration fields."""
+    layer = MagicMock()
+    layer.uuid = uuid_lib.uuid4()
+    layer.name = "Layer"
+    layer.description = None
+    layer.type = "snowflake"
+    layer.cache_timeout = None
+    layer.configuration = {"account": "test", "password": "hunter2"}
+    layer.changed_on_delta_humanized.return_value = "1 day ago"
+
+    mock_dao = mocker.patch("superset.semantic_layers.api.SemanticLayerDAO")
+    mock_dao.find_by_uuid.return_value = layer
+
+    mock_cls = MagicMock()
+    mock_cls.get_configuration_schema.return_value = {
+        "properties": {"password": {"type": "string", "writeOnly": True}},
+    }
+    mocker.patch.dict(
+        "superset.semantic_layers.api.registry",
+        {"snowflake": mock_cls},
+        clear=True,
+    )
+
+    response = client.get(f"/api/v1/semantic_layer/{layer.uuid}")
+
+    assert response.status_code == 200
+    configuration = response.json["result"]["configuration"]
+    assert configuration["account"] == "test"
+    assert configuration["password"] == PASSWORD_MASK
+
+
 def test_infer_discriminators_injects_discriminator() -> None:
     """Test _infer_discriminators injects discriminator values."""
     from superset.semantic_layers.api import _infer_discriminators
@@ -2488,3 +2689,51 @@ def test_semantic_layer_views_flag_off_unwrapped() -> None:
 
     assert response == ("404", 404)
     api.response_404.assert_called_once()
+
+
+def test_semantic_layer_get_list_flag_off_unwrapped() -> None:
+    """Cover get_list() feature-flag guard without auth decorators."""
+    api = SemanticLayerRestApi()
+    api.response_404 = MagicMock(return_value=("404", 404))
+    get_list_fn = inspect.unwrap(SemanticLayerRestApi.get_list)
+
+    with patch(
+        "superset.semantic_layers.api.is_feature_enabled",
+        return_value=False,
+    ):
+        response = get_list_fn(api)
+
+    assert response == ("404", 404)
+    api.response_404.assert_called_once()
+
+
+def test_semantic_layer_get_flag_off_unwrapped() -> None:
+    """Cover get() feature-flag guard without auth decorators."""
+    api = SemanticLayerRestApi()
+    api.response_404 = MagicMock(return_value=("404", 404))
+    get_fn = inspect.unwrap(SemanticLayerRestApi.get)
+
+    with patch(
+        "superset.semantic_layers.api.is_feature_enabled",
+        return_value=False,
+    ):
+        response = get_fn(api, str(uuid_lib.uuid4()))
+
+    assert response == ("404", 404)
+    api.response_404.assert_called_once()
+
+
+def test_semantic_layer_runtime_schema_flag_off_unwrapped() -> None:
+    """Cover runtime_schema() feature-flag guard without auth decorators."""
+    api = SemanticLayerRestApi()
+    api.response_404 = MagicMock(return_value=("404", 404))
+    runtime_schema_fn = inspect.unwrap(SemanticLayerRestApi.runtime_schema)
+
+    with patch(
+        "superset.semantic_layers.api.is_feature_enabled",
+        return_value=False,
+    ):
+        response = runtime_schema_fn(api, str(uuid_lib.uuid4()))
+
+    assert response == ("404", 404)
+    api.response_404.assert_called_once()