diff --git a/strix/report/sarif.py b/strix/report/sarif.py index 821fffbc0..a13bde5b9 100644 --- a/strix/report/sarif.py +++ b/strix/report/sarif.py @@ -37,9 +37,12 @@ Design notes: * When a repository context is supplied (repo URL / commit / branch), the run carries ``versionControlProvenance`` + ``automationDetails`` so code-scanning can bind alerts to the scanned commit and branch. - * Findings without safe locations still appear in the SARIF output, - anchored to SECURITY.md and flagged via - ``properties.synthetic_location`` rather than being dropped silently. + * Findings without safe locations still appear in the SARIF output. + In a run over source code they are anchored to SECURITY.md and flagged + via ``properties.synthetic_location`` rather than being dropped + silently. A run with no source target (DAST: URLs, domains, IPs) has no + repository for a file to resolve against, so its locationless findings + carry only their ``logicalLocations`` (endpoint / resource) anchor. * Coverage rides in the same document as non-failing results (``kind`` of ``pass`` / ``notApplicable`` / ``open``), and run completeness on ``run.invocations``. Consumers that only want alerts filter on @@ -204,6 +207,7 @@ def build_sarif_report( tool_version: str | None = None, repository_context: dict[str, Any] | None = None, coverage: dict[str, Any] | None = None, + synthetic_anchor: bool = True, ) -> dict[str, Any]: """Return a SARIF 2.1.0 document for findings. @@ -226,6 +230,11 @@ def build_sarif_report( run-properties summary the UI can't render, and (b) carry the partialFingerprints + class hash so cross-run dismissal stickiness works for them. + + ``synthetic_anchor=False`` is for runs with no source target (DAST): + there is no repository for SECURITY.md to resolve against, so a + locationless finding that has a logical location (endpoint or + resource) carries only that. Its fingerprints are unchanged. """ rules_by_id: dict[str, dict[str, Any]] = {} rule_index_by_id: dict[str, int] = {} @@ -234,7 +243,9 @@ def build_sarif_report( dropped_unsafe_location_findings: list[dict[str, Any]] = [] for report in vulnerability_reports: - locations, is_synthetic, dropped_location_count = _build_locations(report) + locations, is_locationless, is_synthetic, dropped_location_count = _build_locations( + report, synthetic_anchor=synthetic_anchor + ) if is_synthetic: synthetic_location_count += 1 @@ -253,6 +264,7 @@ def build_sarif_report( rule_index_by_id[rule_id], report, locations, + is_locationless=is_locationless, is_synthetic=is_synthetic, ) ) @@ -309,6 +321,7 @@ def write_sarif_report( tool_version: str | None = None, repository_context: dict[str, Any] | None = None, coverage: dict[str, Any] | None = None, + synthetic_anchor: bool = True, ) -> None: """Write a SARIF report to disk, creating parent directories first. @@ -322,6 +335,7 @@ def write_sarif_report( tool_version=tool_version, repository_context=repository_context, coverage=coverage, + synthetic_anchor=synthetic_anchor, ) tmp_path = output_path.with_name(f"{output_path.name}.{os.getpid()}.tmp") try: @@ -340,6 +354,7 @@ def write_sarif( tool_version: str | None = None, repository_context: dict[str, Any] | None = None, coverage: dict[str, Any] | None = None, + synthetic_anchor: bool = True, filename: str = "findings.sarif", ) -> Path: """Write ``findings.sarif`` alongside existing outputs in ``run_dir``. @@ -355,6 +370,7 @@ def write_sarif( tool_version=tool_version, repository_context=repository_context, coverage=coverage, + synthetic_anchor=synthetic_anchor, ) logger.info( "Wrote SARIF 2.1.0 report: %s (%d results)", @@ -462,10 +478,13 @@ def _build_result( report: dict[str, Any], locations: list[dict[str, Any]], *, + is_locationless: bool = False, is_synthetic: bool = False, ) -> dict[str, Any]: """Build one SARIF result using validated locations. + ``is_locationless`` marks a finding with no real code location; its + fingerprint is computed the same way whether or not it was anchored. ``is_synthetic`` flags results whose location is the SECURITY.md anchor rather than a real code location — surfaces as ``properties.synthetic_location: true`` so reviewers and downstream @@ -494,7 +513,7 @@ def _build_result( # this report already carries (CWE, primary code location, route # tuple) — NOT from the LLM-authored title or message body, which # vary cosmetically across runs of the same finding. - fp = _primary_fingerprint(rule_id, report, locations, is_synthetic=is_synthetic) + fp = _primary_fingerprint(rule_id, report, locations, is_synthetic=is_locationless) if fp: result["partialFingerprints"] = {"primaryLocationLineHash": fp} # File-independent class fingerprint as a sibling property: lets @@ -768,33 +787,41 @@ def _synthetic_location() -> dict[str, Any]: } -def _build_locations(report: dict[str, Any]) -> tuple[list[dict[str, Any]], bool, int]: - """Return ``(locations, is_synthetic, dropped_count)`` for a finding. +def _build_locations( + report: dict[str, Any], *, synthetic_anchor: bool = True +) -> tuple[list[dict[str, Any]], bool, bool, int]: + """Return ``(locations, is_locationless, is_synthetic, dropped_count)``. Physical locations come from validated ``code_locations``. When none - are safe, the result is anchored to SECURITY.md (``is_synthetic``). + are safe (``is_locationless``), the result is anchored to SECURITY.md + (``is_synthetic``) unless ``synthetic_anchor`` is off and the finding + has a logical location to stand on instead. An ``endpoint`` (typical of DAST findings) is added as a ``logicalLocations`` entry; a locationless, endpoint-less finding gets a ``resource`` logical location carrying the target so the finding keeps a human-meaningful anchor. """ physical, dropped_location_count = _build_physical_locations(report.get("code_locations")) - is_synthetic = not physical - locations: list[dict[str, Any]] = list(physical) if physical else [_synthetic_location()] + is_locationless = not physical + logical: list[dict[str, Any]] = [] endpoint = _string_value(report.get("endpoint")) if endpoint: - locations.append( - {"logicalLocations": [{"fullyQualifiedName": endpoint, "kind": "endpoint"}]} - ) - elif is_synthetic: + logical.append({"logicalLocations": [{"fullyQualifiedName": endpoint, "kind": "endpoint"}]}) + elif is_locationless: resource = _string_value(report.get("target")) or _string_value(report.get("title")) if resource: - locations.append( + logical.append( {"logicalLocations": [{"fullyQualifiedName": resource, "kind": "resource"}]} ) - return locations, is_synthetic, dropped_location_count + is_synthetic = is_locationless and (synthetic_anchor or not logical) + locations: list[dict[str, Any]] = list(physical) + if is_synthetic: + locations.append(_synthetic_location()) + locations.extend(logical) + + return locations, is_locationless, is_synthetic, dropped_location_count def _build_physical_locations(raw_locations: Any) -> tuple[list[dict[str, Any]], int]: @@ -982,10 +1009,14 @@ def _primary_fingerprint( vulnerability class on the same rule_id always lands on the same hash, and two different classes on the same rule_id don't collide. + Locationless findings hash with the SECURITY.md URI whether or not + the anchor was emitted, so turning the anchor off does not re-key + existing alerts. + Returns None when no anchor is available AND not synthetic. """ primary_physical = _first_physical_location(locations) - uri = "" + uri = _SYNTHETIC_LOCATION_URI if is_synthetic and not primary_physical else "" start_line: int | None = None if primary_physical: uri = (primary_physical.get("artifactLocation") or {}).get("uri", "") or "" diff --git a/strix/report/state.py b/strix/report/state.py index 104e4af4c..8623786fd 100644 --- a/strix/report/state.py +++ b/strix/report/state.py @@ -132,6 +132,9 @@ def _number(value: Any) -> int | float: return 0 +_SOURCE_TARGET_TYPES = frozenset({"repository", "local_code"}) + + def _parse_repo_full_name(uri: str) -> str | None: """Extract ``owner/repo`` from a git URL or slug, else None.""" text = uri.strip().removesuffix(".git") @@ -888,6 +891,7 @@ class ReportState: tool_version=_strix_version(), repository_context=self._sarif_repository_context(), coverage=coverage, + synthetic_anchor=self._has_source_target(), ) except Exception: logger.exception("SARIF emit failed (non-fatal; CSV/MD unaffected)") @@ -908,6 +912,20 @@ class ReportState: self._sarif_repo_ctx_ready = True return self._sarif_repo_ctx + def _has_source_target(self) -> bool: + """Whether any target is source code (a repository or local directory). + + SARIF anchors locationless findings to SECURITY.md only for these: a + DAST-only run has no repository for that file to resolve against. + """ + targets = self.run_record.get("targets_info") or [] + if not isinstance(targets, list): + return True + return any( + isinstance(target, dict) and target.get("type") in _SOURCE_TARGET_TYPES + for target in targets + ) + def _derive_repository_context(self) -> dict[str, Any] | None: targets = self.run_record.get("targets_info") or [] if not isinstance(targets, list): diff --git a/tests/test_sarif.py b/tests/test_sarif.py index 61ffc39bb..766bba6b1 100644 --- a/tests/test_sarif.py +++ b/tests/test_sarif.py @@ -198,6 +198,63 @@ def test_write_sarif_synthetic_finding_falls_back_to_resource_logical_location( assert logical == [{"fullyQualifiedName": "https://api.example.com", "kind": "resource"}] +def _dast_finding(**overrides: Any) -> dict[str, Any]: + return _finding(code_locations=None, endpoint="/insecure_direct_object_ref_1.php", **overrides) + + +def test_write_sarif_without_synthetic_anchor_keeps_only_the_logical_location( + tmp_path: Path, +) -> None: + # A DAST run has no repository, so a locationless finding must not point + # at a SECURITY.md that does not exist in any scanned commit. + write_sarif(tmp_path, [_dast_finding()], synthetic_anchor=False) + run = _read(tmp_path)["runs"][0] + result = run["results"][0] + assert result["locations"] == [ + { + "logicalLocations": [ + {"fullyQualifiedName": "/insecure_direct_object_ref_1.php", "kind": "endpoint"} + ] + } + ] + assert "synthetic_location" not in result["properties"] + assert "syntheticLocationCount" not in run.get("properties", {}) + + +def test_write_sarif_without_synthetic_anchor_keeps_the_fingerprints(tmp_path: Path) -> None: + anchored, unanchored = tmp_path / "a", tmp_path / "b" + write_sarif(anchored, [_dast_finding()]) + write_sarif(unanchored, [_dast_finding()], synthetic_anchor=False) + a = _read(anchored)["runs"][0]["results"][0] + b = _read(unanchored)["runs"][0]["results"][0] + assert a["partialFingerprints"] == b["partialFingerprints"] + assert a["properties"]["strix_vuln_class_hash"] == b["properties"]["strix_vuln_class_hash"] + + +def test_write_sarif_without_synthetic_anchor_still_anchors_code_findings( + tmp_path: Path, +) -> None: + write_sarif(tmp_path, [_finding()], synthetic_anchor=False) + location = _read(tmp_path)["runs"][0]["results"][0]["locations"][0] + assert location["physicalLocation"]["artifactLocation"]["uri"] == "app.py" + + +def test_write_sarif_without_synthetic_anchor_keeps_it_when_nothing_else_anchors( + tmp_path: Path, +) -> None: + # A result with no logical location either would otherwise be left with no + # location at all, so it keeps the SECURITY.md anchor. + write_sarif( + tmp_path, + [_finding(code_locations=None, endpoint=None, target=None, title=None)], + synthetic_anchor=False, + ) + result = _read(tmp_path)["runs"][0]["results"][0] + uri = result["locations"][0]["physicalLocation"]["artifactLocation"]["uri"] + assert uri == "SECURITY.md" + assert result["properties"]["synthetic_location"] is True + + def test_write_sarif_emits_version_control_provenance(tmp_path: Path) -> None: write_sarif( tmp_path, diff --git a/tests/test_state_repo_context.py b/tests/test_state_repo_context.py index b0a6eb315..07fcee05c 100644 --- a/tests/test_state_repo_context.py +++ b/tests/test_state_repo_context.py @@ -92,3 +92,22 @@ def test_repository_context_derives_commit_and_branch_from_clone(tmp_path: Path) assert ctx["commitSha"] == head assert ctx["branch"] == "main" assert ctx["ref"] == "refs/heads/main" + + +def test_has_source_target_only_for_repository_or_local_code_targets() -> None: + state = ReportState(run_name="t") + state.run_record["targets_info"] = [ + {"type": "web_application", "details": {"target_url": "https://example.com"}}, + {"type": "ip_address", "details": {"target_ip": "10.0.0.1"}}, + ] + assert state._has_source_target() is False + + state.run_record["targets_info"].append( + {"type": "local_code", "details": {"target_path": "/src"}} + ) + assert state._has_source_target() is True + + state.run_record["targets_info"] = [ + {"type": "repository", "details": {"target_repo": "https://github.com/acme/widget"}} + ] + assert state._has_source_target() is True