GitHub OAuth Security Enhancement

[akshayutture-augment]

Test 4

---

Replicated from ai-code-review-evaluation/sentry-greptile#4

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

Code Review: GitHub OAuth Security Enhancement

Summary

This PR introduces a critical security vulnerability by creating an authentication bypass through a race condition in the user validation logic. While the PR adds an OAuth flow to verify user identity during GitHub integration installation, the implementation has a fundamental flaw that could allow attackers to hijack GitHub integrations.

Critical Security Issues

🔴 CRITICAL: Authentication Bypass Through Race Condition

Location: src/sentry/integrations/github/integration.py:500-505

The new code validates that the authenticated user matches the installation user:

# Check that the authenticated GitHub user is the same as who installed the app.
if (
    pipeline.fetch_state("github_authenticated_user")
    != integration.metadata["sender"]["login"]
):
    return error(request, self.active_organization)

VULNERABILITY: This check relies on integration.metadata["sender"] which is set during the webhook event from GitHub when the app is installed.

Attack Scenario:

1. Attacker installs the GitHub app on their account (webhook creates Integration with metadata["sender"]["login"] = "attacker")
2. Attacker discovers the installation_id (from URL or other means)
3. Attacker initiates OAuth flow for victim's Sentry organization with their installation_id
4. Code checks if OrganizationIntegration exists (lines 477-482) - it doesn't yet, so flow continues
5. Code retrieves the Integration and checks if authenticated user matches sender (lines 493-505)
6. Attacker authenticates as themselves ("attacker"), which matches the sender in metadata
7. Attacker successfully links their GitHub installation to victim's Sentry org

Root Cause: The code at lines 477-482 only checks if an OrganizationIntegration exists, not whether the Integration is already "claimed":

try:
    # We want to limit GitHub integrations to 1 organization
    installations_exist = OrganizationIntegration.objects.filter(
        integration=Integration.objects.get(external_id=installation_id)
    ).exists()
except Integration.DoesNotExist:
    return pipeline.next_step()  # New integration, proceed to build_integration

if installations_exist:
    return error(...)  # Already linked to an org
    
# BUG: If Integration exists but no OrganizationIntegration, code continues
# This allows attacker who created Integration to link it to any org

FIX: The validation should happen BEFORE the Integration is created, or you need to verify the authenticated user matches the sender at the webhook reception time, not later. Consider:

# Option 1: Check that Integration doesn't exist at all for new installations
try:
    integration = Integration.objects.get(external_id=installation_id)
    # Integration already exists - someone already installed this
    # Only allow if they're linking to the SAME org that started the flow
    if pipeline.fetch_state("github_authenticated_user") != integration.metadata.get("sender", {}).get("login"):
        return error(request, self.active_organization)
    # Check if already linked to an org
    if OrganizationIntegration.objects.filter(integration=integration).exists():
        return error(request, self.active_organization, 
                    error_short="Github installed on another Sentry organization.",
                    error_long=ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG)
except Integration.DoesNotExist:
    return pipeline.next_step()

# Option 2: Store a secure token during webhook reception and validate it here

🟡 MODERATE: Missing Metadata Validation

Location: src/sentry/integrations/github/integration.py:503

!= integration.metadata["sender"]["login"]

ISSUE: No validation that integration.metadata["sender"] exists or is properly structured. This will raise a KeyError if:

• The Integration was created before this security feature was added (legacy data)
• The webhook payload was malformed
• The metadata was corrupted

FIX: Add defensive checks:

sender = integration.metadata.get("sender", {})
sender_login = sender.get("login") if isinstance(sender, dict) else None

if not sender_login:
    logger.warning("Integration missing sender metadata", extra={"integration_id": integration.id})
    return error(request, self.active_organization)
    
if pipeline.fetch_state("github_authenticated_user") != sender_login:
    return error(request, self.active_organization)

🟡 MODERATE: No Error Handling for get_user_info

Location: src/sentry/integrations/github/integration.py:434-436

authenticated_user_info = get_user_info(payload["access_token"])
if "login" not in authenticated_user_info:
    return error(request, self.active_organization)

ISSUE: The code doesn't handle exceptions or malformed responses from get_user_info(). If the API call fails:

• An exception could crash the flow
• An empty dict could pass through
• Error responses might not contain "login"

FIX: Add proper error handling:

try:
    authenticated_user_info = get_user_info(payload["access_token"])
except Exception as e:
    logger.exception("Failed to get GitHub user info", exc_info=e)
    return error(request, self.active_organization)

if not authenticated_user_info or "login" not in authenticated_user_info:
    logger.warning("Invalid user info response", extra={"response": authenticated_user_info})
    return error(request, self.active_organization)

🟡 MODERATE: Silent Exception Handling

Location: src/sentry/integrations/github/integration.py:425-429

try:
    body = safe_urlread(req).decode("utf-8")
    payload = dict(parse_qsl(body))
except Exception:
    payload = {}

ISSUE: Silently catches all exceptions and returns empty dict. This masks real errors like:

• Network failures
• Invalid OAuth responses
• Encoding errors

FIX: Add logging and be more specific:

try:
    body = safe_urlread(req).decode("utf-8")
    payload = dict(parse_qsl(body))
except Exception as e:
    logger.exception("Failed to exchange OAuth code for token", exc_info=e)
    payload = {}

🟢 MINOR: Inconsistent Capitalization

Location: src/sentry/integrations/github/integration.py:487

error_short="Github installed on another Sentry organization."

Should be "GitHub" not "Github" for consistency.

Positive Aspects

✅ Good: Added OAuth flow to verify user identity - the approach is sound
✅ Good: State parameter validation protects against CSRF attacks
✅ Good: Comprehensive test coverage for the new functionality
✅ Good: Refactored error handling into reusable error() function
✅ Good: Clean separation of concerns with separate PipelineView classes

Test Coverage Analysis

Strong Coverage:

• ✅ test_basic_flow - Tests successful installation flow
• ✅ test_github_installed_on_another_org - Tests duplicate installation prevention
• ✅ test_installation_not_found - Tests missing installation handling
• ✅ NEW test_github_user_mismatch (lines 399-455) - Tests user validation with attacker scenario

Missing Coverage:

• ❌ Race condition scenario where Integration exists but OrganizationIntegration doesn't
• ❌ Legacy Integrations without metadata["sender"] field
• ❌ Error handling for failed get_user_info() calls
• ❌ Malformed webhook payloads

Test Analysis - test_github_user_mismatch:

The test simulates an attacker webhook but doesn't fully test the vulnerability:

webhook_event["sender"]["login"] = "attacker"  # Sets sender to attacker
# ... webhook creates Integration with sender="attacker" ...
resp = self.client.get(init_path_2)  # Different user initiates flow
resp = self.client.get(setup_path_2)  # OAuth returns "octocat" as authenticated user
# Test expects this to FAIL because octocat != attacker

This test validates the security check works when users DON'T match. But it doesn't test when:

• Attacker IS the authenticated user (attacker matches attacker - should this be allowed?)
• Integration exists without OrganizationIntegration

Recommendations

CRITICAL - Must Fix Before Merge:

1. Fix the race condition vulnerability - Rethink the validation logic at lines 477-505. Consider:

   • Only allowing Integration creation for organizations that initiated the flow
   • Adding a secure installation token that ties the webhook to a specific org
   • Preventing ANY linking of existing Integrations without additional validation

2. Add proper metadata validation with defensive checks for missing/corrupted sender data

3. Add comprehensive security event logging for all validation failures

Strongly Recommended:

4. Add test coverage for the race condition scenario and edge cases

5. Add proper exception handling throughout the OAuth flow

6. Consider timing validation - OAuth flow completion should happen within reasonable time window after installation

7. Add monitoring/alerting for repeated failed installation attempts

8. Fix capitalization consistency ("GitHub" not "Github")

Security Impact Assessment

Current Code: 🔴 HIGH RISK - The race condition could allow attackers to hijack GitHub integrations and gain unauthorized access to repository data, commit information, and potentially trigger deployments or access sensitive code.

Recommendation: Do not merge without addressing the critical race condition vulnerability.

---