[Feature] Disable Custom Virtual Key Values via UI Setting#23812
[Feature] Disable Custom Virtual Key Values via UI Setting#23812yuneng-jiang merged 5 commits intolitellm_yj_march_16_2026from
Conversation
Add disable_custom_api_keys UI setting that prevents users from specifying custom key values during key generation and regeneration. When enabled, all keys must be auto-generated, eliminating the risk of key hash collisions in multi-tenant environments. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR introduces a Key changes:
Outstanding concerns (from prior review threads, still unresolved in current code):
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/key_management_endpoints.py | Adds _check_custom_key_allowed guard called in _common_key_generation_helper and get_new_token; get_new_token was converted from sync to async; the security check fires after the _enforce_unique_key_alias DB lookup rather than before it (fail-fast violation), and uses a strict is True identity check instead of a truthiness test — both flagged in prior review threads and still unresolved. |
| litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py | Adds disable_custom_api_keys field to UISettings model and its allowlist; cache is correctly refreshed on update; no issues found. |
| tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py | Six new unit tests added; two existing tests updated to async signature; test_get_new_token_auto_generates_when_custom_keys_disabled sets up a mock that is never invoked (unreachable code path), giving a false sense of cache-bypass verification. |
| ui/litellm-dashboard/src/components/Settings/AdminSettings/UISettings/UISettings.tsx | Adds a toggle for disable_custom_api_keys following the same pattern as existing toggles; no issues found. |
| ui/litellm-dashboard/src/components/organisms/create_key_button.tsx | Reads disable_custom_api_keys and conditionally excludes the key field from the Advanced Settings SchemaFormFields; however, the stale form value issue (field removed from render but not cleared from AntD Form state) is flagged in a prior review thread and remains unresolved. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[POST /key/generate or /key/regenerate] --> B{custom key value\nprovided?}
B -- No --> C[Auto-generate sk- token]
C --> D[Return new key]
B -- Yes --> E[_check_custom_key_allowed]
E --> F[get_ui_settings_cached\ncache TTL=10min]
F --> G{disable_custom_api_keys\n== True?}
G -- No --> H[Proceed with custom key]
H --> I[Validate sk- prefix]
I --> J[Hash and store key]
J --> D
G -- Yes --> K[Raise HTTP 403\nCustom keys disabled]
K --> L[Return 403 to caller]
style K fill:#ff6b6b,color:#fff
style G fill:#ffd93d
style F fill:#a8e6cf
Last reviewed commit: 471e0f1
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return | ||
|
|
||
| ui_settings = await get_ui_settings_cached() | ||
| if ui_settings.get("disable_custom_api_keys", False) is True: |
There was a problem hiding this comment.
is True equality check
ui_settings.get("disable_custom_api_keys", False) is True will only match the Python boolean True. If the value is stored/deserialized as the string "true" or the integer 1 (both of which are truthy), the guard silently passes and the setting is ignored. The standard Python idiom is a simple truthiness check:
| if ui_settings.get("disable_custom_api_keys", False) is True: | |
| if ui_settings.get("disable_custom_api_keys", False): |
| async def _check_custom_key_allowed(custom_key_value: Optional[str]) -> None: | ||
| """Raise 403 if custom API keys are disabled and a custom key was provided.""" | ||
| if custom_key_value is None: | ||
| return | ||
|
|
||
| ui_settings = await get_ui_settings_cached() | ||
| if ui_settings.get("disable_custom_api_keys", False) is True: | ||
| verbose_proxy_logger.warning( | ||
| "Custom API key rejected: disable_custom_api_keys is enabled" | ||
| ) | ||
| raise HTTPException( | ||
| status_code=403, | ||
| detail={ | ||
| "error": "Custom API key values are disabled by your administrator. Keys must be auto-generated." | ||
| }, | ||
| ) |
There was a problem hiding this comment.
_check_custom_key_allowed calls get_ui_settings_cached() unconditionally — even when custom_key_value is None (i.e., the common case where the user does not supply a custom key). The early-return only fires before get_ui_settings_cached() is invoked, so an unnecessary cache/DB lookup still occurs for every auto-generated key request. Per project rules, critical-path code must avoid unnecessary DB/cache requests.
Move the None guard so that the settings lookup is skipped entirely when no custom key is being requested:
async def _check_custom_key_allowed(custom_key_value: Optional[str]) -> None:
"""Raise 403 if custom API keys are disabled and a custom key was provided."""
if custom_key_value is None:
return # no custom key – nothing to check
ui_settings = await get_ui_settings_cached()
if ui_settings.get("disable_custom_api_keys", False):
...Wait — that IS the current code. The issue is that the None check at line 104 returns before the get_ui_settings_cached() call, which is correct. However, _common_key_generation_helper (line 696) and get_new_token (line 3509) call this function inside the request path on every key-generation and key-regeneration call. While the cache mitigates the DB overhead, the async cache lookup still adds latency on every request even when custom_key_value is None would have bailed out first — specifically in get_new_token, the call happens even for the auto-generate branch (data.new_key is None) since _check_custom_key_allowed is called after the if data and data.new_key is not None guard.
Actually, let me re-read:
async def get_new_token(data: Optional[RegenerateKeyRequest]) -> str:
if data and data.new_key is not None:
# Reject custom key values if disabled by admin
await _check_custom_key_allowed(data.new_key)OK so in get_new_token, the call is inside the if data and data.new_key is not None: guard, so the None check inside _check_custom_key_allowed is redundant but not harmful for that path.
For _common_key_generation_helper (line 696), data.key may be None, and _check_custom_key_allowed is called unconditionally — but then it returns early at the None check on line 104. So actually there is no extra cache hit when data.key is None.
Wait, but that means every call to _common_key_generation_helper — even those where no custom key is provided — invokes an async function that starts executing (just to check None). The function itself does not make a cache call when custom_key_value is None, due to the early return. So actually the implementation is correct on performance grounds.
Let me remove this comment and only keep the stricter logic issue (is True vs truthy).
Actually, let me reconsider the is True issue. This is still a valid concern. Let me check what value JSON deserialization would give - JSON booleans are parsed as Python booleans, so True/False. If the value was stored as an integer by accident it would fail. But for the standard JSON boolean, is True would work correctly. Let me think about whether this is a real issue...
Actually, looking at the code in get_ui_settings_cached:
ui_settings = json.loads(raw) if isinstance(raw, str) else dict(raw)JSON parsing will give proper Python booleans for JSON booleans. So is True would work correctly for properly stored values. However, using is True instead of a truthiness check is still a style concern - it's non-idiomatic and could fail in edge cases (e.g., if someone stores 1 or "true" directly).
I'll keep this as a style/logic note.
Now let me think about other issues:
-
UI bypass: The UI hides the
keyfield, but this is purely a client-side mitigation. A malicious user could still send thekeyfield in the JSON body directly (bypassing the UI). The backend check in_check_custom_key_allowedis the actual enforcement mechanism, and it does work at the backend level. This is the correct design. -
No exemption for proxy admins: The PR description says "No exemptions — proxy admins are also blocked when the setting is enabled." This could be a design concern - maybe proxy admins should be able to bypass this restriction? But it's stated as an explicit design decision, so I'll mention it as a design note rather than a bug.
-
Cache invalidation: The 10-minute TTL means that if an admin disables
disable_custom_api_keys, there's a window where users can still create custom keys. This is acceptable for a UI setting. -
Test mock patching: In the tests, the monkeypatch targets
litellm.proxy.management_endpoints.key_management_endpoints.get_ui_settings_cached. This is correct as the function is imported directly. -
Let me think about whether there's anything in the UI changes that has issues...
Looking at the UI:
const disableCustomApiKeys = Boolean(uiSettingsData?.values?.disable_custom_api_keys);This just hides the field. The backend does the actual enforcement.
-
Missing UI feedback: When
disable_custom_api_keysis enabled, thekeyfield is simply hidden from the "Advanced Settings" accordion. There's no visible message to users explaining why the field is missing. This is a UX improvement suggestion, not a bug. -
The
is Truecheck: As I discussed, this is using strict identity comparison instead of truthiness. This is a potential logic issue if values are stored in different formats.
Let me focus on the most meaningful issues:
- The
is Truestrict comparison (could silently fail to enforce the restriction if value isn't PythonTrue) - The PR description says "No exemptions — proxy admins are also blocked" - this might be a design concern worth flagging, as it deviates from usual patterns where proxy admins get elevated permissions
Let me also check if there's any issue with the UI change - does the form still submit the key field even when hidden? Looking at the code, excludedFields should prevent it. Let me verify by reading more context...
Actually, let me check what excludedFields does in this component - does it prevent the field from being submitted, or just hide it visually? If it only hides the field visually but still submits the value (e.g., if the form had a pre-filled value), there could be an issue where the UI sends the field and the backend rejects it.
Actually I think the excludedFields likely excludes the field from the form schema/display entirely, so it wouldn't be submitted. The backend check is still the authoritative guard.
- Stale cache on toggle: If an admin enables
disable_custom_api_keys, existing users with the UI still showing thekeyfield (before their browser refreshes) could still submit a custom key. The backend would reject it (after the cache propagates), but the UX would be jarring. This is a minor UX issue.
Let me look at the is True check more carefully. This is the main logic issue worth flagging.
Let me also think about race conditions - get_ui_settings_cached reads from cache or DB. If the admin has just changed the setting, there's a 10-minute window where custom keys might still be accepted. But this is the expected behavior with TTL caching and is not a bug per se.
Let me finalize the review now.
Without this field on the model, GET /get/ui_settings omits the setting from the response and field_schema, preventing the UI from reading it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| "budget_duration", | ||
| "tpm_limit", | ||
| "rpm_limit", | ||
| ...(disableCustomApiKeys ? ["key"] : []), |
There was a problem hiding this comment.
When disableCustomApiKeys transitions from false to true mid-session (e.g. the admin toggles the setting while a user has the "Create Key" modal open), Ant Design's Form instance retains any value the user had already typed into the key field even after it is removed from the rendered fields. The SchemaFormFields useEffect re-runs because excludedFields changed, but it only calls form.setFieldsValue with default values for non-excluded fields — it never clears the now-excluded key field. As a result form.validateFields() will still include key: "sk-..." in the submitted payload, causing a confusing 403 from the backend for a field the user can no longer even see.
The fix is to explicitly clear the field when the flag becomes active:
useEffect(() => {
if (disableCustomApiKeys) {
form.setFieldValue("key", undefined);
}
}, [disableCustomApiKeys, form]);Add this useEffect alongside the existing form / disableCustomApiKeys declarations.
| from litellm.proxy.management_endpoints.key_management_endpoints import ( | ||
| _check_custom_key_allowed, | ||
| ) | ||
|
|
||
| monkeypatch.setattr( | ||
| "litellm.proxy.management_endpoints.key_management_endpoints.get_ui_settings_cached", | ||
| AsyncMock(return_value={}), | ||
| ) | ||
|
|
||
| # Should not raise | ||
| await _check_custom_key_allowed("sk-custom-key-123") | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_check_custom_key_allowed_none_key_always_passes(monkeypatch): | ||
| """_check_custom_key_allowed does nothing when key is None, even if setting is on.""" | ||
| from unittest.mock import AsyncMock | ||
|
|
||
| from litellm.proxy.management_endpoints.key_management_endpoints import ( | ||
| _check_custom_key_allowed, | ||
| ) |
There was a problem hiding this comment.
None-key test
_check_custom_key_allowed(None) returns early at line 104 (if custom_key_value is None: return) before ever calling get_ui_settings_cached. The AsyncMock set up in test_check_custom_key_allowed_none_key_always_passes is therefore never invoked, which means the test doesn't actually verify that the cache is skipped on a None key — and the mock gives a false sense of security about the DB-bypass guarantee.
Consider either removing the mock entirely (which makes the contract explicit: no DB call needed at all), or asserting that the mock was not called to document the expected fast-path behaviour:
mock_get_settings = AsyncMock(return_value={"disable_custom_api_keys": True})
monkeypatch.setattr(
"litellm.proxy.management_endpoints.key_management_endpoints.get_ui_settings_cached",
mock_get_settings,
)
await _check_custom_key_allowed(None)
mock_get_settings.assert_not_awaited() # None key must not trigger a cache/DB lookupAdds a toggle switch to the admin UI Settings page so administrators can enable/disable custom API key values without making direct API calls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
yuneng-jiang
changed the title
| # Reject custom key values if disabled by admin | ||
| await _check_custom_key_allowed(data.key) |
There was a problem hiding this comment.
The _check_custom_key_allowed call is positioned after _enforce_unique_key_alias, which performs a database lookup whenever key_alias is set. When the disable_custom_api_keys flag is enabled, a request that provides both a custom key and a key_alias will trigger the alias-uniqueness lookup unnecessarily before receiving the 403 response.
Moving _check_custom_key_allowed to before _validate_key_alias_format and _enforce_unique_key_alias ensures the security guard fires before any expensive lookups, which is the standard fail-fast pattern for access controls.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| async def test_get_new_token_auto_generates_when_custom_keys_disabled(monkeypatch): | ||
| """get_new_token auto-generates a key when new_key is None, even if setting is on.""" | ||
| from unittest.mock import AsyncMock | ||
|
|
||
| from litellm.proxy._types import RegenerateKeyRequest | ||
| from litellm.proxy.management_endpoints.key_management_endpoints import ( | ||
| get_new_token, | ||
| ) | ||
|
|
||
| monkeypatch.setattr( | ||
| "litellm.proxy.management_endpoints.key_management_endpoints.get_ui_settings_cached", | ||
| AsyncMock(return_value={"disable_custom_api_keys": True}), | ||
| ) | ||
|
|
||
| data = RegenerateKeyRequest() # no new_key | ||
| result = await get_new_token(data) | ||
|
|
||
| assert result.startswith("sk-") |
There was a problem hiding this comment.
The AsyncMock for get_ui_settings_cached set up in this test is never invoked. When data.new_key is None, get_new_token enters the else branch immediately:
async def get_new_token(data):
if data and data.new_key is not None: # ← False when new_key is None
await _check_custom_key_allowed(...) # ← never reached
else:
new_token = f"sk-{secrets.token_urlsafe(...)}" # ← always taken here_check_custom_key_allowed (and therefore get_ui_settings_cached) is never called, so the mock has no effect on the test outcome. The mock creates a false impression that this test verifies cache-bypass behaviour, similar to the pattern already identified in test_check_custom_key_allowed_none_key_always_passes. Consider removing the mock to make the test contract explicit, or asserting mock_get_settings.assert_not_awaited() to document the expected fast-path.
| async def test_get_new_token_auto_generates_when_custom_keys_disabled(monkeypatch): | |
| """get_new_token auto-generates a key when new_key is None, even if setting is on.""" | |
| from unittest.mock import AsyncMock | |
| from litellm.proxy._types import RegenerateKeyRequest | |
| from litellm.proxy.management_endpoints.key_management_endpoints import ( | |
| get_new_token, | |
| ) | |
| monkeypatch.setattr( | |
| "litellm.proxy.management_endpoints.key_management_endpoints.get_ui_settings_cached", | |
| AsyncMock(return_value={"disable_custom_api_keys": True}), | |
| ) | |
| data = RegenerateKeyRequest() # no new_key | |
| result = await get_new_token(data) | |
| assert result.startswith("sk-") | |
| @pytest.mark.asyncio | |
| async def test_get_new_token_auto_generates_when_custom_keys_disabled(monkeypatch): | |
| """get_new_token auto-generates a key when new_key is None, even if setting is on.""" | |
| from unittest.mock import AsyncMock | |
| from litellm.proxy._types import RegenerateKeyRequest | |
| from litellm.proxy.management_endpoints.key_management_endpoints import ( | |
| get_new_token, | |
| ) | |
| mock_get_settings = AsyncMock(return_value={"disable_custom_api_keys": True}) | |
| monkeypatch.setattr( | |
| "litellm.proxy.management_endpoints.key_management_endpoints.get_ui_settings_cached", | |
| mock_get_settings, | |
| ) | |
| data = RegenerateKeyRequest() # no new_key | |
| result = await get_new_token(data) | |
| assert result.startswith("sk-") | |
| mock_get_settings.assert_not_awaited() # None new_key must not trigger cache/DB |
1 participant
Relevant issues
Closes LIT-1710
Summary
Problem
Users can override their Virtual key value in Advanced Settings, which introduces a security risk — if two users set the same custom key, they generate identical key hashes, potentially causing key collision or unauthorized access in multi-tenant environments.
Fix
disable_custom_api_keysUI setting that admins can toggle from the UI Settings page or viaPATCH /update/ui_settingskeyvalue on/key/generate(403) and any customnew_keyon/key/regenerate(403)keyfield from Advanced Settings in the Create Key form when the setting is onTesting
_check_custom_key_allowedandget_new_tokenintegrationget_new_tokentests updated to match async signatureType
🆕 New Feature
✅ Test