ci(security): run CodeQL on fork pull requests - #36
Merged
Merged
Conversation
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
approved these changes
Sep 14, 2026
Bindy-lbb
left a comment
Collaborator
There was a problem hiding this comment.
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 安全扫描做法。✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
One line deleted from
workflow-templates/bytefolk-security.yml— theif:guard on thecodeqljob. 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.04Blob 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 ungatedAutobuildstep 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
codeqljob is skipped. GitHub does not evaluate a skipped job'sname:expression, so the skipped job publishes the raw stringCodeQL (${{ 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-forklanebytefolk/digital-employee#250worked around this by adding a second job with the inverted guard. That job is identical to the baselinecodeqljob 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 clausegithub.event_name != 'pull_request'is already true.pull_request— the second clause is already true.pull_request— the job was skipped; it now runs. This is the only behavioural change.Fork
pull_requestruns can carry this job:digital-employee#250'scodeql-forkconcludedsuccesson a fork PR while declaringpermissions: security-events: writeand containing nocontinue-on-error, which means theAnalyzestep succeeded — andcodeql-action/analyzereturns HTTP 403 without that permission. Fork runs still get no secrets and read-onlycontents. 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-onlycontents, 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-forklane must delete it once it adopts this template, otherwise two jobs publish the same check name. Today that isdigital-employeevia #250. This PR deliberately does not touchdigital-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) plusbytefolk-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
permissions:block changes. Thecodeqljob keeps exactlycontents: read,actions: read,packages: read,security-events: write.Refs #35