Skip to content

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

Open
linxia0415 wants to merge 5 commits into
masterfrom
pr-67876
Open

fix(security): validate GitHub user during integration installation#1
linxia0415 wants to merge 5 commits into
masterfrom
pr-67876

Conversation

@linxia0415

Copy link
Copy Markdown
Contributor

We're adding one more step in the GitHub integration installation pipeline, namely GitHub OAuth2 authorize. This is transparent from the UX perspective as the data exchange happens without user interaction.

The pipeline will now fail in these cases:

  • If there is a mismatch between currently authenticated GitHub user (derived from OAuth2 authorize step) and the user who installed the GitHub app (https://github.com/apps/sentry-io)
  • If there is a mismatch between state parameter supplied by user and pipeline signature
  • If GitHub could not generate correct access_token from the code (wrong or attempt of re-use of code).

In all those cases, this error is shown:
image

@LX-CodeReview LX-CodeReview deleted a comment from code-hawk-sit Bot Apr 11, 2026
@linxia0415 linxia0415 closed this Apr 11, 2026
@linxia0415 linxia0415 reopened this Apr 11, 2026

@code-hawk-sit code-hawk-sit Bot left a comment

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代码审查报告

变更概览

本次 PR 涉及 3 个文件,新增 +247 行,删除 -50 行。

功能变更摘要

本 PR 为 GitHub 集成增加对 GitHub Enterprise Server 的支持,允许配置自定义域名和 API 端点。同时调整了集成流程处理逻辑,并补充了完整的单元测试覆盖新功能。

文件变更摘要

文件 变更 行数 摘要 发现问题
src/sentry/integrations/github/integration.py 修改 +118/-41 增加 GitHub Enterprise Server 支持,包括自定义域名和 API 端点配置 3 个
tests/sentry/integrations/github/test_integration.py 修改 +125/-2 补充 GitHub Enterprise Server 场景的单元测试
src/sentry/web/frontend/pipeline_advancer.py 修改 +4/-7 简化集成流程处理逻辑以适配企业版支持

问题严重级别分布

级别 数量 占比
🔴 高危 1 33%
🟡 中危 2 66%

代表性问题(至多 10 条,按严重级别优先)

  1. 🔴 高危 src/sentry/integrations/github/integration.py L402: URL 参数注入风险

line 402 处直接拼接 OAuth 授权 URL 参数,未对参数进行 URL 编码。当 redirect_uri 包含 &= 等特殊字符(如 https:… 2. 🟡 **中危** src/sentry/integrations/github/integration.py` L428: None 值未检查导致的 TypeError

line 428 处 get_user_info() 可能返回 None(如网络请求失败或响应解析错误),但 line 429 直接使用 "log… 3. 🟡 **中危** src/sentry/integrations/github/integration.py` L496: 可选字段未检查导致的 KeyError

line 497 处直接访问 integration.metadata["sender"]["login"],但 "sender" 是可选字段(在 `bu…


CodeHawk 提供支持 · nuwa

pipeline.bind_state("installation_id", installation_id)

if not request.GET.get("state"):
state = pipeline.signature

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

🔗 参考链接

try:
body = safe_urlread(req).decode("utf-8")
payload = dict(parse_qsl(body))
except Exception:

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)

🔗 参考链接

try:
integration = Integration.objects.get(
external_id=installation_id, status=ObjectStatus.ACTIVE
)

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)

🔗 参考链接

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants