From 4d877b18447aab713592734f81a53594d5a79383 Mon Sep 17 00:00:00 2001 From: Samir Hanna Verza Date: Tue, 6 Oct 2026 19:20:42 -0300 Subject: [PATCH 1/2] fix(reporting): serialize the duplicate check so concurrent agents cannot file a finding twice The duplicate check awaits an LLM call and compares the candidate with the findings that were stored when it started. Two agents filing the same finding at the same moment each saw a list without the other's report, passed the check and both stored it. Reproduced against the real create functions with a slow check: three concurrent identical filings were stored three times, while a sequential fourth was correctly rejected. Hold a per-run asyncio lock across read existing -> check -> add in both create_vulnerability_report and create_dependency_report. Distinct findings still go through, one duplicate check at a time. The candidate and field dicts are built before the lock (they do not depend on the stored list), so the diff is mostly indentation: `git diff -w` is 13 lines. Fixes #627 Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01TavwVMH7PvrD68JTDfd6Hb --- strix/report/state.py | 4 + strix/tools/reporting/tool.py | 125 ++++++++++++---------- tests/test_reporting_dedupe_race.py | 160 ++++++++++++++++++++++++++++ 3 files changed, 230 insertions(+), 59 deletions(-) create mode 100644 tests/test_reporting_dedupe_race.py diff --git a/strix/report/state.py b/strix/report/state.py index fd0ebd9449..dc52e0f83c 100644 --- a/strix/report/state.py +++ b/strix/report/state.py @@ -1,3 +1,4 @@ +import asyncio import json import logging import re @@ -210,6 +211,9 @@ def __init__(self, run_name: str | None = None): self.end_time: str | None = None self.vulnerability_reports: list[dict[str, Any]] = [] + # Held while a new finding is checked against the stored ones and then added, + # so two agents filing the same finding at once cannot both pass the check. + self.dedupe_lock = asyncio.Lock() self.final_scan_result: str | None = None self.scan_results: dict[str, Any] | None = None diff --git a/strix/tools/reporting/tool.py b/strix/tools/reporting/tool.py index e98201996b..5dfec28a73 100644 --- a/strix/tools/reporting/tool.py +++ b/strix/tools/reporting/tool.py @@ -872,7 +872,6 @@ async def _do_create( from strix.report.dedupe import check_duplicate - existing = report_state.get_existing_vulnerabilities() candidate = { "title": title, "description": description, @@ -913,30 +912,35 @@ async def _do_create( "http_exchange_ids": normalized_http_exchange_ids, } - dedupe = await check_duplicate(candidate, existing) - if dedupe.get("is_duplicate"): - duplicate_id = str(dedupe.get("duplicate_id") or "") - duplicate_title = next( - (r.get("title", "Unknown") for r in existing if r.get("id") == duplicate_id), - "", + # The duplicate check awaits an LLM call. Without a lock, two agents filing + # the same finding at once each compare against a list the other has not + # joined yet and both store it, so read, check and store as one step. + async with report_state.dedupe_lock: + existing = report_state.get_existing_vulnerabilities() + dedupe = await check_duplicate(candidate, existing) + if dedupe.get("is_duplicate"): + duplicate_id = str(dedupe.get("duplicate_id") or "") + duplicate_title = next( + (r.get("title", "Unknown") for r in existing if r.get("id") == duplicate_id), + "", + ) + return { + "success": False, + "error": ( + f"Potential duplicate of '{duplicate_title}' " + f"(id={duplicate_id[:8]}...) — do not re-report the same vulnerability" + ), + "duplicate_of": duplicate_id, + "duplicate_title": duplicate_title, + "confidence": dedupe.get("confidence", 0.0), + "reason": dedupe.get("reason", ""), + } + + report_id = report_state.add_vulnerability_report( + **report_fields, + agent_id=agent_id if isinstance(agent_id, str) else None, + agent_name=agent_name if isinstance(agent_name, str) else None, ) - return { - "success": False, - "error": ( - f"Potential duplicate of '{duplicate_title}' " - f"(id={duplicate_id[:8]}...) — do not re-report the same vulnerability" - ), - "duplicate_of": duplicate_id, - "duplicate_title": duplicate_title, - "confidence": dedupe.get("confidence", 0.0), - "reason": dedupe.get("reason", ""), - } - - report_id = report_state.add_vulnerability_report( - **report_fields, - agent_id=agent_id if isinstance(agent_id, str) else None, - agent_name=agent_name if isinstance(agent_name, str) else None, - ) except Exception as e: logger.exception("create_vulnerability_report persistence failed") return { @@ -1883,7 +1887,7 @@ def _build_dependency_evidence( return evidence -async def _do_create_dependency( # noqa: PLR0912 +async def _do_create_dependency( # noqa: PLR0912, PLR0915 *, title: str, description: str, @@ -2016,7 +2020,6 @@ async def _do_create_dependency( # noqa: PLR0912 from strix.report.dedupe import check_duplicate - existing = report_state.get_existing_vulnerabilities() candidate = { "title": title, "description": description, @@ -2025,39 +2028,43 @@ async def _do_create_dependency( # noqa: PLR0912 "dependency_metadata": dependency_metadata, "technical_analysis": technical_analysis, } - dedupe = await check_duplicate(candidate, existing) - if dedupe.get("is_duplicate"): - duplicate_id = dedupe.get("duplicate_id", "") - return { - "success": False, - "error": ( - f"Potential duplicate (id={duplicate_id[:8]}...) — " - "do not re-report the same dependency finding" - ), - "duplicate_of": duplicate_id, - "confidence": dedupe.get("confidence", 0.0), - "reason": dedupe.get("reason", ""), - } - - report_id = report_state.add_vulnerability_report( - title=title, - description=description, - severity=severity, - impact=impact, - target=target, - technical_analysis=technical_analysis, - remediation_steps=remediation_steps, - evidence=evidence, - assumptions=assumptions, - fix_effort=fix_effort, - cvss=cvss_score if advisory_cvss is not None else None, - cve=parsed_cve, - cwe=cwe, - finding_class="dependency_cve", - dependency_metadata=dependency_metadata, - agent_id=agent_id if isinstance(agent_id, str) else None, - agent_name=agent_name if isinstance(agent_name, str) else None, - ) + # Same race as create_vulnerability_report: the duplicate check awaits an LLM + # call, so read, check and store under one lock. + async with report_state.dedupe_lock: + existing = report_state.get_existing_vulnerabilities() + dedupe = await check_duplicate(candidate, existing) + if dedupe.get("is_duplicate"): + duplicate_id = dedupe.get("duplicate_id", "") + return { + "success": False, + "error": ( + f"Potential duplicate (id={duplicate_id[:8]}...) — " + "do not re-report the same dependency finding" + ), + "duplicate_of": duplicate_id, + "confidence": dedupe.get("confidence", 0.0), + "reason": dedupe.get("reason", ""), + } + + report_id = report_state.add_vulnerability_report( + title=title, + description=description, + severity=severity, + impact=impact, + target=target, + technical_analysis=technical_analysis, + remediation_steps=remediation_steps, + evidence=evidence, + assumptions=assumptions, + fix_effort=fix_effort, + cvss=cvss_score if advisory_cvss is not None else None, + cve=parsed_cve, + cwe=cwe, + finding_class="dependency_cve", + dependency_metadata=dependency_metadata, + agent_id=agent_id if isinstance(agent_id, str) else None, + agent_name=agent_name if isinstance(agent_name, str) else None, + ) except Exception as e: logger.exception("create_dependency_report persistence failed") return { diff --git a/tests/test_reporting_dedupe_race.py b/tests/test_reporting_dedupe_race.py new file mode 100644 index 0000000000..367f95df09 --- /dev/null +++ b/tests/test_reporting_dedupe_race.py @@ -0,0 +1,160 @@ +"""Findings filed at the same moment must go through the duplicate check one at a time. + +The duplicate check awaits an LLM call and compares the candidate with the findings +stored when it started. Two agents filing the same finding concurrently used to both +pass it, because neither saw the other's report yet, and the run stored it twice. +""" + +from __future__ import annotations + +import asyncio +from typing import TYPE_CHECKING, Any + +import pytest + +from strix.report.state import ReportState, set_global_report_state +from strix.tools.reporting.tool import _do_create, _do_create_dependency + + +if TYPE_CHECKING: + from collections.abc import Iterator + from pathlib import Path + + +_CVSS = { + "attack_vector": "N", + "attack_complexity": "L", + "privileges_required": "N", + "user_interaction": "N", + "scope": "U", + "confidentiality": "H", + "integrity": "H", + "availability": "H", +} + + +@pytest.fixture +def state(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[ReportState]: + monkeypatch.chdir(tmp_path) + report_state = ReportState(run_name="dedupe-race") + set_global_report_state(report_state) + yield report_state + set_global_report_state(None) + + +async def _slow_check(candidate: dict[str, Any], existing: list[dict[str, Any]]) -> dict[str, Any]: + """Stand in for the real check: it judges the candidate against the snapshot it is handed.""" + await asyncio.sleep(0.05) # the real one is a model call + for report in existing: + if report.get("title") == candidate["title"]: + return { + "is_duplicate": True, + "duplicate_id": report["id"], + "confidence": 0.99, + "reason": "same title", + } + return {"is_duplicate": False, "duplicate_id": "", "confidence": 0.9, "reason": "new"} + + +@pytest.fixture(autouse=True) +def slow_dedupe(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr("strix.report.dedupe.check_duplicate", _slow_check) + + +def _vulnerability(title: str = "SQL injection in /login") -> dict[str, Any]: + return { + "title": title, + "description": "d", + "impact": "i", + "target": "http://target.example", + "technical_analysis": "ta", + "poc_description": "pd", + "poc_script_code": "curl http://target.example/login", + "remediation_steps": "rs", + "evidence": "ev", + "assumptions": "as", + "counterevidence": "none found", + "confidence": "high", + "severity_change_conditions": "none", + "fix_effort": "low", + "cvss_breakdown": _CVSS, + "endpoint": "/login", + "method": "POST", + "cve": None, + "cwe": None, + "code_locations": None, + } + + +def _dependency(title: str = "Vulnerable requests release") -> dict[str, Any]: + return { + "title": title, + "description": "d", + "target": "http://target.example", + "cve": "CVE-2024-35195", + "package_name": "requests", + "installed_version": "2.31.0", + "impact": "i", + "remediation_steps": "upgrade", + "assumptions": "as", + "package_ecosystem": "pypi", + "fixed_version": "2.32.0", + "cwe": None, + "advisory_cvss": 5.6, + "technical_analysis": "ta", + "fix_effort": "low", + "manifest_path": "requirements.txt", + "reachability": "unknown", + "reachability_evidence": "searched the imports; no direct call found", + "contextual_cvss_breakdown": _CVSS, + "contextual_cvss_reasoning": "published rating unchanged by the usage seen", + } + + +async def test_same_finding_filed_concurrently_is_stored_once(state: ReportState) -> None: + first, second = await asyncio.gather( + _do_create(**_vulnerability()), _do_create(**_vulnerability()) + ) + + assert sorted([first["success"], second["success"]]) == [False, True] + rejected = first if not first["success"] else second + assert rejected["duplicate_of"] == state.vulnerability_reports[0]["id"] + assert len(state.vulnerability_reports) == 1 + + +async def test_distinct_findings_filed_concurrently_are_all_stored(state: ReportState) -> None: + results = await asyncio.gather( + _do_create(**_vulnerability("SQL injection in /login")), + _do_create(**_vulnerability("Reflected XSS in /search")), + _do_create(**_vulnerability("IDOR in /orders")), + ) + + assert all(result["success"] for result in results) + assert len(state.vulnerability_reports) == 3 + + +async def test_same_dependency_finding_filed_concurrently_is_stored_once( + state: ReportState, +) -> None: + first, second = await asyncio.gather( + _do_create_dependency(**_dependency()), _do_create_dependency(**_dependency()) + ) + + assert sorted([first["success"], second["success"]]) == [False, True] + assert len(state.vulnerability_reports) == 1 + + +async def test_a_failing_check_does_not_leave_the_lock_held( + state: ReportState, monkeypatch: pytest.MonkeyPatch +) -> None: + async def broken(*_args: object) -> dict[str, Any]: + raise RuntimeError("dedupe model unreachable") + + monkeypatch.setattr("strix.report.dedupe.check_duplicate", broken) + failed = await _do_create(**_vulnerability()) + assert failed["success"] is False + assert not state.dedupe_lock.locked() + + monkeypatch.setattr("strix.report.dedupe.check_duplicate", _slow_check) + retried = await asyncio.wait_for(_do_create(**_vulnerability()), timeout=5) + assert retried["success"] is True From b6cdb58ae7134321d304e93f6e5b28ba6f82f297 Mon Sep 17 00:00:00 2001 From: Samir Hanna Verza <123646616+samirhvbr@users.noreply.github.com> Date: Thu, 29 Oct 2026 07:00:01 -0300 Subject: [PATCH 2/2] fix(reporting): serialize dependency dedupe model fallback --- strix/tools/reporting/tool.py | 73 +++++++++++++++-------------- tests/test_reporting_dedupe_race.py | 62 +++++++++++++++++++++++- 2 files changed, 99 insertions(+), 36 deletions(-) diff --git a/strix/tools/reporting/tool.py b/strix/tools/reporting/tool.py index 34d3a64b7f..ab83a26b38 100644 --- a/strix/tools/reporting/tool.py +++ b/strix/tools/reporting/tool.py @@ -2028,7 +2028,6 @@ async def _do_create_dependency( # noqa: PLR0912, PLR0915 from strix.report.dedupe import check_duplicate - existing = report_state.get_existing_vulnerabilities() candidate = { "title": title, "description": description, @@ -2037,41 +2036,45 @@ async def _do_create_dependency( # noqa: PLR0912, PLR0915 "dependency_metadata": dependency_metadata, "technical_analysis": technical_analysis, } - dedupe = await check_duplicate(candidate, existing) - if dedupe.get("is_duplicate"): - duplicate_id = dedupe.get("duplicate_id", "") - return { - "success": False, - "error": ( - f"Potential duplicate (id={duplicate_id[:8]}...) — " - "do not re-report the same dependency finding" - ), - "duplicate_of": duplicate_id, - "confidence": dedupe.get("confidence", 0.0), - "reason": dedupe.get("reason", ""), - } + # A same-CVE report without package metadata can send dependency + # deduplication through the model too, so it shares the filing lock. + async with report_state.dedupe_lock: + existing = report_state.get_existing_vulnerabilities() + dedupe = await check_duplicate(candidate, existing) + if dedupe.get("is_duplicate"): + duplicate_id = dedupe.get("duplicate_id", "") + return { + "success": False, + "error": ( + f"Potential duplicate (id={duplicate_id[:8]}...) — " + "do not re-report the same dependency finding" + ), + "duplicate_of": duplicate_id, + "confidence": dedupe.get("confidence", 0.0), + "reason": dedupe.get("reason", ""), + } - report_id = report_state.add_vulnerability_report( - title=title, - description=description, - severity=severity, - impact=impact, - target=target, - technical_analysis=technical_analysis, - remediation_steps=remediation_steps, - evidence=evidence, - assumptions=assumptions, - fix_effort=fix_effort, - cvss=cvss_score if advisory_cvss is not None else None, - cve=parsed_cve, - cwe=cwe, - code_locations=parsed_locations, - fix_verification=fix_verification, - finding_class="dependency_cve", - dependency_metadata=dependency_metadata, - agent_id=agent_id if isinstance(agent_id, str) else None, - agent_name=agent_name if isinstance(agent_name, str) else None, - ) + report_id = report_state.add_vulnerability_report( + title=title, + description=description, + severity=severity, + impact=impact, + target=target, + technical_analysis=technical_analysis, + remediation_steps=remediation_steps, + evidence=evidence, + assumptions=assumptions, + fix_effort=fix_effort, + cvss=cvss_score if advisory_cvss is not None else None, + cve=parsed_cve, + cwe=cwe, + code_locations=parsed_locations, + fix_verification=fix_verification, + finding_class="dependency_cve", + dependency_metadata=dependency_metadata, + agent_id=agent_id if isinstance(agent_id, str) else None, + agent_name=agent_name if isinstance(agent_name, str) else None, + ) except Exception as e: logger.exception("create_dependency_report persistence failed") return { diff --git a/tests/test_reporting_dedupe_race.py b/tests/test_reporting_dedupe_race.py index a6294db4ed..69dd3e7109 100644 --- a/tests/test_reporting_dedupe_race.py +++ b/tests/test_reporting_dedupe_race.py @@ -24,7 +24,7 @@ from strix.config import loader from strix.report import dedupe from strix.report.state import ReportState, set_global_report_state -from strix.tools.reporting.tool import _do_create +from strix.tools.reporting.tool import _do_create, _do_create_dependency if TYPE_CHECKING: @@ -103,6 +103,30 @@ def _vulnerability(title: str = "SQL injection in /login") -> dict[str, Any]: } +def _dependency() -> dict[str, Any]: + return { + "title": "Vulnerable example-package dependency", + "description": "The package is pinned to an affected version.", + "target": "package-lock.json", + "cve": "CVE-2024-0001", + "package_name": "example-package", + "installed_version": "1.0.0", + "impact": "The dependency exposes the vulnerable code path.", + "remediation_steps": "Upgrade to the fixed version.", + "assumptions": "The lockfile describes the deployed application.", + "package_ecosystem": "npm", + "manifest_path": "package-lock.json", + "fixed_version": "1.0.1", + "cwe": None, + "advisory_cvss": 7.5, + "technical_analysis": "The package is present in the lockfile.", + "fix_effort": "low", + "reachability_evidence": "The package is installed; call sites have not been traced.", + "contextual_cvss_breakdown": _CVSS, + "contextual_cvss_reasoning": "The fixture retains the advisory impact assumptions.", + } + + @pytest.fixture def model(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> _FakeDedupeModel: fake = _FakeDedupeModel() @@ -160,6 +184,42 @@ async def test_distinct_findings_filed_concurrently_are_all_stored(state: Report assert len(state.vulnerability_reports) == 4 +async def test_dependency_model_fallback_serializes_concurrent_filings( + state: ReportState, model: _FakeDedupeModel +) -> None: + # A finding with the same CVE but no package identity makes the real + # dependency check fall back to the model instead of returning synchronously. + legacy = await _do_create( + **{**_vulnerability("Existing application finding"), "cve": "CVE-2024-0001"} + ) + assert legacy["success"] is True + calls_before = model.calls + + first, second = await asyncio.gather( + _do_create_dependency(**_dependency()), _do_create_dependency(**_dependency()) + ) + + assert sorted([first["success"], second["success"]]) == [False, True] + stored = [r for r in state.vulnerability_reports if r.get("finding_class") == "dependency_cve"] + assert len(stored) == 1 + rejected = first if not first["success"] else second + assert rejected["duplicate_of"] == stored[0]["id"] + # The first filing calls the model; the next sees its package identity. + assert model.calls - calls_before == 1 + + +async def test_dependency_identity_fast_path_still_needs_no_model( + state: ReportState, model: _FakeDedupeModel +) -> None: + first, second = await asyncio.gather( + _do_create_dependency(**_dependency()), _do_create_dependency(**_dependency()) + ) + + assert sorted([first["success"], second["success"]]) == [False, True] + assert len(state.vulnerability_reports) == 2 + assert model.calls == 0 + + async def test_a_failing_check_does_not_leave_the_lock_held( state: ReportState, monkeypatch: pytest.MonkeyPatch ) -> None: