Skip to content

Dev/internal pipeline#195

Closed
CongkeChen wants to merge 2 commits into
internal_pipeline_testfrom
dev/internal_pipeline
Closed

Dev/internal pipeline#195
CongkeChen wants to merge 2 commits into
internal_pipeline_testfrom
dev/internal_pipeline

Conversation

@CongkeChen

Copy link
Copy Markdown
Contributor

No description provided.

@CongkeChen CongkeChen closed this Jul 16, 2026
@CongkeChen

Copy link
Copy Markdown
Contributor Author

AI Code Review

发现的问题

该 PR 仅新增一个文件 tests/ai_review_demo_bad_code.py,包含多个真实可复现的安全与正确性缺陷。即使作为 demo/测试用途,文件已进入仓库 tests/ 目录,存在被其他测试导入或复用的风险,按真实风险评级。

🚨 Critical

  • tests/ai_review_demo_bad_code.py:15-16:硬编码敏感凭证

    • 将 API Key 与 DB 密码以明文常量写入源码并随仓库提交,存在凭证泄露风险;应移至环境变量/密钥管理,并轮换已暴露的凭证。
  • tests/ai_review_demo_bad_code.py:23-24:命令注入(shell=True + 不可信输入)

    • run_user_command 直接将外部 cmd 传给 subprocess.run(..., shell=True),可执行任意 shell 命令;应改为传列表并禁用 shell,或对输入做严格白名单校验。
      result = subprocess.run(cmd, shell=True, capture_output=True, text=True)
  • tests/ai_review_demo_bad_code.py:28-29:路径穿越

    • read_user_file 用字符串拼接 base_dir + "/" + relative_pathrelative_path../ 即可越权读取任意文件;应基于 Path(base_dir).resolve() 校验解析后路径仍位于 base_dir 内。
  • tests/ai_review_demo_bad_code.py:38-40:权限判定逻辑反转导致越权

    • is_authorizedrole 为空/None 时直接返回 True,空角色被当作已授权,构成权限绕过;空角色应返回 False 或显式拒绝。
  • tests/ai_review_demo_bad_code.py:44-45:空值未处理导致崩溃

    • fetch_profile(user_id: str | None)user_idNone 时调用 .upper()AttributeError;应在入口校验 None 并返回或报错。

⚠️ Warning

  • tests/ai_review_demo_bad_code.py:34-35apply_discount 计算逻辑错误

    • 使用 price + percent / 100 既不是折扣也非加价,与函数语义不符;应为 price * (1 - percent / 100) 并校验 percent 范围。
  • tests/ai_review_demo_bad_code.py:48-49:除零边界缺失

    • average 对空列表 sum([])/len([])ZeroDivisionError;应处理空输入(返回 0 或抛明确异常)。
  • tests/ai_review_demo_bad_code.py:52-60:文件资源未释放

    • load_configopenwith 管理,异常路径下文件句柄泄漏;应改用 with open(...)
  • tests/ai_review_demo_bad_code.py:63-66:无超时且存在 SSRF 风险

    • call_remote_api 对任意 url 调用 urllib.request.urlopen 无 timeout、无协议/域名校验,易被用于内网请求或长时间挂起;应加超时并校验目标地址。
  • tests/ai_review_demo_bad_code.py:81-83:写文件路径穿越

    • write_reportname + ".txt" 拼接,name../ 可写到 output_dir 之外;应校验 name 不含路径分隔符或对最终路径做归属校验。
  • tests/ai_review_demo_bad_code.py:75-77:可变默认参数被就地修改

    • merge_settings 直接 defaults.update(overrides) 并返回原对象,会修改调用方传入的 defaults;应返回新字典或明确文档化副作用。

💡 Suggestion

  • tests/ai_review_demo_bad_code.py:70-72dump_debug 直接打印整个环境变量字典,可能输出敏感信息(TOKEN、密钥等);建议过滤白名单键或脱敏后再输出。

总结

该 PR 新增的 demo 文件集中包含凭证泄露、命令注入、路径穿越、权限绕过等多项 Critical 级问题,必须修复;由于文件位于 tests/ 目录,存在被 CI 收集或被其他开发者复用的真实风险,不能因其为 demo 而豁免。

测试建议

  • 若该文件用于 AI review pipeline 的负样本,建议新增一个 test_* 校验测试,断言这些缺陷函数在被注入恶意输入时被正确拦截/拒绝,以验证 review 流程本身有效,而非让缺陷代码作为可被直接调用的工具函数留在仓库中。

@CongkeChen

Copy link
Copy Markdown
Contributor Author

AI Code Review

这是一个用于 AI 审查流水线的"故意劣质代码"fixture 文件(commit message 已说明)。但按审查规则,我仍按 diff 中代码的真实风险逐条反馈。已确认该文件位于 tests/ 下,但文件名不以 test_ 开头、内部也无 test_ 函数,CI 的 pytest tests/ 不会收集执行它,故无运行时执行风险。

发现的问题

🚨 Critical

  • tests/ai_review_demo_bad_code.py:15-16:硬编码凭证

    • 模块级明文写入 API_KEYDB_PASSWORD,若该文件被复用或误入打包路径会造成凭证泄露;应改为从环境变量/secret manager 读取,并从仓库历史中清除。
  • tests/ai_review_demo_bad_code.py:23-25:命令注入

    • subprocess.run(cmd, shell=True) 直接执行外部传入字符串,shell=True 叠加未校验输入构成命令注入;应使用参数列表 subprocess.run(shlex.split(cmd) ...) 或去掉 shell=True 并对允许的命令做白名单校验。
  • tests/ai_review_demo_bad_code.py:38-41:鉴权绕过

    • is_authorizedrole 为空/None 时直接返回 True,缺失角色即放行,属于权限绕过;空值应默认拒绝(返回 False)。
  • tests/ai_review_demo_bad_code.py:34-35:折扣计算逻辑错误

    • apply_discountprice + percent / 100 计算折扣,既符号错误又公式错误,会返回错误金额;应为 price * (1 - percent / 100) 并校验 0 <= percent <= 100

⚠️ Warning

  • tests/ai_review_demo_bad_code.py:28-30:路径穿越

    • read_user_file 直接拼接 base_dir + "/" + relative_path,未对 ../ 做规范化校验,可读取 base_dir 之外文件;应用 Path(base_dir).resolve() 校验结果仍在 base_dir 内。
  • tests/ai_review_demo_bad_code.py:63-67:SSRF / URL 未校验

    • call_remote_api 直接 urllib.request.urlopen(url) 任意 URL,无协议/host 白名单与超时,存在 SSRF 与无限挂起风险;应校验 scheme 并设置 timeout
  • tests/ai_review_demo_bad_code.py:44-45:空值解引用导致崩溃

    • fetch_profile(user_id)user_idNone 时调用 .upper()AttributeError;应前置 None 校验或约束类型为非空 str
  • tests/ai_review_demo_bad_code.py:48-49:除零边界缺失

    • average 未处理空列表,len(scores)==0ZeroDivisionError;应显式判空并返回 0 或抛出明确异常。
  • tests/ai_review_demo_bad_code.py:52-60:资源未释放

    • load_config 用裸 openwith/close,异常路径下文件句柄泄漏;应改用 with open(...) 上下文管理。
  • tests/ai_review_demo_bad_code.py:70-72:敏感信息输出

    • dump_debug 默认打印整份 os.environ(含 token/密钥),日志泄露风险;应脱敏或显式白名单字段。
  • tests/ai_review_demo_bad_code.py:75-78:可变默认参数副作用

    • merge_settings 直接 defaults.update(overrides) 修改并返回传入的 defaults,污染调用方原始字典;应拷贝后再合并。
  • tests/ai_review_demo_bad_code.py:81-84:写入路径穿越

    • write_report 直接拼接 name + ".txt" 写入 output_dirname../ 时可写到目录外;应对最终路径做 resolve 并校验落在 output_dir 内。

💡 Suggestion

  • tests/ai_review_demo_bad_code.py:1-85:该文件作为 AI 审查流水线的故意劣质代码 fixture,建议在文件顶部或 README 明确标注" intentional bad code, do not import/execute",并置于不被 pytest 收集的独立目录(如 tests/fixtures/),避免被他人误复用到真实路径。

总结

该 PR 仅新增一个用于 AI 审查流水线演示的"故意劣质代码"文件,包含多处真实安全与正确性问题(凭证硬编码、命令注入、鉴权绕过、计算逻辑错误等);因文件名不匹配 pytest 收集规则,CI 不会执行它,无线上运行时风险,但若被复用则问题会全部生效,建议按上述方向修复或明确隔离。

测试建议

暂无额外测试建议——该文件本身是审查目标 fixture 而非被测代码;若未来引入真实的折扣/鉴权逻辑,应补 apply_discount 边界(0%、100%、负值)与 is_authorized(None/unknown role) 的单元测试。

@CongkeChen CongkeChen reopened this Jul 16, 2026
@CongkeChen

Copy link
Copy Markdown
Contributor Author

AI Code Review

发现的问题

🚨 Critical

  • tests/ai_review_demo_bad_code.py:9-10:硬编码凭证

    • API_KEYDB_PASSWORD 以明文写入源码,任何能读仓库的人即可伪造鉴权或获取数据库口令;应改为从环境变量/密钥管理服务读取,并轮换已泄露的值。
  • tests/ai_review_demo_bad_code.py:17-19:命令注入

    • subprocess.run(cmd, shell=True, ...) 直接把不可信 cmd 交给 shell 执行,cmd="ls; rm -rf /" 即可任意执行;应使用参数列表 (shell=False) 并做白名单校验。
      result = subprocess.run(cmd, shell=True, capture_output=True, text=True)
  • tests/ai_review_demo_bad_code.py:22-25:路径穿越

    • base_dir + "/" + relative_path 未做规范化与边界校验,relative_path="../../etc/passwd" 可读任意文件;应基于 Path(base_dir).resolve() 校验最终路径仍在 base_dir 内。
  • tests/ai_review_demo_bad_code.py:28-29:折扣计算逻辑错误

    • price + percent / 100 把折扣当加法,结果与预期完全相反(如 100 元打 10 折返回 101.0);应为 price * (1 - percent / 100),会导致金额计算错误。
  • tests/ai_review_demo_bad_code.py:32-35:权限绕过

    • if not role: return TrueNone/空字符串被判定为已授权,与“拒绝未登录”的意图相反;未登录应 return False

⚠️ Warning

💡 Suggestion

  • tests/ai_review_demo_bad_code.py:1-6:新增源文件缺少仓库统一的 Tencent Apache-2.0 许可头(同目录其他文件均有),建议补齐以保持合规与一致性。

总结

本 PR 新增的 tests/ai_review_demo_bad_code.py 存在多个真实高危问题:硬编码凭证、命令注入、路径穿越、鉴权绕过与折扣逻辑错误,均为必须修复级别。该文件不以 test_ 开头且不在 trpc_agent_sdk/ 下,pytest 与 CI flake8 均不会收集/检查它,因此不会立即阻塞 CI,但若被其他开发者复用或误接入执行路径会带来线上风险。

测试建议

  • 暂无额外测试建议:该文件当前未被 pytest 收集(文件名不匹配 test_*.py),无实际执行路径需要覆盖;若计划将其转为可运行用例,应先修复上述问题并对每个函数补充空值/越界/非法输入用例。

@CongkeChen CongkeChen closed this Jul 16, 2026
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.

1 participant