Skip to content

ci(security): run CodeQL on fork pull requests - #36

Merged
PeterGuy326 merged 2 commits into
mainfrom
ci/35-codeql-run-on-fork-pull-requests
Sep 14, 2026
Merged

PeterGuy326 merged 2 commits into
mainfrom
ci/35-codeql-run-on-fork-pull-requests

Conversation

@PeterGuy326

Copy link
Copy Markdown
Contributor

What this changes

One line deleted from workflow-templates/bytefolk-security.yml — the if: guard on the codeql job. Nothing else in the template changes.

   codeql:
     name: CodeQL (${{ matrix.language }})
-    if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }}
     runs-on: ubuntu-24.04

Blob size goes 1979 -> 1858 bytes. The deleted line is exactly 121 bytes including its newline, so the arithmetic accounts for the whole delta. The double-quoted cron, the # Change this list to the languages present in the repository. comment, the single-language matrix, the ungated Autobuild step and every pinned action SHA are untouched.

Why

The guard's second clause compares the head repository to the base repository, so it is false for every fork pull request and the codeql job is skipped. GitHub does not evaluate a skipped job's name: expression, so the skipped job publishes the raw string CodeQL (${{ matrix.language }}) as its check name and matches none of the per-language contexts consumers declare as required.

Every repository that adopted this template inherited that behaviour. The full evidence, the mechanism and the rejected alternatives are recorded in #35 rather than repeated here.

The consumer-side fix is bytefolk/mem#205, which deletes the same line from mem's copy. That PR is same-repository, so its own three CodeQL contexts can demonstrate the fix working. This template PR is filed separately so the fix propagates on the next template sync instead of regressing.

Why deleting the guard rather than documenting a second codeql-fork lane

bytefolk/digital-employee#250 worked around this by adding a second job with the inverted guard. That job is identical to the baseline codeql job apart from the guard and the language matrix, so a second lane duplicates roughly 37 lines to express what deleting one line already says — and it has to be re-duplicated in every consumer.

The guard is provably redundant for every trigger other than a fork PR:

  • push, schedule, workflow_dispatch — the first clause github.event_name != 'pull_request' is already true.
  • Same-repository pull_request — the second clause is already true.
  • Fork pull_request — the job was skipped; it now runs. This is the only behavioural change.

Fork pull_request runs can carry this job: digital-employee#250's codeql-fork concluded success on a fork PR while declaring permissions: security-events: write and containing no continue-on-error, which means the Analyze step succeeded — and codeql-action/analyze returns HTTP 403 without that permission. Fork runs still get no secrets and read-only contents. All ByteFolk repositories are public, so the private-repository restriction on fork-PR permissions does not apply here.

Tradeoff, stated plainly

Consumers that adopt the corrected template will build untrusted fork code in a job holding security-events: write. Those runs have no secrets and read-only contents, and the other baseline jobs already build the same untrusted code, so this adds no new class of exposure. It is the posture GitHub's own default CodeQL setup takes for public repositories.

Adopting this is a per-repository decision, not an automatic one: the template only changes what a consumer gets the next time it syncs, and a consumer that wants the old behaviour can keep its guard.

Consumer follow-up required

Any consumer that added its own codeql-fork lane must delete it once it adopts this template, otherwise two jobs publish the same check name. Today that is digital-employee via #250. This PR deliberately does not touch digital-employee; that removal belongs in its own reviewable change against that repository.

Interaction with #33

#33 edits this same file, but only the three github/codeql-action/* annotation lines (# v4.37.4 -> # v4.37.9) plus bytefolk-scorecard.yml. This PR deletes the guard line above them. The hunks do not overlap, so the two 3-way-merge cleanly in either order, and #32's AC-003 requirement that annotation changes not be mixed into unrelated PRs is preserved in both directions.

What this does not touch

  • No permissions: block changes. The codeql job keeps exactly contents: read, actions: read, packages: read, security-events: write.
  • No required status context, branch protection rule or ruleset is weakened or removed, in this repository or any consumer.
  • No action SHA, trigger, matrix entry, step or job other than the deleted line.
  • No annotation correction; those stay in chore(ci): correct CodeQL pinned-version annotations #33.

Refs #35

The codeql job guard compared the head repository to the base repository, so it
was false for every fork pull request and the job was skipped. GitHub does not
evaluate a skipped job's name expression, so the skipped job published the raw
string "CodeQL (${{ matrix.language }})" and matched none of the per-language
required contexts that consumer repositories declare.

Deleting the guard leaves push, schedule, workflow_dispatch and same-repository
pull_request runs unchanged: the guard was already true for all of them.

Consumers that added a bespoke codeql-fork lane must delete it, otherwise two
jobs publish the same check name.

Refs #35

@Bindy-lbb Bindy-lbb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. 单行改动,移除 CodeQL job 上的 fork PR 过滤条件:

  • 删除 if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }}
  • 目的是让 fork 来的 PR 也能触发 CodeQL 扫描,增强安全审查覆盖。
  • 风险可控:CodeQL 用 read-only token 运行,不涉及 secret 泄露,这是 GitHub 推荐的 fork PR 安全扫描做法。✅

@PeterGuy326
PeterGuy326 merged commit 78329a8 into main Sep 14, 2026
1 check passed
@PeterGuy326
PeterGuy326 deleted the ci/35-codeql-run-on-fork-pull-requests branch September 14, 2026 14:46
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