feat(proxy): audit remaining system-wide settings updates - #31754
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR extends the
Confidence Score: 5/5Safe to merge — all audit calls are additive, fire-and-forget, and isolated from the main save path so a DB hiccup cannot break configuration writes. The change adds audit logging to every admin settings endpoint without altering any existing write semantics. The SSO before-snapshot fix is correctly implemented: the existing row is read before the upsert, decrypted, and passed to the audit helper which redacts secret fields before persistence. All before-state captures use either copy.deepcopy (for dicts mutated in place) or Python reference-replacement semantics (for dicts swapped out at a key), so no audit log can capture a stale or mutated value. The _update_litellm_setting signature promotion is backed by complete call-site updates in both production and test code. Test coverage is mutation-protected and exercises redaction, before/after diffs, and the audit-fires-before-reload-failure ordering guarantee. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_types.py | Adds SSO_CONFIG_TABLE_NAME and UI_SETTINGS_TABLE_NAME to LitellmTableNames enum; straightforward additive change. |
| litellm/proxy/proxy_server.py | Adds audit log calls (with before-state deep copies) to update_config (four sections), update_config_general_settings, delete_config_general_settings, and delete_callback. All local variables; no mutation-after-task-creation concerns. Audit fires before reload in delete_callback per design. |
| litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py | Wires audit logging to add/delete allowed IP, SSO update (with before-snapshot via find_unique + _decrypt_db_variables), UI theme, UI settings, default team, and MCP semantic filter. _update_litellm_setting promoted user_api_key_dict to required; all callers updated. |
| tests/test_litellm/proxy/test_proxy_server.py | New mutation-protected tests cover: field update/delete audit, all four config/update sections in one request, callback delete before reload failure, and environment_variables wholesale redaction. Assertions are comprehensive and not weakened. |
| tests/test_litellm/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py | New tests cover SSO audit with redacted before-snapshot (mutation-protected), SSO audit-fires-before-env-cleanup-failure, allowed IP add/delete audit, UI theme audit, UI settings audit. Existing tests correctly updated to mock the new find_unique call added to update_sso_settings. |
| tests/test_litellm/proxy/management_endpoints/test_team_default_params.py | Two existing tests updated to supply the now-required user_api_key_dict argument. Assertions unchanged — coverage is not weakened. |
| tests/test_litellm/proxy/management_endpoints/scim/test_scim_v2_endpoints.py | One existing SCIM test updated to pass user_api_key_dict to _update_litellm_setting. Assertion unchanged. |
Reviews (8): Last reviewed commit: "refactor(proxy): audit config writes via..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR extends the audit-log framework to the remaining proxy-wide settings writers:
Confidence Score: 4/5Safe to merge; the audit calls are non-blocking (exceptions are swallowed internally), so no existing request path is broken by this change. The wiring is consistent across all endpoints and the redaction logic, deepcopy-before-snapshot pattern, and ordering guarantees are correct. The one gap is that update_sso_settings always passes before_value=None, so SSO audit rows will never carry a pre-change snapshot — an auditor investigating a misconfiguration can only see what the settings were changed to, not what they were before. litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py — specifically the update_sso_settings function, which omits the before-snapshot in its audit call.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_types.py | Adds SSO_CONFIG_TABLE_NAME and UI_SETTINGS_TABLE_NAME enum values to LitellmTableNames for use in audit log table_name routing. |
| litellm/proxy/proxy_server.py | Adds create_config_audit_log calls to update_config (4 sections), update_config_general_settings, delete_config_general_settings, and delete_callback; all use copy.deepcopy for before-snapshots and the shared helper handles redaction. |
| litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py | Wires audit logging to update_sso_settings, update_ui_theme_settings, update_ui_settings, add_allowed_ip, delete_allowed_ip, and _update_litellm_setting; makes user_api_key_dict required on the shared helper; SSO endpoint always passes before_value=None. |
| tests/test_litellm/proxy/test_proxy_server.py | Adds 6 new audit-log tests covering general_settings update/delete, all four /config/update sections, callback deletion (including reload-failure ordering), and full environment_variable redaction. |
| tests/test_litellm/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py | Adds tests for SSO audit (including env-cleanup-failure ordering), add/delete allowed_ip audit, UI theme audit, and UI settings audit. No regression masking observed. |
Reviews (2): Last reviewed commit: "feat(proxy): audit remaining system-wide..." | Re-trigger Greptile
Greptile review of #31754 flagged update_sso_settings as the one endpoint where before_value is permanently None, so the LiteLLM_SSOConfig audit trail has no pre-change state. An auditor reviewing a secret-rotation event could see what the SSO settings were changed to but not what they were before. Read the existing SSO row before the upsert, decrypt it via proxy_config._decrypt_db_variables, and pass it as before_value. create_config_audit_log's secret-name redaction then masks the *_client_secret fields, so neither the old nor the new plaintext secret lands in the audit row. Add a regression test asserting the before-snapshot reflects the pre-change values for non-secret fields (google_client_id) and is redacted for secret fields (google_client_secret). Mutation-checked against reverting to before_value=None. The pre-existing SSO tests now also mock litellm_ssoconfig.find_unique since the endpoint reads it; the read returns None for tests that do not care about the before-state.
2bb1d97 to
b796ffe
Compare
|
@greptileai re-review please. Addressed the SSO before-snapshot finding from the prior review:
Live-verified end-to-end on a real proxy + Postgres: a rotation from Added a mutation-checked regression test: reverting Rebased onto the parent PR (#31753) which has its own fixes for the prior round of findings. |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Pending zero init migration
- Removed the committed 0_init migration so Prisma no longer treats it as a pending oldest migration on upgraded databases.
You can send follow-ups to the cloud agent here.
|
|
|
@greptileai please re-review the latest HEAD (748707c). The SSO before-snapshot finding from your prior review is addressed:
Live-verified end-to-end on a real proxy + Postgres: a rotation from This PR is rebased on top of #31753 which has its own fixes; both PRs should be reviewed together. |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 748707c. Configure here.
Greptile review of #31754 flagged update_sso_settings as the one endpoint where before_value is permanently None, so the LiteLLM_SSOConfig audit trail has no pre-change state. An auditor reviewing a secret-rotation event could see what the SSO settings were changed to but not what they were before. Read the existing SSO row before the upsert, decrypt it via proxy_config._decrypt_db_variables, and pass it as before_value. create_config_audit_log's secret-name redaction then masks the *_client_secret fields, so neither the old nor the new plaintext secret lands in the audit row. Add a regression test asserting the before-snapshot reflects the pre-change values for non-secret fields (google_client_id) and is redacted for secret fields (google_client_secret). Mutation-checked against reverting to before_value=None. The pre-existing SSO tests now also mock litellm_ssoconfig.find_unique since the endpoint reads it; the read returns None for tests that do not care about the before-state.
748707c to
e79020e
Compare
|
@greptileai please re-review HEAD e79020e. Two changes since your last review: The 12 audit call sites that used blocking The PR body was corrected from "the rest of the LiteLLM_Config writers" to "the remaining system-wide settings writers", with an explicit out-of-scope note for the reload-coordination endpoints (model_cost_map_reload, anthropic_beta_headers_reload) that mutate LiteLLM_Config but are operational state, not admin settings edits. Mutation check preserved (reverting any converted site still fails its regression test). 336 tests pass. |
|
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 e79020e. Configure here.
Extends the audit logging framework introduced in the parent PR to the rest of the LiteLLM_Config writers and the two adjacent settings tables: /config/update (general, environment_variables, litellm_settings, router_settings sections), /config/field/update, /config/field/delete, /config/callback/delete, /update/default_team_settings, /update/mcp_semantic_filter_settings, /add/allowed_ip, /delete/allowed_ip, /update/sso_settings, /update/ui_theme_settings, /update/ui_settings. Each writer records the actor, action, the affected config section, and a redacted before/after snapshot. SSO and UI settings rows use their own table_name (LiteLLM_SSOConfig, LiteLLM_UISettings). The /config/callback and /update/sso_settings audits fire BEFORE the proxy reload and the env cleanup step respectively, so a failure in either leaves the audit row intact. The audit-actor parameter on _update_litellm_setting is now required rather than optional; the chokepoint covers default_team and mcp_semantic_filter for free, and a future caller that forgets the actor fails loudly instead of silently skipping the audit. The two direct-calling tests pass a dummy actor. The environment_variables section redacts every value rather than relying on key-name matching, because it carries credentials under non-secret-looking uppercase keys (e.g. DATABASE_URL).
Greptile review of #31754 flagged update_sso_settings as the one endpoint where before_value is permanently None, so the LiteLLM_SSOConfig audit trail has no pre-change state. An auditor reviewing a secret-rotation event could see what the SSO settings were changed to but not what they were before. Read the existing SSO row before the upsert, decrypt it via proxy_config._decrypt_db_variables, and pass it as before_value. create_config_audit_log's secret-name redaction then masks the *_client_secret fields, so neither the old nor the new plaintext secret lands in the audit row. Add a regression test asserting the before-snapshot reflects the pre-change values for non-secret fields (google_client_id) and is redacted for secret fields (google_client_secret). Mutation-checked against reverting to before_value=None. The pre-existing SSO tests now also mock litellm_ssoconfig.find_unique since the endpoint reads it; the read returns None for tests that do not care about the before-state.
PR A's chokepoint audit call was refactored from a blocking await to asyncio.create_task so that a post-save audit-log failure could not surface as a 500 to the caller. The 12 other audit call sites added in this PR were still using await, reintroducing the exact 500-after-commit exposure at every sibling endpoint. Wrap them all in asyncio.create_task to match the model_management_endpoints / key_management_endpoints / hooks / config_override_endpoints / team_callback_endpoints / cache_settings_endpoints house pattern, so the codebase tells one story. The two direct-invocation tests (test_update_config_general_settings and test_delete_config_general_settings, which call the handler in-process rather than via TestClient) yield with `await asyncio.sleep(0)` after the handler returns so the scheduled audit task runs before the assertion.
e79020e to
b5f8f9f
Compare
|
@greptileai please re-review HEAD b5f8f9f. Rebased onto litellm_internal_staging after #31753 was merged as a squash; PR A's commits are dropped and only PR B's residual (4 commits) remains. The 7-file diff is unchanged bytewise — same as prior reviews on this branch, just replayed on the post-merge base. |
| # Read the existing SSO row first so the audit log captures a real | ||
| # before/after diff. Stored values are encrypted; decrypt them so the | ||
| # before-snapshot has the same shape as after_value, and rely on | ||
| # create_config_audit_log's secret-name redaction to mask the | ||
| # *_client_secret fields before the audit row is written. | ||
| existing_sso_record = await SSOConfigRepository(prisma_client).table.find_unique(where={"id": "sso_config"}) | ||
| before_sso_data: Optional[Dict[str, Any]] = None | ||
| if existing_sso_record and existing_sso_record.sso_settings: | ||
| stored = existing_sso_record.sso_settings | ||
| if isinstance(stored, str): | ||
| stored = json.loads(stored) | ||
| if isinstance(stored, dict): | ||
| before_sso_data = proxy_config._decrypt_db_variables(stored) |
There was a problem hiding this comment.
The find_unique + _decrypt_db_variables block runs before the upsert and has no error handling. If the stored SSO row contains data encrypted under a different key (e.g., after key rotation) or is otherwise malformed, _decrypt_db_variables will raise an unhandled exception, returning a 500 to the caller and preventing any SSO update from succeeding. Before this PR, the update path never read the existing row, so this is a net-new failure mode on a security-critical endpoint.
Wrap the before-state retrieval in a try/except and fall back to before_sso_data = None so a snapshot failure degrades gracefully to a null before-value rather than blocking the save.
|
@greptileai please re-review the current HEAD The SSO Also relevant since your review: all 12 audit call sites are now |
) * feat(proxy): audit remaining system-wide settings updates Extends the audit logging framework introduced in the parent PR to the rest of the LiteLLM_Config writers and the two adjacent settings tables: /config/update (general, environment_variables, litellm_settings, router_settings sections), /config/field/update, /config/field/delete, /config/callback/delete, /update/default_team_settings, /update/mcp_semantic_filter_settings, /add/allowed_ip, /delete/allowed_ip, /update/sso_settings, /update/ui_theme_settings, /update/ui_settings. Each writer records the actor, action, the affected config section, and a redacted before/after snapshot. SSO and UI settings rows use their own table_name (LiteLLM_SSOConfig, LiteLLM_UISettings). The /config/callback and /update/sso_settings audits fire BEFORE the proxy reload and the env cleanup step respectively, so a failure in either leaves the audit row intact. The audit-actor parameter on _update_litellm_setting is now required rather than optional; the chokepoint covers default_team and mcp_semantic_filter for free, and a future caller that forgets the actor fails loudly instead of silently skipping the audit. The two direct-calling tests pass a dummy actor. The environment_variables section redacts every value rather than relying on key-name matching, because it carries credentials under non-secret-looking uppercase keys (e.g. DATABASE_URL). * fix(proxy): capture redacted SSO before-snapshot in audit log Greptile review of BerriAI#31754 flagged update_sso_settings as the one endpoint where before_value is permanently None, so the LiteLLM_SSOConfig audit trail has no pre-change state. An auditor reviewing a secret-rotation event could see what the SSO settings were changed to but not what they were before. Read the existing SSO row before the upsert, decrypt it via proxy_config._decrypt_db_variables, and pass it as before_value. create_config_audit_log's secret-name redaction then masks the *_client_secret fields, so neither the old nor the new plaintext secret lands in the audit row. Add a regression test asserting the before-snapshot reflects the pre-change values for non-secret fields (google_client_id) and is redacted for secret fields (google_client_secret). Mutation-checked against reverting to before_value=None. The pre-existing SSO tests now also mock litellm_ssoconfig.find_unique since the endpoint reads it; the read returns None for tests that do not care about the before-state. * fix: remove committed zero init migration * refactor(proxy): audit config writes via asyncio.create_task everywhere PR A's chokepoint audit call was refactored from a blocking await to asyncio.create_task so that a post-save audit-log failure could not surface as a 500 to the caller. The 12 other audit call sites added in this PR were still using await, reintroducing the exact 500-after-commit exposure at every sibling endpoint. Wrap them all in asyncio.create_task to match the model_management_endpoints / key_management_endpoints / hooks / config_override_endpoints / team_callback_endpoints / cache_settings_endpoints house pattern, so the codebase tells one story. The two direct-invocation tests (test_update_config_general_settings and test_delete_config_general_settings, which call the handler in-process rather than via TestClient) yield with `await asyncio.sleep(0)` after the handler returns so the scheduled audit task runs before the assertion. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
4 participants
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
Reproduced live against a local proxy on
:4010withstore_audit_logs: trueand an enterprise license, backed by a real Postgres. HEAD tested:b5f8f9f0fd(stacked on top of the merged parent PR #31753). Baseline before the sweep wasLiteLLM_AuditLogcount = 0; total rows written across the sweep is 13 (one per audited operation, plus four for the multi-section/config/update).The customer scenario (
PATCH /update/internal_user_settings) lives in the parent PR and is already covered there. This PR extends coverage to:PATCH /update/default_team_settings->object_id=default_team_paramsPATCH /update/mcp_semantic_filter_settings->object_id=mcp_semantic_tool_filterPATCH /update/ui_theme_settings->object_id=ui_theme_configPATCH /update/ui_settings->table_name=LiteLLM_UISettings,object_id=ui_settingsPATCH /update/sso_settings->table_name=LiteLLM_SSOConfig,object_id=sso_config,google_client_secret=REDACTED; plaintextsk-SUPERSECRET-PLAINTEXTverified absent viapsql ILIKEPOST /config/callback/delete->object_id=litellm_settings,action=deleted, before{"success_callback":["datadog"]}after{"success_callback":[]}; audit fires before the proxy reloadPOST /config/updatewithenvironment_variables->object_id=environment_variables, every value redacted wholesale (DATABASE_URL=REDACTED); plaintextAUDIT_PASSWORD,db.internal, andpostgresql://all verified absent from the audit rowPOST /add/allowed_ipandPOST /delete/allowed_ip(use plain127.0.0.1, not CIDR; the allowlist matcher inauth_utils._check_valid_ipis string-equality) ->object_id=general_settings, one row per action,before/aftercapture the list mutationPOST /config/updatewith all four sections in one request -> four audit rows, one per section (general_settings,litellm_settings,router_settings,environment_variables), alltable_name=LiteLLM_ConfigPOST /config/field/update->object_id=general_settings, full before/after snapshotPOST /config/field/delete->object_id=general_settings,action=deleted, full before/after snapshotOperational config writers that are not admin settings edits (model-cost-map reload and anthropic-beta-header reload endpoints, plus internal background reloads) are intentionally out of scope.
Type
🆕 New Feature
Changes
Audit logging now fires on every admin endpoint that writes proxy-wide settings, recording the actor, the API key hash, the action, the affected config section, and a redacted before/after snapshot through the shared
create_config_audit_loghelper (introduced in the parent PR).The generic config API:
/config/update(general_settings, environment_variables, litellm_settings, router_settings),/config/field/update,/config/field/delete,/config/callback/delete.The dedicated settings endpoints in
proxy_setting_endpoints.pythat the admin dashboard uses, which do not route through/config/update:/update/default_team_settings,/update/mcp_semantic_filter_settings(via the shared_update_litellm_setting),/add/allowed_ipand/delete/allowed_ip,/update/sso_settings,/update/ui_theme_settings, and/update/ui_settings. SSO and UI settings rows are tagged with their own table names.New
LitellmTableNamesvalues (SSO_CONFIG_TABLE_NAME,UI_SETTINGS_TABLE_NAME) identify the SSO and UI settings rows. The parent PR addedCONFIG_TABLE_NAMEand the helpers.The audit-actor parameter on
_update_litellm_settingis now required rather than optional; the chokepoint covers default_team and mcp_semantic_filter for free, and a future caller that forgets the actor fails loudly instead of silently skipping the audit. The two direct-calling tests pass a dummy actor.The
/config/callback/deleteand/update/sso_settingsaudits fire BEFORE the proxy reload and the env-var cleanup step respectively, so a failure in either still leaves the audit row intact.object_idis the logical setting being changed, which for the dedicated endpoints is thelitellm_settingssub-key (default_team_params,mcp_semantic_tool_filter,ui_theme_config) rather than the row name, so it reads naturally for an auditor looking for a specific setting.Operational config writers that are not admin settings edits (the model-cost-map reload and anthropic-beta-header reload endpoints, plus internal background reloads) are intentionally out of scope.
Note
Medium Risk
Touches many security-sensitive admin write paths (SSO, env vars, IP allowlists, callbacks); behavior is additive audit logging with redaction, but mis-redaction or missed endpoints would be high impact.
Overview
Admin settings changes across the proxy now emit LiteLLM_AuditLog rows via the shared
create_config_audit_loghelper, using fire-and-forgetasyncio.create_taskso audit failures do not fail saves.Bulk config paths (
/config/updateforgeneral_settings,environment_variables,litellm_settings,router_settings), field update/delete, and/config/callback/deletecapture deep-copied before state and logupdatedordeletedwith the acting admin fromUserAPIKeyAuth.Dashboard settings endpoints in
proxy_setting_endpoints.pyare wired the same way: default team and MCP semantic filter (through_update_litellm_setting, which now requires an audit actor), allowed IP add/delete, SSO, UI theme, and UI flags. SSO audits read/decrypt the prior row for a real before snapshot and useLiteLLM_SSOConfig; UI settings useLiteLLM_UISettings. NewLitellmTableNamesconstants name those tables.Callback delete and SSO update schedule audits before reload or env cleanup so the row is written even when later steps fail. Tests cover per-section audits, secret/
DATABASE_URLredaction, and the new endpoints.Reviewed by Cursor Bugbot for commit e79020e. Bugbot is set up for automated code reviews on this repo. Configure here.