GitHub OAuth Security Enhancement #3
Conversation
…#67876) We're adding one more step in the GitHub integration installation pipeline, namely GitHub OAuth2 authorize. This is transparent from the UX perspective as the data exchange happens without user interaction. The pipeline will now fail in these cases: - If there is a mismatch between currently authenticated GitHub user (derived from OAuth2 authorize step) and the user who installed the GitHub app (https://github.com/apps/sentry-io) - If there is a mismatch between `state` parameter supplied by user and pipeline signature - If GitHub could not generate correct `access_token` from the `code` (wrong or attempt of re-use of `code`). In all those cases, this error is shown: 
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the security of GitHub OAuth integration by implementing user authentication verification to prevent unauthorized installation hijacking.
- Adds OAuth login step to verify the GitHub user's identity before proceeding with integration setup
- Implements user mismatch validation to ensure the authenticated user matches the app installer
- Refactors error handling with a unified error response function
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/sentry/integrations/github/test_integration.py | Adds comprehensive test coverage for OAuth flow, user mismatch scenarios, and updates existing tests to handle the new authentication step |
| src/sentry/web/frontend/pipeline_advancer.py | Removes the global FORWARD_INSTALL_FOR constant and inlines the GitHub-specific logic for better maintainability |
| src/sentry/integrations/github/integration.py | Implements new OAuthLoginView for user authentication, adds user validation in GitHubInstallation, and introduces unified error handling |
| ) | ||
|
|
||
| auth_header = responses.calls[0].request.headers["Authorization"] | ||
| auth_header = responses.calls[2].request.headers["Authorization"] |
There was a problem hiding this comment.
The hardcoded index responses.calls[2] creates a brittle test that depends on the exact number and order of HTTP calls. Consider using a more robust approach like filtering calls by URL or method, or using a helper method to find the specific call you need to verify.
| auth_header = responses.calls[2].request.headers["Authorization"] | |
| auth_call = next( | |
| call for call in responses.calls if "Authorization" in call.request.headers | |
| ) | |
| auth_header = auth_call.request.headers["Authorization"] |
| ) | ||
|
|
||
| # At this point, we are past the GitHub "authorize" step | ||
| if request.GET.get("state") != pipeline.signature: |
There was a problem hiding this comment.
The state parameter comparison should use a constant-time comparison to prevent timing attacks. Consider using secrets.compare_digest() or Django's constant_time_compare() function instead of the != operator.
| if request.GET.get("state") != pipeline.signature: | |
| from django.utils.crypto import constant_time_compare | |
| if not constant_time_compare(request.GET.get("state"), pipeline.signature): |
| except Exception: | ||
| payload = {} |
There was a problem hiding this comment.
Catching all exceptions with a bare Exception clause can mask important errors and make debugging difficult. Consider catching specific exceptions like UnicodeDecodeError, ValueError, or other expected exceptions that could occur during response parsing.
| except Exception: | |
| payload = {} | |
| except UnicodeDecodeError as e: | |
| logger.error("Failed to decode response body: %s", e) | |
| payload = {} | |
| except ValueError as e: | |
| logger.error("Failed to parse response body into a dictionary: %s", e) | |
| payload = {} |
| if ( | ||
| pipeline.fetch_state("github_authenticated_user") | ||
| != integration.metadata["sender"]["login"] |
There was a problem hiding this comment.
The user login comparison should use a constant-time comparison to prevent timing attacks that could leak information about valid usernames. Consider using secrets.compare_digest() or Django's constant_time_compare() function.
| if ( | |
| pipeline.fetch_state("github_authenticated_user") | |
| != integration.metadata["sender"]["login"] | |
| from secrets import compare_digest | |
| if not compare_digest( | |
| str(pipeline.fetch_state("github_authenticated_user")), | |
| str(integration.metadata["sender"]["login"]), |
Test 4