Repository navigation
[benchmark][Python] GitHub OAuth Security Enhancement #11
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: benchmark/sentry/pr-67876-base
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 |
|---|---|---|
| @@ -1,111 +1,188 @@ | ||
| 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, | ||
| 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 | ||
| 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." | ||
| ) | ||
| 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 | ||
| ) | ||
|
|
||
| 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() | ||
| ) | ||
|
|
||
|
|
||
| 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") | ||
|
Owner
Author
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. 🟡 Принудительный OAuth-редирект при отсутствии installation_id OAuthLoginView сразу редиректит на GitHub authorize при отсутствии state, даже если installation_id не передан. Далее dispatch обнаруживает отсутствие installation_id и возвращает ошибку, создавая лишний круг. |
||
| if installation_id: | ||
| pipeline.bind_state("installation_id", installation_id) | ||
|
Owner
Author
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. 🔵 Дублирование привязки installation_id в состояние пайплайна OAuthLoginView и GitHubInstallation.dispatch оба вызывают pipeline.bind_state('installation_id', ...). Второе присваивание избыточно и может перезаписать значение неочевидным образом. |
||
|
|
||
| if not request.GET.get("state"): | ||
|
Owner
Author
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. 🔵 Проверка валидности state выполняется после редиректа При отсутствии 'state' диспетчер сразу редиректит с state=pipeline.signature, не проверяя, что pipeline.signature установлен и непуст. Возможен редирект с пустым 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: | ||
|
Owner
Author
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. 🟡 Параметры URL авторизации OAuth не кодируются (URL-encoding) Значения
Owner
Author
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. 🔵 Параметры OAuth-редиректа не URL-кодируются Значение |
||
| 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) | ||
|
Owner
Author
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. 🟡 Исключение Вызов
Owner
Author
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. 🟡 Слишком широкий except Exception скрывает ошибки обмена токеном Ошибки сети, не-200 ответы, некорректный JSON, отсутствие code/access_token схлопываются в одну общую ошибку без логирования и метрик, что затрудняет диагностику. |
||
|
|
||
| try: | ||
| body = safe_urlread(req).decode("utf-8") | ||
| payload = dict(parse_qsl(body)) | ||
|
Owner
Author
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. 🔵 Обмен токена молча поглощает все исключения, возвращая пустой payload Широкий |
||
| 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"]) | ||
|
Owner
Author
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. 🔵 Результат вызова После получения |
||
| 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( | ||
|
Owner
Author
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. 🟠 GET-параметр installation_id переопределяет состояние пайплайна после OAuth GitHubInstallation.dispatch выбирает installation_id из request.GET, если он есть, игнорируя значение, сохранённое OAuthLoginView в состояние пайплайна. Это позволяет подменить установку через URL. |
||
| "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 | ||
| ).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: | ||
|
Owner
Author
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. 🟠 Проверка соответствия пользователя обходится, если запись Новая проверка sender выполняется только после ветки, в которой повторно извлекается активная |
||
| pipeline.bind_state("installation_id", request.GET["installation_id"]) | ||
| return pipeline.next_step() | ||
|
|
||
| if installations_exist: | ||
|
Owner
Author
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. 🟡 Неактивные интеграции теперь возвращают ошибку вместо разрешения на повторную установку Ранее, если
Owner
Author
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. 🟠 Проверка sender пропускается при отсутствии записи Когда |
||
| 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: | ||
|
Owner
Author
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. 🟡 Неявное изменение поведения для не-ACTIVE интеграций Ранее интеграции со статусом != ACTIVE (например, DISABLED) могли продолжить привязку installation_id. Теперь добавлен фильтр status=ACTIVE и возвращается общая ошибка без детализации и наблюдаемости. |
||
| integration = Integration.objects.get( | ||
|
Owner
Author
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. 🟠 Небезопасный доступ к В строке 175 код обращается к |
||
| 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 ( | ||
|
Owner
Author
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. 🟠 KeyError при обращении к integration.metadata['sender']['login'] Новая проверка авторизации разыменовывает integration.metadata['sender']['login'] без проверок. Если в метаданных нет 'sender' или в нём нет 'login' (устаревшие/частичные вебхуки, установки вне вебхука), возникает KeyError и запрос падает. |
||
| pipeline.fetch_state("github_authenticated_user") | ||
| != integration.metadata["sender"]["login"] | ||
|
Owner
Author
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. 🟡 Прямой доступ к В строке 184, после повторного получения активной интеграции, новая проверка несоответствия пользователя обращается к
Owner
Author
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. 🟡 Новая проверка обращается к |
||
| ): | ||
|
Owner
Author
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. 🟡 Незащищённый доступ к Сравнение sender-login напрямую индексирует |
||
| return error(request, self.active_organization) | ||
|
|
||
| return pipeline.next_step() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,17 @@ | ||
| 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 | ||
|
|
||
|
|
||
| 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 ( | ||
|
Owner
Author
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. 💡 Конфигурируемый список MR удаляет список
Owner
Author
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. 💡 Потеря расширяемости FORWARD_INSTALL_FOR из-за инлайнинга проверки Замена списка FORWARD_INSTALL_FOR на жёсткое сравнение provider_id == 'github' убирает единый источник истины для провайдеров, поддерживающих установку вне пайплайна. |
||
| provider_id in FORWARD_INSTALL_FOR | ||
| provider_id == "github" | ||
| and request.GET.get("setup_action") == "install" | ||
| and pipeline is None | ||
| ): | ||
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.
🔵 Допущение о форме self.active_organization в error()/get_document_origin()
get_document_origin разыменовывает org.organization.slug, а error() вызывается до того, как determine_active_organization гарантированно установит active_organization. Возможен AttributeError на edge-case.