diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml index 74089abd..7a040d95 100644 --- a/.github/workflows/dependabot-auto-merge.yml +++ b/.github/workflows/dependabot-auto-merge.yml @@ -217,7 +217,12 @@ jobs: # low first-dep version makes almost every other name match: false confirmation in the # PERMISSIVE direction (measured against the live endpoint at @0.7.0: aiohttp 30, pillow 29, # urllib3 9, requests 6). `updated-dependencies-json` carries a per-dependency prevVersion. - pairs="$(printf '%s' "$DEPS_JSON" | jq -r '.[] | [.dependencyName, .prevVersion] | @tsv' 2>/dev/null || echo "ERR")" + # `||` outside the substitution (see the advisory lookup below). It matters here for a + # second reason: jq STREAMS, so a mid-array error can leave earlier rows already on stdout, + # and the old form would have appended ERR to a TRUNCATED dependency list — iterating a + # subset of the PR's dependencies while still reporting success. Assigning on failure + # discards the partial output instead of inheriting it. + pairs="$(printf '%s' "$DEPS_JSON" | jq -r '.[] | [.dependencyName, .prevVersion] | @tsv' 2>/dev/null)" || pairs="ERR" if [ "$pairs" = "ERR" ] || [ -z "$pairs" ]; then echo "::warning::no per-dependency metadata — failing closed (manual review)." echo "advisory_ok=false" >> "$GITHUB_OUTPUT" @@ -236,15 +241,30 @@ jobs: advisory_ok=false break fi + # The `||` binds the ASSIGNMENT, never the substitution. `x=$(cmd || echo ERR)` APPENDS + # the sentinel to whatever cmd already wrote to stdout, and `gh api` copies the JSON + # error BODY to stdout on any HTTP error (only the `gh: ... (HTTP nnn)` line goes to + # stderr, which `2>/dev/null` eats). The old form left `count` holding + # `{"message":"API rate limit exceeded",...}ERR` — neither "ERR" nor empty, so the + # sentinel MISSED; `[ "$count" -lt 1 ]` then failed with "integer expression expected" + # and returned 2, which an `if` condition is exempt from under `set -e`. The step printed + # "advisory confirmed", wrote advisory_ok=true and exited 0 — this guard inverted to + # FAIL-OPEN on precisely the rate-limit/API-error class the comment above names first. count="$(gh api -X GET /advisories \ -f ecosystem=pip \ -f affects="${name}@${prev}" \ - --jq '[.[] | select(.withdrawn_at == null)] | length' 2>/dev/null || echo "ERR")" - if [ "$count" = "ERR" ] || [ -z "$count" ]; then - echo "::warning::advisory lookup failed for '$name' — failing closed (manual review)." - advisory_ok=false - break - fi + --jq '[.[] | select(.withdrawn_at == null)] | length' 2>/dev/null)" || count="ERR" + # A SHAPE check, not equality against one sentinel. The numeric comparison below needs + # "is this a number" answered, and only a shape test answers it for values nobody + # anticipated — an equality test recognises exactly the failure it was told about, which + # is how a JSON body walked through the old guard. + case "$count" in + ""|*[!0-9]*) + echo "::warning::advisory lookup failed for '$name' — failing closed (manual review)." + advisory_ok=false + break + ;; + esac if [ "$count" -lt 1 ]; then echo "::warning::no published advisory covers '${name}@${prev}' — failing closed (manual review)." advisory_ok=false @@ -307,7 +327,8 @@ jobs: ;; esac - pairs="$(printf '%s' "$DEPS_JSON" | jq -r '.[] | [.dependencyName, .newVersion] | @tsv' 2>/dev/null || echo "ERR")" + # `||` outside the substitution, and for the same streaming-jq reason as guardrail #2's. + pairs="$(printf '%s' "$DEPS_JSON" | jq -r '.[] | [.dependencyName, .newVersion] | @tsv' 2>/dev/null)" || pairs="ERR" if [ "$pairs" = "ERR" ] || [ -z "$pairs" ]; then echo "::warning::no per-dependency metadata — failing closed (manual review)." echo "age_ok=false" >> "$GITHUB_OUTPUT" @@ -348,19 +369,22 @@ jobs: ;; esac - body="$(curl -sS --fail --max-time 20 --retry 2 "https://pypi.org/pypi/${name}/${ver}/json" 2>/dev/null || echo "ERR")" + # `--fail` suppresses the 4xx/5xx body, so the old form happened to work for HTTP errors + # — but a TRUNCATED transfer (curl 18/56) exits non-zero with partial bytes already on + # stdout, and the sentinel would have been appended to a half a JSON document. Outside. + body="$(curl -sS --fail --max-time 20 --retry 2 "https://pypi.org/pypi/${name}/${ver}/json" 2>/dev/null)" || body="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")" + published="$(printf '%s' "$body" | jq -r '[.urls[].upload_time_iso_8601] | sort | .[0] // empty' 2>/dev/null)" || published="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")" + published_epoch="$(date -u -d "$published" +%s 2>/dev/null)" || published_epoch="ERR" case "$published_epoch" in ""|*[!0-9]*) echo "::warning::unparseable upload timestamp '$published' for '${name}==${ver}' — failing closed." diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 99d03eb0..9b824f59 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -8981,3 +8981,73 @@ which means it cannot catch the next one. Follow the VALUE. **Source:** raised by the `asvs-tracking-rework` session on 2026-08-09 after `proxy` -> `proxy_url` became the third instance: *"that is not a coincidence to note in a residual; it is an argument that the rename boundary itself needs a guard"*. Filed before it was forgotten, per that session's request. +## 1209. the dependency advisory guard inverts to FAIL-OPEN when the advisory API errors + +> 🔢 **Filed 2026-08-09 - FIXED IN THE SAME CHANGE, entry published WITH the fix.** Value **9/10** · Difficulty **2/10**. Guardrail #2 of `dependabot-auto-merge.yml` reads `count="$(gh api ... || echo "ERR")"`. The `||` runs INSIDE the command substitution, so it APPENDS to stdout rather than replacing it - and `gh api` copies the JSON error BODY to stdout on any HTTP error. The sentinel `[ "$count" = "ERR" ]` therefore misses, and the guard emits `advisory_ok=true` for a lookup that never succeeded. + +**Cluster:** CI / supply chain. **Priority:** P1. **Verdict:** build (done). +**Severity:** unlike the redaction items above, this is not conditional on a first deployment - the +workflow runs in CI today. What bounds it is narrower and worth stating exactly: the engine's merge +condition also requires `age_ok`, and the age step returns false for every ecosystem that can be +`eligible`, a disjointness the file documents about itself. So the falsely-true `advisory_ok` cannot +ALONE merge anything as shipped. It flips a security decision the workflow publishes, and the file +labels the surviving blocker "a FORWARD guard ... load-bearing the day a Python allow row is +populated" - one line's edit away from making this directly merge-affecting. + +**The mechanism, reproduced end to end against the shipped step body:** + +``` +gh api on any HTTP error: JSON body -> STDOUT, "gh: ... (HTTP nnn)" -> stderr (eaten by 2>/dev/null) + count = '{"message":"API rate limit exceeded","status":"403"}ERR' + [ "$count" = "ERR" ] || [ -z "$count" ] -> MISSES (neither) + [ "$count" -lt 1 ] -> "integer expression expected", returns 2 + -> an `if` CONDITION is exempt from `set -e` + -> "::notice::published advisory confirmed", advisory_ok=true, step exits 0 +``` + +Measured by running the real `ghsa` body from `origin/main` and from the fix, under `bash -e`, with a +`gh` stub reproducing the stream split: + +``` + gh ERRORS gh returns 1 +pre-fix (main) advisory_ok=true advisory_ok=true <- FAIL OPEN +fixed advisory_ok=false advisory_ok=true <- fails closed, happy path intact +``` + +**A stub that merely exits non-zero would have proved nothing** - it would pass against the defective +code too. The defect is that the BODY reached the variable, so the stub has to write the body. + +**The comment directly above the defect asserted the opposite:** "Fail closed on any error", and the +header, "a rate-limit/API error or no-matching-advisory routes to manual review, never auto-merge." +A compensating control resting on a false premise, which is the shape SDS-3.7 names. + +**The existing test could not see it.** `test_ghsa_step_queries_the_advisory_api_and_emits_a_guard` +asserted the STRING `"advisory_ok=false" in body` - satisfied by a step that merely CONTAINS the words, +and the fail-open lived underneath a passing version of exactly that check. The file already had the +right instrument: `_run_step_body` executes shipped `run:` bodies under `bash -e` and returns the +parsed `$GITHUB_OUTPUT`. Guardrail #2 was the one guard not using it. +`test_the_advisory_guard_fails_closed_when_the_api_errors` now executes the body across three rows, +including a discriminating PASS so the suite cannot be satisfied by a step that denies unconditionally. + +**The domain, because fixing one instance is how this class survives:** a sweep of 63 workflow and +script files across both repositories found 24 instances of the idiom - 16 provably harmless (`git +rev-parse --verify --quiet` writes nothing on failure), and the rest fixed here. Moving the `||` outside +the substitution also fixes the streaming cases for free: jq emits rows before a mid-array error, and +the old form would have appended the sentinel to a TRUNCATED dependency list while still reporting +success. Assigning on failure discards partial output instead of inheriting it. + +The `count` guard additionally moved from an equality test against one sentinel to a SHAPE test +(`case "$count" in ""|*[!0-9]*)`). An equality test recognises exactly the failure it was told about, +which is how a JSON body walked through it; the numeric comparison's real question is "is this a +number", and only a shape test answers that for values nobody anticipated. + +**Sibling, same idiom, in the private scorecard repo:** its `asvs-verifier-drift.yml` mirror-decision +step fails the opposite way - `remote_tip` holds the 404 body instead of the empty string, so it +refuses to decide on EVERY run where the mirror branch does not exist, which is the steady state. That +one fails closed and is therefore a dead control rather than a disclosure; it is why the daily drift +job has never completed its decision step. + +**Source:** found 2026-08-09 while sweeping for siblings of the drift-workflow defect, after a peer +correctly refuted my first diagnosis of that job's failure (I said the control "detected drift and +could not act"; the scheduled run predated the drift by 88 minutes and its parity step passed - the +control has never yet detected this class at all). diff --git a/tests/test_dependabot_automerge_guardrails.py b/tests/test_dependabot_automerge_guardrails.py index d3494182..c15e280e 100644 --- a/tests/test_dependabot_automerge_guardrails.py +++ b/tests/test_dependabot_automerge_guardrails.py @@ -456,6 +456,79 @@ def test_release_age_holds_the_version_track_and_undatable_ecosystems( ) +def _gh_stub(tmp_path: Path, count: str | None) -> Path: + """A ``gh`` on PATH that ignores its arguments. + + ``count=None`` reproduces an HTTP error the way the real client does, and that split is the + whole point: **``gh api`` writes the JSON error BODY to stdout** and only its ``gh: ... (HTTP + nnn)`` line to stderr. A stub that merely exits non-zero would pass against the defective code + and prove nothing — the defect was that the body reached the variable. + """ + stub_dir = tmp_path / "ghstub" + stub_dir.mkdir() + stub = stub_dir / "gh" + if count is None: + stub.write_text( + "#!/usr/bin/env bash\n" + 'echo \'{"message":"API rate limit exceeded","status":"403"}\'\n' + "echo 'gh: API rate limit exceeded (HTTP 403)' >&2\n" + "exit 1\n", + encoding="utf-8", + ) + else: + stub.write_text(f"#!/usr/bin/env bash\necho '{count}'\n", encoding="utf-8") + stub.chmod(0o755) + return stub_dir + + +@pytest.mark.skipif( + shutil.which("bash") is None or shutil.which("jq") is None, + reason="needs bash + jq, same runner matrix as the release-age rows below", +) +@pytest.mark.parametrize( + ("label", "count", "expected"), + [ + # The discriminating PASS — without it the suite would be satisfied by a step that denies + # unconditionally, which is the failure mode a fail-closed guard degrades into. + ("one published advisory", "1", "true"), + ("no advisory covers the version", "0", "false"), + # THE REGRESSION ROW. This is the one that was red before the fix: `gh` exits non-zero with + # a JSON error body on STDOUT, `$(cmd || echo ERR)` APPENDED the sentinel to that body, and + # the resulting value was neither "ERR" nor empty. The equality sentinel missed, the numeric + # comparison failed with "integer expression expected" and returned 2, an `if` condition is + # exempt from `set -e`, and the step printed "advisory confirmed" and emitted + # advisory_ok=true while exiting 0. The guard inverted to FAIL-OPEN on exactly the + # rate-limit/API-error class its own header promises routes to manual review. + ("advisory API errors (rate limit)", None, "false"), + ], +) +def test_the_advisory_guard_fails_closed_when_the_api_errors( + label: str, count: str | None, expected: str, tmp_path: Path +) -> None: + """Guardrail #2 EXECUTED, not string-matched. + + The sibling assertion ``"advisory_ok=false" in body`` cannot distinguish a guard that fails + closed from one that merely contains the words — the fail-open above lived underneath a passing + version of exactly that check. Asserting on the emitted OUTPUT is what makes the difference + visible. + """ + rc, out = _run_step_body( + "ghsa", + { + "DEP_GROUP": "pip-security", + "DEPS_JSON": '[{"dependencyName":"requests","prevVersion":"2.19.0"}]', + "GH_TOKEN": "stub", + }, + tmp_path, + path_prepend=_gh_stub(tmp_path, count), + ) + assert rc == 0, f"the ghsa body aborted under `bash -e` (rc={rc}) — CI would fail the step" + assert out.get("security_track") == "true", "the fixture should select the security track" + assert out.get("advisory_ok") == expected, ( + f"{label} -> advisory_ok={out.get('advisory_ok')!r}, expected {expected!r}" + ) + + 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."""