From b631ee1289bbe883c3ff6e1183b1f01782625fd1 Mon Sep 17 00:00:00 2001 From: nangelovv Date: Tue, 16 Jun 2026 17:36:56 +0200 Subject: [PATCH] fix: authorize ownership and validate before deleting in DELETE /documents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DELETE /documents had two problems: 1. No ownership check — any authenticated caller could delete any file's chunks by passing its file_id, allowing cross-tenant data destruction. 2. It deleted first and validated existence afterwards, so a request mixing valid and unknown ids destroyed the valid rows and then returned 404 ("not found"), implying nothing had happened. Resolve the requester's identity (get_user_id, with optional entity_id), fetch the target documents, and verify existence (404) and ownership (403) BEFORE the destructive delete; nothing is deleted when either check fails. Unowned chunks (user_id is None) remain deletable, consistent with the rest of the per-user model. --- app/routes/document_routes.py | 44 +++++++++++++++++++++++++++--- tests/test_main.py | 50 +++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 4 deletions(-) diff --git a/app/routes/document_routes.py b/app/routes/document_routes.py index 15f5b8d0..14b7b427 100644 --- a/app/routes/document_routes.py +++ b/app/routes/document_routes.py @@ -318,22 +318,58 @@ async def get_documents_by_ids(request: Request, ids: list[str] = Query(...)): @router.delete("/documents") -async def delete_documents(request: Request, document_ids: List[str] = Body(...)): +async def delete_documents( + request: Request, + document_ids: List[str] = Body(...), + entity_id: str = None, +): + user_authorized = get_user_id(request, entity_id) try: if isinstance(vector_store, AsyncPgVector): existing_ids = await vector_store.get_filtered_ids( document_ids, executor=request.app.state.thread_pool ) - await vector_store.delete( - ids=document_ids, executor=request.app.state.thread_pool + documents = await vector_store.get_documents_by_ids( + document_ids, executor=request.app.state.thread_pool ) else: existing_ids = vector_store.get_filtered_ids(document_ids) - vector_store.delete(ids=document_ids) + documents = vector_store.get_documents_by_ids(document_ids) + # Validate existence BEFORE deleting anything. Previously the delete ran + # first and the 404 check came after, so a request mixing valid and + # unknown ids destroyed the valid rows and still returned "not found". if not all(id in existing_ids for id in document_ids): raise HTTPException(status_code=404, detail="One or more IDs not found") + # Ownership check: every chunk must be unowned or owned by the requester, + # so a caller cannot delete another tenant's documents by file_id. + unauthorized = sorted( + { + doc.metadata.get("file_id") + for doc in documents + if doc.metadata.get("user_id") not in (None, user_authorized) + } + ) + if unauthorized: + logger.warning( + "Unauthorized delete attempt by user %s for file_ids %s", + user_authorized, + unauthorized, + ) + raise HTTPException( + status_code=403, + detail="Not authorized to delete one or more of the specified documents", + ) + + # Only now perform the destructive delete. + if isinstance(vector_store, AsyncPgVector): + await vector_store.delete( + ids=document_ids, executor=request.app.state.thread_pool + ) + else: + vector_store.delete(ids=document_ids) + file_count = len(document_ids) return { "message": f"Documents for {file_count} file{'s' if file_count > 1 else ''} deleted successfully" diff --git a/tests/test_main.py b/tests/test_main.py index a7fce89a..06d8f758 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -279,3 +279,53 @@ def test_extract_text_from_file(tmp_path, auth_headers): assert json_data["file_id"] == "test_text_123" assert json_data["filename"] == "test_text_extraction.txt" assert json_data["known_type"] is True # text files are known types + + +def test_delete_documents_rejects_other_users(auth_headers, monkeypatch): + """DELETE must refuse (403) to delete documents owned by another user and + must not call delete (regression: there was no ownership check).""" + from app.services.vector_store.async_pg_vector import AsyncPgVector + + async def other_user_docs(self, ids, executor=None): + return [ + Document(page_content="x", metadata={"file_id": i, "user_id": "someoneelse"}) + for i in ids + ] + + monkeypatch.setattr(AsyncPgVector, "get_documents_by_ids", other_user_docs) + + delete_calls = [] + + async def spy_delete(self, ids=None, collection_only=False, executor=None): + delete_calls.append(ids) + + monkeypatch.setattr(AsyncPgVector, "delete", spy_delete) + + response = client.request( + "DELETE", "/documents", json=["testid1"], headers=auth_headers + ) + assert response.status_code == 403, response.text + assert delete_calls == [], "delete must not run for unauthorized request" + + +def test_delete_documents_validates_before_deleting(auth_headers, monkeypatch): + """Unknown ids must 404 without destroying the valid rows. + + Regression: delete ran before the existence check, so a request mixing + valid and unknown ids destroyed the valid rows and still returned 404. + """ + from app.services.vector_store.async_pg_vector import AsyncPgVector + + delete_calls = [] + + async def spy_delete(self, ids=None, collection_only=False, executor=None): + delete_calls.append(ids) + + monkeypatch.setattr(AsyncPgVector, "delete", spy_delete) + + # "ghost" is not among the dummy's known ids (testid1/testid2) + response = client.request( + "DELETE", "/documents", json=["testid1", "ghost"], headers=auth_headers + ) + assert response.status_code == 404, response.text + assert delete_calls == [], "delete must not run when an id is missing"