diff --git a/litellm/proxy/management_endpoints/model_management_endpoints.py b/litellm/proxy/management_endpoints/model_management_endpoints.py index 47566d6b6d5..c71381f2402 100644 --- a/litellm/proxy/management_endpoints/model_management_endpoints.py +++ b/litellm/proxy/management_endpoints/model_management_endpoints.py @@ -384,16 +384,19 @@ def update_db_model(db_model: Deployment, updated_patch: updateDeployment) -> Pr # passes through (which today re-sends the OLD pricing on every save) cannot # silently undo a litellm_params clear via .update(). # - # Restricted to SPECIAL_MODEL_INFO_PARAMS (input/output cost per token/character - # and cache read/write costs) so this path cannot be used to null out privileged - # model_info fields like team_id or access groups. SPECIAL_MODEL_INFO_PARAMS are - # mirrored between litellm_params and model_info by Deployment.__init__, so the - # clear propagates to both blobs. + # A litellm_params field the caller explicitly sets to null is removed from the + # stored params, so the UI can drop a param (e.g. reasoning_effort) instead of + # only overwriting it. The cross-blob mirror stays scoped to SPECIAL_MODEL_INFO_PARAMS + # (input/output cost per token/character and cache read/write costs), which + # Deployment.__init__ mirrors between litellm_params and model_info. The model_info + # loop below stays restricted to SPECIAL params so this path cannot null out + # privileged model_info fields like team_id or access groups. if updated_patch.litellm_params: for field in updated_patch.litellm_params.model_fields_set: - if field in SPECIAL_MODEL_INFO_PARAMS and getattr(updated_patch.litellm_params, field) is None: + if getattr(updated_patch.litellm_params, field) is None: merged_litellm_params.pop(field, None) - merged_model_info.pop(field, None) + if field in SPECIAL_MODEL_INFO_PARAMS: + merged_model_info.pop(field, None) if updated_patch.model_info: for field in updated_patch.model_info.model_fields_set: if field in SPECIAL_MODEL_INFO_PARAMS and getattr(updated_patch.model_info, field) is None: diff --git a/tests/test_litellm/proxy/management_endpoints/test_model_management_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_model_management_endpoints.py index aafd84e4c83..8836e0d4cfc 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_model_management_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_model_management_endpoints.py @@ -2914,8 +2914,10 @@ class TestUpdateDBModelClearPricing: `litellm_params` and `model_info` (SPECIAL_MODEL_INFO_PARAMS are mirrored between the two by Deployment.__init__). - Restricted to SPECIAL_MODEL_INFO_PARAMS so non-pricing fields (e.g. team_id) - cannot be cleared via this path. + The cross-blob mirror and the model_info clear path stay restricted to + SPECIAL_MODEL_INFO_PARAMS so a privileged model_info field (e.g. team_id) + cannot be nulled through this path. Clearing an ordinary litellm_params field + removes it from litellm_params only; see TestUpdateDBModelClearParam. """ def test_clear_input_cost_removes_from_both_blobs(self): @@ -2991,10 +2993,10 @@ class TestUpdateDBModelClearPricing: assert params["input_cost_per_token"] == 0.000001 assert params["output_cost_per_token"] == 0.000007 - def test_null_on_non_pricing_field_does_not_clear(self): - """Security guard: only SPECIAL_MODEL_INFO_PARAMS can be cleared via null. - Privileged or unrelated model_info fields (e.g. team_id) must be unaffected - by the null-clearing path so a team admin can't ungate a team-scoped model. + def test_null_litellm_param_clears_param_but_not_privileged_model_info(self): + """Security guard: a null on an ordinary litellm_params field clears it from + litellm_params, but must NOT reach into model_info to null a privileged field + (e.g. team_id) so a team admin can't ungate a team-scoped model through this path. """ from litellm.proxy.management_endpoints.model_management_endpoints import ( update_db_model, @@ -3010,13 +3012,12 @@ class TestUpdateDBModelClearPricing: model_name="openai/*", litellm_params=LiteLLM_Params( model="openai/*", + api_base="https://old.example", input_cost_per_token=0.000001, ), model_info=ModelInfo(id="dep-pricing-1", team_id="team-keep-me"), ) - # Patch sends a null for api_base (non-SPECIAL field). Must NOT clear team_id - # or any other non-pricing field from the merged dict. result = update_db_model( db_model=db_model, updated_patch=updateDeployment( @@ -3024,10 +3025,13 @@ class TestUpdateDBModelClearPricing: ), ) + params = json.loads(result["litellm_params"]) info = json.loads(result["model_info"]) + # api_base is cleared from litellm_params (the removal fix) + assert "api_base" not in params # Pricing still present (not part of this patch) assert "input_cost_per_token" in info - # team_id must survive + # team_id (privileged model_info) must survive — the mirror stays SPECIAL-only assert info.get("team_id") == "team-keep-me" def test_clear_survives_model_info_passthrough_with_old_pricing(self): @@ -3192,6 +3196,107 @@ class TestUpdateDBModelClearPricing: assert info["cache_creation_input_token_cost"] == 0.000003 +class TestUpdateDBModelClearParam: + """Deleting an ordinary litellm_params field in the model editor sends it as an + explicit null. update_db_model must drop it from the stored params (the additive + merge alone would keep the old value), while leaving unrelated stored params and + omitted secrets untouched. + """ + + def _db_model(self): + from litellm.types.router import Deployment, LiteLLM_Params, ModelInfo + + return Deployment( + model_name="gpt-5.6-sol", + litellm_params=LiteLLM_Params( + model="gpt-5.6-sol", + api_key="sk-real-secret", + reasoning_effort="none", + temperature=0.5, + ), + model_info=ModelInfo(id="dep-clear-param-0"), + ) + + def test_explicit_null_removes_the_param(self): + from litellm.proxy.management_endpoints.model_management_endpoints import ( + update_db_model, + ) + from litellm.types.router import updateLiteLLMParams + + result = update_db_model( + db_model=self._db_model(), + updated_patch=updateDeployment( + litellm_params=updateLiteLLMParams(reasoning_effort=None) + ), + ) + params = json.loads(result["litellm_params"]) + assert "reasoning_effort" not in params + + def test_unrelated_params_and_omitted_secret_survive(self): + """The UI strips the masked api_key, so it is omitted (not nulled) and the + merge must keep it; only the explicitly-nulled field is removed.""" + from litellm.proxy.management_endpoints.model_management_endpoints import ( + update_db_model, + ) + from litellm.types.router import updateLiteLLMParams + + result = update_db_model( + db_model=self._db_model(), + updated_patch=updateDeployment( + litellm_params=updateLiteLLMParams(reasoning_effort=None) + ), + ) + params = json.loads(result["litellm_params"]) + assert "reasoning_effort" not in params + assert params.get("temperature") == 0.5 + assert params.get("api_key") == "sk-real-secret" + + def test_omitted_param_is_preserved(self): + """PATCH semantics unchanged: a field not in the patch keeps its stored value. + Only an explicit null deletes — omission still means 'leave as is'.""" + from litellm.proxy.management_endpoints.model_management_endpoints import ( + update_db_model, + ) + from litellm.types.router import updateLiteLLMParams + + result = update_db_model( + db_model=self._db_model(), + updated_patch=updateDeployment( + litellm_params=updateLiteLLMParams(temperature=0.9) + ), + ) + params = json.loads(result["litellm_params"]) + assert params.get("temperature") == 0.9 + assert params.get("reasoning_effort") == "none" + + def test_null_team_id_via_litellm_params_does_not_clear_model_info(self): + """The attack the SPECIAL-only mirror guards against: team_id is not a real + litellm_param, so a null for it in litellm_params must not reach model_info.""" + from litellm.proxy.management_endpoints.model_management_endpoints import ( + update_db_model, + ) + from litellm.types.router import ( + Deployment, + LiteLLM_Params, + ModelInfo, + updateLiteLLMParams, + ) + + db_model = Deployment( + model_name="gpt-5.6-sol", + litellm_params=LiteLLM_Params(model="gpt-5.6-sol"), + model_info=ModelInfo(id="dep-clear-param-1", team_id="team-keep-me"), + ) + result = update_db_model( + db_model=db_model, + updated_patch=updateDeployment( + litellm_params=updateLiteLLMParams(team_id=None) + ), + ) + info = json.loads(result["model_info"]) + assert info.get("team_id") == "team-keep-me" + + class TestGetModelInfoWithIdBlocked: """`ProxyConfig.get_model_info_with_id` must propagate the DB-level `blocked` column into the in-memory `model_info` dict so the router filter can read it.""" diff --git a/ui/litellm-dashboard/src/components/model_info_view.tsx b/ui/litellm-dashboard/src/components/model_info_view.tsx index e1a4d4311ab..33be0b590f5 100644 --- a/ui/litellm-dashboard/src/components/model_info_view.tsx +++ b/ui/litellm-dashboard/src/components/model_info_view.tsx @@ -36,7 +36,7 @@ import VectorStoreSelector from "./vector_store_management/VectorStoreSelector"; import { CheckIcon, CopyIcon } from "lucide-react"; import { useEffect, useMemo, useState } from "react"; import { copyToClipboard as utilCopyToClipboard } from "../utils/dataUtils"; -import { isMaskedSecret, stripMaskedSecrets } from "../utils/maskedSecretUtils"; +import { computeRemovedLitellmParamKeys, isMaskedSecret, stripMaskedSecrets } from "../utils/maskedSecretUtils"; import { formItemValidateJSON, truncateString } from "../utils/textUtils"; import AutoRouterConnectionTest from "./add_model/auto_router_connection_test"; import { AutoRouterTestTarget, buildAutoRouterTestTargets } from "./add_model/build_auto_router_test_targets"; @@ -504,6 +504,16 @@ export default function ModelInfoView({ return; } + // A param the user deleted from the editor is absent from updatedLitellmParams; + // send it as an explicit null so the backend clears it, since the PATCH merge is + // additive and would otherwise keep the stored value. + for (const removedKey of computeRemovedLitellmParamKeys( + localModelData.litellm_params ?? {}, + updatedLitellmParams, + )) { + updatedLitellmParams[removedKey] = null; + } + // Final guard: never PATCH a redacted secret. The /model/info snapshot that // seeds this form masks secrets, and any save re-sends the whole params blob; // without this strip a masked value would be re-encrypted over the real secret. diff --git a/ui/litellm-dashboard/src/utils/maskedSecretUtils.test.ts b/ui/litellm-dashboard/src/utils/maskedSecretUtils.test.ts new file mode 100644 index 00000000000..972fe40d7e3 --- /dev/null +++ b/ui/litellm-dashboard/src/utils/maskedSecretUtils.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, it } from "vitest"; + +import { computeRemovedLitellmParamKeys, isMaskedSecret, stripMaskedSecrets } from "./maskedSecretUtils"; + +describe("isMaskedSecret", () => { + it("flags values with a run of 2+ mask chars", () => { + expect(isMaskedSecret("sk-1****2345")).toBe(true); + }); + + it("does not flag a single star (real config like a wildcard model name)", () => { + expect(isMaskedSecret("openai/*")).toBe(false); + }); + + it("only flags strings", () => { + expect(isMaskedSecret(5)).toBe(false); + expect(isMaskedSecret(null)).toBe(false); + }); +}); + +describe("computeRemovedLitellmParamKeys", () => { + it("reports a key the user deleted from the editor", () => { + const stored = { model: "gpt-5.6-sol", reasoning_effort: "none" }; + const saved = { model: "gpt-5.6-sol" }; + expect(computeRemovedLitellmParamKeys(stored, saved)).toEqual(["reasoning_effort"]); + }); + + it("reports nothing when every stored key is still present", () => { + const stored = { model: "gpt-5.6-sol", reasoning_effort: "none" }; + const saved = { model: "gpt-5.6-sol", reasoning_effort: "high" }; + expect(computeRemovedLitellmParamKeys(stored, saved)).toEqual([]); + }); + + it("treats a key present with an undefined/null value as still there, not removed", () => { + // Dedicated form fields spread into the saved blob keep the key even when empty, + // so an emptied field must not be misread as a deletion. + const stored = { model: "gpt-5.6-sol", api_base: "https://old.example" }; + const saved = { model: "gpt-5.6-sol", api_base: undefined }; + expect(computeRemovedLitellmParamKeys(stored, saved)).toEqual([]); + }); + + it("never reports a masked secret, so a stripped credential is not nulled out", () => { + const stored = { model: "gpt-5.6-sol", api_key: "sk-1****2345" }; + const saved = { model: "gpt-5.6-sol" }; + expect(computeRemovedLitellmParamKeys(stored, saved)).toEqual([]); + }); + + it("never reports litellm_credential_name, which has its own save path", () => { + const stored = { model: "gpt-5.6-sol", litellm_credential_name: "openai-cred" }; + const saved = { model: "gpt-5.6-sol" }; + expect(computeRemovedLitellmParamKeys(stored, saved)).toEqual([]); + }); + + it("reports only the removed non-secret keys among several changes", () => { + const stored = { + model: "gpt-5.6-sol", + reasoning_effort: "none", + temperature: 0.5, + api_key: "sk-1****2345", + }; + const saved = { model: "gpt-5.6-sol", temperature: 0.5 }; + expect(computeRemovedLitellmParamKeys(stored, saved).sort()).toEqual(["reasoning_effort"]); + }); +}); + +describe("stripMaskedSecrets", () => { + it("drops keys whose value is a masked secret and keeps the rest", () => { + expect(stripMaskedSecrets({ model: "gpt-5.6-sol", api_key: "sk-1****2345", reasoning_effort: null })).toEqual({ + model: "gpt-5.6-sol", + reasoning_effort: null, + }); + }); +}); diff --git a/ui/litellm-dashboard/src/utils/maskedSecretUtils.ts b/ui/litellm-dashboard/src/utils/maskedSecretUtils.ts index 101316bbd82..61fd6fa4412 100644 --- a/ui/litellm-dashboard/src/utils/maskedSecretUtils.ts +++ b/ui/litellm-dashboard/src/utils/maskedSecretUtils.ts @@ -8,3 +8,17 @@ export const isMaskedSecret = (value: unknown): boolean => typeof value === "str export const stripMaskedSecrets = (params: Record): Record => Object.fromEntries(Object.entries(params).filter(([, value]) => !isMaskedSecret(value))); + +// The model edit form seeds its LiteLLM Params editor from the stored params minus the +// credential name and any masked secret (both preserved through other paths). A key that +// was in that seed but is absent from the params being saved was deleted by the user, so +// it must be sent as an explicit null to clear it — the backend PATCH merge is additive, +// so an omitted key keeps its stored value. Excluding masked secrets here is what stops a +// redacted-and-stripped credential from being mistaken for a deletion and nulled out. +export const computeRemovedLitellmParamKeys = ( + storedParams: Record, + savedParams: Record, +): string[] => + Object.entries(storedParams) + .filter(([key, value]) => key !== "litellm_credential_name" && !isMaskedSecret(value) && !(key in savedParams)) + .map(([key]) => key);