Skip to content

fix(policy): require a second reviewer for workflow changes - #4

Closed
benjiwagner wants to merge 1 commit into
mainfrom
fix/workflow-review-guard
Closed

fix(policy): require a second reviewer for workflow changes#4
benjiwagner wants to merge 1 commit into
mainfrom
fix/workflow-review-guard

Conversation

@benjiwagner

Copy link
Copy Markdown
Contributor

Summary

  • Fail repository-policy when a pull request modifies .github/workflows/** without an approving review from a code owner other than the author.
  • Read CODEOWNERS and reviews from the base ref, and resolve each reviewer's latest non-comment review state, so a pull request cannot nominate its own approvers and a dismissed or withdrawn approval does not count.
  • Skip the requirement where the repository has no code owner besides the author, since no eligible reviewer exists there.
  • Fix the --self-test harness, which could not fail.
  • Add the pull_request_review trigger so a late approval re-evaluates the check instead of leaving a stale failure.

Context

Linear CUS-109. The organization integrity ruleset has zero bypass actors and requires ci. But ci is defined by each repository's own workflow file, triggered on pull_request, so GitHub runs the definition taken from the pull-request head. A pull request that replaces its ci job body with run: true produces a genuine passing check from the pinned GitHub Actions integration, and its author can then merge it using the RepositoryRole/admin approval bypass. From that merge onward the required check verifies nothing.

Pinning integration_id binds who reports a check, not what the check ran, so it does not close this. CODEOWNERS does not close it either: code-owner review is enforced by the approval ruleset, which is exactly the rule the bypass waives. repository-policy is the only required check not exposed to head-side redefinition — its caller runs on pull_request_target, so GitHub reads the caller and its uses: ...@<sha> pin from the base branch, and the job checks out this repository at the immutable job.workflow_sha. A pull request cannot substitute its own policy revision for its own evaluation.

The self-test could not fail. A bare [[ ... ]] assertion inside a function does not abort under set -e in this script, so every assertion in the suite was unenforced. Verified against the unmodified 2af0536 revision: breaking validate_branch so it rejects every valid branch still produced Repository policy self-test passed and exit 0. The ci job here runs only bash -n plus that self-test, so this repository's own required check has been vacuous since the suite was written. Assertions now record failures through expect_error_count / expect_equal and self_test returns nonzero, with the --self-test entry point calling self_test || exit 1 explicitly.

Validation

  • bash -n repository-policy/validate.sh passes; bash repository-policy/validate.sh --self-test passes on bash 3.2 (macOS) and 5.x, hence tr rather than ${var,,}.
  • Mutation-tested — each of these makes the suite fail, where all seven passed silently before the harness fix: guard disabled entirely; any approver accepted; workflow prefix unanchored; dismissed approvals still counted; author no longer excluded from eligible owners; CODEOWNERS parser no longer dropping team handles; and validate_branch broken outright (the pre-existing blind spot).
  • New guard cases: non-workflow pull request unaffected; no approval rejected; self-approval rejected; non-code-owner approval rejected; other-code-owner approval accepted case-insensitively; sole-code-owner repository skipped; docs/.github/workflows/x.yml not treated as a workflow path.
  • All three workflow files parse as YAML.

Impact and rollback

Job permissions are unchanged (contents: read, pull-requests: read, statuses: write) and no pull-request code is checked out; the guard reads metadata through the API only.

This has no effect until each consumer repository moves its uses: ...@<sha> pin, which is deliberate follow-on work since the pin can only reference this commit once merged.

Note on scope: customermates/.github has a single code owner, so the guard is vacuous in this repository and does not gate this pull request. It becomes active where a second code owner exists — today that is customermates/knowledge-base — so the live proof belongs on the pin-bump pull request there, not here.

Roll back by reverting the squash commit. Consumers keep running whichever policy commit they pin, so a revert only takes effect when their pins move.

Screenshots

Not applicable because this pull request changes a shell validator and a workflow trigger only.

The `ci` check every repository relies on is defined by a workflow file that
GitHub evaluates from the pull-request head, so a pull request can rewrite what
its own required check executes and an approval-bypass holder can then merge it
unreviewed. Fail `repository-policy` when a pull request touches
.github/workflows/** without an approving review from a code owner other than
the author, reading CODEOWNERS and reviews from the base ref only.

Also fix the self-test harness, which could not fail: a bare [[ ... ]] assertion
inside a function does not abort under set -e here, so every assertion was
unenforced and the suite passed even with a validator broken outright.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@benjiwagner

Copy link
Copy Markdown
Contributor Author

Closing: the workflow-path guard this PR added does not contain the threat and cannot be made to by widening paths. An adversarial review confirmed 16 bypasses — the ci check is defined by package.json, scripts/, tests/, yarn.lock, capabilities.json and CODEOWNERS, none under .github/workflows/, and editing CODEOWNERS alone (unguarded) trips the guard's own single-owner fail-open. The set of CI-defining files is effectively the whole repository, so no path-scoped guard is sound. Decision: accept the bypass as a documented trust decision rather than a containment boundary. The genuinely valuable part of this PR — a self-test harness that could never fail — is split out to #5.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant