-
Notifications
You must be signed in to change notification settings - Fork 0
fix(security): validate GitHub user during integration installation #1
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
|
|
@@ -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 | ||
|
|
@@ -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." | ||
| ) | ||
|
|
@@ -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 | ||
|
|
@@ -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() | ||
|
|
@@ -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 | ||
|
|
@@ -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"] | ||
| ): | ||
| return error(request, self.active_organization) | ||
|
Comment on lines
+493
to
+505
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guard missing Older integrations, or records created without webhook sender data, will raise 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 |
||
|
|
||
| return pipeline.next_step() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exclude the current organization from the cross-org ownership check.
installations_existbecomesTrueeven when this installation is already linked toself.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
🤖 Prompt for AI Agents