|
| 1 | +# SPDX-License-Identifier: AGPL-3.0-or-later |
| 2 | +# Copyright (C) 2026 MessageFoundry Organization and contributors |
| 3 | +"""``npm audit`` must retry a transport failure, and must never pass without a verdict. |
| 4 | +
|
| 5 | +THE DEFECT THIS EXISTS FOR. ``npm audit`` exits non-zero BOTH when the advisory database reports a |
| 6 | +vulnerability AND when it cannot be reached, so the bare ``npm audit --package-lock-only`` this |
| 7 | +replaced made an unreachable registry indistinguishable from a finding. A red therefore said nothing |
| 8 | +about the dependencies, which is a compensating control resting on a false premise. Measured |
| 9 | +2026-09-04: of 34 failures of the required ``npm-audit (ide dependency vulnerabilities)`` context in |
| 10 | +one night, 32 were ``503 Service Unavailable`` or ``network timeout at .../advisories/bulk`` and 2 |
| 11 | +were real findings. Each one blocked a merge or evicted a merge-queue entry. |
| 12 | +
|
| 13 | +WHAT IS PINNED, AND WHY IT IS THIS AND NOT A SIGNATURE. The retry is the convenience; the property |
| 14 | +that must not regress is that the step stays **fail-closed**. So the load-bearing assertion is not |
| 15 | +"the word retry appears" but that **every** path reaching ``exit 0`` is inside the branch where |
| 16 | +``npm audit`` itself succeeded. A future edit that adds a friendly "registry unreachable, carrying |
| 17 | +on" fall-through would keep every other marker in place and silently turn a required security gate |
| 18 | +into one that passes when it learned nothing. That edit is what this module is here to catch. |
| 19 | +
|
| 20 | +WHAT CANNOT BE TESTED HERE, stated rather than papered over. Whether ``npm audit --json`` really |
| 21 | +emits ``.metadata.vulnerabilities`` on a verdict, and really omits it on a transport error, is a |
| 22 | +property of npm and is not exercised by this module -- it needs a runner with a network. That |
| 23 | +assumption is deliberately arranged so being WRONG about it is safe: if the discriminator never |
| 24 | +matches, a real finding is treated as "no verdict", the retries are exhausted, and the job FAILS. |
| 25 | +The assumption can cost a false red. It cannot produce a false green, which is what the |
| 26 | +``exit 0`` assertion below pins. |
| 27 | +""" |
| 28 | + |
| 29 | +from __future__ import annotations |
| 30 | + |
| 31 | +import re |
| 32 | +from pathlib import Path |
| 33 | + |
| 34 | +import pytest |
| 35 | + |
| 36 | +yaml = pytest.importorskip("yaml") |
| 37 | + |
| 38 | +_REPO = Path(__file__).resolve().parents[1] |
| 39 | +_SECURITY = _REPO / ".github" / "workflows" / "security.yml" |
| 40 | + |
| 41 | +_STEP_NAME = "Audit the locked npm dependencies (install-free)" |
| 42 | + |
| 43 | + |
| 44 | +def _audit_step() -> dict[str, object]: |
| 45 | + """The npm-audit step, located by name rather than by index.""" |
| 46 | + doc = yaml.safe_load(_SECURITY.read_text(encoding="utf-8")) |
| 47 | + steps = [ |
| 48 | + step |
| 49 | + for job in doc["jobs"].values() |
| 50 | + for step in (job.get("steps") or []) |
| 51 | + if isinstance(step, dict) and step.get("name") == _STEP_NAME |
| 52 | + ] |
| 53 | + assert len(steps) == 1, f"expected exactly one {_STEP_NAME!r} step, found {len(steps)}" |
| 54 | + return steps[0] |
| 55 | + |
| 56 | + |
| 57 | +def _run_lines() -> list[str]: |
| 58 | + run = _audit_step().get("run") |
| 59 | + assert isinstance(run, str), "the npm-audit step must carry a multi-line run block" |
| 60 | + return run.strip().splitlines() |
| 61 | + |
| 62 | + |
| 63 | +def test_the_audit_step_retries_rather_than_reporting_a_transport_error_as_a_finding() -> None: |
| 64 | + lines = _run_lines() |
| 65 | + body = "\n".join(lines) |
| 66 | + assert "for attempt in" in body, ( |
| 67 | + "the npm-audit step no longer loops. A single invocation cannot tell an unreachable " |
| 68 | + "registry from a vulnerability, which is the defect this step was changed to fix." |
| 69 | + ) |
| 70 | + assert "sleep" in body, ( |
| 71 | + "a retry loop with no backoff hammers a registry that is already failing" |
| 72 | + ) |
| 73 | + |
| 74 | + |
| 75 | +def test_the_step_discriminates_a_verdict_from_a_transport_failure() -> None: |
| 76 | + body = "\n".join(_run_lines()) |
| 77 | + assert ".metadata.vulnerabilities" in body, ( |
| 78 | + "the step must decide 'verdict or transport failure' on the CONTENT of the audit output. " |
| 79 | + "Exit status alone cannot separate them -- that is the whole defect." |
| 80 | + ) |
| 81 | + |
| 82 | + |
| 83 | +def test_the_only_way_to_pass_is_npm_audit_itself_succeeding() -> None: |
| 84 | + """The fail-closed property. This is the assertion that matters. |
| 85 | +
|
| 86 | + Every ``exit 0`` must sit inside the ``if npm audit ...; then`` branch. Anything else is a path |
| 87 | + that reports success without an audit verdict. |
| 88 | + """ |
| 89 | + lines = _run_lines() |
| 90 | + exits = [i for i, line in enumerate(lines) if re.match(r"\s*exit\s+0\b", line)] |
| 91 | + assert exits, "the step can never succeed; it has no `exit 0` at all" |
| 92 | + assert len(exits) == 1, ( |
| 93 | + f"expected exactly one `exit 0`, found {len(exits)} at lines " |
| 94 | + f"{[i + 1 for i in exits]}. Every additional success path is a way to pass without a " |
| 95 | + "verdict, so they are counted rather than assumed benign." |
| 96 | + ) |
| 97 | + |
| 98 | + # Walk backwards to the ENCLOSING `if`, tracking `fi` so a block that has already closed cannot |
| 99 | + # lend its condition to a later line. An earlier version of this test searched the whole preface |
| 100 | + # for `if npm audit`, which every later line trivially satisfies -- it passed a deliberate |
| 101 | + # fail-open mutation and so proved nothing. |
| 102 | + idx = exits[0] |
| 103 | + depth = 0 |
| 104 | + opener = None |
| 105 | + for line in reversed(lines[:idx]): |
| 106 | + stripped = line.strip() |
| 107 | + if stripped == "fi": |
| 108 | + depth += 1 |
| 109 | + elif re.match(r"if\s+.*;\s*then$", stripped): |
| 110 | + if depth == 0: |
| 111 | + opener = stripped |
| 112 | + break |
| 113 | + depth -= 1 |
| 114 | + assert opener is not None, ( |
| 115 | + f"the `exit 0` on line {idx + 1} sits in no `if` block at all, so it is reached " |
| 116 | + "unconditionally once control arrives there." |
| 117 | + ) |
| 118 | + assert re.match(r"if\s+npm audit\b", opener), ( |
| 119 | + f"the `exit 0` on line {idx + 1} is guarded by {opener!r}, not by `npm audit` succeeding. " |
| 120 | + "A required security gate must not report success when it obtained no verdict -- see this " |
| 121 | + "module's docstring." |
| 122 | + ) |
| 123 | + |
| 124 | + |
| 125 | +def test_exhausting_the_retries_fails_rather_than_passing() -> None: |
| 126 | + lines = _run_lines() |
| 127 | + tail = "\n".join(lines[-6:]) |
| 128 | + assert re.search(r"^\s*exit\s+1\b", tail, re.M), ( |
| 129 | + "after the retry loop the step must exit non-zero. Falling out of the loop into a success " |
| 130 | + "is exactly the fail-open this step exists to avoid." |
| 131 | + ) |
| 132 | + |
| 133 | + |
| 134 | +def test_the_step_pins_bash_so_the_loop_semantics_are_not_the_runner_default() -> None: |
| 135 | + assert _audit_step().get("shell") == "bash", ( |
| 136 | + "the run block relies on bash loop and test semantics; leaving the shell implicit lets a " |
| 137 | + "runner default change them underneath it" |
| 138 | + ) |
0 commit comments