fix(proxy): tighten role gating on /get/config/callbacks response - #31745
Conversation
Greptile SummaryThis PR applies security hardening to
Confidence Score: 5/5Safe to merge — the role gate is correctly placed, the redaction helper handles None values cleanly, and both the redacted and plaintext paths are pinned by new regression tests. The role check is a strict equality on No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/proxy_server.py | Adds _redact_callback_env_vars helper and gates /get/config/callbacks sensitive values behind PROXY_ADMIN role; covers callback variables, Slack webhook URLs, and email alerting fields correctly. |
| litellm/proxy/common_utils/callback_utils.py | Promotes private _is_sensitive_callback_var to the public is_sensitive_callback_key with an optional extra set; _EXTRA_SENSITIVE_CALLBACK_KEYS now matched case-insensitively via .lower(), consistent with existing usage. |
| tests/test_litellm/proxy/proxy_server/test_routes_config.py | Adds four comprehensive regression tests: view-only admin redaction, full-admin plaintext, Slack webhook URL redaction, email alerting redaction — plus body-wide grep assertions that no fixture secret survives in the response. |
| tests/test_litellm/proxy/test_proxy_server.py | Updates four test fixtures from bare MagicMock() auth overrides to explicit PROXY_ADMIN identity so they correctly exercise the plaintext path and don't silently rely on undefined mock attributes. |
| tests/proxy_unit_tests/test_proxy_server.py | Updates test_get_config_callbacks_environment_variables assertions to expect "REDACTED" for LANGFUSE_PUBLIC_KEY, LANGFUSE_SECRET_KEY, and OTEL_HEADERS under the client_no_auth (non-admin) fixture — consistent with the new redaction contract. |
| tests/test_litellm/test_router_retry_policy_update.py | Updates get_config() call to pass an explicit PROXY_ADMIN UserAPIKeyAuth so the retry-policy read-back test still receives plaintext router settings rather than a redacted or error response. |
Reviews (5): Last reviewed commit: "fix(proxy): tighten role gating on /get/..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR adds a redaction layer to the
Confidence Score: 4/5Safe to merge. The redaction logic is correct and fail-closed: any non-PROXY_ADMIN role (including None) gets credentials redacted. FastAPI's dependency cache ensures user_api_key_auth is called only once despite the double Depends pattern. The core redaction is sound and well-tested. Two gaps exist: GCS_PATH_SERVICE_ACCOUNT is treated as sensitive by the encryption path but not by the new env-var redaction allowlist, and the email alerting block is left with only partial asterisk masking for view-only admins while the callback and webhook blocks are fully redacted. Neither gap is introduced by this PR's own changes (the email masking was pre-existing), but the callback/webhook fix creates a visible inconsistency in the same response object. The email alerting variable block in litellm/proxy/proxy_server.py (lines ~14732–14750) deserves a second look for consistency with the new callback redaction pattern.
|
| Filename | Overview |
|---|---|
| litellm/proxy/proxy_server.py | Adds _redact_callback_env_vars helper and wires it into get_config for non-admin callers; logic is correct and fail-closed (None role → redacted). LANGFUSE_PUBLIC_KEY is redacted as a side effect of the name containing 'key' — intentional per the PR description and tests. |
| tests/test_litellm/proxy/proxy_server/test_routes_config.py | Four new mock-only tests covering view-only admin redaction, full-admin plaintext, webhook URL redaction, and the helper directly. No real network calls; test logic is correct and assertions are specific. |
Reviews (2): Last reviewed commit: "test(proxy): align test_get_config_callb..." | Re-trigger Greptile
…unt path on /get/config/callbacks Greptile review feedback on PR #31745: - Email alerting block in the same response used mask_sensitive_keys unconditionally; SMTP_USERNAME came back verbatim to view-only admins and SMTP_PASSWORD got a partial-asterisk prefix/suffix mask rather than full redaction. Same role gate as the callback + Slack webhook blocks now applies: view-only admins get 'REDACTED' for both credential halves; non-secret routing fields (SMTP_HOST, SMTP_PORT, SMTP_SENDER_EMAIL, TEST_EMAIL_ADDRESS, EMAIL_LOGO_URL, EMAIL_SUPPORT_CONTACT) stay visible. Full PROXY_ADMIN continues to see the previous mask_sensitive_keys output so the edit form round-trips. - GCS_PATH_SERVICE_ACCOUNT mirrors callback_utils._EXTRA_SENSITIVE_CALLBACK_KEYS, which already flags 'gcs_path_service_account' for the encryption path. Adding the env-var-cased name to _EXTRA_SECRET_CALLBACK_ENV_VARS keeps the two lists in sync so the leak does not silently re-open if a GCS-backed callback is later wired into AllCallbacks.litellm_callback_params. LANGFUSE_PUBLIC_KEY redaction noted by Greptile is intentional and kept: it is half of an SMTP-style credential pair (Langfuse uses Basic Auth with public+secret) and identifies which Langfuse project the proxy logs to. The conservative default-deny on segment-keyword matches is the same posture as PR #30587.
|
@greptileai re-review please. Latest push addresses two of the three P2s; left a reply on the third explaining why the current behavior is intentional. |
…unt path on /get/config/callbacks Greptile review feedback on PR #31745: - Email alerting block in the same response used mask_sensitive_keys unconditionally; SMTP_USERNAME came back verbatim to view-only admins and SMTP_PASSWORD got a partial-asterisk prefix/suffix mask rather than full redaction. Same role gate as the callback + Slack webhook blocks now applies: view-only admins get 'REDACTED' for both credential halves; non-secret routing fields (SMTP_HOST, SMTP_PORT, SMTP_SENDER_EMAIL, TEST_EMAIL_ADDRESS, EMAIL_LOGO_URL, EMAIL_SUPPORT_CONTACT) stay visible. Full PROXY_ADMIN continues to see the previous mask_sensitive_keys output so the edit form round-trips. - GCS_PATH_SERVICE_ACCOUNT mirrors callback_utils._EXTRA_SENSITIVE_CALLBACK_KEYS, which already flags 'gcs_path_service_account' for the encryption path. Adding the env-var-cased name to _EXTRA_SECRET_CALLBACK_ENV_VARS keeps the two lists in sync so the leak does not silently re-open if a GCS-backed callback is later wired into AllCallbacks.litellm_callback_params. LANGFUSE_PUBLIC_KEY redaction noted by Greptile is intentional and kept: it is half of an SMTP-style credential pair (Langfuse uses Basic Auth with public+secret) and identifies which Langfuse project the proxy logs to. The conservative default-deny on segment-keyword matches is the same posture as PR #30587.
181cd6a to
8359532
Compare
|
bugbot run |
8359532 to
4a7a7a2
Compare
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8359532. Configure here.
11347e3 to
c4b41ec
Compare
|
bugbot run |
c4b41ec to
854ccd8
Compare
|
@greptileai re-review please. Latest push refactors the sensitivity classifier to reuse the existing helper in |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c4b41ec. Configure here.
2d3d7e5 to
3802bdb
Compare
|
@greptileai re-review please. Latest push closes the remaining Slack |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3802bdb. Configure here.
3802bdb to
7201006
Compare
The handler returned decrypted callback environment values and alerting routing values verbatim to callers who were not full PROXY_ADMIN. Gate those on full-admin role, matching the posture used on the sibling config-inspection endpoints. Non-sensitive routing fields (host / base URL / port style values) stay visible so the UI can still label which integration is wired up. Full PROXY_ADMIN sees everything unchanged so the edit form round-trips on save. Resolves LIT-4115.
7201006 to
054e6c1
Compare
…rriAI#31745) The handler returned decrypted callback environment values and alerting routing values verbatim to callers who were not full PROXY_ADMIN. Gate those on full-admin role, matching the posture used on the sibling config-inspection endpoints. Non-sensitive routing fields (host / base URL / port style values) stay visible so the UI can still label which integration is wired up. Full PROXY_ADMIN sees everything unchanged so the edit form round-trips on save. Resolves LIT-4115.
…rriAI#31745) The handler returned decrypted callback environment values and alerting routing values verbatim to callers who were not full PROXY_ADMIN. Gate those on full-admin role, matching the posture used on the sibling config-inspection endpoints. Non-sensitive routing fields (host / base URL / port style values) stay visible so the UI can still label which integration is wired up. Full PROXY_ADMIN sees everything unchanged so the edit form round-trips on save. Resolves LIT-4115.
Relevant issues
Linear ticket
Resolves LIT-4115
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Live-reproduced against a local proxy. Detailed repro steps + before/after payloads are on the internal Linear ticket (LIT-4115); omitted here to avoid publishing a concrete attack payload.
Summary of the behavior change:
PROXY_ADMIN(e.g.PROXY_ADMIN_VIEW_ONLY) now receive"REDACTED"in place of sensitive values in the/get/config/callbacksresponse.PROXY_ADMINsees everything unchanged so the edit form round-trips on save without the UI silently overwriting stored values with the literal string"REDACTED"./config/field/info,/config/list, and MCP routes but did not touch this handler.Type
🐛 Bug Fix
Changes
litellm/proxy/proxy_server.py— add a helper that gates sensitive callback env var values behind the full-admin role, usingSENSITIVE_DATA_MASKER.is_sensitive_keyplus a small allowlist for keys the segment-keyword masker misses. Thread the role gate through the callback block, the Slack webhook block, and the email alerting block of/get/config/callbacks. FullPROXY_ADMINbehavior is unchanged.tests/test_litellm/proxy/proxy_server/test_routes_config.py— regression tests that pin both the redacted-for-view-only-admin and plaintext-for-full-admin response shapes, plus a body-wide grep assertion that no fixture value survives anywhere in the response.tests/test_litellm/proxy/test_proxy_server.py+tests/test_litellm/test_router_retry_policy_update.py— update pre-existing test fixtures that previously bypassed auth viaMagicMock()to pass an explicitPROXY_ADMINidentity so they exercise the intended plaintext path.tests/proxy_unit_tests/test_proxy_server.py— align pre-existing assertion intest_get_config_callbacks_environment_variableswith the new redaction contract (the fixture caller resolves as non-admin underclient_no_auth).Note
Medium Risk
Security-sensitive authorization/redaction on an admin config API; behavior change is role-scoped with broad test coverage, but misclassified keys could still leak or over-redact.
Overview
Closes an information-disclosure gap on
GET /get/config/callbacks: callers who are not fullPROXY_ADMIN(e.g.PROXY_ADMIN_VIEW_ONLY) now get"REDACTED"for sensitive callback/alerting env values instead of plaintext secrets.The handler wires in
UserAPIKeyAuth, treats onlyPROXY_ADMINas full admin, and applies_redact_callback_env_varsto each callback’svariables, plus parallel redaction for Slack (SLACK_WEBHOOK_URL,alerts_to_webhook) and email alerting env vars. Non-sensitive routing fields (hosts, endpoints, ports) stay visible. Full admins still receive real values so the Admin UI can round-trip edits.is_sensitive_callback_keyreplaces the private_is_sensitive_callback_varhelper (optionalextrakey set for env names the generic masker misses, e.g.OTEL_HEADERS,SLACK_WEBHOOK_URL). Tests pin view-only vs full-admin response shapes and update fixtures that previously mocked auth without a role.Reviewed by Cursor Bugbot for commit 3802bdb. Bugbot is set up for automated code reviews on this repo. Configure here.