GitHub OAuth Security Enhancement#17
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 GitHub OAuth integration security by implementing an OAuth-based login flow to validate installation requests and prevent unauthorized installations. The changes ensure that the GitHub user initiating the installation matches the user who authenticated, preventing potential security vulnerabilities where an attacker could hijack an installation process.
Changes:
- Added OAuth authentication step before GitHub app installation
- Implemented validation to verify the authenticated GitHub user matches the installation sender
- Consolidated error handling into reusable helper functions
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/sentry/integrations/github/integration.py | Added OAuth login view, user validation logic, and refactored error handling |
| src/sentry/web/frontend/pipeline_advancer.py | Simplified forward install check from list to direct comparison |
| tests/sentry/integrations/github/test_integration.py | Added comprehensive test coverage for OAuth flow and user mismatch scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return error( | ||
| request, | ||
| self.active_organization, | ||
| error_short="Github installed on another Sentry organization.", |
There was a problem hiding this comment.
Corrected capitalization of 'Github' to 'GitHub' to match the official brand name.
| error_short="Github installed on another Sentry organization.", | |
| error_short="GitHub installed on another Sentry organization.", |
| try: | ||
| body = safe_urlread(req).decode("utf-8") | ||
| payload = dict(parse_qsl(body)) | ||
| except Exception: |
There was a problem hiding this comment.
The bare except Exception catches all exceptions without logging or providing specific error context. Consider logging the exception details before returning the generic error to aid in debugging OAuth token exchange failures.
| try: | |
| body = safe_urlread(req).decode("utf-8") | |
| payload = dict(parse_qsl(body)) | |
| except Exception: | |
| body = "" | |
| try: | |
| body = safe_urlread(req).decode("utf-8") | |
| payload = dict(parse_qsl(body)) | |
| except Exception as exc: | |
| logger.exception( | |
| "Failed to parse GitHub OAuth token exchange response", | |
| extra={"body": body}, | |
| ) |
| if ( | ||
| pipeline.fetch_state("github_authenticated_user") | ||
| != integration.metadata["sender"]["login"] | ||
| ): |
There was a problem hiding this comment.
The metadata access integration.metadata[\"sender\"][\"login\"] could raise a KeyError if the expected structure is missing. Add validation to check for the existence of these nested keys before accessing them to prevent potential crashes and improve error handling.
| if ( | |
| pipeline.fetch_state("github_authenticated_user") | |
| != integration.metadata["sender"]["login"] | |
| ): | |
| metadata = integration.metadata or {} | |
| sender_login: str | None = None | |
| if isinstance(metadata, Mapping): | |
| sender = metadata.get("sender") | |
| if isinstance(sender, Mapping): | |
| sender_login = sender.get("login") | |
| if pipeline.fetch_state("github_authenticated_user") != sender_login: |
| ) | ||
|
|
||
| auth_header = responses.calls[0].request.headers["Authorization"] | ||
| auth_header = responses.calls[2].request.headers["Authorization"] |
There was a problem hiding this comment.
Using a magic number index responses.calls[2] makes the test fragile. If the number or order of API calls changes, this test will break unexpectedly. Consider using a more explicit approach, such as filtering calls by URL or adding a descriptive comment explaining why this is the third call.
Test 4
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/sentry-coderabbit#4