Scope RAG document reads to their repository and owner - #1180
Open
batzela wants to merge 2 commits into
Open
Conversation
get_document and download_document authorized the repository named in the request path, then resolved the document with doc_repo.find_by_id(). That lookup goes through the document_index GSI, which is keyed on document_id alone, so it returns a document regardless of which repository holds it or who owns it. A caller with access to any one repository could name that repository in the path, supply a document id from a repository they cannot access, and receive the document metadata plus a presigned S3 download URL. Both handlers now resolve documents through _get_authorized_document, which requires that the document belong to the repository named in the path and that the caller own it, or be an admin or RAG admin -- the same ownership rule the delete path already enforced via _ensure_document_ownership. Both paths now share _caller_owns_document so they cannot drift apart. Every denial raises NotFoundException with an identical message rather than ForbiddenException, so responses cannot be used to probe which document ids exist or which repository holds them. download_document also rejects a missing documentId with a validation error instead of looking up None. Adds 15 tests covering cross-repository reads, cross-user reads, unknown ids, the admin and RAG admin exemptions, repository scoping that applies even to admins and owners, and indistinguishable denial responses. One test runs the real RagDocumentRepository against the deployed table schema, including the document_index GSI, to confirm the handler denies a request the GSI resolves. Three existing happy-path tests needed repository_id and username on their mock documents to remain authorized requests; no existing assertion changed.
The codespell hook flags userA as a misspelling of "users", which failed the pre-commit job. Renamed the two test identities to user-alpha and user-bravo, matching the repo-alpha and repo-bravo names already used in that test, and reworded the docstring to describe the scenario directly.
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
1 participant
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.
get_document and download_document authorized the repository named in the request path, then resolved the document with doc_repo.find_by_id(). That lookup goes through the document_index GSI, which is keyed on document_id alone, so it returns a document regardless of which repository holds it or who owns it. A caller with access to any one repository could name that repository in the path, supply a document id from a repository they cannot access, and receive the document metadata plus a presigned S3 download URL.
Both handlers now resolve documents through _get_authorized_document, which requires that the document belong to the repository named in the path and that the caller own it, or be an admin or RAG admin -- the same ownership rule the delete path already enforced via _ensure_document_ownership. Both paths now share _caller_owns_document so they cannot drift apart.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.