From 5434868bd029c7b8f7ea2a520bfef5a16f748175 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:13:31 +0100 Subject: [PATCH] Bound the npm audit retry budget by wall clock The retry hardening added in #1885 bounded attempts but not time. npm's own fetch-timeout defaults to five minutes and it retries internally, so three "attempts" against a hanging advisory endpoint ran for 10m56s on job 100986651307, and a second audit step added 3m36s. The Frontend job was cancelled 31s into type-check with all 1183 test files already passing, and a cancelled job reports as a failed required check, so a green run blocked every pull request. #1888 raised the job timeout to 40 minutes to unblock delivery; this decides the policy instead. Each attempt now runs under a hard wall-clock bound and the sequence stops at a total deadline (60s and 240s by default). npm's internal retry loop is disabled in favour of this one, since it was the hidden multiplier. The bound is enforced by a watchdog subshell rather than timeout(1), which is not present on every developer machine. What happens when the endpoint stays unreachable is unchanged, because that split was already right: the run fails when the change touches the dependency graph and the answer is genuinely unknown, and warns without failing when it does not, because the graph is then identical to a base commit that already produced a passing answer. Any advisory at any severity still fails. Also drops the production-only audit from the per-pull-request path. It audits a subset of the same packages, so it reports a subset of the same advisories, and because the complete audit fails the job on any finding, the production step could only ever execute in the cases where it was already guaranteed clean. The dev-versus-production split still runs for every npm workspace in the scheduled security-scan job, where it informs rather than blocks delivery, and Dependabot security updates remain the route for advisories published against unchanged dependencies. With the audit bounded to 4 minutes against an ~11 minute baseline, the job timeout returns to 30: a stalled endpoint should surface as a warning, not be absorbed by a budget large enough to hide it. --- .github/workflows/build-and-test.yml | 23 +++-- .../subsystems/deployment-installability.md | 34 +++++-- .../installtests/build_release_assets_test.go | 30 +++++- scripts/npm-audit-retry.sh | 92 ++++++++++++++++++- scripts/tests/test-npm-audit-retry.sh | 50 ++++++++++ 5 files changed, 205 insertions(+), 24 deletions(-) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 662fc1308..e622b6a6c 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -121,10 +121,11 @@ jobs: needs: changes if: needs.changes.outputs.code == 'true' runs-on: ubuntu-24.04 - # The two npm audit steps retry through registry outages (they burned - # 14m34s on 2026-09-04), which no longer fits a 25 minute budget: the job - # was cancelled mid type-check with all 1183 test files already passing. - timeout-minutes: 40 + # Everything except the audit runs in ~11m. The audit is now bounded to a + # 4 minute wall-clock budget, so 30 leaves real headroom while staying a + # meaningful ceiling: an unbounded 40 would have absorbed the 2026-09-04 + # stall instead of reporting it. + timeout-minutes: 30 env: FRONTEND_DIR: frontend-modern @@ -148,18 +149,20 @@ jobs: # 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. + # + # Only the complete graph is audited here. `--omit=dev` audits a subset + # of these packages, so it can only report a subset of these advisories; + # and because the complete audit fails the job on any finding, the + # production step could only ever run in the cases where it was already + # guaranteed clean. The dev-versus-production split is still reported + # for every workspace by the scheduled npm-audit job in + # security-scan.yml, where it informs rather than blocks delivery. - name: Audit complete frontend dependency graph working-directory: frontend-modern 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 - 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 # the backstop that keeps `make format` a no-op on a clean tree. diff --git a/docs/release-control/v6/internal/subsystems/deployment-installability.md b/docs/release-control/v6/internal/subsystems/deployment-installability.md index c3eaa1afc..b18d3e9a8 100644 --- a/docs/release-control/v6/internal/subsystems/deployment-installability.md +++ b/docs/release-control/v6/internal/subsystems/deployment-installability.md @@ -4002,26 +4002,44 @@ resolved package version and integrity that the release build will actually consume. 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, through `scripts/npm-audit-retry.sh`. That runner exists because +`.github/workflows/build-and-test.yml` frontend job must run the complete +`npm audit` after a clean install, through `scripts/npm-audit-retry.sh`. The +production-only `npm audit --omit=dev` is deliberately not on that +per-pull-request path: it audits a subset of the same packages, so it can only +report a subset of the same advisories, and because the complete audit fails +the job on any finding, the production step could only ever execute in the +cases where it was already guaranteed clean. The dev-versus-production split +is reported instead by the scheduled `npm-audit` job in +`.github/workflows/security-scan.yml`, which covers every npm workspace and +informs rather than blocks delivery. 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 +retried. Retrying is bounded by wall clock and not by attempt count alone, +because npm's own `fetch-timeout` defaults to five minutes and it retries +internally: on 2026-09-04 three attempts against a hanging endpoint ran for +10m56s and cancelled the Frontend job at its own timeout with every test +already passing, so a green run was reported as a failed required check. Each +attempt is therefore bounded, npm's internal retry loop is disabled in favour +of the runner's own, and the sequence stops at a total deadline. That budget +must stay well inside the job timeout; it may never be raised to the point +where an unreachable endpoint can consume the job. When retries are exhausted, +by attempt count or by budget, 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 +unchanged dependencies are the responsibility of Dependabot security updates +and the scheduled scan, 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` +is active, that an unparseable or unrecognised report is never read as +clean, and that neither a hung attempt nor an exhausted budget can outlive +its bound. `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 65d25c4b9..257fa2d50 100644 --- a/scripts/installtests/build_release_assets_test.go +++ b/scripts/installtests/build_release_assets_test.go @@ -3644,14 +3644,20 @@ func TestFrontendDependencySecurityAuditsAreRequired(t *testing.T) { assertFileContainsAll(t, workflowPath, `- name: Audit complete frontend dependency graph`, `npm-audit-retry.sh" all`, - `- name: Audit production frontend dependencies`, - `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 production-only audit reports a subset of the complete audit's + // advisories and cannot gate anything the complete audit did not already + // fail on, so it is off the per-pull-request path. It must still run + // somewhere: the scheduled scan owns the dev-versus-production split. + assertFileContainsAll(t, repoFile(".github", "workflows", "security-scan.yml"), + `- name: Audit production dependencies`, + `npm audit --package-lock-only --omit=dev`, + ) // 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. @@ -3666,6 +3672,26 @@ func TestFrontendDependencySecurityAuditsAreRequired(t *testing.T) { assertFileContainsAll(t, runnerPath, `print("vulnerable" if total else "clean")`, ) + // Retrying must be bounded by wall clock, not by attempt count alone. + // npm's own fetch-timeout defaults to five minutes and it retries + // internally, so three unbounded attempts once ran for 10m56s and + // cancelled the Frontend job with every test already passing. + assertFileContainsAll(t, runnerPath, + `NPM_AUDIT_MAX_SECONDS`, + `NPM_AUDIT_ATTEMPT_TIMEOUT`, + `export npm_config_fetch_retries=0`, + `DEADLINE=`, + ) + // The job budget has to stay above the audit budget by a wide margin, or + // a stalled endpoint reappears as a cancelled job rather than a warning. + workflow, err := os.ReadFile(workflowPath) + if err != nil { + t.Fatalf("read %s: %v", workflowPath, err) + } + frontendJob := workflowJobBlock(t, string(workflow), "frontend") + if !strings.Contains(frontendJob, "timeout-minutes: 30") { + t.Fatal("frontend job must keep a bounded timeout above the audit budget") + } } func TestReleaseCutGatesCriticalFrontendAndWindowsRuntimeProof(t *testing.T) { diff --git a/scripts/npm-audit-retry.sh b/scripts/npm-audit-retry.sh index a28cef045..8530b4054 100755 --- a/scripts/npm-audit-retry.sh +++ b/scripts/npm-audit-retry.sh @@ -20,6 +20,16 @@ # change touches the dependency graph (NPM_AUDIT_REQUIRE_RESULT=true) and # warns without failing when it does not. # +# The retry budget is wall-clock, not just an attempt count, because attempt +# count alone does not bound anything: npm's own `fetch-timeout` defaults to +# five minutes and it retries internally, so a single `npm audit` against a +# hanging endpoint can sit for minutes before this script sees a verdict. On +# 2026-09-04 that produced a 10m56s audit step (two 5m00s attempts, then a +# 9s success) and cancelled the Frontend job at its 25m limit with every test +# already passing — a green run reported as a failed required check. So each +# attempt is bounded, npm's internal retry loop is disabled in favour of this +# one, and the whole sequence stops at a deadline. +# # 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 @@ -32,6 +42,9 @@ # 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_ATTEMPT_TIMEOUT seconds one npm invocation may run (default 60) +# NPM_AUDIT_MAX_SECONDS total wall-clock budget for all attempts +# (default 240) # 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) @@ -50,9 +63,52 @@ esac ATTEMPTS="${NPM_AUDIT_ATTEMPTS:-3}" DELAY="${NPM_AUDIT_RETRY_DELAY:-15}" +ATTEMPT_TIMEOUT="${NPM_AUDIT_ATTEMPT_TIMEOUT:-60}" +MAX_SECONDS="${NPM_AUDIT_MAX_SECONDS:-240}" REQUIRE_RESULT="${NPM_AUDIT_REQUIRE_RESULT:-true}" NPM_BIN="${NPM_AUDIT_CMD:-npm}" +# This script is the retry layer. npm's own fetch retry loop would multiply +# every attempt by an unbounded amount of hidden waiting, which is exactly +# what made a bounded-looking three attempts run for eleven minutes. +export npm_config_fetch_retries=0 +export npm_config_fetch_timeout=$((ATTEMPT_TIMEOUT * 1000)) + +DEADLINE=$(( $(date +%s) + MAX_SECONDS )) + +# Run one audit under a hard wall-clock bound, portably: `timeout` is not +# present on every developer machine, so a watchdog subshell kills the npm +# process if it outlives the limit. Blocking on `wait` for the real child +# avoids the zombie-liveness race that a `kill -0` poll would hit. +run_audit() { + local limit="$1" out="$2" + + : >"${out}" + "${NPM_BIN}" audit --json "${SCOPE_ARGS[@]}" >"${out}" 2>/dev/null & + local npm_pid=$! + + ( + sleep "${limit}" + kill -TERM "${npm_pid}" 2>/dev/null + sleep 2 + kill -KILL "${npm_pid}" 2>/dev/null + ) >/dev/null 2>&1 & + local killer_pid=$! + + wait "${npm_pid}" 2>/dev/null + local status=$? + + kill -TERM "${killer_pid}" 2>/dev/null + wait "${killer_pid}" 2>/dev/null + + # 143 = SIGTERM, 137 = SIGKILL: the watchdog fired. The report is then + # empty or truncated, which classify_report already reads as unreachable. + if [ "${status}" -eq 143 ] || [ "${status}" -eq 137 ]; then + return 124 + fi + return 0 +} + # Classify one audit run. Prints a verdict word on stdout: # clean — audit completed, no vulnerabilities # vulnerable — audit completed, vulnerabilities present @@ -100,9 +156,25 @@ trap 'rm -f "${report_file}"' EXIT attempt=1 delay="${DELAY}" +budget_exhausted=false while [ "${attempt}" -le "${ATTEMPTS}" ]; do - echo "npm audit (${SCOPE}) attempt ${attempt}/${ATTEMPTS}" - "${NPM_BIN}" audit --json "${SCOPE_ARGS[@]}" >"${report_file}" 2>/dev/null + remaining=$(( DEADLINE - $(date +%s) )) + if [ "${remaining}" -le 0 ]; then + echo "npm audit (${SCOPE}): ${MAX_SECONDS}s retry budget exhausted before attempt ${attempt}" + budget_exhausted=true + break + fi + + # Never let one attempt outlive the overall budget. + attempt_limit="${ATTEMPT_TIMEOUT}" + if [ "${attempt_limit}" -gt "${remaining}" ]; then + attempt_limit="${remaining}" + fi + + echo "npm audit (${SCOPE}) attempt ${attempt}/${ATTEMPTS} (limit ${attempt_limit}s, ${remaining}s of budget left)" + if ! run_audit "${attempt_limit}" "${report_file}"; then + echo "npm audit (${SCOPE}): attempt ${attempt} exceeded ${attempt_limit}s and was stopped" + fi verdict_output="$(classify_report <"${report_file}")" verdict="$(printf '%s\n' "${verdict_output}" | head -1)" summary="$(printf '%s\n' "${verdict_output}" | sed -n '2p')" @@ -126,6 +198,12 @@ while [ "${attempt}" -le "${ATTEMPTS}" ]; do esac if [ "${attempt}" -lt "${ATTEMPTS}" ]; then + remaining=$(( DEADLINE - $(date +%s) )) + if [ "${delay}" -ge "${remaining}" ]; then + echo "npm audit (${SCOPE}): ${MAX_SECONDS}s retry budget exhausted" + budget_exhausted=true + break + fi echo "retrying in ${delay}s" sleep "${delay}" delay=$((delay * 2)) @@ -133,10 +211,16 @@ while [ "${attempt}" -le "${ATTEMPTS}" ]; do attempt=$((attempt + 1)) done +if [ "${budget_exhausted}" = "true" ]; then + gave_up="within its ${MAX_SECONDS}s retry budget" +else + gave_up="after ${ATTEMPTS} attempts" +fi + 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." + echo "::error::npm audit (${SCOPE}) could not reach the advisory endpoint ${gave_up}, 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." +echo "::warning::npm audit (${SCOPE}) could not reach the advisory endpoint ${gave_up}. 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 index df7da2a33..e005e7e9d 100755 --- a/scripts/tests/test-npm-audit-retry.sh +++ b/scripts/tests/test-npm-audit-retry.sh @@ -52,6 +52,8 @@ run_case() { out="$(NPM_AUDIT_CMD="${npm_bin}" \ NPM_AUDIT_RETRY_DELAY=0 \ NPM_AUDIT_ATTEMPTS="${NPM_AUDIT_ATTEMPTS:-3}" \ + NPM_AUDIT_ATTEMPT_TIMEOUT="${NPM_AUDIT_ATTEMPT_TIMEOUT:-60}" \ + NPM_AUDIT_MAX_SECONDS="${NPM_AUDIT_MAX_SECONDS:-240}" \ NPM_AUDIT_REQUIRE_RESULT="${require}" \ bash "${SCRIPT}" all 2>&1)" status=$? @@ -117,6 +119,54 @@ run_case "unknown payload shape is not treated as clean" 1 \ "$(make_fake_npm npm-shape '{"metadata":{}}')" true \ "could not reach the advisory endpoint" +# A hung endpoint must be cut off per attempt rather than inheriting npm's +# own five-minute fetch timeout. This is the regression that cancelled the +# Frontend job on 2026-09-04: three "attempts" ran for eleven minutes. +hanging_npm="${WORK_DIR}/npm-hang" +cat > "${hanging_npm}" <<'SH' +#!/usr/bin/env bash +sleep 300 +SH +chmod +x "${hanging_npm}" + +started=$(date +%s) +NPM_AUDIT_ATTEMPT_TIMEOUT=1 NPM_AUDIT_MAX_SECONDS=10 \ + run_case "a hung audit is stopped at the per-attempt limit" 1 \ + "${hanging_npm}" true "was stopped" "could not reach the advisory endpoint" +elapsed=$(( $(date +%s) - started )) +if [ "${elapsed}" -gt 30 ]; then + echo "FAIL: hung audit took ${elapsed}s; the per-attempt limit did not bound it" + failures=$((failures + 1)) +else + echo "ok: hung audit bounded in ${elapsed}s" +fi + +# The total budget, not just the attempt count, has to end the sequence, and +# an exhausted budget must still respect the fail-closed/fail-open split. +started=$(date +%s) +NPM_AUDIT_ATTEMPTS=50 NPM_AUDIT_ATTEMPT_TIMEOUT=1 NPM_AUDIT_MAX_SECONDS=3 \ + run_case "the wall-clock budget ends the retry sequence" 1 \ + "${hanging_npm}" true "retry budget exhausted" +elapsed=$(( $(date +%s) - started )) +if [ "${elapsed}" -gt 25 ]; then + echo "FAIL: 50 attempts under a 3s budget took ${elapsed}s; the budget did not bound them" + failures=$((failures + 1)) +else + echo "ok: wall-clock budget bounded 50 attempts in ${elapsed}s" +fi + +# An exhausted budget is still tolerated when the dependency graph is unchanged. +NPM_AUDIT_ATTEMPTS=50 NPM_AUDIT_ATTEMPT_TIMEOUT=1 NPM_AUDIT_MAX_SECONDS=3 \ + run_case "an exhausted budget warns when dependencies unchanged" 0 \ + "${hanging_npm}" false "::warning::" "retry budget" + +# A real advisory must still fail even under a tight budget: the bound may +# only ever change what happens to an unreachable endpoint. +NPM_AUDIT_ATTEMPT_TIMEOUT=1 NPM_AUDIT_MAX_SECONDS=3 \ + run_case "a vulnerability still fails under a tight budget" 1 \ + "$(make_fake_npm npm-vuln-budget "${VULN}")" false \ + "vulnerabilities present" + if [ "${failures}" -ne 0 ]; then echo "${failures} test(s) failed" exit 1