Repository navigation
Conversation
| # OrganizationIntegration does not exist, but Integration does exist. | ||
| pipeline.bind_state("installation_id", request.GET["installation_id"]) | ||
| try: | ||
| integration = Integration.objects.get( |
There was a problem hiding this comment.
🟠 Небезопасный доступ к integration.metadata["sender"]["login"] без проверки наличия ключа sender
В строке 175 код обращается к integration.metadata["sender"]["login"] напрямую, без проверки существования ключа sender. Поле sender заполняется вебхуком установки GitHub, но OAuth-поток и доставка вебхука асинхронны — пользователь может завершить OAuth до обработки вебхука. Если sender отсутствует, возникает необработанный KeyError (HTTP 500) вместо корректного ответа с ошибкой.
| ) | ||
|
|
||
| # At this point, we are past the GitHub "authorize" step | ||
| if request.GET.get("state") != pipeline.signature: |
There was a problem hiding this comment.
🟡 Параметры 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.
| # 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"] |
There was a problem hiding this comment.
🟡 Прямой доступ к integration.metadata["sender"]["login"] в GitHubInstallation.dispatch может вызвать KeyError
В строке 184, после повторного получения активной интеграции, новая проверка несоответствия пользователя обращается к integration.metadata["sender"]["login"] через прямой индекс. integration.metadata — JSON-поле, структура которого зависит от данных, заполненных вебхуком установки GitHub. Если вебхук не был обработан (например, старая интеграция, созданная до этого изменения, сбой доставки вебхука), ключ sender будет отсутствовать, что приведёт к необработанному KeyError.
| pipeline.bind_state("installation_id", request.GET["installation_id"]) | ||
| return pipeline.next_step() | ||
|
|
||
| if installations_exist: |
There was a problem hiding this comment.
🟡 Неактивные интеграции теперь возвращают ошибку вместо разрешения на повторную установку
Ранее, если Integration существовала, но OrganizationIntegration отсутствовала, код безусловно переходил к pipeline.next_step(). Теперь, если статус интеграции не ObjectStatus.ACTIVE (например, отключена или ожидает удаления), возвращается ошибка. Это блокирует повторную установку ранее отключенных интеграций через данный поток, что может быть регрессией для пользователей, пытающихся переустановить интеграцию.
| if "login" not in authenticated_user_info: | ||
| return error(request, self.active_organization) | ||
|
|
||
| pipeline.bind_state("github_authenticated_user", authenticated_user_info["login"]) |
There was a problem hiding this comment.
🔵 Результат вызова get_user_info() не защищён от исключений
После получения payload["access_token"] код вызывает get_user_info(payload["access_token"]) без блока try/except. В отличие от блока обмена токеном выше, который обёрнут в except Exception, сетевая ошибка или ошибка парсинга из get_user_info приведёт к необработанному HTTP 500 вместо ответа error(...), используемого в остальных местах OAuthLoginView.
🔍 Автоматическое ревью кодаКраткий итогСинтез ревью: 10 уникальных групп замечаний по 2 исходн. выполнениям; 6 для публикации (критических: 0, высоких: 1, средних: 3, низких: 2, мелких: 0). Замечания🟠 Важно
💡 Дополнительно
Автоматическое ревью от Review Engine v2.0 |
|
|
||
| try: | ||
| body = safe_urlread(req).decode("utf-8") | ||
| payload = dict(parse_qsl(body)) |
There was a problem hiding this comment.
🔵 Обмен токена молча поглощает все исключения, возвращая пустой payload
Широкий except Exception вокруг парсинга ответа обмена токена перехватывает все ошибки и устанавливает payload = {}, что затем приводит к проверке "access_token" not in payload. Это предотвращает падения, но делает отладку затруднительной — сетевые ошибки, проблемы кодировки и неожиданные форматы ответа неразличимы. Пользователь видит общий текст «Invalid installation request» без возможности определить реальную причину.
| integration=Integration.objects.get(external_id=installation_id) | ||
| ).exists() | ||
|
|
||
| except Integration.DoesNotExist: |
There was a problem hiding this comment.
🟠 Проверка соответствия пользователя обходится, если запись 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: |
There was a problem hiding this comment.
🟠 Проверка 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 или сессии.
| # 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"] |
There was a problem hiding this comment.
🟡 KeyError при отсутствии ключа sender/login в metadata интеграции
Новая проверка обращается к integration.metadata["sender"]["login"] через прямой индекс. Если запись Integration существует, но её metadata не содержит ключ sender (или sender не содержит login) — например, записи, созданные старым кодом, частичные метаданные или вариации схемы — это вызовет необработанный KeyError и HTTP 500 вместо предполагаемой страницы ошибки. Рекомендуется использовать .get() с проверкой или обернуть доступ в try/except с вызовом error().
| if ( | ||
| pipeline.fetch_state("github_authenticated_user") | ||
| != integration.metadata["sender"]["login"] | ||
| ): |
There was a problem hiding this comment.
🟡 Незащищённый доступ к integration.metadata["sender"]["login"] — риск KeyError
Сравнение sender-login напрямую индексирует integration.metadata["sender"]["login"] без проверки существования ключей. Интеграции, созданные через старые пути кода, webhook-пейлоады с отсутствующими полями sender или частично обработанные метаданные вызовут необработанный KeyError и 500 вместо дружелюбной страницы ошибки. Это сводит на нет назначение хелпера error(). Рекомендуется использовать безопасный доступ через .get() с fallback.
| } | ||
|
|
||
| # similar to OAuth2CallbackView.exchange_token | ||
| req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data) |
There was a problem hiding this comment.
🟡 Исключение safe_urlopen при обмене токена не обрабатывается — 500 вместо страницы ошибки
Вызов req = safe_urlopen(url=..., data=data) находится вне блока try/except, который обёртывает safe_urlread/parse_qsl. Если исходящий запрос обмена токена завершается неудачей (сетевая ошибка, таймаут, DNS, отказ соединения), safe_urlopen выбрасывает исключение, которое распространяется необработанным и приводит к HTTP 500 вместо дружелюбной страницы ошибки, которую возвращают все остальные ветки неудач. Рекомендуется обернуть safe_urlopen в тот же try/except или добавить отдельную обработку.
| ) | ||
|
|
||
| # At this point, we are past the GitHub "authorize" step | ||
| if request.GET.get("state") != pipeline.signature: |
There was a problem hiding this comment.
🔵 Параметры OAuth-редиректа не URL-кодируются
Значение redirect_uri интерполируется напрямую в строку запроса без URL-кодирования. В тестах http://testserver/extensions/github/setup/ работает, но в production-URL с доменами клиентов или спецсимволами OAuth-редирект может сломаться или GitHub может отклонить redirect_uri. Параметр state также не кодируется, хотя, вероятно, является hex-строкой. Рекомендуется использовать urllib.parse.quote или urlencode для всех параметров.
| # 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 ( |
There was a problem hiding this comment.
💡 Конфигурируемый список FORWARD_INSTALL_FOR заменён на хардкод строки "github"
MR удаляет список FORWARD_INSTALL_FOR и инлайнит проверку provider_id == "github". Функционально эквивалентно сегодня, но это reintroduces magic string и убирает точку расширения, которую предыдущий список предоставлял, слегка усложняя добавление другого провайдера с аналогичным поведением прямой установки в будущем.
🔍 Автоматическое ревью кодаКраткий итогСинтез ревью: 8 уникальных групп замечаний по 2 исходн. выполнениям; 7 для публикации (критических: 0, высоких: 2, средних: 3, низких: 1, мелких: 1). Замечания🟠 Важно
💡 Дополнительно
Автоматическое ревью от Review Engine v2.0 |
hsander
left a comment
hsander
left a comment
There was a problem hiding this comment.
🔍 Автоматическое ревью кода
Краткий итог
Синтез ревью: 12 уникальных групп замечаний по 1 исходн. выполнениям; 10 для публикации (критических: 0, высоких: 2, средних: 3, низких: 3, мелких: 2).
Замечания
🟠 Важно
- [src/python/src/sentry/integrations/github/integration.py:136] GET-параметр installation_id переопределяет состояние пайплайна после OAuth — GitHubInstallation.dispatch выбирает installation_id из request.GET, если он есть, игнорируя значение, сохранённое OAuthLoginView в состояние пайплайна. Это позволяет подменить установку через URL.
- [src/python/src/sentry/integrations/github/integration.py:182] KeyError при обращении к integration.metadata['sender']['login'] — Новая проверка авторизации разыменовывает integration.metadata['sender']['login'] без проверок. Если в метаданных нет 'sender' или в нём нет 'login' (устаревшие/частичные вебхуки, установки вне вебхука), возникает KeyError и запрос падает.
💡 Дополнительно
- [src/python/src/sentry/integrations/github/integration.py:111] Слишком широкий except Exception скрывает ошибки обмена токеном — Ошибки сети, не-200 ответы, некорректный JSON, отсутствие code/access_token схлопываются в одну общую ошибку без логирования и метрик, что затрудняет диагностику.
- [src/python/src/sentry/integrations/github/integration.py:85] Принудительный OAuth-редирект при отсутствии installation_id — OAuthLoginView сразу редиректит на GitHub authorize при отсутствии state, даже если installation_id не передан. Далее dispatch обнаруживает отсутствие installation_id и возвращает ошибку, создавая лишний круг.
- [src/python/src/sentry/integrations/github/integration.py:174] Неявное изменение поведения для не-ACTIVE интеграций — Ранее интеграции со статусом != ACTIVE (например, DISABLED) могли продолжить привязку installation_id. Теперь добавлен фильтр status=ACTIVE и возвращается общая ошибка без детализации и наблюдаемости.
- [src/python/src/sentry/integrations/github/integration.py:87] Дублирование привязки installation_id в состояние пайплайна — OAuthLoginView и GitHubInstallation.dispatch оба вызывают pipeline.bind_state('installation_id', ...). Второе присваивание избыточно и может перезаписать значение неочевидным образом.
- [src/python/src/sentry/integrations/github/integration.py:89] Проверка валидности state выполняется после редиректа — При отсутствии 'state' диспетчер сразу редиректит с state=pipeline.signature, не проверяя, что pipeline.signature установлен и непуст. Возможен редирект с пустым state.
- [src/python/src/sentry/integrations/github/integration.py:38] Допущение о форме self.active_organization в error()/get_document_origin() — get_document_origin разыменовывает org.organization.slug, а error() вызывается до того, как determine_active_organization гарантированно установит active_organization. Возможен AttributeError на edge-case.
- [src/python/src/sentry/web/frontend/pipeline_advancer.py:13] Потеря расширяемости FORWARD_INSTALL_FOR из-за инлайнинга проверки — Замена списка FORWARD_INSTALL_FOR на жёсткое сравнение provider_id == 'github' убирает единый источник истины для провайдеров, поддерживающих установку вне пайплайна.
- [src/python/tests/sentry/integrations/github/test_integration.py:84] Тест не покрывает ветку 'integration exists but metadata.sender missing' — В тесте state не совпадает с сигнатурой пайплайна, что проверяет только несовпадение state. Нет теста на отсутствие sender/login в метаданных — самую хрупкую ветку.
Автоматическое ревью от Review Engine v2.0
По вопросам и замечаниям, обращайтесь:
Александр Липкин
AlgLipkin@sberbank.ru
Хоруженко Александр
AVKhoruzhenko@sberbank.ru
|
|
||
| def dispatch(self, request: Request, pipeline: Pipeline) -> HttpResponse: | ||
| if "installation_id" not in request.GET: | ||
| installation_id = request.GET.get( |
There was a problem hiding this comment.
🟠 GET-параметр installation_id переопределяет состояние пайплайна после OAuth
GitHubInstallation.dispatch выбирает installation_id из request.GET, если он есть, игнорируя значение, сохранённое OAuthLoginView в состояние пайплайна. Это позволяет подменить установку через URL.
| return error(request, self.active_organization) | ||
|
|
||
| # Check that the authenticated GitHub user is the same as who installed the app. | ||
| if ( |
There was a problem hiding this comment.
🟠 KeyError при обращении к integration.metadata['sender']['login']
Новая проверка авторизации разыменовывает integration.metadata['sender']['login'] без проверок. Если в метаданных нет 'sender' или в нём нет 'login' (устаревшие/частичные вебхуки, установки вне вебхука), возникает KeyError и запрос падает.
| } | ||
|
|
||
| # similar to OAuth2CallbackView.exchange_token | ||
| req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data) |
There was a problem hiding this comment.
🟡 Слишком широкий except Exception скрывает ошибки обмена токеном
Ошибки сети, не-200 ответы, некорректный JSON, отсутствие code/access_token схлопываются в одну общую ошибку без логирования и метрик, что затрудняет диагностику.
| github_client_id = ghip.get_oauth_client_id() | ||
| github_client_secret = ghip.get_oauth_client_secret() | ||
|
|
||
| installation_id = request.GET.get("installation_id") |
There was a problem hiding this comment.
🟡 Принудительный OAuth-редирект при отсутствии installation_id
OAuthLoginView сразу редиректит на GitHub authorize при отсутствии state, даже если installation_id не передан. Далее dispatch обнаруживает отсутствие installation_id и возвращает ошибку, создавая лишний круг.
|
|
||
| # OrganizationIntegration does not exist, but Integration does exist. | ||
| pipeline.bind_state("installation_id", request.GET["installation_id"]) | ||
| try: |
There was a problem hiding this comment.
🟡 Неявное изменение поведения для не-ACTIVE интеграций
Ранее интеграции со статусом != ACTIVE (например, DISABLED) могли продолжить привязку installation_id. Теперь добавлен фильтр status=ACTIVE и возвращается общая ошибка без детализации и наблюдаемости.
|
|
||
| installation_id = request.GET.get("installation_id") | ||
| if installation_id: | ||
| pipeline.bind_state("installation_id", installation_id) |
There was a problem hiding this comment.
🔵 Дублирование привязки installation_id в состояние пайплайна
OAuthLoginView и GitHubInstallation.dispatch оба вызывают pipeline.bind_state('installation_id', ...). Второе присваивание избыточно и может перезаписать значение неочевидным образом.
| if installation_id: | ||
| pipeline.bind_state("installation_id", installation_id) | ||
|
|
||
| if not request.GET.get("state"): |
There was a problem hiding this comment.
🔵 Проверка валидности state выполняется после редиректа
При отсутствии 'state' диспетчер сразу редиректит с state=pipeline.signature, не проверяя, что pipeline.signature установлен и непуст. Возможен редирект с пустым state.
| return f"{account_type}:{name} {query}".encode() | ||
|
|
||
|
|
||
| def error( |
There was a problem hiding this comment.
🔵 Допущение о форме self.active_organization в error()/get_document_origin()
get_document_origin разыменовывает org.organization.slug, а error() вызывается до того, как determine_active_organization гарантированно установит active_organization. Возможен AttributeError на edge-case.
| # 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 ( |
There was a problem hiding this comment.
💡 Потеря расширяемости FORWARD_INSTALL_FOR из-за инлайнинга проверки
Замена списка FORWARD_INSTALL_FOR на жёсткое сравнение provider_id == 'github' убирает единый источник истины для провайдеров, поддерживающих установку вне пайплайна.
| self.assertDialogSuccess(resp) | ||
| integration = Integration.objects.get(external_id=self.installation_id) | ||
| assert integration.provider == "github" | ||
| resp = self.client.get( |
There was a problem hiding this comment.
💡 Тест не покрывает ветку 'integration exists but metadata.sender missing'
В тесте state не совпадает с сигнатурой пайплайна, что проверяет только несовпадение state. Нет теста на отсутствие sender/login в метаданных — самую хрупкую ветку.
1 participant
Benchmark fixture for sentry PR #67876.\n\nGolden comments:
benchmark/golden_comments/sentry.json— PR67876.