From d374b91803c336e55d1a63b96aad676348eeb55c Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Wed, 22 Jul 2026 03:19:59 +0000 Subject: [PATCH 1/2] =?UTF-8?q?feat:=20implement=20issue=20#864=20?= =?UTF-8?q?=E2=80=94=20[#850]=20standards-deploy=20driver=20can't=20write?= =?UTF-8?q?=20to=20consumer=20repos=20=E2=80=94=20wrong=20token=20(ORG=5FS?= =?UTF-8?q?CORECARD=5FTOKEN)=20+=20opaque=20put-failed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/workflows/standards-deploy.yml | 14 +++- scripts/lib/standards-deploy.sh | 65 ++++++++++++++---- scripts/standards-deploy-driver.sh | 5 +- standards/ci-standards.md | 22 ++++++ test/scripts/lib/standards-deploy.bats | 92 ++++++++++++++++++++++++-- 5 files changed, 179 insertions(+), 19 deletions(-) diff --git a/.github/workflows/standards-deploy.yml b/.github/workflows/standards-deploy.yml index c29dae63a..bb4484300 100644 --- a/.github/workflows/standards-deploy.yml +++ b/.github/workflows/standards-deploy.yml @@ -47,7 +47,19 @@ jobs: permissions: contents: read # only to check out this repo; all cross-repo writes use the PAT below env: - GH_TOKEN: ${{ secrets.ORG_SCORECARD_TOKEN }} + # Cross-repo write identity for the sweep. NOT ORG_SCORECARD_TOKEN — that is + # the audit identity (a fine-grained PAT with Contents: Read-only), so it + # cannot PUT stub content into consumer repos and every deploy 422'd + # (petry-projects/.github#864). GH_PAT_DON_PETRY (falling back to the legacy + # GH_PAT_WORKFLOWS) is the SAME identity dev-lead.yml / pr-auto-review.yml / + # initiative-driver.yml already use to author cross-repo PRs. Its login is + # `don-petry` (or the GH_PAT_WORKFLOWS owner) — deliberately NOT donpetry-bot + # — so the standards-sync PRs it opens can be approved by donpetry-bot via the + # pr-review agent and land through native auto-merge with no `--admin`. + # Least-privilege: needs only contents:write + pull_requests:write on the + # target repos (a GitHub App with those two scopes, non-author-blocked and not + # donpetry-bot, is an acceptable substitute). + GH_TOKEN: ${{ secrets.GH_PAT_DON_PETRY || secrets.GH_PAT_WORKFLOWS }} TARGET_REPO: ${{ inputs.target_repo }} TARGET_WORKFLOW: ${{ inputs.target_workflow }} DRY_RUN: ${{ inputs.dry_run == true && 'true' || 'false' }} diff --git a/scripts/lib/standards-deploy.sh b/scripts/lib/standards-deploy.sh index a85c01539..707cf613e 100644 --- a/scripts/lib/standards-deploy.sh +++ b/scripts/lib/standards-deploy.sh @@ -7,9 +7,10 @@ # default branch directly and NEVER merges or uses --admin. Instead it: # # 1. checks for an already-open PR on the sync branch (idempotent skip), -# 2. creates (or reuses) a sync branch off the repo's default branch, -# 3. PUTs the verbatim file content onto that branch via the Contents API, -# 4. opens a labeled pull request. +# 2. preflights that the token can write the repo (clear finding on a scope gap), +# 3. creates (or reuses) a sync branch off the repo's default branch, +# 4. PUTs the verbatim file content onto that branch via the Contents API, +# 5. opens a labeled pull request. # # Why a PR and not a direct push: a direct Contents-API push to the default # branch is rejected (HTTP 409) on repos whose ruleset enforces required status @@ -81,7 +82,19 @@ sd_deploy_files_via_pr() { return 0 fi - # 2. Resolve the default branch and its tip SHA (branch point). + # 2. Preflight write check. The driver must run under a contents/PR-write + # identity; an audit/read-only token silently produces per-file 422s deep in + # step 4 (petry-projects/.github#864). Probe the repo permission set once, up + # front, so a token-scope gap surfaces as one clear finding, not opaque + # per-file put-failures. + local can_push + can_push=$(gh api "repos/${repo}" --jq '.permissions.push // false' 2>/dev/null || echo "false") + if [ "$can_push" != "true" ]; then + echo "FAILED no-write-access" + return 1 + fi + + # 3. Resolve the default branch and its tip SHA (branch point). local default_branch base_sha default_branch=$(gh api "repos/${repo}" --jq '.default_branch' 2>/dev/null || echo "main") base_sha=$(gh api "repos/${repo}/git/ref/heads/${default_branch}" \ @@ -91,7 +104,7 @@ sd_deploy_files_via_pr() { return 1 fi - # 3. Create the sync branch; reuse it if a prior run already created it. + # 4. Create the sync branch; reuse it if a prior run already created it. if ! gh api "repos/${repo}/git/refs" --method POST \ --raw-field "ref=refs/heads/${branch}" \ --raw-field "sha=${base_sha}" --silent 2>/dev/null; then @@ -101,14 +114,39 @@ sd_deploy_files_via_pr() { fi fi - # 4. PUT each file verbatim onto the branch. The blob SHA is looked up per file + # 5. PUT each file verbatim onto the branch. The blob SHA is looked up per file # on the branch so both create (absent → empty SHA) and update (drifted stub) - # work, whether the branch is fresh or reused. - local path local_file branch_sha encoded j + # work, whether the branch is fresh or reused. Errors are NOT swallowed: a + # read failure that yields an empty SHA would make the subsequent PUT omit + # `sha` and 422 against an existing file — the exact opaque failure in #864. + local path local_file branch_sha encoded j err_file get_rc get_msg put_msg for (( i = 1; i < $#; i += 2 )); do j=$(( i + 1 )); path="${!i}"; local_file="${!j}" + + # Resolve the file's blob SHA on the branch, capturing stderr and exit code. + err_file=$(mktemp) branch_sha=$(gh api "repos/${repo}/contents/${path}?ref=${branch}" \ - --jq '.sha // ""' 2>/dev/null || true) + --jq '.sha // ""' 2>"$err_file") && get_rc=0 || get_rc=$? + if [ "$get_rc" -eq 0 ]; then + # GET succeeded → the file EXISTS on the branch, so its SHA must resolve. + # A blank SHA here means we cannot update it safely; a sha-less PUT would + # 422. Fail loudly rather than mask it. + if [ -z "$branch_sha" ]; then + rm -f "$err_file" + echo "FAILED sha-unresolved:${path}" + return 1 + fi + else + get_msg=$(tr '\n' ' ' < "$err_file"); rm -f "$err_file" + if printf '%s' "$get_msg" | grep -q 'HTTP 404'; then + branch_sha="" # file absent on the branch → create path, no SHA + else + echo "FAILED contents-get-failed:${get_msg}" + return 1 + fi + fi + rm -f "$err_file" + encoded=$(base64 -w 0 "$local_file" 2>/dev/null || base64 -b 0 "$local_file") local put_args=(--method PUT @@ -117,13 +155,16 @@ sd_deploy_files_via_pr() { --raw-field "branch=${branch}") [ -n "$branch_sha" ] && put_args+=(--raw-field "sha=${branch_sha}") - if ! gh api "repos/${repo}/contents/${path}" "${put_args[@]}" --silent 2>/dev/null; then - echo "FAILED put-failed" + err_file=$(mktemp) + if ! gh api "repos/${repo}/contents/${path}" "${put_args[@]}" --silent 2>"$err_file"; then + put_msg=$(tr '\n' ' ' < "$err_file"); rm -f "$err_file" + echo "FAILED put-failed:${put_msg}" return 1 fi + rm -f "$err_file" done - # 5. Ensure the label exists, then open the PR. `gh pr create --label` fails + # 6. Ensure the label exists, then open the PR. `gh pr create --label` fails # outright if the label is absent, and consumer repos do not carry the # standards-sync label by default. Create-if-missing (no --force, so an # existing curated label is never clobbered); ignore the "already exists" diff --git a/scripts/standards-deploy-driver.sh b/scripts/standards-deploy-driver.sh index 144734b56..6b9719944 100755 --- a/scripts/standards-deploy-driver.sh +++ b/scripts/standards-deploy-driver.sh @@ -22,7 +22,10 @@ # Overridable for tests: # DEPLOY_SCRIPT Path to the deploy script (default: sibling in this dir). # -# Requires GH_TOKEN with repo scope for cross-repo branch + PR creation. +# Requires GH_TOKEN to be a cross-repo WRITE identity — contents:write + +# pull_requests:write on the target repos (e.g. GH_PAT_DON_PETRY / the legacy +# GH_PAT_WORKFLOWS). The audit token (ORG_SCORECARD_TOKEN) is read-only for +# Contents and cannot deploy stubs — see petry-projects/.github#864. set -euo pipefail diff --git a/standards/ci-standards.md b/standards/ci-standards.md index 7bdd88443..b232f46af 100644 --- a/standards/ci-standards.md +++ b/standards/ci-standards.md @@ -357,6 +357,28 @@ open for that workflow. Deployment never edits a caller's `uses:`/`agent_ref` pins by hand — a release is rolled out by moving the channel tag (see [Reusable workflow versioning](#reusable-workflow-versioning--the-stable-channel)). +**Deploy identity (least-privilege).** The scheduled driver +([`standards-deploy.yml`](workflows/standards-deploy.yml)) writes stub content and +opens PRs **across the fleet**, so it must run under a cross-repo write identity — +`contents:write` + `pull_requests:write` on the target repos, and nothing more. It +uses `GH_PAT_DON_PETRY` (falling back to the legacy `GH_PAT_WORKFLOWS`), the same +identity `dev-lead.yml` / `pr-auto-review.yml` / `initiative-driver.yml` already +author cross-repo PRs with. It **must not** use `ORG_SCORECARD_TOKEN`: that is the +**audit** identity (a fine-grained PAT with **Contents: Read-only**), so its +content PUTs 422 and every deploy fails with an opaque `put-failed` +([#864](https://github.com/petry-projects/.github/issues/864)). The deploy identity +is also deliberately **not `donpetry-bot`** (it acts as `don-petry`): the PRs it +opens are authored by a distinct code-owner, so `donpetry-bot` can approve them via +the pr-review agent and they land through native auto-merge with **no `--admin`**. A +GitHub App carrying only those two scopes — non-author-blocked and not +`donpetry-bot` — is an acceptable substitute. + +The deploy primitive also **preflights write access per repo** before touching any +branch and **never issues a sha-less content PUT against an existing file**: a +token-scope gap surfaces as a single `no-write-access` finding, and a genuine HTTP +error is reported with its status (e.g. `put-failed:… (HTTP 422)`) instead of being +swallowed. + #### Self-managing meta-repos: `SKIP_REPOS` + `SKIP_OVERRIDES` `deploy-standard-workflows.sh` exempts a small set of **self-managing meta-repos** diff --git a/test/scripts/lib/standards-deploy.bats b/test/scripts/lib/standards-deploy.bats index 4bbdcd16c..8f982f9c7 100644 --- a/test/scripts/lib/standards-deploy.bats +++ b/test/scripts/lib/standards-deploy.bats @@ -39,9 +39,36 @@ case "$sub" in else exit "${GH_REF_EXISTS_RC:-0}" fi ;; - *contents*"?"ref=*) printf '%s' "${GH_FILE_SHA:-}" ;; - *contents*) exit "${GH_PUT_RC:-0}" ;; - repos/*) printf '%s' "${GH_DEFAULT_BRANCH:-main}" ;; + *contents*"?"ref=*) + # contents GET used to resolve a file's blob sha on the branch. + # GH_GET_RC!=0 -> the GET errors (prints GH_GET_ERR to stderr) + # GH_FILE_SHA unset -> file absent on the branch (404 create path) + # GH_FILE_SHA set (maybe + # empty) -> file present; echo the (maybe empty) sha + if [ "${GH_GET_RC:-0}" -ne 0 ]; then + printf '%s\n' "${GH_GET_ERR:-gh: Something went wrong (HTTP 500)}" >&2 + exit "${GH_GET_RC}" + fi + if [ -z "${GH_FILE_SHA+x}" ]; then + printf '%s\n' 'gh: Not Found (HTTP 404)' >&2 + exit 1 + fi + printf '%s' "${GH_FILE_SHA}" ;; + *contents*) + # contents PUT. On failure surface an HTTP-status-bearing stderr message. + if [ "${GH_PUT_RC:-0}" -ne 0 ]; then + printf '%s\n' "${GH_PUT_ERR:-gh: sha was not supplied (HTTP 422)}" >&2 + exit "${GH_PUT_RC}" + fi + exit 0 ;; + repos/*) + # repos/: the default-branch lookup AND the preflight write probe + # (--jq .permissions.push) both land here — differentiate on the jq arg. + if printf '%s' "$args" | grep -q 'permissions.push'; then + printf '%s' "${GH_CAN_PUSH:-true}" + else + printf '%s' "${GH_DEFAULT_BRANCH:-main}" + fi ;; esac ;; esac @@ -190,11 +217,13 @@ deploy() { [ "$output" = "FAILED no-branch" ] } -@test "fails when the PUT is rejected" { +@test "fails when the PUT is rejected and surfaces the real HTTP status" { GH_PUT_RC="1"; export GH_PUT_RC + GH_PUT_ERR='gh: "sha" wasn'\''t supplied. (HTTP 422)'; export GH_PUT_ERR run deploy [ "$status" -ne 0 ] - [ "$output" = "FAILED put-failed" ] + [[ "$output" == FAILED\ put-failed:* ]] + [[ "$output" == *"HTTP 422"* ]] } @test "fails when the PR cannot be created" { @@ -210,3 +239,56 @@ deploy() { [ "$status" -ne 0 ] [ "$output" = "FAILED missing-local-file" ] } + +# --------------------------------------------------------------------------- +# Fix 2 (petry-projects/.github#864): preflight write check + real error +# surfacing + never a sha-less PUT on an existing file. +# --------------------------------------------------------------------------- + +# Preflight: a token without contents/PR write is reported as a clear finding, +# not an opaque per-file put-failed. No branch or PUT is attempted. +@test "fails loudly when the token lacks write access (preflight probe)" { + GH_CAN_PUSH="false"; export GH_CAN_PUSH + run deploy + [ "$status" -ne 0 ] + [ "$output" = "FAILED no-write-access" ] + ! grep -q 'git/refs --method POST' "$GH_CALLS" + ! grep -q 'contents/.* --method PUT' "$GH_CALLS" +} + +# Regression guard for the reported bug: when the stub file already exists on the +# branch, the update PUT must ALWAYS carry its blob sha — a sha-less PUT against +# an existing file is a guaranteed 422. +@test "existing-file update always supplies a sha in the PUT" { + GH_FILE_SHA="deadbeefsha"; export GH_FILE_SHA + run deploy + [ "$status" -eq 0 ] + local put_lines + put_lines=$(grep 'contents/.* --method PUT' "$GH_CALLS") + [ -n "$put_lines" ] + while IFS= read -r line; do + printf '%s' "$line" | grep -q -- '--raw-field sha=deadbeefsha' + done <<< "$put_lines" +} + +# The contents GET must not swallow errors: a non-404 failure (e.g. a token-scope +# 403) is surfaced with its HTTP status, and no PUT is attempted. +@test "fails loudly when the contents GET errors with a non-404 status" { + GH_GET_RC="1"; export GH_GET_RC + GH_GET_ERR='gh: Resource not accessible by integration (HTTP 403)'; export GH_GET_ERR + run deploy + [ "$status" -ne 0 ] + [[ "$output" == FAILED\ contents-get-failed:* ]] + [[ "$output" == *"HTTP 403"* ]] + ! grep -q 'contents/.* --method PUT' "$GH_CALLS" +} + +# The file exists on the branch (GET 200) but its sha cannot be resolved: fail +# loudly rather than issue a sha-less PUT that is guaranteed to 422. +@test "fails loudly when an existing file's sha cannot be resolved" { + GH_FILE_SHA=""; export GH_FILE_SHA # present (GET 200) but empty sha + run deploy + [ "$status" -ne 0 ] + [[ "$output" == FAILED\ sha-unresolved:* ]] + ! grep -q 'contents/.* --method PUT' "$GH_CALLS" +} From d47e1053bfc3b6b4e78ef75211099b33dab7f4d8 Mon Sep 17 00:00:00 2001 From: Claude Code Bot Date: Tue, 21 Jul 2026 22:34:57 -0500 Subject: [PATCH 2/2] fix: address review on standards-deploy token/hardening (#864) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up (codeant/qodo): - preflight: surface the probe's OWN error distinctly (FAILED perm-probe-failed: ) instead of collapsing every gh-api failure into no-write-access — a transient/auth/404 must not be misreported (the exact opacity #864 fixes); add a bats case for it - fix stale "step 4" comment (PUT is step 5 after the preflight became step 2) - driver.bats sha-guard loop: fail on ANY line missing the sha (return 1), not just the last iteration - ci-standards.md: correct the standards-deploy.yml link (.github/workflows/…) Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01CoxYr15rT5ceo7UKZ6r86Y --- scripts/lib/standards-deploy.sh | 17 +++++++++++++---- standards/ci-standards.md | 2 +- test/scripts/lib/standards-deploy.bats | 22 +++++++++++++++++++++- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/scripts/lib/standards-deploy.sh b/scripts/lib/standards-deploy.sh index 707cf613e..af5d0838e 100644 --- a/scripts/lib/standards-deploy.sh +++ b/scripts/lib/standards-deploy.sh @@ -84,11 +84,20 @@ sd_deploy_files_via_pr() { # 2. Preflight write check. The driver must run under a contents/PR-write # identity; an audit/read-only token silently produces per-file 422s deep in - # step 4 (petry-projects/.github#864). Probe the repo permission set once, up + # step 5 (petry-projects/.github#864). Probe the repo permission set once, up # front, so a token-scope gap surfaces as one clear finding, not opaque - # per-file put-failures. - local can_push - can_push=$(gh api "repos/${repo}" --jq '.permissions.push // false' 2>/dev/null || echo "false") + # per-file put-failures. The probe's OWN errors (a transient/auth/404) are + # surfaced distinctly — they must NOT be misreported as "no write access". + # Repo-level push permission implies PR-create for a collaborator identity. + local can_push perm_err perm_rc + perm_err=$(mktemp) + can_push=$(gh api "repos/${repo}" --jq '.permissions.push // false' 2>"$perm_err") && perm_rc=0 || perm_rc=$? + if [ "$perm_rc" -ne 0 ]; then + local perm_msg; perm_msg=$(tr '\n' ' ' < "$perm_err"); rm -f "$perm_err" + echo "FAILED perm-probe-failed:${perm_msg}" + return 1 + fi + rm -f "$perm_err" if [ "$can_push" != "true" ]; then echo "FAILED no-write-access" return 1 diff --git a/standards/ci-standards.md b/standards/ci-standards.md index b232f46af..a58ae81e6 100644 --- a/standards/ci-standards.md +++ b/standards/ci-standards.md @@ -358,7 +358,7 @@ pins by hand — a release is rolled out by moving the channel tag (see [Reusable workflow versioning](#reusable-workflow-versioning--the-stable-channel)). **Deploy identity (least-privilege).** The scheduled driver -([`standards-deploy.yml`](workflows/standards-deploy.yml)) writes stub content and +([`standards-deploy.yml`](../.github/workflows/standards-deploy.yml)) writes stub content and opens PRs **across the fleet**, so it must run under a cross-repo write identity — `contents:write` + `pull_requests:write` on the target repos, and nothing more. It uses `GH_PAT_DON_PETRY` (falling back to the legacy `GH_PAT_WORKFLOWS`), the same diff --git a/test/scripts/lib/standards-deploy.bats b/test/scripts/lib/standards-deploy.bats index 8f982f9c7..b86ab8456 100644 --- a/test/scripts/lib/standards-deploy.bats +++ b/test/scripts/lib/standards-deploy.bats @@ -65,6 +65,10 @@ case "$sub" in # repos/: the default-branch lookup AND the preflight write probe # (--jq .permissions.push) both land here — differentiate on the jq arg. if printf '%s' "$args" | grep -q 'permissions.push'; then + if [ -n "${GH_PERM_RC:-}" ] && [ "${GH_PERM_RC}" != "0" ]; then + printf '%s\n' "${GH_PERM_ERR:-gh: Not Found (HTTP 404)}" >&2 + exit "${GH_PERM_RC}" + fi printf '%s' "${GH_CAN_PUSH:-true}" else printf '%s' "${GH_DEFAULT_BRANCH:-main}" @@ -256,6 +260,21 @@ deploy() { ! grep -q 'contents/.* --method PUT' "$GH_CALLS" } +# Preflight: an ERROR on the probe itself (transient / auth / 404) is surfaced +# distinctly with its HTTP status — NOT misreported as "no-write-access". No +# branch or PUT is attempted. +@test "preflight probe error is surfaced distinctly, not as no-write-access" { + GH_PERM_RC="1"; export GH_PERM_RC + GH_PERM_ERR='gh: Resource not accessible by integration (HTTP 403)'; export GH_PERM_ERR + run deploy + [ "$status" -ne 0 ] + [[ "$output" == FAILED\ perm-probe-failed:* ]] + [[ "$output" == *"HTTP 403"* ]] + [[ "$output" != *"no-write-access"* ]] + ! grep -q 'git/refs --method POST' "$GH_CALLS" + ! grep -q 'contents/.* --method PUT' "$GH_CALLS" +} + # Regression guard for the reported bug: when the stub file already exists on the # branch, the update PUT must ALWAYS carry its blob sha — a sha-less PUT against # an existing file is a guaranteed 422. @@ -267,7 +286,8 @@ deploy() { put_lines=$(grep 'contents/.* --method PUT' "$GH_CALLS") [ -n "$put_lines" ] while IFS= read -r line; do - printf '%s' "$line" | grep -q -- '--raw-field sha=deadbeefsha' + printf '%s' "$line" | grep -q -- '--raw-field sha=deadbeefsha' \ + || { echo "PUT without sha: $line"; return 1; } done <<< "$put_lines" }