Skip to content

GitHub OAuth Security Enhancement #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ShashankFC wants to merge 1 commit into
base: oauth-state-vulnerable
Choose a base branch
from
Open

GitHub OAuth Security Enhancement #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 118 additions & 41 deletions src/sentry/integrations/github/integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,19 @@
import re
from collections.abc import Collection, Mapping, Sequence
from typing import Any
from urllib.parse import parse_qsl

from django.http import HttpResponse
from django.urls import reverse
from django.utils.text import slugify
from django.utils.translation import gettext_lazy as _
from rest_framework.request import Request

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,
IntegrationFeatures,
Expand All @@ -35,6 +39,7 @@
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
from sentry.utils.http import absolute_uri
from sentry.web.helpers import render_to_response

from .client import GitHubAppsClient, GitHubClientMixin
Expand Down Expand Up @@ -108,6 +113,9 @@
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."
)
Expand All @@ -118,6 +126,32 @@ def build_repository_query(metadata: Mapping[str, Any], name: str, query: str) -
return f"{account_type}:{name} {query}".encode()


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:
if org and features.has("organizations:customer-domains", org.organization):
return f'"{generate_organization_url(org.organization.slug)}"'
return "document.origin"


# Github App docs and list of available endpoints
# https://docs.github.com/en/rest/apps/installations
# https://docs.github.com/en/rest/overview/endpoints-available-for-github-apps
Expand Down Expand Up @@ -307,7 +341,7 @@ def post_install(
)

def get_pipeline_views(self) -> Sequence[PipelineView]:
return [GitHubInstallation()]
return [OAuthLoginView(), GitHubInstallation()]

def get_installation_info(self, installation_id: str) -> Mapping[str, Any]:
client = self.get_client()
Expand Down Expand Up @@ -352,15 +386,72 @@ 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()

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}"
)

# 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 = {}

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()


class GitHubInstallation(PipelineView):
def get_app_url(self) -> str:
name = options.get("github-app.name")
return f"https://github.com/apps/{slugify(name)}"

def dispatch(self, request: Request, pipeline: Pipeline) -> HttpResponse:
if "installation_id" not in request.GET:
installation_id = request.GET.get(
"installation_id", pipeline.fetch_state("installation_id")
)
if installation_id is None:
return self.redirect(self.get_app_url())

pipeline.bind_state("installation_id", installation_id)
self.determine_active_organization(request)

integration_pending_deletion_exists = False
Expand All @@ -374,57 +465,43 @@ def dispatch(self, request: Request, pipeline: Pipeline) -> HttpResponse:
).exists()

if integration_pending_deletion_exists:
document_origin = "document.origin"
if self.active_organization and features.has(
"organizations:customer-domains", self.active_organization.organization
):
document_origin = (
f'"{generate_organization_url(self.active_organization.organization.slug)}"'
)
return render_to_response(
"sentry/integrations/github-integration-failed.html",
context={
"error": ERR_INTEGRATION_PENDING_DELETION,
"payload": {
"success": False,
"data": {"error": _("GitHub installation pending deletion.")},
},
"document_origin": document_origin,
},
request=request,
return error(
request,
self.active_organization,
error_short="GitHub installation pending deletion.",
error_long=ERR_INTEGRATION_PENDING_DELETION,
)

try:
# We want to limit GitHub integrations to 1 organization
installations_exist = OrganizationIntegration.objects.filter(
integration=Integration.objects.get(external_id=request.GET["installation_id"])
integration=Integration.objects.get(external_id=installation_id)
).exists()

except Integration.DoesNotExist:
pipeline.bind_state("installation_id", request.GET["installation_id"])
return pipeline.next_step()

if installations_exist:
document_origin = "document.origin"
if self.active_organization and features.has(
"organizations:customer-domains", self.active_organization.organization
):
document_origin = (
f'"{generate_organization_url(self.active_organization.organization.slug)}"'
)
return render_to_response(
"sentry/integrations/github-integration-failed.html",
context={
"error": ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG,
"payload": {
"success": False,
"data": {"error": _("Github installed on another Sentry organization.")},
},
"document_origin": document_origin,
},
request=request,
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.
pipeline.bind_state("installation_id", request.GET["installation_id"])
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"]
):
Comment on lines +501 to +504

@codeant-ai codeant-ai Bot Feb 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
👍 | 👎

return error(request, self.active_organization)

return pipeline.next_step()
11 changes: 4 additions & 7 deletions src/sentry/web/frontend/pipeline_advancer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,6 @@
PIPELINE_CLASSES = [IntegrationPipeline, IdentityProviderPipeline]


# 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.
FORWARD_INSTALL_FOR = ["github"]


from rest_framework.request import Request


Expand All @@ -40,8 +34,11 @@ 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 in FORWARD_INSTALL_FOR
provider_id == "github"
and request.GET.get("setup_action") == "install"
and pipeline is None
):
Expand Down
Loading