Skip to content

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.

Sign up for GitHub

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

Open
linxia0415 wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix(security): validate GitHub user during integration installation #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b31aefd
refactor
oioki Mar 28, 2024
0b8cf7c
add oauth2 login step in pipeline; check for the user match at the en…
oioki Mar 28, 2024
7aa8911
add a critical path test
oioki Apr 2, 2024
8c68d5b
refactor errors
oioki Apr 2, 2024
bb75657
add explicit redirect_uri parameter
oioki Apr 3, 2024
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/sentry/integrations/github/integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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."
)
Expand All @@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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

@code-hawk-sit code-hawk-sit Bot Apr 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 AI 代码审查发现问题

📋 问题概述

URL 参数注入风险

line 402 处直接拼接 OAuth 授权 URL 参数，未对参数进行 URL 编码。当 redirect_uri 包含 &、= 等特殊字符（如 https://example.com/callback?foo=bar）时，会破坏 URL 结构，导致参数被错误解析。攻击者可利用此缺陷构造恶意回调 URL，造成开放重定向或参数注入攻击。

📍 问题详情

🔴 问题 1 | 严重程度: HIGH | 行号: 402

💬 详细说明：

  • URL 参数注入风险
  • line 402 处直接拼接 OAuth 授权 URL 参数，未对参数进行 URL 编码。当 redirect_uri 包含 &、= 等特殊字符（如 https://example.com/callback?foo=bar）时，会破坏 URL 结构，导致参数被错误解析。攻击者可利用此缺陷构造恶意回调 URL，造成开放重定向或参数注入攻击。

📝 问题代码：

                f"{ghip.get_oauth_authorize_url()}?client_id={github_client_id}&state={state}&redirect_uri={redirect_uri}"

💡 修复建议：

使用 urllib.parse.urlencode 对查询参数进行编码后再拼接 URL。先导入 urlencode，然后将所有参数放入字典中编码，确保特殊字符被正确转义。

✅ 修复示例：

            from urllib.parse import urlencode
            
            params = urlencode({
                'client_id': github_client_id,
                'state': state,
                'redirect_uri': redirect_uri,
            })
            return self.redirect(
                f"{ghip.get_oauth_authorize_url()}?{params}"
            )

🔗 参考链接

无


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:

@code-hawk-sit code-hawk-sit Bot Apr 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 AI 代码审查发现问题

📋 问题概述

None 值未检查导致的 TypeError

line 428 处 get_user_info() 可能返回 None（如网络请求失败或响应解析错误），但 line 429 直接使用 "login" not in authenticated_user_info 进行成员检测。当 authenticated_user_info 为 None 时，将抛出 TypeError: argument of type 'NoneType' is not iterable，导致 500 错误而非预期的错误页面。

📍 问题详情

🟡 问题 1 | 严重程度: MEDIUM | 行号: 428

💬 详细说明：

  • None 值未检查导致的 TypeError
  • line 428 处 get_user_info() 可能返回 None（如网络请求失败或响应解析错误），但 line 429 直接使用 "login" not in authenticated_user_info 进行成员检测。当 authenticated_user_info 为 None 时，将抛出 TypeError: argument of type 'NoneType' is not iterable，导致 500 错误而非预期的错误页面。

📝 问题代码：

        authenticated_user_info = get_user_info(payload["access_token"])

💡 修复建议：

在检查 "login" 字段前，先验证 authenticated_user_info 不为 None。使用 if not authenticated_user_info or "login" not in authenticated_user_info 进行双重检查。

✅ 修复示例：

        authenticated_user_info = get_user_info(payload["access_token"])
        if not authenticated_user_info or "login" not in authenticated_user_info:
            return error(request, self.active_organization)

🔗 参考链接

无

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
Expand All @@ -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
)

@code-hawk-sit code-hawk-sit Bot Apr 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 AI 代码审查发现问题

📋 问题概述

可选字段未检查导致的 KeyError

line 497 处直接访问 integration.metadata["sender"]["login"]，但 "sender" 是可选字段（在 build_integration 的 line 371-372 中仅当 state.get("sender") 存在时才设置）。当集成创建时未包含 sender 信息时，此访问将抛出 KeyError，导致 500 错误而非优雅的错误处理。

📍 问题详情

🟡 问题 1 | 严重程度: MEDIUM | 行号: 496

💬 详细说明：

  • 可选字段未检查导致的 KeyError
  • line 497 处直接访问 integration.metadata["sender"]["login"]，但 "sender" 是可选字段（在 build_integration 的 line 371-372 中仅当 state.get("sender") 存在时才设置）。当集成创建时未包含 sender 信息时，此访问将抛出 KeyError，导致 500 错误而非优雅的错误处理。

📝 问题代码：

            != integration.metadata["sender"]["login"]

💡 修复建议：

使用 .get() 方法安全访问可选字段。先获取 sender 字典，检查其存在后再访问 login 字段，避免直接键访问导致的 KeyError。

✅ 修复示例：

        # Check that the authenticated GitHub user is the same as who installed the app.
        github_authenticated_user = pipeline.fetch_state("github_authenticated_user")
        sender_info = integration.metadata.get("sender")
        if not sender_info or github_authenticated_user != sender_info.get("login"):
            return error(request, self.active_organization)

🔗 参考链接

无

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)

return pipeline.next_step()
11 changes: 4 additions & 7 deletions src/sentry/web/frontend/pipeline_advancer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,6 @@
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


Expand All @@ -40,8 +34,11 @@ def handle(self, request: Request, provider_id: str) -> HttpResponseBase:
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 (
provider_id in FORWARD_INSTALL_FOR
provider_id == "github"
and request.GET.get("setup_action") == "install"
and pipeline is None
):
Expand Down
Loading