diff --git a/.github/workflows/dependabot-autofix.yml b/.github/workflows/dependabot-autofix.yml index 8adec3de..396bfb8c 100644 --- a/.github/workflows/dependabot-autofix.yml +++ b/.github/workflows/dependabot-autofix.yml @@ -29,7 +29,22 @@ name: dependabot-autofix # is still pushed, but the checks must be re-triggered by hand. # See doc/handwritten/for-maintainers/workflows/dependabot-autofix.en.md. -on: +# A workflow-security audit (zizmor `dangerous-triggers`) reports this trigger as a +# privilege-escalation vector, and the pattern generally is one: `workflow_run` runs in the +# base context with the repository secrets and a writable token, off the completion of a run +# that may have executed untrusted code. It is kept, because the escalation it describes is +# already closed here and the trigger is not replaceable: +# - Not replaceable — a `pull_request` run raised by Dependabot gets a READ-ONLY token and +# NO secrets, so it could neither read ANTHROPIC_API_KEY nor push the repair. There is no +# other trigger that grants both. +# - The untrusted code is never run — the checkout below is the BASE, never the pull +# request, and the repair does only git operations. The bumped dependency is never built, +# so the write token and the API key never meet freshly bumped third-party code. +# - The identity is settled against the API, not the event payload, and the branch TIP must +# be Dependabot's own GitHub-signed commit (see the resolve step). That is a stricter +# check than `dependabot/fetch-metadata` performs, which validates the FIRST commit. +# Recorded rather than silenced, per ADR-0060: the refusal belongs next to what it refuses. +on: # zizmor: ignore[dangerous-triggers] workflow_run: # Every gating workflow a Dependabot PR passes through. Any completion re-runs # the triage against the head commit's current, combined check status. diff --git a/.github/workflows/dependabot-automerge.yml b/.github/workflows/dependabot-automerge.yml index 78d315e2..aa647f0a 100644 --- a/.github/workflows/dependabot-automerge.yml +++ b/.github/workflows/dependabot-automerge.yml @@ -10,6 +10,32 @@ name: dependabot-automerge # Without a branch-protection rule on `main` that marks the CI checks as required, # auto-merge would merge immediately. The required checks are therefore the safety # gate, not this workflow. +# +# Why the job keys on the pull request's AUTHOR and not on `github.actor`. Auto-merge, +# once enabled, SURVIVES later pushes to the head branch. `github.actor` names whoever +# triggered the run, so a push by someone else made the job skip — leaving the +# auto-merge armed on a tip that nobody re-checked. The author of a pull request never +# changes, so keying on it means this job runs on EVERY event of a Dependabot pull +# request, including a push by someone else, which is precisely when it needs to act +# (Sonar githubactions:S8232, zizmor `bot-conditions`). +# +# Identity is then settled twice, and the two answers do different work: +# - `dependabot/fetch-metadata` re-checks the pull request's author and the FIRST +# commit's signature (verified by reading the action's source; both its +# skip-*-verification inputs default to false). It fails closed — no outputs, so +# the steps below are skipped. +# - The TIP check here is the one the action does not do. `fetch-metadata` validates +# the first commit; auto-merge acts on the tip. Somebody with push access can append +# to a Dependabot branch without changing either the author or the first commit. +# +# The two guards are deliberately asymmetric: +# - ENABLING requires the strong proof — the tip is Dependabot's own GitHub-signed +# commit. Commit author NAMES forge freely; GitHub's signature does not. +# - DISABLING triggers on the weaker signal — the tip's author is not Dependabot — +# because disabling is the fail-safe direction: at worst a human merges by hand. +# Keying the disable on the SIGNATURE instead would fight `dependabot-autofix`, +# which deliberately keeps auto-merge on after a trivial fix; its `--amend` and +# `rebase` preserve Dependabot as the author but drop the signature. on: pull_request: @@ -26,8 +52,8 @@ jobs: runs-on: ubuntu-latest # Fetch metadata + enable auto-merge is seconds; cap a stuck runner. timeout-minutes: 10 - # Only act on PRs raised by Dependabot. - if: github.actor == 'dependabot[bot]' + # Only act on pull requests Dependabot OPENED — a fact no later push can change. + if: github.event.pull_request.user.login == 'dependabot[bot]' permissions: # Enable auto-merge (contents) on the pull request (pull-requests). contents: write @@ -37,11 +63,56 @@ jobs: id: meta uses: dependabot/fetch-metadata@25dd0e34f4fe68f24cc83900b1fe3fe149efef98 # v3 + - name: Inspect the head commit this event is about + id: tip + # Reads the tip the checks actually ran against, not whatever the branch points at + # by the time this job runs. A newer push raises its own event and re-enters here. + # Fail closed: anything unreadable answers "not Dependabot's", which only ever + # withholds or withdraws auto-merge. + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -euo pipefail + tip="$(gh api "repos/${GH_REPO}/commits/${HEAD_SHA}" \ + --jq '"\(.author.login // "")|\(.commit.verification.verified)"' 2>/dev/null || true)" + author="${tip%%|*}" + echo "head ${HEAD_SHA}: author='${author:-unknown}' signed='${tip##*|}'" + # Signed by Dependabot — the strong proof, required to ARM auto-merge. + if [ "$tip" = "dependabot[bot]|true" ]; then + echo "state=dependabot-signed" >> "$GITHUB_OUTPUT" + # Authored by Dependabot but unsigned: what dependabot-autofix leaves behind + # after rewording or rebasing. Not proof enough to arm, not foreign enough to + # disarm — leave whatever is already set alone. + elif [ "$author" = "dependabot[bot]" ]; then + echo "state=dependabot-unsigned" >> "$GITHUB_OUTPUT" + else + echo "state=foreign" >> "$GITHUB_OUTPUT" + fi + - name: Enable auto-merge for patch and minor updates # Major updates fall through this condition, so they never get auto-merge # and stay open for a human to review. - if: steps.meta.outputs.update-type == 'version-update:semver-patch' || steps.meta.outputs.update-type == 'version-update:semver-minor' + if: >- + steps.tip.outputs.state == 'dependabot-signed' && + (steps.meta.outputs.update-type == 'version-update:semver-patch' || + steps.meta.outputs.update-type == 'version-update:semver-minor') env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_URL: ${{ github.event.pull_request.html_url }} run: gh pr merge --auto --merge "$PR_URL" + + - name: Withdraw auto-merge from a foreign head + # The step that closes the gap: auto-merge armed on Dependabot's own commit stays + # armed across later pushes, so a commit appended by someone with push access would + # ride an approval it never got. Withdrawing is idempotent — `|| true` because the + # call errors when auto-merge was not enabled in the first place, which is the + # ordinary case here. + if: steps.tip.outputs.state == 'foreign' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_URL: ${{ github.event.pull_request.html_url }} + run: | + echo "Head is not Dependabot's commit; withdrawing auto-merge so a human decides." + gh pr merge --disable-auto "$PR_URL" || true diff --git a/doc/handwritten/for-maintainers/workflows/dependabot-autofix.en.md b/doc/handwritten/for-maintainers/workflows/dependabot-autofix.en.md index 8ed07dbb..c0c0611f 100644 --- a/doc/handwritten/for-maintainers/workflows/dependabot-autofix.en.md +++ b/doc/handwritten/for-maintainers/workflows/dependabot-autofix.en.md @@ -125,6 +125,17 @@ context. user names), and the branch tip's GitHub signature says who wrote the code the job is about to read, patch and force-push. Commit author and committer *names* are `git config` values and forge freely; the signature does not. +- **A workflow-security audit reports the trigger itself, and the answer is in the + file.** `zizmor`'s `dangerous-triggers` flags `workflow_run` as a + privilege-escalation vector, which the pattern generally is. It is kept because the + trigger is not replaceable — a `pull_request` run raised by Dependabot has a + read-only token and no secrets, so it could neither read the API key nor push — and + because the escalation is already closed by the three properties above: base-only + checkout, no build of the bumped dependency, and API-settled identity down to the + signed tip. That is stricter than `dependabot/fetch-metadata`, which validates the + *first* commit rather than the tip. The refusal is recorded as an inline + `# zizmor: ignore[dangerous-triggers]` carrying its reason (ADR-0060), so it is + answered where it fires rather than re-litigated on each run. ## Related diff --git a/doc/handwritten/for-maintainers/workflows/dependabot-autofix.fr.md b/doc/handwritten/for-maintainers/workflows/dependabot-autofix.fr.md index 8afd0b66..1d479ed3 100644 --- a/doc/handwritten/for-maintainers/workflows/dependabot-autofix.fr.md +++ b/doc/handwritten/for-maintainers/workflows/dependabot-autofix.fr.md @@ -137,6 +137,18 @@ dans son propre contexte Dependabot en lecture seule. écrit le code que le job s'apprête à lire, corriger et force-pusher. Les *noms* d'auteur et de committer d'un commit sont des valeurs `git config` et se forgent librement ; la signature, non. +- **Un audit de sécurité des workflows signale le déclencheur lui-même, et la réponse + est dans le fichier.** La règle `dangerous-triggers` de `zizmor` signale + `workflow_run` comme vecteur d'escalade de privilèges, ce que le motif est en + général. Il est conservé parce qu'il n'est pas remplaçable — un run `pull_request` + levé par Dependabot n'a qu'un token en lecture seule et aucun secret, il ne pourrait + ni lire la clé d'API ni pousser — et parce que l'escalade est déjà fermée par les + trois propriétés ci-dessus : checkout de la base uniquement, aucune compilation de la + dépendance bumpée, identité établie via l'API jusqu'à la pointe signée. C'est plus + strict que `dependabot/fetch-metadata`, qui valide le *premier* commit et non la + pointe. Le refus est consigné par un `# zizmor: ignore[dangerous-triggers]` en ligne + portant sa raison (ADR-0060), pour être répondu là où il se déclenche plutôt que + rejoué à chaque exécution. ## Liens connexes diff --git a/doc/handwritten/for-maintainers/workflows/dependabot-automerge.en.md b/doc/handwritten/for-maintainers/workflows/dependabot-automerge.en.md index a422c524..5e58f2ad 100644 --- a/doc/handwritten/for-maintainers/workflows/dependabot-automerge.en.md +++ b/doc/handwritten/for-maintainers/workflows/dependabot-automerge.en.md @@ -22,15 +22,22 @@ here. ## When it runs - On every **pull request targeting `main`**, but the job is gated on - `github.actor == 'dependabot[bot]'`, so it acts only on Dependabot's PRs. + `github.event.pull_request.user.login == 'dependabot[bot]'` — the pull + request's **author** — so it acts only on Dependabot's PRs. That is + deliberately not `github.actor`; see *Handle with care*. ## How it runs One job, `automerge`: 1. `dependabot/fetch-metadata` reads the update type (patch / minor / major). -2. For **patch or minor** updates, `gh pr merge --auto` enables auto-merge. Major - updates fall through the condition and stay open. +2. The **head commit of this event** is inspected and classified: Dependabot's + own GitHub-signed commit, Dependabot-authored but unsigned, or foreign. +3. For a **signed** head and a **patch or minor** update, `gh pr merge --auto` + enables auto-merge. Major updates fall through the condition and stay open. +4. For a **foreign** head, auto-merge is **withdrawn** (`--disable-auto`). An + unsigned Dependabot-authored head — what `dependabot-autofix` leaves behind + after a reword or a rebase — is left exactly as it is. ## Permissions & security @@ -48,9 +55,27 @@ Workflow default `contents: read`; the job widens to `contents: write` and - **The `major` exclusion is intentional.** Only `semver-patch` and `semver-minor` get auto-merge; majors are left for a human because they are the ones most likely to break. Do not broaden the condition to majors. -- **The actor guard matters.** `if: github.actor == 'dependabot[bot]'` keeps the - elevated `contents: write` / `pull-requests: write` path from running on - human PRs. +- **The guard is the pull request's AUTHOR, and it must not go back to + `github.actor`.** Both keep the elevated `contents: write` / + `pull-requests: write` path off human PRs, but `github.actor` names whoever + triggered the run, so a push by someone else made the job *skip*. Auto-merge + survives later pushes to the head branch, so skipping left it armed on a tip + nobody re-checked. The author of a pull request never changes, so the job now + runs on every event of a Dependabot PR — which is exactly when it needs to act. +- **The two head guards are asymmetric on purpose.** *Arming* requires + Dependabot's own GitHub-signed commit: commit author names are `git config` + values and forge freely, GitHub's signature does not. *Withdrawing* triggers on + the weaker signal — an author that is not Dependabot — because withdrawing is + the fail-safe direction; at worst a human merges by hand. Do not "tidy" this + into one symmetric check: keying the withdrawal on the signature would fight + [`dependabot-autofix`](dependabot-autofix.en.md), whose `--amend` and `rebase` + keep Dependabot as the author but drop the signature, and which deliberately + keeps auto-merge on after a trivial fix. +- **`dependabot/fetch-metadata` is a second gate, but not this one.** It + re-checks the PR author, the **first** commit's author and that commit's + signature, never consults `github.actor`, and fails closed by emitting no + outputs (both its `skip-*-verification` inputs default to `false`). What it + does not check is the **tip** — and the tip is what auto-merge merges. ## Related diff --git a/doc/handwritten/for-maintainers/workflows/dependabot-automerge.fr.md b/doc/handwritten/for-maintainers/workflows/dependabot-automerge.fr.md index f54d8a73..603da8cb 100644 --- a/doc/handwritten/for-maintainers/workflows/dependabot-automerge.fr.md +++ b/doc/handwritten/for-maintainers/workflows/dependabot-automerge.fr.md @@ -23,16 +23,24 @@ ici. ## Quand il s'exécute - À chaque **pull request visant `main`**, mais le job est conditionné à - `github.actor == 'dependabot[bot]'`, donc il n'agit que sur les PR de - Dependabot. + `github.event.pull_request.user.login == 'dependabot[bot]'` — l'**auteur** de + la pull request — donc il n'agit que sur les PR de Dependabot. Ce n'est + délibérément pas `github.actor` ; voir *À manipuler avec précaution*. ## Comment il s'exécute Un seul job, `automerge` : 1. `dependabot/fetch-metadata` lit le type de mise à jour (patch / minor / major). -2. Pour les mises à jour **patch ou minor**, `gh pr merge --auto` active - l'auto-merge. Les majeures ne passent pas la condition et restent ouvertes. +2. Le **commit de tête de cet événement** est inspecté et classé : commit signé + par GitHub appartenant à Dependabot, commit de Dependabot non signé, ou + étranger. +3. Pour une tête **signée** et une mise à jour **patch ou minor**, + `gh pr merge --auto` active l'auto-merge. Les majeures ne passent pas la + condition et restent ouvertes. +4. Pour une tête **étrangère**, l'auto-merge est **retiré** (`--disable-auto`). + Une tête de Dependabot non signée — ce que laisse `dependabot-autofix` après + une réécriture ou un rebase — est laissée telle quelle. ## Permissions & sécurité @@ -52,9 +60,31 @@ la PR. `semver-minor` obtiennent l'auto-merge ; les majeures sont laissées à un humain parce que ce sont elles qui risquent le plus de casser. N'élargissez pas la condition aux majeures. -- **Le garde-fou sur l'acteur compte.** `if: github.actor == 'dependabot[bot]'` - empêche le chemin élevé `contents: write` / `pull-requests: write` de tourner - sur des PR humaines. +- **Le garde-fou porte sur l'AUTEUR de la pull request, et ne doit pas revenir à + `github.actor`.** Les deux empêchent le chemin élevé `contents: write` / + `pull-requests: write` de tourner sur des PR humaines, mais `github.actor` + nomme celui qui a déclenché le run : un push par quelqu'un d'autre faisait donc + *sauter* le job. Or l'auto-merge survit aux pushes suivants sur la branche de + tête ; sauter le laissait donc armé sur une tête que personne ne revérifiait. + L'auteur d'une pull request ne change jamais, si bien que le job tourne + désormais à chaque événement d'une PR Dependabot — c'est-à-dire précisément + quand il doit agir. +- **Les deux gardes sur la tête sont asymétriques à dessein.** *Armer* exige le + commit signé par GitHub appartenant à Dependabot : les noms d'auteur de commit + sont des valeurs `git config` et se forgent librement, la signature de GitHub + non. *Retirer* se déclenche sur le signal faible — un auteur qui n'est pas + Dependabot — parce que retirer est le sens sûr ; au pire un humain merge à la + main. Ne « rangez » pas cela en une vérification symétrique : conditionner le + retrait à la signature entrerait en conflit avec + [`dependabot-autofix`](dependabot-autofix.fr.md), dont les `--amend` et + `rebase` conservent Dependabot comme auteur mais perdent la signature, et qui + garde délibérément l'auto-merge après un correctif trivial. +- **`dependabot/fetch-metadata` est une seconde barrière, mais pas celle-ci.** + Elle revérifie l'auteur de la PR, l'auteur du **premier** commit et la + signature de ce commit, ne consulte jamais `github.actor`, et échoue fermé en + n'émettant aucune sortie (ses deux entrées `skip-*-verification` valent `false` + par défaut). Ce qu'elle ne vérifie pas, c'est la **tête** — et c'est la tête que + l'auto-merge merge. ## En rapport