Skip to content

Derive PR number from the triggering run in the size labeler - #6259

Merged
ChrisJBurns merged 2 commits into
mainfrom
harden-pr-size-labeling
Aug 10, 2026
Merged

Derive PR number from the triggering run in the size labeler#6259
ChrisJBurns merged 2 commits into
mainfrom
harden-pr-size-labeling

Conversation

@ChrisJBurns

@ChrisJBurns ChrisJBurns commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #6259's base is now main#6258 has merged.

Summary

  • The size labeler passed its target across a trust boundary in an artifact. pr-size-labeler.yml runs on pull_request, which means the pull request supplies the workflow definition that produces the artifact. pr-size-label-apply.yml then consumed it on workflow_run — in the base repository, with pull-requests: write — and took both the pull request number and the label name from it verbatim, with no validation.
  • The number is now derived from the consumer's own trigger event, and the label is checked against the five known size/* values. The artifact still carries a pull request number, but nothing reads it — see the rollout note below.

This is the standard workflow_run artifact-trust pattern: the producing workflow is attacker-influenced, the consuming workflow is privileged, and anything crossing between them is untrusted input.

Why workflow_run.pull_requests isn't enough on its own

The obvious fix is github.event.workflow_run.pull_requests[0].number. That field is empty for pull requests from forks — which is the case that matters — so a naive swap would silently stop labelling exactly those pull requests.

Measured over the last 30 runs of this workflow:

Head repository Runs pull_requests populated
forks 9 0
stacklok/toolhive 21 6

So it's unreliable in general, not just for forks. It's kept as a fast path with a listPullRequestsAssociatedWithCommit lookup on head_sha behind it, and the job fails rather than guessing if that doesn't resolve to exactly one open pull request.

Why the artifact still carries a PR number

workflow_run consumers always execute the copy of the workflow on the default branch, never the version in the pull request. So a pull request can change the producer — which does run from the pull request head — but cannot change the consumer reading its output until it merges.

Removing pr-number.txt here in the same change therefore broke the apply job for every open pull request: the producer stopped writing the file while the consumer still on main was reading it. This is a two-phase rollout instead:

  1. This PR — land the consumer change. The producer keeps writing pr-number.txt, bound through env: so it is not interpolated into the shell, and the new consumer ignores it.
  2. Follow-up — once this is on main, drop the write.

Behaviour

No change for a normal pull request, fork or not — the same label lands on the same pull request. The differences are that an unrecognised label now fails the job instead of being applied, and the target is no longer something the artifact can influence.

Part of #6253

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)

  • E2E tests (task test-e2e)

  • Linting (task lint-fix)

  • Manual testing (describe below)

  • Queried the last 30 runs of pr-size-labeler.yml through the API to confirm the pull_requests behaviour above, rather than relying on the documented behaviour.

  • Checked all three embedded github-script bodies with node --check.

  • Both workflows parse as YAML; actionlint reports 12 findings before and after, all pre-existing.

  • zizmor template-injection on pr-size-labeler.yml goes 1 → 0, as a side effect of no longer interpolating the label into the shell.

  • This pull request exercises both workflows directly. The first push dropped pr-number.txt and the apply job failed for exactly the reason above; the current revision keeps it and the job passes.

Does this introduce a user-facing change?

No.

Special notes for reviewers

  • Worth confirming the label that lands on this pull request is correct, since that is the end-to-end test.
  • listPullRequestsAssociatedWithCommit resolves fork pull requests because the head commit is reachable in the base repository via refs/pull/N/head. If it ever returns more or fewer than one open pull request the job fails loudly rather than labelling something arbitrary.
  • The size/* allow-list is duplicated between the resolve step and the existing "Remove old size labels" step. Left as-is to keep the diff focused; worth hoisting if this file grows.

Generated with Claude Code

@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.71%. Comparing base (4d77048) to head (916c1db).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6259      +/-   ##
==========================================
- Coverage   72.72%   72.71%   -0.02%     
==========================================
  Files         742      742              
  Lines       77299    77299              
==========================================
- Hits        56216    56208       -8     
- Misses      17122    17128       +6     
- Partials     3961     3963       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Base automatically changed from harden-ci-expressions to main August 10, 2026 17:26
rdimitrov
rdimitrov previously approved these changes Aug 10, 2026
pr-size-labeler.yml runs on pull_request, so a pull request supplies the
workflow definition that writes the artifact — including one from a fork.
pr-size-label-apply.yml then consumed that artifact in the base
repository, with pull-requests: write, taking both the target pull
request number and the label name from it verbatim.

The number is now derived from this job's own workflow_run event and the
label is checked against the five known size labels. The artifact no
longer carries a pull request number at all, so there is nothing left to
trust in it beyond a value that must match a fixed list.

workflow_run.pull_requests cannot be used on its own: it is empty for
every fork pull request and for most same-repository ones, so it is a
fast path with a head-commit lookup behind it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ChrisJBurns
ChrisJBurns force-pushed the harden-pr-size-labeling branch from 4709a5b to 90a87d3 Compare August 10, 2026 17:27
`workflow_run` consumers always execute the copy of the workflow on the
default branch, so a pull request can change the producer but not the
consumer reading its output. Dropping pr-number.txt here failed the
apply job on every open pull request, because the consumer still on main
reads that file.

The value is written again, bound through env: so it is not interpolated
into the shell. The new consumer ignores it; removing the write is a
follow-up once this has merged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Aug 10, 2026
@ChrisJBurns
ChrisJBurns merged commit d163115 into main Aug 10, 2026
20 checks passed
@ChrisJBurns
ChrisJBurns deleted the harden-pr-size-labeling branch August 10, 2026 17:31
@github-actions github-actions Bot mentioned this pull request Aug 14, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants