GitHub OAuth Security Enhancement

[ShashankFC]

Test 4nn---nReplicated from ai-code-review-evaluation/sentry-greptile#4

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 2m 34s —— View job

---

Security Review Complete ✅

Tasks

• Read repository guidelines
• Review changed files
• Analyze OAuth security enhancements
• Check test coverage
• Provide comprehensive security review

---

Executive Summary

This PR implements critical OAuth security enhancements to prevent installation hijacking attacks in the GitHub integration. The changes add proper state validation and user authentication verification to the GitHub App installation flow.

Security Improvements

✅ OAuth State Parameter Validation (src/sentry/integrations/github/integration.py:412)

• Validates the state parameter matches the pipeline signature
• Prevents CSRF attacks during the OAuth flow
• Returns error on mismatch: "Invalid installation request"

✅ User Identity Verification (src/sentry/integrations/github/integration.py:500-505)

• Verifies the authenticated GitHub user matches the app installer
• Compares github_authenticated_user (from OAuth) with integration.metadata["sender"]["login"] (from webhook)
• Prevents unauthorized users from hijacking an installation

✅ Access Token Validation (src/sentry/integrations/github/integration.py:431-432)

• Validates GitHub returns a valid access_token from the authorization code
• Prevents code reuse attacks

Implementation Analysis

New OAuthLoginView Class (src/sentry/integrations/github/integration.py:389-439)

Security Controls:

1. State generation: Uses pipeline.signature as the OAuth state parameter (line 402)
2. State validation: Verifies returned state matches pipeline signature (line 412)
3. Token exchange: Properly exchanges authorization code for access token (line 423)
4. User verification: Fetches authenticated user info and stores in pipeline (line 438)

Potential Issues:

• ⚠️ Generic exception handling (line 428): except Exception: may silently swallow important errors. Consider logging or handling specific exceptions.

Updated GitHubInstallation Class (src/sentry/integrations/github/integration.py:442-507)

Security Enhancements:

1. Installation ID persistence: Properly manages installation_id across pipeline steps (line 448-449, 454)
2. User mismatch detection: Validates authenticated user against installer (line 501-505)
3. Integration status check: Verifies integration is ACTIVE before proceeding (line 494-498)

Code Quality:

• ✅ Refactored error handling into reusable error() function
• ✅ Centralized error messages as constants

Test Coverage Analysis (tests/sentry/integrations/github/test_integration.py)

New Test: test_github_user_mismatch (lines 399-455)

What it tests:

• Simulates an attacker scenario where GitHub user differs from installer
• Verifies the flow properly rejects mismatched users

Security Coverage:

• ✅ Tests the critical user validation logic
• ✅ Verifies error response is returned
• ✅ Confirms integration is not created

Updated Tests

• ✅ test_basic_flow: Updated to include OAuth step
• ✅ test_github_installed_on_another_org: Updated for new flow
• ✅ test_installation_not_found: Updated error message validation
• ✅ test_github_prevent_install_until_pending_deletion_is_complete: Updated for OAuth flow

Security Concerns & Recommendations

🔴 CRITICAL: State Reuse Vulnerability

Location: src/sentry/integrations/github/integration.py:402, 412

Issue: The pipeline signature is used as the OAuth state parameter, but there's no validation to ensure the state is single-use. An attacker could potentially:

1. Intercept a valid authorization response
2. Replay it multiple times if the pipeline allows state reuse

Recommendation:

# Add nonce validation to prevent replay attacks
if request.GET.get("state") != pipeline.signature:
    return error(request, self.active_organization)

# After successful validation, invalidate the state
pipeline.bind_state("oauth_state_used", True)

# Check if state was already used
if pipeline.fetch_state("oauth_state_used"):
    return error(request, self.active_organization, 
                 error_short="State already used",
                 error_long="This authorization request has already been processed.")

🟡 MEDIUM: Missing Authorization Code Validation

Location: src/sentry/integrations/github/integration.py:417-432

Issue: No validation of the authorization code format or basic sanity checks before sending to GitHub.

Recommendation:

code = request.GET.get("code")
if not code or len(code) < 20:  # GitHub codes are typically longer
    return error(request, self.active_organization)

🟡 MEDIUM: Silent Exception Handling

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

Issue:

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

Recommendation: Log the exception for security monitoring:

except Exception as e:
    logger.exception("Failed to exchange GitHub authorization code", extra={"error": str(e)})
    payload = {}

🟡 MEDIUM: Timing Attack Vulnerability

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

Issue: String comparison of usernames could leak information through timing:

if (
    pipeline.fetch_state("github_authenticated_user")
    != integration.metadata["sender"]["login"]
):

Recommendation: Use constant-time comparison:

import hmac

if not hmac.compare_digest(
    pipeline.fetch_state("github_authenticated_user") or "",
    integration.metadata.get("sender", {}).get("login", "")
):
    return error(request, self.active_organization)

🟢 LOW: Error Message Information Disclosure

Location: src/sentry/integrations/github/integration.py:116-118

Issue: Generic error messages are good, but could include more context for legitimate users while avoiding information disclosure to attackers.

Current:

ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST = _(
    "We could not verify the authenticity of the installation request. We recommend restarting the installation process."
)

Recommendation: Consider adding request ID for support purposes without revealing security details.

Attack Scenarios Mitigated

✅ CSRF Attacks: State parameter validation prevents cross-site request forgery
✅ Installation Hijacking: User verification prevents unauthorized installation claims
✅ Code Reuse: Token validation prevents authorization code replay
✅ Account Takeover: Multi-step verification reduces takeover risk

Attack Scenarios Not Fully Mitigated

⚠️ State Replay: Without single-use state validation, replay attacks remain possible
⚠️ Timing Attacks: Non-constant-time string comparison may leak information

Additional Security Considerations

1. Rate Limiting: Consider adding rate limiting to the OAuth endpoints to prevent brute force attacks
2. Audit Logging: Log all failed authentication attempts for security monitoring
3. Session Timeout: Ensure pipeline state expires after a reasonable time
4. HTTPS Enforcement: Verify all OAuth redirects use HTTPS (appears to be enforced via absolute_uri)

Test Recommendations

Consider adding tests for:

1. ✅ Already covered: User mismatch scenario
2. ⚠️ Missing: State replay/reuse scenario
3. ⚠️ Missing: Invalid authorization code format
4. ⚠️ Missing: Expired/missing pipeline state
5. ⚠️ Missing: Concurrent installation attempts
6. ⚠️ Missing: Race conditions in user validation

Code Quality

Strengths:

• Clear separation of concerns with OAuthLoginView and GitHubInstallation
• Consistent error handling through centralized error() function
• Good test coverage of main flows
• Proper use of Django patterns

Areas for Improvement:

• Exception handling could be more specific
• Consider extracting OAuth logic into a mixin for reusability
• Add type hints to new functions

Conclusion

This PR implements essential security controls that significantly improve the GitHub integration security posture. The OAuth state validation and user verification are critical protections against installation hijacking.

However, the state replay vulnerability should be addressed before merging to production to ensure the OAuth flow is fully secure against replay attacks.

Recommendation: ✅ Approve with required changes - Address the critical state reuse vulnerability, then merge.

---

Files Reviewed:

• src/sentry/integrations/github/integration.py - Core security logic ✅
• src/sentry/web/frontend/pipeline_advancer.py - Pipeline routing ✅
• tests/sentry/integrations/github/test_integration.py - Test coverage ✅

---