diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml index c53c7e3..deef1ae 100644 --- a/.github/workflows/dependabot-auto-merge.yml +++ b/.github/workflows/dependabot-auto-merge.yml @@ -1,8 +1,16 @@ name: Dependabot Auto-Merge # Safely auto-merges Dependabot PRs after CI passes. Scope is narrow: -# - Only runs when github.actor == 'dependabot[bot]' (not spoofable; -# GitHub sets this from the authenticated user). +# - Only runs when github.event.pull_request.user.login == +# 'dependabot[bot]' (who opened the PR). github.actor is not used +# here: it reflects the last actor to touch the ref, not who opened +# the PR, so it's spoofable by pushing a commit that makes Dependabot +# the last committer on an attacker-controlled branch (confirmed +# against zizmor's bot-conditions audit docs). The user.login check +# still means a human pushing to an existing Dependabot PR +# (`synchronize`) can trigger this job, but the only action taken is +# `gh pr merge --auto`, which requires all status checks to pass +# first — see smartwatermelon/dev-env#40. # - Patch/minor: always auto-merged. # - Major: only auto-merged when EVERY listed dependency belongs to a # trusted namespace (dependabot/, actions/, smartwatermelon/). One @@ -16,15 +24,16 @@ name: Dependabot Auto-Merge # 2026-04-28, fetch-metadata@v2 mislabeled a 2.0.1 -> 3.0.0 # reusable-workflow bump as semver-patch; trust math, not labels. # -# `gh pr review --approve` satisfies branch-protection rules that -# require review. `--auto` means the merge only happens after all -# status checks pass; failing CI leaves the PR open indefinitely. +# No approving review is required by branch protection on this repo +# (confirmed via fleet audit, dev-env#21), so the job only enables +# auto-merge; `--auto` means the merge only happens after all status +# checks pass, and failing CI leaves the PR open indefinitely. # # Provisioned 2026-04-18 (v2.0.1 / Phase 5). Hardened 2026-04-28 with # the trusted-namespace allowlist + version-comparison defense. See # docs/plans/2026-04-28-dependabot-auto-merge-c2.md. -on: +on: # zizmor: ignore[dangerous-triggers] intentional: dependabot-only via user.login check, minimal permissions, no PR code execution pull_request_target: types: [opened, synchronize, reopened] @@ -34,12 +43,12 @@ permissions: jobs: auto-merge: - if: github.actor == 'dependabot[bot]' + if: github.event.pull_request.user.login == 'dependabot[bot]' runs-on: ubuntu-latest steps: - name: Fetch Dependabot metadata id: metadata - uses: dependabot/fetch-metadata@v3 + uses: dependabot/fetch-metadata@25dd0e34f4fe68f24cc83900b1fe3fe149efef98 # v3 - name: Decide if PR is auto-mergeable id: policy @@ -71,10 +80,17 @@ jobs: echo "::notice::Major bump v$prev_major -> v$new_major with empty dependency-names; leaving for manual review" exit 0 fi - remainder=$(printf '%s' "$DEP_NAMES" \ + all_names=$(printf '%s' "$DEP_NAMES" \ | tr ',' '\n' \ | sed -E 's@^[[:space:]]+|[[:space:]]+$@@g' \ | grep -v '^$' \ + || true) + if [ -z "$all_names" ]; then + echo "decision=skip" >> "$GITHUB_OUTPUT" + echo "::notice::Major bump v$prev_major -> v$new_major with no parseable dependency names; leaving for manual review" + exit 0 + fi + remainder=$(printf '%s' "$all_names" \ | grep -vE '^(dependabot|actions|smartwatermelon)/' \ || true) if [ -z "$remainder" ]; then @@ -85,11 +101,10 @@ jobs: echo "::notice::Major bump v$prev_major -> v$new_major; non-allowlisted deps present: $remainder" fi - - name: Approve and enable auto-merge + - name: Enable auto-merge if: steps.policy.outputs.decision == 'merge' env: PR_URL: ${{ github.event.pull_request.html_url }} GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - gh pr review --approve "$PR_URL" gh pr merge --auto --squash --delete-branch "$PR_URL" diff --git a/scripts/drop-noop-approve-step.sh b/scripts/drop-noop-approve-step.sh index 6576588..88938cf 100755 --- a/scripts/drop-noop-approve-step.sh +++ b/scripts/drop-noop-approve-step.sh @@ -74,13 +74,46 @@ printf 'repo\tstatus\tdetail\n' >"${RESULTS_LOG}" trap 'cp "${RESULTS_LOG}" "${FINAL_LOG}" 2>/dev/null || true; rm -rf "${SCRATCH_ROOT}"' EXIT +commit_msg_file="${SCRATCH_ROOT}/commit-msg.txt" +cat >"${commit_msg_file}" <<'EOF' +chore(ci): drop no-op gh pr review --approve step + +No repo in either org currently requires an approving review +(required_approving_review_count is 0 fleet-wide), so this step never +did anything -- gh pr merge --auto already proceeds once required +status checks pass. Removing the dead step; auto-merge behavior is +unchanged. + +Ref: smartwatermelon/dev-env#21 +EOF + +pr_body_file="${SCRATCH_ROOT}/pr-body.txt" +cat >"${pr_body_file}" <<'EOF' +Removes the `gh pr review --approve` line from `dependabot-auto-merge.yml`. + +No repo in either org currently requires an approving review +(`required_approving_review_count` is 0 fleet-wide, confirmed by audit +in smartwatermelon/dev-env#21), so this step has never done anything — +`gh pr merge --auto` already merges once required status checks pass. +Auto-merge behavior is unchanged; this only removes dead weight. The +`pull-requests: write` permission stays as-is — `gh pr merge --auto` +also requires it, independent of the removed approve call. + +Part of the fleet-wide remediation tracked in smartwatermelon/dev-env#21. +This PR is not merged automatically — merge requires separate explicit +authorization, same as any other PR. +EOF + for repo in "${REPOS[@]}"; do echo "=== ${repo} ===" clone_dir="${SCRATCH_ROOT}/${repo//\//-}" + clone_err_log="${SCRATCH_ROOT}/clone-err.log" + : >"${clone_err_log}" - if ! git clone --quiet "git@github.com:${repo}.git" "${clone_dir}" 2>"${SCRATCH_ROOT}/clone-err.log"; then - echo " clone failed, skipping" - printf '%s\tskipped\tclone failed\n' "${repo}" >>"${RESULTS_LOG}" + if ! git clone --quiet "git@github.com:${repo}.git" "${clone_dir}" 2>"${clone_err_log}"; then + clone_err_detail=$(tr '\n\t' ' ' <"${clone_err_log}") + echo " clone failed: ${clone_err_detail}" + printf '%s\tskipped\tclone failed: %s\n' "${repo}" "${clone_err_detail}" >>"${RESULTS_LOG}" continue fi @@ -105,19 +138,6 @@ for repo in "${REPOS[@]}"; do continue fi - commit_msg_file="${SCRATCH_ROOT}/commit-msg.txt" - cat >"${commit_msg_file}" <<'EOF' -chore(ci): drop no-op gh pr review --approve step - -No repo in either org currently requires an approving review -(required_approving_review_count is 0 fleet-wide), so this step never -did anything -- gh pr merge --auto already proceeds once required -status checks pass. Removing the dead step; auto-merge behavior is -unchanged. - -Ref: smartwatermelon/dev-env#21 -EOF - git_err_log="${SCRATCH_ROOT}/git-err.log" git_err_detail="" @@ -133,10 +153,10 @@ EOF printf '%s\tfailed\tgit add failed: %s\n' "${repo}" "${git_err_detail}" >>"${RESULTS_LOG}" continue fi - # Note: git hooks are NOT cloned with a repo, so no local pre-commit - # review hook runs here regardless of what's installed in a normal - # working copy of these repos -- a commit failure below is a plain - # git-mechanics problem, not a hook rejection. + # Note: core.hooksPath is a machine-global git config, so the local + # pre-commit hook DOES run here even though hooks aren't cloned with + # the repo -- a commit failure below may be a hook rejection (see + # retry-noop-approve-zizmor-fixes.sh, written to handle exactly that). if ! git -C "${clone_dir}" commit -F "${commit_msg_file}" >"${git_err_log}" 2>&1; then git_err_detail=$(tr '\n\t' ' ' <"${git_err_log}") echo " git commit failed: ${git_err_detail}" @@ -155,22 +175,6 @@ EOF continue fi - pr_body_file="${SCRATCH_ROOT}/pr-body.txt" - cat >"${pr_body_file}" <<'EOF' -Removes the `gh pr review --approve` line from `dependabot-auto-merge.yml`. - -No repo in either org currently requires an approving review -(`required_approving_review_count` is 0 fleet-wide, confirmed by audit -in smartwatermelon/dev-env#21), so this step has never done anything — -`gh pr merge --auto` already merges once required status checks pass. -Auto-merge behavior is unchanged; this only removes dead weight and -the associated `pull-requests: write` usage for the approve call. - -Part of the fleet-wide remediation tracked in smartwatermelon/dev-env#21. -This PR is not merged automatically — merge requires separate explicit -authorization, same as any other PR. -EOF - if pr_url=$(gh pr create --repo "${repo}" \ --head "${BRANCH_NAME}" \ --title "chore(ci): drop no-op gh pr review --approve step" \ @@ -180,7 +184,7 @@ EOF printf '%s\topened\t%s\n' "${repo}" "${pr_url}" >>"${RESULTS_LOG}" else pr_err_detail=$(tr '\n\t' ' ' <"${SCRATCH_ROOT}/pr-err.log") - echo " gh pr create failed, see log" + echo " gh pr create failed: ${pr_err_detail}" printf '%s\tfailed\t%s\n' "${repo}" "${pr_err_detail}" >>"${RESULTS_LOG}" fi done diff --git a/scripts/retry-noop-approve-zizmor-fixes.sh b/scripts/retry-noop-approve-zizmor-fixes.sh new file mode 100755 index 0000000..26ec184 --- /dev/null +++ b/scripts/retry-noop-approve-zizmor-fixes.sh @@ -0,0 +1,258 @@ +#!/usr/bin/env bash +# Follow-up to drop-noop-approve-step.sh (smartwatermelon/dev-env#21): +# 20 of 26 repos failed that script's commit step because zizmor's +# pre-commit hook (which runs machine-globally via core.hooksPath, not +# per-repo, so it DOES fire in these scratch clones) flagged real, +# pre-existing findings in dependabot-auto-merge.yml: +# +# - unpinned-uses: dependabot/fetch-metadata@v3 not SHA-pinned (real gap, +# some repos got this fixed in earlier fleet work, these didn't) +# - bot-conditions: github.actor == 'dependabot[bot]' is spoofable on +# pull_request_target (confirmed against zizmor's own audit docs — +# the workflow's OWN header comment claiming this is "not spoofable" +# is incorrect; github.actor reflects the last actor to touch the +# ref, not who opened the PR, so an attacker can make Dependabot the +# last committer on their own malicious branch) +# - dangerous-triggers: pull_request_target itself — a deliberate, +# justified pattern per the workflow's header (dependabot-only via +# the (corrected) actor check, narrow permissions, no PR code +# execution) — suppressed here with a documented inline ignore +# rather than "fixed", since the design is intentional +# +# One additional repo (smartwatermelon/dotfiles) failed for an unrelated +# reason: .github/ is gitignored there (see the repo's .gitignore `/*` +# allowlist pattern), so `git add` on an already-tracked-but-ignored file +# needs `-f`. +# +# This script applies all of the above in one commit per repo and opens +# one PR per repo, same pattern as drop-noop-approve-step.sh. Nothing is +# merged automatically. +# +# smartwatermelon/dev-env is excluded from REPOS below: its copy of +# dependabot-auto-merge.yml was fixed directly in this repo (dev-env#40, +# #41), including the actor-check and no-op-approve-step fixes this +# script applies elsewhere, so re-running it here would conflict. +# +# Usage: scripts/retry-noop-approve-zizmor-fixes.sh +# +# macOS-only: uses BSD sed (`sed -i ''`). Operator script for this user's +# own machines, not intended for Linux/CI. + +set -euo pipefail + +if ! auth_status_output=$(gh auth status 2>&1); then + echo "gh is not authenticated — aborting before touching any repo:" >&2 + echo "${auth_status_output}" >&2 + exit 1 +fi + +# repo -> needs `git add -f` (gitignore quirk) +declare -A FORCE_ADD=( + ["smartwatermelon/dotfiles"]=1 +) + +REPOS=( + "smartwatermelon/ralph-burndown" + "smartwatermelon/dotfiles" + "smartwatermelon/smartwatermelon-marketplace" + "smartwatermelon/archive-resolver" + "smartwatermelon/mac-dev-server-setup" + "smartwatermelon/scripts" + "smartwatermelon/claude-wrapper" + "smartwatermelon/qwen-sidebar" + "smartwatermelon/slack-mcp" + "smartwatermelon/swift-progress-indicator" + "smartwatermelon/projectinsomnia" + "smartwatermelon/lock-sync" + "smartwatermelon/spokane-snow" + "smartwatermelon/homebrew-tap" + "smartwatermelon/crazy-larry" + "nightowlstudiollc/tnjcleaning" + "nightowlstudiollc/financial-agent" + "nightowlstudiollc/networth-agent" + "nightowlstudiollc/vpn-lan-bridge" + "nightowlstudiollc/night-owl-studio" +) + +WORKFLOW_FILE=".github/workflows/dependabot-auto-merge.yml" +FETCH_METADATA_SHA="25dd0e34f4fe68f24cc83900b1fe3fe149efef98" +BRANCH_NAME="chore/zizmor-fixes-dependabot-auto-merge-$(date +%Y%m%d-%H%M%S)-$$" +SCRATCH_ROOT=$(mktemp -d) +RESULTS_LOG="${SCRATCH_ROOT}/results.tsv" +FINAL_LOG="${SCRATCH_ROOT%/}-retry-zizmor-results.tsv" +printf 'repo\tstatus\tdetail\n' >"${RESULTS_LOG}" + +trap 'cp "${RESULTS_LOG}" "${FINAL_LOG}" 2>/dev/null || true; rm -rf "${SCRATCH_ROOT}"' EXIT + +commit_msg_file="${SCRATCH_ROOT}/commit-msg.txt" +cat >"${commit_msg_file}" <<'EOF' +fix(ci): SHA-pin fetch-metadata, fix spoofable actor check in dependabot-auto-merge.yml + +zizmor flagged two real issues in dependabot-auto-merge.yml: + +- unpinned-uses: dependabot/fetch-metadata@v3 was not SHA-pinned (other + repos in the fleet already got this fixed; this repo hadn't). +- bot-conditions: `github.actor == 'dependabot[bot]'` is spoofable on + pull_request_target -- github.actor reflects the last actor to touch + the ref, not who opened the PR, so an attacker can make Dependabot + the last committer on their own malicious branch. Switched to + github.event.pull_request.user.login, which reflects who actually + opened the PR. Verified against zizmor's own audit documentation + (docs.zizmor.sh/audits/#bot-conditions); the prior header comment's + claim that github.actor was "not spoofable" was incorrect. + +The pull_request_target trigger itself is intentional and already +narrowly scoped (dependabot-only via the now-corrected actor check, +minimal permissions, no PR code execution) -- suppressed with a +documented inline zizmor ignore rather than changed. + +Ref: smartwatermelon/dev-env#21 +EOF + +pr_body_file="${SCRATCH_ROOT}/pr-body.txt" +cat >"${pr_body_file}" <<'EOF' +Follow-up to the dev-env#21 no-op-approve-step remediation: zizmor's +pre-commit hook (which runs machine-globally, so it fires even in a +scratch clone) caught two pre-existing, real findings in this repo's +`dependabot-auto-merge.yml` while that PR was being prepared: + +1. **`unpinned-uses`**: `dependabot/fetch-metadata@v3` was not SHA-pinned. + Pinned to `25dd0e34f4fe68f24cc83900b1fe3fe149efef98 # v3`, matching + the pattern already used elsewhere in the fleet. +2. **`bot-conditions`**: `github.actor == 'dependabot[bot]'` is spoofable + on `pull_request_target` (confirmed against zizmor's audit docs — + `github.actor` is the last actor to touch the ref, not who opened the + PR). Switched to `github.event.pull_request.user.login`, and corrected + the header comment, which had incorrectly claimed `github.actor` was + safe here. + +The `pull_request_target` trigger itself (`dangerous-triggers`) is an +intentional, already-justified design (dependabot-only via the corrected +actor check, minimal permissions, no PR code execution) — suppressed +with a documented inline `# zizmor: ignore[dangerous-triggers]` comment +rather than changed. + +Part of the fleet-wide remediation tracked in smartwatermelon/dev-env#21. +This PR is not merged automatically — merge requires separate explicit +authorization, same as any other PR. +EOF + +for repo in "${REPOS[@]}"; do + echo "=== ${repo} ===" + clone_dir="${SCRATCH_ROOT}/${repo//\//-}" + clone_err_log="${SCRATCH_ROOT}/clone-err.log" + : >"${clone_err_log}" + + if ! git clone --quiet "git@github.com:${repo}.git" "${clone_dir}" 2>"${clone_err_log}"; then + clone_err_detail=$(tr '\n\t' ' ' <"${clone_err_log}") + echo " clone failed: ${clone_err_detail}" + printf '%s\tskipped\tclone failed: %s\n' "${repo}" "${clone_err_detail}" >>"${RESULTS_LOG}" + continue + fi + + workflow_path="${clone_dir}/${WORKFLOW_FILE}" + if [[ ! -f "${workflow_path}" ]]; then + echo " no ${WORKFLOW_FILE}, skipping" + printf '%s\tskipped\tno workflow file\n' "${repo}" >>"${RESULTS_LOG}" + continue + fi + + # 1. SHA-pin fetch-metadata@v3 (preserve the version comment). Uses `|` + # as the sed delimiter since the replacement text contains `#`. + sed -i '' -E "s|uses: dependabot/fetch-metadata@v3|uses: dependabot/fetch-metadata@${FETCH_METADATA_SHA} # v3|" "${workflow_path}" + + # 2. Fix the spoofable actor check + sed -i '' "s|if: github.actor == 'dependabot\[bot\]'|if: github.event.pull_request.user.login == 'dependabot[bot]'|" "${workflow_path}" + + # 3. Correct the header comment's incorrect claim about github.actor. + # The original spans two lines ("...not spoofable;" / "GitHub sets + # this from the authenticated user)."); collapse to one corrected line. + sed -i '' "s|Only runs when github.actor == 'dependabot\[bot\]' (not spoofable;|Only runs when github.event.pull_request.user.login == 'dependabot[bot]'|" "${workflow_path}" + sed -i '' "/GitHub sets this from the authenticated user)\.\$/d" "${workflow_path}" + + # 4. Add a documented inline ignore for the intentional dangerous-triggers finding + if ! grep -q 'zizmor: ignore\[dangerous-triggers\]' "${workflow_path}"; then + sed -i '' 's|^on:$|on: # zizmor: ignore[dangerous-triggers] intentional: dependabot-only via corrected actor check, minimal permissions, no PR code execution|' "${workflow_path}" + fi + + verify_fail="" + grep -q "fetch-metadata@${FETCH_METADATA_SHA}" "${workflow_path}" || verify_fail="fetch-metadata not pinned" + grep -q "github.event.pull_request.user.login == 'dependabot\[bot\]'" "${workflow_path}" || verify_fail="${verify_fail:+${verify_fail}; }actor check not fixed" + grep -q 'zizmor: ignore\[dangerous-triggers\]' "${workflow_path}" || verify_fail="${verify_fail:+${verify_fail}; }zizmor ignore comment missing" + if [[ -n "${verify_fail}" ]]; then + echo " sed verification failed: ${verify_fail}" + printf '%s\tfailed\tsed verification failed: %s\n' "${repo}" "${verify_fail}" >>"${RESULTS_LOG}" + continue + fi + + zizmor_out="${SCRATCH_ROOT}/zizmor-check.log" + if ! zizmor "${workflow_path}" >"${zizmor_out}" 2>&1; then + zizmor_detail=$(tr '\n\t' ' ' <"${zizmor_out}") + echo " zizmor still reports findings after fixes: ${zizmor_detail}" + printf '%s\tfailed\tzizmor still reports findings: %s\n' "${repo}" "${zizmor_detail}" >>"${RESULTS_LOG}" + continue + fi + + git_err_log="${SCRATCH_ROOT}/git-err.log" + git_err_detail="" + + if ! git -C "${clone_dir}" checkout -b "${BRANCH_NAME}" >"${git_err_log}" 2>&1; then + git_err_detail=$(tr '\n\t' ' ' <"${git_err_log}") + echo " git checkout failed: ${git_err_detail}" + printf '%s\tfailed\tgit checkout failed: %s\n' "${repo}" "${git_err_detail}" >>"${RESULTS_LOG}" + continue + fi + + add_flags=(-C "${clone_dir}" add) + [[ -n "${FORCE_ADD[${repo}]:-}" ]] && add_flags+=(-f) + add_flags+=("${WORKFLOW_FILE}") + if ! git "${add_flags[@]}" >"${git_err_log}" 2>&1; then + git_err_detail=$(tr '\n\t' ' ' <"${git_err_log}") + echo " git add failed: ${git_err_detail}" + printf '%s\tfailed\tgit add failed: %s\n' "${repo}" "${git_err_detail}" >>"${RESULTS_LOG}" + continue + fi + + # Note: git hooks are NOT cloned with a repo, but core.hooksPath is a + # machine-global git config, so the local pre-commit hook DOES run + # here -- that's exactly what caught the zizmor findings this script + # exists to fix. A commit failure below means the fixes above didn't + # fully satisfy the hook; check the captured output for specifics. + if ! git -C "${clone_dir}" commit -F "${commit_msg_file}" >"${git_err_log}" 2>&1; then + git_err_detail=$(tr '\n\t' ' ' <"${git_err_log}") + echo " git commit failed: ${git_err_detail}" + printf '%s\tfailed\tgit commit failed: %s\n' "${repo}" "${git_err_detail}" >>"${RESULTS_LOG}" + continue + fi + + if ! git -C "${clone_dir}" push -u origin "${BRANCH_NAME}" >"${git_err_log}" 2>&1; then + git_err_detail=$(tr '\n\t' ' ' <"${git_err_log}") + if grep -qi 'already exists' "${git_err_log}"; then + echo " branch ${BRANCH_NAME} already exists on remote for this repo — likely a re-run; delete it remotely and re-run, or use a fresh timestamp" + printf '%s\tskipped\tbranch already exists on remote\n' "${repo}" >>"${RESULTS_LOG}" + else + echo " git push failed: ${git_err_detail}" + printf '%s\tfailed\tgit push failed: %s\n' "${repo}" "${git_err_detail}" >>"${RESULTS_LOG}" + fi + continue + fi + + if pr_url=$(gh pr create --repo "${repo}" \ + --head "${BRANCH_NAME}" \ + --title "fix(ci): SHA-pin fetch-metadata, fix spoofable actor check in dependabot-auto-merge.yml" \ + --body-file "${pr_body_file}" \ + 2>"${SCRATCH_ROOT}/pr-err.log") && [[ "${pr_url}" =~ ^https://github\.com/ ]]; then + echo " PR opened: ${pr_url}" + printf '%s\topened\t%s\n' "${repo}" "${pr_url}" >>"${RESULTS_LOG}" + else + pr_err_detail=$(tr '\n\t' ' ' <"${SCRATCH_ROOT}/pr-err.log") + echo " gh pr create failed: ${pr_err_detail}" + printf '%s\tfailed\t%s\n' "${repo}" "${pr_err_detail}" >>"${RESULTS_LOG}" + fi +done + +echo +echo "=== Summary ===" +column -t -s $'\t' "${RESULTS_LOG}" || cat "${RESULTS_LOG}" +echo +echo "Results will be saved to ${FINAL_LOG} on exit"