GitHub OAuth Security Enhancement

[ShashankFC]

User description

Test 4

Summary by CodeRabbit

• New Features

  • Added OAuth-based GitHub login flow integration for seamless authentication

• Bug Fixes

  • Enhanced installation validation to prevent mismatches between authenticated users and installations
  • Improved error messaging and handling for invalid installation requests, conflicts, and pending deletions

• Tests

  • Expanded test coverage for GitHub installation flows and error scenarios

_{✏️ Tip: You can customize this high-level summary in your review settings.}

---

Replicated from ai-code-review-evaluation/sentry-coderabbit#4

---

CodeAnt-AI Description

Validate GitHub user and state during installation to prevent invalid or mismatched installs

What Changed

• Installation flow now performs a silent GitHub OAuth authorize step and verifies the returned state and access token before completing an app install; installs fail with a clear error page when the state is invalid, the access token cannot be obtained, or the authenticated GitHub user is missing.
• Installation is rejected if the GitHub account that performed the install does not match the app install sender recorded by GitHub, showing a user-facing failure page instead of continuing.
• The installation step accepts installation_id from either the initial redirect or pipeline state, and direct GitHub install redirects to the org picker when no pipeline is present.
• Tests updated to exercise OAuth authorize/token exchange, user-mismatch, missing installation, and pending-deletion scenarios; failure responses now include a payload returned to the opener window.

Impact

✅ Clearer GitHub installation failures
✅ Fewer accidental or unauthorized org installs
✅ Fewer installs using replayed or invalid OAuth codes

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

[ShashankFC]

@CodeAnt-AI: review

[codeant-ai]

CodeAnt AI is running the review.

[codeant-ai]

Nitpicks 🔍

🔒 No security issues identified

⚡ Recommended areas for review URL Encoding The OAuth authorize redirect URL is constructed via f-string concatenation with unencoded query parameters (client_id, state, redirect_uri). If any of these values (especially redirect_uri) contain characters that require percent-encoding this could produce an invalid authorization URL. Use proper URL encoding to avoid edge-case failures. Possible Bug The code queries for an Integration instance inside an OrganizationIntegration filter: OrganizationIntegration.objects.filter(integration=Integration.objects.get(...)). This forces a lookup of the Integration object and can cause subtle race/semantics issues (e.g. counting inactive integrations, raising DoesNotExist unexpectedly). It is safer / more efficient to express the filter in a single DB query (e.g. filter on the FK relation like integration__external_id=...) and be explicit about status where appropriate. Behavior Change The code replaced a previously-configurable list (FORWARD_INSTALL_FOR) with a hard-coded equality check against "github". This narrows behavior to only GitHub installs; if other providers need the same forwarding behavior in future, they'll require code changes. Verify this intentional narrowing and update docs/tests if necessary. Test Fragility Tests now assert on a specific responses.calls index (e.g. using responses.calls[2]). The index of calls can change if mocks are added/removed, making the test brittle. Prefer matching the request by URL or using responses library utilities to assert on which request contained specific headers rather than depending on call order.

[codeant-ai]

Suggestion: The code unconditionally accesses integration.metadata["sender"]["login"]; for existing GitHub integrations created before this change (or any integration without a sender key), this will raise a KeyError and cause the installation flow to 500 instead of returning a controlled error response, breaking installs for those cases. [possible bug]

Severity Level: Critical 🚨

- ❌ GitHub install setup 500s for older integrations.
- ❌ Multi-organization GitHub linking can silently fail mid-setup.
- ⚠️ Breaks `sentry-organization-integrations-setup` flow for affected installs.
- ⚠️ Inconsistent with installation endpoint guarding missing sender metadata.

Suggested change

	if (
	pipeline.fetch_state("github_authenticated_user")
	!= integration.metadata["sender"]["login"]
	):
	sender = integration.metadata.get("sender") if isinstance(integration.metadata, dict) else None
	if not sender or "login" not in sender:
	return error(request, self.active_organization)
	if pipeline.fetch_state("github_authenticated_user") != sender["login"]:

Steps of Reproduction ✅

1. Create a GitHub integration via the normal setup flow for an organization, following
`GitHubIntegrationTest.assert_setup_flow` in
`tests/sentry/integrations/github/test_integration.py:226-261`, which exercises the
`sentry-organization-integrations-setup` endpoint and completes installation once (see
`test_basic_flow` at lines 295-310).

2. Observe that the created `Integration` row for provider `"github"` has `metadata`
**without** a `"sender"` key, as asserted in `test_basic_flow` at
`tests/sentry/integrations/github/test_integration.py:301-310` (metadata only contains
`access_token`, `expires_at`, `icon`, `domain_name`, `account_type`).

3. From a second Sentry organization, start the GitHub integration setup using the same
GitHub installation, hitting the same setup endpoint as in
`test_github_installed_on_another_org`
(`tests/sentry/integrations/github/test_integration.py:323-336`), which invokes the
provider's pipeline views `OAuthLoginView` then `GitHubInstallation` (configured in
`src/sentry/integrations/github/integration.py:343-344`).

4. After OAuth login stores `pipeline.bind_state("github_authenticated_user", ...)` in
`OAuthLoginView.dispatch` (`integration.py:389-438`), the pipeline proceeds to
`GitHubInstallation.dispatch` (`integration.py:447-507`), where the check at lines 500-503
evaluates `integration.metadata["sender"]["login"]`; because the existing integration's
`metadata` has no `"sender"` key, this raises a `KeyError` instead of returning a
controlled error, resulting in a 500 response for the integration setup endpoint.

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** src/sentry/integrations/github/integration.py
**Line:** 501:504
**Comment:**
	*Possible Bug: The code unconditionally accesses `integration.metadata["sender"]["login"]`; for existing GitHub integrations created before this change (or any integration without a `sender` key), this will raise a `KeyError` and cause the installation flow to 500 instead of returning a controlled error response, breaking installs for those cases.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

👍 | 👎

[codeant-ai]

CodeAnt AI finished running the review.