Merge c65beba299 into c03a38d501

litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py

@@ -4,6 +4,7 @@ import json
 import os
 from collections import Counter
 from collections.abc import Mapping
+from collections.abc import Set as AbstractSet
 from typing import (
     Any,
     Final,
@@ -958,6 +959,57 @@ async def get_sso_settings():
     return result
 
 
+def _restored_sso_secret(
+    secret_field: str,
+    incoming_secret: object,
+    submitted_fields: AbstractSet[str],
+    before_sso_data: Mapping[str, object] | None,
+) -> object:
+    """Return the value to persist for one SSO secret field.
+
+    The stored secret is kept when the client could not have meant to change it:
+    the field was not submitted at all, or the submitted value is exactly the
+    mask of the currently effective secret. Anything else -- a new secret,
+    ``""`` or ``None`` -- is an explicit instruction and passes through, so
+    "Clear SSO Settings" still clears. The effective secret is the stored row
+    value, falling back to the process environment (the precedence
+    get_sso_settings uses when it masks the field).
+    """
+    db_secret: Final = before_sso_data.get(secret_field) if before_sso_data else None
+    stored_secret: Final = db_secret or os.environ.get(SSO_FIELD_ENV_VARS.get(secret_field, ""))
+    if not stored_secret:
+        return incoming_secret
+    masked: Final = mask_sensitive_keys({secret_field: stored_secret}, {secret_field})  # mutable-ok: dict/set API
+    masked_secret: Final = masked[secret_field]
+    if secret_field not in submitted_fields or incoming_secret == masked_secret:
+        return stored_secret
+    return incoming_secret
+
+
+def _restore_masked_sso_secrets(
+    sso_data: Mapping[str, object],
+    submitted_fields: AbstractSet[str],
+    before_sso_data: Mapping[str, object] | None,
+) -> dict[str, object]:  # mutable-ok: dict API downstream
+    """Return a copy of ``sso_data`` with stored SSO secrets restored where the
+    client could not have meant to change them (#38177); see _restored_sso_secret.
+
+    Secret fields are masked before they are sent to the UI (see
+    get_sso_settings), so a client that edits only unrelated fields either
+    omits the secret or sends the masked placeholder back unchanged; persisting
+    either would overwrite e.g. the OAuth client_secret with ``abcd****wxyz``
+    and break SSO login.
+    """
+    return {  # mutable-ok: fresh copy; the original request mapping is never mutated
+        field_name: (
+            _restored_sso_secret(field_name, value, submitted_fields, before_sso_data)
+            if field_name in SSO_SECRET_FIELDS
+            else value
+        )
+        for field_name, value in sso_data.items()
+    }
+
+
 @router.patch(
     "/update/sso_settings",
     tags=["SSO Settings"],
@@ -1020,7 +1072,7 @@ async def update_sso_settings(
         config["general_settings"] = {}
 
     # Update environment variables in config and in memory
-    sso_data: Final = sso_config.model_dump()
+    sso_data: Final = _restore_masked_sso_secrets(sso_config.model_dump(), sso_config.model_fields_set, before_sso_data)
     for field_name, value in sso_data.items():
         if field_name in SSO_FIELD_ENV_VARS:
             env_var_name = SSO_FIELD_ENV_VARS[field_name]
@@ -1094,7 +1146,9 @@ async def update_sso_settings(
     return {
         "message": "SSO settings updated successfully",
         "status": "success",
-        "settings": sso_data,
+        # Never echo plaintext secrets back; the response is masked the same way
+        # get_sso_settings masks them.
+        "settings": mask_sensitive_keys(sso_data, set(SSO_SECRET_FIELDS)),  # mutable-ok: set API
     }
 
 

tests/test_litellm/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py

@@ -580,16 +580,15 @@ class TestProxySettingEndpoints:
         # Verify settings were updated
         settings = data["settings"]
         assert settings["google_client_id"] == new_sso_settings["google_client_id"]
-        assert (
-            settings["google_client_secret"] == new_sso_settings["google_client_secret"]
-        )
+        # Secrets are masked in the response; the plaintext is never echoed back.
+        assert settings["google_client_secret"] != new_sso_settings["google_client_secret"]
+        assert settings["google_client_secret"].startswith("new_")
+        assert "****" in settings["google_client_secret"]
         assert (
             settings["microsoft_client_id"] == new_sso_settings["microsoft_client_id"]
         )
-        assert (
-            settings["microsoft_client_secret"]
-            == new_sso_settings["microsoft_client_secret"]
-        )
+        assert settings["microsoft_client_secret"] != new_sso_settings["microsoft_client_secret"]
+        assert "****" in settings["microsoft_client_secret"]
         assert settings["proxy_base_url"] == new_sso_settings["proxy_base_url"]
         assert settings["user_email"] == new_sso_settings["user_email"]
 

tests/test_litellm/proxy/ui_crud_endpoints/test_sso_secret_backfill.py

@@ -0,0 +1,198 @@
+"""Regression tests for #38177: a partial SSO settings update must not overwrite
+a stored client secret with the masked placeholder the UI sends back (or lose
+it when the field is omitted), while still allowing an intentional clear, a
+genuinely new secret, and never echoing the plaintext secret in the response."""
+
+import json
+import os
+from unittest.mock import AsyncMock, MagicMock
+
+import pytest
+from fastapi.testclient import TestClient
+
+from litellm.litellm_core_utils.sensitive_data_masker import mask_sensitive_keys
+from litellm.proxy.config_resolvers.sso import SSO_SECRET_FIELDS
+from litellm.proxy.proxy_server import app
+
+client = TestClient(app)
+
+REAL_SECRET = "real_generic_secret_ABCD1234"
+
+
+@pytest.fixture
+def mock_auth():
+    from litellm.proxy.auth.user_api_key_auth import user_api_key_auth
+
+    async def _override():
+        return {"user_id": "test_user"}
+
+    app.dependency_overrides[user_api_key_auth] = _override
+    yield
+    app.dependency_overrides.pop(user_api_key_auth, None)
+
+
+def _masked(secret):
+    return mask_sensitive_keys({"generic_client_secret": secret}, set(SSO_SECRET_FIELDS))["generic_client_secret"]
+
+
+def _mock_prisma(monkeypatch, existing_settings):
+    """Wire a prisma client whose SSO row returns existing_settings (or None)."""
+    record = None
+    if existing_settings is not None:
+        record = MagicMock()
+        record.sso_settings = existing_settings
+
+    mock_prisma = MagicMock()
+    mock_prisma.db.litellm_ssoconfig.find_unique = AsyncMock(return_value=record)
+    mock_prisma.db.litellm_ssoconfig.upsert = AsyncMock()
+    mock_prisma.db.litellm_config = MagicMock()
+    mock_prisma.db.litellm_config.find_unique = AsyncMock(return_value=None)
+    mock_prisma.db.litellm_config.update = AsyncMock()
+    monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", mock_prisma)
+
+    from litellm.proxy.proxy_server import proxy_config
+
+    monkeypatch.setattr(proxy_config, "get_config", AsyncMock(return_value={}))
+    monkeypatch.setattr(proxy_config, "_encrypt_env_variables", lambda environment_variables: environment_variables)
+    monkeypatch.setattr(proxy_config, "_decrypt_db_variables", lambda stored: stored)
+    return mock_prisma
+
+
+def _stored_secret(mock_prisma):
+    return json.loads(mock_prisma.db.litellm_ssoconfig.upsert.call_args.kwargs["data"]["update"]["sso_settings"])
+
+
+def test_database_stored_secret_is_preserved(mock_auth, monkeypatch):
+    """A masked round-trip must keep a database-stored secret."""
+    monkeypatch.setenv("LITELLM_SALT_KEY", "test_salt_key")
+    monkeypatch.setattr("litellm.proxy.proxy_server.store_model_in_db", True)
+    monkeypatch.delenv("GENERIC_CLIENT_SECRET", raising=False)
+    mock_prisma = _mock_prisma(
+        monkeypatch,
+        {"generic_client_id": "cid", "generic_client_secret": REAL_SECRET, "proxy_base_url": "https://old.example.com"},
+    )
+
+    edited = {
+        "generic_client_id": "cid",
+        "generic_client_secret": _masked(REAL_SECRET),
+        "proxy_base_url": "https://new.example.com",
+    }
+    resp = client.patch("/update/sso_settings", json=edited)
+    assert resp.status_code == 200
+
+    stored = _stored_secret(mock_prisma)
+    assert stored["generic_client_secret"] == REAL_SECRET
+    assert stored["proxy_base_url"] == "https://new.example.com"
+    # The restored plaintext must not leak back to the caller.
+    assert resp.json()["settings"]["generic_client_secret"] == _masked(REAL_SECRET)
+
+
+def test_environment_sourced_secret_is_preserved(mock_auth, monkeypatch):
+    """A masked round-trip must keep a secret configured via env, even with no DB row."""
+    monkeypatch.setenv("LITELLM_SALT_KEY", "test_salt_key")
+    monkeypatch.setattr("litellm.proxy.proxy_server.store_model_in_db", True)
+    monkeypatch.setenv("GENERIC_CLIENT_SECRET", REAL_SECRET)
+    mock_prisma = _mock_prisma(monkeypatch, None)  # nothing in the database
+
+    edited = {
+        "generic_client_id": "cid",
+        "generic_client_secret": _masked(REAL_SECRET),
+        "proxy_base_url": "https://new.example.com",
+    }
+    resp = client.patch("/update/sso_settings", json=edited)
+    assert resp.status_code == 200
+
+    stored = _stored_secret(mock_prisma)
+    assert stored["generic_client_secret"] == REAL_SECRET
+
+
+def test_empty_secret_clears_intentionally(mock_auth, monkeypatch):
+    """An empty incoming value is an intentional clear, not a masked round-trip."""
+    monkeypatch.setenv("LITELLM_SALT_KEY", "test_salt_key")
+    monkeypatch.setattr("litellm.proxy.proxy_server.store_model_in_db", True)
+    monkeypatch.delenv("GENERIC_CLIENT_SECRET", raising=False)
+    mock_prisma = _mock_prisma(
+        monkeypatch,
+        {"generic_client_id": "cid", "generic_client_secret": REAL_SECRET},
+    )
+
+    edited = {"generic_client_id": "cid", "generic_client_secret": "", "proxy_base_url": "https://x.example.com"}
+    resp = client.patch("/update/sso_settings", json=edited)
+    assert resp.status_code == 200
+
+    stored = _stored_secret(mock_prisma)
+    assert stored["generic_client_secret"] == ""
+
+
+def test_new_secret_is_saved(mock_auth, monkeypatch):
+    """A genuinely new secret (no mask character) replaces the stored one."""
+    monkeypatch.setenv("LITELLM_SALT_KEY", "test_salt_key")
+    monkeypatch.setattr("litellm.proxy.proxy_server.store_model_in_db", True)
+    monkeypatch.delenv("GENERIC_CLIENT_SECRET", raising=False)
+    mock_prisma = _mock_prisma(
+        monkeypatch,
+        {"generic_client_id": "cid", "generic_client_secret": REAL_SECRET},
+    )
+
+    edited = {
+        "generic_client_id": "cid",
+        "generic_client_secret": "brand_new_secret_9999",
+        "proxy_base_url": "https://x",
+    }
+    resp = client.patch("/update/sso_settings", json=edited)
+    assert resp.status_code == 200
+
+    stored = _stored_secret(mock_prisma)
+    assert stored["generic_client_secret"] == "brand_new_secret_9999"
+
+
+def test_omitted_secret_is_preserved(mock_auth, monkeypatch):
+    """A partial payload that leaves the secret out entirely must keep it."""
+    monkeypatch.setenv("LITELLM_SALT_KEY", "test_salt_key")
+    monkeypatch.setattr("litellm.proxy.proxy_server.store_model_in_db", True)
+    monkeypatch.delenv("GENERIC_CLIENT_SECRET", raising=False)
+    mock_prisma = _mock_prisma(
+        monkeypatch,
+        {"generic_client_id": "cid", "generic_client_secret": REAL_SECRET},
+    )
+
+    resp = client.patch("/update/sso_settings", json={"generic_client_id": "cid", "proxy_base_url": "https://x"})
+    assert resp.status_code == 200
+
+    assert _stored_secret(mock_prisma)["generic_client_secret"] == REAL_SECRET
+
+
+def test_explicit_null_clears_intentionally(mock_auth, monkeypatch):
+    """The dashboard's "Clear SSO Settings" sends null; that must clear, not restore."""
+    monkeypatch.setenv("LITELLM_SALT_KEY", "test_salt_key")
+    monkeypatch.setattr("litellm.proxy.proxy_server.store_model_in_db", True)
+    monkeypatch.delenv("GENERIC_CLIENT_SECRET", raising=False)
+    mock_prisma = _mock_prisma(
+        monkeypatch,
+        {"generic_client_id": "cid", "generic_client_secret": REAL_SECRET},
+    )
+
+    resp = client.patch("/update/sso_settings", json={"generic_client_id": "cid", "generic_client_secret": None})
+    assert resp.status_code == 200
+
+    assert _stored_secret(mock_prisma)["generic_client_secret"] is None
+    assert "GENERIC_CLIENT_SECRET" not in os.environ
+
+
+def test_new_secret_containing_asterisk_is_saved(mock_auth, monkeypatch):
+    """Only the exact mask of the stored secret counts as "unchanged"."""
+    monkeypatch.setenv("LITELLM_SALT_KEY", "test_salt_key")
+    monkeypatch.setattr("litellm.proxy.proxy_server.store_model_in_db", True)
+    monkeypatch.delenv("GENERIC_CLIENT_SECRET", raising=False)
+    mock_prisma = _mock_prisma(
+        monkeypatch,
+        {"generic_client_id": "cid", "generic_client_secret": REAL_SECRET},
+    )
+
+    resp = client.patch(
+        "/update/sso_settings", json={"generic_client_id": "cid", "generic_client_secret": "new*secret*with*stars"}
+    )
+    assert resp.status_code == 200
+
+    assert _stored_secret(mock_prisma)["generic_client_secret"] == "new*secret*with*stars"
+    assert resp.json()["settings"]["generic_client_secret"] != "new*secret*with*stars"