diff --git a/litellm/integrations/otel/logger.py b/litellm/integrations/otel/logger.py index e273289168a..2c83406afed 100644 --- a/litellm/integrations/otel/logger.py +++ b/litellm/integrations/otel/logger.py @@ -33,6 +33,7 @@ from litellm.integrations.otel.model.payloads import ( is_mcp_list_tools, is_mcp_tool_call, ) +from litellm.integrations.otel.model.semconv import Error from litellm.integrations.otel.model.spans import SpanRole, span_role_for_service from litellm.integrations.otel.model.utils import to_ns from litellm.integrations.otel.plumbing.context import ( @@ -634,18 +635,23 @@ class OpenTelemetryV2(CustomLogger): """Stamp the v2 error.* attributes on the FastAPI-owned SERVER span for a failure that dies before any LLM-call span exists (malformed body, auth / validation rejection). Called from the proxy's global exception handler via - ``_close_dangling_otel_server_span``. The instrumentor still owns the span's - status and lifecycle, so this only decorates it — never sets status, never - ends it — and emits no exception event, matching v1's SERVER-span behavior - and avoiding a duplicate of the event ``async_post_call_failure_hook`` or - the ``auth`` phase span already records.""" + ``_close_dangling_otel_server_span``, which swallows the exception into a + ``JSONResponse`` so the instrumentor never sees it and leaves the span + ``UNSET``; the status is set here instead (v1 did the same from the handler) + so a failed request reads as failed and not merely as a span carrying an + error message. The instrumentor still owns the span's lifecycle, so this + never ends it. The exception event is recorded only when nothing stamped + this span already — ``async_post_call_failure_hook`` and the ``auth`` phase + span record their own, and a second event would duplicate it — while the + attributes are always restamped so ``error.code`` stays pinned to the real + response status.""" if span is None or not is_recordable_span(span): return + already_stamped: Final = Error.TYPE in (getattr(span, "attributes", None) or ()) stamp_error( span, _span_error_from_exception(exception, status_code=status_code), - record_event=False, - set_status=False, + record_event=not already_stamped, ) async def async_post_call_failure_hook( diff --git a/litellm/proxy/auth/user_api_key_auth.py b/litellm/proxy/auth/user_api_key_auth.py index 9dc450befc2..4baa7b99a4f 100644 --- a/litellm/proxy/auth/user_api_key_auth.py +++ b/litellm/proxy/auth/user_api_key_auth.py @@ -1044,6 +1044,22 @@ def _ensure_parent_otel_span_on_request_state(request: Request) -> None: request.state.parent_otel_span = parent_otel_span +async def _read_request_body_deferring_parse_failure( + request: Request, +) -> tuple[dict, ProxyException | None]: + """Parse the body, returning a parse failure instead of raising it. + + A body that fails to parse is still a request from a known caller, so auth + must run (resolving identity onto the request's trace) before the 400 goes + out; the caller re-raises the returned exception once identity is seeded. + """ + try: + parsed_body: Final = await _read_request_body(request=request) + except ProxyException as parse_exception: + return {}, parse_exception # mutable-ok: request_data is a plain dict across the whole auth path + return populate_request_with_path_params(request_data=parsed_body, request=request), None + + async def _user_api_key_auth_builder( request: Request, api_key: str, @@ -2516,6 +2532,72 @@ def _resolve_request_principal(request: Request, valid_token: UserAPIKeyAuth) -> ) +async def _authorize_authenticated_request( + user_api_key_auth_obj: UserAPIKeyAuth, + request: Request, + request_data: dict, + route: str, + api_key: str, +) -> UserAPIKeyAuth | None: + """Authorize an already-authenticated request: disabled-route check, the single + ``common_checks`` gate (which also reserves budget), and end-user fallback + resolution. Returns the auth object the exception handler recovered when a check + failed but the request may proceed anyway, else ``None``. + """ + ## ENSURE DISABLE ROUTE WORKS ACROSS ALL USER AUTH FLOWS ## + RouteChecks.should_call_route(route=route, valid_token=user_api_key_auth_obj, request=request) + + # Single authorization point. Builder paths MUST NOT call common_checks. + # Route through the same exception handler the builder uses so + # authorization failures (ProxyException, or plain Exception from + # admin-only-route / model-access / budget checks) surface as + # ProxyException consistently with pre-refactor behavior. + try: + await _run_centralized_common_checks( + user_api_key_auth_obj=user_api_key_auth_obj, + request=request, + request_data=request_data, + route=route, + ) + except Exception as e: + return await UserAPIKeyAuthExceptionHandler._handle_authentication_error( + e=e, + request=request, + request_data=request_data, + route=route, + parent_otel_span=user_api_key_auth_obj.parent_otel_span, + api_key=api_key, + resolved_identity=user_api_key_auth_obj, + ) + + # Defense-in-depth: ``_user_api_key_auth_builder`` has multiple early-return + # paths (no master key, /user/auth route, JWT short-circuits) that bypass + # the end-user resolution block. If those paths produced an auth obj + # without an ``end_user_id`` set, fall back to extracting from the request + # body so spend logs are still attributed correctly. Validation honours + # ``litellm.validate_end_user_id_in_db``. + if user_api_key_auth_obj.end_user_id is None: + from litellm.proxy.proxy_server import ( + prisma_client, + proxy_logging_obj, + user_api_key_cache, + ) + + raw_end_user_id: Final = get_end_user_id_from_request_body(request_data, _safe_get_request_headers(request)) + if raw_end_user_id is not None: + resolved_end_user_id: Final = await resolve_and_validate_end_user_id( + raw_end_user_id=raw_end_user_id, + prisma_client=prisma_client, + user_api_key_cache=user_api_key_cache, + parent_otel_span=user_api_key_auth_obj.parent_otel_span, + proxy_logging_obj=proxy_logging_obj, + route=route, + ) + if resolved_end_user_id is not None: + user_api_key_auth_obj.end_user_id = resolved_end_user_id + return None + + @tracer.wrap() async def user_api_key_auth( request: Request, @@ -2536,8 +2618,7 @@ async def user_api_key_auth( # close, and the trace never reaches the backend. _ensure_parent_otel_span_on_request_state(request) - request_data = await _read_request_body(request=request) - request_data = populate_request_with_path_params(request_data=request_data, request=request) + request_data, body_parse_exception = await _read_request_body_deferring_parse_failure(request=request) route: Final[str] = get_request_route(request=request) ## CHECK IF ROUTE IS ALLOWED @@ -2545,69 +2626,41 @@ async def user_api_key_auth( # triggers (key/user/team object reads) nest under it instead of flattening # onto the server span. No-op when OTel V2 isn't active. with phase_span(f"auth {route}"): - user_api_key_auth_obj: Final = await _user_api_key_auth_builder( - request=request, - api_key=api_key, - azure_api_key_header=azure_api_key_header, - anthropic_api_key_header=anthropic_api_key_header, - google_ai_studio_api_key_header=google_ai_studio_api_key_header, - azure_apim_header=azure_apim_header, - request_data=request_data, - custom_litellm_key_header=custom_litellm_key_header, - ) + try: + user_api_key_auth_obj: Final = await _user_api_key_auth_builder( + request=request, + api_key=api_key, + azure_api_key_header=azure_api_key_header, + anthropic_api_key_header=anthropic_api_key_header, + google_ai_studio_api_key_header=google_ai_studio_api_key_header, + azure_apim_header=azure_apim_header, + request_data=request_data, + custom_litellm_key_header=custom_litellm_key_header, + ) + except Exception: + # The body was read first, so a caller who sent both a malformed body and + # a rejected key used to get the 400; the response is unchanged, and the + # auth failure is still recorded on the trace by the handler that ran. + if body_parse_exception is not None: + raise body_parse_exception + raise user_api_key_auth_obj.budget_reservation = None - ## ENSURE DISABLE ROUTE WORKS ACROSS ALL USER AUTH FLOWS ## - RouteChecks.should_call_route(route=route, valid_token=user_api_key_auth_obj, request=request) - - # Single authorization point. Builder paths MUST NOT call common_checks. - # Route through the same exception handler the builder uses so - # authorization failures (ProxyException, or plain Exception from - # admin-only-route / model-access / budget checks) surface as - # ProxyException consistently with pre-refactor behavior. - try: - await _run_centralized_common_checks( + # A body that never parsed is authenticated (so the trace carries identity + # and this ``auth`` span) but not authorized: there is no model to check it + # against, and budget reservation would increment live spend counters that + # only the endpoint's post-call path releases; the endpoint never runs, since + # the parse failure is raised below. + if body_parse_exception is None: + recovered_auth_obj: Final = await _authorize_authenticated_request( user_api_key_auth_obj=user_api_key_auth_obj, request=request, request_data=request_data, route=route, - ) - except Exception as e: - return await UserAPIKeyAuthExceptionHandler._handle_authentication_error( - e=e, - request=request, - request_data=request_data, - route=route, - parent_otel_span=user_api_key_auth_obj.parent_otel_span, api_key=api_key, - resolved_identity=user_api_key_auth_obj, ) - - # Defense-in-depth: ``_user_api_key_auth_builder`` has multiple early-return - # paths (no master key, /user/auth route, JWT short-circuits) that bypass - # the end-user resolution block. If those paths produced an auth obj - # without an ``end_user_id`` set, fall back to extracting from the request - # body so spend logs are still attributed correctly. Validation honours - # ``litellm.validate_end_user_id_in_db``. - if user_api_key_auth_obj.end_user_id is None: - from litellm.proxy.proxy_server import ( - prisma_client, - proxy_logging_obj, - user_api_key_cache, - ) - - raw_end_user_id: Final = get_end_user_id_from_request_body(request_data, _safe_get_request_headers(request)) - if raw_end_user_id is not None: - resolved_end_user_id: Final = await resolve_and_validate_end_user_id( - raw_end_user_id=raw_end_user_id, - prisma_client=prisma_client, - user_api_key_cache=user_api_key_cache, - parent_otel_span=user_api_key_auth_obj.parent_otel_span, - proxy_logging_obj=proxy_logging_obj, - route=route, - ) - if resolved_end_user_id is not None: - user_api_key_auth_obj.end_user_id = resolved_end_user_id + if recovered_auth_obj is not None: + return recovered_auth_obj # Identity is now resolved. Seed it AFTER the auth span closes so the Baggage # persists on the request task (detaching the span's context token inside the @@ -2619,6 +2672,9 @@ async def user_api_key_auth( ) user_api_key_auth_obj.request_route = normalize_request_route(route) + if body_parse_exception is not None: + raise body_parse_exception + # Resolve caller identity once, here at the seam, into a single per-request # Principal projected off the key object the builder already fetched (no # second lookup). Downstream consumers read identity off this instead of diff --git a/tests/test_litellm/integrations/otel/test_otel_v2_logger.py b/tests/test_litellm/integrations/otel/test_otel_v2_logger.py index 2573ad5a375..82b074220fa 100644 --- a/tests/test_litellm/integrations/otel/test_otel_v2_logger.py +++ b/tests/test_litellm/integrations/otel/test_otel_v2_logger.py @@ -1195,8 +1195,13 @@ def test_async_post_call_failure_hook_skips_a_transport_that_already_answered(): def test_record_error_attributes_on_span_decorates_without_ending(): """PATH A: a failure that dies before any LLM-call span (malformed body, validation) is stamped onto the instrumentor-owned SERVER span. The method must - not end the span or emit a duplicate exception event, and must pin error.code - to the real response status (not the exception's own code).""" + not end the span, and must pin error.code to the real response status (not the + exception's own code). + + LIT-4780: the instrumentor never sees the exception (the proxy handler turns it + into a JSONResponse), so nothing else marks the span as failed; the status and + the exception event have to come from here or the trace shows the error message + on an otherwise successful-looking request.""" logger, exporter = _logger() server = logger._emitter.start_span(SpanRole.PROXY_REQUEST, LITELLM_PROXY_REQUEST_SPAN_NAME) logger.record_error_attributes_on_span(server, _proxy_exc("Invalid JSON body", 400), 422) @@ -1206,7 +1211,31 @@ def test_record_error_attributes_on_span_decorates_without_ending(): assert span.attributes["error.type"] == "ProxyException" assert span.attributes["error.message"] == "Invalid JSON body" assert span.attributes["litellm.provider.error.code"] == "422" - assert all(e.name != "exception" for e in span.events) + assert span.status.status_code is StatusCode.ERROR + assert [e.name for e in span.events] == ["exception"] + + +def test_record_error_attributes_on_span_does_not_duplicate_an_already_stamped_error(): + """A failure that already went through ``async_post_call_failure_hook`` reaches + the exception handler too; the second stamp must keep one exception event while + still repinning error.code to the real response status.""" + from litellm.proxy._types import UserAPIKeyAuth + + logger, exporter = _logger() + server = logger._emitter.start_span(SpanRole.PROXY_REQUEST, LITELLM_PROXY_REQUEST_SPAN_NAME) + set_request_root_span(server) + exc = _proxy_exc("Authentication Error, invalid key", 401) + asyncio.run( + logger.async_post_call_failure_hook( + request_data={}, original_exception=exc, user_api_key_dict=UserAPIKeyAuth() + ) + ) + logger.record_error_attributes_on_span(server, exc, 400) + server.end() + (span,) = exporter.get_finished_spans() + assert [e.name for e in span.events] == ["exception"] + assert span.attributes["litellm.provider.error.code"] == "400" + assert span.status.status_code is StatusCode.ERROR def test_record_error_attributes_on_span_ignores_below_400_and_missing_span(): diff --git a/tests/test_litellm/proxy/auth/test_user_api_key_auth.py b/tests/test_litellm/proxy/auth/test_user_api_key_auth.py index 6fc44ef7519..60d9689dc0b 100644 --- a/tests/test_litellm/proxy/auth/test_user_api_key_auth.py +++ b/tests/test_litellm/proxy/auth/test_user_api_key_auth.py @@ -4786,6 +4786,117 @@ async def test_user_api_key_auth_does_not_overwrite_end_user_id_set_by_builder() setattr(_proxy_server_mod, k, v) +@pytest.mark.asyncio +async def test_user_api_key_auth_authenticates_before_raising_malformed_body_error(): + """Regression (LIT-4780): a body that fails to parse must still be authenticated + first, so the rejected request's trace carries the caller's key / team / user + identity instead of an anonymous root span. The parse error is re-raised + unchanged once identity is seeded.""" + from fastapi import Request + from starlette.datastructures import URL + + import litellm.proxy.proxy_server as _proxy_server_mod + + builder_token = UserAPIKeyAuth(api_key="sk-test", user_id="u1", team_id="team-1") + + request = Request( + scope={ + "type": "http", + "headers": [(b"content-type", b"application/json")], + "method": "POST", + } + ) + request._url = URL(url="/chat/completions") + request._body = b'{}{"model": "gpt-4o"}' + + attrs = _proxy_attrs_for_centralized_checks(user_custom_auth=None) + originals = {a: getattr(_proxy_server_mod, a, None) for a in attrs} + try: + for k, v in attrs.items(): + setattr(_proxy_server_mod, k, v) + with ( + patch( + "litellm.proxy.auth.user_api_key_auth._user_api_key_auth_builder", + new_callable=AsyncMock, + return_value=builder_token, + ) as mock_builder, + patch( + "litellm.proxy.auth.user_api_key_auth._run_centralized_common_checks", + new_callable=AsyncMock, + ) as mock_common_checks, + patch( + "litellm.proxy.auth.user_api_key_auth.RouteChecks.should_call_route", + ), + patch( + "litellm.proxy.auth.user_api_key_auth.seed_request_identity", + ) as mock_seed, + ): + with pytest.raises(ProxyException) as exc_info: + await user_api_key_auth(request=request, api_key="Bearer sk-test") + + assert "Invalid JSON payload" in str(exc_info.value.message) + assert exc_info.value.code == str(status.HTTP_400_BAD_REQUEST) + mock_builder.assert_awaited_once() + assert mock_seed.call_args.args[0] is builder_token + # authorization must not run for a request that is about to be rejected: + # ``common_checks`` reserves budget against live spend counters that only the + # endpoint's post-call path releases, and the endpoint never runs here + mock_common_checks.assert_not_awaited() + finally: + for k, v in originals.items(): + setattr(_proxy_server_mod, k, v) + + +@pytest.mark.asyncio +async def test_user_api_key_auth_malformed_body_with_rejected_key_still_returns_the_parse_error(): + """The body is read before the key is authenticated, so a caller who sends both a + malformed body and a key that fails auth gets the 400. Authenticating the request + first (LIT-4780) must not turn that into the auth status code.""" + from fastapi import Request + from starlette.datastructures import URL + + import litellm.proxy.proxy_server as _proxy_server_mod + + request = Request( + scope={ + "type": "http", + "headers": [(b"content-type", b"application/json")], + "method": "POST", + } + ) + request._url = URL(url="/chat/completions") + request._body = b'{}{"model": "gpt-4o"}' + + attrs = _proxy_attrs_for_centralized_checks(user_custom_auth=None) + originals = {a: getattr(_proxy_server_mod, a, None) for a in attrs} + try: + for k, v in attrs.items(): + setattr(_proxy_server_mod, k, v) + with ( + patch( + "litellm.proxy.auth.user_api_key_auth._user_api_key_auth_builder", + new_callable=AsyncMock, + side_effect=ProxyException( + message="Authentication Error, invalid key", + type="auth_error", + param="None", + code=status.HTTP_401_UNAUTHORIZED, + ), + ), + patch( + "litellm.proxy.auth.user_api_key_auth.RouteChecks.should_call_route", + ), + ): + with pytest.raises(ProxyException) as exc_info: + await user_api_key_auth(request=request, api_key="Bearer sk-bad") + + assert "Invalid JSON payload" in str(exc_info.value.message) + assert exc_info.value.code == str(status.HTTP_400_BAD_REQUEST) + finally: + for k, v in originals.items(): + setattr(_proxy_server_mod, k, v) + + def _proxy_attrs_for_db_lookup(): """Minimal proxy_server attributes for driving the real ``_user_api_key_auth_builder`` down to the DB key lookup."""