Improve security: prevent object enumeration and unify permission checks

[JSv4]

Summary

This PR enhances security by preventing object-existence enumeration attacks and unifying permission checks across GraphQL mutations. The changes ensure that users cannot determine whether a resource exists based on error messages, and that permission checks consistently use the application's permission system.

Key Changes

• Prevent object-existence enumeration: Modified three mutations (UpdateDocumentSummary, RestoreDeletedDocument, PermanentlyDeleteDocument) to use visible_to_user() querysets instead of direct .get() calls. This prevents attackers from determining if a document or corpus exists by observing different error messages.

• Unified error messages: Replaced specific error messages ("Document not found", "Corpus not found", "You can only edit summaries you created") with a generic message: "Document or corpus not found, or you do not have permission." This prevents information leakage about resource existence.

• Consistent permission checking: Updated StartDocumentAnalysisMutation to use visible_to_user() querysets instead of checking is_public and creator fields directly, ensuring consistent permission enforcement across the codebase.

• Improved permission logic: In UpdateDocumentSummary, replaced manual permission checks with the user_has_permission_for_obj() utility function with proper guardian integration for corpus update permissions.

• Cleaner exception handling: Removed redundant DoesNotExist exception handlers since objects are now retrieved through filtered querysets that return early if not found.

• Better error messages: Simplified generic error messages in exception handlers to avoid exposing implementation details.

Implementation Details

• All mutations now use visible_to_user() to filter querysets before retrieval, ensuring permission checks happen at the database level
• Early returns with consistent error messages prevent downstream code from executing with unauthorized resources
• The Corpus import was moved to the top of analysis_mutations.py for consistency
• Permission checks now leverage the existing user_has_permission_for_obj() utility function where applicable

https://claude.ai/code/session_01Qgk1iTRL4mFWpYRhouXWo4

[claude]

Code Review

The direction of this PR is correct — using visible_to_user() querysets to prevent object-existence enumeration is the right approach and aligns with the patterns documented in CLAUDE.md. Below are findings ordered by severity.

---

🚨 Potential Breaking Change: UpdateDocumentSummary permission regression

File: config/graphql/document_mutations.py, lines 346–358

The old code for creating a new summary (no existing_summary) allowed three cases:

is_public_corpus = corpus.is_public         # any user on a public corpus
user_has_corpus_perm = user.has_perm("update_corpus", corpus)  # explicit guardian perm
user_is_creator = corpus.creator == user    # corpus creator

The new code only checks:

user_has_permission_for_obj(user, corpus, PermissionTypes.UPDATE, include_group_permissions=True)

Per the docstring of user_has_permission_for_obj (line 316–317 in opencontractserver/utils/permissioning.py):

It does NOT consider:

• Creator status (for models with guardian permissions)

For guardian-enabled models, the function checks only explicit guardian object permissions — not the creator field. The is_public flag only grants read_corpus, not update_corpus.

Impact 1 — Public corpora: Any authenticated user could previously create summaries for public corpora. Now they cannot unless they have explicit update_corpus guardian permissions. This is arguably more correct from a security standpoint but is an undocumented breaking change that should be explicitly called out.

Impact 2 — Creator-created corpora (test regression risk): The existing test test_document_summary_creation_and_editing creates a corpus via Corpus.objects.create() (not through the CreateCorpusMutation). This means no guardian permissions are assigned, because the permission grant at line 152 of corpus_mutations.py only runs inside the mutation's mutate(). As a result, user_has_permission_for_obj would return False for that corpus creator, causing the test's first UpdateDocumentSummary call to fail. Please verify the test still passes after this change.

---

⚠️ Error message leak not fully addressed in UpdateDocumentSummary

File: config/graphql/document_mutations.py, line 388–394

The outer catch block still leaks exception details:

except Exception as e:
    return UpdateDocumentSummary(
        ok=False,
        message=f"Error updating document summary: {str(e)}",  # leaks internals
        ...
    )

The equivalent handlers in RestoreDeletedDocument and PermanentlyDeleteDocument were correctly genericised in this PR (the exception detail is logged but not returned). This one was missed.

---

⚠️ Misleading error message when deleted-path is not found

File: config/graphql/document_mutations.py, lines 1282–1287

if not deleted_path:
    return RestoreDeletedDocument(
        ok=False,
        message=not_found_msg,   # "Document or corpus not found, or you do not have permission."
        ...
    )

At this point the user has already successfully retrieved both the document and corpus via visible_to_user(). The real reason for failure is "the document is not currently in a deleted state in this corpus." Returning the generic not_found_msg here is semantically incorrect and will confuse legitimate users. The original message ("Cannot restore document - it may not be deleted or may not exist in this corpus") was much more helpful and does not disclose anything about other users' resources. Consider a distinct non-leaking message like "Document is not currently in a deleted state in this corpus."

---

ℹ️ Pre-existing: service error messages still pass through

Files: RestoreDeletedDocument (line 1296–1300) and PermanentlyDeleteDocument (line 1372)

Both mutations still return the raw error string from DocumentFolderService to the client. That service can return messages like "Document does not belong to this corpus" and "You do not have permission to restore documents in this corpus". These strings were already passing through before this PR, but since the PR is explicitly focused on information leakage, it is worth addressing here while the touch-points are active.

---

✅ No tests for the enumeration-prevention behaviour

The PR adds no tests that verify the key security property it introduces — i.e. that looking up a nonexistent object and looking up an inaccessible object return identical responses. Consider adding a test case covering:

1. Request a mutation with a nonexistent document ID → expect the generic error message.
2. Request the same mutation with a document owned by a different user → expect the same generic error message.
3. Verify that the HTTP status codes and response shapes are identical in both cases.

---

✅ Positives

• Using visible_to_user() for lookups instead of bare .get() is the correct CLAUDE.md-documented pattern.
• Unifying the "not found" and "permission denied" error messages eliminates the timing/message oracle.
• Moving the Corpus import to the top of analysis_mutations.py is a correct cleanup.
• Adding include_group_permissions=True to the guardian check is an improvement over the old has_perm() call.
• Stripping exception strings from the outer catch blocks in RestoreDeletedDocument and PermanentlyDeleteDocument is good practice.

---

Summary

#	Severity	Location	Issue
1	High	UpdateDocumentSummary permission check	Corpus creator without guardian perms (direct-created) now denied; test may fail
2	Medium	UpdateDocumentSummary exception handler	Inner exception message still leaked to client
3	Low	RestoreDeletedDocument	Generic error where a specific, non-leaking message is correct
4	Low	Both restore/delete mutations	Service error strings still forwarded to client (pre-existing)
5	Low	Test coverage	No test for the anti-enumeration property being added

[claude]

Code Review

A previous automated review left several findings that need correction, plus a couple of genuine issues. Ordered by severity.

---

Correction: previous Finding 2 was incorrect

The prior review claimed UpdateDocumentSummary still leaked the exception string in its outer catch block. Looking at the actual PR code (lines 390-397), the exception detail is moved to logger.error() and only a generic message is returned to the client. This is handled correctly.

---

Correction: previous Finding 3 was incorrect

The prior review claimed RestoreDeletedDocument was returning the generic not_found_msg for the "deleted path not found" branch. The code at line 1288 returns a distinct, specific, non-leaking message:
"Document is not currently in a deleted state in this corpus."
The PR and the updated test assertion are both correct.

---

[Medium] Undocumented behavioral change: public-corpus summary creation

The previous review flagged a creator-check regression but got the specifics wrong. Here is the accurate picture.

Old permission logic when creating a new summary (no existing_summary):

is_public_corpus OR user_has_corpus_perm OR user_is_creator  → allow

New permission logic (document_mutations.py line 348):

has_perm = corpus.creator == user or user_has_permission_for_obj(
    user_val=user, instance=corpus, permission=PermissionTypes.UPDATE,
    include_group_permissions=True,
)

The creator check is preserved (the prior review was wrong). The is_public path is removed. Any user who previously could create summaries on a public corpus because corpus.is_public == True can no longer do so unless they are the creator or hold an explicit update_corpus guardian permission.

This is arguably the correct security posture (visibility ≠ write access), but it is a breaking behavioral change not documented in the PR description. Please call it out explicitly so operators upgrading are aware.

Test impact: the existing test_document_summary_creation_and_editing test will still pass because Corpus.objects.create(creator=self.user) sets corpus.creator correctly, and visible_to_user() includes Q(creator=user) in its filter.

---

[Low] StartDocumentAnalysisMutation outer catch still leaks

The PR updates the permission logic in this mutation but did not update the outer exception handler at line 168:

return StartDocumentAnalysisMutation(ok=False, message=f"Error: {str(e)}")

If Analyzer.objects.get(pk=analyzer_pk) raises DoesNotExist, the client receives the raw Django exception string. Since the PR explicitly targets information leakage, this handler should log the detail and return a generic message, consistent with the other mutations touched here.

---

[Low / Pre-existing] Service error strings forwarded to client

Both RestoreDeletedDocument (line 1301: message=error) and PermanentlyDeleteDocument still forward raw DocumentFolderService error strings directly to the client. These include strings like "You do not have permission to restore documents in this corpus". Pre-existing, but worth addressing while these touch-points are open.

---

What the PR gets right

• visible_to_user() lookups are the correct CLAUDE.md pattern and prevent timing/message-based enumeration.
• Unifying not_found_msg across existence and permission failures eliminates the message oracle.
• include_group_permissions=True on the corpus update check is an improvement over the old bare has_perm() call.
• Exception strings stripped from outer catch blocks in RestoreDeletedDocument and PermanentlyDeleteDocument.
• Moving the Corpus import to the top of analysis_mutations.py is correct cleanup.

---

Summary

Severity	Location	Issue
Medium	UpdateDocumentSummary	is_public corpus write access silently removed; undocumented breaking change
Low	StartDocumentAnalysisMutation L168	Outer catch still leaks str(e) to client
Low	RestoreDeletedDocument, PermanentlyDeleteDocument	Service error strings still forwarded (pre-existing)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!