mirror of
https://github.com/BerriAI/litellm.git
synced 2026-11-06 07:48:06 +00:00
fix(model-edit): clear a litellm param when it is removed in the editor
Deleting a param in the model editor did nothing: the /model/{id}/update
PATCH merges the incoming litellm_params over the stored ones, and an omitted
key kept its stored value, so removed params (e.g. reasoning_effort) persisted.
Backend: update_db_model now clears any litellm_params field the caller sets to
an explicit null, not just the pricing fields. The cross-blob mirror and the
model_info clear path stay scoped to SPECIAL_MODEL_INFO_PARAMS, so a null cannot
reach privileged model_info fields like team_id.
UI: the editor now sends a deleted param as an explicit null. Keys are diffed
against the seed the editor was populated from, which already excludes masked
secrets and the credential name, so a stripped credential is never mistaken for
a deletion and nulled out.
This commit is contained in:
parent
3d76dfc72e
commit
fbf6f2dcad
5 changed files with 221 additions and 17 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
72
ui/litellm-dashboard/src/utils/maskedSecretUtils.test.ts
Normal file
72
ui/litellm-dashboard/src/utils/maskedSecretUtils.test.ts
Normal file
|
|
@ -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,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -8,3 +8,17 @@ export const isMaskedSecret = (value: unknown): boolean => typeof value === "str
|
|||
|
||||
export const stripMaskedSecrets = (params: Record<string, unknown>): Record<string, unknown> =>
|
||||
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<string, unknown>,
|
||||
savedParams: Record<string, unknown>,
|
||||
): string[] =>
|
||||
Object.entries(storedParams)
|
||||
.filter(([key, value]) => key !== "litellm_credential_name" && !isMaskedSecret(value) && !(key in savedParams))
|
||||
.map(([key]) => key);
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue