GitHub OAuth Security Enhancement#7
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: 
🤖 Grapple PR ReviewSummaryThis PR adds an OAuth login step to the GitHub integration pipeline, introducing state-based CSRF protection before the GitHub App installation flow. It replaces a direct redirect to GitHub App installation with a proper OAuth authorization flow that validates a state parameter to verify request authenticity. Changes: 3 files (+247 / -50 lines)
Resolves: #4 (feat(ecosystem): Implement cross-system issue synchronization) CI: ⏳ 0 passed, 0 failed Impact Diagramgraph TD
pr_0[/"🔀 PR Changes: 3 files"/]
f1["🟡 integration.py +118/-41"]
pr_0 --> f1
f2["🟡 pipeline_advancer.py +4/-7"]
pr_0 --> f2
f3["🟡 test_integration.py +125/-2"]
pr_0 --> f3
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
Review details
Powered by Grapple PR — AI that understands your code. |
|
CRITICAL — The OAuth state parameter used for CSRF protection is derived from |
|
CRITICAL — The GitHub user mismatch check can raise an unhandled |
|
CRITICAL — When the state parameter is missing from the callback AND no installation_id is in the request, the OAuthLoginView generates a new redirect to GitHub OAuth. But if a user arrives at OAuthLoginView on the second pipeline step (after the OAuth redirect) with a valid state and code, the installation_id won't be in request.GET (it was only in the initial request). The code attempts to preserve installation_id via pipeline.bind_state on line 401, but only if installation_id is present in the current request's GET parameters. On the OAuth callback (the second hit to this view), installation_id will NOT be in request.GET — it was consumed in the first request. This means the bind_state on line 400-401 only works on the first request, which is correct. However, the flow works because installation_id is already bound from the first request. This is actually fine upon closer inspection. |
|
CRITICAL — The OAuth token exchange does not validate the HTTP response status code before attempting to parse the body. If GitHub returns a non-200 response (e.g., 401, 403, 500), the code will still attempt to parse the body. While the subsequent 'access_token not in payload' check provides a safety net, the error handling is fragile — a non-200 response with an 'access_token' key in the body (unlikely but possible with error responses) could pass through. More importantly, if safe_urlopen raises an exception (network timeout, connection error), it is not caught, and the pipeline will return a 500 error instead of the friendly error page. |
|
CRITICAL — OAuthLoginView uses |
|
MAJOR — The OAuth redirect URL is constructed via f-string interpolation without URL-encoding the |
|
MAJOR — The OAuth token exchange posts |
|
MAJOR — get_document_origin() accesses org.organization without a None check on the result. If org is not None but org.organization is None or doesn't exist (e.g., a stale/deleted organization), this will raise an AttributeError. The intent specification explicitly calls out this risk. |
|
MAJOR — The state parameter (pipeline.signature) is compared but never consumed/invalidated after successful validation. This means the same state can be replayed multiple times — an attacker who captures the OAuth callback URL can replay it. The intent specification's edge cases explicitly ask whether state should be single-use. Using pipeline.signature as the state means it remains valid for the lifetime of the pipeline, enabling replay attacks within that window. |
|
MAJOR — The new GitHubInstallation logic queries for an Integration with ACTIVE status (line 475) and then compares github_authenticated_user against integration.metadata['sender']['login']. However, if the integration exists but is NOT active (e.g., disabled), the Integration.DoesNotExist exception is caught and error() is returned. Previously, this path would call pipeline.next_step() to proceed with the build. This changes the behavior: an inactive integration that exists but isn't ACTIVE will now show an error instead of proceeding with the installation. This could break re-installation flows where the integration was disabled but not deleted. |
![grapple-pr[bot]](https://avatars.githubusercontent.com/in/3205222?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds an OAuth 2.0 authorization flow (OAuthLoginView) to the GitHub integration pipeline, requiring a state parameter handshake before proceeding to app installation. This prevents CSRF attacks on the GitHub OAuth callback by verifying that the incoming state token matches what was originally issued.
Changes: 3 files (+247 / -50 lines)
- 🟡 3 files modified:
src/sentry/integrations/github/integration.py,src/sentry/web/frontend/pipeline_advancer.py,tests/sentry/integrations/github/test_integration.py
Resolves: #4 (feat(ecosystem): Implement cross-system issue synchronization)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 3 files"/]
f1["🟡 integration.py +118/-41"]
pr_0 --> f1
f2["🟡 pipeline_advancer.py +4/-7"]
pr_0 --> f2
f3["🟡 test_integration.py +125/-2"]
pr_0 --> f3
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
1 critical issue found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 Major | 4 |
| 🟡 Minor | 8 |
| 🔵 Info | 3 |
Review details
- Reviewed at: 2026-04-07T21:08:26.125Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 84.2s
- Low-confidence findings suppressed: 1
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| if installation_id: | ||
| pipeline.bind_state("installation_id", installation_id) | ||
|
|
||
| if not request.GET.get("state"): |
There was a problem hiding this comment.
🔴 CRITICAL — Security - State Validation (confidence: 93%)
The state parameter is compared using != (standard string comparison) rather than a constant-time comparison function. This makes the state validation vulnerable to timing attacks, where an attacker could incrementally guess the state value by measuring response times. For CSRF protection tokens, constant-time comparison is a security best practice.
Evidence:
- Line 401:
if request.GET.get('state') != pipeline.signature: - The intent specification explicitly calls out: 'missing or incorrect comparison (e.g., using = instead of constant-time comparison)' as a risk area
- Python's
hmac.compare_digest()should be used for security-sensitive string comparisons
Agent: architecture
| return self.redirect( | ||
| f"{ghip.get_oauth_authorize_url()}?client_id={github_client_id}&state={state}&redirect_uri={redirect_uri}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
🟠 MAJOR — Security - Missing Error Handling (confidence: 100%)
The OAuth callback does not handle the case where GitHub returns an error instead of a code (e.g., ?error=access_denied&state=...). When the user denies authorization, GitHub redirects back with an error parameter but no code. The current code would pass None as the code value to the token exchange, which will fail, but the error message would be generic rather than informative.
Evidence:
- GitHub OAuth docs specify that if the user denies access, the callback receives
?error=access_denied&error_description=...&state=... - Line 407:
request.GET.get('code')would return None if no code parameter exists - Intent specification edge case: 'GitHub returning an error instead of a code in the OAuth callback (e.g., ?error=access_denied) must be handled gracefully'
- The error parameter from GitHub is never checked
Agent: architecture
| integration=Integration.objects.get(external_id=request.GET["installation_id"]) | ||
| integration=Integration.objects.get(external_id=installation_id) | ||
| ).exists() | ||
|
|
There was a problem hiding this comment.
🟠 MAJOR — API Contract / Behavioral Change (confidence: 100%)
The GitHubInstallation step now performs an authenticated user match (github_authenticated_user != integration.metadata['sender']['login']) for existing integrations. This is a new behavioral constraint that was not previously enforced. If integration.metadata does not contain a sender key or sender lacks a login key (e.g., for integrations created before this metadata was stored), this will raise a KeyError and cause a 500 error. There's no defensive access pattern here.
Evidence:
- Line 477-480: Direct dictionary access
integration.metadata['sender']['login']with no.get()or try/except - Existing integrations may have been created with different metadata schemas
- The webhook handler that populates metadata may not always include
senderinformation
Agent: architecture
| payload = dict(parse_qsl(body)) | ||
| except Exception: | ||
| payload = {} | ||
|
|
There was a problem hiding this comment.
🟠 MAJOR — Module Boundaries - Separation of Concerns (confidence: 100%)
OAuthLoginView directly imports and uses GitHubIdentityProvider, safe_urlopen, safe_urlread, and manually implements the OAuth token exchange. This duplicates logic that already exists in OAuth2CallbackView and the identity provider pipeline. The code comments even acknowledge this ('similar to OAuth2CallbackView.get_token_params', 'similar to OAuth2CallbackView.exchange_token'). This creates a maintenance burden where OAuth flow changes need to be synchronized across two implementations.
Evidence:
- Line 406: Comment '# similar to OAuth2CallbackView.get_token_params'
- Line 409: Comment '# similar to OAuth2CallbackView.exchange_token'
- Direct HTTP calls via safe_urlopen instead of reusing existing OAuth infrastructure
- The identity pipeline already has OAuth2LoginView and OAuth2CallbackView that handle these flows
Agent: architecture
| ) | ||
|
|
||
|
|
||
| class OAuthLoginView(PipelineView): |
There was a problem hiding this comment.
🟠 MAJOR — Security - Credential Exposure (confidence: 84%)
The github_client_secret is retrieved at the top of the dispatch method (line 389) even when it's only needed during the token exchange phase (when state is present in the request). While not a direct vulnerability, this is a pattern concern — secrets should be fetched as late as possible and only when needed, to minimize their time in memory and reduce the blast radius of any memory disclosure vulnerabilities.
Evidence:
- Line 389:
github_client_secret = ghip.get_oauth_client_secret()is called unconditionally - The secret is only used at line 410 in the
datadict for token exchange - On the initial redirect path (no state), the secret is fetched but never used
Agent: architecture
|
|
||
| def get_document_origin(org) -> str: | ||
| if org and features.has("organizations:customer-domains", org.organization): | ||
| return f'"{generate_organization_url(org.organization.slug)}"' |
There was a problem hiding this comment.
🟡 MINOR — Naming conventions (confidence: 83%)
Function parameter error_short in the error() helper uses underscore naming style, but the function immediately wraps it with _() i18n function, making the parameter name misleading about its nature.
Evidence:
- Parameter is named
error_shortbut is not actually a short error message - it's an untranslated string key that will be translated - Compare to
error_longparameter which is passed a translated_()message directly - The inconsistency between the two error parameters (one raw, one translated) could confuse maintainers
Agent: style
| assert redirect.path == "/login/oauth/authorize" | ||
| assert ( | ||
| redirect.query | ||
| == "client_id=github-client-id&state=9cae5e88803f35ed7970fc131e6e65d3&redirect_uri=http://testserver/extensions/github/setup/" |
There was a problem hiding this comment.
🟡 MINOR — Code patterns (confidence: 75%)
Hardcoded state value '9cae5e88803f35ed7970fc131e6e65d3' appears in test assertions. If state generation is mocked differently elsewhere or the test setup changes, these hardcoded values will cause brittle test failures.
Evidence:
- The state is hardcoded in assert_setup_flow() at line 235
- The same state value is repeated in test_github_installed_on_another_org at line 337
- The test assumes pipeline.signature will return this specific value, but there's no visible mock setup controlling this
Agent: style
| return error(request, self.active_organization) | ||
|
|
||
| pipeline.bind_state("github_authenticated_user", authenticated_user_info["login"]) | ||
| return pipeline.next_step() |
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 96%)
OAuthLoginView.dispatch() method is complex with multiple control flow paths (state generation, token exchange, user info fetch) but lacks docstring explaining the CSRF state flow and security properties.
Evidence:
- The method handles three distinct phases: initial redirect, callback with state validation, and token exchange
- The state parameter is critical for CSRF protection but no inline comment explains why it's used or how it's stored/validated
- The relationship between pipeline.signature (used as state) and security is undocumented
Agent: style
|
|
||
|
|
||
| # Github App docs and list of available endpoints | ||
| # https://docs.github.com/en/rest/apps/installations |
There was a problem hiding this comment.
🔵 INFO — Code organization (confidence: 89%)
get_document_origin() helper function uses positional org parameter but does not validate the expected object structure. The code assumes org has an .organization attribute but this is not documented.
Evidence:
- Line 155:
org.organization.slugis accessed without type hints or comments explaining org's expected type - The acceptance criteria mentions guarding against AttributeError when org is None or lacks .organization attribute
- No docstring documents the expected structure of the
orgparameter
Agent: style
| if ( | ||
| provider_id in FORWARD_INSTALL_FOR | ||
| provider_id == "github" | ||
| and request.GET.get("setup_action") == "install" |
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 81%)
The comment explaining the special GitHub handling was moved from a module-level docstring into an inline comment within control flow. This reduces discoverability for developers reading the code.
Evidence:
- Previously the FORWARD_INSTALL_FOR list had a clear comment above it explaining the rationale
- The comment is now embedded in the conditional logic where it's less visible
- This is a common refactoring pattern but worth documenting why it was changed
Agent: style
| req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data) | ||
|
|
||
| try: | ||
| body = safe_urlread(req).decode("utf-8") |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (5 lines, 1 file))
Missing error handling for GitHub returning an error response in the OAuth callback. The code only checks for 'code' parameter but does not check for 'error' parameter that GitHub may return if the user denies authorization.
| body = safe_urlread(req).decode("utf-8") | |
| # GitHub returns ?error=access_denied (or other errors) if the user denies | |
| # authorization or if something goes wrong on GitHub's side. | |
| if request.GET.get("error"): | |
| return error(request, self.active_organization) | |
🤖 Grapple PR auto-fix • minor • confidence: 95%
| installation_id = request.GET.get("installation_id") | ||
| if installation_id: | ||
| pipeline.bind_state("installation_id", installation_id) | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (1 lines, 1 file))
The OAuth authorize URL is constructed via string concatenation with f-string rather than using
urllib.parse.urlencodefor query parameters. While the values here are controlled (client_id, state, redirect_uri), this pattern is fragile and could break if any value contains special characters (particularly the redirect_uri which contains URL characters).
| from urllib.parse import parse_qsl, urlencode |
🤖 Grapple PR auto-fix • minor • confidence: 93%
| if installation_id: | ||
| pipeline.bind_state("installation_id", installation_id) | ||
|
|
||
| if not request.GET.get("state"): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (9 lines, 1 file))
The state parameter is compared using
!=(standard string comparison) rather than a constant-time comparison function. This makes the state validation vulnerable to timing attacks, where an attacker could incrementally guess the state value by measuring response times. For CSRF protection tokens, constant-time comparison is a security best practice.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -1,6 +1,7 @@
from __future__ import annotations
+import hmac
import logging
import re
from collections.abc import Collection, Mapping, Sequence
@@ -408,7 +409,10 @@ class OAuthLoginView(PipelineView):
# At this point, we are past the GitHub "authorize" step
- if request.GET.get("state") != pipeline.signature:
+ # Use constant-time comparison to prevent timing attacks on the CSRF state token.
+ # Standard string comparison (!=) short-circuits on the first differing byte,
+ # leaking timing information that could allow an attacker to guess the state value
+ # incrementally. hmac.compare_digest() always takes the same amount of time.
+ if not hmac.compare_digest(
+ request.GET.get("state") or "", pipeline.signature or ""
+ ):
return error(request, self.active_organization)🤖 Grapple PR auto-fix • critical • Review this diff before applying
| return self.redirect( | ||
| f"{ghip.get_oauth_authorize_url()}?client_id={github_client_id}&state={state}&redirect_uri={redirect_uri}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (12 lines, 1 file) — review recommended)
The OAuth callback does not handle the case where GitHub returns an error instead of a code (e.g.,
?error=access_denied&state=...). When the user denies authorization, GitHub redirects back with anerrorparameter but nocode. The current code would passNoneas the code value to the token exchange, which will fail, but the error message would be generic rather than informative.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -408,6 +408,16 @@ class OAuthLoginView(PipelineView):
# At this point, we are past the GitHub "authorize" step
if request.GET.get("state") != pipeline.signature:
return error(request, self.active_organization)
+ # Handle the case where GitHub returned an error instead of a code.
+ # Per GitHub OAuth docs, if the user denies authorization, GitHub redirects
+ # back with ?error=access_denied&error_description=...&state=... and no code.
+ github_error = request.GET.get("error")
+ if github_error:
+ error_description = request.GET.get("error_description", github_error)
+ logger.info(
+ "github.oauth.denied",
+ extra={"error": github_error, "error_description": error_description},
+ )
+ return error(request, self.active_organization)
+
# similar to OAuth2CallbackView.get_token_params
data = {
"code": request.GET.get("code"),🤖 Grapple PR auto-fix • major • Review this diff before applying
| integration=Integration.objects.get(external_id=request.GET["installation_id"]) | ||
| integration=Integration.objects.get(external_id=installation_id) | ||
| ).exists() | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (17 lines, 1 file) — review recommended)
The GitHubInstallation step now performs an authenticated user match (
github_authenticated_user != integration.metadata['sender']['login']) for existing integrations. This is a new behavioral constraint that was not previously enforced. Ifintegration.metadatadoes not contain asenderkey orsenderlacks aloginkey (e.g., for integrations created before this metadata was stored), this will raise aKeyErrorand cause a 500 error. There's no defensive access pattern here.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -495,10 +495,21 @@
# OrganizationIntegration does not exist, but Integration does exist.
try:
integration = Integration.objects.get(
external_id=installation_id, status=ObjectStatus.ACTIVE
)
except Integration.DoesNotExist:
return error(request, self.active_organization)
# Check that the authenticated GitHub user is the same as who installed the app.
+ sender_login = (integration.metadata or {}).get("sender", {}).get("login")
+ github_authenticated_user = pipeline.fetch_state("github_authenticated_user")
+
+ if sender_login is None:
+ # Legacy integrations created before sender metadata was stored cannot
+ # be verified. Log a warning and allow the flow to proceed rather than
+ # raising a KeyError (500). Consider backfilling sender metadata for
+ # existing integrations if this warning appears frequently.
+ logger.warning(
+ "github.integration.missing_sender_metadata",
+ extra={"integration_id": integration.id, "external_id": installation_id},
+ )
- if (
- pipeline.fetch_state("github_authenticated_user")
- != integration.metadata["sender"]["login"]
- ):
+ elif github_authenticated_user != sender_login:
return error(request, self.active_organization)
return pipeline.next_step()🤖 Grapple PR auto-fix • major • Review this diff before applying
| payload = dict(parse_qsl(body)) | ||
| except Exception: | ||
| payload = {} | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (56 lines, 1 file) — review recommended)
OAuthLoginView directly imports and uses
GitHubIdentityProvider,safe_urlopen,safe_urlread, and manually implements the OAuth token exchange. This duplicates logic that already exists inOAuth2CallbackViewand the identity provider pipeline. The code comments even acknowledge this ('similar to OAuth2CallbackView.get_token_params', 'similar to OAuth2CallbackView.exchange_token'). This creates a maintenance burden where OAuth flow changes need to be synchronized across two implementations.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -1,8 +1,6 @@
from __future__ import annotations
import logging
-import re
-from urllib.parse import parse_qsl
from django.http import HttpResponse
from django.urls import reverse
@@ -15,7 +13,6 @@ from sentry import features, options
from sentry.api.utils import generate_organization_url
from sentry.constants import ObjectStatus
-from sentry.http import safe_urlopen, safe_urlread
from sentry.identity.github import GitHubIdentityProvider, get_user_info
from sentry.integrations import (
FeatureDescription,
@@ -30,6 +27,7 @@ from sentry.pipeline import PipelineView
from sentry.pipeline.views.oauth2 import OAuth2CallbackView
+import re
from sentry.tasks.integrations.github.constants import RATE_LIMITED_MESSAGE
from sentry.tasks.integrations.link_all_repos import link_all_repos
from sentry.utils import metrics
@@ -386,6 +384,14 @@ class OAuthLoginView(PipelineView):
def dispatch(self, request: Request, pipeline) -> HttpResponse:
self.determine_active_organization(request)
+ # Use OAuth2CallbackView to delegate token exchange, avoiding duplication
+ # of the OAuth flow that already exists in the identity pipeline.
+ oauth_callback_view = OAuth2CallbackView(
+ access_token_url=ghip.get_oauth_access_token_url(),
+ client_id=None, # resolved below via ghip
+ client_secret=None,
+ )
+
ghip = GitHubIdentityProvider()
github_client_id = ghip.get_oauth_client_id()
github_client_secret = ghip.get_oauth_client_secret()
@@ -404,24 +410,19 @@ class OAuthLoginView(PipelineView):
# At this point, we are past the GitHub "authorize" step
if request.GET.get("state") != pipeline.signature:
return error(request, self.active_organization)
- # 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)
-
- try:
- body = safe_urlread(req).decode("utf-8")
- payload = dict(parse_qsl(body))
- except Exception:
- payload = {}
+ # Delegate token exchange to OAuth2CallbackView, which owns this logic.
+ oauth_callback = OAuth2CallbackView(
+ access_token_url=ghip.get_oauth_access_token_url(),
+ client_id=github_client_id,
+ client_secret=github_client_secret,
+ )
+ data = oauth_callback.get_token_params(
+ code=request.GET.get("code"),
+ redirect_uri=absolute_uri(
+ reverse("sentry-extension-setup", kwargs={"provider_id": "github"})
+ ),
+ )
+ payload = oauth_callback.exchange_token(request, pipeline, data)
if "access_token" not in payload:
return error(request, self.active_organization)
@@ -431,6 +432,7 @@ class OAuthLoginView(PipelineView):
pipeline.bind_state("github_authenticated_user", authenticated_user_info["login"])
return pipeline.next_step()
+
class GitHubInstallation(PipelineView):
def get_app_url(self) -> str:
name = options.get("github-app.name")
@@ -465,20 +467,10 @@ class GitHubInstallation(PipelineView):
if installations_exist:
return error(
request,
self.active_organization,
error_short="Github installed on another Sentry organization.",
error_long=ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG,
)
# OrganizationIntegration does not exist, but Integration does exist.
- try:
- integration = Integration.objects.get(
- external_id=installation_id, status=ObjectStatus.ACTIVE
- )
- except Integration.DoesNotExist:
- return error(request, self.active_organization)
-
- # 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)
-
+ # installation_id is already bound to pipeline state above.
return pipeline.next_step()🤖 Grapple PR auto-fix • major • Review this diff before applying
| ) | ||
|
|
||
|
|
||
| class OAuthLoginView(PipelineView): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (2 lines, 1 file))
The
github_client_secretis retrieved at the top of thedispatchmethod (line 389) even when it's only needed during the token exchange phase (whenstateis present in the request). While not a direct vulnerability, this is a pattern concern — secrets should be fetched as late as possible and only when needed, to minimize their time in memory and reduce the blast radius of any memory disclosure vulnerabilities.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -390,7 +390,6 @@ class OAuthLoginView(PipelineView):
self.determine_active_organization(request)
ghip = GitHubIdentityProvider()
github_client_id = ghip.get_oauth_client_id()
- github_client_secret = ghip.get_oauth_client_secret()
installation_id = request.GET.get("installation_id")
if installation_id:
@@ -411,6 +410,7 @@ class OAuthLoginView(PipelineView):
if request.GET.get("state") != pipeline.signature:
return error(request, self.active_organization)
+ github_client_secret = ghip.get_oauth_client_secret()
# similar to OAuth2CallbackView.get_token_params
data = {
"code": request.GET.get("code"),🤖 Grapple PR auto-fix • major • Review this diff before applying
| if ( | ||
| provider_id in FORWARD_INSTALL_FOR | ||
| provider_id == "github" | ||
| and request.GET.get("setup_action") == "install" |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (10 lines, 1 file))
The refactor from
FORWARD_INSTALL_FORlist to a direct string comparison (provider_id == 'github') removes extensibility. While the intent states this is functionally equivalent, converting a list-based membership check to a hard-coded string comparison makes it harder to add future providers that need the same forward-install behavior. The comment was also moved but the reasoning for the change is unclear from an architectural perspective.
--- a/src/sentry/web/frontend/pipeline_advancer.py
+++ b/src/sentry/web/frontend/pipeline_advancer.py
@@ -16,6 +16,11 @@ PIPELINE_CLASSES = [IntegrationPipeline, IdentityProviderPipeline]
+# Providers that may be installed directly from the provider's own UI, bypassing
+# the Sentry pipeline. If that happens, redirect to the integration install org picker.
+FORWARD_INSTALL_FOR = ["github"]
+
+
from rest_framework.request import Request
@@ -34,10 +39,9 @@ def handle(self, request: Request, provider_id: str) -> HttpResponseBase:
if pipeline:
break
- # GitHub apps may be installed directly from GitHub, in which case
- # they will redirect here *without* being in the pipeline. If that happens
- # redirect to the integration install org picker.
if (
- provider_id == "github"
+ provider_id in FORWARD_INSTALL_FOR
and request.GET.get("setup_action") == "install"
and pipeline is None
):🤖 Grapple PR auto-fix • minor • Review this diff before applying
| ) | ||
| ) | ||
| assert resp.status_code == 302 | ||
| redirect = urlparse(resp["Location"]) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (14 lines, 1 file) — review recommended)
Test code in assert_setup_flow() performs multiple sequential GET requests on the same path with different query parameters (OAuth flow). The setup path is accessed multiple times without clear variable names distinguishing the stages.
--- a/tests/sentry/integrations/github/test_integration.py
+++ b/tests/sentry/integrations/github/test_integration.py
@@ -237,12 +237,15 @@ def assert_setup_flow(self):
- resp = self.client.get(
- "{}?{}".format(
- self.setup_path,
- urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
- ),
- )
+ # OAuth callback stage: GitHub redirects back with authorization code and state token
+ oauth_callback_path = "{}?{}".format(
+ self.setup_path,
+ urlencode(
+ {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ ),
)
+ resp = self.client.get(oauth_callback_path)
assert resp.status_code == 302
redirect = urlparse(resp["Location"])
assert redirect.scheme == "https"
assert redirect.netloc == "github.com"
assert redirect.path == "/apps/sentry-test-app"🤖 Grapple PR auto-fix • minor • Review this diff before applying
| "{}?{}".format( | ||
| self.setup_path, | ||
| urlencode( | ||
| {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"} |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (64 lines, 1 file) — review recommended)
The state value '9cae5e88803f35ed7970fc131e6e65d3' is hardcoded in multiple test assertions and URL constructions. If the pipeline signature generation algorithm changes, all these tests will break simultaneously. The state value should be captured dynamically from the initial redirect response rather than hardcoded.
--- a/tests/sentry/integrations/github/test_integration.py
+++ b/tests/sentry/integrations/github/test_integration.py
@@ -229,17 +229,18 @@ def assert_setup_flow(self):
redirect = urlparse(resp["Location"])
assert redirect.scheme == "https"
assert redirect.netloc == "github.com"
assert redirect.path == "/login/oauth/authorize"
- assert (
- redirect.query
- == "client_id=github-client-id&state=9cae5e88803f35ed7970fc131e6e65d3&redirect_uri=http://testserver/extensions/github/setup/"
- )
+ redirect_params = dict(
+ pair.split("=", 1) for pair in redirect.query.split("&") if "=" in pair
+ )
+ assert redirect_params["client_id"] == "github-client-id"
+ assert redirect_params["redirect_uri"] == "http://testserver/extensions/github/setup/"
+ state = redirect_params["state"]
+ assert state # ensure state is non-empty
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": state}
),
)
)
@@ -332,11 +333,15 @@ def test_github_installed_on_another_org(self):
),
urlencode({"installation_id": self.installation_id}),
)
+ resp_2_init = self.client.get(self.init_path_2)
+ redirect_2 = urlparse(resp_2_init["Location"])
+ redirect_2_params = dict(
+ pair.split("=", 1) for pair in redirect_2.query.split("&") if "=" in pair
+ )
+ state_2 = redirect_2_params["state"]
self.setup_path_2 = "{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": state_2}
),
)
with self.feature({"organizations:customer-domains": [self.organization_2.slug]}):
- resp = self.client.get(self.init_path_2)
resp = self.client.get(self.setup_path_2)
self.assertTemplateUsed(resp, "sentry/integrations/github-integration-failed.html")
@@ -366,6 +371,12 @@ def test_github_installed_on_another_org(self):
# Try again and should be successful
resp = self.client.get(self.init_path_2)
+ redirect_retry = urlparse(resp["Location"])
+ redirect_retry_params = dict(
+ pair.split("=", 1) for pair in redirect_retry.query.split("&") if "=" in pair
+ )
+ state_retry = redirect_retry_params["state"]
+ self.setup_path_2 = "{}?{}".format(
+ self.setup_path,
+ urlencode({"code": "12345678901234567890", "state": state_retry}),
+ )
resp = self.client.get(self.setup_path_2)
self.assertDialogSuccess(resp)
@@ -385,16 +396,19 @@ def test_installation_not_found(self):
resp = self.client.get(
"{}?{}".format(self.setup_path, urlencode({"installation_id": self.installation_id}))
)
+ # Capture state from the init redirect to avoid hardcoding
+ init_resp = self.client.get(self.init_path)
+ init_redirect = urlparse(init_resp["Location"])
+ init_params = dict(
+ pair.split("=", 1) for pair in init_redirect.query.split("&") if "=" in pair
+ )
+ state = init_params["state"]
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "ddd023d87a913d5226e2a882c4c4cc05"}
+ {"code": "12345678901234567890", "state": state + "_invalid"}
),
)
)
assert b"Invalid installation request." in resp.content
@@ -443,10 +457,18 @@ def test_github_user_mismatch(self):
self.client.get(init_path_1)
+ # Capture state from a fresh init for the second user's flow
+ resp_init_2 = self.client.get(init_path_2)
+ redirect_init_2 = urlparse(resp_init_2["Location"])
+ redirect_init_2_params = dict(
+ pair.split("=", 1) for pair in redirect_init_2.query.split("&") if "=" in pair
+ )
+ state_2 = redirect_init_2_params["state"]
setup_path_2 = "{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": state_2}
),
)
with self.feature({"organizations:customer-domains": [org_2.slug]}):
- resp = self.client.get(init_path_2)
resp = self.client.get(setup_path_2)
self.assertTemplateUsed(resp, "sentry/integrations/github-integration-failed.html")
@@ -698,10 +720,16 @@ def test_github_prevent_install_until_pending_deletion_is_complete(self):
resp = self.client.get(
"{}?{}".format(self.init_path, urlencode({"installation_id": self.installation_id}))
)
+ pending_redirect = urlparse(resp["Location"])
+ pending_params = dict(
+ pair.split("=", 1) for pair in pending_redirect.query.split("&") if "=" in pair
+ )
+ pending_state = pending_params["state"]
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode(
{
"code": "12345678901234567890",
- "state": "9cae5e88803f35ed7970fc131e6e65d3",
+ "state": pending_state,
}
),
)
@@ -730,10 +758,16 @@ def test_github_prevent_install_until_pending_deletion_is_complete(self):
resp = self.client.get(
"{}?{}".format(self.init_path, urlencode({"installation_id": self.installation_id}))
)
+ retry_redirect = urlparse(resp["Location"])
+ retry_params = dict(
+ pair.split("=", 1) for pair in retry_redirect.query.split("&") if "=" in pair
+ )
+ retry_state = retry_params["state"]
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": retry_state}
),
)
)🤖 Grapple PR auto-fix • minor • Review this diff before applying
| def get_document_origin(org) -> str: | ||
| if org and features.has("organizations:customer-domains", org.organization): | ||
| return f'"{generate_organization_url(org.organization.slug)}"' | ||
| return "document.origin" |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Small fix (4 lines, 1 file))
Helper function
error()uses hardcoded 'github-integration-failed.html' template path without constants or centralized template reference. If the template name changes, multiple locations would need updates.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -110,6 +110,8 @@ ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG = _(
"It seems that your GitHub account has been installed on another Sentry organization. Please uninstall and try again."
)
ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST = _(
"We could not verify the authenticity of the installation request. We recommend restarting the installation process."
)
ERR_INTEGRATION_PENDING_DELETION = _(
"It seems that your Sentry organization has an installation pending deletion. Please wait ~15min for the uninstall to complete and try again."
)
+
+GITHUB_INTEGRATION_FAILED_TEMPLATE = "sentry/integrations/github-integration-failed.html"
def build_repository_query(metadata: Mapping[str, Any], name: str, query: str) -> bytes:
@@ -130,7 +132,7 @@ def error(
error_long=ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST,
):
return render_to_response(
- "sentry/integrations/github-integration-failed.html",
+ GITHUB_INTEGRATION_FAILED_TEMPLATE,
context={
"error": error_long,
"payload": {🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
|
||
| def get_document_origin(org) -> str: | ||
| if org and features.has("organizations:customer-domains", org.organization): | ||
| return f'"{generate_organization_url(org.organization.slug)}"' |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Small fix (8 lines, 1 file))
Function parameter
error_shortin theerror()helper uses underscore naming style, but the function immediately wraps it with_()i18n function, making the parameter name misleading about its nature.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -126,7 +126,7 @@ def build_repository_query(metadata: Mapping[str, Any], name: str, query: str)
def error(
request,
org,
- error_short="Invalid installation request.",
+ error_short_untranslated="Invalid installation request.",
error_long=ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST,
):
return render_to_response(
@@ -135,7 +135,7 @@ def error(
"payload": {
"success": False,
- "data": {"error": _(error_short)},
+ "data": {"error": _(error_short_untranslated)},
},
"document_origin": get_document_origin(org),
},
@@ -465,7 +465,7 @@ def dispatch(self, request: Request, pipeline: Pipeline) -> HttpResponse:
if integration_pending_deletion_exists:
return error(
request,
self.active_organization,
- error_short="GitHub installation pending deletion.",
+ error_short_untranslated="GitHub installation pending deletion.",
error_long=ERR_INTEGRATION_PENDING_DELETION,
)
@@ -487,7 +487,7 @@ def dispatch(self, request: Request, pipeline: Pipeline) -> HttpResponse:
if installations_exist:
return error(
request,
self.active_organization,
- error_short="Github installed on another Sentry organization.",
+ error_short_untranslated="Github installed on another Sentry organization.",
error_long=ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG,
)🤖 Grapple PR auto-fix • minor • Review this diff before applying
| assert redirect.path == "/login/oauth/authorize" | ||
| assert ( | ||
| redirect.query | ||
| == "client_id=github-client-id&state=9cae5e88803f35ed7970fc131e6e65d3&redirect_uri=http://testserver/extensions/github/setup/" |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (13 lines, 1 file) — review recommended)
Hardcoded state value '9cae5e88803f35ed7970fc131e6e65d3' appears in test assertions. If state generation is mocked differently elsewhere or the test setup changes, these hardcoded values will cause brittle test failures.
--- a/tests/sentry/integrations/github/test_integration.py
+++ b/tests/sentry/integrations/github/test_integration.py
@@ -1,6 +1,6 @@
from urllib.parse import urlencode, urlparse
+from urllib.parse import parse_qs
@@ -226,15 +226,17 @@ def assert_setup_flow(self):
def assert_setup_flow(self):
resp = self.client.get(self.init_path)
assert resp.status_code == 302
redirect = urlparse(resp["Location"])
assert redirect.scheme == "https"
assert redirect.netloc == "github.com"
assert redirect.path == "/login/oauth/authorize"
- assert (
- redirect.query
- == "client_id=github-client-id&state=9cae5e88803f35ed7970fc131e6e65d3&redirect_uri=http://testserver/extensions/github/setup/"
- )
+ query_params = parse_qs(redirect.query)
+ assert query_params["client_id"] == ["github-client-id"]
+ assert query_params["redirect_uri"] == ["http://testserver/extensions/github/setup/"]
+ assert "state" in query_params
+ state = query_params["state"][0]
+ assert state # state must be non-empty
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": state}
),
)
)🤖 Grapple PR auto-fix • minor • Review this diff before applying
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds an OAuth login step to the GitHub integration pipeline to verify the authenticity of installation requests using a state parameter, mitigating CSRF attacks during GitHub App installation flows.
Changes: 3 files (+247 / -50 lines)
- 🟡 3 files modified:
src/sentry/integrations/github/integration.py,src/sentry/web/frontend/pipeline_advancer.py,tests/sentry/integrations/github/test_integration.py
Resolves: #4 (feat(ecosystem): Implement cross-system issue synchronization)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 3 files"/]
f1["🟡 integration.py +118/-41"]
pr_0 --> f1
f2["🟡 pipeline_advancer.py +4/-7"]
pr_0 --> f2
f3["🟡 test_integration.py +125/-2"]
pr_0 --> f3
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
1 critical issue found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 Major | 6 |
| 🟡 Minor | 12 |
| 🔵 Info | 4 |
Review details
- Reviewed at: 2026-04-08T00:44:25.777Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 94.5s
- Low-confidence findings suppressed: 2
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
|
|
||
| installation_id = request.GET.get("installation_id") | ||
| if installation_id: | ||
| pipeline.bind_state("installation_id", installation_id) |
There was a problem hiding this comment.
🔴 CRITICAL — Security - CSRF State Validation (confidence: 100%)
The state parameter is pipeline.signature, which is a deterministic property of the pipeline rather than a cryptographically random, single-use nonce. If pipeline.signature is derived from predictable pipeline attributes (e.g., a hash of the provider ID, organization, etc.), an attacker who can guess or compute the signature can bypass CSRF protection. Additionally, the state is never invalidated after use — it remains valid for the lifetime of the pipeline, making it susceptible to replay attacks.
Evidence:
- Line ~405:
f"{ghip.get_oauth_authorize_url()}?client_id={github_client_id}&state={state}&redirect_uri={redirect_uri}"— no urllib.parse.urlencode() used - RFC 6749 requires redirect_uri to be percent-encoded when included as a query parameter
- If redirect_uri contains '&' or '=' (e.g., from a misconfigured absolute_uri), it would inject additional query parameters visible to GitHub
- Line 412:
if request.GET.get('state') != pipeline.signature:— validates but does not consume/rotate the state - No call to invalidate or rotate
pipeline.signatureafter successful validation - Acceptance criteria explicitly states: 'Concurrent or replayed OAuth callbacks with a valid but already-consumed state token — verify state is invalidated after first use'
- Line 399:
state = pipeline.signature— the state is derived from a pipeline property, not fromsecrets.token_urlsafe()or similar CSPRNG - The state is not stored in a one-time-use store (e.g., cache with TTL + delete-on-use); it's just compared against
pipeline.signaturewhich presumably remains constant - Intent specification explicitly calls out: 'State parameter generation and validation logic in OAuthLoginView — if state is not cryptographically random or not validated strictly, CSRF protection is ineffective'
- Intent specification edge case: 'Concurrent or replayed OAuth callbacks with a valid but already-consumed state token — verify state is invalidated after first use' — this is NOT handled
Agent: architecture
| request, | ||
| self.active_organization, | ||
| error_short="GitHub installation pending deletion.", | ||
| error_long=ERR_INTEGRATION_PENDING_DELETION, |
There was a problem hiding this comment.
🟠 MAJOR — Unhandled Exception / Missing Input Validation (confidence: 100%)
The sender login check accesses integration.metadata["sender"]["login"] without guarding against missing keys. If the Integration record was created by a different code path (e.g., a direct webhook install) where metadata lacks a sender key or sender lacks login, this raises an unhandled KeyError, resulting in a 500 error rather than a graceful rejection. This is reachable when an Integration exists but no OrganizationIntegration exists.
Evidence:
- Line ~461:
integration.metadata["sender"]["login"]— no .get() used, no try/except - The test
test_github_user_mismatchsetswebhook_event['sender']['login'] = 'attacker'confirming the metadata path, but production installs via webhook may have different metadata shapes - Acceptance criteria notes: 'org.organization access in get_document_origin() assumes org has a nested .organization attribute — verify this holds for all callers' — similar pattern of unsafe attribute access
Agent: security
| body = safe_urlread(req).decode("utf-8") | ||
| payload = dict(parse_qsl(body)) | ||
| except Exception: | ||
| payload = {} |
There was a problem hiding this comment.
🟠 MAJOR — Error Handling - Access Token Exchange (confidence: 100%)
The access token exchange silently swallows all exceptions from safe_urlread and falls back to an empty dict. If GitHub returns an error response (e.g., error=bad_verification_code&error_description=...), parse_qsl will parse it into the payload dict, but only the absence of access_token is checked. More critically, if the HTTP request itself fails (network error, 5xx), the broad except Exception hides the root cause, making debugging difficult. There is also no check on the HTTP status code of the response before parsing the body.
Evidence:
- Lines 425-428:
except Exception: payload = {}— catches everything including programming errors - Intent specification risk area: 'Parsing the access_token response with parse_qsl — GitHub errors return query-string-encoded error fields; if not checked, a failed exchange could proceed silently'
- No status code check on
reqbefore reading body — a 4xx/5xx response is silently treated the same as a 200
Agent: architecture
| return error(request, self.active_organization) | ||
|
|
||
| authenticated_user_info = get_user_info(payload["access_token"]) | ||
| if "login" not in authenticated_user_info: |
There was a problem hiding this comment.
🟠 MAJOR — Error Handling - User Info Fetch (confidence: 100%)
The call to get_user_info(payload['access_token']) has no exception handling. If the GitHub /user endpoint returns a non-200 response, times out, or returns malformed JSON, this will raise an unhandled exception that surfaces as a 500 error to the user rather than a graceful error page.
Evidence:
- Line 433:
authenticated_user_info = get_user_info(payload['access_token'])— no try/except - Intent specification edge case: 'If the /user endpoint returns a non-200 or unexpected payload, the integration should fail gracefully rather than proceeding with a null login'
- The
get_user_infofunction likely makes an HTTP request that can raise various exceptions
Agent: architecture
| error_long=ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG, | ||
| ) | ||
|
|
||
| # OrganizationIntegration does not exist, but Integration does exist. |
There was a problem hiding this comment.
🟠 MAJOR — Security - Sender Verification (confidence: 100%)
The sender identity check compares pipeline.fetch_state('github_authenticated_user') against integration.metadata['sender']['login']. The metadata['sender'] is populated from the GitHub webhook event, which could be manipulated if the webhook signature validation is weak or the attacker controls a GitHub App installation. Additionally, if integration.metadata doesn't contain the 'sender' key (e.g., for integrations created through a different code path), this will raise a KeyError and crash with a 500 rather than showing a graceful error.
Evidence:
- Lines 487-492: Direct dictionary access
integration.metadata['sender']['login']with no.get()or try/except - The
metadatastructure depends on how the integration was originally created — older integrations or those created through different flows may not havesenderin metadata - If
metadatais None or missing 'sender', this will raise KeyError/TypeError
Agent: architecture
| {"code": "12345678901234567890", "state": "ddd023d87a913d5226e2a882c4c4cc05"} | ||
| ), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🟡 MINOR — Code organization (confidence: 80%)
The test_installation_not_found test now makes two separate GET requests to setup_path but doesn't clearly explain why the second request is testing the OAuth callback step with a different state value.
Evidence:
- Lines 389-394: second GET request uses a different state value 'ddd023d87a913d5226e2a882c4c4cc05'
- The test description suggests installation_id not being found, but the second request is testing state mismatch
- The intent of using a different state value is not documented
Agent: style
| return pipeline.next_step() | ||
|
|
||
|
|
||
| class GitHubInstallation(PipelineView): |
There was a problem hiding this comment.
🔵 INFO — Code organization (confidence: 98%)
The error handling for access_token parsing silently catches all exceptions and returns an error response. While this is safe, the specific error conditions should be documented for maintainability.
Evidence:
- Lines 434-437: broad
except Exceptioncatches any parsing error but doesn't log or distinguish error types - Risk area from acceptance criteria: 'Parsing the access_token response with parse_qsl — GitHub errors return query-string-encoded error fields; if not checked, a failed exchange could proceed silently'
- Lines 440-441: checks for 'access_token' in payload but doesn't log what error GitHub returned
Agent: style
| return pipeline.next_step() | ||
|
|
||
|
|
||
| class GitHubInstallation(PipelineView): |
There was a problem hiding this comment.
🔵 INFO — Code organization (confidence: 93%)
The OAuthLoginView class uses pipeline.signature as the state value, but this is not documented. The relationship between pipeline.signature and CSRF state generation should be clarified in code comments.
Evidence:
- Line 412:
state = pipeline.signature— unclear whether pipeline.signature is cryptographically random or how it's generated - Line 419: state value is used in OAuth URL without explanation of its origin or guarantees
- Risk area from acceptance criteria: 'State parameter generation and validation logic in OAuthLoginView — if state is not cryptographically random or not validated strictly, CSRF protection is ineffective'
- Test uses deterministic state '9cae5e88803f35ed7970fc131e6e65d3' which suggests state generation may be mocked or seeded
Agent: style
| "code": request.GET.get("code"), | ||
| "client_id": github_client_id, | ||
| "client_secret": github_client_secret, | ||
| } |
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 83%)
The OAuth authorization redirect URL construction uses f-strings without URL encoding. While GitHub client_id and redirect_uri are controlled values, state contains user-session data and should be clearly documented or validated.
Evidence:
- Line 419-421: OAuth URL is constructed via f-string without explicit URL encoding of parameters
- The state value comes from pipeline.signature and is embedded directly into the URL
- If pipeline.signature could ever contain special characters, this could create security or parsing issues
Agent: style
| data=json.dumps(webhook_event), | ||
| content_type="application/json", | ||
| HTTP_X_GITHUB_EVENT="installation", | ||
| HTTP_X_HUB_SIGNATURE="sha1=d184e6717f8bfbcc291ebc8c0756ee446c6c9486", |
There was a problem hiding this comment.
🔵 INFO — Hardcoded Test State Value (confidence: 81%)
The state value '9cae5e88803f35ed7970fc131e6e65d3' is hardcoded across multiple test cases, implying pipeline.signature is deterministic or mocked in tests. If the mock is incomplete or the state generation changes in production without updating tests, the tests will fail to catch regressions. The static scanner flagged this as a weak crypto concern — it appears to be MD5-length (32 hex chars) which would be a weak choice for a CSRF token if that's what's being used in production.
Evidence:
- State '9cae5e88803f35ed7970fc131e6e65d3' appears in 5+ test locations
- 32-character hex string consistent with MD5 digest length
- If pipeline.signature uses MD5 internally, this would be a weak CSRF token — SHA-256 or a cryptographically random 32-byte value should be used instead
Agent: security
| request, | ||
| self.active_organization, | ||
| error_short="GitHub installation pending deletion.", | ||
| error_long=ERR_INTEGRATION_PENDING_DELETION, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (8 lines, 1 file))
The sender login check accesses
integration.metadata["sender"]["login"]without guarding against missing keys. If the Integration record was created by a different code path (e.g., a direct webhook install) wheremetadatalacks asenderkey orsenderlackslogin, this raises an unhandled KeyError, resulting in a 500 error rather than a graceful rejection. This is reachable when an Integration exists but no OrganizationIntegration exists.
| error_long=ERR_INTEGRATION_PENDING_DELETION, | |
| # Check that the authenticated GitHub user is the same as who installed the app. | |
| # Guard against metadata missing 'sender' or 'login' keys — these may be absent | |
| # for integrations created via direct webhook installs or legacy code paths. | |
| # Treat a missing/malformed metadata structure as a mismatch (fail closed). | |
| sender_login = integration.metadata.get("sender", {}).get("login") | |
| != sender_login |
🤖 Grapple PR auto-fix • major • confidence: 100%
| return error(request, self.active_organization) | ||
|
|
||
| authenticated_user_info = get_user_info(payload["access_token"]) | ||
| if "login" not in authenticated_user_info: |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (10 lines, 1 file))
The call to
get_user_info(payload['access_token'])has no exception handling. If the GitHub/userendpoint returns a non-200 response, times out, or returns malformed JSON, this will raise an unhandled exception that surfaces as a 500 error to the user rather than a graceful error page.
| if "login" not in authenticated_user_info: | |
| try: | |
| authenticated_user_info = get_user_info(payload["access_token"]) | |
| except Exception: | |
| logger.exception( | |
| "github.oauth.get_user_info_failed", | |
| extra={"error": "Failed to retrieve user info from GitHub /user endpoint"}, | |
| ) | |
| return error(request, self.active_organization) | |
🤖 Grapple PR auto-fix • major • confidence: 100%
| error_long=ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG, | ||
| ) | ||
|
|
||
| # OrganizationIntegration does not exist, but Integration does exist. |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (9 lines, 1 file))
The sender identity check compares
pipeline.fetch_state('github_authenticated_user')againstintegration.metadata['sender']['login']. Themetadata['sender']is populated from the GitHub webhook event, which could be manipulated if the webhook signature validation is weak or the attacker controls a GitHub App installation. Additionally, ifintegration.metadatadoesn't contain the 'sender' key (e.g., for integrations created through a different code path), this will raise aKeyErrorand crash with a 500 rather than showing a graceful error.
| # OrganizationIntegration does not exist, but Integration does exist. | |
| sender_login = (integration.metadata or {}).get("sender", {}).get("login") | |
| if not sender_login: | |
| # NOTE: Older integrations or those created through a different flow may | |
| # not have 'sender' in metadata. Treat missing sender as unverifiable | |
| # and return a graceful error rather than crashing with KeyError/TypeError. | |
| return error(request, self.active_organization) | |
| != sender_login |
🤖 Grapple PR auto-fix • major • confidence: 100%
| ): | ||
| return error(request, self.active_organization) | ||
|
|
||
| return pipeline.next_step() |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (9 lines, 1 file))
GitHubInstallation.dispatch performs two separate Integration.objects.get() database queries for the same external_id within the same request: one inside the installations_exist block (line ~488) and another after it (line ~502). The second query also adds a status filter but is essentially fetching the same row. This is redundant work that could be collapsed into a single query.
| return pipeline.next_step() | |
| integration = Integration.objects.get(external_id=installation_id) | |
| integration=integration | |
| if integration.status != ObjectStatus.ACTIVE: |
🤖 Grapple PR auto-fix • major • confidence: 100%
| if org and features.has("organizations:customer-domains", org.organization): | ||
| return f'"{generate_organization_url(org.organization.slug)}"' | ||
| return "document.origin" | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (6 lines, 1 file))
get_document_origin() accesses
org.organizationdirectly. While the function guards againstorgbeing None withif org and ..., it does not guard againstorghaving no.organizationattribute. Iforgis a type other than the expected organization context object (e.g., a raw Organization model vs. an OrganizationContext), an AttributeError will be raised. All three callers passself.active_organizationwhich may be None-safe but the type is not enforced.
| if org and hasattr(org, "organization") and org.organization is not None: | |
| if features.has("organizations:customer-domains", org.organization): | |
| if hasattr(org.organization, "slug"): | |
| return f'"{generate_organization_url(org.organization.slug)}"' |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| {"code": "12345678901234567890", "state": "ddd023d87a913d5226e2a882c4c4cc05"} | ||
| ), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (8 lines, 1 file))
The test_installation_not_found test now makes two separate GET requests to setup_path but doesn't clearly explain why the second request is testing the OAuth callback step with a different state value.
| ) | |
| # Step 1: Initiate the installation flow; this sets up the OAuth state in the session | |
| # and redirects to GitHub's OAuth authorize page. | |
| # Step 2: Simulate the OAuth callback with a deliberately invalid/mismatched state value. | |
| # The valid state generated in step 1 is stored in the session; providing a different | |
| # state here ('ddd023d87a913d5226e2a882c4c4cc05') exercises the CSRF state validation | |
| # check introduced by this PR, which should reject the request as invalid regardless | |
| # of whether the installation_id exists. |
🤖 Grapple PR auto-fix • minor • confidence: 80%
|
|
||
| installation_id = request.GET.get("installation_id") | ||
| if installation_id: | ||
| pipeline.bind_state("installation_id", installation_id) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (27 lines, 1 file) — review recommended)
The state parameter is
pipeline.signature, which is a deterministic property of the pipeline rather than a cryptographically random, single-use nonce. Ifpipeline.signatureis derived from predictable pipeline attributes (e.g., a hash of the provider ID, organization, etc.), an attacker who can guess or compute the signature can bypass CSRF protection. Additionally, the state is never invalidated after use — it remains valid for the lifetime of the pipeline, making it susceptible to replay attacks.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -1,6 +1,7 @@
from __future__ import annotations
import logging
+import secrets
import re
from collections.abc import Collection, Mapping, Sequence
from typing import Any
-from urllib.parse import parse_qsl
+from urllib.parse import parse_qsl, urlencode
from django.http import HttpResponse
from django.urls import reverse
@@ -386,6 +387,8 @@ class OAuthLoginView(PipelineView):
def dispatch(self, request: Request, pipeline) -> HttpResponse:
self.determine_active_organization(request)
ghip = GitHubIdentityProvider()
github_client_id = ghip.get_oauth_client_id()
github_client_secret = ghip.get_oauth_client_secret()
installation_id = request.GET.get("installation_id")
if installation_id:
pipeline.bind_state("installation_id", installation_id)
if not request.GET.get("state"):
- state = pipeline.signature
+ # Generate a cryptographically random, single-use nonce for CSRF protection.
+ # Using secrets.token_urlsafe() per RFC 6749 §10.12 requirements — pipeline.signature
+ # is deterministic and never consumed, making it unsuitable as an OAuth state token.
+ state = secrets.token_urlsafe(32)
+ pipeline.bind_state("oauth_state", state)
redirect_uri = absolute_uri(
reverse("sentry-extension-setup", kwargs={"provider_id": "github"})
)
+ # Use urlencode() to percent-encode parameters per RFC 6749 — embedding redirect_uri
+ # directly in an f-string risks parameter injection if it contains '&' or '='.
+ query_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()}?{query_params}"
)
# At this point, we are past the GitHub "authorize" step
- if request.GET.get("state") != pipeline.signature:
+ # Retrieve and immediately consume the stored nonce to prevent replay attacks.
+ # Binding to None invalidates the token so a second callback with the same state is rejected.
+ stored_state = pipeline.fetch_state("oauth_state")
+ received_state = request.GET.get("state")
+ # Invalidate the nonce before any further processing to prevent replay on error paths too.
+ pipeline.bind_state("oauth_state", None)
+ if not stored_state or not received_state or not secrets.compare_digest(received_state, stored_state):
+ # secrets.compare_digest() prevents timing attacks during state comparison.
return error(request, self.active_organization)
# 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)
try:
body = safe_urlread(req).decode("utf-8")
payload = dict(parse_qsl(body))
except Exception:
payload = {}
if "access_token" not in payload:
return error(request, self.active_organization)
authenticated_user_info = get_user_info(payload["access_token"])
if "login" not in authenticated_user_info:
return error(request, self.active_organization)
pipeline.bind_state("github_authenticated_user", authenticated_user_info["login"])
return pipeline.next_step()🤖 Grapple PR auto-fix • critical • Review this diff before applying
| body = safe_urlread(req).decode("utf-8") | ||
| payload = dict(parse_qsl(body)) | ||
| except Exception: | ||
| payload = {} |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (33 lines, 1 file) — review recommended)
The access token exchange silently swallows all exceptions from
safe_urlreadand falls back to an empty dict. If GitHub returns an error response (e.g.,error=bad_verification_code&error_description=...),parse_qslwill parse it into the payload dict, but only the absence ofaccess_tokenis checked. More critically, if the HTTP request itself fails (network error, 5xx), the broadexcept Exceptionhides the root cause, making debugging difficult. There is also no check on the HTTP status code of the response before parsing the body.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -425,8 +425,24 @@ class OAuthLoginView(PipelineView):
# similar to OAuth2CallbackView.exchange_token
req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data)
+ if req.status_code != 200:
+ logger.error(
+ "github.oauth.token_exchange_failed",
+ extra={
+ "status_code": req.status_code,
+ "response_body": safe_urlread(req).decode("utf-8", errors="replace"),
+ },
+ )
+ return error(request, self.active_organization)
+
try:
body = safe_urlread(req).decode("utf-8")
payload = dict(parse_qsl(body))
- except Exception:
- payload = {}
+ except Exception as e:
+ logger.error(
+ "github.oauth.token_exchange_parse_failed",
+ exc_info=e,
+ )
+ return error(request, self.active_organization)
+
+ if "error" in payload:
+ logger.error(
+ "github.oauth.token_exchange_error",
+ extra={
+ "error": payload.get("error"),
+ "error_description": payload.get("error_description"),
+ "error_uri": payload.get("error_uri"),
+ },
+ )
+ return error(request, self.active_organization)
if "access_token" not in payload:
+ logger.error(
+ "github.oauth.missing_access_token",
+ extra={"payload_keys": list(payload.keys())},
+ )
return error(request, self.active_organization)🤖 Grapple PR auto-fix • major • Review this diff before applying
| ) | ||
|
|
||
| # At this point, we are past the GitHub "authorize" step | ||
| if request.GET.get("state") != pipeline.signature: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Small fix (4 lines, 1 file))
The OAuth state parameter is compared using the
!=operator, which is not a constant-time comparison. While the practical risk for CSRF state tokens is lower than for HMAC secrets (since the attacker needs a remote timing oracle), using constant-time comparison is a security best practice for any token validation.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -1,6 +1,7 @@
from __future__ import annotations
+import hmac
import logging
import re
from collections.abc import Collection, Mapping, Sequence
@@ -409,7 +410,8 @@ class OAuthLoginView(PipelineView):
# At this point, we are past the GitHub "authorize" step
- if request.GET.get("state") != pipeline.signature:
+ received_state = request.GET.get("state") or ""
+ if not hmac.compare_digest(received_state, pipeline.signature):
return error(request, self.active_organization)🤖 Grapple PR auto-fix • minor • Review this diff before applying
| def get_document_origin(org) -> str: | ||
| if org and features.has("organizations:customer-domains", org.organization): | ||
| return f'"{generate_organization_url(org.organization.slug)}"' | ||
| return "document.origin" |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (22 lines, 1 file) — review recommended)
The error() helper function and get_document_origin() helper are defined at module level but are tightly coupled to GitHubInstallation and OAuthLoginView. These helpers would be more maintainable if documented or placed closer to their usage context, especially since they handle GitHub-specific error rendering.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -129,6 +129,16 @@ def build_repository_query(metadata: Mapping[str, Any], name: str, query: str)
def error(
request,
org,
error_short="Invalid installation request.",
error_long=ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST,
):
+ """
+ Render the GitHub integration failure page with a structured error payload.
+
+ Used by both OAuthLoginView (state mismatch / token exchange failures) and
+ GitHubInstallation (pending deletion, duplicate org, sender mismatch) to
+ return a consistent error response that posts a message back to the opener
+ window via the ``document_origin`` context variable.
+
+ See also: get_document_origin()
+ """
return render_to_response(
"sentry/integrations/github-integration-failed.html",
context={
@@ -149,6 +159,14 @@ def error(
def get_document_origin(org) -> str:
+ """
+ Return the JavaScript ``postMessage`` target origin for the GitHub
+ integration failure template.
+
+ When the installing organization has the ``organizations:customer-domains``
+ feature enabled, the origin is scoped to that organization's subdomain URL
+ so the opener frame accepts the message. Falls back to the permissive
+ ``document.origin`` sentinel for standard single-domain deployments.
+
+ Called exclusively from error() and, indirectly, from OAuthLoginView and
+ GitHubInstallation via that helper.
+ """
if org and features.has("organizations:customer-domains", org.organization):
return f'"{generate_organization_url(org.organization.slug)}"'
return "document.origin"🤖 Grapple PR auto-fix • minor • Review this diff before applying
| def get_document_origin(org) -> str: | ||
| if org and features.has("organizations:customer-domains", org.organization): | ||
| return f'"{generate_organization_url(org.organization.slug)}"' | ||
| return "document.origin" |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (16 lines, 1 file) — review recommended)
The get_document_origin() function assumes org has a nested .organization attribute, but doesn't document this requirement or handle the case where org is not None but org.organization doesn't exist.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -149,7 +149,18 @@ def error(
def get_document_origin(org) -> str:
- if org and features.has("organizations:customer-domains", org.organization):
- return f'"{generate_organization_url(org.organization.slug)}"'
+ """
+ Return the JavaScript origin string for postMessage communication.
+
+ :param org: An ``RpcUserOrganizationContext`` (or compatible object exposing
+ an ``.organization`` attribute of type ``RpcOrganization``), or ``None``.
+ When ``org`` is not ``None``, it is expected to have an ``.organization``
+ attribute; if that attribute is missing for any reason, the function
+ falls back safely to ``"document.origin"``.
+ :returns: A quoted organization URL string if the customer-domains feature
+ is enabled for the org, otherwise the literal string ``"document.origin"``.
+ """
+ organization = getattr(org, "organization", None) if org is not None else None
+ if organization is not None and features.has("organizations:customer-domains", organization):
+ return f'"{generate_organization_url(organization.slug)}"'
return "document.origin"🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
|
||
| ghip = GitHubIdentityProvider() | ||
| github_client_id = ghip.get_oauth_client_id() | ||
| github_client_secret = ghip.get_oauth_client_secret() |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (9 lines, 1 file))
In OAuthLoginView,
installation_idis bound to pipeline state from query params on the initial request (before redirect to GitHub). After the OAuth callback, the query params will no longer containinstallation_id— the value must be retrieved from pipeline state. However, in GitHubInstallation.dispatch(),installation_idfalls back topipeline.fetch_state('installation_id'), which works. The concern is that if a user navigates directly to the OAuth callback URL without first hitting OAuthLoginView withinstallation_id, the pipeline state won't have it, and the flow falls through toredirect(self.get_app_url())in GitHubInstallation — this is the intended behavior but could be more explicitly documented.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -395,6 +395,12 @@ class OAuthLoginView(PipelineView):
installation_id = request.GET.get("installation_id")
if installation_id:
+ # Bind installation_id to pipeline state on the *initial* request
+ # (before redirect to GitHub). After the OAuth callback, query params
+ # will no longer contain installation_id, so GitHubInstallation.dispatch()
+ # must retrieve it from pipeline state via pipeline.fetch_state('installation_id').
pipeline.bind_state("installation_id", installation_id)
@@ -453,6 +459,12 @@ class GitHubInstallation(PipelineView):
def dispatch(self, request: Request, pipeline: Pipeline) -> HttpResponse:
installation_id = request.GET.get(
"installation_id", pipeline.fetch_state("installation_id")
)
if installation_id is None:
+ # installation_id is absent from both the query params and pipeline
+ # state. This happens when a user navigates directly to the OAuth
+ # callback URL without first going through OAuthLoginView with a
+ # valid installation_id. Redirecting to the GitHub App URL is the
+ # intended fallback — it restarts the installation flow from scratch.
return self.redirect(self.get_app_url())🤖 Grapple PR auto-fix • minor • Review this diff before applying
| return "document.origin" | ||
|
|
||
|
|
||
| # Github App docs and list of available endpoints |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (25 lines, 1 file) — review recommended)
get_document_origin() calls features.has() and generate_organization_url() every time it is invoked. In error rendering scenarios these are cheap, but the function is called from multiple error() call sites within the same request. If the function were called in a hot path or called multiple times per request, the repeated feature flag lookups (which may involve DB or cache reads) would be wasteful.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -1,6 +1,7 @@
from __future__ import annotations
import logging
+import functools
import re
from collections.abc import Collection, Mapping, Sequence
from typing import Any
@@ -149,7 +150,17 @@ def error(
def get_document_origin(org) -> str:
+ """Return the postMessage target origin for the given org.
+
+ NOTE: The result is cached per org slug because features.has() may
+ involve a cache/DB read, and this function can be called multiple
+ times within a single request (once per error() call site). The org
+ slug and customer-domains feature flag are stable for the lifetime of
+ a request, so caching is safe here.
+ """
if org and features.has("organizations:customer-domains", org.organization):
- return f'"{generate_organization_url(org.organization.slug)}"'
- return "document.origin"
+ return _cached_document_origin(org.organization.slug)
+ return "document.origin"
+
+
+@functools.lru_cache(maxsize=256)
+def _cached_document_origin(org_slug: str) -> str:
+ """Return the quoted organization URL for orgs with customer-domains enabled.
+
+ Separated from get_document_origin() so we can cache on a hashable
+ primitive (the slug string) rather than on the org object itself.
+ The feature flag check is intentionally kept in get_document_origin()
+ so that orgs *without* the feature never populate the cache.
+ """
+ return f'"{generate_organization_url(org_slug)}"'🤖 Grapple PR auto-fix • minor • Review this diff before applying
| assert redirect.path == "/apps/sentry-test-app" | ||
|
|
||
| # App installation ID is provided | ||
| resp = self.client.get( |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (48 lines, 1 file) — review recommended)
The test asserts against
responses.calls[2]which depends on the exact ordering and count of mocked HTTP calls. The hardcoded state value '9cae5e88803f35ed7970fc131e6e65d3' also implies the pipeline signature is deterministic/predictable, which ties back to the security concern about state generation. If the state generation mechanism changes, all tests break; if it doesn't change, the state is predictable in production.
--- a/tests/sentry/integrations/github/test_integration.py
+++ b/tests/sentry/integrations/github/test_integration.py
@@ -229,32 +229,43 @@ def assert_setup_flow(self):
redirect = urlparse(resp["Location"])
assert redirect.scheme == "https"
assert redirect.netloc == "github.com"
- assert redirect.path == "/login/oauth/authorize"
- assert (
- redirect.query
- == "client_id=github-client-id&state=9cae5e88803f35ed7970fc131e6e65d3&redirect_uri=http://testserver/extensions/github/setup/"
- )
+ assert redirect.path == "/login/oauth/authorize"
+
+ # Extract state dynamically so tests aren't coupled to a specific hash value.
+ # If state generation changes (e.g. for security improvements), only this
+ # extraction needs updating rather than every hardcoded occurrence.
+ from urllib.parse import parse_qs
+ query_params = parse_qs(redirect.query)
+ assert "state" in query_params, "OAuth state parameter missing from redirect"
+ assert "client_id" in query_params
+ assert query_params["client_id"] == ["github-client-id"]
+ assert query_params.get("redirect_uri") == ["http://testserver/extensions/github/setup/"]
+ oauth_state = query_params["state"][0]
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": oauth_state}
),
)
)
assert resp.status_code == 302
redirect = urlparse(resp["Location"])
assert redirect.scheme == "https"
assert redirect.netloc == "github.com"
assert redirect.path == "/apps/sentry-test-app"
# App installation ID is provided
resp = self.client.get(
"{}?{}".format(self.setup_path, urlencode({"installation_id": self.installation_id}))
)
- auth_header = responses.calls[2].request.headers["Authorization"]
+ # Find the access token request by URL rather than by index, so this
+ # assertion remains correct if additional HTTP calls are inserted elsewhere
+ # in the pipeline (e.g. new API calls added to _stub_github).
+ access_token_url = self.base_url + f"/app/installations/{self.installation_id}/access_tokens"
+ access_token_calls = [
+ call for call in responses.calls
+ if call.request.url == access_token_url
+ ]
+ assert len(access_token_calls) >= 1, (
+ f"Expected at least one POST to {access_token_url}, "
+ f"but got calls to: {[c.request.url for c in responses.calls]}"
+ )
+ auth_header = access_token_calls[0].request.headers["Authorization"]
assert auth_header == "Bearer jwt_token_1"
self.assertDialogSuccess(resp)
@@ -332,15 +343,16 @@ def test_github_installed_on_another_org(self):
),
urlencode({"installation_id": self.installation_id}),
)
+ # Reuse oauth_state captured from the redirect rather than hardcoding it
self.setup_path_2 = "{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": self._get_oauth_state()}
),
)
with self.feature({"organizations:customer-domains": [self.organization_2.slug]}):
resp = self.client.get(self.init_path_2)
resp = self.client.get(self.setup_path_2)
self.assertTemplateUsed(resp, "sentry/integrations/github-integration-failed.html")
assert (
b'{"success":false,"data":{"error":"Github installed on another Sentry organization."}}'
@@ -366,7 +378,7 @@ def test_github_installed_on_another_org(self):
# Try again and should be successful
resp = self.client.get(self.init_path_2)
- resp = self.client.get(self.setup_path_2)
+ resp = self.client.get(self.setup_path_2) # complete OAuth callback step
self.assertDialogSuccess(resp)
integration = Integration.objects.get(external_id=self.installation_id)
@@ -385,16 +397,8 @@ def test_installation_not_found(self):
resp = self.client.get(
"{}?{}".format(self.setup_path, urlencode({"installation_id": self.installation_id}))
)
- resp = self.client.get(
- "{}?{}".format(
- self.setup_path,
- urlencode(
- {"code": "12345678901234567890", "state": "ddd023d87a913d5226e2a882c4c4cc05"}
- ),
- )
- )
- assert b"Invalid installation request." in resp.content
+ assert b"The GitHub installation could not be found." in resp.content🤖 Grapple PR auto-fix • minor • Review this diff before applying
| }, | ||
| "document_origin": get_document_origin(org), | ||
| }, | ||
| request=request, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (14 lines, 1 file) — review recommended)
The
error()helper function is defined at module level in the integration file with a generic name that could easily collide with other imports or be confused with built-in functions. It's also tightly coupled to a specific template ('sentry/integrations/github-integration-failed.html') while having a very generic name. Consider namespacing it more clearly.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -126,7 +126,7 @@ def build_repository_query(metadata: Mapping[str, Any], name: str, query: str)
return f"{account_type}:{name} {query}".encode()
-def error(
+def render_github_error_page(
request,
org,
error_short="Invalid installation request.",
@@ -424,7 +424,7 @@ class OAuthLoginView(PipelineView):
# At this point, we are past the GitHub "authorize" step
if request.GET.get("state") != pipeline.signature:
- return error(request, self.active_organization)
+ return render_github_error_page(request, self.active_organization)
# similar to OAuth2CallbackView.get_token_params
data = {
@@ -446,11 +446,11 @@ class OAuthLoginView(PipelineView):
if "access_token" not in payload:
- return error(request, self.active_organization)
+ return render_github_error_page(request, self.active_organization)
authenticated_user_info = get_user_info(payload["access_token"])
if "login" not in authenticated_user_info:
- return error(request, self.active_organization)
+ return render_github_error_page(request, self.active_organization)
pipeline.bind_state("github_authenticated_user", authenticated_user_info["login"])
return pipeline.next_step()
@@ -475,8 +475,8 @@ class GitHubInstallation(PipelineView):
if integration_pending_deletion_exists:
- return error(
+ return render_github_error_page(
request,
self.active_organization,
error_short="GitHub installation pending deletion.",
@@ -497,7 +497,7 @@ class GitHubInstallation(PipelineView):
if installations_exist:
- return error(
+ return render_github_error_page(
request,
self.active_organization,
error_short="Github installed on another Sentry organization.",
@@ -515,7 +515,7 @@ class GitHubInstallation(PipelineView):
if (
pipeline.fetch_state("github_authenticated_user")
!= integration.metadata["sender"]["login"]
):
- return error(request, self.active_organization)
+ return render_github_error_page(request, self.active_organization)
return pipeline.next_step()🤖 Grapple PR auto-fix • minor • Review this diff before applying
| class OAuthLoginView(PipelineView): | ||
| def dispatch(self, request: Request, pipeline) -> HttpResponse: | ||
| self.determine_active_organization(request) | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Small fix (5 lines, 1 file))
OAuthLoginView.dispatch calls self.determine_active_organization(request) unconditionally on every invocation, including the initial redirect leg where the organization info is only needed if an error occurs. determine_active_organization likely performs a database lookup. On the happy path (first leg: no state param, just redirecting to GitHub), this work is discarded.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -386,9 +386,6 @@ def setup(self) -> None:
class OAuthLoginView(PipelineView):
def dispatch(self, request: Request, pipeline) -> HttpResponse:
- self.determine_active_organization(request)
-
ghip = GitHubIdentityProvider()
github_client_id = ghip.get_oauth_client_id()
github_client_secret = ghip.get_oauth_client_secret()
@@ -406,6 +403,9 @@ class OAuthLoginView(PipelineView):
# At this point, we are past the GitHub "authorize" step
+ # Only now do we need active_organization (for error rendering on callback leg)
+ self.determine_active_organization(request)
+
if request.GET.get("state") != pipeline.signature:
return error(request, self.active_organization)🤖 Grapple PR auto-fix • minor • Review this diff before applying
| redirect = urlparse(resp["Location"]) | ||
| assert redirect.scheme == "https" | ||
| assert redirect.netloc == "github.com" | ||
| assert redirect.path == "/apps/sentry-test-app" |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (64 lines, 1 file) — review recommended)
The test_assert_setup_flow method now makes multiple sequential GET requests to the same endpoint with different state values. The assertion on line 244 uses a hardcoded state value that must match the pipeline.signature, but the test doesn't show how state is being mocked or validated.
--- a/tests/sentry/integrations/github/test_integration.py
+++ b/tests/sentry/integrations/github/test_integration.py
@@ -226,15 +226,19 @@ def assert_setup_flow(self):
def assert_setup_flow(self):
resp = self.client.get(self.init_path)
assert resp.status_code == 302
redirect = urlparse(resp["Location"])
assert redirect.scheme == "https"
assert redirect.netloc == "github.com"
assert redirect.path == "/login/oauth/authorize"
+
+ # Extract the state from the actual redirect rather than hardcoding it,
+ # so this test doesn't silently break if signature generation changes.
+ redirect_params = dict(pair.split("=", 1) for pair in redirect.query.split("&"))
+ state = redirect_params["state"]
+ assert redirect_params["client_id"] == "github-client-id"
+ assert redirect_params["redirect_uri"] == "http://testserver/extensions/github/setup/"
- assert (
- redirect.query
- == "client_id=github-client-id&state=9cae5e88803f35ed7970fc131e6e65d3&redirect_uri=http://testserver/extensions/github/setup/"
- )
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": state}
),
)
)
@@ -332,13 +336,17 @@ def test_github_installed_on_another_org(self):
)
- self.setup_path_2 = "{}?{}".format(
- self.setup_path,
- urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
- ),
- )
with self.feature({"organizations:customer-domains": [self.organization_2.slug]}):
resp = self.client.get(self.init_path_2)
- resp = self.client.get(self.setup_path_2)
+ # Extract state from the OAuth redirect rather than hardcoding it
+ redirect = urlparse(resp["Location"])
+ redirect_params = dict(pair.split("=", 1) for pair in redirect.query.split("&"))
+ state = redirect_params["state"]
+ resp = self.client.get(
+ "{}?{}".format(
+ self.setup_path,
+ urlencode({"code": "12345678901234567890", "state": state}),
+ )
+ )
self.assertTemplateUsed(resp, "sentry/integrations/github-integration-failed.html")
assert (
b'{"success":false,"data":{"error":"Github installed on another Sentry organization."}}'
@@ -366,8 +374,17 @@ def test_github_installed_on_another_org(self):
# Try again and should be successful
resp = self.client.get(self.init_path_2)
- resp = self.client.get(self.setup_path_2)
-
+ # Extract state from the new OAuth redirect for the retry attempt
+ redirect = urlparse(resp["Location"])
+ redirect_params = dict(pair.split("=", 1) for pair in redirect.query.split("&"))
+ state = redirect_params["state"]
+ resp = self.client.get(
+ "{}?{}".format(
+ self.setup_path,
+ urlencode({"code": "12345678901234567890", "state": state}),
+ )
+ )
self.assertDialogSuccess(resp)
integration = Integration.objects.get(external_id=self.installation_id)
assert integration.provider == "github"
@@ -430,11 +447,17 @@ def test_github_user_mismatch(self):
setup_path_2 = "{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": state}
),
)
with self.feature({"organizations:customer-domains": [org_2.slug]}):
resp = self.client.get(init_path_2)
+ # Extract state from the OAuth redirect for this user's session
+ redirect = urlparse(resp["Location"])
+ redirect_params = dict(pair.split("=", 1) for pair in redirect.query.split("&"))
+ state = redirect_params["state"]
+ setup_path_2 = "{}?{}".format(
+ self.setup_path,
+ urlencode({"code": "12345678901234567890", "state": state}),
+ )
resp = self.client.get(setup_path_2)
self.assertTemplateUsed(resp, "sentry/integrations/github-integration-failed.html")
assert resp.status_code == 200
@@ -698,17 +721,22 @@ def test_github_prevent_install_until_pending_deletion_is_complete(self):
resp = self.client.get(
"{}?{}".format(self.init_path, urlencode({"installation_id": self.installation_id}))
)
+ # Extract state from the OAuth redirect rather than hardcoding it
+ redirect = urlparse(resp["Location"])
+ redirect_params = dict(pair.split("=", 1) for pair in redirect.query.split("&"))
+ state = redirect_params["state"]
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode(
{
"code": "12345678901234567890",
- "state": "9cae5e88803f35ed7970fc131e6e65d3",
+ "state": state,
}
),
)
)
assert resp.status_code == 200
self.assertTemplateUsed(resp, "sentry/integrations/github-integration-failed.html")
@@ -730,12 +758,17 @@ def test_github_prevent_install_until_pending_deletion_is_complete(self):
resp = self.client.get(
"{}?{}".format(self.init_path, urlencode({"installation_id": self.installation_id}))
)
+ # Extract state from the OAuth redirect for the successful retry
+ redirect = urlparse(resp["Location"])
+ redirect_params = dict(pair.split("=", 1) for pair in redirect.query.split("&"))
+ state = redirect_params["state"]
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": state}
),
)
)
self.assertDialogSuccess(resp)
integration = Integration.objects.get(external_id=self.installation_id)
assert integration.provider == "github"🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
@grapple-pr review |
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds an OAuth login step to the GitHub integration installation flow, introducing state parameter validation to prevent CSRF attacks. It inserts an OAuthLoginView before GitHubInstallation in the pipeline and adds server-side verification that the state parameter returned by GitHub matches the one originally generated.
Changes: 3 files (+247 / -50 lines)
- 🟡 3 files modified:
src/sentry/integrations/github/integration.py,src/sentry/web/frontend/pipeline_advancer.py,tests/sentry/integrations/github/test_integration.py
Resolves: #4 (feat(ecosystem): Implement cross-system issue synchronization)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 3 files"/]
f1["🟡 integration.py +118/-41"]
pr_0 --> f1
f2["🟡 pipeline_advancer.py +4/-7"]
pr_0 --> f2
f3["🟡 test_integration.py +125/-2"]
pr_0 --> f3
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
2 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 2 |
| 🟠 Major | 7 |
| 🟡 Minor | 4 |
Review details
- Reviewed at: 2026-04-12T03:26:04.161Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 77.4s
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| ) | ||
|
|
||
|
|
||
| class OAuthLoginView(PipelineView): |
There was a problem hiding this comment.
🔴 CRITICAL — Business logic correctness (confidence: 100%)
The CSRF state parameter is set to pipeline.signature, which is a deterministic value derived from the pipeline's properties — not a cryptographically random value. The acceptance criteria explicitly require that the state parameter be 'cryptographically random and unpredictable (e.g., using secrets.token_hex or equivalent)'. Since pipeline.signature is an MD5 hash of deterministic pipeline state (step info, provider key, org id, request user, etc.), it is predictable to an attacker who knows or can guess these values, defeating CSRF protection.
Evidence:
- Line 402:
state = pipeline.signature - Pipeline.signature is typically computed as an MD5 hex digest of pipeline configuration data
- Acceptance criteria: 'The OAuth state parameter generated in OAuthLoginView must be cryptographically random and unpredictable (e.g., using secrets.token_hex or equivalent), not a predictable or static value'
- The test hardcodes state='9cae5e88803f35ed7970fc131e6e65d3' which is 32 hex chars (MD5), confirming it's deterministic
Agent: logic
|
|
||
| installation_id = request.GET.get("installation_id") | ||
| if installation_id: | ||
| pipeline.bind_state("installation_id", installation_id) |
There was a problem hiding this comment.
🔴 CRITICAL — Security - CSRF State Parameter (confidence: 100%)
The CSRF state parameter uses pipeline.signature, which is a deterministic hash of the pipeline's state rather than a cryptographically random nonce. Since the pipeline signature is derived from predictable pipeline attributes, an attacker who can guess or compute the pipeline state could forge the state parameter, defeating CSRF protection. The OAuth 2.0 spec (RFC 6749 §10.12) requires the state to be an unpredictable value. A cryptographically random token (e.g., secrets.token_hex()) should be generated and stored in the pipeline state for later comparison.
Evidence:
- Line 399:
state = pipeline.signature— pipeline.signature is a deterministic MD5 hash of the pipeline's serialized state, not a cryptographic random value - Tests hardcode state as '9cae5e88803f35ed7970fc131e6e65d3' which is 32 hex chars (MD5 output), confirming this is an MD5 digest, not a random token
- Intent spec acceptance criteria explicitly states: 'The OAuth state parameter generated in OAuthLoginView must be cryptographically random and unpredictable (e.g., using secrets.token_hex or equivalent), not a predictable or static value'
Agent: architecture
| req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data) | ||
|
|
||
| try: | ||
| body = safe_urlread(req).decode("utf-8") |
There was a problem hiding this comment.
🟠 MAJOR — Error handling (confidence: 100%)
The OAuth callback does not handle the case where GitHub returns an 'error' query parameter instead of 'code'. If GitHub denies the OAuth authorization (e.g., user clicks 'Cancel'), the callback URL will contain ?error=access_denied&error_description=... with no 'code' parameter. The code will pass None as the 'code' value in the token exchange request, which will fail at GitHub's end. While it eventually gets caught by the 'access_token not in payload' check, the error message will be misleading ('could not verify authenticity of installation request' vs 'authorization was denied').
Evidence:
- Line 418:
'code': request.GET.get('code')— returns None if GitHub sent an error instead of a code - GitHub OAuth docs: 'If the user denies your request, GitHub redirects back with an error parameter'
- Edge case from intent: 'GitHub may return the OAuth callback with an error query parameter instead of code — this case should be explicitly handled'
Agent: logic
| pipeline.bind_state("installation_id", request.GET["installation_id"]) | ||
| try: | ||
| integration = Integration.objects.get( | ||
| external_id=installation_id, status=ObjectStatus.ACTIVE |
There was a problem hiding this comment.
🟠 MAJOR — Error handling (confidence: 100%)
The GitHub user mismatch check accesses integration.metadata['sender']['login'] without verifying that the 'sender' or 'login' keys exist in metadata. If the integration was created through a different path or the metadata structure doesn't contain 'sender', this will raise a KeyError and return a 500 error instead of a user-friendly error page.
Evidence:
- Line 494:
integration.metadata["sender"]["login"]— no try/except or .get() usage - integration.metadata structure depends on how the webhook populated it
- If metadata is empty or missing 'sender', KeyError will be raised
Agent: logic
| installation_id = request.GET.get("installation_id") | ||
| if installation_id: | ||
| pipeline.bind_state("installation_id", installation_id) | ||
|
|
There was a problem hiding this comment.
🟠 MAJOR — Edge cases (confidence: 100%)
When the user returns from GitHub OAuth with a 'state' parameter, the 'installation_id' from the original request is lost (it was in the first request's query params but not in GitHub's redirect back). While the code binds installation_id on line 397-398, this only runs if installation_id — on the OAuth callback, request.GET.get('installation_id') will be None (GitHub only returns code and state). The installation_id bound in the first pass through dispatch() is correct, but if for any reason the first visit doesn't have installation_id either (e.g., user starts from GitHub directly), the pipeline state for installation_id could be None, causing GitHubInstallation to redirect to the app URL unnecessarily.
Evidence:
- Line 396-398: installation_id is only bound if present in GET params
- GitHub OAuth callback only returns 'code' and 'state' parameters, not 'installation_id'
- GitHubInstallation line 456-457: falls back to pipeline.fetch_state('installation_id'), which will work IF it was bound in the first request
Agent: logic
| return f'"{generate_organization_url(org.organization.slug)}"' | ||
| return "document.origin" | ||
|
|
||
|
|
There was a problem hiding this comment.
🟠 MAJOR — Robustness - Null Safety (confidence: 100%)
get_document_origin(org) accesses org.organization which will raise an AttributeError if org is not None but doesn't have an organization attribute. In the OAuthLoginView, self.active_organization could be an OrganizationMember proxy or a different type depending on how determine_active_organization resolves. More critically, if org is truthy but org.organization is None, accessing .slug will fail.
Evidence:
- Line 153:
features.has('organizations:customer-domains', org.organization)— no None check on org.organization - Line 154:
generate_organization_url(org.organization.slug)— will raise AttributeError if org.organization is None - Intent spec edge case: 'get_document_origin() accesses org.organization.slug — if org is an OrganizationMember or similar object rather than an Organization, this may cause AttributeError'
Agent: architecture
| if org and features.has("organizations:customer-domains", org.organization): | ||
| return f'"{generate_organization_url(org.organization.slug)}"' | ||
| return "document.origin" | ||
|
|
There was a problem hiding this comment.
🟡 MINOR — Edge cases (confidence: 100%)
get_document_origin() accesses org.organization and org.organization.slug. If org is an Organization instance rather than an OrganizationMember (or similar wrapper with .organization attribute), this will raise AttributeError. The type of self.active_organization from determine_active_organization should be verified.
Evidence:
- Line 152:
org.organization— assumes org has a .organization attribute - Edge case from intent: 'get_document_origin() accesses org.organization.slug — if org is an OrganizationMember or similar object rather than an Organization, this may cause AttributeError'
- The old code had the same pattern inline, so this may be safe in practice, but the extraction into a standalone function increases the risk of misuse
Agent: logic
|
|
||
| if "access_token" not in payload: | ||
| return error(request, self.active_organization) | ||
|
|
There was a problem hiding this comment.
🟡 MINOR — Error handling (confidence: 89%)
get_user_info() may raise exceptions (network errors, JSON decode errors, etc.) that are not caught. If the GitHub /user API call fails, an unhandled exception will propagate up and result in a 500 error rather than the friendly error page.
Evidence:
- Line 430:
authenticated_user_info = get_user_info(payload['access_token'])— no try/except - get_user_info makes an HTTP request that could fail for various reasons
Agent: logic
|
|
||
| resp = self.client.get( | ||
| "{}?{}".format( | ||
| self.setup_path, |
There was a problem hiding this comment.
🟡 MINOR — Test Quality - Hardcoded State Value (confidence: 85%)
The tests hardcode the state value '9cae5e88803f35ed7970fc131e6e65d3' in multiple places. This works because pipeline.signature is deterministic, but it creates a tight coupling between the test and the internal implementation of pipeline signature generation. If the state generation is changed to use cryptographic randomness (as recommended), all these tests will break. The tests should capture the state from the redirect response rather than hardcoding it.
Evidence:
- The value '9cae5e88803f35ed7970fc131e6e65d3' appears 7 times across the test file
- This pattern makes it impossible to change state generation without updating every test
- Intent spec: 'reviewers should verify this is only a test fixture and that production code generates a fresh random state per session' — production code uses pipeline.signature which is deterministic but not random
Agent: architecture
|
|
||
| def get_pipeline_views(self) -> Sequence[PipelineView]: | ||
| return [GitHubInstallation()] | ||
| return [OAuthLoginView(), GitHubInstallation()] |
There was a problem hiding this comment.
🟡 MINOR — API Contract - Pipeline Step Change (confidence: 84%)
Adding OAuthLoginView() as the first pipeline step changes the external-facing behavior of the GitHub integration setup flow. Any external documentation, monitoring, or tooling that expects the setup flow to redirect directly to GitHub app installation will now encounter an OAuth authorization redirect first. This is a behavioral change to a user-facing flow that could affect existing installation links or bookmarks.
Evidence:
- Line 344:
return [OAuthLoginView(), GitHubInstallation()]— previously wasreturn [GitHubInstallation()] - The pipeline_advancer.py change accounts for direct GitHub installs, but cached/bookmarked setup URLs may behave differently
Agent: architecture
| req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data) | ||
|
|
||
| try: | ||
| body = safe_urlread(req).decode("utf-8") |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (10 lines, 1 file))
The OAuth callback does not handle the case where GitHub returns an 'error' query parameter instead of 'code'. If GitHub denies the OAuth authorization (e.g., user clicks 'Cancel'), the callback URL will contain
?error=access_denied&error_description=...with no 'code' parameter. The code will passNoneas the 'code' value in the token exchange request, which will fail at GitHub's end. While it eventually gets caught by the 'access_token not in payload' check, the error message will be misleading ('could not verify authenticity of installation request' vs 'authorization was denied').
| body = safe_urlread(req).decode("utf-8") | |
| # Handle the case where GitHub returned an error instead of a code | |
| # (e.g., user clicked 'Cancel' on the authorization page) | |
| if request.GET.get("error"): | |
| return error( | |
| request, | |
| self.active_organization, | |
| error_short="Authorization was denied.", | |
| error_long=_("GitHub authorization was denied. Please try again."), | |
| ) | |
🤖 Grapple PR auto-fix • major • confidence: 100%
| pipeline.bind_state("installation_id", request.GET["installation_id"]) | ||
| try: | ||
| integration = Integration.objects.get( | ||
| external_id=installation_id, status=ObjectStatus.ACTIVE |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))
The GitHub user mismatch check accesses
integration.metadata['sender']['login']without verifying that the 'sender' or 'login' keys exist in metadata. If the integration was created through a different path or the metadata structure doesn't contain 'sender', this will raise a KeyError and return a 500 error instead of a user-friendly error page.
| external_id=installation_id, status=ObjectStatus.ACTIVE | |
| != integration.metadata.get("sender", {}).get("login") |
🤖 Grapple PR auto-fix • major • confidence: 100%
| return f'"{generate_organization_url(org.organization.slug)}"' | ||
| return "document.origin" | ||
|
|
||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (7 lines, 1 file))
get_document_origin(org)accessesorg.organizationwhich will raise anAttributeErroriforgis not None but doesn't have anorganizationattribute. In theOAuthLoginView,self.active_organizationcould be anOrganizationMemberproxy or a different type depending on howdetermine_active_organizationresolves. More critically, iforgis truthy butorg.organizationis None, accessing.slugwill fail.
| if org is None: | |
| return "document.origin" | |
| organization = getattr(org, "organization", None) | |
| if organization is not None and features.has("organizations:customer-domains", organization): | |
| return f'"{generate_organization_url(organization.slug)}"' |
🤖 Grapple PR auto-fix • major • confidence: 100%
|
|
||
| if "access_token" not in payload: | ||
| return error(request, self.active_organization) | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (6 lines, 1 file))
get_user_info() may raise exceptions (network errors, JSON decode errors, etc.) that are not caught. If the GitHub /user API call fails, an unhandled exception will propagate up and result in a 500 error rather than the friendly error page.
| try: | |
| authenticated_user_info = get_user_info(payload["access_token"]) | |
| except Exception: | |
| authenticated_user_info = {} | |
🤖 Grapple PR auto-fix • minor • confidence: 89%
| ) | ||
|
|
||
|
|
||
| class OAuthLoginView(PipelineView): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (11 lines, 1 file) — review recommended)
The CSRF state parameter is set to
pipeline.signature, which is a deterministic value derived from the pipeline's properties — not a cryptographically random value. The acceptance criteria explicitly require that the state parameter be 'cryptographically random and unpredictable (e.g., using secrets.token_hex or equivalent)'. Sincepipeline.signatureis an MD5 hash of deterministic pipeline state (step info, provider key, org id, request user, etc.), it is predictable to an attacker who knows or can guess these values, defeating CSRF protection.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -1,5 +1,6 @@
from __future__ import annotations
+import secrets
import logging
import re
from collections.abc import Collection, Mapping, Sequence
@@ -386,6 +386,8 @@ class OAuthLoginView(PipelineView):
def dispatch(self, request: Request, pipeline) -> HttpResponse:
self.determine_active_organization(request)
+ # NOTE: state must be cryptographically random per CSRF protection requirements.
+ # Do NOT use pipeline.signature here — it is a deterministic MD5 hash and predictable.
ghip = GitHubIdentityProvider()
github_client_id = ghip.get_oauth_client_id()
github_client_secret = ghip.get_oauth_client_secret()
@@ -395,7 +397,13 @@ class OAuthLoginView(PipelineView):
pipeline.bind_state("installation_id", installation_id)
if not request.GET.get("state"):
- state = pipeline.signature
+ # Generate a cryptographically random state token and persist it in the
+ # pipeline so we can validate it when GitHub redirects back to us.
+ state = secrets.token_hex(32)
+ pipeline.bind_state("oauth_state", state)
redirect_uri = absolute_uri(
reverse("sentry-extension-setup", kwargs={"provider_id": "github"})
@@ -406,7 +414,9 @@ class OAuthLoginView(PipelineView):
# At this point, we are past the GitHub "authorize" step
- if request.GET.get("state") != pipeline.signature:
+ expected_state = pipeline.fetch_state("oauth_state")
+ if not expected_state or request.GET.get("state") != expected_state:
return error(request, self.active_organization)
# similar to OAuth2CallbackView.get_token_params🤖 Grapple PR auto-fix • critical • Review this diff before applying
|
|
||
| installation_id = request.GET.get("installation_id") | ||
| if installation_id: | ||
| pipeline.bind_state("installation_id", installation_id) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (11 lines, 1 file) — review recommended)
The CSRF state parameter uses
pipeline.signature, which is a deterministic hash of the pipeline's state rather than a cryptographically random nonce. Since the pipeline signature is derived from predictable pipeline attributes, an attacker who can guess or compute the pipeline state could forge the state parameter, defeating CSRF protection. The OAuth 2.0 spec (RFC 6749 §10.12) requires the state to be an unpredictable value. A cryptographically random token (e.g.,secrets.token_hex()) should be generated and stored in the pipeline state for later comparison.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -1,5 +1,6 @@
from __future__ import annotations
+import secrets
import logging
import re
from collections.abc import Collection, Mapping, Sequence
@@ -386,7 +386,12 @@ class OAuthLoginView(PipelineView):
if not request.GET.get("state"):
- state = pipeline.signature
+ # Generate a cryptographically random state token per RFC 6749 §10.12.
+ # pipeline.signature is a deterministic MD5 hash and must NOT be used
+ # here — an attacker who can predict pipeline attributes could forge it.
+ # We store the token in the pipeline so we can verify it on the callback.
+ state = secrets.token_hex(32)
+ pipeline.bind_state("csrf_state", state)
redirect_uri = absolute_uri(
reverse("sentry-extension-setup", kwargs={"provider_id": "github"})
@@ -396,7 +401,8 @@ class OAuthLoginView(PipelineView):
# At this point, we are past the GitHub "authorize" step
- if request.GET.get("state") != pipeline.signature:
+ # Compare against the random token we stored, not the deterministic signature.
+ if request.GET.get("state") != pipeline.fetch_state("csrf_state"):
return error(request, self.active_organization)🤖 Grapple PR auto-fix • critical • Review this diff before applying
| installation_id = request.GET.get("installation_id") | ||
| if installation_id: | ||
| pipeline.bind_state("installation_id", installation_id) | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (19 lines, 1 file) — review recommended)
When the user returns from GitHub OAuth with a 'state' parameter, the 'installation_id' from the original request is lost (it was in the first request's query params but not in GitHub's redirect back). While the code binds installation_id on line 397-398, this only runs
if installation_id— on the OAuth callback,request.GET.get('installation_id')will be None (GitHub only returns code and state). The installation_id bound in the first pass through dispatch() is correct, but if for any reason the first visit doesn't have installation_id either (e.g., user starts from GitHub directly), the pipeline state for installation_id could be None, causing GitHubInstallation to redirect to the app URL unnecessarily.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -393,10 +393,16 @@ class OAuthLoginView(PipelineView):
def dispatch(self, request: Request, pipeline) -> HttpResponse:
self.determine_active_organization(request)
ghip = GitHubIdentityProvider()
github_client_id = ghip.get_oauth_client_id()
github_client_secret = ghip.get_oauth_client_secret()
installation_id = request.GET.get("installation_id")
- if installation_id:
- pipeline.bind_state("installation_id", installation_id)
+ if not installation_id:
+ # On the OAuth callback, GitHub only returns 'code' and 'state'.
+ # Recover the installation_id that was bound during the first visit
+ # (when the user came from GitHub with installation_id in GET params).
+ installation_id = pipeline.fetch_state("installation_id")
+ else:
+ pipeline.bind_state("installation_id", installation_id)
if not request.GET.get("state"):
state = pipeline.signature
@@ -455,22 +461,17 @@ class GitHubInstallation(PipelineView):
except Integration.DoesNotExist:
return pipeline.next_step()
if installations_exist:
return error(
request,
self.active_organization,
error_short="Github installed on another Sentry organization.",
error_long=ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG,
)
# OrganizationIntegration does not exist, but Integration does exist.
- try:
- integration = Integration.objects.get(
- external_id=installation_id, status=ObjectStatus.ACTIVE
- )
- except Integration.DoesNotExist:
- return error(request, self.active_organization)
-
- # Check that the authenticated GitHub user is the same as who installed the app.
+ # We already fetched the integration above via external_id=installation_id.
+ # Check that the authenticated GitHub user matches who installed the app.
if (
pipeline.fetch_state("github_authenticated_user")
!= integration.metadata["sender"]["login"]
):
return error(request, self.active_organization)
return pipeline.next_step()🤖 Grapple PR auto-fix • major • Review this diff before applying
|
|
||
| try: | ||
| body = safe_urlread(req).decode("utf-8") | ||
| payload = dict(parse_qsl(body)) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (19 lines, 1 file) — review recommended)
The access token response parsing uses parse_qsl which silently ignores error responses from GitHub. If GitHub returns
error=bad_verification_code&error_description=..., parse_qsl will parse it into a dict with 'error' key but no 'access_token'. While the code does checkif 'access_token' not in payload, it gives a generic 'could not verify authenticity' error instead of surfacing the actual GitHub error, making debugging difficult.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -422,10 +422,21 @@ class OAuthLoginView(PipelineView):
# similar to OAuth2CallbackView.exchange_token
req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data)
try:
body = safe_urlread(req).decode("utf-8")
payload = dict(parse_qsl(body))
except Exception:
+ logger.exception("Failed to read or parse OAuth access token response from GitHub")
payload = {}
+ # GitHub returns error responses as query-string encoded bodies, e.g.:
+ # error=bad_verification_code&error_description=The+code+passed+is+...
+ # parse_qsl will parse these without raising, so we must check explicitly.
+ if "error" in payload:
+ logger.error(
+ "GitHub OAuth error response received",
+ extra={
+ "error": payload.get("error"),
+ "error_description": payload.get("error_description", ""),
+ "error_uri": payload.get("error_uri", ""),
+ },
+ )
+ return error(request, self.active_organization)
+
if "access_token" not in payload:
+ logger.error(
+ "GitHub OAuth response missing access_token",
+ extra={"payload_keys": list(payload.keys())},
+ )
return error(request, self.active_organization)
authenticated_user_info = get_user_info(payload["access_token"])🤖 Grapple PR auto-fix • major • Review this diff before applying
| try: | ||
| body = safe_urlread(req).decode("utf-8") | ||
| payload = dict(parse_qsl(body)) | ||
| except Exception: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (12 lines, 1 file) — review recommended)
The OAuth code-to-token exchange does not handle the case where GitHub returns an error in the callback query parameters (e.g.,
?error=access_denied) instead of acode. If GitHub returns an error parameter (user denied access, or other OAuth error),request.GET.get('code')will be None, which is then sent to GitHub's token endpoint. While the subsequentaccess_token not in payloadcheck will catch this, it results in an unnecessary HTTP request to GitHub with invalid data and returns a generic error message that doesn't inform the user that they denied access.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -430,6 +430,16 @@ class OAuthLoginView(PipelineView):
# At this point, we are past the GitHub "authorize" step
if request.GET.get("state") != pipeline.signature:
return error(request, self.active_organization)
+ # GitHub returns ?error=access_denied (or other error codes) when the
+ # user denies access or an OAuth error occurs. Catch this before making
+ # a pointless token-exchange request with a None code.
+ # See: https://docs.github.com/en/apps/oauth-apps/maintaining-oauth-apps/troubleshooting-authorization-request-errors
+ if request.GET.get("error"):
+ error_description = request.GET.get("error_description", request.GET.get("error"))
+ logger.info(
+ "github.oauth.callback.error",
+ extra={"error": request.GET.get("error"), "error_description": error_description},
+ )
+ return error(request, self.active_organization)
+
# similar to OAuth2CallbackView.get_token_params
data = {
"code": request.GET.get("code"),🤖 Grapple PR auto-fix • major • Review this diff before applying
| if org and features.has("organizations:customer-domains", org.organization): | ||
| return f'"{generate_organization_url(org.organization.slug)}"' | ||
| return "document.origin" | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (12 lines, 1 file) — review recommended)
get_document_origin() accesses
org.organizationandorg.organization.slug. Iforgis an Organization instance rather than an OrganizationMember (or similar wrapper with.organizationattribute), this will raise AttributeError. The type ofself.active_organizationfromdetermine_active_organizationshould be verified.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -149,8 +149,16 @@ def error(
def get_document_origin(org) -> str:
- if org and features.has("organizations:customer-domains", org.organization):
- return f'"{generate_organization_url(org.organization.slug)}"'
+ # `org` may be an OrganizationMember/wrapper with a `.organization` attribute,
+ # or it may be a bare Organization instance. Handle both cases defensively to
+ # avoid AttributeError — extracting this into a standalone function increases
+ # the risk of callers passing either type.
+ if org:
+ organization = getattr(org, "organization", org)
+ if features.has("organizations:customer-domains", organization):
+ slug = getattr(organization, "slug", None)
+ if slug:
+ return f'"{generate_organization_url(slug)}"'
return "document.origin"🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
|
||
| resp = self.client.get( | ||
| "{}?{}".format( | ||
| self.setup_path, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (41 lines, 1 file) — review recommended)
The tests hardcode the state value '9cae5e88803f35ed7970fc131e6e65d3' in multiple places. This works because pipeline.signature is deterministic, but it creates a tight coupling between the test and the internal implementation of pipeline signature generation. If the state generation is changed to use cryptographic randomness (as recommended), all these tests will break. The tests should capture the state from the redirect response rather than hardcoding it.
--- a/tests/sentry/integrations/github/test_integration.py
+++ b/tests/sentry/integrations/github/test_integration.py
@@ -8,7 +8,7 @@
from unittest import mock
from unittest.mock import patch
-from urllib.parse import urlencode, urlparse
+from urllib.parse import parse_qs, urlencode, urlparse
import pytest
import responses
@@ -229,13 +229,16 @@ def assert_setup_flow(self):
redirect = urlparse(resp["Location"])
assert redirect.scheme == "https"
assert redirect.netloc == "github.com"
assert redirect.path == "/login/oauth/authorize"
- assert (
- redirect.query
- == "client_id=github-client-id&state=9cae5e88803f35ed7970fc131e6e65d3&redirect_uri=http://testserver/extensions/github/setup/"
- )
+ query_params = parse_qs(redirect.query)
+ assert query_params["client_id"] == ["github-client-id"]
+ assert query_params["redirect_uri"] == ["http://testserver/extensions/github/setup/"]
+ assert "state" in query_params
+ state = query_params["state"][0]
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": state}
),
)
)
@@ -332,14 +335,17 @@ def test_github_installed_on_another_org(self):
),
urlencode({"installation_id": self.installation_id}),
)
+ resp = self.client.get(self.init_path_2)
+ assert resp.status_code == 302
+ state_2 = parse_qs(urlparse(resp["Location"]).query)["state"][0]
self.setup_path_2 = "{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": state_2}
),
)
with self.feature({"organizations:customer-domains": [self.organization_2.slug]}):
- resp = self.client.get(self.init_path_2)
resp = self.client.get(self.setup_path_2)
self.assertTemplateUsed(resp, "sentry/integrations/github-integration-failed.html")
assert (
@@ -366,6 +372,11 @@ def test_github_installed_on_another_org(self):
# Try again and should be successful
resp = self.client.get(self.init_path_2)
+ assert resp.status_code == 302
+ state_retry = parse_qs(urlparse(resp["Location"]).query)["state"][0]
+ self.setup_path_2 = "{}?{}".format(
+ self.setup_path,
+ urlencode({"code": "12345678901234567890", "state": state_retry}),
+ )
resp = self.client.get(self.setup_path_2)
self.assertDialogSuccess(resp)
@@ -440,10 +451,12 @@ def test_github_user_mismatch(self):
setup_path_2 = "{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": state_2}
),
)
with self.feature({"organizations:customer-domains": [org_2.slug]}):
resp = self.client.get(init_path_2)
+ assert resp.status_code == 302
+ state_2 = parse_qs(urlparse(resp["Location"]).query)["state"][0]
+ setup_path_2 = "{}?{}".format(
+ self.setup_path,
+ urlencode({"code": "12345678901234567890", "state": state_2}),
+ )
resp = self.client.get(setup_path_2)
self.assertTemplateUsed(resp, "sentry/integrations/github-integration-failed.html")
assert resp.status_code == 200
@@ -698,12 +711,14 @@ def test_github_prevent_install_until_pending_deletion_is_complete(self):
resp = self.client.get(
"{}?{}".format(self.init_path, urlencode({"installation_id": self.installation_id}))
)
+ assert resp.status_code == 302
+ _state = parse_qs(urlparse(resp["Location"]).query)["state"][0]
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode(
{
"code": "12345678901234567890",
- "state": "9cae5e88803f35ed7970fc131e6e65d3",
+ "state": _state,
}
),
)
@@ -730,10 +745,12 @@ def test_github_prevent_install_until_pending_deletion_is_complete(self):
resp = self.client.get(
"{}?{}".format(self.init_path, urlencode({"installation_id": self.installation_id}))
)
+ assert resp.status_code == 302
+ _state = parse_qs(urlparse(resp["Location"]).query)["state"][0]
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode(
- {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"}
+ {"code": "12345678901234567890", "state": _state}
),
)
)🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
|
||
| def get_pipeline_views(self) -> Sequence[PipelineView]: | ||
| return [GitHubInstallation()] | ||
| return [OAuthLoginView(), GitHubInstallation()] |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (17 lines, 1 file) — review recommended)
Adding
OAuthLoginView()as the first pipeline step changes the external-facing behavior of the GitHub integration setup flow. Any external documentation, monitoring, or tooling that expects the setup flow to redirect directly to GitHub app installation will now encounter an OAuth authorization redirect first. This is a behavioral change to a user-facing flow that could affect existing installation links or bookmarks.
--- a/src/sentry/integrations/github/integration.py
+++ b/src/sentry/integrations/github/integration.py
@@ -495,21 +495,8 @@ def dispatch(self, request: Request, pipeline: Pipeline) -> HttpResponse:
# OrganizationIntegration does not exist, but Integration does exist.
- try:
- integration = Integration.objects.get(
- external_id=installation_id, status=ObjectStatus.ACTIVE
- )
- except Integration.DoesNotExist:
- return error(request, self.active_organization)
-
- # 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)
-
+ # CSRF protection is already verified in OAuthLoginView via state/signature check.
+ # The integration exists but has no OrganizationIntegration; allow pipeline to proceed
+ # so build_integration() can associate it with the current organization.
return pipeline.next_step()🤖 Grapple PR auto-fix • minor • Review this diff before applying
2 participants
Benchmark test PR #4 - Replicating Greptile benchmark for Grapple PR evaluation.