From 0b5376db3649bb959d8a9f1ca9c78a8212744ec7 Mon Sep 17 00:00:00 2001 From: DucLong06 Date: Tue, 20 Oct 2026 11:03:31 +0700 Subject: [PATCH] fix(proxy): strip credential names and URL-encode credential routes in UI Credential names created with surrounding whitespace could not be read, updated or deleted by the trimmed name the UI shows. Create now strips the name and rejects a blank one, lookups fall back to the single stored row whose name only differs by surrounding whitespace, and the dashboard percent-encodes the name in credential routes. Fixes #46598 --- .../litellm_core_utils/credential_accessor.py | 9 +- .../common_utils/credential_hydration.py | 7 +- .../proxy/credential_endpoints/endpoints.py | 62 ++++--- .../repositories/credentials_repository.py | 9 +- .../common_utils/test_credential_hydration.py | 21 +++ .../credential_endpoints/test_endpoints.py | 167 +++++++++++++++++- .../components/model_add/CredentialModal.tsx | 2 +- .../src/components/networking.test.ts | 23 +++ .../src/components/networking.tsx | 6 +- 9 files changed, 271 insertions(+), 35 deletions(-) diff --git a/litellm/litellm_core_utils/credential_accessor.py b/litellm/litellm_core_utils/credential_accessor.py index b2310894c3a..ae00f8eb8c5 100644 --- a/litellm/litellm_core_utils/credential_accessor.py +++ b/litellm/litellm_core_utils/credential_accessor.py @@ -15,10 +15,17 @@ from litellm.types.utils import ( class CredentialAccessor: @staticmethod def find_credential(credential_name: str) -> CredentialItem | None: - return next( + exact: Final = next( (credential for credential in litellm.credential_list if credential.credential_name == credential_name), None, ) + if exact is not None: + return exact + stripped: Final = credential_name.strip() + padded: Final = tuple( + credential for credential in litellm.credential_list if credential.credential_name.strip() == stripped + ) + return padded[0] if len(padded) == 1 else None @staticmethod def get_credential_values( diff --git a/litellm/proxy/common_utils/credential_hydration.py b/litellm/proxy/common_utils/credential_hydration.py index f6a6c682cf0..5ac664c27be 100644 --- a/litellm/proxy/common_utils/credential_hydration.py +++ b/litellm/proxy/common_utils/credential_hydration.py @@ -12,6 +12,7 @@ from types import MappingProxyType from typing import Final, cast # noqa: TID251 # narrows untyped credential values at the decrypt boundary import litellm +from litellm.litellm_core_utils.credential_accessor import CredentialAccessor from litellm.proxy.common_utils.encrypt_decrypt_utils import decrypt_value_helper from litellm.proxy.utils import PrismaClient from litellm.repositories.credentials_repository import CredentialsRepository @@ -128,10 +129,8 @@ async def named_credential_wif_fields( fields would see none and allow the write. This reads both and returns the union, so the gate refuses whenever either side says the credential is server-owned. """ - matching: Final = tuple(c for c in litellm.credential_list if c.credential_name == credential_name) - in_memory: Final = tuple( - chain.from_iterable(server_owned_wif_fields_written(c.credential_values) for c in matching) - ) + matching: Final = CredentialAccessor.find_credential(credential_name) + in_memory: Final = () if matching is None else server_owned_wif_fields_written(matching.credential_values) if prisma_client is None: return in_memory db_credential: Final = await CredentialsRepository(prisma_client).find_by_name(credential_name) diff --git a/litellm/proxy/credential_endpoints/endpoints.py b/litellm/proxy/credential_endpoints/endpoints.py index 953c944bfb2..a5bc82f5697 100644 --- a/litellm/proxy/credential_endpoints/endpoints.py +++ b/litellm/proxy/credential_endpoints/endpoints.py @@ -193,6 +193,18 @@ class CredentialHelperUtils: ) +def _normalized_credential_name(credential_name: str) -> str: + trimmed: Final = credential_name.strip() + if not trimmed: + raise ProxyException( + message="credential_name cannot be blank.", + type=ProxyErrorTypes.validation_error.value, + code=status.HTTP_400_BAD_REQUEST, + param="credential_name", + ) + return trimmed + + def _normalized_display_name(display_name: str | None) -> str | None: if display_name is None: return None @@ -308,6 +320,7 @@ async def create_credential( status_code=500, detail={"error": CommonProxyErrors.db_not_connected_error.value}, ) + credential_name: Final = _normalized_credential_name(credential.credential_name) credential_values: Final = ( _resolve_deployment_credentials(llm_router, credential.model_id) if credential.model_id @@ -320,11 +333,11 @@ async def create_credential( ) _reject_non_admin_wif_fields(server_owned_wif_fields_written(credential_values), user_api_key_dict) _reject_non_admin_wif_fields( - await named_credential_wif_fields(credential.credential_name, prisma_client), user_api_key_dict + await named_credential_wif_fields(credential_name, prisma_client), user_api_key_dict ) - _reject_config_defined_name(credential.credential_name) + _reject_config_defined_name(credential_name) processed_credential: Final = CredentialItem( - credential_name=credential.credential_name, + credential_name=credential_name, display_name=_normalized_display_name(credential.display_name), credential_values=_without_null_values(_CREDENTIAL_DICT_ADAPTER.validate_python(credential_values)), credential_info=credential.credential_info, @@ -345,7 +358,7 @@ async def create_credential( except Exception as e: if not is_unique_violation(e): raise - raise HTTPException(status_code=409, detail=_credential_exists_detail(credential.credential_name)) + raise HTTPException(status_code=409, detail=_credential_exists_detail(credential_name)) ## ADD TO LITELLM ## CredentialAccessor.upsert_credentials([processed_credential]) @@ -395,15 +408,15 @@ async def get_credential_by_name( [BETA] endpoint. This might change unexpectedly. """ try: - for credential in litellm.credential_list: - if credential.credential_name == credential_name: - return _credential_view( - credential, - get_masked_values(credential.credential_values, unmasked_length=4, number_of_asterisks=4), - ) - raise HTTPException( - status_code=404, - detail="Credential not found. Got credential name: " + credential_name, + credential: Final = CredentialAccessor.find_credential(credential_name) + if credential is None: + raise HTTPException( + status_code=404, + detail="Credential not found. Got credential name: " + credential_name, + ) + return _credential_view( + credential, + get_masked_values(credential.credential_values, unmasked_length=4, number_of_asterisks=4), ) except Exception as e: verbose_proxy_logger.exception(e) @@ -822,15 +835,19 @@ async def delete_credential( _reject_non_admin_wif_fields( await named_credential_wif_fields(credential_name, prisma_client), user_api_key_dict ) - deleted: Final = await CredentialsRepository(prisma_client).delete_by_name(credential_name) - if deleted is None: + credentials_repository: Final = CredentialsRepository(prisma_client) + stored: Final = await credentials_repository.find_by_name(credential_name) + if stored is None: raise _not_found_unless_config_defined( credential_name, "Credential not found. Got credential name: " + credential_name ) + await credentials_repository.delete_by_name(stored.credential_name) ## DELETE FROM LITELLM ## - litellm.credential_list = [cred for cred in litellm.credential_list if cred.credential_name != credential_name] - await _purge_user_connections_for_credential(credential_name) + litellm.credential_list = [ + cred for cred in litellm.credential_list if cred.credential_name != stored.credential_name + ] + await _purge_user_connections_for_credential(stored.credential_name) return {"success": True, "message": "Credential deleted successfully"} except Exception as e: raise handle_exception_on_proxy(e) @@ -892,7 +909,7 @@ async def update_credential( from litellm.proxy.proxy_server import prisma_client try: - if credential.credential_name and credential.credential_name != credential_name: + if credential.credential_name and credential.credential_name.strip() != credential_name.strip(): raise ProxyException( message="credential_name is immutable. Set display_name to change how the credential is labeled.", type=ProxyErrorTypes.validation_error.value, @@ -917,8 +934,9 @@ async def update_credential( if db_credential is None: raise _not_found_unless_config_defined(credential_name, "Credential not found in DB.") _reject_non_admin_wif_fields(_stored_wif_fields(db_credential), user_api_key_dict) + stored_name: Final = db_credential.credential_name patch: Final = CredentialItem( - credential_name=credential_name, + credential_name=stored_name, display_name=( requested_display_name if "display_name" in credential.model_fields_set else db_credential.display_name ), @@ -931,7 +949,7 @@ async def update_credential( "dict[str, object]", jsonify_object(merged_credential.model_dump(exclude_none=True)) ) await credentials_repository.update_by_name( - credential_name, + stored_name, data={ **credential_object_jsonified, "display_name": merged_credential.display_name, @@ -940,14 +958,14 @@ async def update_credential( ) # Sync in-memory credential_list (skip if not in memory - e.g., proxy restarted) - _sync_in_memory_credential(patch, credential_name) + _sync_in_memory_credential(patch, stored_name) merged_values: Final = cast( # cast-ok: credential_values is a plain dict at runtime Mapping[object, object], merged_credential.credential_values ) still_per_user: Final = merged_values.get(GITHUB_COPILOT_AUTH_TYPE_KEY) == GITHUB_COPILOT_PER_USER_AUTH_TYPE if not still_per_user: - await _purge_user_connections_for_credential(credential_name) + await _purge_user_connections_for_credential(stored_name) return {"success": True, "message": "Credential updated successfully"} except Exception as e: diff --git a/litellm/repositories/credentials_repository.py b/litellm/repositories/credentials_repository.py index cb1bc1d101b..ff0b47d9316 100644 --- a/litellm/repositories/credentials_repository.py +++ b/litellm/repositories/credentials_repository.py @@ -72,8 +72,15 @@ class CredentialsRepository: return await self.table.create(data=data) async def find_by_name(self, credential_name: str) -> CredentialItem | None: + """The row named ``credential_name``, else the single row whose name only differs by + surrounding whitespace, which older versions stored verbatim from the UI.""" record: Final = await self.table.find_unique(where={"credential_name": credential_name}) - return self._to_model(record) + if record is not None: + return self._to_model(record) + stripped: Final = credential_name.strip() + candidates: Final = await self.table.find_many(where={"credential_name": {"contains": stripped}}) + padded: Final = tuple(candidate for candidate in candidates if candidate.credential_name.strip() == stripped) + return self._to_model(padded[0]) if len(padded) == 1 else None async def update_by_name( self, credential_name: str, data: Mapping[str, object] diff --git a/tests/unit/proxy/common_utils/test_credential_hydration.py b/tests/unit/proxy/common_utils/test_credential_hydration.py index ad75b5b8760..91250ddb0b6 100644 --- a/tests/unit/proxy/common_utils/test_credential_hydration.py +++ b/tests/unit/proxy/common_utils/test_credential_hydration.py @@ -4,6 +4,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest import litellm +from litellm.litellm_core_utils.credential_accessor import CredentialAccessor from litellm.proxy.common_utils.credential_hydration import ( effective_server_owned_wif_fields, hydrate_named_credential_authoritative, @@ -34,6 +35,26 @@ async def test_authoritative_hydrate_returns_an_encrypted_empty_value_as_empty(m assert resolved.credential_values == {"api_base": "", "openai_service_account_id": "user-1"} +@pytest.mark.asyncio +async def test_wif_gate_sees_the_padded_credential_a_trimmed_name_resolves_to(): + """Resolution maps ``corp-wif`` onto a credential stored as ``corp-wif ``, so the federation + gate must see that credential's server-owned fields too, or a team admin could swap them out.""" + padded = CredentialItem( + credential_name="corp-wif ", + credential_values={"anthropic_keycloak_token_url": "https://keycloak.internal/token"}, + credential_info={}, + source="config", + ) + with patch.object( + litellm, "credential_list", [padded] + ): # test-quality-ok: the in-memory config credential under test + resolved = CredentialAccessor.find_credential("corp-wif") + gated = await named_credential_wif_fields("corp-wif", None) + + assert resolved is padded + assert "anthropic_keycloak_token_url" in gated + + @pytest.mark.asyncio @pytest.mark.parametrize( "credential_api_key, expected", diff --git a/tests/unit/proxy/credential_endpoints/test_endpoints.py b/tests/unit/proxy/credential_endpoints/test_endpoints.py index 6aeceaf4356..ef0af6f4d61 100644 --- a/tests/unit/proxy/credential_endpoints/test_endpoints.py +++ b/tests/unit/proxy/credential_endpoints/test_endpoints.py @@ -12,6 +12,7 @@ from cryptography.hazmat.primitives import serialization from cryptography.hazmat.primitives.asymmetric import ec from fastapi import HTTPException, Request from fastapi.testclient import TestClient +from pydantic import BaseModel, ConfigDict, field_validator import litellm from litellm.models.credentials import CredentialSource @@ -65,6 +66,7 @@ def _list_credentials(): def _prisma_without_credential_rows() -> MagicMock: prisma_client = MagicMock() prisma_client.db.litellm_credentialstable.find_unique = AsyncMock(return_value=None) + prisma_client.db.litellm_credentialstable.find_many = AsyncMock(return_value=[]) return prisma_client @@ -1128,7 +1130,7 @@ class TestNonAdminCannotTouchAStoredWifCredential: assert litellm.credential_list == [config_credential] def test_proxy_admin_can_delete_a_config_only_wif_credential(self, restore_credential_list, monkeypatch): - """The gate lets the admin through to the row delete. The 400 that follows is the rule for + """The gate lets the admin through to the row lookup. The 405 that follows is the rule for every config-only credential (no row to delete, the entry is back on the next boot), so the in-memory entry stays put too.""" config_credential = _wif_credential("config-wif", source="config") @@ -1139,7 +1141,7 @@ class TestNonAdminCannotTouchAStoredWifCredential: assert response.status_code == 405, response.text assert response.headers["allow"] == "GET" assert "defined in config" in response.json()["error"]["message"] - repository.delete_by_name.assert_awaited_once_with("config-wif") + repository.delete_by_name.assert_not_awaited() assert litellm.credential_list == [config_credential] def test_non_admin_cannot_shadow_a_config_only_wif_credential(self, restore_credential_list, monkeypatch): @@ -1280,6 +1282,7 @@ class TestManagementReadsTheStoredCredential: ) prisma = MagicMock() prisma.db.litellm_credentialstable.find_unique = AsyncMock(return_value=None) + prisma.db.litellm_credentialstable.find_many = AsyncMock(return_value=[]) with patch.object( litellm, "credential_list", [only_in_memory] @@ -1318,7 +1321,11 @@ def test_delete_credential_still_answers_200_and_drops_the_credential_from_memor credential_values={"api_key": "sk-keep"}, credential_info={}, ) - credential_store(in_memory=(stored, survivor), delete_by_name=AsyncMock(return_value=MagicMock())) + credential_store( + in_memory=(stored, survivor), + find_by_name=AsyncMock(return_value=stored), + delete_by_name=AsyncMock(return_value=MagicMock()), + ) response = _delete_credential("doomed") @@ -1716,6 +1723,7 @@ def test_deleting_the_credential_purges_user_connections(monkeypatch, credential rows = [_connection_row(user_id="u1"), _connection_row(user_id="u2")] credential_store( in_memory=[_per_user_credential()], + find_by_name=AsyncMock(return_value=_per_user_credential()), delete_by_name=AsyncMock(return_value=_per_user_credential()), ) prisma_client = MagicMock() @@ -1725,6 +1733,7 @@ def test_deleting_the_credential_purges_user_connections(monkeypatch, credential prisma_client.db.litellm_userprovidercredentials = table prisma_client.writer_db = prisma_client.db prisma_client.db.litellm_credentialstable.find_unique = AsyncMock(return_value=None) + prisma_client.db.litellm_credentialstable.find_many = AsyncMock(return_value=[]) monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", prisma_client) monkeypatch.setattr("litellm.proxy.proxy_server.user_api_key_cache", DualCache()) @@ -2567,3 +2576,155 @@ class TestCredentialDisplayName: assert (by_name["display_name"], by_name["source"]) == ("Prod OpenAI", "db") assert config_by_name["source"] == "config" assert listed["openai-prod"]["credential_values"]["api_key"] != "sk-old" + + +class _CredentialRow(BaseModel): + model_config = ConfigDict(frozen=True) + + credential_name: str + display_name: str | None = None + credential_values: dict[str, object] + credential_info: dict[str, object] + + @field_validator("credential_values", "credential_info", mode="before") + @classmethod + def _decode_json_column(cls, value: object) -> object: + return json.loads(value) if isinstance(value, str) else value + + +class _CredentialsTable: + """In-memory litellm_credentialstable keyed by the exact stored name, as the unique index is.""" + + def __init__(self, *names: str) -> None: + self.rows = { + name: _CredentialRow(credential_name=name, credential_values={}, credential_info={}) for name in names + } + + async def find_unique(self, where): + return self.rows.get(where["credential_name"]) + + async def find_many(self, where): + needle = where["credential_name"]["contains"] + return [row for name, row in self.rows.items() if needle in name] + + async def create(self, data): + row = _CredentialRow.model_validate(data) + self.rows[row.credential_name] = row + return row + + async def update(self, where, data): + name = where["credential_name"] + self.rows[name] = _CredentialRow.model_validate({**self.rows[name].model_dump(), **data}) + return self.rows[name] + + async def delete(self, where): + return self.rows.pop(where["credential_name"], None) + + +@pytest.fixture +def credentials_table(monkeypatch, restore_credential_list): + """A database holding the given credential names, served through the real repository.""" + + def install(*names: str) -> _CredentialsTable: + table = _CredentialsTable(*names) + prisma_client = MagicMock() + prisma_client.db.litellm_credentialstable = table + monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", prisma_client) + monkeypatch.setattr("litellm.proxy.proxy_server.master_key", "sk-test-master") + monkeypatch.setattr( + litellm, + "credential_list", + [ + CredentialItem(credential_name=name, credential_values={"api_key": "sk-old"}, credential_info={}) + for name in names + ], + ) + return table + + return install + + +class TestCredentialNameWhitespace: + def test_create_stores_the_name_without_surrounding_whitespace(self, credentials_table): + table = credentials_table() + + response = _post_credential( + {"credential_name": " test-cred ", "credential_values": {"api_key": "sk-new"}, "credential_info": {}} + ) + + assert response.status_code == 200, response.text + assert list(table.rows) == ["test-cred"] + assert [entry["credential_name"] for entry in _list_credentials().json()["credentials"]] == ["test-cred"] + + @pytest.mark.parametrize("credential_name", ["", " "]) + def test_create_rejects_a_blank_name(self, credentials_table, credential_name): + table = credentials_table() + + response = _post_credential( + {"credential_name": credential_name, "credential_values": {"api_key": "sk-new"}, "credential_info": {}} + ) + + assert response.status_code == 400, response.text + assert response.json()["error"]["param"] == "credential_name" + assert table.rows == {} + assert litellm.credential_list == [] + + def test_get_by_trimmed_name_finds_a_legacy_padded_credential(self, credentials_table): + credentials_table("Openrouter 11/08 ") + + response = _call_as("GET", "/credentials/by_name/Openrouter%2011/08") + + assert response.status_code == 200, response.text + assert response.json()["credential_name"] == "Openrouter 11/08 " + + def test_delete_by_trimmed_name_removes_a_legacy_padded_credential(self, credentials_table): + table = credentials_table("Openrouter 11/08 ", "keeper") + + response = _delete_credential("Openrouter%2011/08") + + assert response.status_code == 200, response.text + assert list(table.rows) == ["keeper"] + assert [credential.credential_name for credential in litellm.credential_list] == ["keeper"] + + def test_patch_by_trimmed_name_updates_the_legacy_padded_row_in_place(self, credentials_table): + table = credentials_table("Openrouter 11/08 ") + + response = _patch_credential( + "Openrouter%2011/08", + { + "credential_name": "Openrouter 11/08 ", + "credential_values": {"api_key": "sk-new"}, + "credential_info": {}, + }, + ) + + assert response.status_code == 200, response.text + assert list(table.rows) == ["Openrouter 11/08 "] + assert "api_key" in table.rows["Openrouter 11/08 "].credential_values + assert [c.credential_values["api_key"] for c in litellm.credential_list] == ["sk-new"] + + def test_an_exact_match_wins_over_a_padded_one(self, credentials_table): + table = credentials_table("foo", "foo ") + + response = _delete_credential("foo") + + assert response.status_code == 200, response.text + assert list(table.rows) == ["foo "] + + def test_a_trimmed_name_matching_several_padded_rows_deletes_nothing(self, credentials_table): + table = credentials_table("foo ", " foo") + + response = _delete_credential("foo") + + assert response.status_code == 404, response.text + assert sorted(table.rows) == [" foo", "foo "] + + @pytest.mark.parametrize("method", ["GET", "DELETE"]) + def test_a_trimmed_name_does_not_match_a_longer_padded_name(self, credentials_table, method): + table = credentials_table("foobar ") + path = "/credentials/by_name/foo" if method == "GET" else "/credentials/foo" + + response = _call_as(method, path) + + assert response.status_code == 404, response.text + assert list(table.rows) == ["foobar "] diff --git a/ui/litellm-dashboard/src/components/model_add/CredentialModal.tsx b/ui/litellm-dashboard/src/components/model_add/CredentialModal.tsx index d0e28a0e273..878891127d9 100644 --- a/ui/litellm-dashboard/src/components/model_add/CredentialModal.tsx +++ b/ui/litellm-dashboard/src/components/model_add/CredentialModal.tsx @@ -131,7 +131,7 @@ export default function CredentialModal({ } const values = projectMountedValues(registry, form.getValues); const meta = { - credential_name: String(values.credential_name), + credential_name: String(values.credential_name).trim(), custom_llm_provider: String(values.custom_llm_provider), ...displayNameChange(values.display_name, existingCredential), }; diff --git a/ui/litellm-dashboard/src/components/networking.test.ts b/ui/litellm-dashboard/src/components/networking.test.ts index e7a55870345..c8977e7363b 100644 --- a/ui/litellm-dashboard/src/components/networking.test.ts +++ b/ui/litellm-dashboard/src/components/networking.test.ts @@ -544,6 +544,29 @@ describe("teamInfoCall", () => { }); }); +describe("credential calls", () => { + const credentialName = "a b/c "; + + const requestedPath = (mockFetch: ReturnType): string => { + const [url] = mockFetch.mock.calls[0]; + return url instanceof Request ? new URL(url.url).pathname : new URL(url.toString(), "http://example.com").pathname; + }; + + it.each([ + ["delete", () => Networking.credentialDeleteCall("token", credentialName), "/credentials/a%20b%2Fc%20"], + ["update", () => Networking.credentialUpdateCall("token", credentialName, {}), "/credentials/a%20b%2Fc%20"], + ["get", () => Networking.credentialGetCall("token", credentialName, null), "/credentials/by_name/a%20b%2Fc%20"], + ])("%s keeps the trailing space and slash of the credential name in the path", async (_, call, expectedPath) => { + const mockFetch = vi.spyOn(global, "fetch").mockResolvedValue(new Response("{}", { status: 200 })); + try { + await call(); + expect(requestedPath(mockFetch).endsWith(expectedPath)).toBe(true); + } finally { + mockFetch.mockRestore(); + } + }); +}); + describe("uiSpendLogsCall exclude_internal_health_checks serialization", () => { const originalFetch = global.fetch; diff --git a/ui/litellm-dashboard/src/components/networking.tsx b/ui/litellm-dashboard/src/components/networking.tsx index 4df34275923..535e7c557b8 100644 --- a/ui/litellm-dashboard/src/components/networking.tsx +++ b/ui/litellm-dashboard/src/components/networking.tsx @@ -2648,7 +2648,7 @@ export const credentialGetCall = async (accessToken: string, credentialName: str let path = `/credentials`; if (credentialName) { - path += `/by_name/${credentialName}`; + path += `/by_name/${encodeURIComponent(credentialName)}`; } else if (modelId) { path += `/by_model/${modelId}`; } @@ -2664,7 +2664,7 @@ export const credentialGetCall = async (accessToken: string, credentialName: str export const credentialDeleteCall = async (accessToken: string, credentialName: string) => { try { - const data = await apiClient.delete(`/credentials/${credentialName}`, { accessToken }); + const data = await apiClient.delete(`/credentials/${encodeURIComponent(credentialName)}`, { accessToken }); return data; // Handle success - you might want to update some state or UI based on the created key } catch (error) { @@ -2741,7 +2741,7 @@ export const credentialUpdateCall = async ( } } - const data = await apiClient.patch(`/credentials/${credentialName}`, { + const data = await apiClient.patch(`/credentials/${encodeURIComponent(credentialName)}`, { accessToken, body: { ...formValues, // Include formValues in the request body