diff --git a/strix/report/coverage.py b/strix/report/coverage.py index a6b08759..0f7a5e8a 100644 --- a/strix/report/coverage.py +++ b/strix/report/coverage.py @@ -355,20 +355,35 @@ def skill_coverage_gaps( return [] carriers: dict[str, list[str]] = {} + carrier_ids: dict[str, set[str]] = {} for agent in agents: + agent_id = str(agent.get("agent_id") or "") for skill in agent["skills"]: leaf = _skill_leaf(skill) if leaf in risk_skills: carriers.setdefault(leaf, []).append(str(agent["agent_name"])) + if agent_id: + carrier_ids.setdefault(leaf, set()).add(agent_id) gaps: list[dict[str, Any]] = [] for skill, agent_names in sorted(carriers.items()): names = ", ".join(sorted(set(agent_names))) topics = _topics_for(skill) + # A bundled skill's surfaces are only covered by rows from an agent that + # actually carried that skill. Otherwise an unrelated row that merely + # shares a word — an open_redirect row mentioning "meta refresh" — would + # mask an untested browser surface. Single-topic skills keep the shared + # ledger semantics: any row naming the class counts, whoever wrote it. + vclass = VULN_CLASSES.get(skill) + if vclass is not None and vclass.sub_topics: + owner_ids = carrier_ids.get(skill, set()) + relevant = [e for e in entries if str(e.get("agent_id") or "") in owner_ids] + else: + relevant = entries uncovered = [ (label, wordlists) for label, wordlists in topics - if not any(_entry_is_about(entry, wordlists) for entry in entries) + if not any(_entry_is_about(entry, wordlists) for entry in relevant) ] if not uncovered: continue diff --git a/strix/report/dedupe.py b/strix/report/dedupe.py index 391ee967..d93a8c64 100644 --- a/strix/report/dedupe.py +++ b/strix/report/dedupe.py @@ -94,9 +94,13 @@ CRITICAL DEDUPLICATION RULES: - Same CVE but different package/ecosystem is NOT a duplicate - Same package/ecosystem but different CVE is NOT a duplicate -5. Different finding_class is NOT a duplicate even under the same CWE: - - client_side_path_traversal vs a server-side/dynamic path-traversal finding - both map to CWE-22, but they are distinct classes fixed by different code +5. finding_class separates classes only when both are specific: + - two reports with different SPECIFIC (non-default) finding_class values are + distinct classes even under one shared CWE - fixed by different code, NOT duplicates + - but `dynamic` is the default catch-all, not a class: a `dynamic` report and a + `client_side_path_traversal` report can still be the SAME bug (e.g. an older CSPT + filed before the class existed). Do not treat dynamic-vs-specific as proof of + distinctness - compare the underlying root cause COMPARISON GUIDELINES: - Focus on the technical root cause, not surface-level similarities @@ -107,8 +111,9 @@ COMPARISON GUIDELINES: FIELDS TO ANALYZE: - title, description: General vulnerability info - target, endpoint, method: Exact location of vulnerability -- finding_class: Machine-readable class; a different finding_class means a - different class of bug even under a shared CWE - not a duplicate +- finding_class: Machine-readable class. Two different SPECIFIC classes under one + CWE are different bugs (not duplicates); but `dynamic` is the default, not a + class, so a `dynamic` report may still duplicate a specific one - compare the bug - technical_analysis: Root cause details - poc_description: How it's exploited - impact: What damage it can cause diff --git a/strix/report/sarif.py b/strix/report/sarif.py index 6e66c80e..7feb4307 100644 --- a/strix/report/sarif.py +++ b/strix/report/sarif.py @@ -920,7 +920,6 @@ _VULN_CLASS_KEYWORDS = ( "default password", "session fixation", "open redirect", - "client-side path traversal", "path traversal", "directory traversal", "command injection", diff --git a/strix/tools/reporting/tool.py b/strix/tools/reporting/tool.py index e1700a3f..1ca5b385 100644 --- a/strix/tools/reporting/tool.py +++ b/strix/tools/reporting/tool.py @@ -17,6 +17,7 @@ from typing import TYPE_CHECKING, Any from agents import RunContextWrapper, function_tool from strix.report.coverage import selectable_finding_classes +from strix.report.sarif import _build_physical_locations from strix.tools.nullish import clean_optional from strix.tools.proxy.tools import existing_request_ids @@ -858,18 +859,24 @@ async def _do_create( f"Invalid finding_class: {finding_class!r}. " f"Must be one of: {sorted(_VALID_FINDING_CLASSES)}" ) - elif finding_class == "client_side_path_traversal" and not ( - str(endpoint or "").strip() or parsed_locations - ): + elif finding_class == "client_side_path_traversal": # CSPT findings are typically locationless (the bug is in a JS bundle, - # not a server route), so without a discriminator multiple distinct - # CSPT findings collapse onto one synthetic SARIF fingerprint. - errors.append( - "finding_class 'client_side_path_traversal' needs a discriminator so " - "locationless CSPT findings do not collapse onto one SARIF fingerprint: " - "set endpoint (the traversed target path, e.g. '/admin/keys') and method, " - "or a code_location for the vulnerable client-side sink." - ) + # not a server route), so without a discriminator multiple distinct CSPT + # findings collapse onto one synthetic SARIF fingerprint. The anchor has + # to be one SARIF can actually use: an endpoint (→ route), or a + # code_location that survives _build_physical_locations (a repo-relative + # file with a real start line — an absolute or drive path is dropped and + # would leave the finding synthetic and collapsed). + has_endpoint = bool(str(endpoint or "").strip()) + physical_locations, _dropped = _build_physical_locations(parsed_locations) + if not (has_endpoint or physical_locations): + errors.append( + "finding_class 'client_side_path_traversal' needs a discriminator SARIF " + "can anchor, or distinct CSPT findings collapse onto one fingerprint: set " + "endpoint (the traversed target path, e.g. '/admin/keys') and method, or a " + "code_location with a repo-relative file and start line (not an absolute or " + "drive path, which SARIF drops)." + ) if errors: return {"success": False, "error": "Validation failed", "errors": errors} diff --git a/tests/fixtures/cspt/README.md b/tests/fixtures/cspt/README.md index c4151632..75c7bfc0 100644 --- a/tests/fixtures/cspt/README.md +++ b/tests/fixtures/cspt/README.md @@ -5,10 +5,13 @@ flows, used by `tests/test_cspt_fixtures.py` as a deterministic, browser-free regression for what CSPT looks like in client code. Unlike a Playwright capture test, these run in ordinary CI with no Chromium. -Naming: `positive_*` is an exploitable flow — an attacker-controlled client +Naming: `positive_*` has the CSPT **primitive** — an attacker-controlled client source (URL query, fragment, referrer, postMessage, storage) reaches a request -sink's path unguarded. `negative_*` is a safe control — the flow is validated, -or the user value never reaches the request path. +sink's path unguarded. That is the source-to-sink reachability only; whether the +redirected request is *exploitable* (a state change, a rendered response, an +internal fetch, an authz bypass) is a separate step, per the exploitability bar +in the `client_side_path_traversal` skill. `negative_*` is a control — the flow +is validated, or the user value never reaches the request path. Convention the test relies on: the attacker-controlled value that reaches a sink is named `userPath`, so a sink-call line containing `userPath` marks a diff --git a/tests/fixtures/cspt/positive_hash_route.html b/tests/fixtures/cspt/positive_hash_route.html index bb7631d2..b2b89570 100644 --- a/tests/fixtures/cspt/positive_hash_route.html +++ b/tests/fixtures/cspt/positive_hash_route.html @@ -1,6 +1,7 @@