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/python/src/sentry/integrations/github/integration.py
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(

Copy link
Copy Markdown
Owner Author

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.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The 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"):

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The 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:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 Параметры URL авторизации OAuth не кодируются (URL-encoding)

Значения client_id, state и redirect_uri подставляются в строку запроса через f-интерполяцию без URL-кодирования. Хотя state — шестнадцатеричный хеш, а client_id обычно алфавитно-цифровой, redirect_uri — полный URL, который согласно спецификации OAuth 2.0 должен быть percent-encoded. Некорректно закодированный redirect_uri может привести к отклонению запроса GitHub или неверной интерпретации callback URL.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 Параметры OAuth-редиректа не URL-кодируются

Значение redirect_uri интерполируется напрямую в строку запроса без URL-кодирования. В тестах http://testserver/extensions/github/setup/ работает, но в production-URL с доменами клиентов или спецсимволами OAuth-редирект может сломаться или GitHub может отклонить redirect_uri. Параметр state также не кодируется, хотя, вероятно, является hex-строкой. Рекомендуется использовать urllib.parse.quote или urlencode для всех параметров.

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)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 Исключение safe_urlopen при обмене токена не обрабатывается — 500 вместо страницы ошибки

Вызов req = safe_urlopen(url=..., data=data) находится вне блока try/except, который обёртывает safe_urlread/parse_qsl. Если исходящий запрос обмена токена завершается неудачей (сетевая ошибка, таймаут, DNS, отказ соединения), safe_urlopen выбрасывает исключение, которое распространяется необработанным и приводит к HTTP 500 вместо дружелюбной страницы ошибки, которую возвращают все остальные ветки неудач. Рекомендуется обернуть safe_urlopen в тот же try/except или добавить отдельную обработку.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 Обмен токена молча поглощает все исключения, возвращая пустой payload

Широкий except Exception вокруг парсинга ответа обмена токена перехватывает все ошибки и устанавливает payload = {}, что затем приводит к проверке "access_token" not in payload. Это предотвращает падения, но делает отладку затруднительной — сетевые ошибки, проблемы кодировки и неожиданные форматы ответа неразличимы. Пользователь видит общий текст «Invalid installation request» без возможности определить реальную причину.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 Результат вызова get_user_info() не защищён от исключений

После получения payload["access_token"] код вызывает get_user_info(payload["access_token"]) без блока try/except. В отличие от блока обмена токеном выше, который обёрнут в except Exception, сетевая ошибка или ошибка парсинга из get_user_info приведёт к необработанному HTTP 500 вместо ответа error(...), используемого в остальных местах OAuthLoginView.

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(

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The 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:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟠 Проверка соответствия пользователя обходится, если запись Integration ещё не существует (race condition)

Новая проверка sender выполняется только после ветки, в которой повторно извлекается активная Integration. Однако когда Integration.objects.get(external_id=installation_id) выбрасывает DoesNotExist (строки 158–163), код немедленно вызывает pipeline.next_step() и создаёт интеграцию для текущей организации без проверки, что аутентифицированный пользователь совпадает с установщиком. Запись Integration заполняется асинхронно через webhook, поэтому атакующий, получивший чужой installation_id и успевший завершить настройку в Sentry до обработки webhook, может обойти всю проверку. Рекомендуется выполнять проверку sender даже в ветке DoesNotExist, например сверяя данные из сессии/pipeline с payload установки.

pipeline.bind_state("installation_id", request.GET["installation_id"])
return pipeline.next_step()

if installations_exist:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 Неактивные интеграции теперь возвращают ошибку вместо разрешения на повторную установку

Ранее, если Integration существовала, но OrganizationIntegration отсутствовала, код безусловно переходил к pipeline.next_step(). Теперь, если статус интеграции не ObjectStatus.ACTIVE (например, отключена или ожидает удаления), возвращается ошибка. Это блокирует повторную установку ранее отключенных интеграций через данный поток, что может быть регрессией для пользователей, пытающихся переустановить интеграцию.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟠 Проверка sender пропускается при отсутствии записи Integration — обход OAuth-верификации

Когда Integration.objects.get(external_id=installation_id) выбрасывает DoesNotExist, код привязывает installation_id и переходит к pipeline.next_step() без какой-либо проверки sender. Это открывает окно для атаки: злоумышленник, знающий чужой installation_id и опередивший обработку webhook установки GitHub, может обойти всё улучшение OAuth-верификации. Рекомендуется добавить проверку sender в ветку DoesNotExist, используя данные из payload или сессии.

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:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 Неявное изменение поведения для не-ACTIVE интеграций

Ранее интеграции со статусом != ACTIVE (например, DISABLED) могли продолжить привязку installation_id. Теперь добавлен фильтр status=ACTIVE и возвращается общая ошибка без детализации и наблюдаемости.

integration = Integration.objects.get(

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟠 Небезопасный доступ к integration.metadata["sender"]["login"] без проверки наличия ключа sender

В строке 175 код обращается к integration.metadata["sender"]["login"] напрямую, без проверки существования ключа sender. Поле sender заполняется вебхуком установки GitHub, но OAuth-поток и доставка вебхука асинхронны — пользователь может завершить OAuth до обработки вебхука. Если sender отсутствует, возникает необработанный KeyError (HTTP 500) вместо корректного ответа с ошибкой.

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 (

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The 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"]

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 Прямой доступ к integration.metadata["sender"]["login"] в GitHubInstallation.dispatch может вызвать KeyError

В строке 184, после повторного получения активной интеграции, новая проверка несоответствия пользователя обращается к integration.metadata["sender"]["login"] через прямой индекс. integration.metadata — JSON-поле, структура которого зависит от данных, заполненных вебхуком установки GitHub. Если вебхук не был обработан (например, старая интеграция, созданная до этого изменения, сбой доставки вебхука), ключ sender будет отсутствовать, что приведёт к необработанному KeyError.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 KeyError при отсутствии ключа sender/login в metadata интеграции

Новая проверка обращается к integration.metadata["sender"]["login"] через прямой индекс. Если запись Integration существует, но её metadata не содержит ключ sender (или sender не содержит login) — например, записи, созданные старым кодом, частичные метаданные или вариации схемы — это вызовет необработанный KeyError и HTTP 500 вместо предполагаемой страницы ошибки. Рекомендуется использовать .get() с проверкой или обернуть доступ в try/except с вызовом error().

):

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 Незащищённый доступ к integration.metadata["sender"]["login"] — риск KeyError

Сравнение sender-login напрямую индексирует integration.metadata["sender"]["login"] без проверки существования ключей. Интеграции, созданные через старые пути кода, webhook-пейлоады с отсутствующими полями sender или частично обработанные метаданные вызовут необработанный KeyError и 500 вместо дружелюбной страницы ошибки. Это сводит на нет назначение хелпера error(). Рекомендуется использовать безопасный доступ через .get() с fallback.

return error(request, self.active_organization)

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

💡 Конфигурируемый список FORWARD_INSTALL_FOR заменён на хардкод строки "github"

MR удаляет список FORWARD_INSTALL_FOR и инлайнит проверку provider_id == "github". Функционально эквивалентно сегодня, но это reintroduces magic string и убирает точку расширения, которую предыдущий список предоставлял, слегка усложняя добавление другого провайдера с аналогичным поведением прямой установки в будущем.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The 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
):
Loading