Skip to content

ci(security): fork pull requests can never satisfy the required CodeQL contexts #35

Description

@PeterGuy326

Outcome

Fork-authored pull requests can never satisfy the required CodeQL (...) status contexts in any ByteFolk repository that adopts workflow-templates/bytefolk-security.yml. External contributors are permanently blocked, and there is no action available to them that unblocks the PR.

Mechanism

workflow-templates/bytefolk-security.yml gates its single codeql job:

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

The second clause compares the head repository to the base repository, so it is false for every fork pull request. The job is skipped.

GitHub does not evaluate a job's name: expression when the job is skipped by its if: guard, so the skipped job publishes the raw template string as its check name. On bytefolk/mem the fork PRs report a check literally named CodeQL (${{ matrix.language }}), which matches none of the three required contexts.

The un-interpolated name is a symptom of the skip, not a separate defect: on bytefolk/digital-employee#250 the identical expression interpolated to CodeQL (javascript-typescript) as soon as the job actually ran.

Evidence

Required contexts on bytefolk/mem (strict: true, 12 contexts): Go, Worker, Web, Conventional title and linked issue, Workflow, scripts and Compose, PostgreSQL integration, Web memory and transfer acceptance, HTTP, CLI and MCP lifecycle, CodeQL (go), CodeQL (javascript-typescript), CodeQL (python), Dependency review.

Two fork PRs by an external contributor (sun-970), measured via GET /repos/bytefolk/mem/commits/{sha}/check-runs:

PR head total success failed skipped
bytefolk/mem#172 a29e9163 17 16 0 1
bytefolk/mem#181 4a25bd25 17 16 0 1

In both cases the single skip is CodeQL (${{ matrix.language }}). Everything else is green, and both PRs report mergeable: true. Before first-time workflow runs were approved they reported total_count: 0 — fork PRs are not listed under repos/{base}/actions/runs?head_sha={fork_sha}; the commits check-runs endpoint is the one that shows them.

A fork PR that did run CodeQL successfully. bytefolk/digital-employee#250 is a fork PR (head PeterGuy326/digital-employee:feat/issue-245-memory-config, sha abb2d58b29710927742c828a510c419c7c53efb8). Its ci.yml adds a codeql-fork job with the inverted guard. That job concluded success (check runs 102757987331 and 102757703884, 84s and 86s). The job declares permissions: security-events: write and contains no continue-on-error, so a success job conclusion means the Analyze step succeeded — and codeql-action/analyze fails with HTTP 403 when that permission is absent. Fork pull_request runs still receive no secrets and read-only contents.

Correction to an earlier hypothesis

Granting the contributor organization membership does not fix this. The guard compares repositories, not author identity, so a member's fork PR still skips. Member-authored branches were therefore mirrored into the base repository instead (bytefolk/mem#125 -> bytefolk/mem#204, bytefolk/roleweave#226 -> bytefolk/roleweave#235), which is not available for a contributor outside the organization.

pull_request_target is rejected: it would hand a write-scoped token to a workflow run over attacker-influenced code.

Self-reporting the three missing contexts through the Statuses API is also rejected: it would fabricate a required check that never ran.

Proposed fix

Delete the if: guard from the codeql job — in the template, and in each consumer that copied it.

Equivalence argument:

  • push, schedule, workflow_dispatch: the guard's first clause is already true, so those runs are unchanged.
  • Same-repository pull_request: the guard's second clause is already true, so those runs are unchanged.
  • Fork pull_request: the job now runs. This is the only behavioural change.

Guard removal is preferred over adding a second codeql-fork lane. digital-employee#250's codeql-fork is identical to the baseline codeql job apart from the inverted guard and the language matrix, so a second lane duplicates roughly 37 lines to express what deleting one line already says.

Tradeoff, stated plainly. Fork PRs would then build untrusted code in a job holding security-events: write. The run still has no secrets and read-only contents, and every other required check in these repositories already builds 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, and all ByteFolk repositories are public.

Follow-up for consumers. Any repository that added its own codeql-fork lane must delete it once the template changes, otherwise two jobs publish the same check name. Today that is digital-employee via #250.

Timing. For pull_request events GitHub takes the workflow YAML from the merge ref, so an open fork PR picks up the fix on its next push or branch update. A plain re-run replays the workflow version captured when the run was created and will not pick it up.

Acceptance criteria

  • REQ-001 / AC-001: the codeql job in workflow-templates/bytefolk-security.yml has no if: guard. No other line in the file changes.
  • REQ-002 / AC-002: each consumer repository that copied the guard receives its own separately reviewable pull request. Guard removal is not mixed into unrelated change sets, including the annotation-only pull requests tracked by chore(ci): correct CodeQL pinned-version annotations #32.
  • REQ-003 / AC-003: after merge, a fork pull request publishes CodeQL (go), CodeQL (javascript-typescript) and CodeQL (python) as real, concluded checks rather than one skipped check carrying an un-interpolated name. bytefolk/mem#172 and bytefolk/mem#181 are the acceptance targets.
  • REQ-004 / AC-004: no branch protection rule, required context or ruleset is weakened to make this pass, and no permission beyond the existing security-events: write is added.

Ownership

Refs bytefolk/mem#172 · Refs bytefolk/mem#181 · Refs bytefolk/digital-employee#250 · Refs #32

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:integrationCross-repository integrationpriority:p0Required for the next shared product proofstatus:readyScope and acceptance criteria are ready for implementationtype:maintenanceReliability, security, release, or engineering maintenance

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions