diff --git a/dependency-vuln-check/README.md b/dependency-vuln-check/README.md index 627024f6..d25e9248 100644 --- a/dependency-vuln-check/README.md +++ b/dependency-vuln-check/README.md @@ -55,6 +55,7 @@ version shows up as `added` in the diff and is evaluated normally. | `base-ref` | yes | — | Base **branch** of the PR (e.g. `main`, `stable/8.8`). Used to find the nearest snapshotted ancestor on the correct branch | | `fallback-base-ref` | no | `main` | Branch to fall back to when `base-ref` has no dependency snapshots (e.g. stacked PRs targeting a feature branch). The gate searches this branch for the nearest snapshotted ancestor of `base-sha` instead of failing closed, and posts a notice to the PR comment | | `snapshot-workflow` | yes | — | Filename of the workflow that submits the base snapshot (e.g. `maven-dependency-snapshot.yml`). Its successful push-event runs are scanned to resolve the effective base | +| `head-snapshot-succeeded` | no | `""` | Whether the job that submits the PR **head** snapshot succeeded. Pass `'true'`/`'success'` (e.g. `needs.pr-maven-snapshot.result == 'success'`). **Any other value** (`'false'`, or a raw result like `'failure'`/`'cancelled'`/`'skipped'`) **fails closed** — an un-submitted head SBOM leaves the head side empty → a real new vuln would silently pass, so an unverified head must block, not no-op. Unset skips the check (backward compatible) | | `max-snapshot-lookback` | no | `30` | How many recent successful snapshot runs to scan when resolving the effective base | | `override-label` | no | `ci:vuln-gate-override` | PR label that bypasses the gate **only** when it cannot verify the PR (outage / no-ancestor). Never bypasses a real finding | | `config-file` | no | `.github/dependency-review-config.json` | Path to the JSON config holding `allow-ghsas` | @@ -101,12 +102,27 @@ The gate **fails closed** when it cannot verify a PR: | API error survives retries | **fail closed** (block), reason named in the log | | No snapshotted ancestor on `base-ref`; `base-ref` differs from `fallback-base-ref` | retry on `fallback-base-ref`; notice posted to PR comment | | No snapshotted ancestor on either branch within `max-snapshot-lookback` | **fail closed** (block) | +| `head-snapshot-succeeded` reported as `'false'` (head SBOM submission failed) | **fail closed** (block) | +| Head side of the diff looks empty (0 added, deps removed) — head SBOM still indexing | re-fetch (3 attempts, backoff), then trust the settled diff | | API works, real vulnerable dependency added | block (normal finding) | | API works, no vulnerable dependency added | pass | Every run writes a summary trail (resolved base, runs scanned, verdict). Failure reasons are named explicitly in the log (rate-limit vs 5xx vs timeout vs permissions vs not-found). +### Head-side verification + +The base side is resolved to a verified snapshotted ancestor, but the head side of the diff is +only as good as the head SBOM. Two guards keep an unverified head from silently passing a real +new vulnerability (a false **negative**): + +- **Submission failure** — the consumer passes `head-snapshot-succeeded`. If the head-SBOM job + did not succeed the head tree is stale/absent, so the gate fails closed rather than trust it. +- **Indexing lag** — a fresh head SBOM can take a few seconds to index. When the head side of + the diff reads as empty (0 added, deps removed) the gate re-fetches the compare a few times to + let the graph settle. A legitimate removal-only PR has the same shape and simply re-confirms — + the gate never *blocks* on this signal, it only waits out indexing lag, so it cannot misfire. + ### Ignoring dependencies the base branch itself added **What this prevents:** a PR being blocked for a vulnerable dependency it never touched, @@ -242,7 +258,9 @@ your-repo/ - For transitive / BOM-pinned dependencies to appear in the diff, both base and head commits need a submitted dependency snapshot (GitHub Dependency Submission API). The base side is resolved automatically to the nearest snapshotted ancestor; the head side - must be submitted by the consuming workflow before this action runs. + must be submitted by the consuming workflow before this action runs. Pass + `head-snapshot-succeeded` so the gate fails closed if that submission failed, and it + re-fetches to absorb indexing lag (see [Head-side verification](#head-side-verification)). ## Tests diff --git a/dependency-vuln-check/action.yml b/dependency-vuln-check/action.yml index a609a258..b23394be 100644 --- a/dependency-vuln-check/action.yml +++ b/dependency-vuln-check/action.yml @@ -27,6 +27,17 @@ inputs: (e.g. maven-dependency-snapshot.yml). Its successful push-event runs are scanned to resolve the effective base. required: true + head-snapshot-succeeded: + description: > + Whether the job that submits the PR head's Maven dependency snapshot + succeeded. Pass 'true'/'success' when it did (e.g. + needs.pr-maven-snapshot.result == 'success'). Any OTHER value — 'false' or a + raw job result like 'failure'/'cancelled'/'skipped' — fails the gate closed, + because an un-submitted head SBOM leaves the head side of the diff empty and + a real new vulnerability would silently pass. Leave unset to skip the check + (backward compatible). + required: false + default: "" fallback-base-ref: description: > Branch to fall back to when base-ref has no dependency snapshots (e.g. @@ -83,6 +94,7 @@ runs: HEAD_SHA: ${{ inputs.head-sha }} BASE_REF: ${{ inputs.base-ref }} SNAPSHOT_WORKFLOW: ${{ inputs.snapshot-workflow }} + HEAD_SNAPSHOT_SUCCEEDED: ${{ inputs.head-snapshot-succeeded }} MAX_SNAPSHOT_LOOKBACK: ${{ inputs.max-snapshot-lookback }} OVERRIDE_LABEL: ${{ inputs.override-label }} CONFIG_FILE: ${{ inputs.config-file }} diff --git a/dependency-vuln-check/check.py b/dependency-vuln-check/check.py index fc791066..74f4e0da 100644 --- a/dependency-vuln-check/check.py +++ b/dependency-vuln-check/check.py @@ -53,6 +53,12 @@ _MAX_ATTEMPTS = 3 _BACKOFF_BASE_SECONDS = 2 +# Head-snapshot indexing can lag a successful submission by a few seconds. When +# the head side of the compare looks empty we re-fetch a few times to let the +# dependency graph settle before trusting the result (see _looks_like_missing_head). +_HEAD_INDEX_MAX_ATTEMPTS = 3 +_HEAD_INDEX_BACKOFF_SECONDS = 5 + # Severity ranking used by both threshold gates. These are the only values the # Dependency Review API emits for `severity`. SEVERITY_ORDER = {"low": 0, "moderate": 1, "high": 2, "critical": 3} @@ -260,6 +266,26 @@ def _dep_triple(dep: dict) -> tuple: return (dep.get("name", ""), dep.get("version", ""), dep.get("manifest", "")) +def _looks_like_missing_head(diff: list) -> bool: + """True when the diff has removed deps but zero added deps. + + After a fresh head-snapshot submission, an all-removed / nothing-added diff + usually means the head SBOM has not finished indexing yet: the head tree + reads as empty against the base, so every base dep shows as ``removed`` and + nothing shows as ``added``. A legitimate removal-only PR has the same shape — + so this signal is used ONLY to wait out indexing lag (re-fetch), never to + block, which keeps it from misfiring on a real removal. + """ + added = removed = 0 + for dep in diff: + change = dep.get("change_type") + if change == "added": + added += 1 + elif change == "removed": + removed += 1 + return added == 0 and removed > 0 + + def _base_branch_pre_existing( repository: str, effective_base: str, snapshot_tip: str | None, base_tip: str | None, token: str, @@ -694,6 +720,11 @@ def main() -> None: base_sha = os.environ["BASE_SHA"] head_sha = os.environ["HEAD_SHA"] base_ref = os.environ["BASE_REF"] + # Result of the consumer's head-SBOM submission job, when reported. Empty/unset + # = a consumer that does not wire it → skip the check (backward compatible with + # existing pins). 'true'/'success' = verified. Any OTHER value fails closed + # (see the guard below) so a wiring mistake can't silently disable it. + head_snapshot_ok = os.environ.get("HEAD_SNAPSHOT_SUCCEEDED", "").strip().lower() snapshot_workflow = os.environ["SNAPSHOT_WORKFLOW"] lookback = int(os.environ.get("MAX_SNAPSHOT_LOOKBACK", "30")) override_label = os.environ.get("OVERRIDE_LABEL", "ci:vuln-gate-override") @@ -770,19 +801,53 @@ def main() -> None: f"(scanned {scanned} run(s); snapshot run {run_id})" ) - # ── Dependency diff against the resolved base ── - try: - diff = _api_get( - f"{_GITHUB_API}/repos/{repository}/dependency-graph/compare/{effective_base}...{head_sha}", - token, - ) - except ApiError as e: + # ── Head snapshot must have been submitted successfully ── + # The consumer reports the result of its head-SBOM submission job. If it did + # not succeed, the head side of the diff is stale/absent and a genuinely new + # vulnerable dependency would silently pass — so fail closed rather than trust + # an unverifiable head. Only 'true'/'success' (or unset, for consumers that do + # not wire it) proceed; ANY other value — 'false', a raw job result like + # 'failure'/'cancelled'/'skipped', or a typo — fails closed, so a wiring + # mistake cannot silently turn this guard into a no-op. + if head_snapshot_ok not in ("", "true", "success"): fail_closed( repository, pr_number, token, override_label, - f"dependency review API failed ({e.reason})", summary_path, + f"head dependency snapshot did not succeed (submission result: '{head_snapshot_ok}') " + "— head dependencies cannot be verified", + summary_path, ) return + # ── Dependency diff against the resolved base ── + # The head SBOM can still be indexing for a few seconds after a successful + # submission. If the head side looks empty (0 added, deps removed) we re-fetch + # a few times to let the graph settle before trusting it. A removal-only PR + # shows the same shape and simply re-confirms — we never block on this signal, + # only wait out indexing lag, so it cannot misfire on a legitimate removal. + diff = None + for attempt in range(1, _HEAD_INDEX_MAX_ATTEMPTS + 1): + try: + diff = _api_get( + f"{_GITHUB_API}/repos/{repository}/dependency-graph/compare/{effective_base}...{head_sha}", + token, + ) + except ApiError as e: + fail_closed( + repository, pr_number, token, override_label, + f"dependency review API failed ({e.reason})", summary_path, + ) + return + if attempt == _HEAD_INDEX_MAX_ATTEMPTS or not _looks_like_missing_head(diff): + break + delay = _HEAD_INDEX_BACKOFF_SECONDS * attempt + print( + f"::notice::Head side of the dependency diff looks empty " + f"(0 added, {sum(1 for d in diff if d.get('change_type') == 'removed')} removed) — " + f"the head snapshot may still be indexing; re-fetching in {delay}s " + f"(attempt {attempt}/{_HEAD_INDEX_MAX_ATTEMPTS})" + ) + time.sleep(delay) + # ── Pre-existing dep filter: suppress base-branch-added deps (Workstream F) ── # Find deps the base branch itself added after our snapshot — those are # pre-existing on the branch, not PR-introduced. We diff effective_base against diff --git a/dependency-vuln-check/test_check.py b/dependency-vuln-check/test_check.py index 96b019af..bc7495f4 100644 --- a/dependency-vuln-check/test_check.py +++ b/dependency-vuln-check/test_check.py @@ -367,6 +367,7 @@ def _set_main_env(monkeypatch): monkeypatch.delenv("GITHUB_STEP_SUMMARY", raising=False) monkeypatch.delenv("GITHUB_EVENT_PATH", raising=False) monkeypatch.delenv("CONFIG_FILE", raising=False) + monkeypatch.delenv("HEAD_SNAPSHOT_SUCCEEDED", raising=False) def test_main_real_vuln_blocks_even_with_override_label(monkeypatch): @@ -391,6 +392,146 @@ def test_main_fail_closed_when_no_ancestor(monkeypatch): assert ei.value.code == 1 +# ── Head-snapshot guard: _looks_like_missing_head helper ── + + +def test_looks_like_missing_head_all_removed(): + # 0 added, some removed → head reads as empty (indexing-lag signature). + assert check._looks_like_missing_head([_dep(change_type="removed")]) is True + + +def test_looks_like_missing_head_has_added(): + # Any added dep means the head is populated — not the missing-head shape. + assert check._looks_like_missing_head( + [_dep(change_type="removed"), _dep(change_type="added")] + ) is False + + +def test_looks_like_missing_head_empty_diff(): + # A no-op PR (nothing added or removed) is not the missing-head shape. + assert check._looks_like_missing_head([]) is False + + +# ── Head-snapshot guard: gap 1 (submission-failed → fail closed) ── + + +def test_main_fail_closed_when_head_snapshot_failed(monkeypatch): + # Consumer reports the head-SBOM submission failed → the head side is + # unverifiable → fail closed (exit 1), regardless of the (empty) diff. + _set_main_env(monkeypatch) + monkeypatch.setenv("HEAD_SNAPSHOT_SUCCEEDED", "false") + monkeypatch.setattr(check, "latest_snapshotted_ancestor", lambda *a, **k: ("eff", 1, 1, "latest")) + monkeypatch.setattr(check, "has_override_label", lambda *a, **k: False) + monkeypatch.setattr(check, "_pr_number", lambda: None) + with pytest.raises(SystemExit) as ei: + check.main() + assert ei.value.code == 1 + + +def test_main_head_snapshot_failed_bypassed_by_override(monkeypatch): + # Same as above but the override label is present → the unverifiable PR is + # allowed through (exit 0), consistent with the other fail-closed paths. + _set_main_env(monkeypatch) + monkeypatch.setenv("HEAD_SNAPSHOT_SUCCEEDED", "false") + monkeypatch.setattr(check, "latest_snapshotted_ancestor", lambda *a, **k: ("eff", 1, 1, "latest")) + monkeypatch.setattr(check, "has_override_label", lambda *a, **k: True) + monkeypatch.setattr(check, "_pr_number", lambda: None) + with pytest.raises(SystemExit) as ei: + check.main() + assert ei.value.code == 0 + + +@pytest.mark.parametrize("value", ["failure", "cancelled", "skipped", "False", "oops"]) +def test_main_fail_closed_on_unexpected_head_snapshot_value(monkeypatch, value): + # A wiring mistake (raw job result or typo) must fail closed, not silently + # no-op the guard. Only 'true'/'success'/unset proceed. + _set_main_env(monkeypatch) + monkeypatch.setenv("HEAD_SNAPSHOT_SUCCEEDED", value) + monkeypatch.setattr(check, "latest_snapshotted_ancestor", lambda *a, **k: ("eff", 1, 1, "latest")) + monkeypatch.setattr(check, "has_override_label", lambda *a, **k: False) + monkeypatch.setattr(check, "_pr_number", lambda: None) + with pytest.raises(SystemExit) as ei: + check.main() + assert ei.value.code == 1 + + +@pytest.mark.parametrize("value", ["success", "SUCCESS", "true"]) +def test_main_head_snapshot_recognized_ok_values_proceed(monkeypatch, value): + # Both the documented boolean ('true') and a raw successful result ('success') + # proceed; matching is case-insensitive. + _set_main_env(monkeypatch) + monkeypatch.setenv("HEAD_SNAPSHOT_SUCCEEDED", value) + monkeypatch.setattr(check, "latest_snapshotted_ancestor", lambda *a, **k: ("eff", 1, 1, "latest")) + monkeypatch.setattr(check, "_base_branch_pre_existing", lambda *a, **k: set()) + monkeypatch.setattr(check, "_api_get", lambda url, tok: [_dep(change_type="added", severity="low", fix=None)]) + monkeypatch.setattr(check, "_pr_number", lambda: None) + check.main() # clean/low → no SystemExit + + +def test_main_head_snapshot_true_proceeds(monkeypatch): + # Submission succeeded and the diff is clean → no block (main returns). + _set_main_env(monkeypatch) + monkeypatch.setenv("HEAD_SNAPSHOT_SUCCEEDED", "true") + monkeypatch.setattr(check, "latest_snapshotted_ancestor", lambda *a, **k: ("eff", 1, 1, "latest")) + monkeypatch.setattr(check, "_base_branch_pre_existing", lambda *a, **k: set()) + monkeypatch.setattr(check, "_api_get", lambda url, tok: [_dep(change_type="added", severity="low", fix=None)]) + monkeypatch.setattr(check, "_pr_number", lambda: None) + check.main() # low/no-fix does not block → no SystemExit + + +# ── Head-snapshot guard: gap 2 (indexing-lag retry, never misfires) ── + + +def test_main_retries_when_head_appears_unindexed(monkeypatch): + # First fetch: head looks empty (all removed) → indexing still settling. + # Second fetch: settled, a real fixable vuln is added → blocks on the final + # result. Confirms we re-fetch and trust the settled diff. + _set_main_env(monkeypatch) + monkeypatch.setenv("HEAD_SNAPSHOT_SUCCEEDED", "true") + monkeypatch.setattr(check, "latest_snapshotted_ancestor", lambda *a, **k: ("eff", 1, 1, "latest")) + monkeypatch.setattr(check, "_base_branch_pre_existing", lambda *a, **k: set()) + monkeypatch.setattr(check, "has_override_label", lambda *a, **k: False) + monkeypatch.setattr(check, "_pr_number", lambda: None) + sleeps = [] + monkeypatch.setattr(check.time, "sleep", lambda s: sleeps.append(s)) + responses = [[_dep(change_type="removed")], [_dep(severity="high", fix="2.0.0")]] + calls = {"n": 0} + + def fake_get(url, tok): + i = calls["n"] + calls["n"] += 1 + return responses[min(i, len(responses) - 1)] + + monkeypatch.setattr(check, "_api_get", fake_get) + with pytest.raises(SystemExit) as ei: + check.main() + assert ei.value.code == 1 # settled diff has the real vuln + assert calls["n"] == 2 # re-fetched once + assert sleeps # waited for indexing + + +def test_main_persistent_empty_head_never_blocks(monkeypatch): + # Head stays all-removed across every retry. A legitimate removal-only PR + # looks identical, so the gate must NOT block on this signal — it exhausts + # the retries and proceeds (0 blocking). Guards against a misfire FP. + _set_main_env(monkeypatch) + monkeypatch.setenv("HEAD_SNAPSHOT_SUCCEEDED", "true") + monkeypatch.setattr(check, "latest_snapshotted_ancestor", lambda *a, **k: ("eff", 1, 1, "latest")) + monkeypatch.setattr(check, "_base_branch_pre_existing", lambda *a, **k: set()) + monkeypatch.setattr(check, "has_override_label", lambda *a, **k: False) + monkeypatch.setattr(check, "_pr_number", lambda: None) + monkeypatch.setattr(check.time, "sleep", lambda s: None) + calls = {"n": 0} + + def fake_get(url, tok): + calls["n"] += 1 + return [_dep(change_type="removed")] + + monkeypatch.setattr(check, "_api_get", fake_get) + check.main() # must NOT raise SystemExit(1) + assert calls["n"] == check._HEAD_INDEX_MAX_ATTEMPTS # exhausted retries, then proceeded + + def test_stacked_pr_falls_back_to_default_branch(monkeypatch): # base_ref targets a feature branch (no snapshots); fallback_ref (main) resolves. _set_main_env(monkeypatch)