Fix some minor security flags

[JSv4]

Coupled return messages were leaking obj existence

[claude]

Code Review Summary

This PR addresses IDOR (Insecure Direct Object Reference) vulnerabilities in 6 GraphQL mutations by unifying error messages for "not found" and "permission denied" cases. This prevents attackers from enumerating valid object IDs by observing different error responses.

✅ Strengths

1. Correct IDOR Protection Pattern: All mutations now use visible_to_user(user).get(pk=pk) which provides unified errors for both non-existent and inaccessible objects.

2. Comprehensive Test Coverage: Added 376 lines of new IDOR protection tests across two test files:

   • test_permission_fixes.py: 4 new test methods for annotations and relationships
   • test_voting_mutations_graphql.py: 2 new test methods for voting operations
   • Tests verify that unauthorized users receive identical error messages for non-existent vs inaccessible objects

3. Consistent Error Messages: All mutations now return the same error format: "[Object] not found or you do not have permission to access it"

4. Security Best Practice: Two-layer defense:

   • First filter with visible_to_user() (unified error)
   • Then check explicit permission with user_has_permission_for_obj() (same unified error)

🔍 Issues Found

Critical Issue: Missing CHANGELOG Entry

Severity: 🔴 CRITICAL (Per CLAUDE.md requirements)

The CHANGELOG.md was not updated for these security fixes. According to CLAUDE.md:

IMPORTANT: Always update CHANGELOG.md when making significant changes to the codebase.

When to update:

• Production code bugs fixed (document file location, line numbers, and impact)
• Security fixes

Required Fix: Add a section like this to CHANGELOG.md under the most recent Unreleased heading:

### Security

#### IDOR Vulnerabilities Fixed in 6 GraphQL Mutations (PR #800)
- **HIGH**: Fixed information leakage allowing object ID enumeration via different error messages
  - `RemoveAnnotation` (`config/graphql/mutations.py:2173-2209`)
  - `RejectAnnotation` (`config/graphql/mutations.py:2226-2266`)
  - `ApproveAnnotation` (`config/graphql/mutations.py:2289-2329`)
  - `RemoveRelationship` (`config/graphql/mutations.py:2450-2490`)
  - `VoteMessageMutation` (`config/graphql/voting_mutations.py:68-110`)
  - `RemoveVoteMutation` (`config/graphql/voting_mutations.py:148-175`)
- **Attack Vector**: Unauthorized users could distinguish between "object doesn't exist" and "object exists but you can't access it" by observing different error messages
- **Impact**: Allowed enumeration of valid annotation/relationship/message IDs
- **Solution**: All mutations now use `visible_to_user()` pattern with unified error messages
- **Test Coverage**: Added 376 lines of IDOR protection tests in `test_permission_fixes.py` and `test_voting_mutations_graphql.py`

Minor Issue: Test File Docstring Claims 7 Vulnerabilities

File: opencontractserver/tests/test_permission_fixes.py:1-16

The docstring states:

This test suite verifies the 7 critical GraphQL mutation vulnerabilities

But this PR only fixes 6 mutations (as stated in the commit message). The 7th mentioned in REMEDIATION_GUIDE.md is DeleteMultipleLabelMutation, which is not addressed in this PR.

Recommendation: Update the docstring to be more accurate:

"""
Tests for IDOR protection fixes in GraphQL mutations.

This test suite verifies IDOR vulnerabilities have been properly fixed
by ensuring identical error messages for 'not found' vs 'no permission' cases.

Tests cover mutations fixed in PR #800:
1. RemoveAnnotation - Cannot delete annotations without permission
2. RejectAnnotation - Cannot reject annotations without permission  
3. ApproveAnnotation - Cannot approve annotations without permission
4. RemoveRelationship - Cannot delete relationships without permission
5. VoteMessage - Cannot vote on inaccessible messages
6. RemoveVote - Cannot remove votes on inaccessible messages

Also includes tests for previous security fixes (badges, moderation, etc.)
"""

🎯 Code Quality Observations

1. Voting Mutations: Permission Check Simplification

Files: config/graphql/voting_mutations.py:68-86, 148-162

The refactor removed redundant permission checks, which is good:

Before:

message = ChatMessage.objects.get(pk=pk)  # Get message
conversation = message.conversation
if not user_has_permission_for_obj(user, conversation, PermissionTypes.READ):  # Check conversation
    return error

After:

message = ChatMessage.objects.visible_to_user(user).get(pk=pk)  # Automatically checks conversation permissions

This is cleaner because ChatMessage.visible_to_user() already implements conversation-level permission checks (see opencontractserver/conversations/models.py:175-222).

2. Nested Try-Except Pattern

Files: config/graphql/mutations.py:2179-2188, 2450-2464

The nested try-except blocks work correctly but could be slightly more elegant:

try:
    user = info.context.user
    annotation_pk = from_global_id(annotation_id)[1]
    
    try:  # Inner try for visible_to_user
        annotation_obj = Annotation.objects.visible_to_user(user).get(pk=annotation_pk)
    except Annotation.DoesNotExist:
        return RemoveAnnotation(...)
    
    # ... rest of logic
except Exception as e:  # Outer try for unexpected errors
    logger.error(...)

Alternative pattern (used in other mutations like RejectAnnotation:2232-2240):

try:
    annotation = Annotation.objects.visible_to_user(user).get(pk=pk)
except (ObjectDoesNotExist, Annotation.DoesNotExist):
    return RejectAnnotation(...)

Both patterns are functionally equivalent. The dual-exception catch is slightly more defensive (catches both generic and model-specific DoesNotExist).

3. Test Quality: Excellent Assertion Messages

The new tests include excellent descriptive messages:

self.assertEqual(
    error_msg_no_perm,
    error_msg_not_found,
    "IDOR vulnerability: Different error messages allow enumeration of valid message IDs",
)

This makes debugging test failures very clear. 👍

🛡️ Security Analysis

IDOR Protection: ✅ Correct Implementation

The pattern used is the correct approach per OpenContracts' security standards:

1. Query by visibility first: Model.objects.visible_to_user(user).get(pk=pk)
2. Unified error messages: Same message whether object doesn't exist OR user lacks permission
3. Prevents timing attacks: Both code paths execute similar queries (no measurable timing difference)

Edge Case: What if visible_to_user() throws an exception?

The mutations handle this with the outer try-except:

except Exception as e:
    logger.error(f"Error deleting annotation {annotation_id}: {e}")
    return RemoveAnnotation(ok=False, message=f"Error deleting annotation: {str(e)}")

⚠️ Potential information leakage: The generic exception handler returns str(e) which could leak sensitive error details.

Recommendation for future PR: Consider returning a generic error message for unexpected exceptions:

except Exception as e:
    logger.error(f"Error deleting annotation {annotation_id}: {e}")
    return RemoveAnnotation(ok=False, message="An unexpected error occurred")

📊 Test Coverage Assessment

Tests Added: 376 Lines

• test_permission_fixes.py: 260 lines (4 test classes)
• test_voting_mutations_graphql.py: 114 lines (2 test methods)

Coverage Scenarios:

✅ Non-existent object (ID 999999)
✅ Existing object without permissions
✅ Error message comparison
✅ Both mutations fail (ok=False)

Missing Test Scenarios (Non-blocking):

• ⚪ Timing attack verification (optional - difficult to test reliably)
• ⚪ GraphQL error path testing (currently only tests successful error responses)

🔄 Consistency with Existing Code

The changes follow existing patterns in the codebase:

• badge_mutations.py: Already uses visible_to_user() pattern with unified errors
• agent_mutations.py: Uses same pattern per REMEDIATION_GUIDE.md
• notification_mutations.py: Uses ownership model (recipient=user)

⚡ Performance Considerations

No performance regression expected:

• visible_to_user() uses the same database queries that would be needed for explicit permission checks
• Removed redundant conversation permission check in voting mutations (slight performance improvement)

🎯 Recommendations

Must Fix (Before Merge):

1. Add CHANGELOG.md entry (see template above)
2. Update test file docstring to accurately reflect 6 fixes, not 7

Optional Improvements (Future PRs):

3. Consider generic error messages for unexpected exceptions to avoid information leakage
4. Extract the IDOR protection pattern to a utility function to reduce code duplication:

   def get_object_or_permission_error(model, pk, user, error_prefix):
       try:
           return model.objects.visible_to_user(user).get(pk=pk), None
       except model.DoesNotExist:
           return None, f"{error_prefix} not found or you do not have permission to access it"

✅ Final Verdict

Strong approval pending CHANGELOG update. The security fixes are correctly implemented and well-tested. The only blocking issue is the missing CHANGELOG entry per project requirements.

Once the CHANGELOG is updated, this PR will:

• ✅ Fix 6 HIGH-severity IDOR vulnerabilities
• ✅ Add comprehensive test coverage
• ✅ Follow established security patterns
• ✅ Maintain backward compatibility
• ✅ Have no performance impact

Excellent security work! 🔒