diff --git a/strix/report/state.py b/strix/report/state.py index 6f111178f..e3fcfde67 100644 --- a/strix/report/state.py +++ b/strix/report/state.py @@ -1,5 +1,6 @@ import json import logging +import re import subprocess import threading from collections.abc import Callable @@ -18,6 +19,7 @@ from strix.report.sarif import write_sarif from strix.report.usage import LLMUsageLedger from strix.report.writer import ( + hydrate_report_evidence, read_run_record, write_executive_report, write_run_record, @@ -216,6 +218,7 @@ def hydrate_from_run_dir(self) -> None: ) self.vulnerability_reports = [r for r in data if isinstance(r, dict)] for r in self.vulnerability_reports: + hydrate_report_evidence(run_dir, r) rid = r.get("id") if isinstance(rid, str): self._saved_vuln_ids.add(rid) @@ -239,6 +242,7 @@ def add_vulnerability_report( assumptions: str | None = None, fix_effort: str | None = None, cvss: float | None = None, + cvss_vector: str | None = None, cvss_breakdown: dict[str, str] | None = None, endpoint: str | None = None, method: str | None = None, @@ -282,6 +286,8 @@ def add_vulnerability_report( report["fix_effort"] = fix_effort.strip().lower() if cvss is not None: report["cvss"] = cvss + if cvss_vector: + report["cvss_vector"] = cvss_vector if cvss_breakdown: report["cvss_breakdown"] = cvss_breakdown if endpoint: @@ -430,21 +436,41 @@ def cleanup(self, status: str = "stopped") -> None: self.save_run_data(status=status) def _format_final_scan_result(self, scan_results: dict[str, Any]) -> str: + # The finish_scan prompt asks the LLM to lead each field with its own + # "#
" heading; the template below adds the same headings, so + # strip a leading duplicate from each field to avoid doubled headings. + titles = { + "executive_summary": "Executive Summary", + "methodology": "Methodology", + "technical_analysis": "Technical Analysis", + "recommendations": "Recommendations", + } + + def section(key: str) -> str: + text = str(scan_results.get(key, "")).strip() + return re.sub( + rf"^#\s+{re.escape(titles[key])}\s*\n+", + "", + text, + count=1, + flags=re.IGNORECASE, + ).strip() + return f"""# Executive Summary -{str(scan_results.get("executive_summary", "")).strip()} +{section("executive_summary")} # Methodology -{str(scan_results.get("methodology", "")).strip()} +{section("methodology")} # Technical Analysis -{str(scan_results.get("technical_analysis", "")).strip()} +{section("technical_analysis")} # Recommendations -{str(scan_results.get("recommendations", "")).strip()} +{section("recommendations")} """ def _save_artifacts(self) -> None: diff --git a/strix/report/writer.py b/strix/report/writer.py index 2cdbae22f..895bb2e77 100644 --- a/strix/report/writer.py +++ b/strix/report/writer.py @@ -8,6 +8,7 @@ import logging import re import tempfile +from contextlib import suppress from datetime import UTC, datetime from pathlib import Path from typing import TYPE_CHECKING, Any, cast @@ -17,6 +18,7 @@ from pygments.util import ClassNotFound from strix.core.paths import run_record_path +from strix.utils.secret_files import SECRET_FILE_MODE, write_secret_text if TYPE_CHECKING: @@ -25,6 +27,8 @@ logger = logging.getLogger(__name__) _SEVERITY_ORDER = {"critical": 0, "high": 1, "medium": 2, "low": 3, "info": 4} +_PRIVATE_EVIDENCE_DIR = "private_evidence" +_PRIVATE_EVIDENCE_DIR_MODE = 0o700 _FENCE_RE = re.compile(r"^```([^\n`]*)\r?\n(.*?)\r?\n?```$", re.DOTALL) _BACKTICK_RUN = re.compile(r"`+") @@ -122,6 +126,80 @@ def write_executive_report(run_dir: Path, final_scan_result: str) -> None: logger.info("Saved final penetration test report to: %s", path) +def evidence_artifact_relpath(report_id: str) -> str: + return f"{_PRIVATE_EVIDENCE_DIR}/{report_id}.md" + + +def hydrate_report_evidence(run_dir: Path, report: dict[str, Any]) -> dict[str, Any]: + """Restore raw evidence from the private local artifact when present.""" + artifact = str(report.get("evidence_artifact") or "").strip() + evidence = str(report.get("evidence") or "").strip() + if not artifact: + return report + if evidence and evidence != _public_evidence_reference(artifact): + return report + + evidence_path = run_dir / artifact + if not evidence_path.is_file(): + return report + + try: + report["evidence"] = evidence_path.read_text(encoding="utf-8").rstrip("\n") + except OSError: + logger.warning("Could not read evidence artifact: %s", evidence_path) + return report + + +def _public_evidence_reference(artifact: str) -> str: + return ( + "Raw verification evidence is stored in the local artifact " + f"`{artifact}`. This report omits live credentials, session tokens, " + "and other replay material." + ) + + +def _public_report(report: dict[str, Any]) -> dict[str, Any]: + public = dict(report) + artifact = str(public.get("evidence_artifact") or "").strip() + + if artifact: + public["evidence"] = _public_evidence_reference(artifact) + else: + public.pop("evidence", None) + + return public + + +def _write_evidence_artifact(run_dir: Path, report: dict[str, Any]) -> None: + evidence = str(report.get("evidence") or "").strip() + report_id = str(report.get("id") or "").strip() + if not evidence or not report_id: + return + + artifact = str(report.get("evidence_artifact") or "").strip() or evidence_artifact_relpath( + report_id, + ) + if evidence == _public_evidence_reference(artifact): + # Hydration left the public reference in place because the private + # artifact was unreadable. Never overwrite the original evidence with + # that reference on a subsequent save. + return + + evidence_path = run_dir / artifact + try: + evidence_path.parent.mkdir(parents=True, exist_ok=True) + with suppress(OSError): + evidence_path.parent.chmod(_PRIVATE_EVIDENCE_DIR_MODE) + write_secret_text(evidence_path, f"{evidence}\n") + with suppress(OSError): + evidence_path.chmod(SECRET_FILE_MODE) + except OSError: + logger.exception("Could not write private evidence artifact: %s", evidence_path) + return + + report["evidence_artifact"] = artifact + + def write_vulnerabilities( run_dir: Path, vulnerability_reports: list[dict[str, Any]], @@ -131,16 +209,22 @@ def write_vulnerabilities( vuln_dir.mkdir(exist_ok=True) new_reports = [r for r in vulnerability_reports if r["id"] not in saved_vuln_ids] + public_reports = [] + + for report in vulnerability_reports: + _write_evidence_artifact(run_dir, report) + public_report = _public_report(report) + public_reports.append(public_report) + + report_path = vuln_dir / f"{report['id']}.md" + if report["id"] not in saved_vuln_ids or report_path.exists(): + _atomic_write_text(report_path, render_vulnerability_md(public_report)) for report in new_reports: - _atomic_write_text( - vuln_dir / f"{report['id']}.md", - render_vulnerability_md(report), - ) saved_vuln_ids.add(report["id"]) sorted_reports = sorted( - vulnerability_reports, + public_reports, key=lambda r: (_SEVERITY_ORDER.get(r["severity"], 5), r["timestamp"]), ) csv_path = run_dir / "vulnerabilities.csv" @@ -162,7 +246,7 @@ def write_vulnerabilities( _atomic_write_text( run_dir / "vulnerabilities.json", - json.dumps(vulnerability_reports, ensure_ascii=False, indent=2, default=str), + json.dumps(public_reports, ensure_ascii=False, indent=2, default=str), ) if new_reports: @@ -215,6 +299,8 @@ def render_vulnerability_md(report: dict[str, Any]) -> str: # noqa: PLR0912, PL cvss = report.get("cvss") if cvss is not None: metadata.append(("CVSS", cvss)) + if report.get("cvss_vector"): + metadata.append(("CVSS Vector", report["cvss_vector"])) advisory_cvss = dep_meta.get("advisory_cvss") if advisory_cvss is not None and advisory_cvss != cvss: metadata.append(("Advisory CVSS", advisory_cvss)) diff --git a/strix/tools/finish/tool.py b/strix/tools/finish/tool.py index b9704ad1a..cfbfd359a 100644 --- a/strix/tools/finish/tool.py +++ b/strix/tools/finish/tool.py @@ -156,16 +156,32 @@ async def finish_scan( field. - Tone: formal, third-person, objective, concise. This is a consultant deliverable, not an engineering log. + - **Disclose writes to the target.** If the assessment created any + data on the target (test accounts, uploaded files, telemetry + events, modified records), the report must state in + ``methodology`` what was written, how much, and whether cleanup + was performed. Never claim "no data was modified" when the PoCs + wrote to production endpoints — that conflicts with the PoC + evidence and undermines the deliverable's credibility. - Each section has a specific role: - ``executive_summary`` — for non-technical leadership. Risk posture, business impact (data exposure / compliance / reputation), notable criticals, overarching remediation - theme. + theme. Distinguish what was actually verified: "endpoints + require authentication" is not "object-level authorization + held up" — never claim authorization/IDOR coverage unless + cross-account or ownership checks were demonstrated. - ``methodology`` — frameworks followed (OWASP WSTG, PTES, OSSTMM, NIST), engagement type (black/gray/white box), scope and constraints, categories of testing performed. **No** - internal execution detail. + internal execution detail. State what was NOT tested or only + partially tested (missing credentials, unauthenticated-only + coverage, WAF interference) so "no findings" cannot be read + as a guarantee. Include a coverage summary — counts of + endpoints/APIs enumerated vs. actually tested, and the reason + each untested area was skipped — so the reader can judge how + much of the surface the assessment actually exercised. - ``technical_analysis`` — consolidated findings overview with severity model and systemic root causes. Reference individual vuln reports for repro steps; don't duplicate raw evidence. diff --git a/strix/tools/reporting/tool.py b/strix/tools/reporting/tool.py index 6cae75f1c..c6c82d2a8 100644 --- a/strix/tools/reporting/tool.py +++ b/strix/tools/reporting/tool.py @@ -236,7 +236,7 @@ async def _do_create( # noqa: PLR0912 return {"success": False, "error": "Validation failed", "errors": errors} try: - cvss_score, severity, _vector = _calculate_cvss(cvss_breakdown) + cvss_score, severity, cvss_vector = _calculate_cvss(cvss_breakdown) except ValueError as exc: return {"success": False, "error": "Validation failed", "errors": [str(exc)]} @@ -299,6 +299,7 @@ async def _do_create( # noqa: PLR0912 assumptions=assumptions, fix_effort=fix_effort, cvss=cvss_score, + cvss_vector=cvss_vector, cvss_breakdown=cvss_breakdown, endpoint=endpoint, method=method, @@ -401,6 +402,21 @@ async def create_vulnerability_report( attacker-controlled exploit and direct security impact. Client errors, compatibility issues, fingerprinting, and attack-surface discovery alone should not be filed as vulnerabilities. + - **Circular preconditions void a finding.** If exploitation + requires the attacker to already control a same-origin page, an + authenticated session the attacker cannot obtain, or another + unproven compromise, the premise is circular — downgrade to + informational (legacy code / hardening item), do not file as + XSS/injection with a real CVSS. Self-XSS and same-origin-gated + issues are informational unless a realistic cross-origin or + direct-victim path is demonstrated. + - **A successful 2xx on a data/telemetry endpoint proves the + endpoint parsed the request — nothing more.** Do not claim data + persistence, downstream consumption, metric pollution, rate-limit + absence, or backend resource abuse from request success or + batch-response timing alone. Those claims need ingestion, + persistence, or reporting-impact evidence; without it, file at + most an informational "unverified integrity risk" or omit. - Before filing, verify that the impact narrative, PoC, and every non-None CVSS impact metric describe the same demonstrated consequence. When evidence is incomplete, lower the metric or @@ -432,6 +448,27 @@ async def create_vulnerability_report( - Field discipline: ``poc_description`` is steps only — NO code (all code goes in ``poc_script_code``); ``remediation_steps`` is prose only — NO code/diffs (code fixes go in ``code_locations``). + - Conditional impact: keep verified consequences and plausible + follow-on risks clearly separated. State unverified execution + paths (e.g. "may trigger in MIME-sniffing clients") explicitly as + conditional in ``impact``, and never let them inflate CVSS + metrics; the metrics must reflect only the demonstrated + consequence. + - ``evidence`` is persisted into a separate local verification + artifact outside the client report files + (``vulnerabilities.json`` / per-finding markdown). Put the real + replay material there — including test-account credentials, + session tokens, object keys, and exact URLs/requests/responses + used during the assessment — so reviewers can re-run and confirm + the finding. External/client-facing artifacts keep only a safe + reference to that local evidence artifact. + - ``title`` must describe the precise defect (e.g. "File upload + without content validation" not "arbitrary file upload" when + extensions are allowlisted), without exaggerating. + - ``remediation_steps``: prefer the fix order that preserves + intended functionality (e.g. decode/re-encode content before + forcing download-only headers, which breaks inline previews), and + state trade-offs when a mitigation has user-visible side effects. - Numbered steps allowed only in PoC and Remediation sections. - Avoid hedging language; be precise and non-vague. - Follow a standard pentest report structure across the fields: @@ -559,7 +596,9 @@ async def create_vulnerability_report( remediation_steps: Specific, actionable fix (prose, no code). evidence: Concrete proof the issue is real and exploitable — request/response excerpts, observed behavior, tool output. - Use fenced code blocks; no internal identifiers/paths. + Use fenced code blocks. This content is stored in a separate + local evidence artifact rather than the client report files; + no internal identifiers/paths. assumptions: Short note on the assumptions/prerequisites that make this finding impactful or exploitable (e.g. "assumes an authenticated low-privilege user"). diff --git a/tests/test_report_writer.py b/tests/test_report_writer.py index 05222796f..33984a01d 100644 --- a/tests/test_report_writer.py +++ b/tests/test_report_writer.py @@ -4,11 +4,13 @@ import csv import json +import stat from typing import TYPE_CHECKING, Any import pytest from strix.report.writer import ( + hydrate_report_evidence, read_run_record, render_vulnerability_md, write_executive_report, @@ -163,6 +165,33 @@ def test_write_vulnerabilities_creates_markdown_csv_and_json(tmp_path: Path) -> assert csv_rows[0]["severity"] == "CRITICAL" +def test_write_vulnerabilities_separates_private_evidence_from_public_artifacts( + tmp_path: Path, +) -> None: + raw_evidence = "session_token=SECRET123\\ncredential=PRIVATE-TEST-VALUE" + reports = [_sample_report(id="vuln-0001", evidence=raw_evidence)] + + write_vulnerabilities(tmp_path, reports, set()) + + evidence_path = tmp_path / "private_evidence" / "vuln-0001.md" + assert evidence_path.read_text(encoding="utf-8") == f"{raw_evidence}\n" + assert stat.S_IMODE(evidence_path.stat().st_mode) == 0o600 + assert stat.S_IMODE(evidence_path.parent.stat().st_mode) == 0o700 + + public_json = (tmp_path / "vulnerabilities.json").read_text(encoding="utf-8") + public_markdown = (tmp_path / "vulnerabilities" / "vuln-0001.md").read_text( + encoding="utf-8", + ) + assert raw_evidence not in public_json + assert raw_evidence not in public_markdown + assert "private_evidence/vuln-0001.md" in public_json + assert "private_evidence/vuln-0001.md" in public_markdown + + restored = json.loads(public_json)[0] + hydrate_report_evidence(tmp_path, restored) + assert restored["evidence"] == raw_evidence + + def test_write_vulnerabilities_skips_already_saved_ids(tmp_path: Path) -> None: reports = [_sample_report(id="vuln-0001")] saved: set[str] = {"vuln-0001"} @@ -174,6 +203,82 @@ def test_write_vulnerabilities_skips_already_saved_ids(tmp_path: Path) -> None: assert (tmp_path / "vulnerabilities.csv").exists() +def test_write_vulnerabilities_externalizes_raw_evidence(tmp_path: Path) -> None: + raw_evidence = ( + "```http\n" + "GET /api/me HTTP/1.1\n" + "Authorization: Bearer live-session-token\n" + "Cookie: session=abc123\n" + "```" + ) + reports = [_sample_report(id="vuln-0001", evidence=raw_evidence)] + + write_vulnerabilities(tmp_path, reports, set()) + + public_json = (tmp_path / "vulnerabilities.json").read_text(encoding="utf-8") + public_report = json.loads(public_json)[0] + public_md = (tmp_path / "vulnerabilities" / "vuln-0001.md").read_text(encoding="utf-8") + evidence_path = tmp_path / "private_evidence" / "vuln-0001.md" + + assert "live-session-token" not in public_json + assert "session=abc123" not in public_json + assert public_report["evidence_artifact"] == "private_evidence/vuln-0001.md" + assert "Raw verification evidence is stored" in public_report["evidence"] + + assert "live-session-token" not in public_md + assert "session=abc123" not in public_md + assert "`private_evidence/vuln-0001.md`" in public_md + + assert evidence_path.read_text(encoding="utf-8") == f"{raw_evidence}\n" + assert evidence_path.stat().st_mode & 0o777 == 0o600 + assert evidence_path.parent.stat().st_mode & 0o777 == 0o700 + + +def test_write_vulnerabilities_does_not_overwrite_unreadable_private_evidence( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + raw_evidence = "Authorization: Bearer preserved-token" + reports = [_sample_report(id="vuln-0001", evidence=raw_evidence)] + write_vulnerabilities(tmp_path, reports, set()) + + evidence_path = tmp_path / "private_evidence" / "vuln-0001.md" + public_report = json.loads((tmp_path / "vulnerabilities.json").read_text(encoding="utf-8"))[0] + original_read_text = type(evidence_path).read_text + + def fail_private_evidence_read(path: Path, *args: Any, **kwargs: Any) -> str: + if path == evidence_path: + raise OSError("private evidence unavailable") + return original_read_text(path, *args, **kwargs) + + monkeypatch.setattr(type(evidence_path), "read_text", fail_private_evidence_read) + hydrate_report_evidence(tmp_path, public_report) + monkeypatch.undo() + + write_vulnerabilities(tmp_path, [public_report], {"vuln-0001"}) + + assert evidence_path.read_text(encoding="utf-8") == f"{raw_evidence}\n" + + +def test_write_vulnerabilities_persists_public_artifacts_when_private_evidence_fails( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + raw_evidence = "Authorization: Bearer unsaved-token" + reports = [_sample_report(id="vuln-0001", evidence=raw_evidence)] + + def fail_private_evidence_write(*_args: Any, **_kwargs: Any) -> None: + raise OSError("disk full") + + monkeypatch.setattr("strix.report.writer.write_secret_text", fail_private_evidence_write) + write_vulnerabilities(tmp_path, reports, set()) + + public = json.loads((tmp_path / "vulnerabilities.json").read_text(encoding="utf-8"))[0] + public_markdown = (tmp_path / "vulnerabilities" / "vuln-0001.md").read_text(encoding="utf-8") + assert raw_evidence not in json.dumps(public) + assert raw_evidence not in public_markdown + assert "evidence" not in public + assert "evidence_artifact" not in public def test_write_executive_report_writes_markdown(tmp_path: Path) -> None: write_executive_report(tmp_path, "Scan complete. No critical issues.") content = (tmp_path / "penetration_test_report.md").read_text(encoding="utf-8") diff --git a/tests/test_reporting_fields.py b/tests/test_reporting_fields.py index d433db40f..375558593 100644 --- a/tests/test_reporting_fields.py +++ b/tests/test_reporting_fields.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json from typing import TYPE_CHECKING import pytest @@ -93,6 +94,55 @@ async def test_create_report_persists_new_fields(report_state: ReportState) -> N assert report["finding_class"] == "dynamic" +async def test_create_report_externalizes_public_evidence_and_restores_on_hydrate( + report_state: ReportState, +) -> None: + raw_evidence = ( + "```http\n" + "GET /api/me HTTP/1.1\n" + "Authorization: Bearer live-session-token\n" + "Cookie: session=abc123\n" + "```" + ) + + result = await _do_create( + title="IDOR in profile API", + description="Profile endpoint accepts another user's id.", + impact="Cross-account data disclosure.", + target="https://app.example.com", + technical_analysis="Handler trusts the caller-supplied id.", + poc_description="1. Request another user's profile id.", + poc_script_code="GET /api/profile?id=2", + remediation_steps="Authorize object ownership server-side.", + evidence=raw_evidence, + assumptions="Assumes any authenticated user can reach the endpoint.", + fix_effort="low", + cvss_breakdown=_CVSS, + endpoint="/api/profile", + method="GET", + cve=None, + cwe="CWE-639", + code_locations=None, + ) + + assert result["success"] is True + + run_dir = report_state.get_run_dir() + public_json = (run_dir / "vulnerabilities.json").read_text(encoding="utf-8") + public_report = json.loads(public_json)[0] + + assert "live-session-token" not in public_json + assert "session=abc123" not in public_json + assert public_report["evidence_artifact"] == "private_evidence/vuln-0001.md" + assert "Raw verification evidence is stored" in public_report["evidence"] + + reloaded = ReportState(run_name="test-run") + reloaded.hydrate_from_run_dir() + + assert reloaded.vulnerability_reports[0]["evidence"] == raw_evidence + assert reloaded.vulnerability_reports[0]["evidence_artifact"] == "private_evidence/vuln-0001.md" + + async def test_create_report_requires_evidence_and_assumptions( report_state: ReportState, ) -> None: