From f63782f67877fe675c6997f82855bc4049fbd6ff Mon Sep 17 00:00:00 2001 From: yucheng Date: Sat, 19 Sep 2026 23:16:54 +0000 Subject: [PATCH] fix(otel v2): reject a langfuse_span_scope that conflicts with another callback entry on the same team or key Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../callback_config_validation.py | 44 +++++++++++++++--- .../team_callback_endpoints.py | 18 +++++--- .../test_callback_config_validation.py | 45 +++++++++++++++++++ .../test_team_callback_endpoints.py | 45 +++++++++++++++++++ .../src/components/team/LoggingSettings.tsx | 4 +- 5 files changed, 141 insertions(+), 15 deletions(-) diff --git a/litellm/proxy/common_utils/callback_config_validation.py b/litellm/proxy/common_utils/callback_config_validation.py index 8a6b554c56a..8b98c6d96e7 100644 --- a/litellm/proxy/common_utils/callback_config_validation.py +++ b/litellm/proxy/common_utils/callback_config_validation.py @@ -157,6 +157,25 @@ def cross_entry_family_error( ) +def conflicting_span_scope_error( + callback_vars: Mapping[str, str] | None, + stored_vars_by_entry: Sequence[Mapping[str, str]], +) -> str | None: + """Reject a ``langfuse_span_scope`` another entry already sets differently; the entries flatten last-wins.""" + incoming: Final = None if callback_vars is None else callback_vars.get(_LANGFUSE_SPAN_SCOPE_VAR) + if incoming is None: + return None + return next( + ( + f"{_LANGFUSE_SPAN_SCOPE_VAR} is already set to {stored!r} by another callback entry. " + f"Every entry shares one scope: remove that entry or send the same value." + for entry in stored_vars_by_entry + if (stored := entry.get(_LANGFUSE_SPAN_SCOPE_VAR)) not in (None, incoming) + ), + None, + ) + + def logging_metadata_config_error(metadata: Mapping[str, object] | None) -> str | None: """Validate every ``logging`` entry of a team/key metadata payload.""" if not metadata: @@ -164,23 +183,34 @@ def logging_metadata_config_error(metadata: Mapping[str, object] | None) -> str entries: Final = metadata.get("logging") if not isinstance(entries, Sequence) or isinstance(entries, (str, bytes)): return None + entry_vars: Final = tuple(_entry_callback_vars(entry) for entry in entries) return next( - (error for error in (_logging_entry_error(entry) for entry in entries) if error is not None), + ( + error + for error in ( + *(_logging_entry_error(entry) for entry in entries), + *(conflicting_span_scope_error(entry_vars[i], entry_vars[:i]) for i in range(len(entry_vars))), + ) + if error is not None + ), None, ) +def _entry_callback_vars(entry: object) -> Mapping[str, str]: + callback_vars: Final = entry.get("callback_vars") if isinstance(entry, Mapping) else None + if not isinstance(callback_vars, Mapping): + return MappingProxyType({}) + return MappingProxyType({str(key): str(value) for key, value in callback_vars.items()}) + + def _logging_entry_error(entry: object) -> str | None: if not isinstance(entry, Mapping): return None callback_name: Final = entry.get("callback_name") - callback_vars: Final = entry.get("callback_vars") - if not isinstance(callback_name, str) or not isinstance(callback_vars, Mapping): + if not isinstance(callback_name, str) or not isinstance(entry.get("callback_vars"), Mapping): return None - return callback_config_error( - callback_name, - MappingProxyType({str(key): str(value) for key, value in callback_vars.items()}), - ) + return callback_config_error(callback_name, _entry_callback_vars(entry)) def _newrelic_config_error(callback_vars: Mapping[str, str]) -> str | None: diff --git a/litellm/proxy/management_endpoints/team_callback_endpoints.py b/litellm/proxy/management_endpoints/team_callback_endpoints.py index f7ae1eec06e..ac13b6150b7 100644 --- a/litellm/proxy/management_endpoints/team_callback_endpoints.py +++ b/litellm/proxy/management_endpoints/team_callback_endpoints.py @@ -31,6 +31,7 @@ from litellm.proxy._types import ( from litellm.proxy.auth.user_api_key_auth import user_api_key_auth from litellm.proxy.common_utils.callback_config_validation import ( callback_config_error, + conflicting_span_scope_error, cross_entry_family_error, ) from litellm.proxy.common_utils.callback_utils import ( @@ -344,6 +345,16 @@ async def add_team_callbacks( if team_callback_settings is None or not isinstance(team_callback_settings, list): team_callback_settings = [] + # Decrypted, because the checks compare 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 checks, never stored + entry.get("callback_vars") or {} for entry in stored_entries + ] + scope_error: Final = conflicting_span_scope_error(data.callback_vars, stored_entry_vars) + if scope_error is not None: + raise _callback_config_error(scope_error) # One entry has to own a credential family end to end. The entries are # flattened into one dict before a request reads them, so an entry # naming only a destination would pair with a key written on another @@ -352,13 +363,6 @@ async def add_team_callbacks( # 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 stored_entries - ] family_error: Final = cross_entry_family_error(data.callback_vars, stored_entry_vars) if family_error is not None: raise HTTPException( diff --git a/tests/test_litellm/proxy/common_utils/test_callback_config_validation.py b/tests/test_litellm/proxy/common_utils/test_callback_config_validation.py index 418ce5c46ed..a707c92dc15 100644 --- a/tests/test_litellm/proxy/common_utils/test_callback_config_validation.py +++ b/tests/test_litellm/proxy/common_utils/test_callback_config_validation.py @@ -1,5 +1,9 @@ +import pytest + from litellm.proxy.common_utils.callback_config_validation import ( callback_config_error, + conflicting_span_scope_error, + logging_metadata_config_error, ) @@ -37,3 +41,44 @@ def test_a_bad_span_scope_is_reported_even_when_the_environment_is_fine(): "langfuse_otel", {"langfuse_environment": "team-a-prod", "langfuse_span_scope": "everything"} ) assert error is not None and "langfuse_span_scope" in error + + +@pytest.mark.parametrize( + "new_vars, stored, rejected", + [ + ({"langfuse_span_scope": "llm_only"}, [{"langfuse_span_scope": "full"}], True), + ({"langfuse_span_scope": "full"}, [{"langfuse_public_key": "pk"}, {"langfuse_span_scope": "llm_only"}], True), + ({"langfuse_span_scope": "llm_only"}, [{"langfuse_span_scope": "llm_only"}], False), + ({"langfuse_span_scope": "llm_only"}, [{"langfuse_public_key": "pk"}], False), + ({"langfuse_span_scope": "llm_only"}, [], False), + ({"langfuse_public_key": "pk"}, [{"langfuse_span_scope": "llm_only"}], False), + (None, [{"langfuse_span_scope": "llm_only"}], False), + ], +) +def test_one_span_scope_per_team(new_vars, stored, rejected): + """The entries flatten last-wins, so a second scope would export whichever entry + was stored last. An entry that names no scope leaves the stored one in charge.""" + error = conflicting_span_scope_error(new_vars, stored) + assert (error is not None) is rejected + if rejected: + assert "langfuse_span_scope" in error and stored[-1]["langfuse_span_scope"] in error + + +def test_key_logging_entries_may_not_disagree_on_the_span_scope(): + disagreeing = { + "logging": [ + {"callback_name": "langfuse_otel", "callback_type": "success", "callback_vars": {"langfuse_span_scope": "full"}}, + {"callback_name": "langfuse_otel", "callback_type": "failure", "callback_vars": {"langfuse_span_scope": "llm_only"}}, + ] + } + error = logging_metadata_config_error(disagreeing) + assert error is not None and "langfuse_span_scope" in error and "'full'" in error + + agreeing = { + "logging": [ + {"callback_name": "langfuse_otel", "callback_type": "success", "callback_vars": {"langfuse_span_scope": "llm_only"}}, + {"callback_name": "langfuse_otel", "callback_type": "failure", "callback_vars": {"langfuse_span_scope": "llm_only"}}, + {"callback_name": "otel", "callback_type": "success", "callback_vars": {}}, + ] + } + assert logging_metadata_config_error(agreeing) is 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 dfda61b6560..acc7c8ca21a 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 @@ -1565,3 +1565,48 @@ def test_one_entry_owns_a_credential_family(new_vars, stored, rejected): """ error = cross_entry_family_error(new_vars, stored) assert (error is not None) is rejected + + +@pytest.mark.asyncio +@pytest.mark.parametrize("caller", [_admin_auth(), UserAPIKeyAuth(user_role=LitellmUserRoles.INTERNAL_USER, user_id="victim_admin", api_key="sk-team-admin")]) +async def test_a_second_entry_may_not_flip_the_span_scope(patched_prisma, caller): + """The entries flatten last-wins at request time, so a failure entry saying + llm_only next to a success entry saying full would export whichever is stored + last. Neither a proxy admin nor a team admin gets to store the disagreement.""" + patched_prisma.get_data = AsyncMock( + return_value=_team_row( + metadata={ + "logging": [ + { + "callback_name": "langfuse_otel", + "callback_type": "success", + "callback_vars": {"langfuse_public_key": "pk", "langfuse_secret_key": "sk", "langfuse_span_scope": "full"}, + } + ] + } + ) + ) + data = AddTeamCallback( + callback_name="langfuse_otel", + callback_type="failure", + callback_vars={"langfuse_public_key": "pk", "langfuse_secret_key": "sk", "langfuse_span_scope": "llm_only"}, + ) + with pytest.raises(HTTPException) as exc: + await add_team_callbacks( + data=data, + http_request=Mock(spec=Request), + team_id="team-victim", + user_api_key_dict=caller, + ) + assert exc.value.status_code == 400 + assert "langfuse_span_scope" in str(exc.value.detail) and "'full'" in str(exc.value.detail) + patched_prisma.db.litellm_teamtable.update.assert_not_called() + + data.callback_vars["langfuse_span_scope"] = "full" + await add_team_callbacks( + data=data, + http_request=Mock(spec=Request), + team_id="team-victim", + user_api_key_dict=caller, + ) + patched_prisma.db.litellm_teamtable.update.assert_awaited_once() diff --git a/ui/litellm-dashboard/src/components/team/LoggingSettings.tsx b/ui/litellm-dashboard/src/components/team/LoggingSettings.tsx index e760939b3fe..2f66e68eca4 100644 --- a/ui/litellm-dashboard/src/components/team/LoggingSettings.tsx +++ b/ui/litellm-dashboard/src/components/team/LoggingSettings.tsx @@ -170,7 +170,9 @@ const LoggingSettings: React.FC = ({ width={400} placeholder={`os.environ/${paramName.toUpperCase()}`} value={config.callback_vars[paramName] || ""} - onChange={(e: any) => updateCallbackVar(configIndex, paramName, e.target.value)} + onChange={(e: React.ChangeEvent) => + updateCallbackVar(configIndex, paramName, e.target.value) + } /> ); }