Skip to content

fix(workflows): harden pr-labels-ci.yml against fork-PR shell injection (#332)#333

Merged
cmeans-claude-dev[bot] merged 2 commits into
mainfrom
fix/pr-labels-ci-hardening-332
Apr 20, 2026
Merged

fix(workflows): harden pr-labels-ci.yml against fork-PR shell injection (#332)#333
cmeans-claude-dev[bot] merged 2 commits into
mainfrom
fix/pr-labels-ci-hardening-332

Conversation

@cmeans-claude-dev
Copy link
Copy Markdown
Contributor

@cmeans-claude-dev cmeans-claude-dev Bot commented Apr 20, 2026

Closes #332.

Summary

.github/workflows/pr-labels-ci.yml on this repo was in the pre-cmeans/mcp-clipboard#88 state for the sibling cmeans/* cascade: contributor-controlled workflow_run fields (head_branch, id, repository) were inlined directly as ${{ ... }} expressions inside run: bodies. Git refnames allow shell metacharacters ($, backtick, ;, &, |, etc.), so a malicious fork-PR branch name would render as directly-executed shell once GHA substituted the expression at queue time.

This PR cascades the hardening from cmeans/mcp-clipboard:

  1. Bump codecov/codecov-action from 5 to 6 #88 pattern — env routing. All contributor-controlled values now flow through step-level env: blocks and are referenced as shell variables ("$HEAD_BRANCH", "$REPO", "$RUN_ID"). Job-level if: conditionals continue to use ${{ ... }} (that expression context is safe — it's evaluated by GHA itself, not handed to a shell).
  2. Make entries.updated nullable — NULL on insert, set only on update #92 pattern — comment escape. Comments inside run: blocks avoid any literal ${{ }} sequence. GHA's queue-time parser substitutes expressions even inside shell # comments and rejects empty expressions with An expression was expected on workflow_dispatch / fresh-repo registration. Without this, the Bump codecov/codecov-action from 5 to 6 #88 hardening would have landed a latent trap.

Cascaded verbatim from cmeans/mcp-clipboard's .github/workflows/pr-labels-ci.yml at the main HEAD after PR #92 merged (2026-04-20T20:26:12Z), preserving this repo's AGPL-3.0 header at lines 1–16. No functional behavior change — label transitions, PR lookup, force-push tolerance, and Dev Active skip logic are all byte-identical to before, just with shell values now arriving via environment instead of via text substitution.

Scope

  • .github/workflows/pr-labels-ci.yml — hardened (22 lines changed)
  • CHANGELOG.md[Unreleased] → ### Security entry

No source, tests, migrations, or docs.

References

QA

Prerequisites

Pure workflow-file change. No deploy, no env setup, no Python changes. All verification is static (yq/python -c, grep) plus an optional post-merge Actions UI smoke.

Automated checks

The CHANGELOG check, ruff, mypy, pytest, Codecov — none of these touch the workflow file. They should all pass unchanged against main. Confirm via the green CI checks on this PR before approving.

Manual tests

All local verification runs from the repo root on the PR branch (fix/pr-labels-ci-hardening-332).

    • YAML parse is clean.
      python3 -c "import yaml; yaml.safe_load(open('.github/workflows/pr-labels-ci.yml')); print('OK')"
      
      Expected: prints OK, exit 0. Confirms no syntax regression.
    • No literal empty ${{ }} anywhere in the file.
      grep -n '\${{ }}' .github/workflows/pr-labels-ci.yml || echo "(none — good)"
      
      Expected: prints (none — good). Guarantees the #92 parser trap can't surface on workflow_dispatch / fresh-repo registration.
    • No contributor-controlled field appears inside a run: body as an expression.
      awk '/^        run: \|/,/^      - name:|^  [^ ]|^jobs:/' .github/workflows/pr-labels-ci.yml | grep -nE '\$\{\{ *github\.event\.workflow_run\.(head_branch|id) *\}\}' || echo "(none — good)"
      
      Expected: prints (none — good). The only remaining ${{ github.event.workflow_run.head_branch }} references are inside job-level if: conditionals (safe context, evaluated by GHA not a shell) and step-level env: assignments (safe by design — the whole point of the cascade).
    • env: blocks route the three contributor-controlled fields.
      grep -nE '^[[:space:]]+(REPO|RUN_ID|HEAD_BRANCH):' .github/workflows/pr-labels-ci.yml
      
      Expected: six lines total (three per job), each assigning from the matching ${{ github.event.workflow_run.* }} / ${{ github.repository }} expression.
    • workflows: [CI] still matches the CI workflow name in this repo.
      grep -A1 'workflows:' .github/workflows/pr-labels-ci.yml | head -3; grep '^name:' .github/workflows/ci.yml
      
      Expected: workflows: [CI] appears in this file AND name: CI appears in .github/workflows/ci.yml. Names match, so workflow_run will still fire on CI completion.
    • AGPL header preserved.
      sed -n '1,16p' .github/workflows/pr-labels-ci.yml
      
      Expected: lines 1–16 are the existing mcp-awareness — ambient system awareness for AI agents / Copyright (C) 2026 Chris Means / AGPLv3 preamble. Unchanged from main.
    • Diff against main is hardening-only (no behavior drift).
      git diff origin/main -- .github/workflows/pr-labels-ci.yml
      
      Expected: only adds REPO:, RUN_ID:, HEAD_BRANCH: to the two env: blocks, deletes the shell-level REPO=${{...}} / HEAD_BRANCH=${{...}} assignments, rewrites API_OUT=...${{ ... }}/pull_requests to .../$RUN_ID/pull_requests, and updates two comment blocks to avoid ${{ }} literals. No changes to if: conditionals, permissions, on:, job names, label manipulation logic, or exit 0 paths.
    • (Post-merge) Smoke the label automation end-to-end. This workflow is workflow_run-triggered (no workflow_dispatch), so pre-merge dispatch isn't available. After merge, take any in-flight Awaiting CI PR and confirm the label transitions still happen on the next CI completion:

Out of scope (explicitly)

  • pr-labels.yml (the on: pull_request: sibling) is not in this PR. Its current trigger makes secrets.GITHUB_TOKEN read-only for fork PRs and the values it does inline into run: blocks today are all safe types (numeric, repo name, hex SHA), so no injection vector exists at the current configuration. Symmetric env-routing as defense-in-depth tracked in #334 (P3, non-blocking).
  • Adding workflow_dispatch to pr-labels-ci.yml for post-merge verifiability. Not cascaded because mcp-clipboard doesn't have it either. Track separately if desired.

…on (#332)

Route contributor-controlled workflow_run fields (head_branch, id,
repository) through step-level env: blocks instead of inlining them as
${{ ... }} expressions inside run: bodies. Git refnames allow shell
metacharacters, so a malicious fork branch name would previously render
as executed shell once GHA substituted the expression.

Also cascades the PR-#28 comment-escape discipline: comments inside
run: blocks avoid any literal "\${{ }}" sequence (GHA's queue-time
parser substitutes them even inside shell # comments and rejects empty
expressions on workflow_dispatch).

Cascaded verbatim from cmeans/mcp-clipboard (PR #87 hardening + PR #90
comment rewording), preserving this repo's AGPL-3.0 header.

Closes #332.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA label Apr 20, 2026
@cmeans-claude-dev cmeans-claude-dev Bot added the Dev Active Developer is actively working on this PR; QA should not start label Apr 20, 2026
@github-actions github-actions Bot removed the Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA label Apr 20, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cmeans-claude-dev cmeans-claude-dev Bot removed the Dev Active Developer is actively working on this PR; QA should not start label Apr 20, 2026
@github-actions github-actions Bot added the Ready for QA Dev work complete — QA can begin review label Apr 20, 2026
@cmeans cmeans added the QA Active QA is actively reviewing; Dev should not push changes label Apr 20, 2026
@cmeans
Copy link
Copy Markdown
Owner

cmeans commented Apr 20, 2026

QA Active — starting review of the pr-labels-ci.yml hardening cascade.

@github-actions github-actions Bot removed the Ready for QA Dev work complete — QA can begin review label Apr 20, 2026
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

QA Review — Round 1

Verdict: QA Failed.

The functional cascade is correct: byte-identical to cmeans/mcp-clipboard/.github/workflows/pr-labels-ci.yml from line 17 onward (post-AGPL header), all 7 pre-merge QA steps pass, CI is fully green, no behavior drift in the diff. The blockers are in the PR body and CHANGELOG references — they point at mcp-clipboard PRs that don't reflect what actually merged, and they mischaracterize pr-labels.yml's trigger. A future maintainer chasing the cascade trail will hit dead ends.

Findings

  1. Substantive — stale cmeans/mcp-clipboard PR references in PR body and CHANGELOG.

    • The PR body and CHANGELOG.md cite cmeans/mcp-clipboard#87 as the "original security hardening." PR #87 does not exist in that repo (gh pr view 87 --repo cmeans/mcp-clipboard returns Could not resolve to a PullRequest with the number of 87). The actually-merged hardening PR is #88fix(workflows): harden GitHub Actions context handling against script injection, merged 2026-04-20T01:31:47Z.
    • The PR body cites cmeans/mcp-clipboard#90 (open) as the comment-escape cascade source. PR #90 is CLOSED, not merged (fix(workflows): remove literal dollar-brace-brace from pr-labels-ci.yml comments). The actually-merged comment-escape PR is #92fix(workflows): remove literal empty GHA expression from pr-labels-ci.yml comments, merged 2026-04-20T20:26:12Z (about a minute after this PR was opened).
    • Fix: update the body's narrative paragraph (Cascaded verbatim from cmeans/mcp-clipboard's fix/pr-labels-ci-escape-comments branch (PR #90, CI green, awaiting QA)), the References section, and the ### Security CHANGELOG entry to point at #88 and #92.
  2. Substantive — pr-labels.yml mischaracterized as pull_request_target in "Out of scope".
    The PR body says "pr-labels.yml (the pull_request_target sibling) is not in this PR." The file actually uses on: pull_request: (see .github/workflows/pr-labels.yml:22). That distinction is load-bearing: under pull_request from a fork, secrets.GITHUB_TOKEN is read-only and the gh pr edit --add-label calls would be rejected — so the shell-injection scenario this PR hardens against does not apply to that file at its current trigger configuration. Correct the description so future security audits don't start from a false premise.

  3. Observation / suggested follow-up — pr-labels.yml symmetric hardening.
    pr-labels.yml does still inline contributor-touchable values directly into run: blocks (e.g., PR=${{ github.event.pull_request.number }}, REPO=${{ github.repository }}, HEAD_SHA=${{ github.event.pull_request.head.sha }}). All current values are safe types (numeric, repo name, hex SHA), and the pull_request trigger blocks fork-PR writes anyway, so there's no injection vector today. But a future trigger change to pull_request_target, or someone parameterizing the workflow with a contributor-controlled string field, would re-introduce the same class of bug. Defense-in-depth would benefit from cascading the env-routing pattern symmetrically. Please file a follow-up issue (nice-to-have, not blocking).

Steps verified (all pre-merge steps pass)

Step Result
1. YAML parse clean OK
2. No literal empty ${{ }} (none — good)
3. No contributor-controlled fields in run: bodies (none — good)
4. env: blocks route the three fields 6 lines (3 per job) ✓
5. workflows: [CI] matches name: CI in ci.yml
6. AGPL header preserved (lines 1–16)
7. Diff is hardening-only, no behavior drift
8. Post-merge label-automation smoke Deferred (workflow_run-only trigger)

Cascade cross-check

  • diff <(sed -n '17,$p' .github/workflows/pr-labels-ci.yml) ../mcp-clipboard/.github/workflows/pr-labels-ci.yml → no differences. "Verbatim cascade" holds at the file-content level.
  • git diff origin/main shows env-routing and comment-rewording only. No changes to if: conditionals, permissions, on:, job names, label manipulation logic, or exit 0 paths.
  • All CI checks green: lint, typecheck, test (3.10/3.11/3.12), codecov/patch, license/cla.

Round 2

Fix findings 1 and 2 in this PR; file an issue for finding 3 (or fold it in if you prefer). I'll re-verify cascade equivalence and re-read the references on round 2.

@cmeans
Copy link
Copy Markdown
Owner

cmeans commented Apr 20, 2026

Audit: applying QA Failed. Cascade content is byte-identical to mcp-clipboard's pr-labels-ci.yml and all 7 pre-merge QA steps pass under SHA f09b5a8, but two substantive doc-drift findings (stale mcp-clipboard PR refs in body + CHANGELOG; pr-labels.yml mischaracterized as pull_request_target) need a fix before signoff. Details in the review above. Removing QA Active, applying QA Failed as the final act.

@cmeans cmeans added QA Failed QA found issues — needs dev attention and removed QA Active QA is actively reviewing; Dev should not push changes labels Apr 20, 2026
@cmeans-claude-dev cmeans-claude-dev Bot added Dev Active Developer is actively working on this PR; QA should not start and removed QA Failed QA found issues — needs dev attention labels Apr 20, 2026
Addresses QA round-1 finding #1 on PR #333. The actually-merged
mcp-clipboard PRs are:

  - #88 (merged 2026-04-20T01:31:47Z) — original env-routing hardening.
    #87 does not exist in that repo.
  - #92 (merged 2026-04-20T20:26:12Z) — comment-escape cascade.
    #90 was superseded and closed without merging.

No code change; CHANGELOG narrative only. PR body updated in parallel
with the same corrections (including the pull_request / pull_request_target
trigger correction for pr-labels.yml). Symmetric hardening follow-up
for pr-labels.yml filed as #334 (P3, non-blocking).

Refs #332.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA label Apr 20, 2026
@cmeans-claude-dev
Copy link
Copy Markdown
Contributor Author

Dev Response — Round 2

Thanks for the thorough review. Both substantive findings verified and addressed; filing the third as a follow-up.

Finding 1 — stale cmeans/mcp-clipboard PR references ✅ Fixed

Verified:

  • gh pr view 87 --repo cmeans/mcp-clipboardCould not resolve to a PullRequest with the number of 87
  • gh pr view 88 --repo cmeans/mcp-clipboardMERGED at 2026-04-20T01:31:47Z, title fix(workflows): harden GitHub Actions context handling against script injection — this is the actual env-routing hardening.
  • gh pr view 90 --repo cmeans/mcp-clipboardCLOSED, not merged.
  • gh pr view 92 --repo cmeans/mcp-clipboardMERGED at 2026-04-20T20:26:12Z, title fix(workflows): remove literal empty GHA expression from pr-labels-ci.yml comments — this is the actual comment-escape PR, merged about a minute after I opened this PR.

Fixed in commit 0e636a5 (CHANGELOG) and the PR body edit alongside:

Finding 2 — pr-labels.yml mischaracterized as pull_request_target ✅ Fixed

Verified: .github/workflows/pr-labels.yml:21-24 is:

on:
  pull_request:
    branches: [main]
    types: [opened, synchronize, labeled, unlabeled]

Fixed in the PR-body "Out of scope" block — reworded to identify the actual trigger (on: pull_request:), note why the injection scenario this PR hardens against does not apply under that trigger today (fork PRs get a read-only GITHUB_TOKEN, so the gh pr edit label-write calls would be rejected regardless of branch-name content), and link to the new follow-up issue.

Finding 3 — symmetric env-routing for pr-labels.yml ✅ Filed as #334

Labeled enhancement, P3: low. Issue explains:

  • Why it is not a latent security bug at the current pull_request trigger (read-only token on fork PRs + all currently-inlined values are safe types: numeric / repo name / hex SHA).
  • Why to cascade the pattern anyway: trigger-drift risk, parameterization-drift risk, cascade-consistency across cmeans/* repos.
  • Concrete scope and acceptance checks.

Kept out of this PR to preserve the one-concern-per-PR discipline and the "cascade verbatim, no drift" property of the file-content cascade.

Head SHA for re-verification

0e636a5 on fix/pr-labels-ci-hardening-332. The .github/workflows/pr-labels-ci.yml content is unchanged from round 1 (f09b5a8) — all round-2 changes are doc-only (CHANGELOG + PR body). Transitioning back to Ready for QA after CI goes green.

@cmeans-claude-dev cmeans-claude-dev Bot removed the Dev Active Developer is actively working on this PR; QA should not start label Apr 20, 2026
@github-actions github-actions Bot added Ready for QA Dev work complete — QA can begin review and removed Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA labels Apr 20, 2026
@cmeans cmeans added the QA Active QA is actively reviewing; Dev should not push changes label Apr 20, 2026
@cmeans
Copy link
Copy Markdown
Owner

cmeans commented Apr 20, 2026

QA Active — round 2, verifying the doc fixes on 0e636a5 and the #334 follow-up.

@github-actions github-actions Bot removed the Ready for QA Dev work complete — QA can begin review label Apr 20, 2026
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

QA Review — Round 2

Verdict: Ready for QA Signoff.

All round-1 findings closed. The workflow file is byte-identical to round 1 (only CHANGELOG.md was touched in 0e636a5, and the PR body was edited in parallel). All 7 pre-merge QA steps re-run against 0e636a5 pass. CI fully green. Cascade still byte-identical to cmeans/mcp-clipboard/.github/workflows/pr-labels-ci.yml.

Round-1 finding closure

# Finding Round-1 Round-2
1 PR body + CHANGELOG cite mcp-clipboard#87 (404) and #90 (closed) broken refs now reference #88 (merged hardening) and #92 (merged comment escape) — verified via grep for #87/#90 returns no hits in PR body or CHANGELOG.md
2 PR body's "Out of scope" mischaracterizes pr-labels.yml as pull_request_target wrong premise now reads "(the on: pull_request: sibling)" with accurate read-only-token rationale
3 Symmetric env-routing for pr-labels.yml (defense-in-depth) suggested follow-up filed as #334 (P3, enhancement) — well-scoped, accurate trigger analysis, defense-in-depth + drift-risk rationale, mechanical acceptance criteria

Re-verification (0e636a5)

Step Result
1. YAML parse OK
2. No literal empty ${{ }} (none — good)
3. No contributor fields in run: bodies (none — good)
4. env: blocks route the three fields 6 lines (3/job) ✓
5. workflows: [CI] matches name: CI in ci.yml
6. AGPL header preserved
7. git diff origin/main hardening-only, 22 lines (13/9) ✓
8. Post-merge label-automation smoke Deferred (workflow_run-only trigger)

CI rollup on 0e636a5: lint, typecheck, test (3.10/3.11/3.12), codecov/patch, license/cla — all SUCCESS.

Follow-ups outstanding (non-blocking)

  • #334 — symmetric env-routing for pr-labels.yml (P3, defense-in-depth). Not exploitable today (pull_request trigger ⇒ read-only fork token; inlined values are all safe types), so this is genuinely nice-to-have, not gating.

Maintainer to apply QA Approved when ready.

@cmeans
Copy link
Copy Markdown
Owner

cmeans commented Apr 20, 2026

Audit: applying Ready for QA Signoff as the final act. Round-2 verified on 0e636a5: all 7 pre-merge QA steps re-pass, cascade still byte-identical to mcp-clipboard, all CI checks green, all 3 round-1 findings closed (CHANGELOG + PR body refs corrected to #88/#92, pr-labels.yml trigger correctly described as on: pull_request:, follow-up #334 filed). Removing QA Active, applying Ready for QA Signoff. Maintainer to apply QA Approved.

@cmeans cmeans added Ready for QA Signoff QA passed — ready for maintainer final review and merge and removed QA Active QA is actively reviewing; Dev should not push changes labels Apr 20, 2026
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

LGTM

@cmeans cmeans added QA Approved Manual QA testing completed and passed and removed Ready for QA Signoff QA passed — ready for maintainer final review and merge labels Apr 20, 2026
@cmeans-claude-dev cmeans-claude-dev Bot merged commit 0349dd0 into main Apr 20, 2026
36 checks passed
@cmeans-claude-dev cmeans-claude-dev Bot deleted the fix/pr-labels-ci-hardening-332 branch April 20, 2026 21:24
cmeans-claude-dev Bot added a commit that referenced this pull request Apr 21, 2026
Patch release stamping the hygiene work shipped this session. **No code
changes** beyond the version bump and CHANGELOG reshuffle — everything
under this release landed in its own feature PR with full QA.

## What ships in v0.18.1

| PR | Summary |
|----|---------|
| [#333](#333) |
**Security:** hardened `pr-labels-ci.yml` against fork-PR shell
injection (closes
[#332](#332)) |
| [#335](#335) |
`.github/CODEOWNERS` — maintainer auto-requested on every PR |
| [#336](#336) |
`SECURITY.md` vulnerability disclosure policy (closes
[#309](#309)) |
| [#337](#337) |
`docs/backup.md` self-hoster backup + restore guide (closes
[#310](#310)) |
| [#343](#343) |
`.github/dependabot.yml` expanded to 4 ecosystems with grouped weekly
updates |

Semver is **patch** (`0.18.0 → 0.18.1`): all changes are CI, security
hardening, policy docs, and operational tooling — no runtime behavior
change for the Python package.

## Changes in this PR

- **`CHANGELOG.md`** — `## [Unreleased]` content renamed to `## [0.18.1]
- 2026-04-20`; a fresh empty `[Unreleased]` header added above it to
accumulate future work; `[Unreleased]` comparison link updated to point
at `v0.18.1...HEAD`; new `[0.18.1]` link added pointing at
`v0.18.0...v0.18.1`.
- **`pyproject.toml`** — `version = "0.18.0"` → `"0.18.1"`.

No source, no tests, no migrations. `docker-compose.yaml` uses `:latest`
— no update needed per the release process.

## Review

Docs-and-version-only change; no QA section per the repo's release
process (feature PRs already carried their own QA). A reviewer should
verify:

1. `head -12 CHANGELOG.md` shows a fresh `## [Unreleased]` followed by
`## [0.18.1] - 2026-04-20` — both present, in that order.
2. `grep '^\[Unreleased\]:' CHANGELOG.md` resolves to `v0.18.1...HEAD`.
3. `grep '^\[0\.18\.1\]:' CHANGELOG.md` resolves to `v0.18.0...v0.18.1`.
4. `grep '^version' pyproject.toml` → `0.18.1`.
5. `git diff --stat origin/main` shows exactly `CHANGELOG.md` +
`pyproject.toml`, nothing else.

## Merge + tag (maintainer)

After `QA Approved` lands and CI is green:

```bash
gh pr merge <this-pr> --repo cmeans/mcp-awareness --squash --delete-branch
git checkout main && git pull --ff-only
git tag -a v0.18.1 -m "v0.18.1 — beta-readiness hygiene bundle (SECURITY.md, backup guide, CODEOWNERS, dependabot, workflow hardening)"
git push origin v0.18.1
```

Docker Publish workflow will build and publish
`ghcr.io/cmeans/mcp-awareness:v0.18.1` and update `:latest` on tag push.

Co-authored-by: cmeans-claude-dev[bot] <3223881+cmeans-claude-dev[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cmeans-claude-dev Bot added a commit that referenced this pull request Apr 21, 2026
…334) (#351)

Closes [#334](#334). Also
closes CodeQL alerts #1, #2, #3 (three flags of
`actions/missing-workflow-permissions` in `ci.yml`).

## Summary

Two workflow-hardening fixes bundled because they're the same theme
(least-privilege + contributor-controlled-input discipline) and both
surfaced from the same security review:

### 1. `pr-labels.yml` — cascade the `#333` env-routing pattern (closes
#334)

Three job steps in `pr-labels.yml` (`on-push`, `on-unlabel`, `on-label`)
previously inlined contributor-visible fields as `${{ ... }}`
expressions inside `run:` bodies:

```yaml
# Before
run: |
  PR=${{ github.event.pull_request.number }}
  REPO=${{ github.repository }}
  HEAD_SHA=${{ github.event.pull_request.head.sha }}
  # ... shell script references PR / REPO / HEAD_SHA
```

Now they route through step-level `env:` and are referenced as shell
variables:

```yaml
# After
env:
  PR: ${{ github.event.pull_request.number }}
  REPO: ${{ github.repository }}
  HEAD_SHA: ${{ github.event.pull_request.head.sha }}
run: |
  # ... shell script references "$PR" / "$REPO" / "$HEAD_SHA"
```

**Not a currently-exploitable bug.** The `on: pull_request:` trigger
means fork PRs get a read-only `GITHUB_TOKEN` — the `gh pr edit
--add-label` / `--remove-label` calls would be rejected from a fork PR
regardless of what `PR`/`REPO`/`HEAD_SHA` contained. And all three
values are typed (numeric PR, repo name validated by GHA, hex SHA) —
none come from user-authored text like titles or bodies.

**Why change it anyway**, per #334's rationale:

- **Trigger-drift risk.** If `pr-labels.yml` ever switches to
`pull_request_target` (to allow label automation on fork PRs), the same
injection class that #333 closed on `pr-labels-ci.yml` reappears — and
now the hardening would already be in place.
- **Parameterization-drift risk.** A future maintainer adding a
contributor-authored string field (label name, PR title fragment, branch
name) to a `run:` block won't be prompted to route via `env:` first
because the file already establishes the inline `${{ ... }}` style as
"fine here."
- **Cascade consistency.** `pr-labels-ci.yml` uses env-routing since
#333; having the sibling workflow use a different style is a readability
cost for anyone auditing the repo.

### 2. `ci.yml` — add workflow-level `permissions: contents: read`
(closes CodeQL #1/#2/#3)

`ci.yml` had no `permissions:` block at workflow or job level, so all
three jobs (`lint`, `typecheck`, `test`) inherited whatever repo-level
default `GITHUB_TOKEN` scope is configured. CodeQL flagged this three
times (one per job).

Fix: declare `permissions: contents: read` at the workflow level. Every
job inherits read-only content access, which is sufficient for lint /
typecheck / pytest / codecov. No job actually needs write access to
anything.

## Audit sweep results

While touching workflow files, checked all six for missing
`permissions:`:

| Workflow | Had `permissions:`? | This PR's action |
|----------|---------------------|------------------|
| `ci.yml` | No (CodeQL flagged 3x) | Added `contents: read` at workflow
level |
| `docker-publish.yml` | Yes, line 23 | No change |
| `docker-smoke.yml` | Yes, line 40 (from #350) | No change |
| `pr-labels-ci.yml` | Yes, line 35 (from #333) | No change |
| `pr-labels.yml` | Yes, line 26 | No change to permissions block;
env-routing changes only |
| `qa-gate.yml` | Yes, line 24 | No change |

`ci.yml` was the last gap. Sweep is complete.

## Scope

- `.github/workflows/ci.yml` — `+7 lines` (permissions block with inline
rationale comment)
- `.github/workflows/pr-labels.yml` — `+10, -11` (three `run:` bodies
lose two shell-assignment lines each; three `env:` blocks gain two-three
entries each; explanatory comment added in the `on-unlabel` case)
- `CHANGELOG.md` — `+4 lines` (new `### Security` subsection under
`[Unreleased]`)

No source, no tests, no migrations.

## References

- Closes [#334](#334)
- Closes CodeQL alerts #1, #2, #3
(`actions/missing-workflow-permissions` on `ci.yml:27/41/53`)
- Cascade source: PR
[#333](#333) (same pattern
for `pr-labels-ci.yml`, which closed #332)
- Related CodeQL alerts not addressed by this PR: #5/#6/#7/#8 (OAuth
clear-text logging in `oauth.py` and `oauth_proxy.py`) — separate audit
PR, coming next. #4 (socket bind in tests) — dismiss via UI.

## QA

### Prerequisites

None. Pure workflow-YAML changes.

### Automated checks

- `lint`, `typecheck`, `test (3.10/3.11/3.12)` — none touch YAML, should
remain green.
- `CodeQL (actions)` — will re-scan `ci.yml` and `pr-labels.yml` on this
PR. Expected outcome: alerts #1/#2/#3 flip to "fixed" on merge; no new
alerts introduced.
- `docker-smoke` — not triggered (no changes under `Dockerfile` /
`pyproject.toml` / `uv.lock` / `.dockerignore`).

### Manual tests

1. - [x] **Both workflow files parse.**
     ```
python3 -c "import yaml; [yaml.safe_load(open(f)) for f in
['.github/workflows/ci.yml', '.github/workflows/pr-labels.yml']];
print('OK')"
     ```
     Expected: `OK`.

2. - [x] **`ci.yml` now has `permissions: contents: read`.**
     ```
     grep -A1 '^permissions:' .github/workflows/ci.yml
     ```
     Expected: `permissions:` header followed by `  contents: read`.

3. - [x] **No contributor-controlled inputs in `pr-labels.yml` `run:`
bodies.**
     ```
awk '/^[[:space:]]+run: \|/,/^[[:space:]]+-
name:|^[[:space:]]{2,6}[a-z-]+:$/' .github/workflows/pr-labels.yml |
grep -nE '\$\{\{ *github\.(event|repository|head_ref)' || echo "(none —
good)"
     ```
Expected: `(none — good)`. All `github.event.*` / `github.repository`
references are now in `env:` blocks (and in job-level `if:`
conditionals, which is safe context).

4. - [x] **All six workflows now have `permissions:`.**
     ```
     for f in .github/workflows/*.yml; do
if ! grep -q '^permissions:\|^ permissions:\|^ permissions:' "$f"; then
         echo "$f: MISSING permissions"
       fi
     done
     echo "(if no 'MISSING' lines above, sweep is complete)"
     ```
     Expected: no `MISSING` lines.

5. - [x] **Label automation still functions on this PR.** When I push,
`pr-labels.yml`'s `on-push` should reset labels to `Awaiting CI` and
strip any stale QA labels. When `Dev Active` is removed, `on-unlabel`
should promote to `Ready for QA` after CI passes. Empirically validated
if the label transitions on this PR itself behave identically to recent
merged PRs (self-test).

6. - [x] **`permissions: contents: read` doesn't break anything.** Lint
/ typecheck / pytest / codecov upload only need read access to
`GITHUB_TOKEN` — none of them push labels, create comments, or mutate
repo state. If any of the existing CI checks start failing on this PR
with "resource not accessible" errors, that's a signal the permissions
block is too tight (unlikely, but the empirical test is: does this PR's
CI go green?).

7. - [x] **Diff review.**
     ```
     git diff --stat origin/main
     ```
Expected: `.github/workflows/ci.yml` (+7),
`.github/workflows/pr-labels.yml` (+10, -11), `CHANGELOG.md` (+4).
Nothing else.

### Acceptance

- ✅ `#334` — symmetric env-routing cascade landed in `pr-labels.yml`
- ✅ CodeQL `#1`, `#2`, `#3` — `ci.yml` now has explicit `permissions:`
- ☐ CodeQL re-scan confirmation — post-merge, the three alerts flip from
Open → Fixed automatically on the next `Analyze (actions)` run against
`main`

Post-merge, also worth a look at CodeQL's /security/code-scanning
dashboard to confirm Open count drops from 8 → 5 (just the four
OAuth-logging + the one test-file socket-bind remaining).

Co-authored-by: cmeans-claude-dev[bot] <3223881+cmeans-claude-dev[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cmeans-claude-dev Bot added a commit that referenced this pull request Apr 28, 2026
…#378) (#410)

## Summary

Closes #378. Two stale-label traps in `pr-labels-ci.yml` fixed
symmetrically; both rooted in narrow outer guards that only fired on
`Awaiting CI`, missing the post-`CI Failed` recovery arc and the `Ready
for QA → CI Failed` regression arc.

| Job | Today | Now |
| --- | --- | --- |
| `on-ci-pass` | Promotes only when `Awaiting CI` is present | Promotes
when `Awaiting CI` OR `CI Failed` is present |
| `on-ci-fail` | Adds `CI Failed` only when `Awaiting CI` is present |
Adds `CI Failed` when `Awaiting CI` OR `Ready for QA` is present |

### Bug 1 — `CI Failed → CI pass` silently no-ops (issue #378)

Reproduction trail in #377 (2026-04-22): a lint-failing push moved
labels to `CI Failed`; the fix-up push made CI go green; `on-ci-pass`
fired and ran, but its outer `if echo "$LABELS" | grep -q "^Awaiting
CI$"` was false (only `CI Failed` was present), so it silently no-op'd.
PR sat at `CI Failed` while CI was actually green. Required a manual `gh
pr edit --remove-label "CI Failed" --add-label "Ready for QA"` to
unstick.

### Bug 2 — `Ready for QA → CI re-fail` keeps the green label
(symmetric)

Mirror trap on `on-ci-fail`: a CI re-run on a PR sitting at `Ready for
QA` (e.g., manual re-trigger after a flake, or a workflow change forcing
a re-run) that turns red leaves the PR labelled `Ready for QA` because
the outer `if echo "$LABELS" | grep -q "^Awaiting CI$"` is false. The
status check goes red but the label still says ready — QA might pick it
up assuming CI is green.

### Review-state preservation

Broadening the triggers introduces a new risk: if a `QA Active` / `Ready
for QA Signoff` / `QA Approved` label coexists with a CI label (race, or
manual mistake), the broader trigger could overwrite review-machine
state with `Ready for QA` (on pass) or `CI Failed` (on fail). To prevent
that, both jobs now short-circuit explicitly when any of those three
labels is present:

```bash
for QA_STATE in "QA Active" "Ready for QA Signoff" "QA Approved"; do
  if echo "$LABELS" | grep -q "^$QA_STATE$"; then
    echo "$QA_STATE present — skipping (review in progress)"
    exit 0
  fi
done
```

Rationale: review state advances independently of CI re-runs. A passing
or failing CI re-run on a PR that's already in QA review is visible via
the check itself; the label transition would be redundant on success and
destructive on failure. `Dev Active` short-circuit preserved unchanged.

### Safety

- Trigger remains `workflow_run` — base-branch context, immune to
PR-branch edit attacks (same protection class as the
`pull_request_target` migration in #409).
- No new contributor-controlled inputs. Label list still read via `gh pr
view --json labels` (repo-owned strings, not fork-controlled).
- All grep patterns remain anchored (`^Label$`) so labels like `Awaiting
CI Failed` (if one ever existed) cannot accidentally satisfy a
`^Awaiting CI$` check.
- Existing env-routing of `HEAD_BRANCH` / `RUN_ID` / `PR` / `REPO`
(hardened in #332/#333) is unchanged. Nothing I add interpolates new
contributor-controlled values into shell.

### State-machine trace (full)

Pre-state → CI conclusion → resulting transition (✓ = covered, ✗ =
no-op, * = new):

| Pre-state | CI = success | CI = failure |
| --- | --- | --- |
| `Awaiting CI` | → `Ready for QA` ✓ | → `CI Failed` ✓ |
| `CI Failed` | → `Ready for QA` ✓* | stays `CI Failed` ✓ |
| `Ready for QA` | stays `Ready for QA` ✓ | → `CI Failed` ✓* |
| `Dev Active` | no-op (skip) ✓ | no-op (skip) ✓ |
| `QA Active` | no-op (skip) ✓* | no-op (skip) ✓* |
| `Ready for QA Signoff` | no-op (skip) ✓* | no-op (skip) ✓* |
| `QA Approved` | no-op (skip) ✓* | no-op (skip) ✓* |

The * entries are new in this PR. The `Dev Active` and "no pre-state"
cases were already correct.

## Test plan

Workflow YAML only. No tests to add.

## QA

### Prerequisites
- None — pure workflow YAML change.

### Manual tests

1. - [x] **Workflow YAML parses cleanly.** Confirm the Actions tab on
this PR shows no parse-error annotations on `pr-labels-ci.yml`.

2. - [x] **Diff matches the state-machine trace table above.** Read
`.github/workflows/pr-labels-ci.yml` head-to-toe; for each row of the
trace, confirm the corresponding code path emits the expected transition
(or skip).

3. - [x] **#409 migration live-validation (deferred from #409 QA test
plan #4).** This is the first PR opened against `main` since the
`pr-labels.yml` / `qa-gate.yml` migration to `pull_request_target`.
Confirm:
- `pr-labels.yml` `on-push` fired on opening: `Awaiting CI` was applied
automatically (no manual addition required this time).
- `qa-gate.yml` posted a `QA Gate` status on this PR's head SHA from app
`15368` (GitHub Actions). Visible in the status-check rollup.
- These two observations together confirm #409's migration works
end-to-end on a real PR — not just on the introduction PR's
bootstrap-skipped path.

4. - [ ] **Verification of the bug-fix itself is post-merge.**
`workflow_run` triggers always run from the default branch (per the
`LIMITATION` comment at the top of `pr-labels-ci.yml`), so this PR's
changes do not run on this PR. The natural validation is the next
CI-fail-then-pass PR after this lands — when that happens, the PR should
auto-promote `CI Failed → Ready for QA` without manual intervention.
Reviewer should add a follow-up note here (or in the awareness milestone
for this PR) once that natural validation occurs.

### Out-of-scope follow-ups (not for this PR)

- The `dismiss_stale_reviews_on_push` setting interacts with these
transitions in subtle ways (review approvals get auto-dismissed on push,
then CI re-runs). No change proposed; just flagging for awareness.
- A future enhancement could add a `QA Invalidated` style label for the
case where CI re-fails on a PR in QA review, but doing so requires
designing the QA recovery path. Out of scope for #378.

Co-authored-by: cmeans-claude-dev[bot] <272174644+cmeans-claude-dev[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

QA Approved Manual QA testing completed and passed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: pr-labels-ci.yml is pre-hardening (shell-injection risk) and lacks the PR-#28 comment escape

1 participant