Artifact review: stop the credential at the API boundary (v2.8.0)

.env.example

@@ -157,20 +157,28 @@ SCAN_HTTP_POSTURE=true
 # The raw HTML is already scanned, so a plainly-embedded secret is caught either
 # way; this decodes the JSON so a credential mangled by \uXXXX escaping is still
 # recognised. Local decode only — no extra requests, scan stays passive.
-# Asset caching (v2.7.7). Sends If-None-Match / If-Modified-Since on re-scans.
-# An asset that is BOTH unchanged (304) AND produced no finding last time is
-# skipped entirely; one that previously had a finding is always refetched so the
-# finding cannot silently vanish from a report. No response body is ever cached
-# — only validators, a content hash, and a clean/dirty flag — because a client's
-# JavaScript can contain live credentials. Cache entries expire after 24h.
 # Client reports redact the credential by default (sk_abc…******…3fc4 (51 chars)).
 # A report gets emailed, forwarded and archived, so writing a live secret into
 # one turns the deliverable itself into a second exposure — and the RoE we ask
 # clients to sign promises redaction. The redacted form stays greppable, so a
 # developer can still identify exactly which key to rotate. Set true only when
 # you deliberately need the full value in the artefact.
+#
+# This affects generated reports ONLY. The dashboard, the WebSocket stream and
+# the REST API always mask, with no opt-out — those are screen-sharing and
+# screenshot surfaces, not deliverables an operator chose to produce.
 REPORT_FULL_SECRETS=false
 
+# Asset caching (v2.7.7). Sends If-None-Match / If-Modified-Since on re-scans.
+# A *terminal* asset (JS bundle, source map) that is BOTH unchanged (304) AND
+# produced no finding last time is skipped entirely; one that previously had a
+# finding is always refetched so the finding cannot silently vanish from a
+# report. Crawled HTML pages are never skipped, even when unchanged (v2.8.0): a
+# page is a link graph, and skipping its body means never discovering the
+# bundles it references — which turned a re-scan of an unchanged site into a
+# false all-clear. No response body is ever cached — only validators, a content
+# hash, and a clean/dirty flag — because a client's JavaScript can contain live
+# credentials. Cache entries expire after 24h.
 ASSET_CACHE=true
 
 SCAN_INLINE_JSON=true

CHANGELOG.md

@@ -3,6 +3,82 @@
 All notable changes to SecretNode are documented here. This project adheres to
 [Semantic Versioning](https://semver.org/).
 
+## [2.8.0] — Artifact review: the credential stops at the API boundary
+
+Driven by a side-by-side read of the artifacts from a real v2.7.9 deep scan — the dashboard modal,
+the Discord alert, and the SARIF/CSV/HTML exports of the same three findings. v2.7.9 had fixed
+redaction in the *reports*; comparing the surfaces showed how much of the same job was left undone
+everywhere else, and turned up two correctness bugs that had nothing to do with display.
+
+### Fixed
+
+- **A re-scan of an unchanged site reported CLEAN while the credential was still exposed.** The
+  worst bug in this release, and the only one that produces a wrong *answer* rather than a wrong
+  *presentation*. The asset cache treated a `304 Not Modified` on the root page as "unchanged and
+  previously clean, skip" — but an HTML page is a link graph, not just something to grep. Skipping
+  its body meant never parsing its `<script>` tags, so every JS bundle it referenced dropped out of
+  the scan. Reproduced on a lab target: scan 1 found a planted key, scan 2 never requested the file
+  holding it and reported no findings. Crawled pages now always come back with a body
+  (`allow_cache_skip=False`) — the conditional GET is still sent, so the bandwidth saving on
+  unchanged terminal assets (JS bundles, source maps) is untouched.
+- **The dashboard rendered the full credential.** `redact_secret()` was added to `report.py` in
+  v2.7.9 and nowhere else, so `/api/scans/{id}` and the WebSocket stream shipped `raw_match`
+  verbatim and the finding-detail modal displayed it — under a heading reading "MATCHED VALUE
+  (PARTIAL)" that a 51-character key comfortably fit inside. Redaction now happens at the API
+  boundary (`public_scan` / `public_event` in `main.py`) using a `mask_secret()` that has no
+  opt-out; `REPORT_FULL_SECRETS` remains available for a report an operator deliberately generates,
+  but no longer unmasks every dashboard session and WebSocket subscriber.
+- **The `scan_complete` event carried both unmasked finding lists.** Found by capturing a live
+  WebSocket stream rather than by reading the code: per-finding events were being scrubbed, but the
+  final frame of every scan ships the entire result dict, and it was going out untouched.
+- **The code snippet leaked the secret the matched-value field was hiding.** `context_snippet` was
+  stored and served verbatim, so the modal and the JSON export both contained the credential in
+  full even once `raw_match` was masked. Now masked in `ValidatedFinding.to_dict()`, where the
+  complete value is still available to match against — masking it downstream from the 80-character
+  `raw_match` cap would have left the tail of a longer secret exposed.
+- **The JSON export was the one deliverable format still containing live keys.** HTML, CSV and
+  SARIF all redacted; `format=json` returned the stored record as-is.
+- **The dashboard showed MEDIUM for findings the reports called HIGH.** The frontend re-derived
+  severity from a hardcoded list of 14 secret type names. Every detector added since that list was
+  written — the entire AI/ML provider family — fell through to the MEDIUM default. It now reads the
+  `severity` field the backend already computes from the pattern registry. A missing `.badge-low`
+  style meant LOW findings rendered unstyled; added.
+- **Discord alerts announced "SecretNode v2.4.0"** for five releases, and coloured every embed with
+  a per-type table covering 16 of 60+ detectors — so an ElevenLabs key and an AWS root key arrived
+  looking identical. Version now comes from the shared `version.py`; embed colour is keyed on
+  effective severity, and the severity is stated as a field.
+- **Deep-scan reports lost every scan-level metric.** `DeepScanResult.to_dict()` never emitted
+  `assets_fetched`, so a SARIF from a 25-host run claimed `assets_fetched: 0`; the same omission
+  dropped `raw_findings`, `duration_seconds`, posture findings and the verification counts. All are
+  now rolled up, with duration measured as wall-clock rather than summed across concurrently
+  scanned hosts.
+- **A fully-cached re-scan reported "0 assets analysed".** True as a download count, wrong as
+  coverage — it reads as "nothing was scanned". Split into `assets_fetched` (downloaded),
+  `assets_cached` (unchanged and clean, skipped) and `assets_scanned` (coverage); reports lead with
+  coverage and name the cached portion.
+- **Deep scans inherited asset-cache state from whatever ran before them in the same process.** The
+  cache is module-level and only the single-target endpoint primes it, so a deep scan could report
+  a previous scan's cache hits as its own coverage — and, worse, act on a stale validator. Deep
+  scans now start from an empty cache, and `run_scan` resets the hit tally alongside the throttle.
+
+### Changed
+
+- Deep-scan HTML gains the redacted matched value, verification status and impact per finding, plus
+  an asset-coverage stat — the format a client actually reads was the one carrying the least
+  evidence. It also notes that one value appearing on several hosts is a single credential shared
+  across environments, which is what a fanned-out dev/QA/preprod exposure actually looks like.
+- SARIF results carry `matched_value_partial`, `found_at` and (on deep scans) the originating
+  `host`, so a triager can correlate a result against the CSV row and the Discord alert. Run
+  properties gained coverage and duration.
+- Version is single-sourced in `backend/version.py`; a test now fails the build on any hardcoded
+  `SecretNode vX.Y.Z` literal in the backend.
+
+### Tests
+
+- `test_v280.py`: 35 tests covering each fix, including a mutation check that the leak guards can
+  actually fail, and a reproduction of the false all-clear. Suite: 347 → 382.
+
+
 ## [2.7.9] — Deep QA before release: reports no longer leak the credential
 
 A full pre-PR QA pass: booted the application, ran real end-to-end scans against a local target

backend/main.py

@@ -179,6 +179,72 @@ async def broadcast_global(self, event: dict[str, Any]) -> None:
 _registry: dict[str, dict[str, Any]] = {}
 
 
+# ─────────────────────────────────────────────────────────────────────────────
+# Credential redaction at the API boundary
+# ─────────────────────────────────────────────────────────────────────────────
+#
+# The registry and SQLite hold the full matched value on purpose: report
+# generation needs it (report.py masks on the way out, and REPORT_FULL_SECRETS
+# lets an operator opt into the real value in a report they control), and the
+# fingerprint used for false-positive suppression is derived from it.
+#
+# What must never happen is that value reaching a browser or a WebSocket
+# subscriber. Until now it did: /api/scans/{id} returned the finding dicts
+# verbatim and the dashboard's finding-detail modal rendered `raw_match` under
+# a "MATCHED VALUE (PARTIAL)" heading that was simply not true — a 51-character
+# key fits inside the 60-character client-side slice. The dashboard is served
+# over whatever transport the operator deployed it on, gets screenshotted into
+# tickets, and is the one surface a client is most likely to be looking at
+# over someone's shoulder.
+#
+# So: redact here, at the boundary, and leave storage untouched.
+
+_FINDING_LISTS = ("confirmed_findings", "needs_review_findings")
+
+
+def public_finding(finding: dict[str, Any]) -> dict[str, Any]:
+    """Return a copy of a finding safe to hand to an API/WebSocket client."""
+    raw = finding.get("raw_match")
+    if not raw:
+        return finding
+    # mask_secret, not redact_secret: REPORT_FULL_SECRETS is an opt-in for a
+    # report the operator generates and controls, not for every dashboard
+    # session and WebSocket subscriber.
+    return {**finding, "raw_match": report_gen.mask_secret(str(raw))}
+
+
+def public_scan(scan: dict[str, Any]) -> dict[str, Any]:
+    """Return a copy of a scan record with every credential masked."""
+    if not isinstance(scan, dict):
+        return scan
+    out = dict(scan)
+    for key in _FINDING_LISTS:
+        findings = out.get(key)
+        if isinstance(findings, list):
+            out[key] = [public_finding(f) if isinstance(f, dict) else f
+                        for f in findings]
+    return out
+
+
+def public_event(event: dict[str, Any]) -> dict[str, Any]:
+    """Redact live scan events before they go out over a WebSocket.
+
+    Two shapes carry findings, and missing the second is easy: the per-finding
+    events wrap one finding under "data", but `scan_complete` ships the entire
+    result dict under "result" — including both finding lists. Scrubbing only
+    the former left the whole unmasked set going out in the final frame of
+    every scan.
+    """
+    etype = event.get("type")
+    if etype in ("finding", "finding_needs_review"):
+        data = event.get("data")
+        return {**event, "data": public_finding(data)} if isinstance(data, dict) else event
+    if etype in ("scan_complete", "deep_scan_complete"):
+        result = event.get("result")
+        return {**event, "result": public_scan(result)} if isinstance(result, dict) else event
+    return event
+
+
 # ─────────────────────────────────────────────────────────────────────────────
 # Lifespan
 # ─────────────────────────────────────────────────────────────────────────────
@@ -317,7 +383,7 @@ async def start_scan(request: ScanRequest, http_request: Request) -> dict[str, A
     state   = ScanState()
 
     async def broadcaster(event: dict[str, Any]) -> None:
-        await manager.broadcast_scan(scan_id, event)
+        await manager.broadcast_scan(scan_id, public_event(event))
 
     async def _run() -> None:
         try:
@@ -427,7 +493,7 @@ async def start_deep_scan(request: DeepScanRequest, http_request: Request) -> di
     scan_id = str(uuid.uuid4())
 
     async def broadcaster(event: dict[str, Any]) -> None:
-        await manager.broadcast_scan(scan_id, event)
+        await manager.broadcast_scan(scan_id, public_event(event))
 
     async def _run() -> None:
         try:
@@ -495,7 +561,7 @@ async def list_scans(
 ) -> dict[str, Any]:
     """List known scans for this session (in-memory)."""
     scans = [
-        entry["meta"]
+        public_scan(entry["meta"])
         for entry in list(_registry.values())[offset: offset + limit]
     ]
     return {"scans": scans, "count": len(scans)}
@@ -507,22 +573,22 @@ async def scan_history(
     offset: int = Query(default=0, ge=0),
 ) -> dict[str, Any]:
     """List all scans ever run, persisted in SQLite — survives restarts."""
-    scans = await load_scans(limit=limit, offset=offset)
+    scans = [public_scan(s) for s in await load_scans(limit=limit, offset=offset)]
     return {"scans": scans, "count": len(scans)}
 
 
 @app.get("/api/scans/{scan_id}", dependencies=[Depends(require_api_key)])
 async def get_scan(scan_id: str) -> dict[str, Any]:
     entry = _registry.get(scan_id)
     if entry:
-        meta = entry["meta"].copy()
+        meta = public_scan(entry["meta"])
         task: asyncio.Task = entry["task"]
         if task.done():
             meta["status"] = "complete" if not task.cancelled() else "cancelled"
         return meta
     persisted = await load_scan(scan_id)
     if persisted:
-        return persisted
+        return public_scan(persisted)
     raise HTTPException(status_code=404, detail="Scan not found")
 
 
@@ -538,11 +604,15 @@ async def get_scan_status(scan_id: str) -> dict[str, Any]:
     return {
         "scan_id": scan_id,
         "status":  status,
-        "meta":    entry["meta"],
+        "meta":    public_scan(entry["meta"]),
     }
 
 
 async def _resolve_scan(scan_id: str) -> dict[str, Any]:
+    """Fetch the *unredacted* scan record for report generation. Every report
+    format masks the credential itself (report.redact_secret), so this is the
+    one path that must not pre-redact — otherwise a report would show a mask of
+    a mask, and REPORT_FULL_SECRETS would have nothing left to reveal."""
     entry = _registry.get(scan_id)
     if entry:
         return entry["meta"]
@@ -594,7 +664,7 @@ async def get_scan_report(
         return Response(content=body, media_type="application/sarif+json", headers={
             "Content-Disposition": f'attachment; filename="secretnode_report_{scan_id[:8]}.sarif"'
         })
-    return JSONResponse(content=scan)
+    return JSONResponse(content=report_gen.generate_json_report(scan))
 
 
 @app.post("/api/findings/suppress", dependencies=[Depends(require_api_key)])
@@ -638,7 +708,7 @@ async def list_suppressed_findings(
 @app.get("/api/active", dependencies=[Depends(require_api_key)])
 async def active_scans() -> dict[str, Any]:
     active = [
-        entry["meta"]
+        public_scan(entry["meta"])
         for entry in _registry.values()
         if not entry["task"].done()
     ]