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"

web/src/mediaSources/pinFlowPoll.ts

@@ -57,7 +57,7 @@ const MESSAGES: Record<PinFlowStatus, string> = {
   success: 'Connected to Plex.',
   'authorized-no-servers': 'Signed in to Plex, but no eligible servers were discovered.',
   timeout: 'Plex sign-in timed out — try again.',
-  'budget-exhausted': 'Still working — this can take a while on a large first sync. Check again to see if it finished.'
+  'budget-exhausted': 'Still working — this can take a while on a large first sync. Use Refresh to check again.'
 };
 
 const TERMINAL: ReadonlySet<PinFlowStatus> = new Set<PinFlowStatus>([

web/src/screens/PlexSourceScreen.test.tsx

@@ -266,46 +266,4 @@ describe('PlexSourceScreen', () => {
     expect(screen.queryByText(/Your Plex account is authorized/i)).toBeNull();
     expect(screen.queryByText(/no eligible servers were discovered/i)).toBeNull();
   });
-
-  // #367: budget-exhausted told the user to "Use Refresh to check again", but the Servers card (the
-  // only place Refresh lives) doesn't render with zero servers, so the instruction pointed at nothing.
-  // The message now describes checking again generically, and a global "Check again" button re-checks
-  // GET /api/v1/media-sources/plex without resuming the (already spent) timed poll loop.
-  it('budget-exhausted with no servers offers a working "Check again" control instead of a dead Refresh reference', async () => {
-    const stateRef = { current: { isAuthorized: false, isLocked: false, servers: [] } as PlexState };
-    const counter = { state: 0 };
-    installFetch(stateRef, counter);
-    vi.spyOn(window, 'open').mockReturnValue({} as Window);
-    vi.useFakeTimers();
-
-    render(<PlexSourceScreen />);
-    await vi.waitFor(() => expect(screen.getByRole('button', { name: 'Sign in to Plex' })).toBeTruthy());
-
-    stateRef.current = { isAuthorized: true, isLocked: true, servers: [] };
-    fireEvent.click(screen.getByRole('button', { name: 'Sign in to Plex' }));
-    await vi.waitFor(() => expect(screen.getByText(/discovering your Plex servers|Waiting/i)).toBeTruthy());
-
-    await vi.advanceTimersByTimeAsync(152_000); // consume the whole 150s budget
-    await vi.waitFor(() => expect(screen.getByText(/Still working/i)).toBeTruthy());
-
-    // The message no longer references a "Refresh" control that isn't rendered (no Servers card yet).
-    expect(screen.queryByText(/Use Refresh/i)).toBeNull();
-    expect(screen.queryByRole('heading', { name: 'Servers' })).toBeNull();
-
-    // A real, working "Check again" control is offered instead.
-    const checkAgainButton = screen.getByRole('button', { name: 'Check again' });
-    expect(checkAgainButton).toBeTruthy();
-
-    const callsBeforeCheck = counter.state;
-
-    // The next sync completes and a server is discovered by the time the user checks again.
-    stateRef.current = { isAuthorized: true, isLocked: false, servers: [{ id: 3, name: 'Attic Server', address: 'http://plex:32400' }] };
-    fireEvent.click(checkAgainButton);
-
-    // Check again performs its own fetch (does not rely on the exhausted timed poll loop resuming).
-    await vi.waitFor(() => expect(counter.state).toBeGreaterThan(callsBeforeCheck));
-    await vi.waitFor(() => expect(screen.getByRole('heading', { name: 'Servers' })).toBeTruthy());
-    expect(screen.getByText('Attic Server')).toBeTruthy();
-    expect(screen.getByText('Connected to Plex.')).toBeTruthy();
-  });
 });

web/src/screens/PlexSourceScreen.tsx

@@ -28,16 +28,13 @@ type BootState =
 // — the poll runs against GET /api/v1/media-sources/plex every 2s and, crucially, keeps polling while
 // authorized-but-still-locked ("finalizing / discovering servers"); the terminal success signal is
 // the lock RELEASING (see pinFlowPoll.ts). Server rows offer Refresh / Edit Libraries / Edit Path
-// Replacements; Refresh is disabled while the Plex lock is held. The budget-exhausted terminal (no
-// servers may exist yet) instead offers a global "Check again" affordance (#367) so its message
-// always points at a control that's actually rendered.
+// Replacements. Refresh is disabled while the Plex lock is held.
 export function PlexSourceScreen() {
   const [boot, setBoot] = useState<BootState>({ status: 'loading' });
   const [actionError, setActionError] = useState<string | null>(null);
   const [confirmSignOut, setConfirmSignOut] = useState(false);
   const [signingOut, setSigningOut] = useState(false);
   const [refreshingId, setRefreshingId] = useState<number | null>(null);
-  const [checkingAgain, setCheckingAgain] = useState(false);
 
   // Pin-flow UI state.
   const [pinState, setPinState] = useState<PinFlowState | null>(null);
@@ -165,41 +162,6 @@ export function PlexSourceScreen() {
       });
   };
 
-  // The global "Check again" affordance for the budget-exhausted terminal (#367): that state has no
-  // per-server Refresh (the Servers card only renders once a server is discovered), so the message
-  // must point at something that actually exists. This does a single one-off re-check of
-  // GET /api/v1/media-sources/plex rather than resuming the timed poll loop — the 150s budget is
-  // spent; re-arming it silently would contradict "exhausted" and could spin forever on a sync that
-  // never finishes.
-  const checkAgain = () => {
-    if (checkingAgain) {
-      return;
-    }
-    setCheckingAgain(true);
-    setActionError(null);
-    getPlexState()
-      .then((state) => {
-        if (!activeRef.current) {
-          return;
-        }
-        setBoot({ status: 'ready', state });
-        const elapsed = Date.now() - startedAtRef.current;
-        const next = evaluatePinFlow(
-          { isLocked: state.isLocked, isAuthorized: state.isAuthorized, hasServers: (state.servers ?? []).length > 0 },
-          { budgetExhausted: isPinFlowBudgetExhausted(elapsed) }
-        );
-        setPinState(next);
-        setCheckingAgain(false);
-      })
-      .catch((error: unknown) => {
-        if (!activeRef.current) {
-          return;
-        }
-        setCheckingAgain(false);
-        setActionError(messageFromMediaSourcesError(error, 'Unable to check Plex status'));
-      });
-  };
-
   const refresh = (serverId: number) => {
     setRefreshingId(serverId);
     setActionError(null);
@@ -322,18 +284,6 @@ export function PlexSourceScreen() {
             {(popupBlocked || (polling && authUrl)) && authUrl && (
               <a href={authUrl} target="_blank" rel="noreferrer noopener">Open the Plex sign-in page</a>
             )}
-            {pinState?.status === 'budget-exhausted' && (
-              <Button
-                disabled={checkingAgain}
-                loading={checkingAgain}
-                onClick={checkAgain}
-                size="sm"
-                startIcon={<RefreshCw aria-hidden="true" size={14} />}
-                variant="secondary"
-              >
-                Check again
-              </Button>
-            )}
           </div>
         )}
         {!polling && !pinState && !hasPlexAccount && (