mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-17 23:51:30 +00:00
fix(memory): close cross-user write gap + sanitize 500 errors (Veria)
Addresses two Veria findings: **High — cross-user memory tampering via team membership.** The visibility filter uses an OR (`user_id == caller OR team_id == caller`) so team members can SEE each other's team-scoped rows. That's intentional for list/get. But because PUT/DELETE used the same filter to find the target row, any team member could overwrite or delete a teammate's *personal* row whenever both `user_id` and `team_id` were stamped on it — broader visibility was being silently treated as broader authority. New `_assert_write_access(row, caller)` enforces ownership for mutations. Non-admin rules: - The row's `user_id` must match the caller (personal ownership), OR - The row has no `user_id` and its `team_id` matches the caller's team (a "pure team row" intended for shared writes). Admins bypass the check. The same gate runs in PUT (both regular and post-race-recovery branches) and DELETE. **Medium — DB internals leaked through 500 detail.** Every `except` block was raising `HTTPException(500, detail=str(e))`, which surfaces Prisma error strings (table/column names, host:port, error class names) to API callers. New `_internal_error()` helper logs the real exception server-side and returns a generic, caller-safe `detail`. Applied to create, list, upsert (general fallthrough), and delete. Also tightened the race-recovery 409 message to drop the "in a different scope" wording — the caller never needs to know whose scope it lives in. Tests (+5): - teammate cannot overwrite personal row → 403 - teammate cannot delete personal row → 403 - teammate CAN modify pure team row (no user_id stamped) → 200 - admin bypasses write-auth → 200 - 500 response never echoes Prisma internals (table/host/class names) 25/25 unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
4a4b1123fb
commit
36c5174c99
2 changed files with 182 additions and 12 deletions
|
|
@ -86,6 +86,53 @@ def _require_prisma():
|
|||
return prisma_client
|
||||
|
||||
|
||||
def _internal_error(
|
||||
log_message: str, exc: Exception, default_detail: str
|
||||
) -> HTTPException:
|
||||
"""
|
||||
Build a 500 HTTPException with a generic, caller-safe `detail` while
|
||||
logging the actual exception server-side. Avoids leaking internal Prisma /
|
||||
DB details (table names, columns, connection metadata) to API callers.
|
||||
"""
|
||||
verbose_proxy_logger.exception(log_message, exc)
|
||||
return HTTPException(status_code=500, detail=default_detail)
|
||||
|
||||
|
||||
def _assert_write_access(row: Any, user_api_key_dict: UserAPIKeyAuth) -> None:
|
||||
"""
|
||||
Enforce ownership for mutations (PUT/DELETE).
|
||||
|
||||
The visibility filter uses an OR (`user_id == caller OR team_id == caller`)
|
||||
so team members can READ each other's team-scoped rows. That's intentional
|
||||
for list/get. For writes, broader visibility != broader authority — without
|
||||
this check, any team member could overwrite or delete a teammate's
|
||||
personal row whenever both `user_id` and `team_id` are stamped on it.
|
||||
|
||||
Rule (non-admin): the row must be authored by the caller's user_id, or be
|
||||
a "pure team row" (no user_id stamped) within the caller's team.
|
||||
Admins may modify any row.
|
||||
"""
|
||||
if _is_admin(user_api_key_dict):
|
||||
return
|
||||
row_user_id = getattr(row, "user_id", None)
|
||||
row_team_id = getattr(row, "team_id", None)
|
||||
|
||||
# Personal ownership: caller authored this row.
|
||||
if row_user_id and row_user_id == user_api_key_dict.user_id:
|
||||
return
|
||||
# Pure team row (no user_id stamped) inside the caller's team.
|
||||
if (
|
||||
row_user_id is None
|
||||
and row_team_id is not None
|
||||
and row_team_id == user_api_key_dict.team_id
|
||||
):
|
||||
return
|
||||
raise HTTPException(
|
||||
status_code=403,
|
||||
detail="You do not have permission to modify this memory entry.",
|
||||
)
|
||||
|
||||
|
||||
def _is_unique_violation(exc: Exception) -> bool:
|
||||
"""
|
||||
Detect a Prisma unique-constraint violation.
|
||||
|
|
@ -195,8 +242,11 @@ async def create_memory(
|
|||
status_code=409,
|
||||
detail=f"Memory with key '{body.key}' already exists.",
|
||||
)
|
||||
verbose_proxy_logger.exception("Error creating memory: %s", e)
|
||||
raise HTTPException(status_code=500, detail=str(e))
|
||||
raise _internal_error(
|
||||
"Error creating memory: %s",
|
||||
e,
|
||||
"Internal error creating memory entry.",
|
||||
)
|
||||
|
||||
return _row_to_model(row)
|
||||
|
||||
|
|
@ -252,8 +302,9 @@ async def list_memory(
|
|||
take=page_size,
|
||||
)
|
||||
except Exception as e:
|
||||
verbose_proxy_logger.exception("Error listing memory: %s", e)
|
||||
raise HTTPException(status_code=500, detail=str(e))
|
||||
raise _internal_error(
|
||||
"Error listing memory: %s", e, "Internal error listing memory entries."
|
||||
)
|
||||
|
||||
return MemoryListResponse(memories=[_row_to_model(r) for r in rows], total=total)
|
||||
|
||||
|
|
@ -343,6 +394,11 @@ async def upsert_memory(
|
|||
try:
|
||||
existing = await _find_existing()
|
||||
if existing is not None:
|
||||
# Visibility != write authority. Make sure the caller actually
|
||||
# owns this row (their user_id matches, or it's a pure team row in
|
||||
# their team) — otherwise a teammate could overwrite a personal
|
||||
# entry through the OR-based visibility filter.
|
||||
_assert_write_access(existing, user_api_key_dict)
|
||||
row = await prisma_client.db.litellm_memorytable.update(
|
||||
where={"memory_id": existing.memory_id},
|
||||
data=data,
|
||||
|
|
@ -384,11 +440,10 @@ async def upsert_memory(
|
|||
# (owned by someone else). Treat as conflict.
|
||||
raise HTTPException(
|
||||
status_code=409,
|
||||
detail=(
|
||||
f"Memory with key '{key}' already exists in a "
|
||||
"different scope."
|
||||
),
|
||||
detail=f"Memory with key '{key}' already exists.",
|
||||
)
|
||||
# Same write-authorization check as the non-race path.
|
||||
_assert_write_access(existing_after_race, user_api_key_dict)
|
||||
row = await prisma_client.db.litellm_memorytable.update(
|
||||
where={"memory_id": existing_after_race.memory_id},
|
||||
data=data,
|
||||
|
|
@ -396,8 +451,9 @@ async def upsert_memory(
|
|||
except HTTPException:
|
||||
raise
|
||||
except Exception as e:
|
||||
verbose_proxy_logger.exception("Error upserting memory: %s", e)
|
||||
raise HTTPException(status_code=500, detail=str(e))
|
||||
raise _internal_error(
|
||||
"Error upserting memory: %s", e, "Internal error updating memory entry."
|
||||
)
|
||||
|
||||
return _row_to_model(row)
|
||||
|
||||
|
|
@ -415,12 +471,15 @@ async def delete_memory(
|
|||
"""Delete a memory entry by key, scoped to the caller."""
|
||||
prisma_client = _require_prisma()
|
||||
row = await _find_memory_for_caller(prisma_client, key, user_api_key_dict)
|
||||
# Visibility != write authority — see the upsert handler for the rationale.
|
||||
_assert_write_access(row, user_api_key_dict)
|
||||
try:
|
||||
await prisma_client.db.litellm_memorytable.delete(
|
||||
where={"memory_id": row.memory_id}
|
||||
)
|
||||
except Exception as e:
|
||||
verbose_proxy_logger.exception("Error deleting memory: %s", e)
|
||||
raise HTTPException(status_code=500, detail=str(e))
|
||||
raise _internal_error(
|
||||
"Error deleting memory: %s", e, "Internal error deleting memory entry."
|
||||
)
|
||||
|
||||
return MemoryDeleteResponse(key=key, deleted=True)
|
||||
|
|
|
|||
|
|
@ -479,6 +479,117 @@ class TestMemoryEndpoints:
|
|||
resp = client.put("/v1/memory/notes", json={})
|
||||
assert resp.status_code == 400
|
||||
|
||||
def test_put_memory_teammate_cannot_overwrite_personal_row(self):
|
||||
"""
|
||||
Visibility OR-filter lets a team member SEE a teammate's row, but the
|
||||
write-authorization check must prevent them from overwriting it.
|
||||
Teammate B should get 403, not silently take over user A's entry.
|
||||
"""
|
||||
table = self.prisma.db.litellm_memorytable
|
||||
table.rows.append(
|
||||
_make_row(
|
||||
memory_id="m1",
|
||||
key="user_role",
|
||||
value="A's notes",
|
||||
user_id="user-a",
|
||||
team_id="team-shared",
|
||||
)
|
||||
)
|
||||
# User B is on the same team but a different user_id.
|
||||
client = _make_client(_user_auth("user-b", "team-shared"))
|
||||
with _patch_prisma(self.prisma):
|
||||
resp = client.put("/v1/memory/user_role", json={"value": "B overwrite"})
|
||||
assert resp.status_code == 403, resp.text
|
||||
# Row is unchanged.
|
||||
assert table.rows[0].value == "A's notes"
|
||||
|
||||
def test_delete_memory_teammate_cannot_delete_personal_row(self):
|
||||
"""Same as above, but for DELETE."""
|
||||
table = self.prisma.db.litellm_memorytable
|
||||
table.rows.append(
|
||||
_make_row(
|
||||
memory_id="m1",
|
||||
key="user_role",
|
||||
value="A's notes",
|
||||
user_id="user-a",
|
||||
team_id="team-shared",
|
||||
)
|
||||
)
|
||||
client = _make_client(_user_auth("user-b", "team-shared"))
|
||||
with _patch_prisma(self.prisma):
|
||||
resp = client.delete("/v1/memory/user_role")
|
||||
assert resp.status_code == 403, resp.text
|
||||
assert len(table.rows) == 1
|
||||
|
||||
def test_put_memory_teammate_can_modify_pure_team_row(self):
|
||||
"""
|
||||
A "pure" team row (no user_id stamped, only team_id) is intended to be
|
||||
shared — any team member can modify it.
|
||||
"""
|
||||
table = self.prisma.db.litellm_memorytable
|
||||
table.rows.append(
|
||||
_make_row(
|
||||
memory_id="m1",
|
||||
key="team_playbook",
|
||||
value="v1",
|
||||
user_id=None,
|
||||
team_id="team-shared",
|
||||
)
|
||||
)
|
||||
client = _make_client(_user_auth("user-b", "team-shared"))
|
||||
with _patch_prisma(self.prisma):
|
||||
resp = client.put("/v1/memory/team_playbook", json={"value": "v2"})
|
||||
assert resp.status_code == 200, resp.text
|
||||
assert table.rows[0].value == "v2"
|
||||
|
||||
def test_admin_can_modify_any_row(self):
|
||||
"""Admin bypasses write-authorization."""
|
||||
table = self.prisma.db.litellm_memorytable
|
||||
table.rows.append(
|
||||
_make_row(
|
||||
memory_id="m1",
|
||||
key="user_role",
|
||||
value="A's notes",
|
||||
user_id="user-a",
|
||||
team_id="team-shared",
|
||||
)
|
||||
)
|
||||
client = _make_client(_admin_auth())
|
||||
with _patch_prisma(self.prisma):
|
||||
resp = client.put("/v1/memory/user_role", json={"value": "admin override"})
|
||||
assert resp.status_code == 200, resp.text
|
||||
assert table.rows[0].value == "admin override"
|
||||
|
||||
def test_internal_error_does_not_leak_db_details(self):
|
||||
"""
|
||||
500 responses must not echo Prisma internals (table names, columns,
|
||||
connection strings) back to the caller.
|
||||
"""
|
||||
table = self.prisma.db.litellm_memorytable
|
||||
|
||||
async def boom(*_args, **_kwargs):
|
||||
raise Exception(
|
||||
"PrismaClientKnownRequestError: column "
|
||||
'"LiteLLM_MemoryTable.value" does not exist '
|
||||
"on host db.internal:5432"
|
||||
)
|
||||
|
||||
original_create = table.create
|
||||
table.create = boom # type: ignore[assignment]
|
||||
|
||||
client = _make_client(_user_auth("user-a", "team-a"))
|
||||
with _patch_prisma(self.prisma):
|
||||
resp = client.post("/v1/memory", json={"key": "x", "value": "y"})
|
||||
|
||||
table.create = original_create # type: ignore[assignment]
|
||||
|
||||
assert resp.status_code == 500
|
||||
body_text = resp.text
|
||||
for leak in ("LiteLLM_MemoryTable", "db.internal", "PrismaClient"):
|
||||
assert (
|
||||
leak not in body_text
|
||||
), f"Leaked '{leak}' in 500 response: {body_text}"
|
||||
|
||||
def test_delete_memory(self):
|
||||
table = self.prisma.db.litellm_memorytable
|
||||
table.rows.append(
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue