From ecbd6578b0d8d2b9671c2edb65551b2bb8b79dcd Mon Sep 17 00:00:00 2001 From: Gabriel Torres Ruiz Date: Thu, 27 Aug 2026 18:42:51 -0300 Subject: [PATCH] fix(db_engine_specs): always mask OAuth2 client secret in encrypted extra (#43491) --- superset/db_engine_specs/base.py | 22 +++---- superset/utils/json.py | 8 ++- tests/unit_tests/db_engine_specs/test_base.py | 64 ++++++++++++++++++- tests/unit_tests/utils/json_tests.py | 21 ++++++ 4 files changed, 101 insertions(+), 14 deletions(-) diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py index 9f156c2d4ef..14fcce0642c 100644 --- a/superset/db_engine_specs/base.py +++ b/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: diff --git a/superset/utils/json.py b/superset/utils/json.py index e482248b540..e311b2ea269 100644 --- a/superset/utils/json.py +++ b/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 diff --git a/tests/unit_tests/db_engine_specs/test_base.py b/tests/unit_tests/db_engine_specs/test_base.py index e4e495ff58c..ece1bb2c020 100644 --- a/tests/unit_tests/db_engine_specs/test_base.py +++ b/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"}, ), ], ) diff --git a/tests/unit_tests/utils/json_tests.py b/tests/unit_tests/utils/json_tests.py index 6187f00ce3e..660c9020443 100644 --- a/tests/unit_tests/utils/json_tests.py +++ b/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"