From 36c5174c999dee142b350f079d1c417fd9bc4c4d Mon Sep 17 00:00:00 2001 From: Krrish Dholakia Date: Fri, 24 Apr 2026 16:38:39 -0700 Subject: [PATCH] fix(memory): close cross-user write gap + sanitize 500 errors (Veria) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- litellm/proxy/memory/memory_endpoints.py | 83 +++++++++++-- .../proxy/memory/test_memory_endpoints.py | 111 ++++++++++++++++++ 2 files changed, 182 insertions(+), 12 deletions(-) diff --git a/litellm/proxy/memory/memory_endpoints.py b/litellm/proxy/memory/memory_endpoints.py index 937647ce7a0..0d536903f85 100644 --- a/litellm/proxy/memory/memory_endpoints.py +++ b/litellm/proxy/memory/memory_endpoints.py @@ -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) diff --git a/tests/test_litellm/proxy/memory/test_memory_endpoints.py b/tests/test_litellm/proxy/memory/test_memory_endpoints.py index fd82ba9b391..022caddd5aa 100644 --- a/tests/test_litellm/proxy/memory/test_memory_endpoints.py +++ b/tests/test_litellm/proxy/memory/test_memory_endpoints.py @@ -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(