From c01d152a35ccaf1761a4ac93ef788232649fc609 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Fri, 4 Sep 2026 08:06:35 +0100 Subject: [PATCH] Separate a real advisory from an unreachable npm audit endpoint On 2026-09-03 registry.npmjs.org returned 503s and then timeouts from its bulk advisory endpoint for over an hour. `npm audit` exits non-zero both for a real advisory and for an endpoint it cannot reach, so the Frontend job failed four times running and, because it is a required check, no pull request could land at all - including Go-only ones that touch no JavaScript. Every one of those failures was the outage. In two of the runs the other audit call in the same job succeeded and reported zero vulnerabilities. The audits now run through scripts/npm-audit-retry.sh, which separates the two cases and does nothing else. A conclusive result is acted on immediately, and the gate stays exactly as strict as before: any vulnerability at any severity still fails, and no severity threshold is introduced. Only an unreachable endpoint is retried, with backoff. When retries are exhausted the outcome depends on whether the answer is actually unknown. A change that touches frontend-modern/package.json, frontend-modern/package-lock.json, or the runner itself fails, because the dependency graph moved and no result means no answer. A change that touches none of them warns and continues, because the graph is then identical to the base commit that already produced a passing answer. Advisories published later against unchanged dependencies are what Dependabot security updates are for, not a per-pull-request audit. Deliberately not done: relaxing the severity threshold. That was my first instinct, but the evidence does not support it. None of the four failures was an advisory, the lockfile reports zero vulnerabilities at every severity, and a threshold would have weakened the gate without fixing anything. The contract's rule that audit suppression is not a valid closure stands. The pinning test now requires the runner's invocation and the dependency detection wiring, and additionally asserts the runner carries no --audit-level flag, so the strictness cannot be quietly traded away later. --- .github/workflows/build-and-test.yml | 26 +++- .../subsystems/deployment-installability.md | 19 ++- .../installtests/build_release_assets_test.go | 23 ++- scripts/npm-audit-retry.sh | 142 ++++++++++++++++++ scripts/tests/test-npm-audit-retry.sh | 124 +++++++++++++++ 5 files changed, 328 insertions(+), 6 deletions(-) create mode 100755 scripts/npm-audit-retry.sh create mode 100755 scripts/tests/test-npm-audit-retry.sh diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 70c20b572..62c2bfdf2 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -71,6 +71,7 @@ jobs: timeout-minutes: 5 outputs: code: ${{ steps.filter.outputs.code }} + frontend_deps: ${{ steps.filter.outputs.frontend_deps }} steps: - name: Checkout repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -88,8 +89,9 @@ jobs: base="${{ github.event.before }}" fi if [ -z "$base" ] || [ "$base" = "0000000000000000000000000000000000000000" ] || ! git cat-file -e "$base" 2>/dev/null; then - echo "Base commit unavailable; assuming code changed." + echo "Base commit unavailable; assuming code and dependencies changed." echo "code=true" >> "$GITHUB_OUTPUT" + echo "frontend_deps=true" >> "$GITHUB_OUTPUT" exit 0 fi changed=$(git diff --name-only "$base" "${{ github.sha }}") @@ -102,6 +104,17 @@ jobs: code=false fi echo "code=$code" >> "$GITHUB_OUTPUT" + # Whether this change moves the frontend dependency graph. It decides + # what an unreachable npm advisory endpoint means: unknown answer + # (must block) versus the base commit's already-passing answer. A + # change to the audit runner itself also demands a real result, so it + # can never be relaxed under cover of its own tolerant mode. + if printf '%s\n' "$changed" | grep -qE '^frontend-modern/package(-lock)?\.json$|^scripts/npm-audit-retry\.sh$'; then + frontend_deps=true + else + frontend_deps=false + fi + echo "frontend_deps=$frontend_deps" >> "$GITHUB_OUTPUT" frontend: name: Frontend @@ -129,13 +142,20 @@ jobs: working-directory: frontend-modern run: npm ci + # npm audit exits 1 both for a real advisory and for an unreachable + # advisory endpoint. The runner keeps the advisory verdict exactly as + # strict and only retries the endpoint being down; see the script header. - name: Audit complete frontend dependency graph working-directory: frontend-modern - run: npm audit + env: + NPM_AUDIT_REQUIRE_RESULT: ${{ needs.changes.outputs.frontend_deps }} + run: bash "$GITHUB_WORKSPACE/scripts/npm-audit-retry.sh" all - name: Audit production frontend dependencies working-directory: frontend-modern - run: npm audit --omit=dev + env: + NPM_AUDIT_REQUIRE_RESULT: ${{ needs.changes.outputs.frontend_deps }} + run: bash "$GITHUB_WORKSPACE/scripts/npm-audit-retry.sh" production # Whole-tree, not staged-only: the pre-commit formatter only ever sees # staged files, so drift in untouched files is invisible to it. This is diff --git a/docs/release-control/v6/internal/subsystems/deployment-installability.md b/docs/release-control/v6/internal/subsystems/deployment-installability.md index 2ad16bccf..c3eaa1afc 100644 --- a/docs/release-control/v6/internal/subsystems/deployment-installability.md +++ b/docs/release-control/v6/internal/subsystems/deployment-installability.md @@ -4004,7 +4004,24 @@ Frontend dependency-security changes use their own proof route rather than borrowing the local dev-runtime orchestration tests. The canonical `.github/workflows/build-and-test.yml` frontend job must run both the complete `npm audit` and the production-only `npm audit --omit=dev` after a clean -install. `frontend-modern/src/security/__tests__/dependencySecurity.test.ts` +install, through `scripts/npm-audit-retry.sh`. That runner exists because +`npm audit` exits non-zero both for a real advisory and for an unreachable +advisory endpoint: on 2026-09-03 registry.npmjs.org returned 503s and timeouts +for over an hour and no pull request could land, including changes that touch +no JavaScript. It separates the two and nothing else. A conclusive result is +acted on immediately and any vulnerability at any severity still fails, so a +severity threshold must never be introduced; only an unreachable endpoint is +retried. When retries are exhausted, the run fails if the change touches +`frontend-modern/package.json`, `frontend-modern/package-lock.json`, or the +runner itself, because then the answer is genuinely unknown and the runner may +never be relaxed under cover of its own tolerant mode, and warns without +failing when it does not, because the dependency graph is then identical to the base commit that +already produced a passing answer. Advisories published later against +unchanged dependencies are the responsibility of Dependabot security updates, +not of a per-pull-request audit. `scripts/tests/test-npm-audit-retry.sh` pins +that split, including that a real advisory fails even when the tolerant mode +is active and that an unparseable or unrecognised report is never read as +clean. `frontend-modern/src/security/__tests__/dependencySecurity.test.ts` pins the known safe floors for advisories remediated by commit `6ba85a185`, including DOMPurify `GHSA-55q2-fjhq-7xh7`, brace-expansion `GHSA-mh99-v99m-4gvg` and `GHSA-rgw5-rvv9-x895`, and nanoid diff --git a/scripts/installtests/build_release_assets_test.go b/scripts/installtests/build_release_assets_test.go index ccc452c0f..65d25c4b9 100644 --- a/scripts/installtests/build_release_assets_test.go +++ b/scripts/installtests/build_release_assets_test.go @@ -3643,9 +3643,28 @@ func TestFrontendDependencySecurityAuditsAreRequired(t *testing.T) { workflowPath := repoFile(".github", "workflows", "build-and-test.yml") assertFileContainsAll(t, workflowPath, `- name: Audit complete frontend dependency graph`, - `run: npm audit`, + `npm-audit-retry.sh" all`, `- name: Audit production frontend dependencies`, - `run: npm audit --omit=dev`, + `npm-audit-retry.sh" production`, + // The runner may retry an unreachable advisory endpoint, but only a + // change that leaves the dependency graph untouched may proceed + // without a fresh result. + `NPM_AUDIT_REQUIRE_RESULT: ${{ needs.changes.outputs.frontend_deps }}`, + `frontend_deps: ${{ steps.filter.outputs.frontend_deps }}`, + ) + // The gate itself must stay strict. An unreachable endpoint may be + // retried, but no severity threshold may be introduced that lets a real + // advisory through, and any vulnerability total must still fail. + runnerPath := repoFile("scripts", "npm-audit-retry.sh") + runner, err := os.ReadFile(runnerPath) + if err != nil { + t.Fatalf("read %s: %v", runnerPath, err) + } + if strings.Contains(string(runner), "--audit-level") { + t.Fatalf("%s must not weaken the audit with a severity threshold", runnerPath) + } + assertFileContainsAll(t, runnerPath, + `print("vulnerable" if total else "clean")`, ) } diff --git a/scripts/npm-audit-retry.sh b/scripts/npm-audit-retry.sh new file mode 100755 index 000000000..a28cef045 --- /dev/null +++ b/scripts/npm-audit-retry.sh @@ -0,0 +1,142 @@ +#!/usr/bin/env bash +# npm-audit-retry.sh — Run npm audit, separating a real advisory from an +# unreachable advisory endpoint. +# +# Usage: scripts/npm-audit-retry.sh +# +# `npm audit` exits 1 both when it finds vulnerabilities and when it cannot +# reach registry.npmjs.org. Treating those the same made a required check +# depend on npm's availability: on 2026-09-03 the advisory bulk endpoint +# returned 503s and timeouts for over an hour and no pull request could land, +# including Go-only ones. Four consecutive failures, zero advisories. +# +# This keeps the gate exactly as strict about advisories — any vulnerability at +# any severity still fails, and suppression is never a valid closure — and +# changes only what happens when npm cannot answer: +# +# * a conclusive answer is acted on immediately, pass or fail; +# * an unreachable endpoint is retried with backoff; +# * if it is still unreachable after every attempt, the run fails when this +# change touches the dependency graph (NPM_AUDIT_REQUIRE_RESULT=true) and +# warns without failing when it does not. +# +# That last split is the whole safety argument. When package.json and +# package-lock.json are untouched, the audit answer for this change is the one +# the base commit already produced, so skipping it adds no risk from this +# change; advisories published later against unchanged dependencies are caught +# by Dependabot security updates, not by a per-pull-request audit. When the +# dependency graph does move, the answer is unknown and only then does an +# unreachable endpoint have to block. +# +# Env: +# NPM_AUDIT_ATTEMPTS attempts before giving up (default 3) +# NPM_AUDIT_RETRY_DELAY seconds before the first retry, doubled each +# time (default 15) +# NPM_AUDIT_REQUIRE_RESULT "true" to fail when no answer was obtained +# (default true — the safe default) +# NPM_AUDIT_CMD npm executable to invoke (test seam) + +set -uo pipefail + +SCOPE="${1:-}" +case "${SCOPE}" in + all) SCOPE_ARGS=() ;; + production) SCOPE_ARGS=(--omit=dev) ;; + *) + echo "Usage: $0 " >&2 + exit 2 + ;; +esac + +ATTEMPTS="${NPM_AUDIT_ATTEMPTS:-3}" +DELAY="${NPM_AUDIT_RETRY_DELAY:-15}" +REQUIRE_RESULT="${NPM_AUDIT_REQUIRE_RESULT:-true}" +NPM_BIN="${NPM_AUDIT_CMD:-npm}" + +# Classify one audit run. Prints a verdict word on stdout: +# clean — audit completed, no vulnerabilities +# vulnerable — audit completed, vulnerabilities present +# unreachable — npm could not get an answer from the advisory endpoint +classify_report() { + python3 -c ' +import json, sys + +raw = sys.stdin.read().strip() +if not raw: + print("unreachable") + sys.exit(0) +try: + report = json.loads(raw) +except ValueError: + print("unreachable") + sys.exit(0) + +# npm reports an unusable audit endpoint as an error object, ENOAUDIT being +# the code it uses for 5xx, timeouts and offline runs alike. +if isinstance(report, dict) and report.get("error"): + print("unreachable") + sys.exit(0) + +meta = report.get("metadata") if isinstance(report, dict) else None +vulns = meta.get("vulnerabilities") if isinstance(meta, dict) else None +if not isinstance(vulns, dict) or "total" not in vulns: + # No usable verdict in the payload: treat as unreachable rather than + # silently passing on a shape we do not understand. + print("unreachable") + sys.exit(0) + +total = vulns.get("total", 0) +detail = " ".join( + f"{name}={vulns.get(name, 0)}" + for name in ("critical", "high", "moderate", "low", "info") +) +print("vulnerable" if total else "clean") +print(f"total={total} {detail}") +' +} + +report_file="$(mktemp)" +trap 'rm -f "${report_file}"' EXIT + +attempt=1 +delay="${DELAY}" +while [ "${attempt}" -le "${ATTEMPTS}" ]; do + echo "npm audit (${SCOPE}) attempt ${attempt}/${ATTEMPTS}" + "${NPM_BIN}" audit --json "${SCOPE_ARGS[@]}" >"${report_file}" 2>/dev/null + verdict_output="$(classify_report <"${report_file}")" + verdict="$(printf '%s\n' "${verdict_output}" | head -1)" + summary="$(printf '%s\n' "${verdict_output}" | sed -n '2p')" + + case "${verdict}" in + clean) + echo "npm audit (${SCOPE}): no vulnerabilities (${summary})" + exit 0 + ;; + vulnerable) + echo "npm audit (${SCOPE}): vulnerabilities present (${summary})" + echo "::error::npm audit (${SCOPE}) found vulnerabilities: ${summary}" + # Re-run without --json so the log carries the human-readable advisory + # detail a maintainer needs to act on. + "${NPM_BIN}" audit "${SCOPE_ARGS[@]}" || true + exit 1 + ;; + *) + echo "npm audit (${SCOPE}): advisory endpoint did not return a usable result" + ;; + esac + + if [ "${attempt}" -lt "${ATTEMPTS}" ]; then + echo "retrying in ${delay}s" + sleep "${delay}" + delay=$((delay * 2)) + fi + attempt=$((attempt + 1)) +done + +if [ "${REQUIRE_RESULT}" = "true" ]; then + echo "::error::npm audit (${SCOPE}) could not reach the advisory endpoint after ${ATTEMPTS} attempts, and this change touches the dependency graph, so the result cannot be assumed." + exit 1 +fi + +echo "::warning::npm audit (${SCOPE}) could not reach the advisory endpoint after ${ATTEMPTS} attempts. This change does not touch package.json or package-lock.json, so the dependency graph is identical to the base commit that already passed; continuing without a fresh result." +exit 0 diff --git a/scripts/tests/test-npm-audit-retry.sh b/scripts/tests/test-npm-audit-retry.sh new file mode 100755 index 000000000..df7da2a33 --- /dev/null +++ b/scripts/tests/test-npm-audit-retry.sh @@ -0,0 +1,124 @@ +#!/usr/bin/env bash +# +# Smoke tests for scripts/npm-audit-retry.sh — the gate must stay exactly as +# strict about advisories and only tolerate an unreachable endpoint. + +set -euo pipefail + +ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +SCRIPT="${ROOT_DIR}/scripts/npm-audit-retry.sh" + +WORK_DIR="$(mktemp -d)" +trap 'rm -rf "${WORK_DIR}"' EXIT + +failures=0 + +# Build a fake npm that emits a canned payload per invocation. Each line of +# the mode list is used for one successive call, so retry behaviour is +# observable. +make_fake_npm() { + local name="$1" + shift + local path="${WORK_DIR}/${name}" + { + printf '#!/usr/bin/env bash\n' + printf 'count_file="%s/${0##*/}.count"\n' "${WORK_DIR}" + printf 'n=$(cat "$count_file" 2>/dev/null || echo 0)\n' + printf 'n=$((n + 1))\n' + printf 'printf "%%s" "$n" > "$count_file"\n' + printf 'case "$n" in\n' + local i=1 + for payload in "$@"; do + printf ' %d) cat <<'"'"'JSON'"'"'\n%s\nJSON\n ;;\n' "${i}" "${payload}" + i=$((i + 1)) + done + printf ' *) cat <<'"'"'JSON'"'"'\n%s\nJSON\n ;;\n' "${!#}" + printf 'esac\n' + printf 'exit 1\n' + } > "${path}" + chmod +x "${path}" + printf '%s' "${path}" +} + +CLEAN='{"metadata":{"vulnerabilities":{"info":0,"low":0,"moderate":0,"high":0,"critical":0,"total":0}}}' +VULN='{"metadata":{"vulnerabilities":{"info":0,"low":0,"moderate":1,"high":0,"critical":0,"total":1}}}' +ENOAUDIT='{"error":{"code":"ENOAUDIT","summary":"503 Service Unavailable","detail":""}}' + +run_case() { + local desc="$1" expected_status="$2" npm_bin="$3" require="$4" + shift 4 + local out status + set +e + out="$(NPM_AUDIT_CMD="${npm_bin}" \ + NPM_AUDIT_RETRY_DELAY=0 \ + NPM_AUDIT_ATTEMPTS="${NPM_AUDIT_ATTEMPTS:-3}" \ + NPM_AUDIT_REQUIRE_RESULT="${require}" \ + bash "${SCRIPT}" all 2>&1)" + status=$? + set -e + if [ "${status}" != "${expected_status}" ]; then + echo "FAIL: ${desc} — exit ${status}, want ${expected_status}" + printf '%s\n' "${out}" | sed 's/^/ /' + failures=$((failures + 1)) + return + fi + for needle in "$@"; do + if ! printf '%s' "${out}" | grep -qF -- "${needle}"; then + echo "FAIL: ${desc} — output missing: ${needle}" + printf '%s\n' "${out}" | sed 's/^/ /' + failures=$((failures + 1)) + return + fi + done + echo "ok: ${desc}" +} + +# A clean audit passes on the first attempt. +run_case "clean audit passes" 0 "$(make_fake_npm npm-clean "${CLEAN}")" true \ + "no vulnerabilities" + +# A vulnerability fails, and must fail even when the dependency graph is +# untouched — the outage tolerance must never soften a real finding. +run_case "vulnerability fails" 1 "$(make_fake_npm npm-vuln "${VULN}")" false \ + "vulnerabilities present" + +# A transient endpoint failure that clears on retry passes. +run_case "retry recovers from a transient outage" 0 \ + "$(make_fake_npm npm-recover "${ENOAUDIT}" "${CLEAN}")" true \ + "did not return a usable result" "no vulnerabilities" + +# A vulnerability found only after a transient failure still fails. +run_case "retry surfacing a vulnerability fails" 1 \ + "$(make_fake_npm npm-late-vuln "${ENOAUDIT}" "${VULN}")" true \ + "vulnerabilities present" + +# A sustained outage fails when this change touches the dependency graph. +run_case "sustained outage fails when dependencies changed" 1 \ + "$(make_fake_npm npm-out-required "${ENOAUDIT}")" true \ + "could not reach the advisory endpoint" + +# A sustained outage warns but passes when the dependency graph is unchanged. +run_case "sustained outage warns when dependencies unchanged" 0 \ + "$(make_fake_npm npm-out-optional "${ENOAUDIT}")" false \ + "::warning::" "does not touch package.json" + +# Unparseable output is treated as unreachable, never as a pass. +run_case "garbage output is not treated as clean" 1 \ + "$(make_fake_npm npm-garbage "not json at all")" true \ + "could not reach the advisory endpoint" + +# An empty report is likewise not a pass. +run_case "empty output is not treated as clean" 1 \ + "$(make_fake_npm npm-empty "")" true \ + "could not reach the advisory endpoint" + +# An unknown payload shape must not pass silently. +run_case "unknown payload shape is not treated as clean" 1 \ + "$(make_fake_npm npm-shape '{"metadata":{}}')" true \ + "could not reach the advisory endpoint" + +if [ "${failures}" -ne 0 ]; then + echo "${failures} test(s) failed" + exit 1 +fi +echo "all npm-audit-retry tests passed"