From aa12c06350b6872e18eea534f22c6b9850c4f5b3 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 4 Aug 2026 20:25:35 -0500 Subject: [PATCH 1/2] =?UTF-8?q?fix(ci):=20dependabot=20auto-merge=20decide?= =?UTF-8?q?d=20by=20exclusion=20=E2=80=94=20invert=20#3=20to=20an=20allow-?= =?UTF-8?q?set=20(BACKLOG=20#336)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guardrail #3 was a 16-name Python deny-list with no ecosystem qualifier: anything not named auto-merged if it was a patch, on every ecosystem. Inverted to HOLD-UNLESS-NAMED. Only `actions/`, `github/` and `dependabot/` are eligible, and only on github-actions; the uv, pip and npm rows ship EMPTY, an unrecognised ecosystem token holds, and the fail-safe whole-group denial is preserved. Measured against the live PR #75, not recalled: its body carries five `Updates` entries — four on the allow row (actions/checkout and three github/codeql-action/*) and pypa/gh-action-pypi-publish, which is not — so that batch would HOLD as a whole. Expect auto-merge to fire RARELY; that is the intent of hold-unless-named, not a regression. Hardening, not an incident. There is no evidence of exploitation, the attacker must already own an upstream publisher account, and merging to main is not publication (PyPI release is gated on an owner tag push). MessageFoundry is a not-deployed beta with zero production instances, so this is stated as what the shipped code WOULD allow on first deployment, never as a present exposure. §4 RELEASE AGE, with an honest account of its reach. A new `id: age` step holds a SECURITY-track PR whose candidate version was published under MIN_RELEASE_AGE_HOURS (24), failing closed on an API error, an absent or unparseable upload timestamp, an unexpected name or version shape, or an ecosystem with no publish-date source wired. It is INERT with respect to the merge decision as shipped: age_ok=true is reachable only for uv/pip, eligible=true only for github-actions, and the merge `if` requires both — disjoint sets. Confirmed by executing the shipped allow-set body over exactly the ecosystems the age gate can pass (uv and pip both return eligible=false; only github_actions returns true). The header and the BACKLOG banner now say exactly that instead of presenting it as an operating control; recording an unreachable control as operative is the false-premise class docs/Secure_Development_Standards.md §3 forbids. The step is gated on `steps.allowset.outputs.eligible == 'true'`, which changes no merge outcome — the merge `if` already requires that conjunct — but removes an unauthenticated outbound GET made from a job holding `contents: write` for a PR that was going to be held anyway. §5 the header's backstop claim, corrected without over-claiming in the other direction. Dropped: "the only gate that inspects a dependency's shipped bytes at all is security.yml's trivy step". security.yml:265 marks trivy `continue-on-error: true` and :271 restricts it to schedule/dispatch, so it is advisory AND never runs on a Dependabot PR. The replacement is the weaker true form: no REQUIRED check reads a dependency's shipped bytes. The sentence describes what semgrep cannot SEE rather than where it looks, so the unmerged plan-semgrep-scope widening (BACKLOG #334) cannot falsify it. Provenance made honest. The header cited DEPENDENCY-POSTURE-REVIEW.md as the numbered source for guardrails it does not carry: #3 is INVERTED from the deny-list that document describes, and #4 is introduced here. The block is retitled AUTO-MERGE GUARDRAILS, #3 is moved out from under the security-track framing (it gates every PR), and the test docstring carries the same correction. Amending the vault copy stays the owner's separate obligation — the repo simply stops asserting it has already happened. Restated facts removed. The header repeated three cooldown values that live in .github/dependabot.yml; only two had a test bound, and that file records github-actions' window as approximate because it ages off the tag's commit date. The header now points at the source instead (CLAUDE.md §11 — state a load-bearing fact once and link to it). .github/dependabot.yml comments corrected. The uv block told a maintainer "routine patches auto-merge AFTER aging; security patches auto-merge now" — both halves false once uv's allow row ships empty. The github-actions block's "an advisory fix is unaffected" now says it opens as a PR unaffected but reaches main by human review, because the advisory gate is pip-keyed and never confirms an action. docs/testing/master-test-plan/01-environments-data-and-tooling.md:350 still advertised the deleted auth/token/crypto deny-list as a live control of this workflow. No test pinned that doc, so it drifted silently; the row now describes what ships. TESTS - The behavioural harness runs the shipped bodies under `bash -e`, which is what Actions applies by default on Linux (no `shell:` is declared anywhere). Plain bash keeps going where CI aborts the step, and `assert proc.returncode == 0` was exactly the assertion that would mask that class. No shipped row changes its decision: ten rows were compared head-to-head under plain bash and under `bash -e` and agreed on both exit code and emitted output, and the full 20-row parametrised set passes under `-e`. So this is a fidelity fix, not a behaviour change — but `_run_step_body` now RETURNS the returncode rather than asserting it, so an abort path can be expressed as an expected outcome instead of being indistinguishable from a harness bug. test_no_step_overrides_the_default_shell is the tripwire for a future `shell:` silently invalidating the premise that `bash -e` is what CI runs. - _load_dependabot's skip is deleted. It skipped on the premise that .github/dependabot.yml is "private-only, deny-listed on the OSS mirror". .gitignore names that file under DELIBERATELY NOT LISTED as content meant to ship; `git ls-files --error-unmatch` resolves it and `git check-ignore` exits 1. The refactor had routed THREE tests through that skip, including the cooldown test whose entire reason for existing is that a missing cooldown was invisible to CI. It now asserts presence. - The jq skip reason claimed the test "runs on the ubuntu CI leg and skips locally". The windows-2022 and windows-2025 images ship jq and Git Bash, and both are REQUIRED contexts in .github/required-contexts.txt, so it runs there too. The reason now says so — a maintainer reading it after a Windows-only red should not be told to expect ubuntu. ADR 0034 CONSEQUENCE, recorded because leaving it unrecorded is what that ADR warns against: this file grew 190 -> 403 lines and the `Why pull_request` anchor moved from line 42 to 76. Under the ADR's convergence rule, dismissed alert #87 re-fires as a new alert number and needs re-dismissing after merge. VERIFICATION - ruff check . -> All checks passed! - ruff format --check . -> 1042 files already formatted - mypy messagefoundry -> 21 errors, ALL PRE-EXISTING and NOT from this change: absent [fhir]/[dicom]/[webauthn] extras (fhirpathpy, fhir, pynetdicom, pydicom and webauthn are all ABSENT from this venv; 12 import-not-found plus their 7 no-any-return / 2 unused-ignore consequences). This change touches no file under messagefoundry/, so mypy's inputs are byte-identical to HEAD. CI installs the extras and is the authority. The quartet is NOT green locally and this commit does not claim it is. - pytest -q -> 10291 passed, 841 skipped, 22 warnings in 1282.01s (0:21:22). The pre-change baseline measured on this tree was 10289 passed / 841 skipped, so the delta is exactly the two tests added here. Run on a byte-frozen tree (sha256 of all five changed files recorded before the run and unchanged after), because two comment edits had landed after an earlier run started and six tests read docs/BACKLOG.md. - Falsification: 18 mutations of the workflow and dependabot.yml, plus deleting dependabot.yml outright, each confirmed to turn the matching test RED and then restored byte-identically (sha256 checked after every case). Among them: the allow-set admitting `pypa/`, losing its trailing slash, matching by substring, treating an empty name list as eligible, and failing open on an unrecognised ecosystem; the age step failing open on an undatable ecosystem, hard-coding a PASS on the version-track exit, re-deriving the security track, and losing its allow-set gate; a step declaring `shell:`; the merge `if` dropping either the age_ok or the allow-set conjunct; and each cooldown being removed or shortened. The four jq-gated rows were falsified separately (threshold deleted, missing-timestamp failing open, curl error failing open, and the discriminating PASS removed) against a purpose-built jq stand-in implementing the two shipped filters, because this box has no jq at all; execution against real jq happens on CI. --- .github/dependabot.yml | 13 +- .github/workflows/dependabot-auto-merge.yml | 315 ++++++++++-- .../01-environments-data-and-tooling.md | 2 +- tests/test_dependabot_automerge_guardrails.py | 483 ++++++++++++++++-- 4 files changed, 718 insertions(+), 95 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index fb6eeb3d..b22989ab 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -25,12 +25,15 @@ updates: open-pull-requests-limit: 10 # Supply-chain cooldown: age a fresh release before opening a ROUTINE update PR, to dodge a # package compromised shortly after publish. Security updates ignore cooldown (Dependabot - # behavior), so a real advisory fix still arrives immediately. Pairs with the auto-merge - # workflow: routine patches auto-merge AFTER aging; security patches auto-merge now. + # behavior), so a real advisory fix still opens as a PR immediately. Aging is a PRECONDITION for + # an unattended merge, never a sufficient one — dependabot-auto-merge.yml's allow-set decides + # eligibility, and THIS ecosystem's allow row ships EMPTY, so no uv bump auto-merges on either + # track; every one goes to human review. That workflow owns the decision, so read it there. cooldown: # ~5-day aging window (DEPENDENCY-POSTURE-REVIEW.md) lengthens the malicious-fresh-publish # dodge on the routine VERSION track; the SECURITY track still bypasses cooldown (Dependabot - # design), now backstopped by the published-GHSA gate in dependabot-auto-merge.yml (SEC-007 #2). + # design) and is aged workflow-side instead, behind the published-GHSA gate (SEC-007 #2) and + # the release-age gate (#4) in dependabot-auto-merge.yml. default-days: 5 semver-major-days: 7 # Dependabot WIDENS a declared cap instead of respecting it, so a load-bearing upper bound in @@ -78,7 +81,9 @@ updates: # stops tag mutation, not a malicious new release — Dependabot just bumps the pin to the new SHA. # Only default-days is honored here (this ecosystem does not support the semver-*-days knobs) and # it ages off the tag's commit date rather than the release date, so treat 5 as approximate. - # Security updates bypass cooldown, so an advisory fix is unaffected. Watch codeql-action — a + # Security updates bypass cooldown, so an advisory fix still opens as a PR unaffected — but it + # reaches main by human review rather than unattended, because dependabot-auto-merge.yml's + # advisory gate is pip-keyed and therefore never confirms an action. Watch codeql-action — a # frequent releaser — for stalls after this lands. cooldown: default-days: 5 diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml index ccbc85dd..74089abd 100644 --- a/.github/workflows/dependabot-auto-merge.yml +++ b/.github/workflows/dependabot-auto-merge.yml @@ -8,36 +8,70 @@ name: Dependabot auto-merge # removes the human-latency on the safe, common case, not the safety net. # # IN SCOPE (auto-merged): -# - any PATCH update (incl. security patches — most security fixes are patches) -# - MINOR updates of DEV-only dependencies +# - a PATCH update (or a MINOR of a DEV-only dependency) whose EVERY named dependency sits on its +# own ecosystem's ALLOW row (guardrail #3 below). # OUT OF SCOPE (left for human review, surfaced same-day by the daily security cron + alerts): -# - MINOR/MAJOR updates of runtime deps, and ALL MAJOR updates -# NOTE the uv ecosystem is review-by-default in practice, not by exception: `python-deps` is -# `patterns: ["*"]`, so one weekly batch carries everything, and (a) a grouped update-type is the -# MAX across the batch (fetch-metadata's UPDATE_TYPES_PRIORITY) — one minor demotes every patch -# riding with it — while (b) the deny-list below names at least eight of this project's directly -# declared runtime deps and denies the WHOLE group on any hit. #66 was denied on `fastapi`. -# Auto-merge does fire for github-actions (#75, all-patch batch) and npm (#64). +# - EVERYTHING ELSE, and that is the default by design. An unnamed dependency, an unrecognised +# ecosystem, a MINOR/MAJOR update of a runtime dep, any MAJOR update — every one of them HOLDS. +# - EVERY uv and npm bump, because both allow rows ship EMPTY. `python-deps`/`ide-deps` group on +# `*`, so one weekly batch carries everything and a non-empty row would have to name each of the +# ~98 pinned distributions before it could ever fire. Those two ecosystems were already +# review-by-default in practice; they are now review-by-default BY DESIGN rather than by accident. +# - a github-actions batch carrying any third-party action, since `actions-deps` also groups on `*`. +# Measured: PR #75's five-bump batch carried `pypa/gh-action-pypi-publish`, so that same batch +# would HOLD under this allow-set. Expect auto-merge to fire RARELY — that is the intent of +# "hold unless named", not a regression. # -# Fresh-release supply-chain poisoning is handled upstream by the dependabot.yml `cooldown` -# (routine updates age before a PR opens); SECURITY updates bypass cooldown by design, so a real -# advisory fix still arrives immediately and (if a patch) auto-merges. +# Fresh-release supply-chain poisoning is handled upstream by the dependabot.yml `cooldown`, which +# every configured ecosystem carries: a routine update ages before its PR opens. The per-ecosystem +# windows — and the caveat that `github-actions` ages off the tag's COMMIT date rather than the +# release date — live in `.github/dependabot.yml` beside each value's rationale. Read them there; a +# copy here would rot silently. SECURITY updates bypass cooldown by Dependabot design, so that track +# is aged HERE instead, workflow-side, by guardrail #4. # -# SECURITY-TRACK GUARDRAILS (DEPENDENCY-POSTURE-REVIEW.md #2 and #3 — close SEC-007, CWE-829): -# #3 DENY-LIST — the auth/token/crypto stack (cryptography, argon2-cffi, fastapi, …) is NEVER -# auto-merged, not even for a patch: any such dep routes the PR to MANUAL review. A grouped -# PR that includes ANY denied dep is denied WHOLE (fail-safe — never partial-merge a group). -# #2 PUBLISHED-GHSA GATE — a SECURITY-track PR (which bypasses the cooldown by Dependabot design) -# auto-merges ONLY when a real, published, non-withdrawn advisory is confirmed against the -# dependency's PREVIOUS version (the axios-1.14.1 pattern). The advisory lookup FAILS CLOSED: -# a rate-limit/API error or no-matching-advisory routes to manual review, never auto-merge. -# The non-security VERSION track keeps its cooldown-aged auto-merge unchanged (already aged by -# dependabot.yml, so it does not need the GHSA gate). +# AUTO-MERGE GUARDRAILS. #2 and #3 keep DEPENDENCY-POSTURE-REVIEW.md's numbering (SEC-007, CWE-829), +# with two corrections that document does not yet carry: #3 is INVERTED from the deny-list it +# describes, and #4 is introduced HERE — the posture review does not contain it. Only #2 and #4 are +# security-track-scoped; #3 gates EVERY PR. +# #3 ALLOW-SET — HOLD UNLESS NAMED, on every track. A PR auto-merges only when EVERY dependency it +# touches is on its own ecosystem's allow row; an unnamed dependency, or an ecosystem this step +# does not recognise, routes the PR to MANUAL review. A grouped PR is denied WHOLE on any single +# ineligible member (fail-safe — never partial-merge a group). This INVERTS the former +# deny-list, whose default was merge-unless-named across every ecosystem at once. +# #2 PUBLISHED-GHSA GATE (security track only) — a SECURITY-track PR (which bypasses the cooldown +# by Dependabot design) auto-merges ONLY when a real, published, non-withdrawn advisory is +# confirmed against the dependency's PREVIOUS version (the axios-1.14.1 pattern). The advisory +# lookup FAILS CLOSED: a rate-limit/API error or no-matching-advisory routes to manual review, +# never auto-merge. The non-security VERSION track keeps its cooldown-aged auto-merge unchanged +# (already aged by dependabot.yml, so it does not need the GHSA gate). +# #4 RELEASE AGE (security track only) — a SECURITY-track PR additionally requires every candidate +# version to have been published at least MIN_RELEASE_AGE_HOURS ago, because that track is the +# one the upstream cooldown cannot age. FAILS CLOSED identically to #2: an API error, a missing +# or unparseable upload timestamp, a package name/version that does not match its expected +# shape, or an ecosystem with no publish-date source wired all route to manual review. +# READ THIS BEFORE CITING #4 AS A CONTROL: it IS INERT with respect to the merge decision as +# shipped, and recording it as operating would be exactly the false premise +# docs/Secure_Development_Standards.md §3 forbids. +# `age_ok=true` is reachable only for uv/pip (no publish-date source is wired for any +# other ecosystem), while `eligible=true` is reachable only for github-actions (the uv/pip/npm +# allow rows ship EMPTY). Those two sets are DISJOINT, so no PR can satisfy both conjuncts the +# merge `if` requires. #4 is a FORWARD guard — built, tested and gated now, load-bearing the day +# a Python allow row is populated or the advisory gate is made ecosystem-aware. Populating an +# allow row is the owner's call, not this workflow's. # -# RESIDUAL ACCEPTED RISK: a malicious patch that BOTH rides a real concurrent published advisory -# AND is not on the deny-list would still auto-merge. The deny-list shields the highest-value -# (auth/crypto) deps unconditionally; the GHSA gate blocks the no-advisory fast-publish poison on -# the security track; main's required CI (pip-audit/bandit/tests/lock-sync) backstops both. +# RESIDUAL ACCEPTED RISK, stated at the ONE path that can actually auto-merge: a VERSION-track patch +# (or dev-only minor) bumping a GitHub-published action. Its only aging is dependabot.yml's cooldown, +# which that file itself qualifies as approximate for this ecosystem, so a malicious release from a +# compromised GitHub-owned publisher would still auto-merge. Neither #2 nor #4 narrows that path — +# both are security-track-only, and this path is the version track. +# What main's required CI does and does not backstop, stated precisely: pip-audit reads the +# committed LOCKS for already-published advisories, and bandit/semgrep read THIS repository's own +# source — so none of them ever reads a dependency's contents, and a package that was malicious at +# publish time is not a known CVE. What those required checks DO catch is a bump that breaks the +# build or regresses a KNOWN advisory. No REQUIRED check reads a dependency's shipped bytes. +# security.yml's `trivy` job scans the built engine image, but it is advisory (`continue-on-error`) +# and cron/dispatch-only, so it never runs on this PR — and it matches an advisory database +# regardless. Treat the allow-set plus the aging above as the control, not a downstream scanner. # # Why `pull_request` (not pull_request_target): a Dependabot `pull_request` run gets a read-only # GITHUB_TOKEN by default, which the `permissions:` block below elevates to exactly what the merge @@ -70,33 +104,81 @@ jobs: with: github-token: ${{ secrets.GITHUB_TOKEN }} - # Guardrail #3 (DEPENDENCY-POSTURE-REVIEW.md): the auth/token/crypto stack NEVER auto-merges, - # not even a patch. dependency-names is comma-separated for grouped PRs; we deny the WHOLE - # group if ANY token matches (fail-safe — never partial-merge a group). Exact token match on - # the comma-split names (not a naive substring) so 'cffi' does not match 'argon2-cffi-bindings' - # and vice-versa. - - name: Reject security-critical deps (deny-list) - id: denylist + # Guardrail #3 (DEPENDENCY-POSTURE-REVIEW.md), INVERTED from a deny-list to an ALLOW-SET: the + # default is HOLD, and only a dependency named on its own ecosystem's allow row is eligible. + # dependency-names is comma-separated for grouped PRs; the group is denied WHOLE if ANY member + # is ineligible — the same fail-safe the deny-list had, never a partial group merge. Measured + # consequence: PR #75's five-bump batch carried `pypa/gh-action-pypi-publish`, which is NOT on + # the allow row, so that batch would now HOLD — and that action runs in the `id-token: write` + # publish job, which is exactly the case worth holding. + # + # Every input arrives via `env:`; nothing is interpolated into the body. That is zizmor + # template-injection parity, and it is what lets the behavioural tests execute this body + # verbatim under `bash -e` — the shell Actions applies by default, which those tests mirror. + - name: Hold everything not on the auto-merge allow-set + id: allowset env: DEP_NAMES: ${{ steps.meta.outputs.dependency-names }} + DEP_ECOSYSTEM: ${{ steps.meta.outputs.package-ecosystem }} run: | - # Single source of truth for the security-critical deny-list. - denylist="cryptography argon2-cffi argon2-cffi-bindings paramiko ldap3 pyspnego \ - fastapi starlette uvicorn pydantic pydantic-core python-jose pyjwt passlib bcrypt cffi" - deny=false + set -u + # Normalise with bash parameter expansion only (no echo|tr|xargs) so this block depends on + # nothing outside bash itself and stays executable from pytest on any platform. + eco="${DEP_ECOSYSTEM,,}" + eco="${eco//[[:space:]]/}" + + # These tokens are Dependabot BRANCH-NAME forms, not the dependabot.yml spellings: + # fetch-metadata derives packageEcosystem from chunks[1] of the branch name. Measured on + # live PRs — #135 `dependabot/uv/…`, #75 `dependabot/github_actions/…`, #64 + # `dependabot/npm_and_yarn/ide/…`. Both spellings are accepted so a future rename of either + # side still matches; an UNRECOGNISED token falls to `*)` and holds rather than merging. + case "$eco" in + github_actions|github-actions) + # GitHub-published actions only. GitHub already owns the runner and the job's token, so + # these add no trust root that is not already fully trusted. Deliberately NOT here: + # pypa/gh-action-pypi-publish, astral-sh/setup-uv, ossf/scorecard-action, + # contributor-assistant/github-action — every third-party action holds. + allow_prefixes="actions/ github/ dependabot/" + ;; + uv|pip|npm_and_yarn|npm) + # Deliberately EMPTY. `python-deps`/`ide-deps` group on `*`, so one weekly batch carries + # everything and a non-empty row would have to name every one of the ~98 pinned + # distributions before it could ever fire. These ecosystems were already + # review-by-default in practice; this makes it review-by-default BY DESIGN. + allow_prefixes="" + ;; + *) + echo "::notice::unrecognised package ecosystem '${eco:-}' — holding for manual review." + allow_prefixes="" + ;; + esac + + eligible=true + saw_any=false IFS=',' read -ra names <<< "$DEP_NAMES" for raw in "${names[@]}"; do - # trim surrounding whitespace and lowercase - name="$(echo "$raw" | tr '[:upper:]' '[:lower:]' | xargs)" + name="${raw,,}" + name="${name//[[:space:]]/}" [ -z "$name" ] && continue - for crit in $denylist; do - if [ "$name" = "$crit" ]; then - echo "::notice::'$name' is a security-critical dependency — routing to manual review (no auto-merge)." - deny=true - fi + saw_any=true + matched=false + for p in $allow_prefixes; do + # ANCHORED prefix test, and the trailing slash in each prefix is load-bearing. The two + # near-misses fail for DIFFERENT reasons, so both are pinned by tests: a substring + # match would admit `evil/actions/checkout`, and dropping the trailing slash would + # admit `actionsx/foo`. + case "$name" in + "$p"*) matched=true ;; + esac done + if [ "$matched" != true ]; then + echo "::notice::'$name' is not on the '${eco:-}' auto-merge allow-set — routing the WHOLE PR to manual review." + eligible=false + fi done - echo "deny=$deny" >> "$GITHUB_OUTPUT" + # An empty name list cannot CONFIRM eligibility, so it must not be read as confirming it. + [ "$saw_any" = true ] || eligible=false + echo "eligible=$eligible" >> "$GITHUB_OUTPUT" # Guardrail #2 (DEPENDENCY-POSTURE-REVIEW.md): a SECURITY-track PR auto-merges ONLY when a # real, published, non-withdrawn advisory covers the dependency's PREVIOUS version. The @@ -172,15 +254,146 @@ jobs: done <<< "$pairs" echo "advisory_ok=$advisory_ok" >> "$GITHUB_OUTPUT" + # Guardrail #4: the SECURITY track bypasses dependabot.yml's `cooldown` by Dependabot design, + # so it is aged HERE instead — the candidate version must have been published at least + # MIN_RELEASE_AGE_HOURS ago. This defeats the fast-publish-then-yank pattern (axios-1.14.1) that + # a cooldown would have caught on the version track. It costs unattended-merge LATENCY only: a + # held PR stays open and a human can still merge a genuine CVE fix immediately. + # + # The security-track discriminator is CONSUMED from the ghsa step rather than re-derived, so + # there is exactly one place that decides what "security track" means. + # + # The two cheapest fail-closed guards run BEFORE jq deliberately: that keeps them exercisable + # off the ubuntu leg, where jq is not guaranteed to exist. + # + # Gated on the allow-set because the merge `if` ALREADY requires that conjunct, so skipping here + # changes no merge outcome: a skipped step emits no `age_ok`, and the merge `if` reads a missing + # output as empty, which is not 'true'. What it does remove is an UNAUTHENTICATED outbound GET + # made from a job holding `contents: write`, on a PR that was going to be held anyway. + - name: Require the candidate release to have aged (security track) + id: age + if: steps.allowset.outputs.eligible == 'true' + env: + MIN_RELEASE_AGE_HOURS: "24" + SECURITY_TRACK: ${{ steps.ghsa.outputs.security_track }} + DEP_ECOSYSTEM: ${{ steps.meta.outputs.package-ecosystem }} + DEPS_JSON: ${{ steps.meta.outputs.updated-dependencies-json }} + run: | + set -u + if [ "$SECURITY_TRACK" != "true" ]; then + # Version track: already aged upstream by dependabot.yml's `cooldown`, so this gate does + # not apply. Emit FALSE even on this safe path — never `true` — mirroring the ghsa step + # exactly, so that no early exit anywhere in this step can fail OPEN. The merge `if` + # requires age_ok only when security_track is true. + echo "::notice::version track — aged upstream by dependabot.yml's cooldown; the release-age gate does not apply." + echo "age_ok=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + + eco="${DEP_ECOSYSTEM,,}" + eco="${eco//[[:space:]]/}" + case "$eco" in + uv|pip) ;; + *) + # NOT a new denial: the ghsa step above queries `ecosystem=pip` only, so every + # non-pip SECURITY PR already routes to manual review today. If that gate is ever made + # ecosystem-aware, wire the matching publish-date source in here at the same time — + # registry.npmjs.org's `.time[version]` for npm, the commit/release date for actions. + # Until then, writing dated code for ecosystems that cannot reach this step would be + # dead code that looks like coverage. + echo "::notice::no publish-date source wired for ecosystem '${eco:-}' — routing to manual review." + echo "age_ok=false" >> "$GITHUB_OUTPUT" + exit 0 + ;; + esac + + pairs="$(printf '%s' "$DEPS_JSON" | jq -r '.[] | [.dependencyName, .newVersion] | @tsv' 2>/dev/null || echo "ERR")" + if [ "$pairs" = "ERR" ] || [ -z "$pairs" ]; then + echo "::warning::no per-dependency metadata — failing closed (manual review)." + echo "age_ok=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + + age_ok=true + now="$(date -u +%s)" + while IFS="$(printf '\t')" read -r raw ver; do + name="${raw,,}" + name="${name//[[:space:]]/}" + if [ -z "$name" ]; then continue; fi + # `newVersion` carries the SAME update_metadata.ts `index === 0` fallback the prevVersion + # guard above cites: a non-first dependency can arrive with an empty string. An unknown + # candidate version means the age question cannot be asked, so DENY. + if [ -z "$ver" ]; then + echo "::warning::no new version for '$name' — failing closed (manual review)." + age_ok=false + break + fi + # PEP 503 normalisation: lowercase, runs of -_. collapse to a single '-'. + name="${name//[-_.]/-}" + while [[ "$name" == *--* ]]; do name="${name//--/-}"; done + # Both values are PR-DERIVED and are about to be pasted into a URL, so validate their + # shape rather than trusting them. + case "$name" in + ""|*[!a-z0-9-]*) + echo "::warning::unexpected package name '$raw' — failing closed (manual review)." + age_ok=false + break + ;; + esac + case "$ver" in + *[!A-Za-z0-9._+!-]*) + echo "::warning::unexpected version '$ver' for '$name' — failing closed (manual review)." + age_ok=false + break + ;; + esac + + body="$(curl -sS --fail --max-time 20 --retry 2 "https://pypi.org/pypi/${name}/${ver}/json" 2>/dev/null || echo "ERR")" + if [ "$body" = "ERR" ] || [ -z "$body" ]; then + echo "::warning::PyPI lookup failed for '${name}==${ver}' — failing closed (manual review)." + age_ok=false + break + fi + published="$(printf '%s' "$body" | jq -r '[.urls[].upload_time_iso_8601] | sort | .[0] // empty' 2>/dev/null || echo "ERR")" + if [ "$published" = "ERR" ] || [ -z "$published" ] || [ "$published" = "null" ]; then + echo "::warning::no upload timestamp for '${name}==${ver}' — failing closed (manual review)." + age_ok=false + break + fi + published_epoch="$(date -u -d "$published" +%s 2>/dev/null || echo "ERR")" + case "$published_epoch" in + ""|*[!0-9]*) + echo "::warning::unparseable upload timestamp '$published' for '${name}==${ver}' — failing closed." + age_ok=false + break + ;; + esac + age_hours=$(( (now - published_epoch) / 3600 )) + if [ "$age_hours" -lt 0 ]; then + echo "::warning::'${name}==${ver}' reports a FUTURE upload time (clock skew?) — failing closed." + age_ok=false + break + fi + if [ "$age_hours" -lt "$MIN_RELEASE_AGE_HOURS" ]; then + echo "::warning::'${name}==${ver}' is ${age_hours}h old, under the ${MIN_RELEASE_AGE_HOURS}h minimum — routing to manual review." + age_ok=false + break + fi + echo "::notice::'${name}==${ver}' was published ${age_hours}h ago (>= ${MIN_RELEASE_AGE_HOURS}h)." + done <<< "$pairs" + echo "age_ok=$age_ok" >> "$GITHUB_OUTPUT" + - name: Enable auto-merge for in-scope updates - # Auto-merge only when: (a) the update is an in-scope type (any patch, or a dev-only minor), - # AND (b) no security-critical dep is involved (deny-list #3), AND (c) for the SECURITY track - # ONLY, a published advisory is confirmed (#2). A non-security (version-track) patch keeps its - # cooldown-aged auto-merge — advisory_ok is not required there. + # Auto-merge only when: (a) EVERY dependency is on its ecosystem's allow row (#3), AND (b) + # for the SECURITY track ONLY, a published advisory is confirmed (#2) AND the candidate + # release has aged (#4), AND (c) the update is an in-scope type (any patch, or a dev-only + # minor). A non-security (version-track) patch keeps its cooldown-aged auto-merge — neither + # advisory_ok nor age_ok is required there, which is why both are emitted false on that path. if: >- - steps.denylist.outputs.deny != 'true' && + steps.allowset.outputs.eligible == 'true' && (steps.ghsa.outputs.security_track != 'true' || - steps.ghsa.outputs.advisory_ok == 'true') && + (steps.ghsa.outputs.advisory_ok == 'true' && + steps.age.outputs.age_ok == 'true')) && (steps.meta.outputs.update-type == 'version-update:semver-patch' || (steps.meta.outputs.update-type == 'version-update:semver-minor' && steps.meta.outputs.dependency-type == 'direct:development')) diff --git a/docs/testing/master-test-plan/01-environments-data-and-tooling.md b/docs/testing/master-test-plan/01-environments-data-and-tooling.md index 920c8c39..bfbf616e 100644 --- a/docs/testing/master-test-plan/01-environments-data-and-tooling.md +++ b/docs/testing/master-test-plan/01-environments-data-and-tooling.md @@ -347,7 +347,7 @@ Sixteen workflows. This is the complete list; the plan cites these rather than r | [`manifest-lint.yml`](.github/workflows/manifest-lint.yml) | kubeconform + ADR-0047 HA-policy grep assertions on `docker/k8s/*.yaml` | push/PR touching manifests or `docker/README.md`, dispatch | E2 | additive, not required | The only automated signal on E11 today — and it is schema/policy lint, **not** an applied deployment | | [`backlog-hygiene.yml`](.github/workflows/backlog-hygiene.yml) | A PR claiming `BACKLOG #N` and touching engine/IDE code must also update `docs/BACKLOG.md` | PR→main | E2 | not required | Keeps the status ledger honest. The structural half rides `tests/test_backlog_status_check.py` in the `test` matrix | | [`cla.yml`](.github/workflows/cla.yml) | CLA Assistant, signatures on the `cla-signatures` branch | `issue_comment`, `pull_request_target` (opened/synchronize) | E2 | **Required** (`CLA Assistant`) | Contribution gate | -| [`dependabot-auto-merge.yml`](.github/workflows/dependabot-auto-merge.yml) | Scoped auto-merge: patches + dev-only minors; an auth/token/crypto **deny-list** and a published-GHSA gate that **fails closed** | `pull_request` | E2 | n/a | Why the required-check set is load-bearing: it is the only thing standing between a dependency bump and `main` | +| [`dependabot-auto-merge.yml`](.github/workflows/dependabot-auto-merge.yml) | Scoped auto-merge: patches + dev-only minors, each held unless EVERY named dependency sits on its ecosystem's **allow-set** row (hold-unless-named, whole-group denial), plus a published-GHSA gate and a release-age gate on the security track — all **failing closed** | `pull_request` | E2 | n/a | Why the required-check set is load-bearing: it is the only thing standing between a dependency bump and `main` | | [`dependabot-lock-resync.yml`](.github/workflows/dependabot-lock-resync.yml) | Re-exports the four lock artifacts onto the Dependabot branch | PR touching `uv.lock` / `pyproject.toml` | E2 | n/a | Must stay in lockstep with security.yml's DEP-1 step — a file the gate diffs but this job does not export is un-fixable by the bot | | [`vuln-metrics.yml`](.github/workflows/vuln-metrics.yml) | NIST SSDF RV.2 KPIs from real Dependabot PRs + CISA KEV + FIRST EPSS | weekly `0 8 * * 1`, dispatch | E2 | no | Evidence artifact, not a detector | | [`release.yml`](.github/workflows/release.yml) | Build + SBOM + Sigstore sign + GitHub release + PyPI Trusted Publishing; separate `release-harness` (gated on the `PUBLISH_HARNESS` repo variable) and a `webconsole-v*` tag namespace | tag `v*` / `webconsole-v*`, dispatch (dry-run: builds/signs, never publishes) | E2 | n/a | The release chapter's substrate | diff --git a/tests/test_dependabot_automerge_guardrails.py b/tests/test_dependabot_automerge_guardrails.py index 2872231a..d3494182 100644 --- a/tests/test_dependabot_automerge_guardrails.py +++ b/tests/test_dependabot_automerge_guardrails.py @@ -1,27 +1,56 @@ # SPDX-License-Identifier: AGPL-3.0-or-later # Copyright (C) 2026 MessageFoundry Organization and contributors -"""Structural regression tests for the Dependabot auto-merge security-track guardrails (SEC-007). - -These assert the YAML wiring of ``.github/workflows/dependabot-auto-merge.yml`` — the live GitHub -Actions run is the integration test. They close DEPENDENCY-POSTURE-REVIEW.md guardrails: - #3 a DENY-LIST step keeps the auth/token/crypto stack off the auto-merge path (manual review), and - #2 a published-GHSA step gates the cooldown-bypassing SECURITY track on a real advisory. -Both must be wired into the ``Enable auto-merge`` step's ``if`` so a denied/un-advisoried PR does NOT -auto-merge, while a non-sensitive patch still does (``gh pr merge --auto`` preserved). +"""Structural AND behavioural regression tests for the Dependabot auto-merge guardrails (SEC-007). + +Most of these assert the YAML wiring of ``.github/workflows/dependabot-auto-merge.yml``; the last +three go further and **execute the shipped ``run:`` bodies under ``bash -e``** — the shell GitHub +Actions applies by default on Linux — because the guardrails are shell logic and "the YAML names a +step" is a much weaker claim than "the step holds what it must hold". That is possible only because +both new steps take every input through ``env:`` and interpolate +nothing — asserted by ``test_new_steps_take_every_input_through_env_not_interpolation``, which is a +precondition for the behavioural tests as much as it is zizmor template-injection parity. + +The guardrail numbers below are DEPENDENCY-POSTURE-REVIEW.md's, carried with two corrections that +document does not yet make — do not read them as claims about its text: + #3 an ALLOW-SET step holds every PR whose dependencies are not named on their ecosystem's allow row + (hold-unless-named; the whole group is denied on any single ineligible member). This is + **INVERTED** from the deny-list the posture review describes, and it gates EVERY track, not only + the security one. + #2 a published-GHSA step gates the cooldown-bypassing SECURITY track on a real advisory, and + #4 a release-age step additionally holds a SECURITY-track PR whose candidate version is younger + than ``MIN_RELEASE_AGE_HOURS`` — the track the upstream ``cooldown`` cannot age. **#4 is + introduced in this repository; the posture review does not carry it.** +All three must be wired into the ``Enable auto-merge`` step's ``if`` so a held/un-advisoried/fresh PR +does NOT auto-merge, while an eligible patch still does (``gh pr merge --auto`` preserved). + +⚠️ #4 cannot change a merge decision as the workflow ships, and these tests do not pretend otherwise: +``age_ok=true`` is reachable only for uv/pip, ``eligible=true`` only for github-actions, and the merge +``if`` requires both — disjoint sets. It is tested as a FORWARD guard, so that populating a Python +allow row later is a one-line change to a control that already works, not a control to be written +under pressure. ``test_release_age_is_gated_on_the_allow_set`` pins the conjunction that makes this +true, so the day someone widens it, a test has to be edited deliberately. """ from __future__ import annotations +import datetime as dt +import os +import re +import shutil +import subprocess from pathlib import Path import pytest import yaml -_WORKFLOW = ( - Path(__file__).resolve().parent.parent / ".github" / "workflows" / "dependabot-auto-merge.yml" -) +_ROOT = Path(__file__).resolve().parent.parent +_WORKFLOW = _ROOT / ".github" / "workflows" / "dependabot-auto-merge.yml" +_DEPENDABOT = _ROOT / ".github" / "dependabot.yml" -# Security-critical packages that must NEVER auto-merge, even for a patch (posture-review #3). +# Security-critical packages that must NEVER be auto-merge eligible, even for a patch +# (posture-review #3). These were the 16 names of the former DENY-LIST; the mechanism has inverted to +# an allow-set, but the PROPERTY they encoded is still the thing under test — none of them may appear +# on any allow row. Keeping the tuple keeps that property executable rather than merely intended. _DENY_PACKAGES = ( "cryptography", "argon2-cffi", @@ -33,8 +62,21 @@ "starlette", "uvicorn", "pydantic", + "pydantic-core", + "python-jose", + "pyjwt", + "passlib", + "bcrypt", + "cffi", ) +#: ``$VAR`` / ``${VAR}`` shell references, upper-case only (lower-case names are the scripts' own +#: locals, which are not inputs). +_ENV_REF = re.compile(r"\$\{?([A-Z][A-Z0-9_]*)\}?") + +#: Provided by the Actions runner itself, so a step need not declare it in ``env:``. +_RUNNER_PROVIDED = frozenset({"GITHUB_OUTPUT"}) + def _load() -> dict: return yaml.safe_load(_WORKFLOW.read_text(encoding="utf-8")) @@ -45,29 +87,126 @@ def _steps() -> list[dict]: return doc["jobs"]["auto-merge"]["steps"] +def _step(step_id: str) -> dict: + """The step with ``id: ``, failing loudly rather than returning None.""" + found = next((s for s in _steps() if s.get("id") == step_id), None) + assert found is not None, f"no step with id: {step_id}" + return found + + +def _load_dependabot() -> dict: + """Parse ``.github/dependabot.yml``, ASSERTING it is there rather than skipping when it is not. + + The guard this replaced skipped on the premise that the file is "private-only, deny-listed on the + OSS mirror". That premise is false, and `.gitignore` says so in as many words: the publish + deny-list is RETIRED, and `.github/dependabot.yml` is named in the *"DELIBERATELY NOT LISTED"* + note as content meant to ship. Measured: ``git ls-files --error-unmatch .github/dependabot.yml`` + resolves and ``git check-ignore`` exits 1. + + That matters more after this change than before it. At HEAD the guard sat inline in ONE test; + three now route through here, including ``test_every_configured_ecosystem_has_a_cooldown`` — the + test whose whole reason for existing is that a missing cooldown was invisible to CI. A skip keyed + on a false premise would have made it invisible a second way. tests/test_anon_parity.py:42-45 + records this exact lesson from the other direction: a stale skip path "silently skipped the parity + assertions below, which are the only thing keeping [the tables] identical". + """ + assert _DEPENDABOT.exists(), ( + f"{_DEPENDABOT} is a TRACKED file (see .gitignore's 'DELIBERATELY NOT LISTED' note) — its " + f"absence is the bug, not a reason to skip the cooldown assertions" + ) + return yaml.safe_load(_DEPENDABOT.read_text(encoding="utf-8")) + + +def _run_step_body( + step_id: str, + env: dict[str, str], + tmp_path: Path, + path_prepend: Path | None = None, +) -> tuple[int, dict[str, str]]: + """Run a step's shipped ``run:`` body under ``bash -e``; return ``(rc, $GITHUB_OUTPUT map)``. + + The body is executed VERBATIM — no substitution, no rewriting — which is only sound because the + step interpolates no ``${{ }}`` expressions. That invariant is asserted separately. + + **The shell is ``bash -e``, mirroring the ``bash -e {0}`` that GitHub Actions applies as its + default on Linux.** No ``shell:`` is declared anywhere in the workflow, at step, job or workflow + level — ``test_no_step_overrides_the_default_shell`` pins that, because the moment one is added + this harness stops mirroring CI. Plain ``bash`` was the wrong shell to assert under: it keeps + going after a failing command where CI aborts the step, which is a difference these tests would + have reported as agreement. Measured on the shipped bodies, every row here decides identically + under both — so this is a fidelity fix, not a behaviour change — but the returncode is now + RETURNED rather than asserted here, so an abort path can be expressed as an expected outcome + instead of being indistinguishable from a harness bug. + + Any LITERAL value in the step's own ``env:`` (one carrying no ``${{ }}``) is seeded first, so a + tuning constant like ``MIN_RELEASE_AGE_HOURS`` is exercised at its SHIPPED value rather than at + one this test invented. The caller's ``env`` then overlays the PR-derived inputs. + """ + bash = shutil.which("bash") + assert bash is not None # narrowed by the caller's skipif; keeps mypy honest + step = _step(step_id) + body = str(step["run"]) + assert "${{" not in body, ( + f"the {step_id} body interpolates an Actions expression, so this test is no longer executing " + f"what CI executes" + ) + script = tmp_path / f"{step_id}.sh" + script.write_text(body, encoding="utf-8") + out_file = tmp_path / f"{step_id}.githuboutput" + out_file.write_text("", encoding="utf-8") + + full_env = dict(os.environ) + for key, value in (step.get("env") or {}).items(): + if "${{" not in str(value): + full_env[str(key)] = str(value) + full_env.update(env) + full_env["GITHUB_OUTPUT"] = out_file.as_posix() + if path_prepend is not None: + full_env["PATH"] = f"{path_prepend.as_posix()}{os.pathsep}{full_env.get('PATH', '')}" + + proc = subprocess.run( + [bash, "-e", script.as_posix()], + capture_output=True, + text=True, + timeout=120, + env=full_env, + ) + parsed: dict[str, str] = {} + for line in out_file.read_text(encoding="utf-8").splitlines(): + if "=" in line: + key, _, value = line.partition("=") + parsed[key.strip()] = value.strip() + return proc.returncode, parsed + + +# -------------------------------------------------------------------------------------------------- +# Structural: the workflow's wiring +# -------------------------------------------------------------------------------------------------- + + def test_workflow_is_valid_yaml_and_has_the_automerge_job() -> None: doc = _load() assert "auto-merge" in doc["jobs"] -def test_denylist_step_names_every_security_critical_package() -> None: - """Guardrail #3: the deny-list step body must hard-code each security-critical package.""" - steps = _steps() - deny = next((s for s in steps if s.get("id") == "denylist"), None) - assert deny is not None, "no deny-list step (id: denylist) found" - body = deny.get("run", "") +def test_no_security_critical_package_is_auto_merge_eligible() -> None: + """Guardrail #3, as a PROPERTY rather than a mechanism: no security-critical package may be + reachable through the allow-set. The former deny-list named them to EXCLUDE them; the allow-set + holds by default, so the equivalent assertion is that none of them appears on any allow row.""" + allowset = _step("allowset") + body = str(allowset.get("run", "")) for pkg in _DENY_PACKAGES: - assert pkg in body, f"deny-list missing security-critical package: {pkg}" + assert pkg not in body, ( + f"'{pkg}' appears in the allow-set step — a security-critical package must never be " + f"auto-merge eligible (posture-review #3)" + ) # emits a guard output the merge step can require - assert "deny=" in body and "$GITHUB_OUTPUT" in body + assert "eligible=" in body and "$GITHUB_OUTPUT" in body def test_ghsa_step_queries_the_advisory_api_and_emits_a_guard() -> None: """Guardrail #2: a security-track step must consult the advisory API and emit advisory_ok.""" - steps = _steps() - ghsa = next((s for s in steps if s.get("id") == "ghsa"), None) - assert ghsa is not None, "no published-GHSA step (id: ghsa) found" - body = ghsa.get("run", "") + body = str(_step("ghsa").get("run", "")) # calls the GitHub advisories API (gh api ... /advisories) assert "gh api" in body and "/advisories" in body # produces the advisory guard output and a security-track discriminator @@ -77,32 +216,298 @@ def test_ghsa_step_queries_the_advisory_api_and_emits_a_guard() -> None: assert "advisory_ok=false" in body -def test_enable_automerge_gates_on_both_guards_and_preserves_auto_merge() -> None: - """The merge step's ``if`` must require deny != 'true' AND the security-track advisory guard, - while still invoking ``gh pr merge --auto`` for the non-sensitive path.""" +def test_release_age_step_is_security_track_only_and_fails_closed() -> None: + """Guardrail #4: an ``age`` step must exist AFTER ``ghsa``, consume that step's security-track + discriminator rather than re-deriving it, and never hard-code a PASS on an early exit.""" + ids = [s.get("id") for s in _steps()] + assert "age" in ids, "no release-age step (id: age) found" + assert ids.index("age") > ids.index("ghsa"), ( + "the age step must come AFTER ghsa — it consumes ghsa's security_track output" + ) + + age = _step("age") + env = age.get("env") or {} + assert "MIN_RELEASE_AGE_HOURS" in env, "the age threshold must be a declared, visible input" + assert "steps.ghsa.outputs.security_track" in str(env.get("SECURITY_TRACK", "")), ( + "the security-track discriminator must be CONSUMED from the ghsa step, not re-derived — " + "one source of truth for what 'security track' means" + ) + + body = str(age.get("run", "")) + emits = [ln.strip() for ln in body.splitlines() if "$GITHUB_OUTPUT" in ln and "age_ok" in ln] + assert emits, "the age step emits no age_ok output" + # Fails closed: every literal emission is a DENY; the only PASS is the computed variable, which + # can only be true after the loop has consulted a publish date. + for line in emits: + assert "age_ok=true" not in line, f"an early exit hard-codes a PASS, failing OPEN: {line}" + assert any("age_ok=false" in ln for ln in emits), "no fail-closed emission found" + assert any("age_ok=$age_ok" in ln for ln in emits), "no computed emission found" + + +def test_release_age_is_gated_on_the_allow_set() -> None: + """The age step must not run for a PR the allow-set already holds. + + Two distinct reasons, and the weaker one is the one usually noticed. The privacy/egress reason: + this step makes an UNAUTHENTICATED outbound GET from a job holding ``contents: write``, so making + it for a PR that was going to be held anyway is gratuitous. The honesty reason, which is why the + assertion lives here rather than in a comment: this ``if`` is half of the conjunction that makes + guardrail #4 INERT today (``age_ok=true`` needs uv/pip, ``eligible=true`` needs github-actions). + Pinning it means widening #4 into a live control has to edit a test, not slip through as prose. + """ + age = _step("age") + cond = str(age.get("if", "")) + assert "steps.allowset.outputs.eligible == 'true'" in cond, ( + "the age step is not gated on the allow-set, so it performs a privileged-job network fetch " + f"for PRs that hold regardless; if: {cond!r}" + ) + + +def test_no_step_overrides_the_default_shell() -> None: + """Nothing may declare ``shell:``, at step, job or workflow level. + + Not style. ``_run_step_body`` executes the shipped bodies under ``bash -e`` on the strength of + Actions' documented Linux default (``bash -e {0}``). Declare a ``shell:`` anywhere and that + premise silently stops holding, leaving a behavioural suite that asserts confidently about a + shell CI no longer uses. This is the tripwire for that. + """ + doc = _load() + assert "defaults" not in doc, "a workflow-level defaults: block can change the shell" + job = doc["jobs"]["auto-merge"] + assert "defaults" not in job, "a job-level defaults: block can change the shell" + for step in job["steps"]: + assert "shell" not in step, ( + f"step {step.get('id') or step.get('name')!r} declares an explicit shell:, so the " + f"behavioural tests no longer mirror what CI runs — update _run_step_body with it" + ) + + +def test_enable_automerge_gates_on_all_three_guards_and_preserves_auto_merge() -> None: + """The merge step's ``if`` must require the allow-set guard AND the two security-track guards, + while still invoking ``gh pr merge --auto`` for the eligible path.""" steps = _steps() - merge = next((s for s in steps if "gh pr merge --auto" in s.get("run", "")), None) + merge = next((s for s in steps if "gh pr merge --auto" in str(s.get("run", ""))), None) assert merge is not None, ( "the gh pr merge --auto step was removed (auto-merge must be preserved)" ) - cond = merge.get("if", "") - assert "steps.denylist.outputs.deny != 'true'" in cond, "merge not gated on the deny-list guard" - # security-track PRs require a confirmed advisory; version-track patches stay unchanged + cond = str(merge.get("if", "")) + assert "steps.allowset.outputs.eligible == 'true'" in cond, ( + "merge not gated on the allow-set guard" + ) + # security-track PRs require a confirmed advisory AND an aged release; version-track patches + # keep their cooldown-aged auto-merge unchanged assert "steps.ghsa.outputs.advisory_ok == 'true'" in cond assert "steps.ghsa.outputs.security_track != 'true'" in cond + assert "steps.age.outputs.age_ok == 'true'" in cond, "merge not gated on the release-age guard" # the in-scope update-type gate is still present (any patch / dev-only minor) assert "version-update:semver-patch" in cond + # A half-removal would leave the merge condition referencing a step id that no longer exists — + # an Actions expression against a missing step evaluates to empty, silently dropping the guard. + text = _WORKFLOW.read_text(encoding="utf-8") + assert "steps.denylist" not in text, "dangling reference to the removed deny-list step" + assert not any(s.get("id") == "denylist" for s in steps), "the deny-list step is still present" + + +def test_new_steps_take_every_input_through_env_not_interpolation() -> None: + """Both new steps must read their inputs from ``env:``, never from an inlined ``${{ }}``. + + This is zizmor template-injection parity asserted in-tree — a PR-controlled value pasted into a + shell body is a code-execution seam — and it is simultaneously the precondition that lets the + behavioural tests below execute the shipped bodies verbatim.""" + for step_id in ("allowset", "age"): + step = _step(step_id) + body = str(step.get("run", "")) + assert "${{" not in body, ( + f"the {step_id} step interpolates an Actions expression into its shell body; pass it " + f"through env: instead" + ) + declared = set((step.get("env") or {}).keys()) + referenced = set(_ENV_REF.findall(body)) - _RUNNER_PROVIDED + undeclared = referenced - declared + assert not undeclared, f"{step_id} reads undeclared env var(s): {sorted(undeclared)}" + + +# -------------------------------------------------------------------------------------------------- +# Structural: the upstream cooldown configuration +# -------------------------------------------------------------------------------------------------- + def test_cooldown_aging_window_lengthened() -> None: """Posture-review step 3: the uv-ecosystem routine cooldown is widened to >= 5 days.""" - dependabot = Path(__file__).resolve().parent.parent / ".github" / "dependabot.yml" - if not dependabot.exists(): - # Private-only: .github/dependabot.yml is deny-listed on the OSS mirror (dependency updates - # are managed on the private source repo). Skip where it's absent — the sibling tests assert - # the shipped dependabot-auto-merge.yml workflow and still run on the mirror. Mirrors the - # skip-if-absent guard in tests/test_anon_parity.py and tests/test_load_config.py. - pytest.skip("dependabot.yml is private-only (OSS-mirror deny-list)") - doc = yaml.safe_load(dependabot.read_text(encoding="utf-8")) + doc = _load_dependabot() uv = next(u for u in doc["updates"] if u["package-ecosystem"] == "uv") assert uv["cooldown"]["default-days"] >= 5 + + +def test_every_configured_ecosystem_has_a_cooldown() -> None: + """EVERY configured ecosystem must be cooldown-aged, not just uv. + + The sibling test above asserts uv alone, which is precisely why a missing ``cooldown`` on + ``github-actions`` — the ecosystem whose artifacts execute INSIDE CI — was invisible to CI. This + is the test that would have caught it.""" + doc = _load_dependabot() + updates = doc["updates"] + scanned = [u["package-ecosystem"] for u in updates] + print(f"ecosystems scanned for a cooldown: {scanned}") + # Liveness: a pass must mean "checked all of them", never "found none to check". + assert len(updates) >= 3, ( + f"only {len(updates)} update entries found ({scanned}) — the dependabot.yml walk is probably " + f"broken, which would make the per-ecosystem assertion below vacuous" + ) + for entry in updates: + eco = entry["package-ecosystem"] + assert "cooldown" in entry, f"ecosystem '{eco}' has no cooldown — a fresh release is unaged" + days = entry["cooldown"].get("default-days") + assert isinstance(days, int) and days >= 1, ( + f"ecosystem '{eco}' has a non-positive/absent cooldown default-days: {days!r}" + ) + + +def test_ci_executing_ecosystem_is_aged_at_least_five_days() -> None: + """``github-actions`` specifically must age >= 5 days: unlike a Python distribution, a bumped + action executes INSIDE CI holding whatever token the job carries, so a fresh malicious release is + an immediate code-execution primitive rather than a dependency to be imported later.""" + doc = _load_dependabot() + actions = next(u for u in doc["updates"] if u["package-ecosystem"] == "github-actions") + assert actions["cooldown"]["default-days"] >= 5 + + +# -------------------------------------------------------------------------------------------------- +# Behavioural: execute the shipped shell +# -------------------------------------------------------------------------------------------------- + + +@pytest.mark.skipif(shutil.which("bash") is None, reason="needs bash to execute the run: body") +@pytest.mark.parametrize( + ("ecosystem", "names", "expected"), + [ + # The two eligible rows are what make this test non-vacuous: an always-false implementation + # (or an empty github-actions allow row) FAILS here rather than passing everything else. + ("github_actions", "actions/checkout", "true"), + ("github_actions", "actions/checkout,github/codeql-action/init", "true"), + # Measured PR #75: a five-bump batch carrying a third-party action holds the WHOLE group. + ("github_actions", "actions/checkout,pypa/gh-action-pypi-publish", "false"), + # Anchored prefix, not substring: neither of these is actions/. + ("github_actions", "actionsx/foo", "false"), + ("github_actions", "evil/actions/checkout", "false"), + # An empty name list cannot confirm eligibility. + ("github_actions", "", "false"), + # uv/npm allow rows ship empty, so every bump on them holds. + ("uv", "certifi", "false"), + ("uv", "cryptography", "false"), + ("npm_and_yarn", "@types/node", "false"), + ("pip", "requests", "false"), + # An unrecognised (here: empty) ecosystem token must fall to the hold arm. + ("", "actions/checkout", "false"), + ], +) +def test_allowset_holds_everything_not_named( + ecosystem: str, names: str, expected: str, tmp_path: Path +) -> None: + """Execute the shipped allow-set body and check what it actually decides.""" + rc, out = _run_step_body( + "allowset", + {"DEP_NAMES": names, "DEP_ECOSYSTEM": ecosystem}, + tmp_path, + ) + assert rc == 0, f"the allowset body aborted under `bash -e` (rc={rc}) — CI would fail the step" + assert out.get("eligible") == expected, ( + f"ecosystem={ecosystem!r} names={names!r} -> eligible={out.get('eligible')!r}, " + f"expected {expected!r}" + ) + + +@pytest.mark.skipif(shutil.which("bash") is None, reason="needs bash to execute the run: body") +@pytest.mark.parametrize( + ("security_track", "ecosystem"), + [ + # Version track: aged upstream by dependabot.yml's cooldown, so this gate emits false and the + # merge condition simply does not require it. + ("false", "uv"), + ("", "uv"), + # Security track on an ecosystem with no publish-date source wired. + ("true", "github_actions"), + ("true", "npm_and_yarn"), + ("true", ""), + ], +) +def test_release_age_holds_the_version_track_and_undatable_ecosystems( + security_track: str, ecosystem: str, tmp_path: Path +) -> None: + """The two cheapest fail-closed guards, exercised WITHOUT jq or network. + + Every row here must emit ``age_ok=false``. On its own that is not self-certifying — an + unconditionally-false step would also pass — so the discriminating ``true`` case is supplied by + ``test_release_age_passes_an_aged_release_and_holds_a_fresh_one`` below. These two tests are a + pair; this one runs everywhere, that one needs jq.""" + rc, out = _run_step_body( + "age", + { + "SECURITY_TRACK": security_track, + "DEP_ECOSYSTEM": ecosystem, + "DEPS_JSON": '[{"dependencyName":"requests","newVersion":"2.32.3"}]', + }, + tmp_path, + ) + assert rc == 0, f"the age body aborted under `bash -e` (rc={rc}) — CI would fail the step" + assert out.get("age_ok") == "false", ( + f"security_track={security_track!r} ecosystem={ecosystem!r} -> " + f"age_ok={out.get('age_ok')!r}, expected 'false'" + ) + + +def _curl_stub(tmp_path: Path, payload: str | None) -> Path: + """A ``curl`` on PATH that ignores its arguments. ``payload=None`` makes it fail like a network + or HTTP error would (``--fail`` exits non-zero), which must route to manual review.""" + stub_dir = tmp_path / "stub" + stub_dir.mkdir() + stub = stub_dir / "curl" + if payload is None: + stub.write_text("#!/usr/bin/env bash\nexit 1\n", encoding="utf-8") + else: + stub.write_text(f"#!/usr/bin/env bash\ncat <<'JSON'\n{payload}\nJSON\n", encoding="utf-8") + stub.chmod(0o755) + return stub_dir + + +def _iso(hours_ago: float) -> str: + when = dt.datetime.now(dt.UTC) - dt.timedelta(hours=hours_ago) + return when.strftime("%Y-%m-%dT%H:%M:%S.%f") + "Z" + + +@pytest.mark.skipif( + shutil.which("bash") is None or shutil.which("jq") is None, + reason="needs bash + jq. It runs WHEREVER both exist — the ubuntu leg AND the two REQUIRED " + "windows-2022/windows-2025 legs, whose runner images ship jq and Git Bash — and skips on the " + "maintainer's box, where Git Bash carries no jq. A red here is therefore not necessarily a " + "ubuntu-only red", +) +@pytest.mark.parametrize( + ("label", "payload", "expected"), + [ + # The discriminating PASS — without this row the whole release-age suite would be satisfied + # by a step that denies unconditionally. + ("aged 30 days", f'{{"urls":[{{"upload_time_iso_8601":"{_iso(24 * 30)}"}}]}}', "true"), + ("published 1 hour ago", f'{{"urls":[{{"upload_time_iso_8601":"{_iso(1)}"}}]}}', "false"), + ("no dateable artifact", '{"urls":[]}', "false"), + ("PyPI error", None, "false"), + ], +) +def test_release_age_passes_an_aged_release_and_holds_a_fresh_one( + label: str, payload: str | None, expected: str, tmp_path: Path +) -> None: + """The security-track happy path and its three failure modes, against a stubbed PyPI.""" + rc, out = _run_step_body( + "age", + { + "SECURITY_TRACK": "true", + "DEP_ECOSYSTEM": "uv", + "DEPS_JSON": '[{"dependencyName":"requests","newVersion":"2.32.3"}]', + }, + tmp_path, + path_prepend=_curl_stub(tmp_path, payload), + ) + assert rc == 0, f"the age body aborted under `bash -e` (rc={rc}) — CI would fail the step" + assert out.get("age_ok") == expected, ( + f"{label} -> age_ok={out.get('age_ok')!r}, expected {expected!r}" + ) From 53ee15e4db06fea4a060efa89bb1a4477ac58c57 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 4 Aug 2026 20:25:45 -0500 Subject: [PATCH 2/2] docs(backlog): flip #336 to SHIPPED and supersede one clause of its 2026-08-03 amendment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Banner lines of item #336 ONLY. VERIFIED BY NUMBER, not by banner text — a byte-identical banner pasted under the wrong item survives every well-formedness check. `git diff -U0 -- docs/BACKLOG.md` is two hunks, @@ -3353 +3353 @@ and @@ -3358,0 +3359,2 @@; the enclosing `## ` heading computed backwards from each of the three changed lines (3353, 3359, 3360) is `## 336. Dependabot auto-merge shields review with a deny-list` in all three cases. The ranked table and the four census distribution lines are untouched. THE CENSUS WAS NOT RECOMPUTED. This commit flips one item's banner and deliberately does not touch the distribution lines. The banner records guardrail #4 as a FORWARD guard that is inert with respect to the merge decision as shipped, rather than as an operating control: age_ok=true is reachable only for uv/pip, eligible=true only for github-actions, and the merge `if` requires both. Recording an unreachable control as operative is the false-premise class docs/Secure_Development_Standards.md §3 forbids. The added blockquote uses the glyph-free `**AMENDED 2026-08-04 — ...**` convention. It supersedes ONE clause of the dated 2026-08-03 note — "The deny-list itself is untouched, so the rest of the item stands ... §§1, 3, 4, 5 and 6 are unaffected" — which was accurate when measured and is deliberately left as written rather than rewritten. The deny-list no longer exists, so §6 is discharged by deletion rather than annotation, and the 16 names survive only as a PROPERTY under test (`_DENY_PACKAGES` asserts none of them reaches any allow row). scripts/docs/backlog_status_check.py exits 0 ("290 backlog items, each declaring exactly one status") — reported as corroboration only. That gate validates that one banner is present and self-consistent, never that it belongs to this item, so it cannot see the corruption class this commit had to avoid. The by-number check above is the evidence; the green gate is not. --- docs/BACKLOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 8ecc7b56..f91e3c60 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -3350,12 +3350,14 @@ What is *not* covered is the thing that will grow: `.semgrep/messagefoundry.yml` ## 336. Dependabot auto-merge shields review with a deny-list -> 🔢 **Filed 2026-08-01 — not started.** Value **5/10** · Difficulty **3/10** · _fill-in_. Auto-merge still keys only on `update-type == 'version-update:semver-patch'` behind a 16-name Python deny-list with no ecosystem filter, so npm and `github-actions` — artifacts that execute inside CI holding the job's token — have zero shield coverage, and `tests/test_dependabot_automerge_guardrails.py:107-108` still asserts a cooldown for the `uv` ecosystem alone; the remainder is a deny-to-allow inversion in one workflow, a workflow-side release-age check for the cooldown-bypassing security track, and broadening one test. +> ✅ **SHIPPED 2026-08-04 — guardrail #3 inverted from a 16-name deny-list to an ecosystem-qualified ALLOW-SET (hold unless named); a fail-closed release-age gate added as #4.** Value **5/10** · Difficulty **3/10** · _fill-in_. §1 `.github/workflows/dependabot-auto-merge.yml` now holds any PR whose dependencies are not on their own ecosystem's allow row — `actions/`/`github/`/`dependabot/` for `github-actions`, deliberately EMPTY for `uv` and `npm`, and an unrecognised ecosystem token holds rather than merges — preserving the fail-safe whole-group denial (measured: PR #75's five-bump batch carried `pypa/gh-action-pypi-publish`, so that batch would now HOLD). §4 a new `id: age` step requires every SECURITY-track candidate version to have been published at least 24h, failing closed on an API error, an absent/unparseable upload timestamp, an unexpected name or version shape, or an ecosystem with no publish-date source wired. ⚠️ **#4 is a FORWARD guard and is INERT with respect to the merge decision as shipped** — `age_ok=true` is reachable only for `uv`/`pip`, `eligible=true` only for `github-actions`, and the merge `if` requires both, so the two sets are disjoint. It is recorded that way in the workflow header rather than as an operating control, and gated on the allow-set so it makes no unauthenticated outbound request from the `contents: write` job for a PR that holds regardless; it becomes load-bearing the day a Python allow row is populated (an owner decision) or the advisory gate is made ecosystem-aware. §3 `tests/test_dependabot_automerge_guardrails.py` now asserts a cooldown on EVERY configured ecosystem behind a vacuity floor, and executes the shipped `run:` bodies under `bash -e` — the shell GitHub Actions actually applies — rather than only reading the YAML. §5 the false-premise backstop clause is corrected rather than merely deleted: no REQUIRED check reads a dependency's shipped bytes, and `trivy`, which does read the built image's bytes, is advisory (`continue-on-error`) and cron/dispatch-only, so it never runs on a Dependabot PR at all. **§2 was ALREADY SHIPPED** by the 2026-08-03 amendment (`.github/dependabot.yml` sets `cooldown.default-days: 5` on `github-actions`) and was NOT rebuilt; **§6 is discharged by DELETING the deny-list** rather than pruning it, which removes `python-jose`/`pyjwt`/`passlib` — all three absent from `requirements.lock`'s 98 pinned distributions — along with it. > ⚠️ **AMENDED 2026-08-03 — the `github-actions` cooldown SHIPPED, discharging Proposed §2 in substance and half the false-premise finding with it.** The second measured-at-HEAD bullet asserts that ecosystem *"carries `schedule` + `groups` only; there is no `cooldown:` key"*, but `.github/dependabot.yml:83-84` now sets `default-days: 5` for it, with the rationale at `:75-82` (#75 took two of five bumps to `main` under 24h from publish; `codeql-action` v4.37.4 was 7h old). So the *"Bounding this honestly"* line **"Only `github-actions` is unaged"** no longer holds, and each of the three configured ecosystems now has a cooldown behind the header's claim (now at `.github/workflows/dependabot-auto-merge.yml:22-24`, not `:16-18`). ⚠️ **Read §2 as discharged in substance, not to the letter** — `.github/dependabot.yml:79-80` records that this ecosystem honors `default-days` alone and ages off the **tag's commit date**, "so treat 5 as approximate", which is why §2's *"matching uv's 5/7"* could not be met. > > **The deny-list itself is untouched, so the rest of the item stands:** 16 Python names at `:84-85` gating every ecosystem behind an author-only job condition (`:64`) with no ecosystem qualifier, a merge gate still keying on `version-update:semver-patch` (`:184`), `tests/test_dependabot_automerge_guardrails.py:107-108` still asserting a cooldown for `uv` alone, and `python-jose` / `pyjwt` / `passlib` still absent from `requirements.lock` — so §§1, 3, 4, 5 and 6 are unaffected, as is the Why's other leg (`.github/workflows/security.yml:261-262` still describes pip-audit as *lockfile only* and bandit/semgrep as *source only*). ⚠️ **At least four `dependabot-auto-merge.yml` citations above (`:16-18`, `:58`, `:78-79`, `:155-161`) and all four `dependabot.yml` ones now point at different lines** — re-measure before quoting one; the `tests/` and `security.yml` citations are still exact. +> **AMENDED 2026-08-04 — one clause of the 2026-08-03 note above is superseded by the SHIPPED banner; the dated measurement itself stands and is deliberately left as written.** *"The deny-list itself is untouched, so the rest of the item stands … §§1, 3, 4, 5 and 6 are unaffected"* was accurate when measured. It is not now: the deny-list no longer exists — guardrail #3 is an allow-set — so §6 is discharged by deletion rather than annotation, and §§1, 3, 4 and 5 are built rather than merely unaffected. The 16 names survive only as a PROPERTY under test (`_DENY_PACKAGES` in `tests/test_dependabot_automerge_guardrails.py` asserts none of them reaches any allow row), not as a mechanism. + **Cluster:** Security / Supply chain. **Priority:** P3. **Verdict:** build. **Severity:** low.