From bf29ecf1153f2d4e266e21557191eff62cc882d3 Mon Sep 17 00:00:00 2001 From: Yucheng Zhu Date: Tue, 1 Sep 2026 14:22:32 -0700 Subject: [PATCH] fix(team): let one integration cover both callback events The family rule compared variable names only, so a team admin who registered an integration for the success event could not register the same integration, with the same values, for the failure event. Compare the values as well: repeating what the owning entry already stores flattens to the same dict, so there is nothing to redirect. The stored side is decrypted first, because the credentials are encrypted at rest and ciphertext never equals the plaintext coming in. --- .../callback_config_validation.py | 27 +++++++++++++------ .../team_callback_endpoints.py | 11 +++++--- .../test_team_callback_endpoints.py | 5 ++++ 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/litellm/proxy/common_utils/callback_config_validation.py b/litellm/proxy/common_utils/callback_config_validation.py index c13e07c00aa..805c8897b9d 100644 --- a/litellm/proxy/common_utils/callback_config_validation.py +++ b/litellm/proxy/common_utils/callback_config_validation.py @@ -77,7 +77,7 @@ def cross_entry_family_error( callback_vars: Mapping[str, str] | None, stored_vars_by_entry: Sequence[Mapping[str, str]], ) -> str | None: - """Reject an entry that joins a credential family another entry already holds. + """Reject an entry that changes a credential family another entry already holds. Every stored entry's variables are flattened into one dict before a request reads them, and the flattened dict is what the exporter authenticates and @@ -86,20 +86,31 @@ def cross_entry_family_error( with the key from the first, and the request carries that key to the new host. - Requiring one entry to own a family end to end removes the pairing. Only the - writers this endpoint newly admits are held to it, because a proxy admin - already holds every credential the proxy has. A team admin who does want to - move a family deletes the entry holding it first, which reveals nothing. + Requiring one entry to own a family end to end removes the pairing. Repeating + a value the holding entry already stores is allowed, because flattening then + produces the same dict either way -- that is how the same integration gets + registered for both the success and the failure event. A team admin who does + want to move a family deletes the entry holding it first, which reveals + nothing. + + Only the writers this endpoint newly admits are held to this, because a proxy + admin already holds every credential the proxy has. + + ``stored_vars_by_entry`` has to arrive decrypted; the credential values are + encrypted at rest and ciphertext never equals the plaintext coming in. """ if not callback_vars: return None - held: Final = {family for entry in stored_vars_by_entry for family in map(_family_of, entry) if family is not None} + stored: Final = { # mutable-ok: local view of the stored entries, never stored + var: value for entry in stored_vars_by_entry for var, value in entry.items() if _family_of(var) is not None + } + held: Final = {_family_of(var) for var in stored} # mutable-ok: local index, never stored return next( ( f"{family} is already configured by another callback entry on this team. " f"Remove that entry before setting {var} here." - for var, family in ((v, _family_of(v)) for v in callback_vars) - if family is not None and family in held + for var, value, family in ((v, callback_vars[v], _family_of(v)) for v in callback_vars) + if family in held and stored.get(var) != value ), None, ) diff --git a/litellm/proxy/management_endpoints/team_callback_endpoints.py b/litellm/proxy/management_endpoints/team_callback_endpoints.py index c9cedd47e26..136ccdac4da 100644 --- a/litellm/proxy/management_endpoints/team_callback_endpoints.py +++ b/litellm/proxy/management_endpoints/team_callback_endpoints.py @@ -347,11 +347,16 @@ async def add_team_callbacks( # flattened into one dict before a request reads them, so an entry # naming only a destination would pair with a key written on another # entry and carry it to that destination -- a key a team admin can read - # back nowhere. Proxy admins are exempt: they already hold every - # credential the proxy has. + # back nowhere. Repeating a value the owning entry already stores is + # fine, which is how one integration covers both events. Proxy admins + # are exempt: they already hold every credential the proxy has. if user_api_key_dict.user_role != LitellmUserRoles.PROXY_ADMIN: + # Decrypted, because the check compares the incoming values against + # the stored ones and the credentials are encrypted at rest. + decrypted_logging: Final = decrypt_callback_vars(team_metadata).get("logging") + stored_entries: Final = decrypted_logging if isinstance(decrypted_logging, list) else () stored_entry_vars: Final = [ # mutable-ok: read-only input to the check, never stored - entry.get("callback_vars") or {} for entry in team_callback_settings + entry.get("callback_vars") or {} for entry in stored_entries ] family_error: Final = cross_entry_family_error(data.callback_vars, stored_entry_vars) if family_error is not None: diff --git a/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py index 4f4904b97d9..7b812fd6e61 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py @@ -1538,6 +1538,11 @@ async def test_proxy_admin_still_told_the_team_is_unknown(): ({"langsmith_api_key": "k"}, [{"dd_api_key": "k"}], False), # variables that configure no backend carry nothing to redirect ({"turn_off_message_logging": "true"}, [{"langfuse_secret_key": "sk"}], False), + # the same integration registered for a second event: identical values + # flatten to the identical dict, so there is nothing to redirect + ({"langfuse_host": "https://us.cloud.langfuse.com", "langfuse_public_key": "pk", "langfuse_secret_key": "sk"}, [{"langfuse_host": "https://us.cloud.langfuse.com", "langfuse_public_key": "pk", "langfuse_secret_key": "sk"}], False), + # the same shape with one value moved is the redirect again + ({"langfuse_host": "http://attacker.invalid", "langfuse_public_key": "pk", "langfuse_secret_key": "sk"}, [{"langfuse_host": "https://us.cloud.langfuse.com", "langfuse_public_key": "pk", "langfuse_secret_key": "sk"}], True), ], ) def test_one_entry_owns_a_credential_family(new_vars, stored, rejected):