GitHub OAuth Security Enhancement

[hussam789]

User description

PR #4

---

PR Type

Bug fix, Enhancement

---

Description

• Add OAuth2 authorization step to GitHub integration installation pipeline

• Validate authenticated GitHub user matches app installer to prevent unauthorized installations

• Verify state parameter and access token validity during OAuth callback

• Refactor error handling into reusable helper function with consistent error responses

---

Diagram Walkthrough

flowchart LR
  A["GitHub App Install"] --> B["OAuthLoginView"]
  B --> C["GitHub OAuth Authorize"]
  C --> D["Validate State Parameter"]
  D --> E["Exchange Code for Token"]
  E --> F["Get Authenticated User"]
  F --> G["GitHubInstallation"]
  G --> H["Verify User Matches Installer"]
  H --> I{Valid?}
  I -->|Yes| J["Complete Installation"]
  I -->|No| K["Error Response"]

Loading

File Walkthrough

	Relevant files
Security, enhancement	integration.py Implement OAuth2 user validation for GitHub app installation src/sentry/integrations/github/integration.py Add new OAuthLoginView pipeline step to handle GitHub OAuth2 authorization before app installation Implement state parameter validation and access token exchange with GitHub OAuth endpoint Retrieve authenticated GitHub user info and validate it matches the app installer Extract error rendering logic into reusable error() helper function and get_document_origin() utility Update GitHubInstallation to fetch installation_id from pipeline state and verify authenticated user matches integration metadata Add new error constant ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST for OAuth validation failures +118/-41
Refactoring	pipeline_advancer.py Simplify GitHub direct install redirect logic                        src/sentry/web/frontend/pipeline_advancer.py Move GitHub direct install redirect logic inline into the handler method Simplify provider check from list-based FORWARD_INSTALL_FOR to direct string comparison Update comment placement for better code organization +4/-7
Tests	test_integration.py Add OAuth validation tests and update existing test flows tests/sentry/integrations/github/test_integration.py Add OAuth access token and user endpoint mocking to _stub_github() method Update assert_setup_flow() to verify OAuth authorize redirect and handle OAuth callback with code/state parameters Add new test test_github_user_mismatch() to verify installation fails when authenticated user differs from app installer Update existing tests to include OAuth callback step with proper code and state parameters Adjust response call indices to account for new OAuth endpoint calls +125/-2

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	OAuth token handling Description: The OAuth access token exchange response is parsed from a URL-encoded body without verifying the HTTP status code or content type, potentially allowing malformed responses to be treated as valid and proceed in the flow. integration.py [423-433] Referred Code 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)
	External call robustness Description: The authenticated GitHub user info is fetched using the access token without explicit timeout or error handling, risking unhandled exceptions or hangs if the GitHub API is slow or fails. integration.py [434-439] Referred Code 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()
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: New OAuth and installation validation paths do not include explicit logging of critical actions (state validation failures, token exchange failures, user mismatch, pending deletion) making audit reconstruction unclear. Referred Code def error( request, org, error_short="Invalid installation request.", error_long=ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST, ): return render_to_response( "sentry/integrations/github-integration-failed.html", context={ "error": error_long, "payload": { "success": False, "data": {"error": _(error_short)}, }, "document_origin": get_document_origin(org), }, request=request, ) def get_document_origin(org) -> str: ... (clipped 358 lines)
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Limited error context: The error() helper returns generic messages without logging contextual details (e.g., missing state, token exchange errors, absent access_token, user info fetch failures), which may hinder production debugging. Referred Code def error( request, org, error_short="Invalid installation request.", error_long=ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST, ): return render_to_response( "sentry/integrations/github-integration-failed.html", context={ "error": error_long, "payload": { "success": False, "data": {"error": _(error_short)}, }, "document_origin": get_document_origin(org), }, request=request, ) def get_document_origin(org) -> str: ... (clipped 288 lines)
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Input validation gaps: OAuth callback relies on GET parameters without explicit format validation or CSRF protection details, and access token/user info errors fall back to generic failures without explicit verification of parameter types or lengths. Referred Code 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 redirect_uri = absolute_uri( reverse("sentry-extension-setup", kwargs={"provider_id": "github"}) ) return self.redirect( f"{ghip.get_oauth_authorize_url()}?client_id={github_client_id}&state={state}&redirect_uri={redirect_uri}" ) ... (clipped 29 lines)

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Improve error handling and logging Improve error handling in the GitHub OAuth token exchange by logging exceptions and explicitly checking for error messages in the response payload. src/sentry/integrations/github/integration.py [422-432] # 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 = {} +except Exception as e: + logging.exception("github.oauth.exchange-token-error", extra={"error": str(e)}) + return error(request, self.active_organization) + +if "error" in payload: + logging.error( + "github.oauth.exchange-token-error", + extra={"error": payload.get("error"), "error_description": payload.get("error_description")}, + ) + return error(request, self.active_organization) if "access_token" not in payload: + logging.error("github.oauth.missing-access-token") return error(request, self.active_organization) Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that swallowing exceptions and not checking for API-level errors makes debugging difficult. Implementing logging and explicit error checks significantly improves the robustness and maintainability of the OAuth flow.	Medium
Security	Strengthen user validation security check Strengthen the user validation security check by explicitly verifying that an authenticated user exists in the pipeline state before comparing it to the app installer's username. src/sentry/integrations/github/integration.py [500-505] # 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"] -): +authenticated_user = pipeline.fetch_state("github_authenticated_user") +if not authenticated_user or authenticated_user != integration.metadata["sender"]["login"]: return error(request, self.active_organization) Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion improves the clarity and explicitness of a security-sensitive check by adding a not authenticated_user condition, making the code more robust against unexpected None values.	Low
More