fix(db_engine_specs): always mask OAuth2 client secret in encrypted extra (#43491)

superset/db_engine_specs/base.py

@@ -746,18 +746,18 @@ class BaseEngineSpec:  # pylint: disable=too-many-public-methods
     @classmethod
     def encrypted_extra_sensitive_field_paths(cls) -> set[str]:
         """
-        Returns a set of paths for fields that should be masked in the
-        ``masked_encrypted_extra`` JSON.
+        Returns a set of JSONPath expressions for fields that should be masked
+        in the ``masked_encrypted_extra`` JSON.
 
-        :param cls: Description
-        :return: Description
-        :rtype: set[str]
+        The OAuth2 client secret is always included, since
+        ``Database.get_oauth2_config`` reads ``oauth2_client_info`` from the
+        ``encrypted_extra`` of any database regardless of its engine, so engine
+        specs that override ``encrypted_extra_sensitive_fields`` cannot
+        accidentally expose it.
         """
-        return (
-            set(cls.encrypted_extra_sensitive_fields)
-            if isinstance(cls.encrypted_extra_sensitive_fields, dict)
-            else cls.encrypted_extra_sensitive_fields
-        )
+        return set(cls.encrypted_extra_sensitive_fields) | {
+            "$.oauth2_client_info.secret"
+        }
 
     @classmethod
     def get_rls_method(cls) -> RLSMethod:
@@ -2870,7 +2870,7 @@ class BaseEngineSpec:  # pylint: disable=too-many-public-methods
         corresponding entry is updated, otherwise the old value is used (see
         `unmask_encrypted_extra` below).
         """
-        if encrypted_extra is None or not cls.encrypted_extra_sensitive_fields:
+        if encrypted_extra is None:
             return encrypted_extra
 
         try:

superset/utils/json.py

@@ -304,8 +304,12 @@ def reveal_sensitive(
         jsonpath_expr = parse(json_path)
         for match in jsonpath_expr.find(revealed_payload):
             if match.value == PASSWORD_MASK:
-                old_value = match.full_path.find(old_payload)
-                match.context.value[match.path.fields[0]] = old_value[0].value
+                # a masked value can arrive for a path the stored payload never
+                # had, e.g. a payload copied from another connection. There is
+                # nothing to reveal, so the mask is passed through, which is
+                # what engine specs that do not list the path already do.
+                if old_value := match.full_path.find(old_payload):
+                    match.context.value[match.path.fields[0]] = old_value[0].value
 
     return revealed_payload
 

tests/unit_tests/db_engine_specs/test_base.py

@@ -366,6 +366,66 @@ def test_unmask_encrypted_extra() -> None:
     )
 
 
+def test_mask_encrypted_extra_oauth2_client_info_with_narrow_override() -> None:
+    """
+    Test that the OAuth2 client secret is masked even when an engine spec
+    overrides `encrypted_extra_sensitive_fields` without including it.
+    """
+
+    class NarrowFieldsSpec(BaseEngineSpec):
+        encrypted_extra_sensitive_fields = {"$.auth_params.password"}
+
+    config = json.dumps(
+        {
+            "auth_params": {"password": "my_password"},
+            "oauth2_client_info": {
+                "id": "my_client_id",
+                "secret": "my_client_secret",
+            },
+        }
+    )
+
+    assert NarrowFieldsSpec.mask_encrypted_extra(config) == json.dumps(
+        {
+            "auth_params": {"password": "XXXXXXXXXX"},
+            "oauth2_client_info": {
+                "id": "my_client_id",
+                "secret": "XXXXXXXXXX",
+            },
+        }
+    )
+
+
+def test_mask_encrypted_extra_oauth2_client_info_without_sensitive_fields() -> None:
+    """
+    Test that the OAuth2 client secret is masked even when an engine spec
+    declares no sensitive fields at all.
+    """
+
+    class NoFieldsSpec(BaseEngineSpec):
+        encrypted_extra_sensitive_fields: set[str] = set()
+
+    config = json.dumps(
+        {
+            "auth_params": {"password": "my_password"},
+            "oauth2_client_info": {
+                "id": "my_client_id",
+                "secret": "my_client_secret",
+            },
+        }
+    )
+
+    assert NoFieldsSpec.mask_encrypted_extra(config) == json.dumps(
+        {
+            "auth_params": {"password": "my_password"},
+            "oauth2_client_info": {
+                "id": "my_client_id",
+                "secret": "XXXXXXXXXX",
+            },
+        }
+    )
+
+
 @pytest.mark.parametrize(
     "masked_encrypted_extra,expected_result",
     [
@@ -377,6 +437,7 @@ def test_unmask_encrypted_extra() -> None:
             {
                 "$.credentials_info.private_key",
                 "$.access_token",
+                "$.oauth2_client_info.secret",
             },
         ),
         (
@@ -387,11 +448,12 @@ def test_unmask_encrypted_extra() -> None:
             {
                 "$.credentials_info.private_key",
                 "$.access_token",
+                "$.oauth2_client_info.secret",
             },
         ),
         (
             None,
-            {"$.*"},
+            {"$.*", "$.oauth2_client_info.secret"},
         ),
     ],
 )

tests/unit_tests/utils/json_tests.py

@@ -403,6 +403,27 @@ def test_get_masked_fields(
     assert sorted(masked) == sorted(expected_result)
 
 
+def test_reveal_sensitive_missing_in_old_payload() -> None:
+    """
+    Test that a masked value with no counterpart in the old payload is passed
+    through, matching what engine specs that do not list the path already do.
+    """
+    old_payload = {"foo": "bar"}
+    new_payload = {
+        "foo": "bar",
+        "oauth2_client_info": {"secret": PASSWORD_MASK},
+    }
+
+    assert json.reveal_sensitive(
+        old_payload,
+        new_payload,
+        {"$.oauth2_client_info.secret"},
+    ) == {
+        "foo": "bar",
+        "oauth2_client_info": {"secret": PASSWORD_MASK},
+    }
+
+
 def test_format_timedelta():
     assert json.format_timedelta(timedelta(0)) == "0:00:00"
     assert json.format_timedelta(timedelta(days=1)) == "1 day, 0:00:00"