Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
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,
)
Comment on lines 475 to 490

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Exclude the current organization from the cross-org ownership check.

installations_exist becomes True even when this installation is already linked to self.active_organization, so re-running setup for the same org now returns the “another Sentry organization” error. This should only fail for a different owning org.

Suggested fix
+        active_org_id = self.active_organization.organization.id if self.active_organization else None
         try:
             # We want to limit GitHub integrations to 1 organization
-            installations_exist = OrganizationIntegration.objects.filter(
-                integration=Integration.objects.get(external_id=installation_id)
+            integration = Integration.objects.get(
+                external_id=installation_id,
+                status=ObjectStatus.ACTIVE,
+            )
+            installations_exist = OrganizationIntegration.objects.filter(
+                integration=integration
+            ).exclude(
+                organization_id=active_org_id
             ).exists()
 
         except Integration.DoesNotExist:
             return pipeline.next_step()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/integrations/github/integration.py` around lines 475 - 490, The
check that sets installations_exist currently counts this installation even when
it's linked to self.active_organization; change the query so it only checks for
OrganizationIntegration records owned by a different org by excluding
self.active_organization (e.g., use
OrganizationIntegration.objects.filter(integration=Integration.objects.get(external_id=installation_id)).exclude(organization=self.active_organization).exists()),
keep the Integration.DoesNotExist handling and subsequent
pipeline.next_step()/error(...) behavior the same.


# 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"]
):
return error(request, self.active_organization)
Comment on lines +493 to +505

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard missing sender metadata before indexing into it.

Older integrations, or records created without webhook sender data, will raise KeyError on integration.metadata["sender"]["login"] and turn this validation into a 500 instead of the generic invalid-installation response.

Suggested fix
-        if (
-            pipeline.fetch_state("github_authenticated_user")
-            != integration.metadata["sender"]["login"]
-        ):
+        sender_login = integration.metadata.get("sender", {}).get("login")
+        if (
+            not sender_login
+            or pipeline.fetch_state("github_authenticated_user") != sender_login
+        ):
             return error(request, self.active_organization)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/integrations/github/integration.py` around lines 493 - 505, The
code assumes integration.metadata["sender"]["login"] exists and will KeyError
for older integrations; update the validation in the block after retrieving
Integration (function/class using Integration, pipeline.fetch_state, error,
self.active_organization) to first check that integration.metadata is a dict and
contains "sender" with a mapping that has "login" (e.g., using .get or explicit
key checks), and if missing treat it as an invalid installation by calling
error(request, self.active_organization); only perform the equality comparison
with pipeline.fetch_state("github_authenticated_user") when the login value is
present.


return pipeline.next_step()
11 changes: 4 additions & 7 deletions src/sentry/web/frontend/pipeline_advancer.py
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