GitHub OAuth Security Enhancement#12
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: 
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes introduce OAuth user authorization into the GitHub integration flow via a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Integration as GitHub Integration
participant GitHubAPI as GitHub API
participant Pipeline as Pipeline State
rect rgb(220, 245, 220)
Note over User,Pipeline: New OAuth Login Step
User->>Integration: Start installation
Integration->>GitHubAPI: Redirect to GitHub OAuth
GitHubAPI->>User: Request user authorization
User->>GitHubAPI: Authorize app
GitHubAPI->>Integration: Redirect with code
Integration->>GitHubAPI: Exchange code for token
GitHubAPI->>Integration: Return access token
Integration->>GitHubAPI: Fetch authenticated user info
GitHubAPI->>Integration: Return user details
Integration->>Pipeline: Store github_authenticated_user
end
rect rgb(245, 230, 220)
Note over User,Pipeline: Installation Validation
User->>Integration: Complete installation
Integration->>Pipeline: Retrieve installation_id & sender
Integration->>Integration: Compare auth user vs sender
alt User mismatch
Integration->>User: Error: sender mismatch
else User valid
Integration->>Pipeline: Proceed to complete install
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/sentry/integrations/github/integration.py (1)
425-432: Overly broad exception handling masks potential errors.Catching all exceptions and setting an empty payload hides important error details that could help diagnose OAuth failures. Consider logging the exception or being more specific about which exceptions to catch.
Apply this diff to improve error handling:
+ import logging + logger = logging.getLogger(__name__) + try: body = safe_urlread(req).decode("utf-8") payload = dict(parse_qsl(body)) - except Exception: + except Exception as e: + logger.exception("Failed to parse OAuth token response") payload = {}
📜 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 (13)
src/sentry/web/frontend/pipeline_advancer.py (1)
37-48: LGTM - Cleaner direct comparison.The refactor from a constant list to a direct string comparison simplifies the code without changing behavior. This makes the GitHub-specific handling more explicit.
src/sentry/integrations/github/integration.py (9)
7-7: LGTM - Necessary OAuth imports.The new imports support the OAuth login flow, token exchange, and user information retrieval.
Also applies to: 10-10, 18-19, 42-42
116-118: LGTM - Clear error messaging.The error constant provides a user-friendly message for installation authenticity failures.
129-153: LGTM - Centralized error handling with customer domain support.These helper functions provide consistent error rendering and properly handle customer domain URLs for postMessage security.
401-409: LGTM - Proper OAuth state generation for CSRF protection.The state parameter is correctly generated from
pipeline.signatureand included in the authorization URL. This protects against CSRF attacks.
411-413: Excellent - State validation prevents CSRF attacks.The state validation ensures that the OAuth callback matches the original authorization request, which is critical for preventing CSRF attacks in the OAuth flow.
343-344: LGTM - OAuth login correctly added to pipeline.The OAuth login view is properly positioned before the installation view in the pipeline sequence.
448-454: LGTM - Flexible installation ID retrieval and persistence.The code correctly handles installation_id from either the request or pipeline state, ensuring it's available for subsequent pipeline steps.
423-423: Verify timeout configuration for OAuth token exchange.The token exchange HTTP request should have a timeout to prevent hanging if GitHub's OAuth endpoint is slow or unresponsive.
501-505: Excellent security enhancement - prevents installation hijacking.This validation ensures that the user completing the OAuth flow is the same user who initiated the GitHub App installation. This effectively prevents a scenario where User A installs the GitHub App and User B attempts to complete the installation in a different Sentry organization.
However, verify that
integration.metadata["sender"]is always present to prevent KeyError exceptions:tests/sentry/integrations/github/test_integration.py (3)
116-123: LGTM - Comprehensive OAuth endpoint stubs.The stubs correctly simulate GitHub's OAuth token endpoint (form-urlencoded response) and user info endpoint, enabling proper testing of the OAuth flow.
226-261: LGTM - Comprehensive OAuth flow test coverage.The updated test properly validates the complete OAuth flow including:
- Initial redirect to GitHub OAuth authorization
- OAuth callback with code and state parameters
- Subsequent redirect to GitHub App installation
398-455: Excellent security test - validates user mismatch detection.This test comprehensively validates the security enhancement by:
- Simulating a GitHub App installation with sender="attacker" via webhook
- Having a different user authenticate via OAuth (returns "octocat")
- Verifying that the mismatch is detected and the installation is blocked
This directly tests the critical security check that prevents installation hijacking across organizations.
| # similar to OAuth2CallbackView.get_token_params | ||
| data = { | ||
| "code": request.GET.get("code"), | ||
| "client_id": github_client_id, | ||
| "client_secret": github_client_secret, | ||
| } | ||
|
|
||
| # similar to OAuth2CallbackView.exchange_token | ||
| req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data) |
There was a problem hiding this comment.
Missing redirect_uri in token exchange violates OAuth 2.0 specification.
According to the OAuth 2.0 specification (RFC 6749 Section 4.1.3), if redirect_uri was included in the authorization request (line 404-408), it must also be included in the token exchange request for security verification. GitHub's OAuth implementation may be lenient, but this violates the spec and could cause issues.
Apply this diff to include the redirect_uri:
# similar to OAuth2CallbackView.get_token_params
+ redirect_uri = absolute_uri(
+ reverse("sentry-extension-setup", kwargs={"provider_id": "github"})
+ )
data = {
"code": request.GET.get("code"),
"client_id": github_client_id,
"client_secret": github_client_secret,
+ "redirect_uri": redirect_uri,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # similar to OAuth2CallbackView.get_token_params | |
| data = { | |
| "code": request.GET.get("code"), | |
| "client_id": github_client_id, | |
| "client_secret": github_client_secret, | |
| } | |
| # similar to OAuth2CallbackView.exchange_token | |
| req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data) | |
| # similar to OAuth2CallbackView.get_token_params | |
| redirect_uri = absolute_uri( | |
| reverse("sentry-extension-setup", kwargs={"provider_id": "github"}) | |
| ) | |
| data = { | |
| "code": request.GET.get("code"), | |
| "client_id": github_client_id, | |
| "client_secret": github_client_secret, | |
| "redirect_uri": redirect_uri, | |
| } | |
| # similar to OAuth2CallbackView.exchange_token | |
| req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data) |
🤖 Prompt for AI Agents
In src/sentry/integrations/github/integration.py around lines 415 to 423, the
token exchange data dict omits redirect_uri which was sent in the authorization
request (lines ~404-408); add the same redirect_uri value to the data payload
used in safe_urlopen so the token request includes "redirect_uri": redirect_uri
(i.e., reuse the exact redirect_uri variable/build from the earlier auth
request), ensuring the token exchange matches the original authorization request
per RFC 6749.
2 participants
Test 4
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Replicated from ai-code-review-evaluation/sentry-coderabbit#4