GitHub OAuth Security Enhancement#9
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 |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR introduces OAuth-based GitHub login flow for integration setup, adds standardized error handling helpers, validates GitHub user-installation matching, and simplifies pipeline advancer logic by removing an unused forwarding constant. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Request
participant OAuthView as OAuthLoginView
participant GitHub as GitHub OAuth
participant Provider as GitHubIdentityProvider
participant Integration as Integration Store
participant Response as Error/Success Handler
Client->>OAuthView: GET with code & state
OAuthView->>OAuthView: Validate state parameter
OAuthView->>GitHub: Exchange code for token
GitHub-->>OAuthView: Access token
OAuthView->>Provider: Get user info with token
Provider->>GitHub: Fetch authenticated user
GitHub-->>Provider: User data
Provider-->>OAuthView: GitHub user info
OAuthView->>Integration: Retrieve stored installation
alt User matches installation sender
Integration-->>OAuthView: Installation found & valid
OAuthView->>Response: Return success
else User mismatch or installation invalid
Integration-->>OAuthView: User/Installation mismatch
OAuthView->>Response: Render error(request, org)
end
Response-->>Client: HTTP Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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
🤖 Fix all issues with AI Agents
In @src/sentry/integrations/github/integration.py:
- Around line 500-505: The code assumes integration.metadata["sender"]["login"]
exists and can raise KeyError; update the check in the block that compares
pipeline.fetch_state("github_authenticated_user") so it first verifies "sender"
is present in integration.metadata and that integration.metadata["sender"] has a
"login" key (or otherwise handle missing sender by returning error(request,
self.active_organization) or skipping the comparison). Concretely, guard the
access to integration.metadata["sender"]["login"] using an if "sender" not in
integration.metadata or "login" not in integration.metadata -> return
error(...), then perform the existing comparison with
pipeline.fetch_state("github_authenticated_user").
🧹 Nitpick comments (2)
src/sentry/integrations/github/integration.py (2)
404-409: URL parameters should be properly encoded.The
redirect_uriis appended to the URL without URL encoding. Whileabsolute_uritypically produces safe URLs, special characters could cause issues. Consider usingurlencodefor query parameter construction.🔎 Proposed fix
+from urllib.parse import urlencode + redirect_uri = absolute_uri( reverse("sentry-extension-setup", kwargs={"provider_id": "github"}) ) +params = urlencode({ + "client_id": github_client_id, + "state": state, + "redirect_uri": redirect_uri, +}) return self.redirect( - f"{ghip.get_oauth_authorize_url()}?client_id={github_client_id}&state={state}&redirect_uri={redirect_uri}" + f"{ghip.get_oauth_authorize_url()}?{params}" )
425-429: Consider catching a more specific exception.Catching a bare
Exceptionis flagged by static analysis (BLE001). Since the failure modes here are parsing issues fromsafe_urlreadorparse_qsl, consider catching more specific exceptions likeUnicodeDecodeErrorandValueError, or add a comment explaining why the broad catch is necessary.🔎 Proposed fix
try: body = safe_urlread(req).decode("utf-8") payload = dict(parse_qsl(body)) -except Exception: +except (UnicodeDecodeError, ValueError, OSError): + # Handle malformed responses from GitHub OAuth endpoint payload = {}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sentry/integrations/github/integration.pysrc/sentry/web/frontend/pipeline_advancer.pytests/sentry/integrations/github/test_integration.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/sentry/integrations/github/test_integration.py (1)
tests/sentry/web/frontend/test_auth_saml2.py (1)
setup_path(90-91)
🪛 Ruff (0.14.10)
tests/sentry/integrations/github/test_integration.py
116-116: Possible hardcoded password assigned to: "access_token"
(S105)
src/sentry/integrations/github/integration.py
428-428: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (7)
src/sentry/web/frontend/pipeline_advancer.py (1)
37-48: LGTM! Simplification looks good.Replacing the list membership check with a direct string comparison is appropriate since GitHub was the only provider in that list. The added comments provide helpful context about when this code path is triggered.
src/sentry/integrations/github/integration.py (2)
129-152: LGTM! Good consolidation of error handling logic.The
error()helper centralizes the error rendering pattern, andget_document_origin()properly handles the customer domains feature flag for cross-origin postMessage.
343-344: Good addition of OAuth validation step to the pipeline.Adding
OAuthLoginView()beforeGitHubInstallation()ensures the GitHub user is authenticated before the installation is validated, which is the core security enhancement.tests/sentry/integrations/github/test_integration.py (4)
116-123: LGTM! OAuth endpoint stubs are correctly configured.The mock access token and user endpoint responses properly simulate the GitHub OAuth flow for testing. The static analysis warning about hardcoded passwords is a false positive for test fixtures.
226-261: LGTM! Test properly updated to verify the new OAuth flow.The assertions correctly validate:
- Initial redirect to GitHub OAuth authorize
- State parameter handling
- Code exchange callback
- Final redirect to GitHub app installation
398-454: Excellent test coverage for the security fix.This test properly validates the core security enhancement by simulating a scenario where:
- An attacker (
sender.login = "attacker") installs the GitHub app- A different user (
octocatvia OAuth) attempts to proceed with the installation- The mismatch is correctly detected and an error is returned
This covers the main attack vector that the PR aims to prevent.
335-376: LGTM! Test properly updated for multi-org installation scenario.The
setup_path_2correctly simulates the OAuth flow for the second organization's installation attempt, and the test validates both the error case and successful retry after deletion.
| # Check that the authenticated GitHub user is the same as who installed the app. | ||
| if ( | ||
| pipeline.fetch_state("github_authenticated_user") | ||
| != integration.metadata["sender"]["login"] | ||
| ): | ||
| return error(request, self.active_organization) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for how sender is populated in integration metadata
rg -n "sender" --type=py -C3 src/sentry/integrations/github/Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 3290
🏁 Script executed:
# Look at the build_integration function around line 376-377
rg -n "build_integration" --type=py -C10 src/sentry/integrations/github/ | head -100Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 3789
🏁 Script executed:
# Check how metadata is accessed elsewhere in the GitHub integration
rg -n "metadata\[" --type=py src/sentry/integrations/github/ | head -20Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 889
🏁 Script executed:
# Look for initialization or default values for sender in metadata
rg -n '"sender"' --type=py src/sentry/integrations/github/ -B2 -A2Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 2306
Potential KeyError if sender is missing from integration metadata.
The code at line 503 directly accesses integration.metadata["sender"]["login"] without checking if the sender key exists. Since build_integration() conditionally sets sender only if present in state (if state.get("sender"): at line 376), integrations created without a sender in their initial state will lack this metadata, causing a KeyError.
This risk is already recognized elsewhere in the codebase—installation.py line 44 includes a defensive check (if "sender" not in integration.metadata) before accessing sender. The same guard is needed here.
Proposed fix
# Check that the authenticated GitHub user is the same as who installed the app.
-if (
- pipeline.fetch_state("github_authenticated_user")
- != integration.metadata["sender"]["login"]
-):
- return error(request, self.active_organization)
+sender = integration.metadata.get("sender")
+if not sender or pipeline.fetch_state("github_authenticated_user") != sender.get("login"):
+ return error(request, self.active_organization)📝 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.
| # Check that the authenticated GitHub user is the same as who installed the app. | |
| if ( | |
| pipeline.fetch_state("github_authenticated_user") | |
| != integration.metadata["sender"]["login"] | |
| ): | |
| return error(request, self.active_organization) | |
| # Check that the authenticated GitHub user is the same as who installed the app. | |
| sender = integration.metadata.get("sender") | |
| if not sender or pipeline.fetch_state("github_authenticated_user") != sender.get("login"): | |
| return error(request, self.active_organization) |
🤖 Prompt for AI Agents
In @src/sentry/integrations/github/integration.py around lines 500 - 505, The
code assumes integration.metadata["sender"]["login"] exists and can raise
KeyError; update the check in the block that compares
pipeline.fetch_state("github_authenticated_user") so it first verifies "sender"
is present in integration.metadata and that integration.metadata["sender"] has a
"login" key (or otherwise handle missing sender by returning error(request,
self.active_organization) or skipping the comparison). Concretely, guard the
access to integration.metadata["sender"]["login"] using an if "sender" not in
integration.metadata or "login" not in integration.metadata -> return
error(...), then perform the existing comparison with
pipeline.fetch_state("github_authenticated_user").
2 participants
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