ci(338): distinguish ZAP warning (exit 2) from failure (exit 1) in security-scan

fixes #338

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

docs/ci-cd.md

@@ -566,12 +566,25 @@ running product from outside our C#/review stack.
   several minutes, and is noisy (expect to tune, not take raw). The continuous layer is the per-PR
   white-box gates + the weekly `dependency-scan`; this is the per-release black-box pass. Re-run it each
   release and before any change to the exposure posture.
-- **Triage.** ZAP exits non-zero on any FAIL-level alert; the tooling is noisy, so triage each finding
-  false-positive vs real. Real, in-scope, go-live-blocking findings get fixed (e.g. the security headers
-  from the #319 baseline; the Microsoft.OpenApi pin above); LAN-expected noise (Private-IP disclosure) is
-  revisited only for genuine remote exposure. nuclei (template-based CVE fingerprinting) is an optional
-  third pass — deferred while its template fetch is blocked in the runner env (pre-seed a template volume
-  to add it); ZAP covers the DAST baseline and semgrep the SAST, so it is not on the critical path.
+- **Exit-code contract (ersatztv#338).** `zap-api-scan.py`'s raw exit code is NOT a simple pass/fail — it
+  conflates a clean run with a warnings-only run unless you know its wrapper contract: **0** clean (no
+  FAIL or WARN alerts), **2** WARN-only (triage required, but **not** release-blocking), **1** FAIL (at
+  least one FAIL-level alert — release-blocking), **124** the script's own `timeout` wrapper killed a
+  hung post-scan cleanup (the report written before the hang is still usable — triage it), any other
+  code means the scanner/tool itself errored (not a scan result at all). `scripts/security-scan.sh`
+  encodes this in `classify_zap_exit()` and prints an unambiguous `==> ZAP result: <PASS|WARN|FAIL|
+  TIMEOUT|TOOL ERROR> ...` line; the script's own exit status reflects that classification (0 for
+  clean/WARN, 1 for FAIL/timeout/tool-error) rather than ZAP's raw code, so a warnings-only run no longer
+  reads as a failed scan. Found when the v26.8.0 release scan (#335) returned raw exit 2 for a report with
+  `FAIL-NEW: 0` and two known/expected warning classes — the shell result looked like a failure though the
+  release gate had actually passed. Run `scripts/security-scan.sh --selftest` for a docker-free regression
+  check of the classification logic.
+- **Triage.** Triage each WARN/FAIL finding false-positive vs real. Real, in-scope, go-live-blocking
+  findings get fixed (e.g. the security headers from the #319 baseline; the Microsoft.OpenApi pin above);
+  LAN-expected noise (Private-IP disclosure) is revisited only for genuine remote exposure. nuclei
+  (template-based CVE fingerprinting) is an optional third pass — deferred while its template fetch is
+  blocked in the runner env (pre-seed a template volume to add it); ZAP covers the DAST baseline and
+  semgrep the SAST, so it is not on the critical path.
 
 ## Static analysis & formatting
 

scripts/security-scan.sh

@@ -23,12 +23,60 @@
 #     ETV_SCAN_OUT   host dir for reports      (default: /tmp/etv-scan-out)
 #     ETV_SCAN_SKIP_SEMGREP=1  skip the SAST pass (DAST only)
 #
-# Reports land in $ETV_SCAN_OUT: zap-api-report.{html,md,json}. Exit code is ZAP's:
-# 0 = no FAIL-level alerts, non-zero = at least one FAIL (triage it — this tooling is noisy; expect to
-# tune the ignore list in .zap/ rather than take the raw report at face value).
+# Reports land in $ETV_SCAN_OUT: zap-api-report.{html,md,json}. zap-api-scan.py's exit code is NOT a
+# simple pass/fail — its wrapper contract (ersatztv#338, found via the #335 exact-image release scan
+# returning 2 for a clean-enough report):
+#   0     clean — no FAIL or WARN alerts
+#   2     WARN  — only WARN-level alerts (triage, but NOT release-blocking) — do not fail the job on this
+#   1     FAIL  — at least one FAIL-level alert — release-blocking
+#   124   our own `timeout` wrapper killed a hung wrapper (see the NOTE below) — report is still usable
+#   other the scanner/tool itself errored (bad args, crash) — not a scan result at all
+# classify_zap_exit() (below) is the single place this contract is encoded; the script's own final exit
+# code reflects the CLASSIFICATION (0 for clean/WARN, 1 for FAIL/timeout/tool-error), not ZAP's raw code.
 
 set -euo pipefail
 
+# classify_zap_exit RC — sets ZAP_CLASS_MSG to an unambiguous one-line classification of ZAP's raw exit
+# code and returns the exit status this SCRIPT should use for that outcome (see contract above). Kept as
+# a standalone function (not inlined) so `--selftest` can exercise it without docker/ZAP.
+classify_zap_exit() {
+  local rc="$1"
+  case "$rc" in
+    0)   ZAP_CLASS_MSG="PASS — no FAIL or WARN alerts"; return 0 ;;
+    2)   ZAP_CLASS_MSG="WARN — WARN-level alerts only; triage required, NOT release-blocking"; return 0 ;;
+    1)   ZAP_CLASS_MSG="FAIL — at least one FAIL-level alert; release-blocking, do not ship"; return 1 ;;
+    124) ZAP_CLASS_MSG="TIMEOUT — wrapper hit ETV_SCAN_TIMEOUT; triage the partial report before shipping"; return 1 ;;
+    *)   ZAP_CLASS_MSG="TOOL ERROR (raw exit $rc) — scanner itself failed, this is not a scan result"; return 1 ;;
+  esac
+}
+
+if [ "${1:-}" = "--selftest" ]; then
+  # Lightweight regression check for the classification contract above (ersatztv#338 Done-when) — no
+  # docker/ZAP required, so it can run anywhere (incl. off bumblebee, incl. in per-PR CI if ever added).
+  selftest_failed=0
+  selftest_check() {
+    local rc="$1" want_status="$2" want_substr="$3" got_status
+    if classify_zap_exit "$rc"; then got_status=0; else got_status=$?; fi
+    if [ "$got_status" != "$want_status" ] || [[ "$ZAP_CLASS_MSG" != *"$want_substr"* ]]; then
+      echo "SELFTEST FAIL: rc=$rc -> status=$got_status msg='$ZAP_CLASS_MSG' (want status=$want_status, msg containing '$want_substr')" >&2
+      selftest_failed=1
+    else
+      echo "selftest ok: rc=$rc -> status=$got_status ($ZAP_CLASS_MSG)"
+    fi
+  }
+  selftest_check 0 0 "PASS"
+  selftest_check 2 0 "WARN"
+  selftest_check 1 1 "FAIL"
+  selftest_check 124 1 "TIMEOUT"
+  selftest_check 3 1 "TOOL ERROR"
+  selftest_check 77 1 "TOOL ERROR"
+  if [ "$selftest_failed" = 0 ]; then
+    echo "selftest: all ZAP exit classifications OK"; exit 0
+  else
+    echo "selftest: FAILURES ABOVE" >&2; exit 1
+  fi
+fi
+
 IMAGE="${1:-192.168.1.95:3000/timothy/ersatztv:latest}"
 PORT="${2:-8411}"
 OUT="${ETV_SCAN_OUT:-/tmp/etv-scan-out}"
@@ -87,10 +135,13 @@ timeout "${ETV_SCAN_TIMEOUT:-45m}" docker run --rm --network host -v "$OUT":/zap
   -z "replacer.full_list(0).description=apikey;replacer.full_list(0).enabled=true;replacer.full_list(0).matchtype=REQ_HEADER;replacer.full_list(0).matchstr=X-Api-Key;replacer.full_list(0).regex=false;replacer.full_list(0).replacement=$KEY"
 zap_rc=$?
 [ "$zap_rc" = 124 ] && echo "NOTE: ZAP wrapper hit the ${ETV_SCAN_TIMEOUT:-45m} timeout (usually a post-scan cleanup hang) — the report written before the hang is still valid; triage it."
+classify_zap_exit "$zap_rc"
+zap_class_status=$?
 set -e
 # (The ZAP container + target + temp dir are reaped by cleanup() on the EXIT trap — every path, incl.
 # a `timeout` kill or Ctrl-C mid-scan.)
-echo "==> ZAP report: $OUT/zap-api-report.html (+ .md/.json). Exit code: $zap_rc"
+echo "==> ZAP report: $OUT/zap-api-report.html (+ .md/.json)"
+echo "==> ZAP result: $ZAP_CLASS_MSG (raw exit $zap_rc)"
 
 if [ "${ETV_SCAN_SKIP_SEMGREP:-0}" = "1" ]; then
   echo "==> semgrep SAST skipped (ETV_SCAN_SKIP_SEMGREP=1)"
@@ -104,4 +155,4 @@ else
     "$REPO_ROOT" || echo "(semgrep reported findings — triage in the report above)"
 fi
 
-exit "$zap_rc"
+exit "$zap_class_status"