Skip to content

Conversation

@BLumia
Copy link
Member

@BLumia BLumia commented Jan 22, 2026

No description provided.

@deepin-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BLumia

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@deepin-ci-robot
Copy link
Contributor

deepin pr auto review

这段代码是向 GitHub Actions 工作流中添加了一个新的 Job,用于检查源代码文件的 SPDX 版权声明。整体逻辑清晰,使用了社区现成的 Action 工具。

以下是对该代码的审查意见,分为语法逻辑、代码质量、代码性能和代码安全四个方面:

1. 语法逻辑

  • Job 依赖关系
    • 问题:新增的 Check-SPDX-Copyright Job 目前是独立的,没有设置 needs 字段。
    • 建议:如果这个检查是 PR 合并的必要条件,且不依赖其他 Job 的运行结果,保持现状即可。但如果它依赖于其他 Job(例如构建环境准备),或者希望它在其他 Job 成功后才运行以节省资源,应添加 needs 依赖。例如,如果它应该和上面的 license-check 并行或串行,需明确意图。通常作为独立检查并行运行是可以的。
  • 触发条件
    • 观察:代码中使用了 ref: ${{ github.event.pull_request.head.sha }}
    • 建议:这表明该 Job 专门针对 Pull Request 事件。请确保该 Workflow 文件的顶层 on: 触发器包含了 pull_request 事件,否则在非 PR 触发(如 push)时,此变量可能为空或指向不正确的提交,导致 Job 失败或行为异常。

2. 代码质量

  • 硬编码的持有者名称
    • 问题holder: '*UnionTech*' 是硬编码的。如果项目将来更换名称或贡献者协议变更,需要修改此处。
    • 建议:考虑将 holder 定义为 Workflow 的环境变量或 Secret,或者如果该 Action 支持多版权持有者检查,需确认当前配置是否过于严格或宽松。
  • 文件扩展名列表
    • 问题include 参数包含了一个非常长的文件扩展名字符串。虽然 YAML 支持这种写法,但可读性稍差且容易出错。
    • 建议:如果 zccrs/github-actions-spdx-checker@v1 支持数组形式的配置(需查阅该 Action 文档),建议改为数组形式;如果不支持,保持现状但需注意拼写检查。目前的列表覆盖了主流语言,看起来比较全面。

3. 代码性能

  • Action 版本
    • 观察:使用了 actions/checkout@v5
    • 评价:这是最新的主要版本,性能和安全性通常优于旧版本,做得很好。
  • 并行执行
    • 分析:由于没有 needs 依赖,该 Job 将与 Workflow 中的其他 Job 并行运行。
    • 建议:这有助于减少总 CI 时间,性能策略是合理的。只要该 Job 不消耗过多资源(版权检查通常是轻量级的 IO 操作),这种设置是高效的。

4. 代码安全

  • Action 来源安全性
    • 问题:使用了 zccrs/github-actions-spdx-checker@v1。这是一个第三方 Action。
    • 建议:为了防止供应链攻击,建议使用 Commit SHA(如 @a1b2c3d)来锁定 Action 的版本,而不是使用 Tag @v1。Tag 可以被移动或篡改,而 Commit SHA 是不可变的。如果必须使用 Tag,请确保您信任该维护者。
  • 权限控制
    • 问题:代码片段中未显示 Job 级别的 permissions 设置。
    • 建议:遵循最小权限原则。如果该 Job 只是读取代码进行检查,不需要写入权限(contents: write)。建议显式添加:
    permissions:
      contents: read
    这样即使攻击者篡改了 Action 脚本,也无法利用此 Job 向仓库写入内容或修改代码。

改进后的代码示例

结合以上建议,改进后的代码片段如下:

  Check-SPDX-Copyright:
    runs-on: ubuntu-latest
    # 显式设置最小权限
    permissions:
      contents: read
    # 如果希望在其他检查通过后再运行,取消下面的注释并根据需要修改依赖的 Job 名称
    # needs: [job_name_1, job_name_2] 
    steps:
      - uses: actions/checkout@v5
        with:
          # 确保只在 PR 上下文中有效,否则可能需要处理非 PR 的情况
          ref: ${{ github.event.pull_request.head.sha }}
      - uses: zccrs/github-actions-spdx-checker@v1 # 建议替换为具体的 commit SHA 以提高安全性
        with:
          base: origin/master
          include: '*.py,*.js,*.jsx,*.tsx,*.java,*.cpp,*.c,*.h,*.hpp,*.go,*.rs,*.rb,*.php,*.sh,*.xml,*.yaml,*.yml,*.qml,CMakeLists.txt,Makefile'
          exclude: 'vendor/**,node_modules/**'
          # 建议将持有者名称提取为变量,方便统一管理
          holder: '*UnionTech*'

总结

这段代码逻辑基本正确,能够实现检查版权的目的。主要的改进点在于安全性(锁定第三方 Action 版本、限制权限)和健壮性(明确触发条件和依赖关系)。

- uses: actions/checkout@v5
with:
ref: ${{ github.event.pull_request.head.sha }}
- uses: zccrs/github-actions-spdx-checker@v1
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self:需等待 zccrs/github-actions-spdx-checker 补充 v1 tag,或转用 @main

Copy link
Member Author

@BLumia BLumia left a comment

Choose a reason for hiding this comment

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

暂时请勿合入,简单测试发现此 action 无效:

  1. 需要等待一个有效的 v1 tag 或者转用 @main
  2. 检查无效:测试 PR CI 结果

@BLumia BLumia marked this pull request as draft January 22, 2026 12:00
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