fix(recon): 3 regex/logic correctness bugs on the run path (+ review report)

[raccioly]

Summary

Installed websec-validator from a clean clone and exercised the whole surface (editable install, byte-compile, --help/--version, doctor, websec run on both fixtures, and run --scan with the real scanners shelling out). The install/build/test path is healthy — all existing tests pass on Python 3.12 and the live pipeline produces a briefing without crashing.

So this is not a "make it build" PR. It's a focused correctness pass: a read-only review of the ~6.5k LOC of package code turned up three real bugs on the always-on websec run path, each empirically reproduced. This PR fixes exactly those three, each with a regression test that fails before the fix and passes after. Nothing else is touched.

@antonio — the three fixes below are safe to merge (tested, minimal, no behavior change beyond correcting the bug). The "Pending — needs your decision" section lists everything else I found but deliberately did not change, because each one involves a product judgment call (new parser, dedup tradeoff, advanced dynamic-phase behavior). Proposed fixes are included so you can green-light them individually.

Verification: python -m unittest discover -s tests → 198/198 OK · compileall clean · ruff introduces no new violations · live websec run smoke OK. Stdlib-only, zero new dependencies.

---

Fixed in this PR (3 — tested, minimal)

1. Flask route fallback dropped routes and mislabeled methods — extractors/routes.py

The regex-fallback (used when OWASP Noir is absent) for @app.route(...) used a greedy .* with re.S, so the optional methods=[...] group reached across the whole file to the last methods= and back-tracked. Net effect on a multi-route module:

@app.route("/a")                          # → reported as /a with ["POST"]  ❌ (POST is /b's)
@app.route("/b", methods=["POST"])        # → DROPPED entirely               ❌
@app.route("/c")                          # → /c (only routes AFTER the last methods= survived)

findall returned [('/a', '"POST"'), ('/c', '')] — /b vanished and /a stole its verb. Since this is the Noir-absent path, a Flask app reviewed without Noir got a corrupted route map (→ wrong authz coverage, wrong probe targeting). Fix: constrain the optional group to the single route(...) call with [^)]*? and drop re.S. Now returns all three routes with correct verbs. (test: test_fallback_flask_multiroute_no_route_dropped)

2. Password-policy drift detector masked lowercase-only regressions — extractors/policy_consistency.py

_RE_UPPER/_RE_LOWER are compiled with re.I, but their character-class branch is a literal A-Z / a-z. Under re.I the literal A-Z also matches a-z, so a lowercase-only rule (?=.*[a-z]) was counted as also requiring uppercase. Consequence: a weak sibling route requiring only lowercase computed the same class-set as a strong upper+lower policy, so the strict-subset drift comparison saw them as equal and reported no drift — a security false-negative in the detector whose entire job is catching that regression. Fix: make just the range token case-sensitive with (?-i:A-Z) / (?-i:a-z); the min*/require* keyword branches keep re.I. (test: test_lowercase_only_subset_is_drift)

3. GHA-injection finding detail double-prefixed github. — extractors/iac_ci.py

UNTRUSTED.findall(...) returns the whole ${{ github.… }} string, but the detail was built as "github." + c, emitting the malformed github.${{ github.event.pull_request.title }}. Detail-string only — detection, severity, and the SHA-context check were unaffected — but it's user-facing output handed to the agent. Fix: drop the "github." + prefix. (assertion added to test_gha_injection_in_run_flagged)

---

Pending — needs your decision (NOT changed here)

Real findings I left alone because each is a product/behavior call rather than an obvious correctness fix. Ranked by impact; proposed fix included for each.

P1 · Checkov runs but 100% of its findings are silently discarded — scanners.py

_checkov writes with --output-file-path <out.parent> (checkov names the file results_json.json itself), so the scanners/checkov.json path the tool records never exists; _count_findings has no checkov branch (always returns 0); and _PARSERS has no checkov parser, so even if the file were found, nothing normalizes it into findings.json. Checkov is detected, executed, and its entire output is dropped. Why deferred: a real fix needs a _norm_checkov (mapping checkov's failed_checks[]), a severity default (checkov severities are often null off-platform), and a dedup scheme vs Trivy's overlapping misconfig findings — i.e. a noise-vs-coverage decision. Proposed: read results_json.json, add _norm_checkov + a _count_findings branch; you pick the severity default + dedup key.

P2 · Secret de-dup can hide a second real secret — scanners.py:312,335

Secret fingerprints are secret|file|rule (no line), so two distinct secrets matched by the same rule in one file collapse to one row (the second's line/title is lost). Contrast _norm_semgrep, which includes line. Why deferred: adding line could reduce legitimate cross-tool merges (gitleaks reads git history lines, trivy reads working-tree lines — they may differ). Proposed: add StartLine to the secret fingerprint (safe direction for a security tool: never hide a distinct secret; accept rare cross-tool duplicates).

P3 · gitignored-secret downgrade is probably a no-op — scanners.py:357-371,402-408

_gitignored pipes Trivy's file strings to git -C <target> check-ignore --stdin and matches the echoed paths against f["file"]. git check-ignore echoes back the exact input string, and expects repo-relative paths — but trivy fs <abs target> typically emits absolute/root-prefixed paths, so the set comes back empty and the bug-066(b) downgrade does nothing (gitignored .env.local secrets stay CRITICAL). Why deferred: needs confirming exactly what Trivy emits in this setup before changing path handling. Proposed: normalize both sides via os.path.relpath(file, target) before comparing.

P4 · websec dynamic / calibration robustness (advanced opt-in phase)

• (a) mint() crashes on a scalar tenant field — dynamic.py:76-77: tenants = user.get(field) or []; tenants[0]. A singular groupId: 5 (int) → 5[0] TypeError (uncaught); a string → silently-wrong single-char tenant. Singular tenant ids are common. Proposed: coerce to a list before indexing.
• (b) cross-tenant LEAK verdict is brittle — dynamic.py:115 string-matches a 3-element empty-body allowlist ([], {}, {"data":[]}). Common empty wrappers ({"items":[]}, { "data": [] }, paginated) get misclassified as LEAK. Proposed: parse JSON and test emptiness structurally.
• (c) calibration overlay poisoning — cli.py:218 + calibration.samples_from_dynamic: write-auth verdicts are folded into the persistent local overlay (~/.cache/...) with no check on fail_open_suspected, even though findings.build_ledger deliberately distrusts those same verdicts in a fail-open env. One fail-open run permanently skews future P(real). Proposed: skip the write-auth samples when fail-open is suspected (BOLA leaks can stay).
• (d) calibrate --ingest crashes on a findings-less JSON object — cli.py:264: rows = data.get("findings", data) leaves rows as the dict, then for r in rows yields keys (str) → AttributeError. Proposed: data.get("findings", []) + skip non-dict rows.

P5 · Minor / hardening

• SEV_ORDER subscript inconsistency — scanners.py:420,426 use SEV_ORDER[...] while :408 uses .get(...,0). No trigger today (all parsers clamp), latent KeyError. → use .get(..., 1).
• dynamic urlopen has no scheme guard — dynamic.py:49: a file:///internal target would be followed. Low risk under the self-scan threat model. → assert http(s).
• latest symlink replace is non-atomic — a crash between unlink and symlink_to leaves no latest (next dynamic can't find FACTS.json). → temp-symlink + os.replace.
• authz reads relative code_path against CWD — authz.py: heuristic/fallback routes set code_path=rel; ctx.text(Path(cp)) resolves vs CWD not root, so the handler text is often empty and the endpoint is bucketed unknown (precision loss, Noir-absent only). → resolve relative paths against ctx.root.
• dead binding — routes.py:92 proc = subprocess.run(...) is never used (lint only).

---

Checked and found NOT to be bugs (diligence notes)

• is_localhost (dynamic.py:59) correctly uses urlparse(...).hostname + an exact-match loopback set — http://localhost@evil.com/ resolves to evil.com and is rejected, so the --probe-writes localhost guard is sound (DNS-rebinding aside, out of scope for a self-targeting tool).
• All scanner/git invocations use argv lists (no shell=True), so an adversarial repo path can't inject a command.
• The Wilson confidence-interval math in calibration.py was verified numerically against the shipped calibration.json (exact match) — no change needed.

🤖 Generated with Claude Code