From fc305aa2a1a272c91e0df7cc0ce3d779d17b488b Mon Sep 17 00:00:00 2001 From: jibanez-staticduo Date: Tue, 22 Sep 2026 13:30:19 +0200 Subject: [PATCH] fix(chatgpt): keep membership reads fail-closed and clear the CI-only gates Four required checks were red on the merge commit. None of them came from a conflict hunk: each is a place where the merged tree disagrees with the code around it, and two only misbehave inside CI's environment. - `litellm/proxy/auth/auth_checks.py`: this PR gave `get_team_membership` a `raise_on_error` flag and defaulted it to False, so every caller -- all seven of them pre-existing -- started treating a failed membership read as "no membership row". Upstream main has no such flag and lets the exception surface, so the default quietly inverted fail-closed authorization into a grant: a Redis or database outage handed a team member the team's model scope and budget instead of rejecting the request. Default it to True, which covers the member budget check, the member model-scope check, JWT team resolution, and the compact summary gate, and name the one caller that genuinely wants the other behavior: `_team_member_granted_models` outside strict mode only attributes grants, so an unreadable member scope still degrades to "no member-level scope". - `tests/test_litellm/proxy/test_live_route_registration.py`: the websocket test runs the proxy lifespan, and the boot check now refuses an unset or weak master key before the app serves a single request, so the request never reached a Live route and all nine parametrizations failed where CI exports no key. Set one in the test rather than inherit whatever the environment has. A real key, not the weak-key opt-out, keeps the assertion honest: with a key in place the legacy sideband dependency rejects `Bearer test`, so awaiting `_auth` still proves the Live routes authenticate first. - `litellm/proxy/realtime_endpoints/call_supervision.py`: observer cleanup gathered a tuple whose length depended on a branch, which no `gather` overload can bind. That was the one `reportCallIssue` this PR added over the basedpyright budget ceiling. Two explicit branches gather the same tasks with the same cancellation and drain semantics. Correction to the merge commit message: it says the config-over-database login throttle test was dropped in favour of upstream's; both tests were actually kept, and both pass. Validation on this tree: 4546 passed, 1 skipped for the whole `proxy-auth` shard (auth, hooks, policy engine, client, realtime call redis) under `TZ=UTC`, which is the suite CI runs; the six membership fail-closed tests that were red now pass; `test_live_route_registration.py` passes 30 with the environment master key set, unset, and set to a rejected weak key; 546 passed across `tests/test_litellm/proxy/realtime_endpoints` and `tests/unit/realtime_api` after the gather change; `basedpyright` reports no `reportCallIssue` in `call_supervision.py`, and every remaining `reportCallIssue` in a file this PR touches sits on a line `git blame` traces to upstream main, so the rule returns to its base count of 113; `ruff check` and `ruff format --check` clean on the changed files. Local-only note, unrelated to this PR: `tests/test_litellm/proxy/hooks/test_batch_rate_limiter.py::test_cumulative_batch_tokens_over_tpd_returns_429_with_remaining_daily_window` fails under `TZ=Europe/Brussels` and passes under `TZ=UTC`. Upstream added it in `438d46cb50` and CI runs in UTC, so it is not affected by anything here. --- litellm/proxy/auth/auth_checks.py | 11 ++++++++++- litellm/proxy/realtime_endpoints/call_supervision.py | 10 ++++++---- .../proxy/test_live_route_registration.py | 5 +++++ 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/litellm/proxy/auth/auth_checks.py b/litellm/proxy/auth/auth_checks.py index 2cabef85398..6526c70488e 100644 --- a/litellm/proxy/auth/auth_checks.py +++ b/litellm/proxy/auth/auth_checks.py @@ -2338,12 +2338,18 @@ async def get_team_membership( user_api_key_cache: UserApiKeyCache, parent_otel_span: Span | None = None, proxy_logging_obj: ProxyLogging | None = None, - raise_on_error: bool = False, + raise_on_error: bool = True, ) -> Optional["LiteLLM_TeamMembership"]: """ Returns team membership object if user is member of team. Do a isolated check for team membership vs. doing a combined key + team + user + team-membership check, as key might come in frequently for different users/teams. Larger call will slowdown query time. This way we get to cache the constant (key/team/user info) and only update based on the changing value (team membership). + + ``raise_on_error`` defaults to True because the callers that apply member-level limits -- the budget and + model-scope checks in ``common_checks``, the JWT team resolution, and the compact summary gate -- cannot + tell an absent row apart from a failed read, so swallowing an outage there hands the member whatever the + team allows. A caller that only attributes grants, and can proceed with the lists it already holds, + passes False and degrades to "no member-level scope". """ if user_id is None or team_id is None: return None @@ -4514,6 +4520,9 @@ async def _team_member_granted_models( prisma_client=prisma_client, user_api_key_cache=user_api_key_cache, proxy_logging_obj=proxy_logging_obj, + # Spelled out because it is the one caller that wants the opposite of the default: outside + # strict mode this walk only attributes grants, so an unreadable member scope degrades to + # "no member-level scope" instead of failing the request. raise_on_error=strict_grant_lookup, ) return () if team_membership is None else _member_allowed_models(team_membership) diff --git a/litellm/proxy/realtime_endpoints/call_supervision.py b/litellm/proxy/realtime_endpoints/call_supervision.py index 3f9ed521624..710220b17d2 100644 --- a/litellm/proxy/realtime_endpoints/call_supervision.py +++ b/litellm/proxy/realtime_endpoints/call_supervision.py @@ -186,10 +186,12 @@ class CallSupervisor: reader.cancel() if lease_failed is not None: lease_failed.cancel() - await asyncio.gather( - *((reader, stopped, lease_failed) if lease_failed is not None else (reader, stopped)), - return_exceptions=True, - ) + # Branching rather than a conditional star-unpacked tuple: the overload solver cannot + # bind one result type across a tuple whose length depends on the branch. + if lease_failed is not None: + await asyncio.gather(reader, stopped, lease_failed, return_exceptions=True) + else: + await asyncio.gather(reader, stopped, return_exceptions=True) with suppress(Exception): await self._upstream.close() if not self._usage_complete(): diff --git a/tests/test_litellm/proxy/test_live_route_registration.py b/tests/test_litellm/proxy/test_live_route_registration.py index 80f3a5a9478..30d58d8a6e2 100644 --- a/tests/test_litellm/proxy/test_live_route_registration.py +++ b/tests/test_litellm/proxy/test_live_route_registration.py @@ -50,6 +50,11 @@ def test_public_live_websockets_reach_live_auth_before_legacy_sideband(monkeypat authenticate = AsyncMock(side_effect=HTTPException(403, "Live authentication rejected")) monkeypatch.setattr(live, "_auth", authenticate) monkeypatch.setattr(proxy_server, "general_settings", {}) + # TestClient runs the proxy lifespan, and the boot check refuses a weak or unset master key + # before the app serves anything. Set a safe key here instead of relying on the ambient one, so + # the request really reaches the routes: with a key in place the legacy sideband dependency + # would reject the "Bearer test" header, so awaiting _auth still proves which auth ran first. + monkeypatch.setenv("LITELLM_MASTER_KEY", "sk-live-route-registration-test-master-key") # A previous proxy test may leave the module scheduler bound to a closed loop. monkeypatch.setattr(proxy_server, "scheduler", None) with TestClient(proxy_server.app) as client: