feat(proxy): audit default user settings updates - #31753
Conversation
Adds audit logging for the customer-impacting path: PATCH /update/internal_user_settings, which is what the admin dashboard hits when an admin changes Default User Settings and which today leaves no record of who changed what. Introduces the small framework that future system-wide settings audits will share: a CONFIG_TABLE_NAME enum value, a create_config_audit_log helper that reuses the existing create_object_audit_log path (so the enterprise gate and store_audit_logs flag still apply), and a _dump_redacted_config helper that strips secret leaves before the row is written using the same matcher /config/field/info applies for non-admins. The helper handles environment_variables as a special case where every value is redacted, since that section carries credentials under non-secret-looking uppercase keys (e.g. DATABASE_URL). Only update_internal_user_settings is wired up in this change. Coverage for the other LiteLLM_Config writers (/config/update sections, /config/field/update, /config/field/delete, /config/callback/delete, default_team_settings, mcp_semantic_filter, allowed_ip, sso_settings, ui_theme, ui_settings) is intentionally a follow-up so each can be verified live against the credential-bearing fields it actually carries. The audit-actor parameter on _update_litellm_setting is optional today so non-audited callers keep working unchanged; the follow-up will make it required once every caller is wired up.
Greptile SummaryThis PR closes the audit gap for
Confidence Score: 5/5All three previously raised issues have been addressed: the blocking audit await is now fire-and-forget via asyncio.create_task, json.dumps uses default=str throughout, and the dead non-dict redact_all_values branch that would have written a bare REDACTED string to the audit row has been removed. The change is narrowly scoped to the update_internal_user_settings path and does not touch auth or request routing. No remaining defects in the changed code. The three prior findings are fully resolved, the helper functions are correct, and the new tests exercise the key contracts: secret redaction, YAML-native type handling, the store_audit_logs=False no-op, correct audit-row fields, and the fire-and-forget guarantee that a failing audit write never turns a successful config save into a 500. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_types.py | Adds CONFIG_TABLE_NAME = 'LiteLLM_Config' to LitellmTableNames enum — minimal, correct addition. |
| litellm/proxy/proxy_server.py | Adds _dump_redacted_config (with default=str for YAML-loaded non-JSON-native values) and create_config_audit_log. Dead non-dict redact_all_values branch from a prior iteration has been removed; the fallthrough to _redact_secret_values_in_obj is now the correct behavior for that unreachable path. create_object_audit_log type contract (Optional[str] before/after) is satisfied. |
| litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py | Threads user_api_key_dict through _update_litellm_setting and schedules the audit write via asyncio.create_task (fire-and-forget) after save_config, so audit failures never surface as 500s. before_value is captured before any in-memory or DB update. |
| tests/test_litellm/proxy/test_proxy_server.py | New unit tests for _dump_redacted_config (None passthrough, secret-leaf redaction, default=str for datetime) and create_config_audit_log (audit row written with correct fields; noop when store_audit_logs=False). Deleted the prior misleading test for the non-dict redact_all_values dead branch. |
| tests/test_litellm/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py | Two new integration tests: one asserts the full audit row is written with correct fields (object_id, action, table_name, changed_by, before/after values) and one asserts 200 is returned even when the audit helper raises — validating the fire-and-forget contract. |
Reviews (6): Last reviewed commit: "refactor(proxy): drop unreachable non-di..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR adds audit logging for
Confidence Score: 3/5The settings change itself is always committed correctly; only the audit-log write has unguarded failure modes that could surface errors to callers after the fact. After save_config succeeds, create_config_audit_log is awaited without a try/except. Any unexpected exception there would propagate to the caller as a 500 while leaving the setting already changed in the DB. Audit logging should never block or fail the main request path. proxy_setting_endpoints.py (unguarded audit call after save) and proxy_server.py (_dump_redacted_config fallback logic)
|
| Filename | Overview |
|---|---|
| litellm/proxy/_types.py | Adds CONFIG_TABLE_NAME enum value to LitellmTableNames; straightforward and correct. |
| litellm/proxy/proxy_server.py | Adds _dump_redacted_config and create_config_audit_log helpers; _dump_redacted_config has a silent fallback when redact_all_values=True and value is not a dict. |
| litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py | Wires user_api_key_dict through _update_litellm_setting; the create_config_audit_log call after save_config is not wrapped in try/except. |
| tests/test_litellm/proxy/test_proxy_server.py | Adds unit tests for _dump_redacted_config and create_config_audit_log using mocks only; well-structured. |
| tests/test_litellm/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py | Adds integration-style regression test for the audit-log write through the full endpoint; mocks only, no real network calls. |
Reviews (2): Last reviewed commit: "feat(proxy): audit default user settings..." | Re-trigger Greptile
Greptile review of #31753 surfaced three robustness issues with the audit-log call path. The settings change always commits; these fixes prevent post-commit audit failures from surfacing as 500 responses. Switch the audit-log call in _update_litellm_setting from a blocking await to asyncio.create_task, matching the create_object_audit_log pattern every other call site uses (model_management_endpoints etc.). A transient prisma blip or a JSON serialization error in the audit row no longer turns a successful save_config into a 500 the caller sees. Add default=str to both json.dumps calls in _dump_redacted_config so a YAML-loaded value with a non-JSON-native leaf (datetime, custom object) serializes cleanly. The sibling audit-log serializers in team_endpoints.py already pass default=str for the same reason. Tighten the redact_all_values branch to redact wholesale for non-dict inputs rather than silently falling through to the key-name matcher; defensive against a future change that stores a section as a list or scalar. Each fix has a regression test mutation-checked against reverting the fix.
|
@greptileai re-review please. Addressed all three findings on the original commit:
Each fix has a mutation-checked regression test. |
|
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 24068f8. Configure here.
|
@greptileai please re-review the latest HEAD (24068f8). The three findings from your prior review are addressed:
Each fix has a mutation-checked regression test. |
|
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 24068f8. Configure here.
The defensive non-dict fallback in _dump_redacted_config emitted
json.dumps("REDACTED") which, if ever hit, would crash LiteLLM_AuditLogs
construction (mask_api_keys validator calls json.loads on the already-
parsed bare string). Reachability is zero: redact_all_values is True
only for param_name=="environment_variables", which is always a dict.
Delete the dead branch and its test rather than ship provably-wrong
defensive code with a test that green-lights it.
|
@greptileai please re-review HEAD 6e5053f. Addresses the code-review finding that the non-dict |
|
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 6e5053f. Configure here.
* feat(proxy): audit default user settings updates Adds audit logging for the customer-impacting path: PATCH /update/internal_user_settings, which is what the admin dashboard hits when an admin changes Default User Settings and which today leaves no record of who changed what. Introduces the small framework that future system-wide settings audits will share: a CONFIG_TABLE_NAME enum value, a create_config_audit_log helper that reuses the existing create_object_audit_log path (so the enterprise gate and store_audit_logs flag still apply), and a _dump_redacted_config helper that strips secret leaves before the row is written using the same matcher /config/field/info applies for non-admins. The helper handles environment_variables as a special case where every value is redacted, since that section carries credentials under non-secret-looking uppercase keys (e.g. DATABASE_URL). Only update_internal_user_settings is wired up in this change. Coverage for the other LiteLLM_Config writers (/config/update sections, /config/field/update, /config/field/delete, /config/callback/delete, default_team_settings, mcp_semantic_filter, allowed_ip, sso_settings, ui_theme, ui_settings) is intentionally a follow-up so each can be verified live against the credential-bearing fields it actually carries. The audit-actor parameter on _update_litellm_setting is optional today so non-audited callers keep working unchanged; the follow-up will make it required once every caller is wired up. * fix(proxy): make audit-log call non-blocking and serializer defensive Greptile review of BerriAI#31753 surfaced three robustness issues with the audit-log call path. The settings change always commits; these fixes prevent post-commit audit failures from surfacing as 500 responses. Switch the audit-log call in _update_litellm_setting from a blocking await to asyncio.create_task, matching the create_object_audit_log pattern every other call site uses (model_management_endpoints etc.). A transient prisma blip or a JSON serialization error in the audit row no longer turns a successful save_config into a 500 the caller sees. Add default=str to both json.dumps calls in _dump_redacted_config so a YAML-loaded value with a non-JSON-native leaf (datetime, custom object) serializes cleanly. The sibling audit-log serializers in team_endpoints.py already pass default=str for the same reason. Tighten the redact_all_values branch to redact wholesale for non-dict inputs rather than silently falling through to the key-name matcher; defensive against a future change that stores a section as a list or scalar. Each fix has a regression test mutation-checked against reverting the fix. * refactor(proxy): drop unreachable non-dict redact_all_values branch The defensive non-dict fallback in _dump_redacted_config emitted json.dumps("REDACTED") which, if ever hit, would crash LiteLLM_AuditLogs construction (mask_api_keys validator calls json.loads on the already- parsed bare string). Reachability is zero: redact_all_values is True only for param_name=="environment_variables", which is always a dict. Delete the dead branch and its test rather than ship provably-wrong defensive code with a test that green-lights it.
Relevant issues
Linear ticket
Resolves LIT-3839
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 reviewScreenshots / Proof of Fix
This PR closes the customer-impacting path: an admin changes Default User Settings from the dashboard and the change is recorded with who did it and what changed. The dashboard hits
PATCH /update/internal_user_settings, NOT/config/update, and today that path writes nothing toLiteLLM_AuditLog.Reproduced live against a local proxy on
:4010withstore_audit_logs: trueand an enterprise license, backed by a real Postgres.Before (current behavior on
litellm_internal_staging):After (this PR):
$ curl -X PATCH localhost:4010/update/internal_user_settings ... \ -d '{"max_budget":999.0,"models":["gpt-4.1-mini"]}' {"message":"Internal user settings updated successfully","status":"success", ...}Audit row written:
{ "action": "updated", "table_name": "LiteLLM_Config", "object_id": "default_internal_user_params", "changed_by": "default_user_id", "changed_by_api_key": "litellm_proxy_master_key", "before": {"user_role":"internal_user_viewer","max_budget":100.0,"models":["gpt-3.5-turbo","gpt-4"]}, "after": {"user_role":"internal_user_viewer","max_budget":999.0,"models":["gpt-4.1-mini"]} }changed_byrecords the acting user andchanged_by_api_keyrecords the key hash, so a UI/SSO admin edit (session token) is distinguishable from a direct API call (master or virtual key).Type
🆕 New Feature
Changes
Adds audit logging for the customer-impacting path:
PATCH /update/internal_user_settings. The dashboard's Default User Settings page hits this route, and there is no record today of who changed what.Introduces the small framework that future system-wide settings audits will share: a
LitellmTableNames.CONFIG_TABLE_NAMEenum value, acreate_config_audit_loghelper that reuses the existingcreate_object_audit_logpath (sostore_audit_logsand the enterprise gate still apply), and a_dump_redacted_confighelper that strips secret leaves from the snapshots using the same matcher/config/field/infoapplies for non-admins.environment_variablesis special-cased to redact every value, since it carries credentials under non-secret-looking uppercase keys (e.g.DATABASE_URL).Only
update_internal_user_settingsis wired up in this change. Coverage for the otherLiteLLM_Configwriters (the generic/config/*endpoints,default_team_settings,mcp_semantic_filter, allowed-IP add/delete,sso_settings,ui_theme_settings,ui_settings) is the follow-up so each can be verified live against the credential-bearing fields it actually carries. The follow-up is open against this branch as the broader-scope PR.The audit-actor parameter on
_update_litellm_settingis optional in this PR so non-audited callers keep working unchanged; the follow-up will make it required once every caller is wired up.Note
Medium Risk
Touches proxy config persistence and audit storage with credential redaction logic; scope is narrow (one UI route) and audit is fire-and-forget after commit.
Overview
Dashboard Default User Settings changes via
PATCH /update/internal_user_settingsnow writeLiteLLM_AuditLogrows (tableLiteLLM_Config, object iddefault_internal_user_params) with actor, before/after snapshots, and existingstore_audit_logs/ enterprise gating.Adds shared helpers:
create_config_audit_log(viacreate_object_audit_log),_dump_redacted_config(secret-key redaction aligned with/config/field/info;environment_variablesvalues fully redacted), andLitellmTableNames.CONFIG_TABLE_NAME._update_litellm_settingaccepts an optional admin actor and schedules audit writes aftersave_configso audit failures cannot 500 the request.Only
update_internal_user_settingsis wired in this PR; other config writers remain for follow-up. Tests cover redaction, audit row shape, disabled audit logs, and 200 on audit failure.Reviewed by Cursor Bugbot for commit 6e5053f. Bugbot is set up for automated code reviews on this repo. Configure here.