CVE detection engine — correctness fixes, spec, tests, audit #2

Merged
autonomic-bot merged 10 commits from review/1-cve-engine into review/session-base 2026-08-11 19:03:53 +00:00
Owner

Ten commits hardening advisory-scan.py, each driven by a wrong number it actually produced.

  • spurious ? — URL punctuation captured from markdown, a stale registry URL, and "no upgrade" semantics.
  • the false 133 — discourse counted 34 redis CVEs against its own window; each image is now judged by its OWN version range, and semver→calver jumps fall back to advisory publish dates resolved from git tag dates.
  • UNKNOWN, never 0 — a 0 in a security column asserts safety; the scan now refuses to emit a number it cannot justify.
  • pagination — the advisories endpoint ignores ?page= and silently re-serves page 1; discourse has 286 advisories, so one page cannot even cover a single window.
  • sidecar CVEs counted via --image NAME=FROM:TO, which is what surfaced discourse's critical CVE-2025-49844 (redis) — previously invisible.
  • advisory-scan.SPEC.md — step-by-step spec, written so the reasoning survives me.
  • 58 offline tests + 6 live regressions, plus audit-advisory-scan.py which re-derives every count with a separate semver implementation. Both found real bugs: ragged tuple comparison (a fix in 18.0 fell outside a window ending at 18 — bare major tags are the norm for sidecars), and advisories with no knowable fix version being silently counted as "not fixed".

Retrospective review PR. These commits were pushed straight to main during the 2026-08-11 session; they are already live. This PR exists so the work is reviewable and commentable as a unit — its base is a branch pinned at the pre-session commit, so merging it simply advances that pointer and changes nothing on main. Going forward, orchestrator changes go through a PR before landing.

Ten commits hardening `advisory-scan.py`, each driven by a wrong number it actually produced. - **spurious `?`** — URL punctuation captured from markdown, a stale registry URL, and "no upgrade" semantics. - **the false 133** — discourse counted 34 redis CVEs against its own window; each image is now judged by its OWN version range, and semver→calver jumps fall back to advisory publish dates resolved from git tag dates. - **`UNKNOWN`, never `0`** — a `0` in a security column asserts safety; the scan now refuses to emit a number it cannot justify. - **pagination** — the advisories endpoint ignores `?page=` and silently re-serves page 1; discourse has 286 advisories, so one page cannot even cover a single window. - **sidecar CVEs counted** via `--image NAME=FROM:TO`, which is what surfaced discourse's **critical** `CVE-2025-49844` (redis) — previously invisible. - **`advisory-scan.SPEC.md`** — step-by-step spec, written so the reasoning survives me. - **58 offline tests + 6 live regressions**, plus `audit-advisory-scan.py` which re-derives every count with a *separate* semver implementation. Both found real bugs: ragged tuple comparison (a fix in `18.0` fell outside a window ending at `18` — bare major tags are the norm for sidecars), and advisories with no knowable fix version being silently counted as "not fixed". --- *Retrospective review PR.* These commits were pushed straight to `main` during the 2026-08-11 session; they are already live. This PR exists so the work is reviewable and commentable as a unit — its base is a branch pinned at the pre-session commit, so merging it simply advances that pointer and changes nothing on `main`. Going forward, orchestrator changes go through a PR before landing.
autonomic-bot added 10 commits 2026-08-11 19:03:38 +00:00
The 2026-08-07 regeneration rendered '?' for 5 of 21 recipes. '?' is meant to be a
rare 'we tried and could not tell'; at that rate it is indistinguishable from noise
and hides the real unknowns. Three causes, none of them genuine uncertainty:

1. URL EXTRACTION BUG (mine). The registry is markdown, so urls appear inside
   `backticks` and 'quotes'. The extractor captured the trailing punctuation, so
   it fetched https://docs.n8n.io/release-notes/` and https://git.autonomic.zone'`
   — both 404 on the malformed url, both 200 when clean. Trailing markdown
   punctuation is now stripped. Fixed immich + n8n.
2. STALE REGISTRY URL. mattermost-lts pointed at
   docs.mattermost.com/about/mattermost-changelog.html, which 404s; the page moved
   to /deploy/. Corrected (same class as the pgautoupgrade fix).
3. WRONG SEMANTICS FOR 'NO UPGRADE'. lasuite-docs and custom-html-tiny were
   up-to-date this run, so no scan block existed and the report fell back to '?'.
   But a recipe with no upgrade has nothing an upgrade could have fixed — that is
   0, not unknown. The report skill now says so explicitly, restricts '?' to scans
   that RAN and reported genuinely failed sources, states that benign notes
   (no-advisories-published / template URL) never trigger '?', and instructs that
   many '?' is itself a bug to raise in the Addendum.

Result across all 16 scanned recipes of that run: 0 failed sources (was 5).
Counts also improved with the classifier fix: discourse 130->133, keycloak ->7.
Operator disbelieved discourse's '133 CVEs fixed' — correctly. Two defects made it
confidently wrong:

1. ONE WINDOW APPLIED TO EVERY IMAGE. The scan queries all source repos in the
   recipe's registry (app + redis/postgres/nginx sidecars) but judged them all with
   the APP's version window. 34 of the 133 were redis advisories, including
   CVE-2021-21309 — patched in redis 6.0.11 back in 2021 — scored as 'fixed by this
   upgrade' purely because 6.0.11 sits numerically inside discourse's 3.5.3 ->
   2026.7.1 range. Only the PRIMARY app repo is now classified; other sources are
   reported as unclassified so they stay visible without inflating the count.

2. VERSION-SCHEME CHANGES BREAK ORDERING. discourse moved semver -> calver
   (3.5.3 -> 2026.7.1), so 2025.12.2 compares 'newer' than 3.5.3 while shipping
   earlier. Numeric comparison cannot order that. The scan now detects a leading-
   component jump >= 100, refuses to classify, and says so in the block: the count
   is '0 by refusal, not by evidence — read the vendor's release notes'.

Refusing to answer beats answering wrongly: a fabricated 133 in a public security
report is worse than an explicit 'cannot determine'.

Verified after the fix: discourse 133 -> 0 (with the refusal caveat), gitea still
exactly 2 (both criticals, patched 1.27.1), keycloak 7 all genuinely from
keycloak/keycloak patched in 26.7.1, plausible 1. No other count changed.
Operator: 'the scanner should not say 0 when it was not able to scan.' Correct — the
previous patch still led with '0 identified' and relegated the caveat to a footnote,
so the headline number was wrong even though the prose was right. A 0 in a security
column is an assertion of safety; it must never be emitted for an undetermined result.

Now: cve_count_fixed is null (not 0) in JSON, a count_known flag distinguishes
'counted zero' from 'could not count', and the markdown headline reads
'CVEs fixed by this upgrade: UNKNOWN — the scan could NOT determine a count' with an
explicit 'This is NOT zero' and instructions to render '?'.

Verified: discourse 3.5.3 -> 2026.7.1 (semver->calver) now reports UNKNOWN; gitea
1.27.0 -> 1.27.1 still reports 2.
Answers 'how can the weekly run produce counts like the hand count?' — by doing
exactly what the hand count did, deterministically. Two changes:

1. PAGINATION. The scanner requested per_page=100 and stopped. This endpoint caps at
   100 AND ignores ?page= (it re-returns the same rows — which is how a manual count
   first produced exact triplicates and a bogus 300). Busy projects were silently
   truncated: discourse has 286 advisories, so a single page could not see the window
   at all. Now follows the Link rel=next cursor to exhaustion.

2. DATE-BASED FALLBACK. Version strings cannot be ordered across a scheme change
   (discourse semver 3.5.3 -> calver 2026.7.1), which is why the scan first reported a
   false 133, then correctly refused. Release DATES always order. When the version path
   refuses, the scan now resolves both versions to their git tag dates on the primary
   repo and counts advisories PUBLISHED in that window, labelling the method in the
   output. The version path is still preferred when usable — it is exact rather than
   temporal.

Verified: discourse 3.5.3 -> 2026.7.1 now reports 123, matching the hand count
(1 critical, 16 high, 91 medium, 16 low; window 2025-12-30 -> 2026-07-31); gitea
1.27.0 -> 1.27.1 still reports 2 via the version path.
Step-by-step specification of cc-ci-plan/advisory-scan.py: inputs, the three source
classes and why each is ranked where it is, the union, both classification paths
(patched-version ranges, and the advisory-publish-date fallback for version-scheme
changes), the output contract, and how /recipe-report must read it.

Each rule records the production wrong answer that motivated it — the false 133 from
cross-image counting, the n8n misclassification from reading only vulnerabilities[0],
the '?' sprawl from url punctuation and benign-404s, and the 'never emit 0 for an
undetermined count' rule. Claims cross-checked against the implementation.
Keep this file in the same commit as any behaviour change.
A recipe upgrades several images, each through its own version range. The scan
previously classified only the app repo, so sidecar bumps contributed nothing — the
alternative to the earlier bug where sidecars were judged by the APP's window and
produced a false 133.

Now: --window KEY=FROM:TO (repeatable) gives any other source its own range; each
window is classified independently (one may use patched-version ranges while another
falls back to advisory dates) and the count is the UNION. An image with no window is
still not counted — the scan will not guess a range it was not given. If ANY requested
window cannot be ordered, the total is UNKNOWN rather than a partial number.

/recipe-upgrade now instructs passing a --window per bumped sidecar.

Verified on discourse app 3.5.3->2026.7.1 + redis 7.4->8.10: 128 = 123 (app, by
publish date) + 5 (redis, by version range). The redis five are genuine for that bump
(patched 7.4.1 / 7.4.6 / 8.2.3) and include CVE-2025-49844, CRITICAL — previously
invisible. Regressions clean: gitea still 2, discourse without the sidecar window
still 123.
The skill still told the reporter to publish '?' whenever the scan hit a
version-scheme change. The scan now resolves those by advisory publish date, so
that instruction would have re-introduced a '?' for a count it can determine.

Also documents that counts are a union across per-image windows, and that a
sidecar-sourced critical must name its image in the bulletin.
The image name was packed into the value, so the flag needed a hand-rolled
KEY=FROM:TO parser with its own malformed-input branch, and 'window' named the
wrong thing — the tool has two kinds of window (version ranges and, on the date
fallback, real date windows) and the flag meant only the first.

Now each part is its own argument: --image redis 7.4 8.10, repeatable, all in
one call. argparse enforces the arity, so the string parsing and its error path
are deleted. 'windows' survives internally as the computed-range concept.

Counts unchanged: discourse 128 with redis / 123 without, gitea 2.
Restores the single-value form (operator preference) under the --image name.
Repeat the flag per image, all in one call. Malformed values warn on stderr and
are skipped rather than aborting the scan, since it is an additive pre-step.

Counts unchanged: discourse 128 with redis / 123 without, gitea 2.
Adds test-advisory-scan.py (58 offline tests on fixtures + 6 live regressions
against the week-2026-08-07 report) and audit-advisory-scan.py, which re-derives
every count with a SEPARATE semver implementation and its own release fetch and
diffs against the scanner. Both found real defects:

1. Window membership was compared on ragged tuples, so (18,) < (18,0) — a CVE
   patched in 18.0 fell OUTSIDE a window ending at 18. Bare major tags are the
   norm for sidecars (postgres:18, redis:8-alpine). Now zero-padded, which also
   keeps the upper bound conservative (18.5 stays out of a window ending at 18).

2. Advisories with no knowable fix version were silently counted as 'not fixed'.
   Twelve redis advisories say patched_versions 'TBD' or '7.4.X' with an
   open-ended range — six of them high severity. They are now INDETERMINATE:
   not counted, not dismissed, and surfaced in the output.

   All twelve turned out to be genuinely fixed: redis names each in the release
   notes of every branch that got the fix (CVE-2025-32023 -> 6.2.19, 7.2.10,
   7.4.5, 8.0.3, 8.2.0). So a third deterministic method resolves them from
   release notes, with the naming tags recorded as the citation. discourse's
   redis contribution goes 5 -> 17, and its total 128 -> 140.

Pass 2 (--adjudicate) is the model-judged stage for what arithmetic cannot
settle: it hands over each open case's full evidence, plus every verdict pass 1
reached, and takes FIXED/NOT-FIXED/STILL-UNKNOWN with a reason citing that
evidence. It may only raise a count. Vendor-page-only CVEs — the shape of both
gitea CVSS-9.8 RCEs — now reach it instead of being dropped.

Tests cover pass 1 only, by design; pass 2's judgement is a model's. What is
tested there is deterministic: which cases it selects, and that truncation is
announced rather than silent.

SPEC.md rewritten around the two passes.
autonomic-bot merged commit 7c331745d8 into review/session-base 2026-08-11 19:03:53 +00:00
Sign in to join this conversation.
No Reviewers
No labels
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: recipe-maintainers/cc-ci-orchestrator#2