-
Notifications
You must be signed in to change notification settings - Fork 0
Automated Test: oauth-state-secure #330
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. Potential Line 503 accesses 🐛 Proposed fix try:
integration = Integration.objects.get(
external_id=installation_id, status=ObjectStatus.ACTIVE
)
except Integration.DoesNotExist:
return error(request, self.active_organization)
+ sender_login = (integration.metadata or {}).get("sender", {}).get("login")
# 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"]
+ != sender_login
):
return error(request, self.active_organization)📝 Committable suggestion
Suggested change
🤖 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.
Missing error handling for
get_user_info— unhandled exceptions will 500.The token exchange on lines 425-429 is wrapped in a try/except, but the
get_user_infocall on line 434 is unprotected. If the GitHub/userAPI call fails (network error, rate limit, invalid token), this will raise an unhandled exception and return a 500 to the user instead of the friendly error page.🐛 Proposed fix
📝 Committable suggestion
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 428-428: Do not catch blind exception:
Exception(BLE001)
🤖 Prompt for AI Agents