fix: authorize ownership and validate before deleting in DELETE /documents - #304
Open
nangelovv wants to merge 1 commit into
Open
fix: authorize ownership and validate before deleting in DELETE /documents#304nangelovv wants to merge 1 commit into
nangelovv wants to merge 1 commit into
Conversation
…ments
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Add an ownership check to
DELETE /documentsand move both the existence and ownership checks before the destructive delete.Why
Fixes #303.
user_idcheck, so any authenticated caller could delete another tenant's chunks byfile_id.Change
get_user_id(request, entity_id)(adds an optionalentity_idform/query param, consistent with the embed endpoints).user_idis neitherNone(unowned) nor the requester — before deleting.vector_store.delete(...).Note — contract change
DELETE /documentsnow requires the caller to own (or the document to be unowned) every id. Self-deletes via the user's identity are unaffected; public-mode documents (user_id="public") still match. An unauthenticated process deleting arbitrary ids would now be rejected. (Related closed PR: #262.)Testing
tests/test_main.py:test_delete_documents_rejects_other_users— another user's docs → 403 anddelete()is never called.test_delete_documents_validates_before_deleting— an unknown id → 404 anddelete()is never called.test_delete_documents(self-owned/unowned) still passes.tests/test_main.py: 12 passed.