Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions litellm/proxy/management_endpoints/key_management_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -2120,7 +2120,10 @@ async def update_key_fn(
data=data, existing_key_row=existing_key_row
)

_validate_key_alias_format(key_alias=non_default_values.get("key_alias", None))
# Only validate key_alias format if it's actually being changed
new_key_alias = non_default_values.get("key_alias", None)
if new_key_alias != existing_key_row.key_alias:
_validate_key_alias_format(key_alias=new_key_alias)
Comment on lines +2124 to +2126
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 None from missing key vs. explicit None are indistinguishable here

non_default_values.get("key_alias", None) returns None both when the client omits key_alias from the payload (field not present after exclude_unset=True) AND when the client explicitly sends "key_alias": null to clear the alias.

If the existing key has a non-None alias (e.g. "user@domain.com") and the client sends a payload without key_alias at all, this evaluates to None != "user@domain.com"True, and _validate_key_alias_format(None) is called. _validate_key_alias_format handles None safely (returns early), so there is no runtime error. However, the semantics are slightly misleading: the condition fires as though the alias "changed to None" even when it wasn't touched.

This is a pre-existing ambiguity and not a regression introduced by this PR, but it is worth documenting with a comment so future maintainers understand the intent.


await _enforce_unique_key_alias(
key_alias=non_default_values.get("key_alias", None),
Expand Down Expand Up @@ -3579,7 +3582,10 @@ async def _execute_virtual_key_regeneration(
non_default_values = await prepare_key_update_data(
data=data, existing_key_row=key_in_db
)
_validate_key_alias_format(key_alias=non_default_values.get("key_alias"))
# Only validate key_alias format if it's actually being changed
new_key_alias = non_default_values.get("key_alias")
if new_key_alias != key_in_db.key_alias:
_validate_key_alias_format(key_alias=new_key_alias)
verbose_proxy_logger.debug("non_default_values: %s", non_default_values)
update_data.update(non_default_values)
update_data = prisma_client.jsonify_object(data=update_data)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7833,3 +7833,128 @@ async def test_admin_can_remove_user_id(self):
prisma_client=mock_prisma_client,
user_api_key_cache=MagicMock(),
)


class TestKeyAliasSkipValidationOnUnchanged:
"""
Test that updating/regenerating a key without changing its key_alias
does NOT re-validate the alias. This prevents legacy aliases (created
before stricter validation rules) from blocking edits to other fields.
"""

@pytest.fixture(autouse=True)
def enable_validation(self):
litellm.enable_key_alias_format_validation = True
yield
litellm.enable_key_alias_format_validation = False

@pytest.fixture
def mock_prisma(self):
prisma = MagicMock()
prisma.db = MagicMock()
prisma.db.litellm_verificationtoken = MagicMock()
prisma.get_data = AsyncMock(return_value=None) # no duplicate alias
prisma.update_data = AsyncMock(return_value=None)
prisma.jsonify_object = MagicMock(side_effect=lambda data: data)
return prisma

@pytest.fixture
def existing_key_with_legacy_alias(self):
"""A key whose alias contains '@' — valid now, but simulates a legacy alias."""
return LiteLLM_VerificationToken(
token="hashed_token_123",
key_alias="user@domain.com",
team_id="team-1",
models=[],
max_budget=100.0,
)

@pytest.mark.asyncio
async def test_update_key_unchanged_legacy_alias_passes(
self, mock_prisma, existing_key_with_legacy_alias
Comment on lines +7873 to +7874
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 mock_prisma fixture parameter is unused in tests 1–3

The mock_prisma fixture is requested as a parameter in test_update_key_unchanged_legacy_alias_passes, test_update_key_changed_alias_still_validated, and test_update_key_changed_to_valid_alias_passes, but its value is never referenced inside any of those test bodies. Since none of the tests actually call update_key_fn or interact with the DB, the fixture is dead weight here. If the intent is to guard against real DB calls, the fixture should be used; if there is no such guard needed, the parameter can be removed.

):
"""
Updating a key without changing its key_alias should skip format
validation — even if the alias wouldn't pass current rules.
"""
from litellm.proxy.management_endpoints.key_management_endpoints import (
_validate_key_alias_format,
)

# Temporarily make the regex reject '@' to simulate stricter rules
import re
from litellm.proxy.management_endpoints import key_management_endpoints as mod

original_pattern = mod._KEY_ALIAS_PATTERN
mod._KEY_ALIAS_PATTERN = re.compile(
r"^[a-zA-Z0-9][a-zA-Z0-9_\-/\.]{0,253}[a-zA-Z0-9]$"
)
try:
# Confirm the alias WOULD fail validation directly
with pytest.raises(ProxyException):
_validate_key_alias_format("user@domain.com")

# But prepare_key_update_data + the skip logic should allow it
# Simulate what update_key_fn does: alias is in non_default_values
# but matches existing_key_row.key_alias => skip validation
existing_alias = existing_key_with_legacy_alias.key_alias
new_alias = "user@domain.com" # same as existing
assert new_alias == existing_alias # unchanged

# This is the core logic from update_key_fn:
if new_alias != existing_alias:
_validate_key_alias_format(new_alias)
# No exception raised — test passes
Comment on lines +7904 to +7907
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Tests replicate logic inline instead of calling the actual endpoints

The new tests in TestKeyAliasSkipValidationOnUnchanged manually copy-paste the if new_alias != existing_alias: _validate_key_alias_format(new_alias) conditional inline rather than calling update_key_fn or _execute_virtual_key_regeneration directly. This means these tests will not catch a regression if someone later removes or misplaces the skip logic inside the actual endpoint functions.

For example, if update_key_fn is refactored and the if new_key_alias != existing_key_row.key_alias: guard is accidentally dropped, all four tests here would still pass while the production bug reappears.

For stronger regression protection, consider calling the actual functions with a mocked prisma_client (as the existing TestLIT1884KeyUpdateValidation class does) rather than re-implementing the conditional inline.

finally:
mod._KEY_ALIAS_PATTERN = original_pattern

@pytest.mark.asyncio
async def test_update_key_changed_alias_still_validated(
Comment on lines +7911 to +7912
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 @pytest.mark.asyncio on effectively synchronous tests

test_update_key_changed_alias_still_validated, test_update_key_changed_to_valid_alias_passes, and test_update_key_alias_none_skips_validation are all decorated with @pytest.mark.asyncio but contain no await expressions — they are effectively synchronous. Depending on the version of pytest-asyncio in use, this can produce warnings or unexpected behavior in strict mode. These three tests should either be plain def functions (removing async def and the decorator) or actually exercise async code paths.

self, mock_prisma, existing_key_with_legacy_alias
):
"""
When the alias IS being changed, validation should still run.
"""
from litellm.proxy.management_endpoints.key_management_endpoints import (
_validate_key_alias_format,
)

existing_alias = existing_key_with_legacy_alias.key_alias
new_alias = "!invalid!"

assert new_alias != existing_alias
with pytest.raises(ProxyException):
if new_alias != existing_alias:
_validate_key_alias_format(new_alias)

@pytest.mark.asyncio
async def test_update_key_changed_to_valid_alias_passes(
self, mock_prisma, existing_key_with_legacy_alias
):
"""
Changing the alias to a new valid value should pass validation.
"""
from litellm.proxy.management_endpoints.key_management_endpoints import (
_validate_key_alias_format,
)

existing_alias = existing_key_with_legacy_alias.key_alias
new_alias = "new-valid-alias"

assert new_alias != existing_alias
# Should not raise
if new_alias != existing_alias:
_validate_key_alias_format(new_alias)

@pytest.mark.asyncio
async def test_update_key_alias_none_skips_validation(self):
"""
When key_alias is not in the update payload (None), validation
should be skipped regardless.
"""
from litellm.proxy.management_endpoints.key_management_endpoints import (
_validate_key_alias_format,
)

# None alias should always pass
_validate_key_alias_format(None)
Loading