fix: redact callback credentials in /get/config/callbacks for non-admin callers - #31798
Open
devin-ai-integration[bot] wants to merge 4 commits into
Open
Conversation
…in callers GET /get/config/callbacks returned plaintext environment_variables per callback (LANGFUSE_SECRET_KEY, LANGSMITH_API_KEY, DD_API_KEY, etc.) and alert_to_webhook_url mappings (Slack incoming-webhook secrets) to any caller with admin_viewer access, including PROXY_ADMIN_VIEW_ONLY. Accept user_api_key_dict in the handler, check the caller role, and for non-PROXY_ADMIN callers redact callback variable values and replace each alert_to_webhook_url value with REDACTED. Full admins still see plaintext. Follows the same pattern as PR #30587 which fixed the same family of leaks on /config/field/info, /config/list, and MCP endpoints. Regression tests verify both admin-sees-plaintext and viewer-gets-redacted paths, including callback variables and alerts_to_webhook.
|
|
Contributor
Greptile SummaryThis PR adds credential redaction to
Confidence Score: 2/5Not safe to merge as-is — two unrelated regressions are bundled with the intended security hardening. The redaction fix itself is correct, but the PR also silently removes all audit logging for admin config mutations and converts concurrent spend-counter updates to sequential ones, breaking the error-isolation guarantee in the budget enforcement path. Either regression alone would warrant holding the PR; together they make the change risky to ship. litellm/proxy/proxy_server.py — the
|
| Filename | Overview |
|---|---|
| litellm/proxy/proxy_server.py | Adds credential redaction for non-admin callers on /get/config/callbacks (correct fix), but also removes all config-change audit logging and replaces concurrent spend-counter increments with sequential awaits that break error isolation. |
| tests/test_litellm/proxy/proxy_server/test_routes_config.py | Adds two focused regression tests covering admin plaintext and viewer-only redaction for both callback variables and alerts_to_webhook; tests are mock-only with no real network calls, matching repo test standards. |
Reviews (1): Last reviewed commit: "fix: redact callback credentials in /get..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+2282
to
2388
| if token is not None: | ||
| # token arrives pre-hashed from metadata["user_api_key"] (auth flow | ||
| # hashes raw "sk-..." keys before they reach the callback). The | ||
| # startswith("sk-") check is a safety net matching update_cache — | ||
| # if a raw key somehow arrives, hash it; otherwise use as-is to | ||
| # avoid double-hashing (budget checks read valid_token.token which | ||
| # is single-hashed). | ||
| hashed_token = ( | ||
| hash_token(token=key_token) if isinstance(key_token, str) and key_token.startswith("sk-") else key_token | ||
| ) | ||
| hashed_token = hash_token(token=token) if isinstance(token, str) and token.startswith("sk-") else token | ||
| key_counter_key = f"spend:key:{hashed_token}" | ||
| if key_counter_key not in reserved_counter_keys: | ||
| await _init_and_increment_spend_counter( | ||
| counter_key=key_counter_key, | ||
| source_cache_key=hashed_token, | ||
| increment=cost, | ||
| increment=response_cost, | ||
| ) | ||
|
|
||
| # Increment per-window budget counters for multi-budget keys | ||
| key_obj = await user_api_key_cache.async_get_cache(key=hashed_token) | ||
| if key_obj is None: | ||
| return | ||
| key_budget_limits = getattr(key_obj, "budget_limits", None) or ( | ||
| key_obj.get("budget_limits") if isinstance(key_obj, dict) else None | ||
| ) | ||
| if isinstance(key_budget_limits, str): | ||
| key_budget_limits = json.loads(key_budget_limits) | ||
| if not isinstance(key_budget_limits, list): | ||
| return | ||
| for window in key_budget_limits: | ||
| duration = window["budget_duration"] if isinstance(window, dict) else window.budget_duration | ||
| key_window_counter = f"spend:key:{hashed_token}:window:{duration}" | ||
| if key_window_counter not in reserved_counter_keys: | ||
| await _init_and_increment_window_spend_counter( | ||
| counter_key=key_window_counter, | ||
| entity_type="Key", | ||
| entity_id=hashed_token, | ||
| window_start=get_budget_window_start(window), | ||
| increment=cost, | ||
| ) | ||
| if key_obj is not None: | ||
| key_budget_limits = getattr(key_obj, "budget_limits", None) or ( | ||
| key_obj.get("budget_limits") if isinstance(key_obj, dict) else None | ||
| ) | ||
| if isinstance(key_budget_limits, str): | ||
| key_budget_limits = json.loads(key_budget_limits) | ||
| if isinstance(key_budget_limits, list): | ||
| for window in key_budget_limits: | ||
| duration = window["budget_duration"] if isinstance(window, dict) else window.budget_duration | ||
| key_window_counter = f"spend:key:{hashed_token}:window:{duration}" | ||
| if key_window_counter not in reserved_counter_keys: | ||
| from litellm.proxy.spend_tracking.budget_reservation import ( | ||
| get_budget_window_start, | ||
| ) | ||
|
|
||
| async def _team_scope(scope_team_id: str) -> None: | ||
| team_counter_key = f"spend:team:{scope_team_id}" | ||
| await _init_and_increment_window_spend_counter( | ||
| counter_key=key_window_counter, | ||
| entity_type="Key", | ||
| entity_id=hashed_token, | ||
| window_start=get_budget_window_start(window), | ||
| increment=response_cost, | ||
| ) | ||
|
|
||
| if team_id is not None: | ||
| team_counter_key = f"spend:team:{team_id}" | ||
| if team_counter_key not in reserved_counter_keys: | ||
| await _init_and_increment_spend_counter( | ||
| counter_key=team_counter_key, | ||
| source_cache_key=f"team_id:{scope_team_id}", | ||
| increment=cost, | ||
| ) | ||
| source_cache_key=f"team_id:{team_id}", | ||
| increment=response_cost, | ||
| ) | ||
|
|
||
| # Increment per-window budget counters for multi-budget teams | ||
| team_obj = await user_api_key_cache.async_get_cache(key=f"team_id:{team_id}") | ||
| if team_obj is not None: | ||
| team_budget_limits = getattr(team_obj, "budget_limits", None) or ( | ||
| team_obj.get("budget_limits") if isinstance(team_obj, dict) else None | ||
| ) | ||
| if isinstance(team_budget_limits, str): | ||
| team_budget_limits = json.loads(team_budget_limits) | ||
| if isinstance(team_budget_limits, list): | ||
| for window in team_budget_limits: | ||
| duration = window["budget_duration"] if isinstance(window, dict) else window.budget_duration | ||
| team_window_counter = f"spend:team:{team_id}:window:{duration}" | ||
| if team_window_counter not in reserved_counter_keys: | ||
| from litellm.proxy.spend_tracking.budget_reservation import ( | ||
| get_budget_window_start, | ||
| ) | ||
|
|
||
| team_obj = await user_api_key_cache.async_get_cache(key=f"team_id:{scope_team_id}") | ||
| if team_obj is None: | ||
| return | ||
| team_budget_limits = getattr(team_obj, "budget_limits", None) or ( | ||
| team_obj.get("budget_limits") if isinstance(team_obj, dict) else None | ||
| ) | ||
| if isinstance(team_budget_limits, str): | ||
| team_budget_limits = json.loads(team_budget_limits) | ||
| if not isinstance(team_budget_limits, list): | ||
| return | ||
| for window in team_budget_limits: | ||
| duration = window["budget_duration"] if isinstance(window, dict) else window.budget_duration | ||
| team_window_counter = f"spend:team:{scope_team_id}:window:{duration}" | ||
| if team_window_counter not in reserved_counter_keys: | ||
| await _init_and_increment_window_spend_counter( | ||
| counter_key=team_window_counter, | ||
| entity_type="Team", | ||
| entity_id=scope_team_id, | ||
| window_start=get_budget_window_start(window), | ||
| increment=cost, | ||
| ) | ||
| await _init_and_increment_window_spend_counter( | ||
| counter_key=team_window_counter, | ||
| entity_type="Team", | ||
| entity_id=team_id, | ||
| window_start=get_budget_window_start(window), | ||
| increment=response_cost, | ||
| ) | ||
|
|
||
| async def _team_member_scope(scope_user_id: str, scope_team_id: str) -> None: | ||
| team_member_counter_key = f"spend:team_member:{scope_user_id}:{scope_team_id}" | ||
| if team_member_counter_key in reserved_counter_keys: | ||
| return | ||
| await _init_and_increment_spend_counter( | ||
| counter_key=team_member_counter_key, | ||
| source_cache_key=f"team_membership:{scope_user_id}:{scope_team_id}", | ||
| increment=cost, | ||
| ) | ||
| if user_id is not None and team_id is not None: | ||
| team_member_counter_key = f"spend:team_member:{user_id}:{team_id}" | ||
| if team_member_counter_key not in reserved_counter_keys: | ||
| await _init_and_increment_spend_counter( | ||
| counter_key=team_member_counter_key, | ||
| source_cache_key=f"team_membership:{user_id}:{team_id}", | ||
| increment=response_cost, | ||
| ) | ||
|
|
||
| async def _user_scope(scope_user_id: str) -> None: | ||
| user_counter_key = f"spend:user:{scope_user_id}" | ||
| if user_counter_key in reserved_counter_keys: | ||
| return | ||
| await _init_and_increment_spend_counter( | ||
| counter_key=user_counter_key, | ||
| source_cache_key=scope_user_id, | ||
| increment=cost, | ||
| ) | ||
|
|
||
| scope_coros = tuple( | ||
| coro | ||
| for coro in ( | ||
| _key_scope(token) if token is not None else None, | ||
| _team_scope(team_id) if team_id is not None else None, | ||
| _team_member_scope(user_id, team_id) if user_id is not None and team_id is not None else None, | ||
| _user_scope(user_id) if user_id is not None else None, | ||
| _increment_end_user_and_tag_spend_counters( | ||
| end_user_id=end_user_id, | ||
| tags=tags, | ||
| response_cost=cost, | ||
| reserved_counter_keys=reserved_counter_keys, | ||
| ) | ||
| if end_user_id is not None or tags is not None | ||
| else None, | ||
| _increment_org_spend_counter( | ||
| org_id=org_id, | ||
| response_cost=cost, | ||
| reserved_counter_keys=reserved_counter_keys, | ||
| ) | ||
| if org_id is not None | ||
| else None, | ||
| ) | ||
| if coro is not None | ||
| ) | ||
| if user_id is not None: | ||
| user_counter_key = f"spend:user:{user_id}" | ||
| if user_counter_key not in reserved_counter_keys: | ||
| await _init_and_increment_spend_counter( | ||
| counter_key=user_counter_key, | ||
| source_cache_key=user_id, | ||
| increment=response_cost, | ||
| ) | ||
|
|
||
| # return_exceptions so a failing scope does not leave its siblings running | ||
| # as orphaned tasks that race the caller's reservation-counter invalidation; | ||
| # all scopes settle, then the first error propagates as before. | ||
| scope_results = await asyncio.gather(*scope_coros, return_exceptions=True) | ||
| scope_errors = [r for r in scope_results if isinstance(r, BaseException)] | ||
| if scope_errors: | ||
| raise scope_errors[0] | ||
| await _increment_end_user_and_tag_spend_counters( | ||
| end_user_id=end_user_id, | ||
| tags=tags, | ||
| response_cost=response_cost, | ||
| reserved_counter_keys=reserved_counter_keys, | ||
| ) | ||
|
|
||
| await _increment_org_spend_counter( | ||
| org_id=org_id, | ||
| response_cost=response_cost, | ||
| reserved_counter_keys=reserved_counter_keys, | ||
| ) | ||
| if budget_reservation is not None: | ||
| budget_reservation["finalized"] = True |
Contributor
There was a problem hiding this comment.
The old implementation ran all scopes (_key_scope, _team_scope, _user_scope, _team_member_scope, end-user/tag, org) concurrently with asyncio.gather(..., return_exceptions=True), which guaranteed every scope would be updated even if one raised an exception. The new sequential awaits abandon that guarantee: if the token scope (or any earlier scope) throws — say, due to a transient Redis error — the team, user, org, end-user, and tag counters are never incremented for that request, silently under-counting spend against those budgets.
Comment on lines
13952
to
13961
| @@ -13972,11 +13961,6 @@ async def _upsert_section(param_name: str, value: dict) -> None: | |||
| existing["alerting"].append("slack") | |||
Contributor
There was a problem hiding this comment.
The create_config_audit_log helper and every call to it have been deleted, covering update_config (general_settings, environment_variables, litellm_settings, router_settings), update_config_general_settings, delete_config_general_settings, and delete_callback. Config mutations no longer write audit log rows. This is unrelated to the stated scope of this PR and silently drops audit coverage for all admin config operations going forward.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The existing /get/config/callbacks tests used a plain MagicMock() for the auth override, which lacks user_role. With the credential redaction change, this caused the handler to treat callers as non-admin and redact values, breaking assertions that expect plaintext. Set user_role to PROXY_ADMIN in the override to match the test intent.
Same issue as previous commit; the legacy test_proxy_server.py tests in tests/proxy_unit_tests used client_no_auth which returns INTERNAL_USER role. With credential redaction, these tests need an explicit PROXY_ADMIN role override to see plaintext values.
When get_config() is called directly (not via FastAPI HTTP router), the user_api_key_dict parameter defaults to the Depends() marker object rather than a resolved UserAPIKeyAuth. Guard the role check with an isinstance check so internal/direct callers are treated as full admin.
yucheng-berri
requested changes
Jul 6, 2026
yucheng-berri
left a comment
yucheng-berri
left a comment
Contributor
There was a problem hiding this comment.
Please close this PR, #31745 resolved the issue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relevant issues
Fixes VERIA-440
Linear ticket
Resolves LIT-4115
Pre-Submission checklist
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
To verify end-to-end:
The view-only response shows
"REDACTED"for all callback variable values and allalerts_to_webhookvalues, while callback names, types, and alert type lists remain visibleType
Bug Fix
Changes
GET /get/config/callbacksreturned the proxy's decryptedenvironment_variablesper callback (LANGFUSE_SECRET_KEY, LANGSMITH_API_KEY, DD_API_KEY, BRAINTRUST_API_KEY, etc.) and thealert_to_webhook_urlmapping (Slack incoming-webhook credentials) to any caller inadmin_viewer_routes, includingPROXY_ADMIN_VIEW_ONLY. Same family as VERIA-214 / PR #30587 which fixed/config/field/info,/config/list, and MCP endpoints but did not touch this routeThe fix adds
user_api_key_dictto the handler signature, checks the caller's role, and for non-PROXY_ADMINcallers:"REDACTED"(preservingNonefor variables that were never set, so the UI can still distinguish configured vs unconfigured)alert_to_webhook_urlvalue with"REDACTED"(preserving keys so the UI still knows which alert types have webhooks configured)Full admins see plaintext as before. Direct internal calls (without HTTP auth context) also see plaintext since there is no security boundary to enforce
Two regression tests added in the mapped test file:
test_get_config_callbacks_admin_sees_plaintextandtest_get_config_callbacks_viewer_gets_redacted, both exercising callback variables andalerts_to_webhookto prevent this leak from regressing