mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-26 01:12:21 +00:00
test(proxy_behavior): address Greptile review — env force, pagination, dedup
- conftest: force LITELLM_MASTER_KEY / CONFIG_FILE_PATH unconditionally
instead of setdefault. An ambient LITELLM_MASTER_KEY with a different
value would make the proxy authenticate on that key while the tests
still send MASTER_KEY → silent 401s.
- test_key_list: paginate /key/list instead of a single size=100 request.
size is capped at 100 by the endpoint, so on a non-fresh DB a single
page could truncate PROXY_ADMIN's view and a seeded key could fall off
the page. Walk total_pages.
- conftest: hoist the duplicated _create_scratch_key helper (copy-pasted
and already diverged across test_key_{update,regenerate,delete}.py)
into a single shared create_scratch_key.
- Delete regression_replay/README.md — G4 regression-replay evidence
belongs in the PR description, not a committed doc file (repo docs
policy + the effort's own plan both say so). Content moved to the PR.
This commit is contained in:
Yuneng Jiang
parent
cc00ad9af1
commit
016b7c6e09
6 changed files with 68 additions and 198 deletions
|
|
@ -4,7 +4,7 @@ import os
|
|||
import tempfile
|
||||
import uuid
|
||||
from dataclasses import dataclass
|
||||
from typing import AsyncIterator
|
||||
from typing import Any, AsyncIterator, Dict, Optional
|
||||
|
||||
import httpx
|
||||
import pytest_asyncio
|
||||
|
|
@ -44,10 +44,11 @@ async def proxy_app():
|
|||
|
||||
# proxy_startup_event re-reads master_key from LITELLM_MASTER_KEY and
|
||||
# unconditionally overwrites the global, even when initialize() already
|
||||
# set it from the config YAML. Without these env vars, the entire auth
|
||||
# stack degrades to user_id=None / non-PROXY_ADMIN for every token.
|
||||
os.environ.setdefault("LITELLM_MASTER_KEY", MASTER_KEY)
|
||||
os.environ.setdefault("CONFIG_FILE_PATH", config_path)
|
||||
# set it from the config YAML. Force (not setdefault) both vars: an
|
||||
# ambient LITELLM_MASTER_KEY with a different value would make the proxy
|
||||
# authenticate on that key while the tests still send MASTER_KEY.
|
||||
os.environ["LITELLM_MASTER_KEY"] = MASTER_KEY
|
||||
os.environ["CONFIG_FILE_PATH"] = config_path
|
||||
|
||||
await initialize(config=config_path)
|
||||
|
||||
|
|
@ -96,6 +97,33 @@ class Scratch:
|
|||
return f"{self.prefix}-{suffix}" if suffix else self.prefix
|
||||
|
||||
|
||||
async def create_scratch_key(
|
||||
proxy_client,
|
||||
seeder_cleartext: str,
|
||||
scratch_prefix: str,
|
||||
*,
|
||||
user_id: str,
|
||||
team_id: Optional[str] = None,
|
||||
organization_id: Optional[str] = None,
|
||||
) -> str:
|
||||
"""Seed a scratch-tagged key via /key/generate; returns its cleartext.
|
||||
|
||||
Shared by the write-scenario matrices (key update/regenerate/delete).
|
||||
"""
|
||||
body: Dict[str, Any] = {"key_alias": scratch_prefix, "user_id": user_id}
|
||||
if team_id is not None:
|
||||
body["team_id"] = team_id
|
||||
if organization_id is not None:
|
||||
body["organization_id"] = organization_id
|
||||
resp = await proxy_client.post(
|
||||
"/key/generate",
|
||||
headers={"Authorization": f"Bearer {seeder_cleartext}"},
|
||||
json=body,
|
||||
)
|
||||
assert resp.status_code == 200, f"setup failed: {resp.text}"
|
||||
return resp.json()["key"]
|
||||
|
||||
|
||||
@pytest_asyncio.fixture
|
||||
async def scratch(prisma):
|
||||
handle = Scratch(prefix=f"{SCRATCH_PREFIX}{uuid.uuid4().hex[:12]}")
|
||||
|
|
|
|||
|
|
@ -1,104 +0,0 @@
|
|||
# G4 — Regression-replay set for PR1 (Key Tier-1)
|
||||
|
||||
This directory documents the regression-replay verification for the
|
||||
behavior-pinning suite. For each in-scope recent fix-PR touching
|
||||
`litellm/proxy/management_endpoints/key_management_endpoints.py`, we record:
|
||||
|
||||
- which scenario(s) in the matrix catch the behavior the fix introduced,
|
||||
- a RED-at-parent / GREEN-at-tip transcript for at least one canonical PR.
|
||||
|
||||
## Methodology
|
||||
|
||||
For a given fix-PR `<sha>` with parent `<parent>`:
|
||||
|
||||
```bash
|
||||
# 1. Save current handler
|
||||
cp litellm/proxy/management_endpoints/key_management_endpoints.py /tmp/key_mgmt.HEAD.py
|
||||
|
||||
# 2. Replace with the pre-fix version
|
||||
git show <parent>:litellm/proxy/management_endpoints/key_management_endpoints.py \
|
||||
> litellm/proxy/management_endpoints/key_management_endpoints.py
|
||||
|
||||
# 3. Run the catching slice — expect RED on the scenarios that flip
|
||||
DATABASE_URL=postgresql://litellm:litellm@localhost:5432/litellm_test \
|
||||
uv run --no-sync pytest tests/proxy_behavior/management/<test_file>.py -v
|
||||
|
||||
# 4. Restore + confirm GREEN
|
||||
cp /tmp/key_mgmt.HEAD.py litellm/proxy/management_endpoints/key_management_endpoints.py
|
||||
DATABASE_URL=postgresql://litellm:litellm@localhost:5432/litellm_test \
|
||||
uv run --no-sync pytest tests/proxy_behavior/management/<test_file>.py -v
|
||||
```
|
||||
|
||||
When the fix changed multiple files, the in-place swap is constrained to the
|
||||
handler module — if a referenced helper moved between modules the swap may
|
||||
not run; in that case use `git worktree add` for a clean replay.
|
||||
|
||||
## Replay table
|
||||
|
||||
| # | Fix SHA | Subject (truncated) | Endpoint | Catching scenarios | Verified |
|
||||
|---|---------|---------------------|----------|--------------------|----------|
|
||||
| 1 | `c7c3df2b02` | extend /key/update admin check to non-budget fields | `/key/update` | `test_key_update.py::self/{team_admin,internal_user,owner,unrelated_same_org,cross_org_user,service_account}` — all 6 flip 200→403 between parent and HEAD | ✅ RED→GREEN below |
|
||||
| 2 | `8bbc61e03c` | harden /key/update authorization checks | `/key/update` | `test_key_update.py::owner_target/*` — non-admins blocked from updating peers' keys | by-inspection (overlapping coverage with #1) |
|
||||
| 3 | `1b2756811e` | close project hijacking and key org IDOR | `/key/update` (org_id field) | `test_key_update.py` matrix asserts no row mutation on denied responses (`row.models != [MARKER]`) | by-inspection |
|
||||
| 4 | `c7c3df2b02` siblings: `f6cd0a827a` | /key/update returns 404 (not 401) for nonexistent body key | `/key/update` | NOT covered by current matrix — the matrix only exercises existing target keys. Future: add 404-on-missing scenarios. | gap (filed below) |
|
||||
| 5 | `133471f882` | double-counting bug in org/team key limit checks on update | `/key/update` (counting) | NOT directly covered; matrix asserts status only, not counts. Future: budget/limit assertions. | gap (filed below) |
|
||||
| 6 | `574633fcf1` | exclude budget_limits from deleted verification token | `/key/delete` | `test_key_delete.py` matrix verifies post-delete authentication fails — would catch a shape-of-delete change but not a budget_limits-specific bug | by-inspection (partial) |
|
||||
| 7 | `db8ef44323` | enforce upperbound_key_generate_params on /key/regenerate | `/key/regenerate` | `test_key_regenerate.py` matrix asserts status; a regen that exceeds upperbound limits would surface IF the test bodies passed disallowed params. Currently they don't. | gap (filed below) |
|
||||
| 8 | `2220f3076a` | tighten caller-permission checks on key route fields | multiple | spans /key/generate + /key/update; partial overlap with our `team_id`/`user_id` boundary scenarios | by-inspection |
|
||||
| 9 | `5190bd07eb` | extend caller-permission to service-account | /key/generate, /key/service-account/generate | service_account actor IS in our matrix; some sub-scenarios overlap | by-inspection |
|
||||
| 10 | `12005c4a02` | /key/aliases auth | `/key/aliases` | not Tier-1 (out of PR1 scope; PR3 territory) | out of scope |
|
||||
| 11 | `818c097ca9` | Self-exclusion hash mismatch | `/key/update` | partial overlap | by-inspection |
|
||||
| 12 | `daf7c0c3a8` | virtual keys team filter | `/key/list` | `test_key_list.py` filter param scenarios deferred; default-visibility scenarios already in matrix | gap (deferred) |
|
||||
|
||||
## Verified replay: `c7c3df2b02`
|
||||
|
||||
**Fix subject**: `fix(proxy): extend /key/update admin check to non-budget fields`
|
||||
**Parent SHA**: `662d05531d`
|
||||
**Catching slice**: `tests/proxy_behavior/management/test_key_update.py`
|
||||
|
||||
### Parent (pre-fix) — RED
|
||||
|
||||
```
|
||||
FAILED tests/proxy_behavior/management/test_key_update.py::test_key_update_authz_matrix[self/team_admin]
|
||||
FAILED tests/proxy_behavior/management/test_key_update.py::test_key_update_authz_matrix[self/internal_user]
|
||||
FAILED tests/proxy_behavior/management/test_key_update.py::test_key_update_authz_matrix[self/owner]
|
||||
FAILED tests/proxy_behavior/management/test_key_update.py::test_key_update_authz_matrix[self/unrelated_same_org]
|
||||
FAILED tests/proxy_behavior/management/test_key_update.py::test_key_update_authz_matrix[self/cross_org_user]
|
||||
FAILED tests/proxy_behavior/management/test_key_update.py::test_key_update_authz_matrix[self/service_account]
|
||||
6 failed, 15 passed
|
||||
```
|
||||
|
||||
Each of the 6 self-target scenarios returned `200` under the parent code (the
|
||||
admin check only gated budget/spend changes, so a non-admin could rewrite
|
||||
`models`) but is pinned at `403` post-fix.
|
||||
|
||||
### HEAD (post-fix) — GREEN
|
||||
|
||||
```
|
||||
21 passed
|
||||
```
|
||||
|
||||
The matrix correctly flipped from 6/21 RED to 21/21 GREEN solely on the
|
||||
handler swap — confirming the suite catches the c7c3df2b02 behavior change.
|
||||
|
||||
## Identified coverage gaps (deferred to PR2/PR3)
|
||||
|
||||
These cells in the replay table call out genuine matrix gaps — the current
|
||||
PR1 surface does not pin the specific behavior the fix introduced. Each is
|
||||
worth a follow-up scenario:
|
||||
|
||||
- **404-on-missing-key** (rows 4, paralleling `f6cd0a827a` and
|
||||
`19efe556cb`) — add an explicit "actor calls /key/update with a body
|
||||
`key` that does not exist" scenario per endpoint, asserting 404.
|
||||
- **Budget/limit counting bugs** (row 5, `133471f882`) — add scenarios that
|
||||
read the team/org spend rows after a denied /key/update and assert no
|
||||
counter movement.
|
||||
- **Upperbound enforcement on /key/regenerate** (row 7, `db8ef44323`) —
|
||||
extend the regenerate matrix with at least one scenario that requests
|
||||
params exceeding the team's `upperbound_key_generate_params`.
|
||||
- **`/key/list` filter-param view** (row 12, `daf7c0c3a8`) — PR1 only
|
||||
pins default-visibility. Filter combinations (`team_id=`,
|
||||
`include_team_keys=true`, etc.) belong in a follow-up.
|
||||
|
||||
Filed as TODOs rather than blocking PR1: the matrix shape is correct,
|
||||
these are scope extensions.
|
||||
|
|
@ -1,10 +1,9 @@
|
|||
from typing import Any, Dict, Optional
|
||||
|
||||
import pytest
|
||||
|
||||
from litellm.proxy.utils import hash_token
|
||||
|
||||
from .actors import TEAM_ALPHA, TEAM_BETA, Actor
|
||||
from .conftest import create_scratch_key
|
||||
|
||||
pytestmark = pytest.mark.asyncio(loop_scope="session")
|
||||
|
||||
|
|
@ -36,26 +35,6 @@ _SCENARIOS = [
|
|||
]
|
||||
|
||||
|
||||
async def _create_scratch_key(
|
||||
proxy_client,
|
||||
seeder_cleartext: str,
|
||||
scratch_prefix: str,
|
||||
*,
|
||||
user_id: str,
|
||||
team_id: Optional[str] = None,
|
||||
) -> str:
|
||||
body: Dict[str, Any] = {"key_alias": scratch_prefix, "user_id": user_id}
|
||||
if team_id is not None:
|
||||
body["team_id"] = team_id
|
||||
resp = await proxy_client.post(
|
||||
"/key/generate",
|
||||
headers={"Authorization": f"Bearer {seeder_cleartext}"},
|
||||
json=body,
|
||||
)
|
||||
assert resp.status_code == 200, f"setup failed: {resp.text}"
|
||||
return resp.json()["key"]
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"actor,target_shape,expected_status",
|
||||
[(a, t, s) for (_id, a, t, s) in _SCENARIOS],
|
||||
|
|
@ -74,11 +53,11 @@ async def test_key_delete_authz_matrix(
|
|||
seeder = world.keys[Actor.PROXY_ADMIN].cleartext
|
||||
|
||||
if target_shape == "self":
|
||||
target_cleartext = await _create_scratch_key(
|
||||
target_cleartext = await create_scratch_key(
|
||||
proxy_client, seeder, scratch.prefix, user_id=caller.user_id
|
||||
)
|
||||
elif target_shape == "owner":
|
||||
target_cleartext = await _create_scratch_key(
|
||||
target_cleartext = await create_scratch_key(
|
||||
proxy_client,
|
||||
seeder,
|
||||
scratch.prefix,
|
||||
|
|
@ -86,7 +65,7 @@ async def test_key_delete_authz_matrix(
|
|||
team_id=TEAM_ALPHA,
|
||||
)
|
||||
elif target_shape == "cross_org":
|
||||
target_cleartext = await _create_scratch_key(
|
||||
target_cleartext = await create_scratch_key(
|
||||
proxy_client,
|
||||
seeder,
|
||||
scratch.prefix,
|
||||
|
|
|
|||
|
|
@ -21,6 +21,27 @@ _VISIBILITY = {
|
|||
}
|
||||
|
||||
|
||||
async def _all_visible_hashes(proxy_client, caller_cleartext) -> set:
|
||||
"""Walk every /key/list page — size is capped at 100 by the endpoint, so a
|
||||
single request can truncate PROXY_ADMIN's view on a non-fresh DB."""
|
||||
hashes: set = set()
|
||||
page = 1
|
||||
while True:
|
||||
resp = await proxy_client.get(
|
||||
f"/key/list?page={page}&size=100",
|
||||
headers={"Authorization": f"Bearer {caller_cleartext}"},
|
||||
)
|
||||
assert resp.status_code == 200, resp.text
|
||||
body = resp.json()
|
||||
for entry in body.get("keys", []):
|
||||
tok = entry.get("token") if isinstance(entry, dict) else entry
|
||||
if tok:
|
||||
hashes.add(tok)
|
||||
if page >= (body.get("total_pages") or 1):
|
||||
return hashes
|
||||
page += 1
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"actor,expected_visible",
|
||||
list(_VISIBILITY.items()),
|
||||
|
|
@ -32,16 +53,7 @@ async def test_key_list_visibility(
|
|||
caller = world.keys[actor]
|
||||
hashed_to_actor = {world.keys[a].hashed: a for a in Actor}
|
||||
|
||||
resp = await proxy_client.get(
|
||||
"/key/list?size=100",
|
||||
headers={"Authorization": f"Bearer {caller.cleartext}"},
|
||||
)
|
||||
assert resp.status_code == 200, f"{actor.value}: {resp.text}"
|
||||
|
||||
returned_hashes = {
|
||||
(entry.get("token") if isinstance(entry, dict) else entry)
|
||||
for entry in resp.json().get("keys", [])
|
||||
}
|
||||
returned_hashes = await _all_visible_hashes(proxy_client, caller.cleartext)
|
||||
visible_seeded = {
|
||||
hashed_to_actor[h] for h in returned_hashes if h in hashed_to_actor
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,8 +1,7 @@
|
|||
from typing import Any, Dict, Optional
|
||||
|
||||
import pytest
|
||||
|
||||
from .actors import TEAM_ALPHA, TEAM_BETA, Actor
|
||||
from .conftest import create_scratch_key
|
||||
|
||||
pytestmark = pytest.mark.asyncio(loop_scope="session")
|
||||
|
||||
|
|
@ -35,26 +34,6 @@ _SCENARIOS = [
|
|||
]
|
||||
|
||||
|
||||
async def _create_scratch_key(
|
||||
proxy_client,
|
||||
seeder_cleartext: str,
|
||||
scratch_prefix: str,
|
||||
*,
|
||||
user_id: str,
|
||||
team_id: Optional[str] = None,
|
||||
) -> str:
|
||||
body: Dict[str, Any] = {"key_alias": scratch_prefix, "user_id": user_id}
|
||||
if team_id is not None:
|
||||
body["team_id"] = team_id
|
||||
resp = await proxy_client.post(
|
||||
"/key/generate",
|
||||
headers={"Authorization": f"Bearer {seeder_cleartext}"},
|
||||
json=body,
|
||||
)
|
||||
assert resp.status_code == 200, f"setup failed: {resp.text}"
|
||||
return resp.json()["key"]
|
||||
|
||||
|
||||
async def _info(proxy_client, cleartext: str):
|
||||
return await proxy_client.get(
|
||||
"/key/info", headers={"Authorization": f"Bearer {cleartext}"}
|
||||
|
|
@ -78,11 +57,11 @@ async def test_key_regenerate_authz_matrix(
|
|||
seeder = world.keys[Actor.PROXY_ADMIN].cleartext
|
||||
|
||||
if target_shape == "self":
|
||||
target_cleartext = await _create_scratch_key(
|
||||
target_cleartext = await create_scratch_key(
|
||||
proxy_client, seeder, scratch.prefix, user_id=caller.user_id
|
||||
)
|
||||
elif target_shape == "owner":
|
||||
target_cleartext = await _create_scratch_key(
|
||||
target_cleartext = await create_scratch_key(
|
||||
proxy_client,
|
||||
seeder,
|
||||
scratch.prefix,
|
||||
|
|
@ -90,7 +69,7 @@ async def test_key_regenerate_authz_matrix(
|
|||
team_id=TEAM_ALPHA,
|
||||
)
|
||||
elif target_shape == "cross_org":
|
||||
target_cleartext = await _create_scratch_key(
|
||||
target_cleartext = await create_scratch_key(
|
||||
proxy_client,
|
||||
seeder,
|
||||
scratch.prefix,
|
||||
|
|
@ -122,7 +101,7 @@ async def test_key_regenerate_authz_matrix(
|
|||
async def test_key_path_regenerate_smoke(proxy_client, scratch, world):
|
||||
"""Pins that POST /key/{key:path}/regenerate shares the same handler."""
|
||||
caller = world.keys[Actor.PROXY_ADMIN]
|
||||
target_cleartext = await _create_scratch_key(
|
||||
target_cleartext = await create_scratch_key(
|
||||
proxy_client, caller.cleartext, scratch.prefix, user_id=caller.user_id
|
||||
)
|
||||
|
||||
|
|
|
|||
|
|
@ -1,10 +1,9 @@
|
|||
from typing import Any, Dict, Optional
|
||||
|
||||
import pytest
|
||||
|
||||
from litellm.proxy.utils import hash_token
|
||||
|
||||
from .actors import TEAM_ALPHA, TEAM_BETA, Actor
|
||||
from .conftest import create_scratch_key
|
||||
|
||||
pytestmark = pytest.mark.asyncio(loop_scope="session")
|
||||
|
||||
|
|
@ -40,29 +39,6 @@ _SCENARIOS = [
|
|||
MARKER_MODEL = "behavior-pin-update-marker-model"
|
||||
|
||||
|
||||
async def _create_scratch_key(
|
||||
proxy_client,
|
||||
seeder_cleartext: str,
|
||||
scratch_prefix: str,
|
||||
*,
|
||||
user_id: str,
|
||||
team_id: Optional[str] = None,
|
||||
organization_id: Optional[str] = None,
|
||||
) -> str:
|
||||
body: Dict[str, Any] = {"key_alias": scratch_prefix, "user_id": user_id}
|
||||
if team_id is not None:
|
||||
body["team_id"] = team_id
|
||||
if organization_id is not None:
|
||||
body["organization_id"] = organization_id
|
||||
resp = await proxy_client.post(
|
||||
"/key/generate",
|
||||
headers={"Authorization": f"Bearer {seeder_cleartext}"},
|
||||
json=body,
|
||||
)
|
||||
assert resp.status_code == 200, f"setup failed: {resp.text}"
|
||||
return resp.json()["key"]
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"actor,target_shape,expected_status",
|
||||
[(a, t, s) for (_id, a, t, s) in _SCENARIOS],
|
||||
|
|
@ -81,11 +57,11 @@ async def test_key_update_authz_matrix(
|
|||
seeder = world.keys[Actor.PROXY_ADMIN].cleartext
|
||||
|
||||
if target_shape == "self":
|
||||
target_cleartext = await _create_scratch_key(
|
||||
target_cleartext = await create_scratch_key(
|
||||
proxy_client, seeder, scratch.prefix, user_id=caller.user_id
|
||||
)
|
||||
elif target_shape == "owner":
|
||||
target_cleartext = await _create_scratch_key(
|
||||
target_cleartext = await create_scratch_key(
|
||||
proxy_client,
|
||||
seeder,
|
||||
scratch.prefix,
|
||||
|
|
@ -93,7 +69,7 @@ async def test_key_update_authz_matrix(
|
|||
team_id=TEAM_ALPHA,
|
||||
)
|
||||
elif target_shape == "cross_org":
|
||||
target_cleartext = await _create_scratch_key(
|
||||
target_cleartext = await create_scratch_key(
|
||||
proxy_client,
|
||||
seeder,
|
||||
scratch.prefix,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue