GitHub OAuth Security Enhancement#4
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: 
WalkthroughA new OAuth login step is introduced to the GitHub integration pipeline, adding explicit user authentication and validation. The integration flow is updated with centralized error handling, improved installation checks, and stricter user matching. Related tests are expanded to cover OAuth flows, error cases, and user mismatch scenarios. Minor simplification is made to pipeline advancer logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Sentry
participant GitHub
User->>Sentry: Start GitHub integration
Sentry->>User: Redirect to GitHub OAuth authorize
User->>GitHub: Authorize app
GitHub->>User: Redirect back with code & state
User->>Sentry: Callback with code & state
Sentry->>GitHub: Exchange code for access token
GitHub->>Sentry: Return access token
Sentry->>GitHub: Fetch user info
GitHub->>Sentry: Return user login
Sentry->>User: Redirect to GitHub App installation
User->>GitHub: Install app
GitHub->>Sentry: Send installation event (with sender login)
Sentry->>Sentry: Validate sender login matches OAuth user
alt Success
Sentry->>User: Complete integration
else Failure
Sentry->>User: Render error page
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/sentry/integrations/github/integration.py (1)
493-498: Consider handling inactive integrations.The query filters for
ObjectStatus.ACTIVEintegrations, but if an integration exists in a different status, it will raiseIntegration.DoesNotExistand show a generic error. Consider checking for integrations in any status and providing a more specific error message.- try: - integration = Integration.objects.get( - external_id=installation_id, status=ObjectStatus.ACTIVE - ) - except Integration.DoesNotExist: - return error(request, self.active_organization) + try: + integration = Integration.objects.get(external_id=installation_id) + if integration.status != ObjectStatus.ACTIVE: + return error( + request, + self.active_organization, + error_short="GitHub integration is not active.", + error_long="The GitHub integration exists but is not in an active state. Please contact support if this issue persists." + ) + except Integration.DoesNotExist: + return error(request, self.active_organization)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sentry/integrations/github/integration.py(7 hunks)src/sentry/web/frontend/pipeline_advancer.py(1 hunks)tests/sentry/integrations/github/test_integration.py(9 hunks)
🔇 Additional comments (15)
src/sentry/web/frontend/pipeline_advancer.py (1)
37-41: Good simplification of the GitHub provider check.The refactoring from list membership check to direct string comparison is appropriate since the list only contained a single element. Moving the comment closer to the relevant code also improves readability.
tests/sentry/integrations/github/test_integration.py (7)
15-15: LGTM!The import is correctly added to support the new test case for GitHub user mismatch validation.
116-124: Well-structured OAuth endpoint mocking.The OAuth token exchange and user info endpoints are properly mocked with appropriate test data. The hardcoded values are suitable for test fixtures.
232-250: Comprehensive OAuth flow testing.The updated
assert_setup_flowmethod properly tests the complete OAuth authorization flow, including URL validation, parameter checking, and callback simulation. The flow correctly follows the OAuth 2.0 authorization code grant pattern.
335-343: Correctly updated to include OAuth flow.The test properly simulates the OAuth callback as part of the installation flow when testing duplicate installations across organizations.
386-397: Good coverage of error handling in OAuth flow.The test correctly verifies that installation validation occurs after OAuth authentication, ensuring proper error messages are displayed for invalid installations.
399-455: Excellent security test coverage.The
test_github_user_mismatchmethod thoroughly validates the critical security check that prevents unauthorized users from completing GitHub app installations. The test properly simulates the attack scenario and verifies the appropriate error response.
701-741: Consistent OAuth flow testing in edge cases.The pending deletion test properly includes OAuth callback simulations both during the error case and after successful deletion, ensuring comprehensive coverage of the OAuth flow in all scenarios.
src/sentry/integrations/github/integration.py (7)
7-7: LGTM!The imports are appropriately added to support OAuth functionality and improved error handling.
Also applies to: 10-10, 18-19, 42-42
116-118: Clear and helpful error message.The error message appropriately informs users about authentication verification failures and provides actionable guidance.
129-153: Well-designed error handling utilities.The
error()function provides a centralized way to render error pages with consistent messaging, whileget_document_origin()properly handles the customer domains feature flag. These utilities improve code maintainability and user experience.
344-344: Correct pipeline view ordering.Adding
OAuthLoginViewbeforeGitHubInstallationensures proper OAuth authentication flow.
389-441: Secure and well-implemented OAuth flow.The
OAuthLoginViewclass properly implements the OAuth 2.0 authorization code flow with:
- CSRF protection via state parameter validation
- Secure token exchange
- Proper error handling at each step
- User authentication verification
The implementation follows security best practices.
448-455: Good flexibility in installation ID retrieval.The code properly handles installation ID from both direct requests and pipeline state, supporting different entry points into the integration flow.
500-506: Critical security validation.Excellent implementation of user verification that ensures the authenticated GitHub user matches the installation sender. This prevents unauthorized users from hijacking GitHub app installations.
2 participants
Test 4
Summary by CodeRabbit
New Features
Bug Fixes
Tests