From a290f22f2565ff7361edd54a55c93aff00052620 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 9 Aug 2026 17:53:31 -0500 Subject: [PATCH] fix(ci): the advisory guard fails OPEN on an API error -- move the || outside (#1209) Guardrail #2 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 (only its "gh: ... (HTTP nnn)" line goes to stderr, which 2>/dev/null eats). So count held '{"message":"API rate limit exceeded",...}ERR'. The sentinel missed, being neither "ERR" nor empty. The numeric comparison then failed with "integer expression expected" and returned 2 -- and an `if` CONDITION is exempt from set -e. The step printed "published advisory confirmed", wrote advisory_ok=true, and exited 0. The guard inverted to fail-open on exactly the rate-limit/API-error class its own header promises routes to manual review. The comment directly above it says "Fail closed on any error": a compensating control resting on a false premise. Measured against the real shipped body, origin/main vs this branch, 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 <- closed, happy path intact A stub that merely exited non-zero would have passed against the defective code and proved nothing; the defect is that the body reached the variable, so the stub must write a body. Bounded today, and the bound is worth stating exactly rather than leaning on: the merge condition also requires age_ok, and the age step denies every ecosystem that can be eligible -- a disjointness this file documents about itself, and labels a FORWARD guard load-bearing the day a Python allow row is populated. One line's edit from directly merge-affecting. THE EXISTING TEST COULD NOT SEE IT. It asserted the STRING "advisory_ok=false" appears in the step body -- satisfied by a step that merely contains the words, and the fail-open lived underneath a passing version of 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. Now three executed rows, including a discriminating PASS so the suite cannot be satisfied by unconditional denial. DOMAIN, because fixing one instance is how this class survives: a sweep of 63 workflow and script files across both repos found 24 instances, 16 provably harmless (git rev-parse --verify --quiet writes nothing on failure). All six in this file are fixed. Moving the || outside 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. The count guard also moves from equality against one sentinel to a SHAPE test. An equality test recognises exactly the failure it was told about, which is how a JSON body walked through it. --- .github/workflows/dependabot-auto-merge.yml | 46 +++++++++--- docs/BACKLOG.md | 70 ++++++++++++++++++ tests/test_dependabot_automerge_guardrails.py | 73 +++++++++++++++++++ 3 files changed, 178 insertions(+), 11 deletions(-) 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."""