Skip to content

[Fix] /key/list user_id Empty String Edge Case#20623

Merged
yuneng-jiang merged 1 commit intomainfrom
litellm_user_id_fix
Feb 7, 2026
Merged

[Fix] /key/list user_id Empty String Edge Case#20623
yuneng-jiang merged 1 commit intomainfrom
litellm_user_id_fix

Conversation

@yuneng-jiang
Copy link
Collaborator

Relevant issues

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have Added testing in the tests/litellm/ directory, Adding at least 1 test is a hard requirement - see details
  • My PR passes all unit tests on make test-unit
  • My PR's scope is as isolated as possible, it only solves 1 specific problem

CI (LiteLLM team)

CI status guideline:

  • 50-55 passing tests: main is stable with minor issues.
  • 45-49 passing tests: acceptable but needs attention
  • <= 40 passing tests: unstable; be careful with your merges and assess the risk.
  • Branch creation CI run
    Link:

  • CI run for the last commit
    Link:

  • Merge / cherry-pick CI run
    Links:

Type

🐛 Bug Fix
✅ Test

Changes

Fixes an issue where an empty user_id will return the entire list of keys

Screenshot

image

@vercel
Copy link

vercel bot commented Feb 7, 2026

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

Project Deployment Actions Updated (UTC)
litellm Ready Ready Preview, Comment Feb 7, 2026 2:03am

Request Review

@yuneng-jiang yuneng-jiang changed the title fixing user_id [Fix] /key/list user_id Empty String Edge Case Feb 7, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 7, 2026

Greptile Overview

Greptile Summary

Fixed a security vulnerability where non-admin users could list all keys by passing an empty user_id query parameter (e.g., ?user_id=). Changed the check from if user_id is None to if not user_id on line 3752 to properly handle both None and empty string values.

Key Changes

  • Security Fix: Empty user_id strings now trigger auto-assignment to the authenticated user's ID for non-admin users, preventing unauthorized access to all keys
  • Test Added: New test validates user_id=None behavior, though it doesn't cover the empty string case which is the actual vulnerability vector

Issues Found

  • Test coverage gap: The test only validates user_id=None but doesn't test user_id="" (empty string), which is how the vulnerability would manifest in practice when users pass ?user_id= in the query string

Confidence Score: 4/5

  • This PR fixes a critical security vulnerability and is safe to merge with minor test coverage improvements recommended
  • The security fix is correct and addresses a real vulnerability where users could access all keys. However, the test doesn't fully validate the actual attack vector (empty string vs None), which slightly reduces confidence that the fix is fully tested
  • The test file would benefit from adding an empty string test case to fully validate the security fix

Important Files Changed

Filename Overview
litellm/proxy/management_endpoints/key_management_endpoints.py Fixed security issue where empty user_id query parameter could bypass authorization checks and return all keys instead of user-specific keys
tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py Added test for auto-setting user_id when None, but doesn't test the empty string case which was the actual security vulnerability

Sequence Diagram

sequenceDiagram
    participant Client as Non-Admin User
    participant API as list_keys Endpoint
    participant Auth as Authorization Check
    participant Helper as _list_key_helper
    participant DB as Database
    
    Note over Client,DB: Before Fix (Vulnerability)
    Client->>API: GET /key/list?user_id=
    API->>Auth: Check if user_id is None
    Auth-->>API: False (empty string != None)
    API->>Helper: _list_key_helper(user_id="")
    Helper->>DB: Query with empty user_id filter
    DB-->>Helper: Returns ALL keys
    Helper-->>Client: Unauthorized access to all keys
    
    Note over Client,DB: After Fix (Secure)
    Client->>API: GET /key/list?user_id=
    API->>Auth: Check if not user_id
    Auth-->>API: True (empty string is falsy)
    API->>API: Set user_id = authenticated_user_id
    API->>Helper: _list_key_helper(user_id="test-user-123")
    Helper->>DB: Query filtered by user_id
    DB-->>Helper: Returns only user's keys
    Helper-->>Client: Authorized access to own keys only
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

await list_keys(
request=mock_request,
user_api_key_dict=mock_user_api_key_dict,
user_id=None, # This should be auto-set to test_user_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Test should also include user_id="" (empty string) case, which is the actual security vulnerability being fixed. Query parameters like ?user_id= result in empty strings, not None.

@yuneng-jiang yuneng-jiang merged commit 271877f into main Feb 7, 2026
10 of 12 checks passed
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