Skip to content

[Fix] Key Expiry Default Duration - #22712

Closed
yuneng-jiang wants to merge 9 commits into
mainfrom
litellm_key_expiry_max_validation
Closed

[Fix] Key Expiry Default Duration#22712
yuneng-jiang wants to merge 9 commits into
mainfrom
litellm_key_expiry_max_validation

Conversation

@yuneng-jiang

Copy link
Copy Markdown
Contributor

Relevant issues

Summary

Problem

Two related issues with virtual key expiration validation against a team's team_member_key_duration limit:

  1. Key creation: When a user entered a duration longer than the team maximum, the backend silently capped it to the team max and returned a success response. The UI showed success, but after refresh the key had a different expiry than what was entered — giving the user no indication their value was changed.

  2. Key regeneration: The regeneration endpoint (POST /key/{key}/regenerate) never validated the requested duration against the team's limit at all. The user-provided duration was stored as-is, allowing the team maximum to be bypassed entirely.

Fix

  • In _common_key_generation_helper: added a duration check against team_table.metadata["team_member_key_duration"] before the enterprise params block. Returns HTTP 400 if the requested duration exceeds the team max.
  • In _execute_virtual_key_regeneration: added _validate_regenerate_key_duration_against_team helper that looks up the key's team, checks team_member_key_duration, and returns HTTP 400 if exceeded.

Both checks treat "-1" (never-expires) as infinite, which always exceeds a finite team max.

Testing

11 new unit tests added to tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py:

  • TestCommonKeyGenerationHelperTeamDurationValidation (5 tests): key creation raises 400 when duration exceeds team max, passes when within limit, skips when no duration or no team
  • TestValidateRegenerateKeyDurationAgainstTeam (6 tests): regeneration raises 400 when duration exceeds team max, passes when within limit, skips when no data/duration/team

Type

🐛 Bug Fix
✅ Test

Virtual keys created or regenerated with a duration exceeding the team's
team_member_key_duration limit were silently accepted. On creation, the
backend silently capped the value giving the user a misleading success
response. On regeneration, the user value was stored as-is, bypassing
the team limit entirely.

Add a duration check in _common_key_generation_helper (key creation) and
a _validate_regenerate_key_duration_against_team helper called from
_execute_virtual_key_regeneration (key regeneration). Both raise HTTP 400
when the requested duration exceeds the team maximum.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vercel

vercel Bot commented Mar 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
litellm Error Error Mar 6, 2026 1:23am

Request Review

@greptile-apps

greptile-apps Bot commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes two virtual key expiration enforcement gaps: (1) key creation silently capped durations instead of returning an error, and (2) the /key/{key}/regenerate endpoint performed no team-max duration validation at all. The fix adds explicit HTTP 400 responses for both paths and introduces a "Never Expires" checkbox in the UI that maps to an explicit null duration, deprecating the old "-1" sentinel.

Key changes:

  • Backend (key creation): New team-max validation block in _common_key_generation_helper raises HTTP 400 when duration > team_member_key_duration, and correctly treats explicit null as infinite.
  • Backend (key regeneration): New _validate_regenerate_key_duration_against_team helper performs the same check for the regeneration endpoint; service-account keys are correctly exempt.
  • Enterprise module: add_team_member_key_duration now only applies the team max as a default when the user did not supply a duration, instead of always overwriting.
  • prepare_key_update_data: Switches the never-expires sentinel from "-1" to None, which is a backwards-incompatible change for API callers that currently send duration: "-1" to /key/update.
  • UI: Adds a "Never Expires" checkbox to KeyLifecycleSettings, RegenerateKeyModal, and CreateKey, replacing the "-1" convention with an explicit null value and properly distinguishing "not provided" from "null".
  • Several issues identified in previous review threads remain open, including: the broad except Exception in the regeneration helper that silently skips validation on DB errors; potential ValueError from calling duration_in_seconds("-1") if a team max is stored as "-1"; and the hard HTTP 400 for existing API clients still sending duration: "-1" without a migration path.

Confidence Score: 2/5

  • Not safe to merge without addressing the backwards-incompatible "-1" sentinel removal and the broad exception swallowing in the regeneration helper.
  • The core logic is sound and well-tested (11 new unit tests), but multiple concerns from previous review threads remain unresolved: (1) the hard HTTP 400 for duration: "-1" breaks existing API integrations without a migration path, violating the project's backward-compatibility policy; (2) the broad except Exception in _validate_regenerate_key_duration_against_team silently skips the new validation on any DB error, re-opening the bypass this PR aims to close; and (3) potential unhandled ValueError from duration_in_seconds("-1") on the team-max path. Combined with the UI initialization issue for existing never-expiring keys, these warrant a lower score.
  • litellm/proxy/management_endpoints/key_management_endpoints.py requires the most attention — specifically the exception handling in _validate_regenerate_key_duration_against_team, the "-1" backward-compatibility removal in prepare_key_update_data, and the ValueError crash path if a team's max duration is stored as "-1". ui/litellm-dashboard/src/components/common_components/KeyLifecycleSettings.tsx needs the neverExpires initialization fix for the edit view.

Important Files Changed

Filename Overview
enterprise/litellm_enterprise/proxy/management_endpoints/key_management_endpoints.py Changes add_team_member_key_duration to only apply the team max as a default when the user did not explicitly supply a duration, allowing user-provided shorter durations to be respected.
litellm/proxy/management_endpoints/key_management_endpoints.py Core backend changes: adds explicit null-duration check for upperbound enforcement, replaces the "-1" never-expires sentinel with HTTP 400 (breaking change), adds a new team-max duration validation block, fixes prepare_key_update_data to check duration is None instead of "-1", and adds _validate_regenerate_key_duration_against_team helper. Several backward-compat and crash-path concerns noted in previous threads remain.
tests/test_litellm/enterprise/proxy/test_key_duration_ceiling.py New test file for enterprise ceiling behavior. Good coverage across six cases; the service-account test assertion uses the default None value, making the guard invisible if removed. Module-level pytest.mark.skipif guards against missing enterprise source.
ui/litellm-dashboard/src/components/common_components/KeyLifecycleSettings.tsx Adds a "Never Expires" checkbox with correct else-branch handling; however, the neverExpires state is always initialized to false and is never derived from the form's current duration value, so the checkbox appears unchecked when editing an existing never-expiring key.
ui/litellm-dashboard/src/components/organisms/regenerate_key_modal.tsx Adds a "Never Expires" checkbox initialized from the token's current expiry; correctly handles null vs omitted duration in the submit handler. Minor: "New expiry: Never" is shown immediately on checkbox check, before regeneration is triggered.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Key Generation / Regeneration Request] --> B{"'duration' in\nmodel_fields_set?"}
    B -- No --> C["Skip team validation\n(duration not provided)"]
    C --> D["Enterprise module:\napply team_max as default\nif not already set"]
    D --> E[Proceed]

    B -- "Yes (duration=null)" --> F{"Global upperbound\nduration set?"}
    F -- Yes --> G["HTTP 400:\nnull exceeds upperbound"]
    F -- No --> H{"team_member_key_duration\nset on team?"}
    H -- Yes --> I["HTTP 400:\nnull (∞) exceeds team max"]
    H -- No --> E

    B -- "Yes (duration=string)" --> J{"Global upperbound\nduration set?"}
    J -- Yes --> K{"duration >\nglobal upperbound?"}
    K -- Yes --> L["HTTP 400:\nexceeds global upperbound"]
    K -- No --> M{"team_member_key_duration\nset on team?"}
    J -- No --> M
    M -- Yes --> N{"duration >\nteam max?"}
    N -- Yes --> O["HTTP 400:\nexceeds team max"]
    N -- No --> E
    M -- No --> E

    style G fill:#f88,stroke:#c00
    style I fill:#f88,stroke:#c00
    style L fill:#f88,stroke:#c00
    style O fill:#f88,stroke:#c00
    style E fill:#8f8,stroke:#080
Loading

Comments Outside Diff (1)

  1. ui/litellm-dashboard/src/components/organisms/regenerate_key_modal.tsx, line 264-265 (link)

    "New expiry: Never" shown before regeneration completes

    The condition {neverExpires && <div>New expiry: Never</div>} is evaluated as soon as the checkbox is ticked, before the user clicks "Regenerate" and the API call succeeds. This means the "New expiry: Never" confirmation message appears immediately on check — potentially misleading the user into thinking the key has already been updated.

    The existing non-never-expires branch correctly gates on newExpiryTime (set only after a successful API response). Apply the same guard here:

    Alternatively, introduce a dedicated regenerationSucceeded boolean that is set to true after the API call completes and use it to gate both messages.

Last reviewed commit: 74b4ab2

Comment on lines 3395 to +3396
)
async def _validate_regenerate_key_duration_against_team(

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.

Missing blank line between functions

PEP 8 requires two blank lines between top-level function definitions. There's no blank line between the end of _insert_deprecated_key (line 3395) and the start of _validate_regenerate_key_duration_against_team (line 3396).

Suggested change
)
async def _validate_regenerate_key_duration_against_team(
)
async def _validate_regenerate_key_duration_against_team(

Comment on lines +3410 to +3419
try:
team_table = await get_team_object(
team_id=team_id,
prisma_client=prisma_client,
user_api_key_cache=user_api_key_cache,
parent_otel_span=None,
check_db_only=True,
)
except Exception:
return

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.

Bare except silently skips validation

The broad except Exception on line 3418 catches all errors from get_team_object (including DB connectivity issues, timeouts, etc.) and silently returns, skipping the duration validation entirely. Since this PR is specifically fixing a security gap where the regeneration endpoint bypassed the team's max duration, silently skipping validation on any error re-introduces the same bypass.

Consider narrowing the exception to only catch "team not found" (HTTP 404) or failing closed (rejecting the request when the team can't be looked up). For example, catch HTTPException specifically and only return on status_code == 404, re-raising otherwise.

…heck

The broad except was silently swallowing errors, which could allow the
team max duration to be bypassed if get_team_object raised. Add a
warning log so bypasses are visible in audit trails while preserving
fail-open behavior consistent with the same team lookup pattern in
generate_key_fn.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines +557 to +558
team_max_duration = team_table.metadata["team_member_key_duration"]
team_max_seconds = duration_in_seconds(duration=team_max_duration)

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.

Unhandled ValueError if team max duration is "-1"

If an admin sets team_member_key_duration to "-1" in the team metadata, duration_in_seconds("-1") will raise a ValueError because the parser regex ((\d+)(mo|[smhdw]?)) doesn't match negative numbers. The user-provided duration correctly handles "-1" with the special float("inf") branch, but the team max side does not.

This applies to both this block (line 558) and the regeneration helper (line 3434). Consider adding a symmetric check:

Suggested change
team_max_duration = team_table.metadata["team_member_key_duration"]
team_max_seconds = duration_in_seconds(duration=team_max_duration)
team_max_duration = team_table.metadata["team_member_key_duration"]
if team_max_duration == "-1":
team_max_seconds: float = float("inf")
else:
team_max_seconds = duration_in_seconds(duration=team_max_duration)

Comment on lines +3433 to +3434
team_max_duration = team_table.metadata["team_member_key_duration"]
team_max_seconds = duration_in_seconds(duration=team_max_duration)

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.

Same "-1" crash risk for team max duration

Same issue as the key creation path — if team_member_key_duration is "-1", duration_in_seconds("-1") will raise ValueError. Apply the same fix here:

Suggested change
team_max_duration = team_table.metadata["team_member_key_duration"]
team_max_seconds = duration_in_seconds(duration=team_max_duration)
team_max_duration = team_table.metadata["team_member_key_duration"]
if team_max_duration == "-1":
team_max_seconds: float = float("inf")
else:
team_max_seconds = duration_in_seconds(duration=team_max_duration)

… Never Expires checkbox

- In `_validate_regenerate_key_duration_against_team`: use `model_fields_set` to
  distinguish "duration not sent" (leave unchanged) from "duration: null" (never
  expires / infinite). Guard against team max being "-1" (no limit) before calling
  `duration_in_seconds` which would crash on that sentinel.
- In `_common_key_generation_helper`: same "-1" guard for team max duration so the
  creation path no longer crashes when the team has no configured limit.
- In `prepare_key_update_data`: treat `duration is None` the same as `"-1"` — both
  set `expires = None` (never expires).
- UI: replace the "Enter -1 for unlimited expiry" pattern in `KeyLifecycleSettings.tsx`
  and `regenerate_key_modal.tsx` with a "Never Expires" checkbox mirroring the budget
  unlimited approach.
- Tests: add/fix tests for all new cases (null-as-infinite, team max "-1" skips,
  not-sent skips via model_fields_set).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines +550 to +556
# Validate key duration against the team's max key duration
if (
team_table is not None
and team_table.metadata is not None
and team_table.metadata.get("team_member_key_duration")
and data.duration is not None
):

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.

Key creation validation bypassed when duration is null

The condition data.duration is not None on line 555 causes the entire team-max-duration check to be skipped when the UI sends duration: null (i.e., when the user checks "Never Expires"). This means a user can create a never-expiring key even when the team has a finite team_member_key_duration limit — the exact bypass this PR aims to fix.

The regeneration helper (_validate_regenerate_key_duration_against_team) correctly handles this case by distinguishing "duration not sent" (via model_fields_set) from "duration explicitly null" (never-expires). The creation path needs the same treatment.

Suggested change
# Validate key duration against the team's max key duration
if (
team_table is not None
and team_table.metadata is not None
and team_table.metadata.get("team_member_key_duration")
and data.duration is not None
):
# Validate key duration against the team's max key duration
if (
team_table is not None
and team_table.metadata is not None
and team_table.metadata.get("team_member_key_duration")
):

With this change, also add handling for data.duration is None inside the block (treating it as float("inf"), similar to the regeneration path).

Replace the legacy "-1" sentinel string with proper null semantics:
- User duration: null means never-expires (no more "-1" accepted)
- Team max: absence of the field means no limit (no more "-1" stored)
- Creation path team validation now uses model_fields_set to distinguish
  "duration not sent" (skip) from "duration: null" (never-expires, validate)
- Tests updated: -1 sentinels replaced with None, -1 team max tests replaced
  with "field not set" equivalent

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
user_duration = float("inf") # Infinite duration
else:
user_duration = duration_in_seconds(duration=value)
user_duration = duration_in_seconds(duration=value)

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.

Regression: removed "-1" guard causes ValueError

This line previously had special handling for value == "-1" (mapping it to float("inf")). That branch was removed in this PR, so now duration_in_seconds("-1") is called directly. The regex in _extract_from_regex (r"(\d+)(mo|[smhdw]?)") doesn't match "-1" (negative sign is not a digit), causing an unhandled ValueError.

While the UI now sends null instead of "-1", API users calling /key/generate directly with duration: "-1" will hit this crash. Previously they got a clean 400 response; now they get an unhandled 500.

Consider preserving the guard here:

Suggested change
user_duration = duration_in_seconds(duration=value)
if value is None:
user_duration = float("inf")
else:
user_duration = duration_in_seconds(duration=value)

This mirrors the null-means-infinite convention used in the new team validation block below.

Comment on lines +62 to +73
const handleNeverExpiresChange = (e: any) => {
const checked = e.target.checked;
setNeverExpires(checked);
if (checked) {
setDurationValue("");
if (form && typeof form.setFieldValue === "function") {
form.setFieldValue("duration", null);
} else if (form && typeof form.setFieldsValue === "function") {
form.setFieldsValue({ duration: null });
}
}
};

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.

Unchecking "Never Expires" leaves form duration as null

When checked is true, the handler sets form.duration = null. But when checked becomes false (user unchecks the box), there is no corresponding reset of the form value back from null to "". The TextInput shows empty text (via local durationValue state), but the underlying form value remains null.

If the user checks "Never Expires", then unchecks it and submits without typing a new duration, the backend receives duration: null — which it interprets as "never expires" — contradicting the user's intent.

Add an else branch to reset the form value:

Suggested change
const handleNeverExpiresChange = (e: any) => {
const checked = e.target.checked;
setNeverExpires(checked);
if (checked) {
setDurationValue("");
if (form && typeof form.setFieldValue === "function") {
form.setFieldValue("duration", null);
} else if (form && typeof form.setFieldsValue === "function") {
form.setFieldsValue({ duration: null });
}
}
};
const handleNeverExpiresChange = (e: any) => {
const checked = e.target.checked;
setNeverExpires(checked);
if (checked) {
setDurationValue("");
if (form && typeof form.setFieldValue === "function") {
form.setFieldValue("duration", null);
} else if (form && typeof form.setFieldsValue === "function") {
form.setFieldsValues({ duration: null });
}
} else {
if (form && typeof form.setFieldValue === "function") {
form.setFieldValue("duration", "");
} else if (form && typeof form.setFieldsValues === "function") {
form.setFieldsValues({ duration: "" });
}
}
};

…s uncheck

- Return HTTP 400 with a clear migration message when duration="-1" is passed
  to the upperbound validation path, instead of crashing with an unhandled
  ValueError (500).
- Fix KeyLifecycleSettings: add else branch to handleNeverExpiresChange so
  unchecking "Never Expires" resets the form's duration field to undefined,
  preventing it from being submitted as null (never-expires).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- create_key_button: remove initialValue={null} on hidden duration field;
  change submit handler to delete duration when empty/undefined rather than
  converting to null, so untouched expiry settings don't accidentally trigger
  never-expires behavior on the backend.
- key_edit_view: remove initialValue="" on hidden duration field; add same
  duration cleanup to handleSubmit so an untouched field sends no duration
  key instead of an empty string.
- In both cases duration: null is preserved (Never Expires checkbox).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines +1491 to 1494
if duration is None:
# null (never-expires checkbox) means no expiry
non_default_values["expires"] = None
elif duration and (isinstance(duration, str)) and len(duration) > 0:

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.

duration: "-1" crashes /key/update with unhandled ValueError

The change from duration == "-1" to duration is None removes the guard that previously mapped "-1" to expires = None. Now if an API caller sends duration: "-1" to /key/update (which was the documented "never expires" sentinel for this endpoint until this PR), it falls into the elif branch and calls duration_in_seconds("-1"). The regex in _extract_from_regex only matches digits and unit suffixes, so "-1" raises an unhandled ValueError, resulting in an HTTP 500.

Previously: duration == "-1"expires = None (never expires).
Now: duration == "-1"duration_in_seconds("-1")ValueError / 500 crash.

To handle both gracefully — preserving backward compat for existing callers while also accepting the new null convention — consider rejecting "-1" with a clean 400 similar to the approach in _common_key_generation_helper:

        if duration is None:
            # null (never-expires checkbox) means no expiry
            non_default_values["expires"] = None
        elif duration == "-1":
            raise HTTPException(
                status_code=400,
                detail={
                    "error": "'-1' is no longer a valid duration value. Pass duration: null to set a key to never expire."
                },
            )
        elif duration and (isinstance(duration, str)) and len(duration) > 0:
            duration_s = duration_in_seconds(duration=duration)
            expires = datetime.now(timezone.utc) + timedelta(seconds=duration_s)
            non_default_values["expires"] = expires

Context Used: Rule from dashboard - What: avoid backwards-incompatible changes without user-controlled flags

Why: This breaks current ... (source)

Comment on lines 537 to +543
if value == "-1":
user_duration = float("inf") # Infinite duration
else:
user_duration = duration_in_seconds(duration=value)
raise HTTPException(
status_code=400,
detail={
"error": "'-1' is no longer a valid duration value. Pass duration: null to set a key to never expire."
},
)

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.

Breaking change for existing API clients using duration: "-1"

The enterprise params validation block previously mapped value == "-1" to float("inf") (infinite duration). This PR changes it to raise HTTP 400 instead.

While the UI is updated to use null instead of "-1", API clients that directly call /key/generate with duration: "-1" — which was previously documented and silently accepted — will now receive an unexpected 400 error. This is a backwards-incompatible change with no migration path or feature flag, meaning existing integrations break silently on upgrade.

Per the project's compatibility policy, breaking changes to existing behavior should be guarded by a user-controlled flag rather than changed unconditionally. Consider either:

  • Keeping the old "-1" → infinite behaviour (and separately restricting it if a team max applies), or
  • Returning a clear deprecation warning (via response headers or a non-error field in the response) rather than a hard 400, with the hard rejection gated behind a config flag.

Context Used: Rule from dashboard - What: avoid backwards-incompatible changes without user-controlled flags

Why: This breaks current ... (source)

Comment on lines +3424 to +3431
try:
team_table = await get_team_object(
team_id=team_id,
prisma_client=prisma_client,
user_api_key_cache=user_api_key_cache,
parent_otel_span=None,
check_db_only=True,
)

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.

check_db_only=True bypasses cache on every regeneration

get_team_object is called with check_db_only=True, which forces a direct database query and skips the in-memory/dual cache. This means every key-regeneration request incurs a synchronous DB round-trip purely for duration validation, even if the team object is already cached.

Consider omitting check_db_only=True so the helper uses the existing cache layer and falls back to the DB only on a cache miss. This aligns with the project guideline of avoiding unnecessary direct DB queries.

…d value

Previously, add_team_member_key_duration unconditionally overwrote the user's
requested duration with team_member_key_duration, even when the user explicitly
set a shorter duration. This made the team setting act as a fixed value rather
than a maximum.

Change: only apply team_member_key_duration when the user did not provide a
duration (not in model_fields_set). If the user supplied an explicit duration
(already validated against team max upstream), their value is respected.

Adds 7 unit tests covering: no duration → team max applied; shorter duration
respected; equal duration respected; no team, no metadata, missing field, and
service account all leave data unchanged.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines +237 to +244
onChange={(e) => {
const checked = e.target.checked;
setNeverExpires(checked);
if (checked) {
form.setFieldValue("duration", null);
setRegenerateFormData((prev: any) => ({ ...prev, duration: null }));
}
}}

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.

The onChange handler sets form.setFieldValue("duration", null) when the checkbox is checked, but does nothing when unchecked. Because the form value remains null, submitting without entering a new duration after unchecking will leave duration: null in the request, which the backend interprets as "never expires" — contradicting the user's intent.

KeyLifecycleSettings.tsx has the correct pattern with an else branch that resets the form value to undefined. Apply the same fix here:

Suggested change
onChange={(e) => {
const checked = e.target.checked;
setNeverExpires(checked);
if (checked) {
form.setFieldValue("duration", null);
setRegenerateFormData((prev: any) => ({ ...prev, duration: null }));
}
}}
onChange={(e) => {
const checked = e.target.checked;
setNeverExpires(checked);
if (checked) {
form.setFieldValue("duration", null);
setRegenerateFormData((prev: any) => ({ ...prev, duration: null }));
} else {
form.setFieldValue("duration", undefined);
}
}}

Comment on lines +3420 to +3422
team_id = getattr(key_in_db, "team_id", None)
if not team_id:
return

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.

The enterprise add_team_member_key_duration function explicitly skips service accounts with if data.user_id is None: return data. The regeneration helper has no equivalent guard, so service account keys will be subject to team member duration validation during regeneration even though the creation-time check does not apply to them.

Add a consistent check after retrieving the key from the database:

Suggested change
team_id = getattr(key_in_db, "team_id", None)
if not team_id:
return
team_id = getattr(key_in_db, "team_id", None)
if not team_id:
return
if getattr(key_in_db, "user_id", None) is None:
return # service account keys are exempt from team member duration limits

…x uncheck UI bug

- _validate_regenerate_key_duration_against_team: remove check_db_only=True from
  get_team_object call so cache is used on hits and DB only on misses, consistent
  with the guideline of avoiding unnecessary direct DB queries.
- Add service account guard (user_id is None → skip validation), mirroring the
  enterprise add_team_member_key_duration guard so service account keys are not
  subject to team member duration limits during regeneration.
- regenerate_key_modal.tsx: add else branch to the Never Expires onChange handler
  so unchecking resets the form field to undefined instead of leaving it as null
  (which the backend would interpret as never-expires).
- Tests: set mock_key.user_id = "user-123" explicitly in all regeneration tests
  where validation should fire; add test_service_account_key_skips_validation.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines +21 to +37
def _load_local_add_team_member_key_duration():
"""Load add_team_member_key_duration from the local enterprise source tree."""
local_path = os.path.normpath(
os.path.join(
os.path.dirname(__file__),
"..", "..", "..", "..",
"enterprise", "litellm_enterprise", "proxy",
"management_endpoints", "key_management_endpoints.py",
)
)
module_name = "_local_enterprise_key_management_endpoints"
# Remove cached version so we always reload from the local file
sys.modules.pop(module_name, None)
spec = importlib.util.spec_from_file_location(module_name, local_path)
module = importlib.util.module_from_spec(spec) # type: ignore[arg-type]
spec.loader.exec_module(module) # type: ignore[union-attr]
return module.add_team_member_key_duration

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.

Fragile dynamic import may fail silently if enterprise module is absent

_load_local_add_team_member_key_duration() uses importlib.util.spec_from_file_location to load the enterprise module via a hard-coded path. If the enterprise directory is missing (open-source install) or the layout changes, spec.loader.exec_module(module) will raise AttributeError at module import time, causing the entire test suite to fail with a confusing error instead of a clear skip message.

Add an explicit guard with pytest.skip:

def _load_local_add_team_member_key_duration():
    """Load add_team_member_key_duration from the local enterprise source tree."""
    local_path = os.path.normpath(
        os.path.join(
            os.path.dirname(__file__),
            "..", "..", "..", "..",
            "enterprise", "litellm_enterprise", "proxy",
            "management_endpoints", "key_management_endpoints.py",
        )
    )
    if not os.path.exists(local_path):
        pytest.skip("enterprise module not found")
    
    module_name = "_local_enterprise_key_management_endpoints"
    sys.modules.pop(module_name, None)
    spec = importlib.util.spec_from_file_location(module_name, local_path)
    module = importlib.util.module_from_spec(spec)
    spec.loader.exec_module(module)
    return module.add_team_member_key_duration

Comment on lines +111 to +118
def test_service_account_returns_unchanged(self):
"""user_id=None (service account) → data is returned unchanged."""
fn = _load_local_add_team_member_key_duration()

data = GenerateKeyRequest(user_id=None)
result = fn(_make_team("30d"), data)

assert result.duration is None

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.

test_service_account_returns_unchanged has weak assertion that doesn't exercise the guard

The test constructs GenerateKeyRequest(user_id=None) and asserts result.duration is None. However, GenerateKeyRequest defaults duration to None, so this assertion would pass even if the service-account guard (the if data.user_id is None: return data branch on line 13 of the enterprise module) were accidentally removed.

A stronger test would explicitly set a non-default duration to verify the guard actually prevented modification:

def test_service_account_returns_unchanged(self):
    """user_id=None (service account) → data is returned unchanged."""
    fn = _load_local_add_team_member_key_duration()

    data = GenerateKeyRequest(user_id=None)
    data.duration = "99d"          # service account: set a non-default to make the guard observable
    result = fn(_make_team("30d"), data)

    assert result.duration == "99d"  # guard fired; team max was NOT applied

…arden enterprise tests

- Upperbound loop now raises 400 when duration=null (never expires) conflicts
  with a finite upperbound, instead of silently replacing it with the upperbound
- Enterprise test file uses pytest.mark.skipif for graceful skip when enterprise
  source is unavailable
- Strengthened service account test assertion to explicitly verify team duration
  was not applied
- Added TestUpperboundNullDurationValidation with two tests

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@@ -31,6 +31,7 @@ const KeyLifecycleSettings: React.FC<KeyLifecycleSettingsProps> = ({
const [showCustomInput, setShowCustomInput] = useState(isCustomInterval);
const [customInterval, setCustomInterval] = useState(isCustomInterval ? rotationInterval : "");

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.

neverExpires not initialized from existing form data

neverExpires is always initialized to false, so when KeyLifecycleSettings is rendered inside the key-edit view for a key that currently has duration: null (never expires), the "Never Expires" checkbox will appear unchecked — giving the user a false impression that the key has an expiry.

The fix is to derive the initial value from the form at mount time, mirroring how durationValue is initialized:

Suggested change
const [customInterval, setCustomInterval] = useState(isCustomInterval ? rotationInterval : "");
const [neverExpires, setNeverExpires] = useState<boolean>(
form?.getFieldValue?.("duration") === null
);

This ensures the checkbox is pre-checked when editing a key that already never expires.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant