Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
benjiwagner wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/enforce-policy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ on:
pull_request_target:
branches: [main]
types: [opened, edited, reopened, synchronize, ready_for_review]
# The workflow guard reads review state, so a late approval has to re-evaluate the check instead
# of leaving a stale failure. Like pull_request_target, this event runs the base-branch workflow
# and never checks out pull-request code.
pull_request_review:
types: [submitted, dismissed]

permissions: {}

Expand Down
192 changes: 184 additions & 8 deletions repository-policy/validate.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,78 @@ set -euo pipefail
readonly TYPES='build|chore|ci|docs|feat|fix|perf|refactor|revert|style|test'
readonly BRANCH_TYPES="$TYPES|sandbox"
readonly HEADER_MAX_LENGTH=100
readonly WORKFLOW_PATH_PREFIX='.github/workflows/'

errors=()

add_error() {
errors+=("$1")
}

# GitHub logins are case-insensitive, and this script must stay portable to the bash 3.2 that
# ships on macOS, so lowercase through tr rather than ${var,,}.
to_lower() {
printf '%s' "$1" | tr '[:upper:]' '[:lower:]'
}

list_contains() {
local needle="$1" haystack="$2" item
[[ -n "$needle" ]] || return 1
needle="$(to_lower "$needle")"
while IFS= read -r item; do
[[ -n "$item" ]] || continue
[[ "$(to_lower "$item")" == "$needle" ]] && return 0
done <<< "$haystack"
return 1
}

# Extracts individual @user owners. Team handles (@org/team) are ignored: this organization defines
# no teams, and a team handle cannot be resolved to a reviewer set with `pull-requests: read` alone.
parse_code_owners() {
awk '{
sub(/#.*/, "")
for (i = 2; i <= NF; i++) {
if (substr($i, 1, 1) == "@" && index($i, "/") == 0) print substr($i, 2)
}
}' | sort -fu
}

# The API returns reviews in chronological order, so the last non-comment state per reviewer is
# their current one. An approval that was later dismissed or replaced by changes-requested must not
# count, which is why this cannot simply grep for APPROVED.
latest_approvers() {
awk -F'\t' '
$2 != "COMMENTED" { latest[tolower($1)] = $2 }
END { for (login in latest) if (latest[login] == "APPROVED") print login }
' | sort -fu
}

# Pure decision logic, kept free of network access so --self-test can exercise every branch.
validate_workflow_review() {
local changed_files="$1" owners="$2" approvers="$3" author="$4"
local owner eligible_owner_exists=0

grep -q "^${WORKFLOW_PATH_PREFIX}" <<< "$changed_files" || return 0

while IFS= read -r owner; do
[[ -n "$owner" ]] || continue
if [[ -n "$author" ]] && [[ "$(to_lower "$owner")" == "$(to_lower "$author")" ]]; then
continue
fi
eligible_owner_exists=1
if list_contains "$owner" "$approvers"; then
return 0
fi
done <<< "$owners"

# No code owner other than the author exists, so no eligible reviewer exists either. The sole
# owner already holds the whole trust boundary and requiring the impossible would only deadlock
# workflow maintenance. This is a deliberate fail-open on single-owner repositories.
(( eligible_owner_exists == 0 )) && return 0

add_error "Pull requests that modify ${WORKFLOW_PATH_PREFIX}** require an approving review from a code owner other than the author."
}

validate_branch() {
local branch="$1"

Expand Down Expand Up @@ -139,33 +204,55 @@ validate_body() {
fi
}

# Assertions must report and record rather than rely on `set -e`. A bare `[[ ... ]]` inside a
# function does not abort the script here, so every assertion in this suite was previously
# unenforced: the suite passed even when a validator was broken outright.
self_test_failures=0

expect_error_count() {
local label="$1" expected="$2" actual="${#errors[@]}"
if (( actual != expected )); then
printf 'SELF-TEST FAILED: %s (expected %d error(s), got %d)\n' "$label" "$expected" "$actual" >&2
self_test_failures=$(( self_test_failures + 1 ))
fi
}

expect_equal() {
local label="$1" expected="$2" actual="$3"
if [[ "$actual" != "$expected" ]]; then
printf 'SELF-TEST FAILED: %s\n expected: %s\n actual: %s\n' "$label" "$expected" "$actual" >&2
self_test_failures=$(( self_test_failures + 1 ))
fi
}

self_test() {
local temporary_directory
temporary_directory="$(mktemp -d)"
trap 'rm -rf "$temporary_directory"' RETURN
self_test_failures=0

errors=()
validate_branch "feat/contact-import"
validate_branch "sandbox/rewe"
validate_branch "dependabot/npm_and_yarn/zod-4.0.0"
[[ ${#errors[@]} -eq 0 ]]
expect_error_count "conventional and dependabot branches are accepted" 0

errors=()
validate_branch "feature/contact-import"
validate_branch "feat/Contact_import"
[[ ${#errors[@]} -eq 2 ]]
expect_error_count "unknown type and non-kebab-case branches are rejected" 2

errors=()
validate_header "Header" "feat(contacts): add contact import"
validate_header "Header" "fix!: prevent duplicate messages"
validate_header "Header" "chore(release): 1.4.0"
[[ ${#errors[@]} -eq 0 ]]
expect_error_count "conventional headers are accepted" 0

errors=()
validate_header "Header" "Feature: Add contact import"
validate_header "Header" "feat: add contact import."
validate_header "Header" "feat(Bad Scope): add contact import"
[[ ${#errors[@]} -eq 3 ]]
expect_error_count "malformed headers are rejected" 3

printf '%s\n' \
'## Summary' 'Adds contact import.' '' \
Expand All @@ -174,7 +261,7 @@ self_test() {
'## Impact and rollback' 'Revert the pull request.' > "$temporary_directory/valid-body"
errors=()
validate_body "$temporary_directory/valid-body"
[[ ${#errors[@]} -eq 0 ]]
expect_error_count "a complete, ordered body is accepted" 0

printf '%s\n' \
'## Context' 'Out of order.' '' \
Expand All @@ -183,14 +270,73 @@ self_test() {
'## Validation' 'Duplicate.' > "$temporary_directory/invalid-body"
errors=()
validate_body "$temporary_directory/invalid-body"
[[ ${#errors[@]} -ge 4 ]]
if (( ${#errors[@]} < 4 )); then
printf 'SELF-TEST FAILED: %s (expected at least 4 errors, got %d)\n' \
"an out-of-order, empty, duplicated, incomplete body is rejected" "${#errors[@]}" >&2
self_test_failures=$(( self_test_failures + 1 ))
fi

local two_owners docs_only workflow_change
two_owners=$'benjiwagner\neljulsi'
docs_only=$'README.md\nplatform/github.md'
workflow_change=$'.github/workflows/knowledge-base.yml\nREADME.md'

errors=()
validate_workflow_review "$docs_only" "$two_owners" "" "eljulsi"
expect_error_count "a pull request touching no workflow file is unaffected" 0

errors=()
validate_workflow_review "$workflow_change" "$two_owners" "" "eljulsi"
expect_error_count "a workflow change with no approval is rejected" 1

errors=()
validate_workflow_review "$workflow_change" "$two_owners" "eljulsi" "eljulsi"
expect_error_count "self-approval does not satisfy the workflow guard" 1

errors=()
validate_workflow_review "$workflow_change" "$two_owners" "some-contributor" "eljulsi"
expect_error_count "approval from a non-code-owner does not satisfy the workflow guard" 1

errors=()
validate_workflow_review "$workflow_change" "$two_owners" "BenjiWagner" "eljulsi"
expect_error_count "approval from another code owner satisfies it, case-insensitively" 0

errors=()
validate_workflow_review "$workflow_change" "benjiwagner" "" "benjiwagner"
expect_error_count "a sole-code-owner repository has no eligible reviewer and skips" 0

errors=()
validate_workflow_review "docs/.github/workflows/example.yml" "$two_owners" "" "eljulsi"
expect_error_count "the workflow prefix is anchored to the start of the path" 0

expect_equal "CODEOWNERS parsing keeps users and drops comments, patterns, and team handles" \
$'benjiwagner\neljulsi' \
"$(printf '%s\n' '# owners' '* @benjiwagner @eljulsi' '/docs/ @customermates/writers' | parse_code_owners)"

expect_equal "only each reviewer's latest non-comment review state counts" \
$'charlie\ndelta' \
"$(printf '%s\t%s\n' \
'alpha' 'APPROVED' \
'alpha' 'DISMISSED' \
'bravo' 'APPROVED' \
'bravo' 'CHANGES_REQUESTED' \
'charlie' 'CHANGES_REQUESTED' \
'charlie' 'APPROVED' \
'delta' 'APPROVED' \
'delta' 'COMMENTED' | latest_approvers)"

if (( self_test_failures > 0 )); then
printf '%s\n' "Repository policy self-test failed: ${self_test_failures} assertion(s)." >&2
return 1
fi

printf '%s\n' "Repository policy self-test passed."
}

if [[ "${1:-}" == "--self-test" ]]; then
self_test
exit
# Explicit, because a nonzero return from self_test does not abort under `set -e` here.
self_test || exit 1
exit 0
fi

: "${GH_TOKEN:?GH_TOKEN is required}"
Expand All @@ -201,6 +347,7 @@ readonly pr_number="$(jq -r '.pull_request.number // empty' "$GITHUB_EVENT_PATH"
readonly pr_author="$(jq -r '.pull_request.user.login // ""' "$GITHUB_EVENT_PATH")"
readonly head_sha="$(jq -r '.pull_request.head.sha // empty' "$GITHUB_EVENT_PATH")"
readonly head_branch="$(jq -r '.pull_request.head.ref // empty' "$GITHUB_EVENT_PATH")"
readonly base_sha="$(jq -r '.pull_request.base.sha // empty' "$GITHUB_EVENT_PATH")"
readonly title="$(jq -r '.pull_request.title // ""' "$GITHUB_EVENT_PATH")"

: "${pr_number:?Pull request number is missing from the event}"
Expand Down Expand Up @@ -229,6 +376,35 @@ while IFS=$'\t' read -r commit_sha commit_header; do
validate_header "Commit ${commit_sha:0:12}" "$commit_header"
done < "$commits_file"

# Workflow-tamper guard. The `ci` check 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. This
# check runs from the base branch inside the bypass-free integrity layer, which is what makes it
# able to constrain that. CODEOWNERS and the reviews are both read from the base ref for the same
# reason: reading either from the head would let a pull request nominate its own approvers.
changed_files="$(gh api --paginate \
--header "X-GitHub-Api-Version: 2022-11-28" \
"repos/${GITHUB_REPOSITORY}/pulls/${pr_number}/files?per_page=100" \
--jq '.[].filename')"

if grep -q "^${WORKFLOW_PATH_PREFIX}" <<< "$changed_files"; then
code_owners=""
if [[ -n "$base_sha" ]]; then
code_owners="$(gh api \
--header "X-GitHub-Api-Version: 2022-11-28" \
--header "Accept: application/vnd.github.raw" \
"repos/${GITHUB_REPOSITORY}/contents/.github/CODEOWNERS?ref=${base_sha}" 2>/dev/null \
| parse_code_owners || true)"
fi

approving_reviewers="$(gh api --paginate \
--header "X-GitHub-Api-Version: 2022-11-28" \
"repos/${GITHUB_REPOSITORY}/pulls/${pr_number}/reviews?per_page=100" \
--jq '.[] | [.user.login, .state] | @tsv' \
| latest_approvers)"

validate_workflow_review "$changed_files" "$code_owners" "$approving_reviewers" "$pr_author"
fi

if (( ${#errors[@]} > 0 )); then
printf '%s\n' "Repository policy failed:" >&2
printf ' - %s\n' "${errors[@]}" >&2
Expand Down