diff --git a/docs/advanced/skills.mdx b/docs/advanced/skills.mdx index ce624424..2f6f0a5a 100644 --- a/docs/advanced/skills.mdx +++ b/docs/advanced/skills.mdx @@ -48,6 +48,7 @@ Core vulnerability classes with deep exploitation techniques. | `business_logic` | Logic flaws, state manipulation, race conditions | | `race_conditions` | TOCTOU, parallel request attacks | | `path_traversal_lfi_rfi` | File inclusion, path traversal | +| `client_side_path_traversal` | Exploitable CSPT: route/query/hash into fetch/router | | `open_redirect` | Redirect bypasses, URL parsing tricks | | `mass_assignment` | Attribute injection, hidden parameter pollution | | `insecure_file_uploads` | Upload bypasses, extension tricks | diff --git a/strix/report/coverage.py b/strix/report/coverage.py index c519853a..c3444a95 100644 --- a/strix/report/coverage.py +++ b/strix/report/coverage.py @@ -29,6 +29,7 @@ from __future__ import annotations import json import logging +from dataclasses import dataclass from datetime import UTC, datetime from typing import TYPE_CHECKING, Any @@ -65,80 +66,162 @@ _INCOMPLETE_RUN_STATUSES = frozenset({"failed", "interrupted", "stopped", "runni #: so holding one implies no coverage obligation. _RISK_SKILL_CATEGORY = "vulnerabilities" -#: How each vulnerability skill can legitimately appear in a ledger row. -#: -#: Matching a skill to a row is textual, and a skill's filename is not how a -#: pentester writes the class down: an agent carrying ``path_traversal_lfi_rfi`` -#: records "Path Traversal", and one carrying ``weak_password_detection`` -#: records "weak password policy". A row matches when it contains every word -#: of *any one* phrasing here. Skills absent from this map fall back to their -#: own words, so a new skill is merely matched strictly, never crashed on — -#: but add an entry, because a false gap asserts something untrue in a report. -_SKILL_PHRASINGS: dict[str, tuple[str, ...]] = { - "agentic_system_security": ( - "agentic", - "agent tool", - "mcp", - "confused deputy", - "tool invocation", +@dataclass(frozen=True) +class SubTopic: + """One independently-accountable surface of a bundled skill. + + ``label`` is how the surface is named back to a reader; ``phrasings`` are + the ledger-row wordings that count as covering it (a row matches when it + contains every word of any one phrasing). + """ + + label: str + phrasings: tuple[str, ...] + + +@dataclass(frozen=True) +class VulnClass: + """Canonical description of one vulnerability class. + + ``aliases`` are the phrasings a ledger row may use to name the class — a + skill's filename is not how a pentester writes the class down (an agent + carrying ``path_traversal_lfi_rfi`` records "Path Traversal"), so the + aliases bridge the two. ``sub_topics`` are the independently-accountable + surfaces of a *bundled* skill: testing one surface must not imply the rest, + so each is checked on its own. ``cwe`` is the canonical id(s) for the class + — advisory metadata today (SARIF keeps its own CWE map), but the registry + is the place to unify it. + """ + + aliases: tuple[str, ...] + sub_topics: tuple[SubTopic, ...] = () + cwe: tuple[str, ...] = () + #: True when a reporting agent may set this class as a report's + #: machine-readable ``finding_class`` to separate it from other classes + #: that share a CWE (e.g. client-side vs server-side path traversal, both + #: CWE-22). Most classes rely on ``ruleId`` alone and leave this False. + selectable_finding_class: bool = False + + +#: The canonical registry of vulnerability classes, keyed by the bare skill +#: name (the ``strix/skills/vulnerabilities/.md`` stem). This is the one +#: place these facts live, so the same class cannot be described by two +#: unrelated string lists — the drift that let client-side path traversal hide +#: behind the server-side "path traversal" name. A new skill absent here is +#: matched strictly by its own name, never crashed on — but add an entry, +#: because a false gap asserts something untrue in a report. +VULN_CLASSES: dict[str, VulnClass] = { + "agentic_system_security": VulnClass( + ("agentic", "agent tool", "mcp", "confused deputy", "tool invocation") ), - "argument_injection": ("argument injection", "option injection", "argv"), - "authentication_jwt": ("authentication", "jwt", "session"), - "broken_function_level_authorization": ( - "function level authorization", - "authorization", - "access control", - "privilege escalation", + "argument_injection": VulnClass( + ("argument injection", "option injection", "argv"), cwe=("CWE-88",) ), - "browser_security": ( - "browser", - "postmessage", - "xs leak", - "service worker", - "cross origin state", + "authentication_jwt": VulnClass(("authentication", "jwt", "session"), cwe=("CWE-287",)), + "broken_function_level_authorization": VulnClass( + ( + "function level authorization", + "authorization", + "access control", + "privilege escalation", + ), + cwe=("CWE-862",), ), - "business_logic": ("business logic", "logic flaw"), - "csrf": ("csrf", "cross site request forgery"), - "header_injection": ("header injection", "host header", "crlf"), - "http_request_smuggling": ("request smuggling", "desync"), - "idor": ("idor", "object level authorization", "bola", "direct object reference"), - "information_disclosure": ( - "information disclosure", - "information leak", - "sensitive data", - "data exposure", + "browser_security": VulnClass( + ("browser", "postmessage", "xs leak", "service worker", "cross origin state"), + sub_topics=( + SubTopic("postMessage", ("postmessage", "message listener", "window messaging")), + SubTopic( + "client-side path traversal", + ("client side path traversal", "cspt", "client side path"), + ), + SubTopic("XS-Leaks", ("xs leak", "xs-leaks", "cross origin oracle", "cross site leak")), + SubTopic( + "service workers and caches", + ("service worker", "cache api", "cache poisoning"), + ), + SubTopic("web workers", ("web worker", "worker script")), + SubTopic( + "navigation and redirect control", + ("client side redirect", "spa navigation", "history manipulation", "meta refresh"), + ), + SubTopic( + "CSP and browser parsing", + ("content security policy", "csp bypass", "trusted types"), + ), + SubTopic("local network access", ("local network", "loopback", "dns rebinding")), + SubTopic("JavaScript gadgets", ("javascript gadget", "gadget chain", "dom gadget")), + ), ), - "insecure_deserialization": ("deserialization",), - "insecure_file_uploads": ("file upload",), - "llm_prompt_injection": ("prompt injection",), - "mass_assignment": ("mass assignment", "parameter binding"), - "nosql_injection": ("nosql",), - "open_redirect": ("redirect",), - "path_traversal_lfi_rfi": ( - "path traversal", - "directory traversal", - "file inclusion", - "lfi", - "rfi", + "business_logic": VulnClass(("business logic", "logic flaw"), cwe=("CWE-840",)), + "client_side_path_traversal": VulnClass( + ("client side path traversal", "cspt", "client side path"), + cwe=("CWE-22",), + selectable_finding_class=True, ), - "prototype_pollution": ("prototype pollution",), - "race_conditions": ("race condition", "toctou"), - "rce": ("rce", "remote code execution", "code execution", "command injection"), - "semantic_confusion": ( - "semantic confusion", - "parser differential", - "normalization", - "validator sink mismatch", + "csrf": VulnClass(("csrf", "cross site request forgery"), cwe=("CWE-352",)), + "header_injection": VulnClass(("header injection", "host header", "crlf"), cwe=("CWE-113",)), + "http_request_smuggling": VulnClass(("request smuggling", "desync"), cwe=("CWE-444",)), + "idor": VulnClass( + ("idor", "object level authorization", "bola", "direct object reference"), + cwe=("CWE-639",), ), - "sql_injection": ("sql injection", "sqli"), - "ssrf": ("ssrf", "server side request forgery"), - "ssti": ("ssti", "template injection"), - "subdomain_takeover": ("subdomain takeover",), - "weak_password_detection": ("password", "credential", "brute force"), - "xss": ("xss", "cross site scripting", "script injection"), - "xxe": ("xxe", "xml external entity", "xml entity"), + "information_disclosure": VulnClass( + ("information disclosure", "information leak", "sensitive data", "data exposure"), + cwe=("CWE-200",), + ), + "insecure_deserialization": VulnClass(("deserialization",), cwe=("CWE-502",)), + "insecure_file_uploads": VulnClass(("file upload",), cwe=("CWE-434",)), + "llm_prompt_injection": VulnClass(("prompt injection",), cwe=("CWE-1427",)), + "mass_assignment": VulnClass(("mass assignment", "parameter binding"), cwe=("CWE-915",)), + "nosql_injection": VulnClass(("nosql",), cwe=("CWE-943",)), + "open_redirect": VulnClass(("redirect",), cwe=("CWE-601",)), + "path_traversal_lfi_rfi": VulnClass( + ("path traversal", "directory traversal", "file inclusion", "lfi", "rfi"), + cwe=("CWE-22",), + ), + "prototype_pollution": VulnClass(("prototype pollution",), cwe=("CWE-1321",)), + "race_conditions": VulnClass(("race condition", "toctou"), cwe=("CWE-362",)), + "rce": VulnClass( + ("rce", "remote code execution", "code execution", "command injection"), cwe=("CWE-94",) + ), + "semantic_confusion": VulnClass( + ("semantic confusion", "parser differential", "normalization", "validator sink mismatch") + ), + "sql_injection": VulnClass(("sql injection", "sqli"), cwe=("CWE-89",)), + "ssrf": VulnClass(("ssrf", "server side request forgery"), cwe=("CWE-918",)), + "ssti": VulnClass(("ssti", "template injection"), cwe=("CWE-1336",)), + "subdomain_takeover": VulnClass(("subdomain takeover",)), + "weak_password_detection": VulnClass( + ("password", "credential", "brute force"), cwe=("CWE-521",) + ), + "xss": VulnClass(("xss", "cross site scripting", "script injection"), cwe=("CWE-79",)), + "xxe": VulnClass(("xxe", "xml external entity", "xml entity"), cwe=("CWE-611",)), } +#: Backward-compatible view: skill name -> alias phrasings, derived from the +#: registry so the two can never drift. Used wherever a class's top-level +#: phrasings are needed (and imported by the coverage tests). +_SKILL_PHRASINGS: dict[str, tuple[str, ...]] = { + name: vclass.aliases for name, vclass in VULN_CLASSES.items() +} + + +def selectable_finding_classes() -> frozenset[str]: + """Vulnerability classes a reporting agent may set as a report's + ``finding_class``. + + ``finding_class`` is a machine-readable sub-classification that separates + findings sharing a CWE rule (``ruleId``) in SARIF and dedup — client-side + path traversal and server-side path traversal / LFI / RFI all key on + CWE-22, so the rule alone cannot tell them apart. Sourcing the selectable + set from the registry keeps it from being hand-maintained in the reporting + tool, where it would silently drift from the class definitions here. + """ + return frozenset( + name for name, vclass in VULN_CLASSES.items() if vclass.selectable_finding_class + ) + def read_agent_graph(state_dir: Path) -> dict[str, Any]: """Load the coordinator's snapshot, or ``{}`` when it isn't readable. @@ -215,55 +298,182 @@ def agents_from_graph(graph: dict[str, Any]) -> list[dict[str, Any]]: return agents -def _skill_phrasings(skill: str) -> list[list[str]]: - """Word lists that would each count as a ledger row naming *skill*.""" - phrasings = _SKILL_PHRASINGS.get(skill) or (skill,) - return [terms for phrase in phrasings if (terms := _normalized(phrase).split())] +def _phrasing_wordlists(phrases: tuple[str, ...]) -> list[list[str]]: + """Normalize each phrasing into a word list for :func:`_entry_is_about`.""" + return [terms for phrase in phrases if (terms := _normalized(phrase).split())] + + +def cwe_for_skill(skill: str) -> tuple[str, ...]: + """Canonical CWE id(s) for a vulnerability skill, or empty if unmapped.""" + vclass = VULN_CLASSES.get(_skill_leaf(skill)) + return vclass.cwe if vclass else () + + +def _term_in_words(term: str, words: set[str]) -> bool: + """A phrasing word matches a row word exactly, or as its simple plural. + + Matching whole words, not substrings, is what stops a short class token + like ``rce`` matching inside ``brute force`` / ``enforcement`` or ``idor`` + inside ``corridor``. The plural allowance (``worker`` matches a ``workers`` + row, ``password`` a ``passwords`` row) keeps a trivial inflection from + opening a false gap. + + Only the forward direction (phrasing term + ``s``) is allowed, never the + reverse (row word + ``s`` == term): the reverse would let the row word + ``xs`` satisfy the ``xss`` class alias (``"xs" + "s" == "xss"``), so an + unrelated XS-Leaks row would falsely mark XSS covered. A phrasing that + needs to match a singular row carries the singular spelling as its own + alias instead. + """ + return term in words or f"{term}s" in words def _entry_is_about(entry: dict[str, Any], phrasings: list[list[str]]) -> bool: - """True when a ledger row plausibly concerns any phrasing of a risk class.""" - haystack = _normalized(f"{entry.get('risk_area', '')} {entry.get('surface', '')}") - return any(all(term in haystack for term in terms) for terms in phrasings) + """True when a ledger row names any phrasing of a risk class. + + A phrasing matches when every one of its words appears as a **word** in the + row (``risk_area`` + ``surface``) — not as a substring, so "brute force" + never counts as ``rce`` coverage and "corridor" never as ``idor``. As a + fallback it also matches the phrasing's words joined into one token, so a + row that recorded a joined / CamelCase name the normalizer kept whole + ("DirectoryTraversal" -> "directorytraversal", "XSLeaks" -> "xsleaks") + still counts — still a whole-token check, never a substring. + """ + words = set(_normalized(f"{entry.get('risk_area', '')} {entry.get('surface', '')}").split()) + for terms in phrasings: + if all(_term_in_words(term, words) for term in terms): + return True + if len(terms) > 1 and _term_in_words("".join(terms), words): + return True + return False + + +def _entry_authored_by(entry: dict[str, Any], owner_ids: set[str]) -> bool: + """True when this row — now, or in a superseded revision — was written by + one of *owner_ids*. + + ``update_coverage`` overwrites ``agent_id`` with the updater's, so a surface + a carrier genuinely assessed would look unexamined after an unrelated agent + edits the shared row. History preserves the earlier author, so an earlier + assessment by a carrier still counts. + """ + if str(entry.get("agent_id") or "") in owner_ids: + return True + history = entry.get("history") + if isinstance(history, list): + return any( + isinstance(item, dict) and str(item.get("agent_id") or "") in owner_ids + for item in history + ) + return False + + +def _topics_for(skill: str) -> list[tuple[str | None, list[list[str]]]]: + """The independently-accountable topics a carried skill must cover. + + A *bundled* skill (one with declared ``sub_topics``) is accountable per + surface: proving one does not imply the rest, so each sub-topic is a topic + of its own. Every other skill has a single implicit topic — the class + itself, matched by its aliases — which reproduces the original skill-level + behaviour exactly. Each topic is ``(label, wordlists)``; the label is + ``None`` for the single-topic case. + """ + vclass = VULN_CLASSES.get(skill) + if vclass is None: + return [(None, _phrasing_wordlists((skill,)))] + if vclass.sub_topics: + return [(st.label, _phrasing_wordlists(st.phrasings)) for st in vclass.sub_topics] + return [(None, _phrasing_wordlists(vclass.aliases))] def skill_coverage_gaps( entries: list[dict[str, Any]], agents: list[dict[str, Any]] ) -> list[dict[str, Any]]: - """Vulnerability classes an agent was equipped for but never recorded. + """Vulnerability classes (and bundled-skill surfaces) an agent was equipped + for but never recorded. A skill assigned to an agent is a declaration of intent that the runtime observed independently of anything the agent later said. When no ledger row mentions that class, the class is unaccounted for — which is a very different report line from "tested, nothing found". + + A bundled skill is held to a finer standard: each of its surfaces is + checked on its own, so testing the easy one (a ``postMessage`` origin + check) cannot mark the dangerous one (client-side path traversal) as + covered. A skill nobody touched still collapses to a single class-level + gap; a partially tested one yields one gap per untested surface. """ risk_skills = _risk_skill_names() if not risk_skills: 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()): - phrasings = _skill_phrasings(skill) - if any(_entry_is_about(entry, phrasings) for entry in entries): + 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 _entry_authored_by(e, owner_ids)] + else: + relevant = entries + uncovered = [ + (label, wordlists) + for label, wordlists in topics + if not any(_entry_is_about(entry, wordlists) for entry in relevant) + ] + if not uncovered: continue - gaps.append( - { - "kind": "unrecorded_risk_class", - "risk_area": skill.replace("_", " "), - "detail": ( - f"Agent(s) {', '.join(sorted(set(agent_names)))} were assigned the " - f"'{skill}' skill, but no coverage entry records this class being " - "assessed. Treat it as unexamined, not as clean." - ), - } - ) + if len(uncovered) == len(topics): + # Nothing about this class was recorded at all — one class-level gap. + detail = ( + f"Agent(s) {names} were assigned the '{skill}' skill, but no coverage " + "entry records this class being assessed. Treat it as unexamined, not " + "as clean." + ) + if len(topics) > 1: + surfaces = ", ".join(str(label) for label, _ in topics) + detail += f" Its surfaces ({surfaces}) are each separately accountable." + gaps.append( + { + "kind": "unrecorded_risk_class", + "risk_area": skill.replace("_", " "), + "detail": detail, + } + ) + continue + # Some surfaces were tested but not all: name each untested surface, so a + # bundled skill cannot be marked covered on the strength of its easiest one. + for label, _wordlists in uncovered: + gaps.append( + { + "kind": "unrecorded_sub_topic", + "skill": skill, + "risk_area": str(label), + "detail": ( + f"Agent(s) {names} carried '{skill}' and recorded coverage for some " + f"of its surfaces, but nothing records '{label}'. Testing one surface " + "of a multi-surface skill does not cover the others — treat " + f"'{label}' as unexamined." + ), + } + ) return gaps diff --git a/strix/report/dedupe.py b/strix/report/dedupe.py index 93f9ad8a..d93a8c64 100644 --- a/strix/report/dedupe.py +++ b/strix/report/dedupe.py @@ -94,6 +94,14 @@ CRITICAL DEDUPLICATION RULES: - Same CVE but different package/ecosystem is NOT a duplicate - Same package/ecosystem but different CVE is NOT a duplicate +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 - Same vulnerability type (SQLi, XSS) doesn't mean duplicate - location matters @@ -103,6 +111,9 @@ COMPARISON GUIDELINES: FIELDS TO ANALYZE: - title, description: General vulnerability info - target, endpoint, method: Exact location of vulnerability +- 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 @@ -145,6 +156,7 @@ def _prepare_report_for_comparison(report: dict[str, Any]) -> dict[str, Any]: "endpoint", "method", "cve", + "finding_class", "dependency_metadata", ] diff --git a/strix/report/sarif.py b/strix/report/sarif.py index 821fffbc..7feb4307 100644 --- a/strix/report/sarif.py +++ b/strix/report/sarif.py @@ -556,6 +556,15 @@ def _result_properties( if value not in (None, ""): strix[key] = value + # Surface a non-default finding sub-class (e.g. client_side_path_traversal) + # so consumers can machine-separate it from findings that share the same + # CWE rule — CSPT and server-side path traversal / LFI / RFI all key on + # CWE-22, so ``ruleId`` alone cannot tell them apart. The default + # ``dynamic`` carries no signal and is omitted to avoid noise. + finding_class = _string_value(report.get("finding_class")) + if finding_class and finding_class != "dynamic": + strix["finding_class"] = finding_class + dependency_metadata = report.get("dependency_metadata") if isinstance(dependency_metadata, dict) and dependency_metadata: strix["dependency_metadata"] = dependency_metadata @@ -1017,12 +1026,18 @@ def _primary_fingerprint( parts.append(f"route:{route}") if is_synthetic: - # Augment with class keyword so locationless findings of + # Augment with the class token so locationless findings of # different vuln classes don't collide. rule_id is already in # the composite so we don't also need to stamp the CWE. - title = _string_value(report.get("title")) or "" - if title: - parts.append(f"synth_class:{_class_keyword(title)}") + # Prefer the structured finding_class over a title keyword: a CSPT + # finding and a server-side traversal finding both key on CWE-22 + # and can share the generic "path traversal" title keyword, so + # keying on the title alone would collapse them onto one primary + # fingerprint. _class_token resolves to the discriminating + # structured field when present. + token = _class_token(report) + if token: + parts.append(f"synth_class:{token}") composite = "|".join(parts) return hashlib.sha256(composite.encode("utf-8")).hexdigest() @@ -1045,23 +1060,41 @@ def _class_fingerprint(rule_id: str, report: dict[str, Any]) -> str | None: primary fingerprint (typical case: file rename, or a fix that moves the vulnerable code to a new module). - Composite of (rule_id, vuln-class keyword extracted from title). - Title is LLM-authored so it's stochastic at the prose level, but - the class keyword extraction picks up the discrete vulnerability - category, which is much more stable than the full title. - - Falls back to the first 5 lowercased words of the title when no - curated keyword matches. Acceptable as a fallback because the - class fingerprint is a tiebreaker, not a primary reconciliation key. + Composite of (rule_id, vuln-class token). The token comes from + ``_class_token``, which prefers the structured ``finding_class`` and + falls back to a title keyword — see that helper for why the structured + field must win (CSPT vs. server-side traversal both key on CWE-22). """ - title = _string_value(report.get("title")) or "" - keyword = _class_keyword(title) if title else "" - if not keyword: + token = _class_token(report) + if not token: return None - composite = f"rule:{rule_id}|class:{keyword}" + composite = f"rule:{rule_id}|class:{token}" return hashlib.sha256(composite.encode("utf-8")).hexdigest() +def _class_token(report: dict[str, Any]) -> str: + """Resolve a finding's vulnerability-class token for fingerprinting. + + Prefers the structured ``finding_class`` when it carries signal (present + and not the default ``dynamic``); otherwise falls back to a keyword from + the (LLM-authored) title. Returns an empty string when neither yields a + token. + + Shared by ``_class_fingerprint`` (file-rename dismissal carryover) and the + synthetic branch of ``_primary_fingerprint`` (locationless distinguisher) + so both fingerprints agree on what counts as "the same class". This is what + stops a CSPT finding whose title contains the generic ``path traversal`` + keyword from collapsing onto a server-side path-traversal finding's primary + fingerprint: the structured field distinguishes them and both hashes + consume it. + """ + finding_class = _string_value(report.get("finding_class")) + if finding_class and finding_class != "dynamic": + return finding_class + title = _string_value(report.get("title")) or "" + return _class_keyword(title) if title else "" + + def _class_keyword(title: str) -> str: """Pick the first matching curated keyword in ``title``, or fall back to the first 5 lowercased alpha-numeric words. diff --git a/strix/skills/README.md b/strix/skills/README.md index 87a48ddf..e28c96bf 100644 --- a/strix/skills/README.md +++ b/strix/skills/README.md @@ -46,6 +46,7 @@ Notable source-aware skills: - `semantic_confusion` (vulnerabilities): cross-boundary parser, normalization, and representation mismatch analysis - `agentic_system_security` (vulnerabilities): effective-authority and MCP/tool ecosystem security testing - `browser_security` (vulnerabilities): browsing-context, postMessage, XS-Leaks, service-worker, and cross-origin state-machine testing +- `client_side_path_traversal` (vulnerabilities): exploitable client-side path traversal — route/query/hash into fetch/router/BFF request, escalated by sink - `azure` (cloud): Azure and Microsoft Entra privilege, PIM, workload identity, and cross-plane escalation analysis - `infrastructure_lifecycle` (reconnaissance): abandoned or mutable external dependencies such as update endpoints, MX, storage, and control domains - `argument_injection` (vulnerabilities): shell-free CLI option smuggling, secondary argument-file parsing, and platform-specific argv transformation boundaries diff --git a/strix/skills/analysis/source_aware_discovery.md b/strix/skills/analysis/source_aware_discovery.md index ef13cde8..2391e937 100644 --- a/strix/skills/analysis/source_aware_discovery.md +++ b/strix/skills/analysis/source_aware_discovery.md @@ -117,6 +117,22 @@ row because the fetch is an intended feature, because the filter is operator-configured or empty by default, or because it only runs pre-request. +**Client-side request construction.** Enumerate every place a route +segment, query value, or URL hash flows into a client request URL — +`fetch`/XHR/axios or any other HTTP client — into a client-router +navigation, or into a server-side/BFF fetch whose path the client +shapes. Keep both lines candidate-visible: the decode/normalize step +(where `../`, `%2f`, `%2e`, a backslash, or double-encoded forms survive) +and the request-construction sink that consumes the tainted path. +Promote only when the traversed request has a security-relevant effect — +a state change (CSRF-like), a response rendered or executed in an unsafe +sink (XSS), a server-side fetch reaching an internal/privileged target +(SSRF), an authorization or permission bypass, or disclosure of another +user's or tenant's data. A router or framework that merely decodes `../` +with no request sink, or a sink whose redirected request has no +security-relevant effect, is a false positive — do not report it. Point +to `client_side_path_traversal`. + **Command and action runners.** Enumerate every attacker-controllable argument type and execution mode before you call command injection covered. Type-safety maps, unsafe-type denylists, template substitution, diff --git a/strix/skills/custom/source_aware_sast.md b/strix/skills/custom/source_aware_sast.md index fee2fd92..dcff75ce 100644 --- a/strix/skills/custom/source_aware_sast.md +++ b/strix/skills/custom/source_aware_sast.md @@ -176,6 +176,35 @@ lighter syntax/anti-pattern check when ESLint is over-eager. The `JS-Snooper` / `jsniper.sh` tools (in `katana.md`) are the right next step to mine those bundles for endpoint candidates. +### Route-Param-Into-Request (Client-Side Path Traversal) + +`eslint --rule no-eval` and `retire` will NOT surface this — neither +flags tainted-path flow or framework routing. Grep the router inputs and +the request sinks, then pair them by hand: + +```bash +# Router / query / hash inputs +grep -rnE 'useParams|useSearchParams|useLocation|useRouter|usePathname' src/ # React/Next +grep -rnE 'route\.params|route\.query|\$route|ActivatedRoute|paramMap' src/ # Vue, Angular + +# Request sinks that may consume a tainted path +grep -rnE 'fetch\(|axios\.|\.get\(|\.post\(|new URL\(' src/ +``` + +Pair the source with the sink structurally with ast-grep: + +```bash +sg run --pattern 'fetch($URL)' +sg run --pattern 'axios.get($URL)' +``` + +A route/query/hash value that reaches the URL argument (directly or via a +string-interpolated URL) without re-encoding is a candidate. Include +Next.js route handlers (`app/api/**/route.ts`) that forward a +client-supplied path segment into a server-side `fetch` — the traversal +then runs server-side (SSRF / internal reach). Point to +`client_side_path_traversal`. + ## Converting Static Signals Into Exploits When source contains model-provider SDKs, prompt templates, retrieval/vector stores, tool/function calling, model loading, training/feedback pipelines, or token/agent-loop accounting, load `llm_applications`. Use its OWASP 2026 LLM01-LLM10 map to trace data provenance, model output, retrieval authorization, tool authority, and resource multipliers rather than treating the provider call as the sink. diff --git a/strix/skills/frameworks/nextjs.md b/strix/skills/frameworks/nextjs.md index 4bdb0be9..a95ae2bf 100644 --- a/strix/skills/frameworks/nextjs.md +++ b/strix/skills/frameworks/nextjs.md @@ -200,6 +200,15 @@ Look for `_metadata`, `_internal`, `__typename` (GraphQL), nested sensitive obje Server vs client render differences can enable gadget-based XSS. +**Client-Side Path Traversal** + +`params`/`searchParams` (or a client-supplied path forwarded by a Route +Handler / BFF) used to build a `fetch` or `router.push`/navigation URL is +a client-side path traversal surface — decoded `../`/`%2f` can repoint +the request to another endpoint. Treat it as a finding only when the +repointed request has a security-relevant effect (CSRF-like / XSS / SSRF +/ authz / cross-tenant); load `client_side_path_traversal`. + ### Draft/Preview Mode - Secret URLs/cookies enabling preview diff --git a/strix/skills/scan_modes/deep.md b/strix/skills/scan_modes/deep.md index af2b85dd..c4b55fce 100644 --- a/strix/skills/scan_modes/deep.md +++ b/strix/skills/scan_modes/deep.md @@ -102,6 +102,7 @@ Test every input vector with every applicable technique. - Cache poisoning and cache deception - Subdomain takeover - Prototype pollution (JavaScript applications) +- Client-side path traversal: client-rendered front end (SPA in React/Vue/Angular/Svelte/Next.js, or any page building a fetch/XHR/axios URL or client router navigation from the URL path, query, or hash) → load `client_side_path_traversal` and test for EXPLOITABLE CSPT (a decoded `../` reaching no request sink with real impact is a false positive) - CORS misconfiguration exploitation - WebSocket security testing - GraphQL-specific attacks (introspection, batching, nested queries) diff --git a/strix/skills/scan_modes/diff.md b/strix/skills/scan_modes/diff.md index 49a7d14c..eb44c85a 100644 --- a/strix/skills/scan_modes/diff.md +++ b/strix/skills/scan_modes/diff.md @@ -29,6 +29,12 @@ Also in scope, and routinely missed: - A behavioral change that invalidates an assumption elsewhere: a type loosened, a default flipped, a validator made optional, an error path changed from reject to log-and-continue. +- A change that adds or alters client-side URL or route construction + from user-controlled URL input (path, query, or hash) feeding a + fetch/XHR/axios call or client router navigation — a + CSPT-introduction candidate. Load `client_side_path_traversal` and + confirm an EXPLOITABLE sink; a decoded `../` reaching no request sink + with real impact is a false positive. **Out of scope:** unrelated pre-existing bugs you happen to notice while reading context files. Note them, do not file them against this PR. The diff --git a/strix/skills/scan_modes/quick.md b/strix/skills/scan_modes/quick.md index a8824786..a66eec5c 100644 --- a/strix/skills/scan_modes/quick.md +++ b/strix/skills/scan_modes/quick.md @@ -38,6 +38,7 @@ Test in priority order: 4. **SQL injection** - authentication endpoints, search, filters 5. **SSRF** - URL parameters, webhooks, integrations 6. **Exposed secrets** - hardcoded credentials, API keys, config files +7. **Client-side path traversal** - client-rendered front end (SPA in React/Vue/Angular/Svelte/Next.js, or any page building a fetch/XHR/axios URL or client router navigation from the URL path, query, or hash) → load `client_side_path_traversal` and test for EXPLOITABLE CSPT; a decoded `../` reaching no request sink with real impact is a false positive Skip for quick scans: - Exhaustive subdomain enumeration diff --git a/strix/skills/scan_modes/standard.md b/strix/skills/scan_modes/standard.md index b7a37395..2941c680 100644 --- a/strix/skills/scan_modes/standard.md +++ b/strix/skills/scan_modes/standard.md @@ -51,6 +51,7 @@ Test each attack surface methodically. Spawn focused subagents for different are - File upload bypass attempts - Search and filter parameter manipulation - Redirect and URL parameter handling +- Client-side path traversal: client-rendered front end (SPA in React/Vue/Angular/Svelte/Next.js, or any page building a fetch/XHR/axios URL or client router navigation from the URL path, query, or hash) → load `client_side_path_traversal` and test for EXPLOITABLE CSPT (a decoded `../` reaching no request sink with real impact is a false positive) **Authentication & Session** - Brute force protection diff --git a/strix/skills/vulnerabilities/browser_security.md b/strix/skills/vulnerabilities/browser_security.md index f76ffc64..a8b35e9a 100644 --- a/strix/skills/vulnerabilities/browser_security.md +++ b/strix/skills/vulnerabilities/browser_security.md @@ -1,6 +1,6 @@ --- name: browser-security -description: Browser-internals security testing for browsing-context relationships, postMessage, client-side path traversal, XS-Leaks, service workers, Web Workers, navigation behavior, CSP interactions, caches, and cross-origin state machines +description: Browser-internals security testing for browsing-context relationships, postMessage, client-side path traversal (SPA fetch-or-router URL built from a URL path, query, or hash), XS-Leaks, service workers, Web Workers, navigation behavior, CSP interactions, caches, and cross-origin state machines --- # Browser Security @@ -45,6 +45,8 @@ Draw the context graph. Security checks on `event.origin`, `event.source`, or a ### Client-Side Path Traversal +For the full exploitable-CSPT discovery/escalation/validation workflow, load `client_side_path_traversal`. + Trace the complete source-to-request pipeline: ```text diff --git a/strix/skills/vulnerabilities/client_side_path_traversal.md b/strix/skills/vulnerabilities/client_side_path_traversal.md new file mode 100644 index 00000000..611a87ef --- /dev/null +++ b/strix/skills/vulnerabilities/client_side_path_traversal.md @@ -0,0 +1,128 @@ +--- +name: client-side-path-traversal +description: Client-side path traversal (CSPT) testing in SPAs — React Router, Vue Router, Angular router, Next.js client navigation — where a route/query/hash parameter flows into a fetch/XHR/axios URL, a client router navigation, or a BFF proxy path +--- + +# Client-Side Path Traversal + +Client-Side Path Traversal (CSPT) is a traversal where the attacker-controlled source is a browser-visible part of the URL (a path segment, query value, or hash) and the sink is a request the page builds or shapes — a `fetch`/XHR/axios call, a client-router navigation, or a server-side/BFF fetch whose path the client supplies. There is no filesystem and no local file read: the `../`, `%2f`, `%2e`, or backslash survives decoding/normalization and rewrites the *request path* the browser (or the BFF on its behalf) ultimately issues. Treat it as a request-redirection primitive inside the app, not as server-side path traversal; the impact comes from where the redirected request lands and what it does. + +## Exploitability Bar + +CSPT is a finding only when ALL THREE hold: + +1. **Controlled source survives.** A browser-controlled source — a URL path segment, query value, or hash — is decoded/normalized so that path syntax (`../`, `%2f`, `%2e`, backslash, double-encoded forms) still carries into the next stage instead of being stripped or re-encoded. +2. **Reaches a request sink.** That value reaches a request-construction sink in the page: `fetch`/XHR/axios/another HTTP client, OR a client-router navigation, OR a server-side/BFF fetch whose path the client controls. +3. **Produces real impact.** The resulting request has a security-relevant effect: a state change (CSRF-like), a response rendered/executed in an unsafe sink (XSS), a server-side fetch reaching an internal/privileged target (SSRF), an authorization/permission bypass, or disclosure of another user's or tenant's data. + +**False-positive rule:** a router or framework that merely decodes `../` with NO request sink, or a sink whose redirected request has NO security-relevant effect, is a FALSE POSITIVE — do not report it. This mirrors the repo-wide closure discipline (`confirmed` / `ruled_out` / `open_proof_gap`) and the `counterevidence` pass: no proven sink and impact means `open_proof_gap`, not a finding. + +## Source -> Sink -> Impact + +Trace the whole pipeline before claiming anything: + +```text +browser URL -> router parser -> route/query/hash accessor -> app URL construction -> fetch/XHR/router -> final normalized request +``` + +Each decode or re-encode along that chain is a decision point: confirm the traversal is still present in the request that actually leaves the page. + +Sink taxonomy and the impact it maps to: + +- **State-changing API call** (`POST`/`PUT`/`PATCH`/`DELETE` built from the source) -> CSRF-like unintended action on a different resource. +- **Response rendered/executed in the DOM** (traversed response body into `innerHTML`, template, `eval`, script/JSON-driven render) -> XSS. +- **Server-side / BFF fetch** keyed on a client-supplied path segment -> SSRF toward internal/metadata/privileged targets. +- **Authorization/permission check keyed on the path** (resource id, tenant slug, role segment) -> authz bypass or cross-tenant disclosure of another user's/tenant's data. + +One source can reach several sinks; classify by the strongest proven impact, not the easiest to trigger. + +Minimal shape of the primitive — a profile viewer whose id comes from the route: + +```text +app route: /users/ +app builds: fetch(`/api/users/${id}`) +attacker id: ../../admin/settings (or ..%2f..%2fadmin%2fsettings) +final req: GET /api/admin/settings <- traversal survived into the sink +``` + +Whether this is a finding depends entirely on what `/api/admin/settings` does for the current principal — map that before reporting. + +## Discovery -- black-box (runtime) + +Instrument in a controlled browser session to capture the FINAL request shape (method + origin + path only — never bodies, credentials, or query strings): + +```javascript +const realFetch = window.fetch; +window.fetch = (...args) => { + const input = args[0]; + const raw = typeof input === 'string' ? input : input.url; + const u = new URL(raw, location.href); + const method = args[1]?.method || input?.method || 'GET'; + console.log('fetch', {method, origin: u.origin, path: u.pathname}); + return realFetch(...args); +}; +const realOpen = XMLHttpRequest.prototype.open; +XMLHttpRequest.prototype.open = function (method, url, ...rest) { + const u = new URL(url, location.href); + console.log('xhr', {method, origin: u.origin, path: u.pathname}); + return realOpen.call(this, method, url, ...rest); +}; +``` + +Restore `window.fetch = realFetch` and the XHR prototype after validation. This adapts the wrapper in `browser_security`; load that skill for the full browser-state model and router-decoding notes. The wrapper captures `fetch` and XHR only — a pure client-router navigation sink (`history.pushState`, `router.push`) issues no HTTP request from this hook, so watch those in Caido's outbound capture or wrap the history/router API separately. + +Then vary the route/query/hash independently with `%2f`, `%2e`, `%5c`, raw dot-segments (`../`, `..%2f`), and double-encoded forms (`%252f`, `%252e`). For each source, read the logged final path and determine exactly where the sequence decodes, is removed, or re-encodes. Path parameters, query values, and fragments are commonly transformed differently — test each on its own. agent-browser (see `tooling/agent_browser`) runs in the sandbox with Caido capturing the final request, so compare the console log against the captured outbound request. + +## Discovery -- white-box (source) + +Find a route/query/hash value flowing into a request URL. Grep or `ast-grep` per framework: + +- **React:** `useParams`, `useSearchParams`, `useLocation`, `useRouter` whose result is interpolated into `fetch(`, `axios`, `.get(`/`.post(`. + ```bash + rg -n "useParams|useSearchParams|useLocation|useRouter" --glob '*.{ts,tsx,js,jsx}' -A4 | rg "fetch\(|axios|\.(get|post|put|patch|delete)\(" + ``` +- **Vue:** `useRoute`, `route.params`, `route.query`, `$route` feeding an HTTP client (`fetch`, `axios`, `$http`). +- **Angular:** `ActivatedRoute`, `paramMap`, `snapshot.params`/`snapshot.queryParams` feeding `HttpClient.get/post`. +- **Svelte/SvelteKit:** `$page.params`, `$page.url.searchParams`, and `load({ params, url })` whose value is interpolated into `fetch(`/`axios`, or routed through a `+server.ts`/`+page.server.ts` handler that forwards the client-supplied segment. +- **Generic:** string-interpolated URL construction — `` `${base}/${param}` ``, `url + param`, `new URL(param, base)`, path `.join`/concat — where `param` is any router-derived value. + +Call out BFF and Next.js route handlers (`app/**/route.ts`, `pages/api/*`) that forward a client-supplied path into a server `fetch`; those turn CSPT into SSRF. See `nextjs` for RSC/BFF path forwarding. The same framework API can decode differently in client components, server components, and route handlers — do not assume one behavior. + +## Escalation by sink + +Prove the impact-class-specific fact before calling it exploitable: + +- **CSRF-like:** show the redirected request hit a *different*, state-changing endpoint and the state change occurred (not merely a 404/redirect). Include a benign-path control. +- **XSS:** show the traversed response body reached an unsafe render sink AND executed (script ran / handler fired) — a reflected string that never executes is not XSS. +- **SSRF:** show the server-side/BFF fetch reached an internal or metadata target (e.g. a controlled internal canary or `169.254.169.254`) and returned or acted on that response; a same-origin public redirect is not SSRF. +- **Authz bypass / cross-tenant:** show the request returned or mutated a resource the current principal must not access (another user/tenant id), with a paired same-principal control that is denied or scoped correctly. + +## Validation + +1. Capture the final network request (method + origin + path) AND the security-relevant response or action it caused. +2. Run a paired control: a benign path next to the traversed path, same session and state, so the difference is attributable to the traversal and not to unrelated behavior. +3. Provide a minimal PoC — the smallest source value that redirects the request and the single observed effect. Do not paste large payload catalogs. +4. Close under the repo discipline: a proven source+sink+impact is `confirmed`; a decode with no reachable sink, or a sink with no security-relevant effect, is `ruled_out` (name the control) or `open_proof_gap` (name the missing proof). Run the `counterevidence` pass before reporting. + +## False Positives + +- Decode/normalization of `../` with no request sink downstream — the value never shapes a request. +- A sink whose redirected request has no security-relevant effect (static asset, idempotent read of already-authorized data, dead/404 path). +- Same-origin-only redirection to an equally-authorized endpoint — the principal could already call it directly. +- A framework that re-encodes or segment-normalizes before the sink, so the traversal does not survive into the final request. + +## Pro Tips + +1. The attacker-supplied string is not evidence; the browser-parsed final request path is. Always read the logged `origin` + `path`, never the raw input. +2. Query and hash values are usually decoded automatically; path segments vary by router and by execution context. Test each source separately. +3. Double-encoding (`%252f`) often survives one decode layer that strips single-encoded forms — probe it whenever a single `%2f` is removed. +4. A BFF/route-handler hop is the difference between a same-origin dead end and SSRF; trace whether the server fetch inherits the client-supplied segment. +5. No sink and no impact is `open_proof_gap`, never a finding. Name the missing proof instead of reporting the decode. +6. Prefer a one-line minimal PoC and a paired control over a payload catalog; the reusable unit is the source-to-sink pipeline, not the string. + +## Cross-links + +- `browser_security` — browser-state model, router-decoding notes, runtime instrumentation this skill adapts. +- `semantic_confusion` — decode/normalization differentials across router vs server when the two disagree on the same bytes. +- `open_redirect`, `csrf`, `xss`, `ssrf` — the escalation sinks; use the matching skill once the impact class is known. +- `nextjs` — RSC/BFF route handlers that forward a client-supplied path into a server fetch. diff --git a/strix/skills/vulnerabilities/csrf.md b/strix/skills/vulnerabilities/csrf.md index 10837d63..ae750967 100644 --- a/strix/skills/vulnerabilities/csrf.md +++ b/strix/skills/vulnerabilities/csrf.md @@ -152,6 +152,7 @@ Cross-site request forgery abuses ambient authority (cookies, HTTP auth) across - CSRF + IDOR: force actions on other users' resources once references are known - CSRF + Clickjacking: guide user interactions to bypass UI confirmations - CSRF + OAuth mix-up: bind victim sessions to unintended clients +- CSRF + client-side path traversal: a browser-controlled path can retarget a same-origin, credentialed request to a state-changing endpoint, defeating assumptions that only the intended path is called — see `client_side_path_traversal` ## Testing Methodology diff --git a/strix/skills/vulnerabilities/path_traversal_lfi_rfi.md b/strix/skills/vulnerabilities/path_traversal_lfi_rfi.md index 6de0b0e0..2dfb009d 100644 --- a/strix/skills/vulnerabilities/path_traversal_lfi_rfi.md +++ b/strix/skills/vulnerabilities/path_traversal_lfi_rfi.md @@ -13,6 +13,9 @@ Improper file path handling and dynamic inclusion enable sensitive file disclosu - Read files outside intended roots via `../`, encoding, normalization gaps - Write or create files outside intended roots, then evaluate framework-controlled resolution paths separately from direct web access +**Client-Side Variant** +- A browser-controlled path builds a client-issued request (fetch/XHR/router navigation) rather than a filesystem path — see `client_side_path_traversal` + **Local File Inclusion (LFI)** - Include server-side files into interpreters/templates @@ -194,7 +197,7 @@ Test search order with candidate marker files or filesystem traces. Trigger the ## False Positives -- In-app virtual paths that do not map to filesystem; content comes from safe stores (DB/object storage) +- In-app virtual paths that do not map to filesystem; content comes from safe stores (DB/object storage). When a browser-controlled path segment builds a client-side fetch/XHR/router URL (or a server-side/BFF fetch the client shapes), that is client-side path traversal and may be exploitable — see `client_side_path_traversal` — not an automatic false positive - Canonicalized paths constrained to an allowlist/root after normalization - Wrappers disabled and includes using constant templates only - Archive extractors that sanitize paths and enforce destination directories diff --git a/strix/skills/vulnerabilities/ssrf.md b/strix/skills/vulnerabilities/ssrf.md index f0fc66bd..d9b49b6e 100644 --- a/strix/skills/vulnerabilities/ssrf.md +++ b/strix/skills/vulnerabilities/ssrf.md @@ -157,11 +157,11 @@ Server-Side Request Forgery enables the server to reach networks and services th ## False Positives -- Client-side fetches only (no server request) +- Client-side fetches only (no server request) — but when a server-side/BFF proxy forwards the browser-controlled path that built that client fetch to an internal target, it is CSPT-to-SSRF; see `client_side_path_traversal` - Strict allowlists with DNS pinning and no redirect following - SSRF simulators/mocks returning canned responses without real egress - Blocked egress confirmed by uniform errors across all targets and protocols -- OAST callbacks where the source IP matches the tester's machine, not the server — the browser or a client-side fetch made the request, not the backend +- OAST callbacks where the source IP matches the tester's machine, not the server — the browser or a client-side fetch made the request, not the backend (unless a server-side/BFF proxy then forwards the client-shaped path to an internal target: CSPT-to-SSRF; see `client_side_path_traversal`) ## Impact diff --git a/strix/skills/vulnerabilities/xss.md b/strix/skills/vulnerabilities/xss.md index a17db5bb..2b66d526 100644 --- a/strix/skills/vulnerabilities/xss.md +++ b/strix/skills/vulnerabilities/xss.md @@ -61,6 +61,7 @@ Distinguish HTML-string input from live DOM objects sanitized with `IN_PLACE`. R **Sources** - `location.*` (hash/search), `document.referrer`, postMessage, storage, service worker messages +- A URL path/query/hash segment used to build a fetch/navigation URL whose response is rendered in an unsafe sink (client-side path traversal → XSS) — see `client_side_path_traversal` **Sinks** - `innerHTML`/`outerHTML`/`insertAdjacentHTML`, `document.write` diff --git a/strix/tools/coverage/tools.py b/strix/tools/coverage/tools.py index 94c31fb9..8c019738 100644 --- a/strix/tools/coverage/tools.py +++ b/strix/tools/coverage/tools.py @@ -292,6 +292,12 @@ def _update_impl( superseded["evidence"] = existing["evidence"] if existing.get("agent_name"): superseded["agent_name"] = existing["agent_name"] + # Preserve the superseded author's id, not just their name: a later + # update by a different agent overwrites ``agent_id``, and coverage + # accounting attributes a bundled skill's surface to the agent that + # actually assessed it, so the earlier authorship has to survive. + if existing.get("agent_id"): + superseded["agent_id"] = existing["agent_id"] history = existing.get("history") existing["history"] = [*history, superseded] if isinstance(history, list) else [superseded] diff --git a/strix/tools/reporting/tool.py b/strix/tools/reporting/tool.py index 791bae79..e1ecef43 100644 --- a/strix/tools/reporting/tool.py +++ b/strix/tools/reporting/tool.py @@ -16,6 +16,8 @@ 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 @@ -803,6 +805,7 @@ async def _do_create( confidence_rationale: str | None = None, fix_verification: str | None = None, fix_pr_body: str | None = None, + finding_class: str | None = None, agent_id: str | None = None, agent_name: str | None = None, ) -> dict[str, Any]: @@ -850,6 +853,31 @@ async def _do_create( ) errors.extend(http_exchange_errors) + finding_class = (finding_class or "dynamic").strip().lower() + if finding_class not in _VALID_FINDING_CLASSES: + errors.append( + f"Invalid finding_class: {finding_class!r}. " + f"Must be one of: {sorted(_VALID_FINDING_CLASSES)}" + ) + 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. 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} @@ -883,6 +911,7 @@ async def _do_create( "poc_script_code": poc_script_code, "endpoint": endpoint, "method": method, + "finding_class": finding_class, } report_fields: dict[str, Any] = { "title": title, @@ -911,6 +940,7 @@ async def _do_create( "fix_verification": fix_verification, "fix_pr_body": fix_pr_body, "http_exchange_ids": normalized_http_exchange_ids, + "finding_class": finding_class, } dedupe = await check_duplicate(candidate, existing) @@ -1005,6 +1035,7 @@ async def create_vulnerability_report( confidence_rationale: str | None = None, fix_verification: str | None = None, fix_pr_body: str | None = None, + finding_class: str | None = None, ) -> str: """File a vulnerability report — one report per fully-verified finding. @@ -1410,6 +1441,22 @@ async def create_vulnerability_report( fix (summary + rationale). Prose/markdown only — the code change itself belongs in ``code_locations``. Omit for black-box findings. + finding_class: Optional machine-readable sub-class, default + ``"dynamic"``. Set ``"client_side_path_traversal"`` for a + confirmed CSPT finding — attacker-controlled client input + (URL path/query/fragment, ``document.referrer``, + ``postMessage`` data, browser storage) reaching a browser + request sink (``fetch`` / ``XMLHttpRequest`` / ``axios`` / + ``EventSource`` / ``WebSocket``) so the victim's own + authenticated browser is steered to an unintended + same-origin endpoint. CSPT shares ``CWE-22`` with + server-side path traversal / LFI / RFI but is a distinct + class; setting this keeps it separated in the report + artifacts and SARIF (do NOT use it for server-side + traversal — leave those ``dynamic``). A CSPT report must + also carry a discriminator: ``endpoint`` (the traversed + target path) and ``method``, or a ``code_location`` for the + client sink. See the ``client_side_path_traversal`` skill. """ ( http_exchange_ids, @@ -1454,6 +1501,7 @@ async def create_vulnerability_report( http_exchange_ids=http_exchange_ids, fix_verification=fix_verification, fix_pr_body=fix_pr_body, + finding_class=finding_class, agent_id=agent_id, agent_name=agent_name, ) @@ -2325,7 +2373,13 @@ _SEVERITY_ORDER = { "none": 5, } _VALID_SEVERITIES = frozenset(_SEVERITY_ORDER) -_VALID_FINDING_CLASSES = frozenset({"dynamic", "dependency_cve"}) +# ``dynamic`` (the PoC-backed default) and ``dependency_cve`` (reserved for +# create_dependency_report) are report-type markers; the remaining selectable +# classes come from the canonical vulnerability-class registry so this set +# never drifts from the class definitions. ``client_side_path_traversal`` is +# the first registry-sourced member — machine-separable from server-side +# path traversal / LFI / RFI, which share CWE-22. +_VALID_FINDING_CLASSES = frozenset({"dynamic", "dependency_cve"}) | selectable_finding_classes() _REPORT_DESCRIPTION_PREVIEW_CHARS = 280 # Compact, listing-safe fields — no full bodies / PoC code / evidence. diff --git a/tests/fixtures/cspt/README.md b/tests/fixtures/cspt/README.md new file mode 100644 index 00000000..75c7bfc0 --- /dev/null +++ b/tests/fixtures/cspt/README.md @@ -0,0 +1,19 @@ +# CSPT fixtures + +Minimal HTML pages exercising client-side path traversal (CSPT) source to sink +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_*` has the CSPT **primitive** — an attacker-controlled client +source (URL query, fragment, referrer, postMessage, storage) reaches a request +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 +tainted flow. The sources, sinks, and mitigation markers the matcher keys on +are listed at the top of `tests/test_cspt_fixtures.py`. diff --git a/tests/fixtures/cspt/negative_hardcoded.html b/tests/fixtures/cspt/negative_hardcoded.html new file mode 100644 index 00000000..aeaf65ee --- /dev/null +++ b/tests/fixtures/cspt/negative_hardcoded.html @@ -0,0 +1,20 @@ + + + + Profile + +

+ 

+    
+  
+
diff --git a/tests/fixtures/cspt/negative_validated.html b/tests/fixtures/cspt/negative_validated.html
new file mode 100644
index 00000000..dca42541
--- /dev/null
+++ b/tests/fixtures/cspt/negative_validated.html
@@ -0,0 +1,20 @@
+
+
+
+  Item
+  
+    

+    
+  
+
diff --git a/tests/fixtures/cspt/positive_hash_route.html b/tests/fixtures/cspt/positive_hash_route.html
new file mode 100644
index 00000000..b2b89570
--- /dev/null
+++ b/tests/fixtures/cspt/positive_hash_route.html
@@ -0,0 +1,20 @@
+
+
+
+  Item
+  
+    

+    
+  
+
diff --git a/tests/fixtures/cspt/positive_query_param.html b/tests/fixtures/cspt/positive_query_param.html
new file mode 100644
index 00000000..501fbb60
--- /dev/null
+++ b/tests/fixtures/cspt/positive_query_param.html
@@ -0,0 +1,20 @@
+
+
+
+  Document viewer
+  
+    

+    
+  
+
diff --git a/tests/test_cspt_finding_class.py b/tests/test_cspt_finding_class.py
new file mode 100644
index 00000000..a06b4e6a
--- /dev/null
+++ b/tests/test_cspt_finding_class.py
@@ -0,0 +1,79 @@
+"""CSPT ``finding_class`` separation: registry sourcing, SARIF, dedup.
+
+Client-side path traversal shares CWE-22 with server-side path traversal /
+LFI / RFI, so ``ruleId`` alone cannot tell them apart. These tests pin the
+machine-readable separation that keeps them distinct in the report artifacts —
+pure-Python, no browser or network, so they run in ordinary CI.
+"""
+
+from __future__ import annotations
+
+from strix.report.coverage import selectable_finding_classes
+from strix.report.dedupe import _prepare_report_for_comparison
+from strix.report.sarif import _class_token, _primary_fingerprint
+
+
+_SECURITY_MD = [{"physicalLocation": {"artifactLocation": {"uri": "SECURITY.md"}}}]
+
+
+def test_class_token_prefers_structured_finding_class() -> None:
+    """The structured field wins over the (LLM-authored) title keyword."""
+    assert (
+        _class_token(
+            {"finding_class": "client_side_path_traversal", "title": "Path traversal in fetch"}
+        )
+        == "client_side_path_traversal"
+    )
+    # default 'dynamic' carries no signal, so fall back to the title keyword
+    assert _class_token({"finding_class": "dynamic", "title": "Path traversal in download"}) == (
+        "path traversal"
+    )
+    assert _class_token({"title": "Reflected XSS in q"}) == "xss"
+
+
+def test_cspt_and_server_side_cwe22_get_distinct_primary_fingerprints() -> None:
+    """The reviewer's repro: a locationless CSPT and a server-side traversal,
+    both CWE-22 and both anchored to SECURITY.md, must not collapse onto one
+    SARIF primary fingerprint."""
+    cspt = {
+        "title": "Path traversal via client fetch",
+        "finding_class": "client_side_path_traversal",
+    }
+    server = {"title": "Path traversal in file download", "finding_class": "dynamic"}
+
+    fp_cspt = _primary_fingerprint("CWE-22", cspt, _SECURITY_MD, is_synthetic=True)
+    fp_server = _primary_fingerprint("CWE-22", server, _SECURITY_MD, is_synthetic=True)
+
+    assert fp_cspt and fp_server
+    assert fp_cspt != fp_server
+
+
+def test_same_class_still_reconciles_to_one_fingerprint() -> None:
+    """Two server-side CWE-22 findings of the same class still share a
+    fingerprint — the separation is by class, not by prose."""
+    server_a = {"title": "Path traversal in file download", "finding_class": "dynamic"}
+    server_b = {"title": "Path traversal reading logs", "finding_class": "dynamic"}
+
+    assert _primary_fingerprint(
+        "CWE-22", server_a, _SECURITY_MD, is_synthetic=True
+    ) == _primary_fingerprint("CWE-22", server_b, _SECURITY_MD, is_synthetic=True)
+
+
+def test_valid_finding_classes_are_sourced_from_the_registry() -> None:
+    """The selectable CSPT class comes from VULN_CLASSES, not a hand-maintained
+    list in the reporting tool."""
+    from strix.tools.reporting.tool import _VALID_FINDING_CLASSES
+
+    assert "client_side_path_traversal" in selectable_finding_classes()
+    assert "client_side_path_traversal" in _VALID_FINDING_CLASSES
+    assert {"dynamic", "dependency_cve"} <= _VALID_FINDING_CLASSES
+
+
+def test_dedupe_comparison_includes_finding_class() -> None:
+    """Dedup sees finding_class, so two CWE-22 findings of different classes are
+    not judged duplicates."""
+    cleaned = _prepare_report_for_comparison(
+        {"id": "1", "title": "x", "finding_class": "client_side_path_traversal"}
+    )
+
+    assert cleaned.get("finding_class") == "client_side_path_traversal"
diff --git a/tests/test_cspt_fixtures.py b/tests/test_cspt_fixtures.py
new file mode 100644
index 00000000..9126eefb
--- /dev/null
+++ b/tests/test_cspt_fixtures.py
@@ -0,0 +1,73 @@
+"""Deterministic, browser-free regression over the CSPT fixtures.
+
+This checks one thing only: whether an attacker-controlled client source reaches
+a request sink's path **unguarded** — the CSPT *primitive*. It deliberately does
+NOT decide exploitability: a reached sink is not yet a finding. Whether the
+redirected request has security impact (a state change, a rendered response, an
+internal fetch, an authz bypass) is proven separately, per the exploitability bar
+in the `client_side_path_traversal` skill. So a passing test here means "the
+primitive is present/absent as labelled", not "exploitable".
+
+It is a fixture/pattern regression, not a static taint tracker, kept honest by a
+convention: the attacker value that reaches a sink is named ``userPath``.
+"""
+
+from __future__ import annotations
+
+from pathlib import Path
+
+
+_FIXTURES = Path(__file__).parent / "fixtures" / "cspt"
+
+#: Browser-controlled input sources an attacker influences.
+_SOURCES = (
+    "location.search",
+    "URLSearchParams",
+    "location.hash",
+    "document.referrer",
+    "postMessage",
+    "localStorage",
+)
+#: Request sinks whose path, if attacker-shaped, carries the traversal.
+_SINKS = ("fetch(", "XMLHttpRequest", "axios", "EventSource", "WebSocket")
+#: Markers of a guard that neutralises the flow before the sink.
+_MITIGATIONS = ("/^", ".test(", "allowlist", "hardcoded")
+
+
+def _reaches_sink_unguarded(text: str) -> bool:
+    """True when the attacker value (``userPath``) reaches a sink-call line."""
+    return any(
+        any(sink in line for sink in _SINKS) and "userPath" in line
+        for line in text.splitlines()
+    )
+
+
+def _detect_primitive(text: str) -> str:
+    """"reaches_sink" when an unguarded attacker source reaches a sink path.
+
+    This is the CSPT primitive, not an exploitability verdict — impact is a
+    separate, out-of-band step the skill describes.
+    """
+    has_source = any(token in text for token in _SOURCES)
+    has_sink = any(token in text for token in _SINKS)
+    guarded = any(token in text for token in _MITIGATIONS)
+    if has_source and has_sink and _reaches_sink_unguarded(text) and not guarded:
+        return "reaches_sink"
+    return "no_unguarded_sink"
+
+
+def test_cspt_fixtures_match_their_labelled_primitive() -> None:
+    fixtures = sorted(_FIXTURES.glob("*.html"))
+    assert fixtures, "no CSPT fixtures found"
+
+    positives = [f for f in fixtures if f.name.startswith("positive_")]
+    negatives = [f for f in fixtures if f.name.startswith("negative_")]
+    assert len(positives) >= 2, "need at least two primitive-present fixtures"
+    assert len(negatives) >= 2, "need at least two guarded / no-flow controls"
+
+    for fixture in fixtures:
+        # positive_* = the unguarded source-to-sink primitive is present
+        # (exploitability is proven separately); negative_* = guarded or no flow.
+        expected = "reaches_sink" if fixture.name.startswith("positive_") else "no_unguarded_sink"
+        actual = _detect_primitive(fixture.read_text(encoding="utf-8"))
+        assert actual == expected, f"{fixture.name}: expected {expected}, matched {actual}"
diff --git a/tests/test_report_coverage.py b/tests/test_report_coverage.py
index 76200edf..50cf30fc 100644
--- a/tests/test_report_coverage.py
+++ b/tests/test_report_coverage.py
@@ -7,7 +7,9 @@ from typing import TYPE_CHECKING, Any
 
 from strix.report.coverage import (
     _SKILL_PHRASINGS,
+    VULN_CLASSES,
     build_coverage_document,
+    cwe_for_skill,
     read_agent_graph,
     write_coverage,
 )
@@ -84,6 +86,46 @@ def test_assigned_risk_skill_without_coverage_becomes_a_gap() -> None:
     assert [gap["risk_area"] for gap in gaps] == ["sql injection"]
 
 
+def test_substring_match_does_not_hide_a_gap() -> None:
+    """A short class token matches whole words, not substrings: 'brute force' is
+    not RCE coverage, 'enforcement' is not 'rce', 'corridor' is not 'idor'."""
+    cases = [
+        ("rce", "POST /login", "brute force protection"),
+        ("rce", "GET /api/resources/{id}", "object-level authorization"),
+        ("rce", "Access control enforcement", "authorization"),
+        ("idor", "GET /building/corridor/{id}", "information disclosure"),
+    ]
+    for skill, surface, risk in cases:
+        doc = _document(
+            entries=[_entry(surface=surface, risk_area=risk)],
+            agent_graph=_graph(metadata={"agent-1": {"skills": [skill]}}),
+        )
+        classes = [g["risk_area"] for g in doc["gaps"] if g["kind"] == "unrecorded_risk_class"]
+        assert skill.replace("_", " ") in classes, (skill, surface, risk, classes)
+
+
+def test_joined_camelcase_name_counts_as_coverage() -> None:
+    """A row recorded as a joined/CamelCase class name ('DirectoryTraversal')
+    still counts, via the joined-token fallback — no substring matching."""
+    doc = _document(
+        entries=[_entry(risk_area="DirectoryTraversal reviewed", surface="/download")],
+        agent_graph=_graph(metadata={"agent-1": {"skills": ["path_traversal_lfi_rfi"]}}),
+    )
+
+    assert not [g for g in doc["gaps"] if g["kind"] == "unrecorded_risk_class"]
+
+
+def test_simple_plural_still_counts_as_coverage() -> None:
+    """Whole-word matching tolerates a trivial plural, so 'passwords' /
+    'credentials' still cover weak_password_detection."""
+    doc = _document(
+        entries=[_entry(risk_area="weak passwords and credentials reviewed", surface="/login")],
+        agent_graph=_graph(metadata={"agent-1": {"skills": ["weak_password_detection"]}}),
+    )
+
+    assert not [g for g in doc["gaps"] if g["kind"] == "unrecorded_risk_class"]
+
+
 def test_recorded_risk_class_is_not_reported_as_a_gap() -> None:
     doc = _document(
         entries=[_entry(risk_area="SQL injection", surface="GET /search?q=")],
@@ -100,6 +142,148 @@ def test_synonym_phrasing_counts_as_recorded_coverage() -> None:
     assert not [gap for gap in doc["gaps"] if gap["kind"] == "unrecorded_risk_class"]
 
 
+def test_cspt_acronym_phrasing_counts_as_recorded_coverage() -> None:
+    """The ledger spells out "Client-Side Path Traversal (CSPT)"; the skill is
+    called client_side_path_traversal."""
+    doc = _document(
+        entries=[
+            _entry(risk_area="Client-Side Path Traversal (CSPT)", surface="GET /app#/profile")
+        ],
+        agent_graph=_graph(metadata={"agent-1": {"skills": ["client_side_path_traversal"]}}),
+    )
+
+    assert not [gap for gap in doc["gaps"] if gap["kind"] == "unrecorded_risk_class"]
+
+
+def test_bundled_skill_partially_tested_flags_untested_subtopics() -> None:
+    """browser_security bundles many surfaces; proving postMessage must not
+    mark client-side path traversal covered."""
+    doc = _document(
+        entries=[_entry(risk_area="postMessage origin validation", surface="window listener")],
+        agent_graph=_graph(metadata={"agent-1": {"skills": ["browser_security"]}}),
+    )
+
+    flagged = {g["risk_area"] for g in doc["gaps"] if g["kind"] == "unrecorded_sub_topic"}
+    assert "client-side path traversal" in flagged
+    assert "postMessage" not in flagged  # the surface that was tested is not re-flagged
+    # a partially-tested bundled skill is not also reported as a wholesale gap
+    assert not [
+        g
+        for g in doc["gaps"]
+        if g["kind"] == "unrecorded_risk_class" and g["risk_area"] == "browser security"
+    ]
+
+
+def test_bundled_skill_wholly_untested_is_a_single_gap() -> None:
+    """A bundled skill nobody touched collapses to one class-level gap, not one
+    per surface — the honest short statement is "not examined"."""
+    doc = _document(
+        entries=[_entry(risk_area="SQL injection", surface="GET /search")],
+        agent_graph=_graph(metadata={"agent-1": {"skills": ["browser_security"]}}),
+    )
+
+    class_gaps = [
+        g
+        for g in doc["gaps"]
+        if g["kind"] == "unrecorded_risk_class" and g["risk_area"] == "browser security"
+    ]
+    assert len(class_gaps) == 1
+    assert "surfaces" in class_gaps[0]["detail"]
+    assert not [g for g in doc["gaps"] if g["kind"] == "unrecorded_sub_topic"]
+
+
+def test_bundled_skill_with_every_surface_covered_is_clean() -> None:
+    """Covering every surface of a bundled skill leaves no coverage gap."""
+    entries = [
+        _entry(risk_area=sub.phrasings[0], surface=sub.label)
+        for sub in VULN_CLASSES["browser_security"].sub_topics
+    ]
+    doc = _document(
+        entries=entries,
+        agent_graph=_graph(metadata={"agent-1": {"skills": ["browser_security"]}}),
+    )
+
+    assert not [
+        g
+        for g in doc["gaps"]
+        if g["kind"] in {"unrecorded_risk_class", "unrecorded_sub_topic"}
+    ]
+
+
+def test_unrelated_agents_row_does_not_cover_a_browser_subtopic() -> None:
+    """An open_redirect agent's 'meta refresh' row must not mark browser_security's
+    navigation surface covered — only a browser_security carrier's row counts."""
+    doc = _document(
+        entries=[
+            _entry(
+                risk_area="postMessage origin validation",
+                surface="listener",
+                agent_id="agent-1",
+                agent_name="browser-tester",
+            ),
+            _entry(
+                risk_area="open redirect via meta refresh",
+                surface="/login?next=",
+                agent_id="agent-2",
+                agent_name="redirect-tester",
+            ),
+        ],
+        agent_graph=_graph(
+            statuses={"agent-1": "completed", "agent-2": "completed"},
+            names={"agent-1": "browser-tester", "agent-2": "redirect-tester"},
+            metadata={
+                "agent-1": {"skills": ["browser_security"]},
+                "agent-2": {"skills": ["open_redirect"]},
+            },
+        ),
+    )
+
+    sub = {g["risk_area"] for g in doc["gaps"] if g["kind"] == "unrecorded_sub_topic"}
+    assert "navigation and redirect control" in sub  # not masked by the open_redirect row
+    assert "postMessage" not in sub  # the browser agent's own row still counts
+
+
+def test_updated_browser_row_keeps_its_carrier_authorship() -> None:
+    """update_coverage overwrites agent_id with the updater's; a browser surface a
+    carrier assessed must still count via the author preserved in history."""
+    doc = _document(
+        entries=[
+            _entry(
+                risk_area="client-side path traversal",
+                surface="fetch from route param",
+                agent_id="agent-2",  # current author after an unrelated update
+                agent_name="redirect-tester",
+                history=[
+                    {
+                        "outcome": "no_issue_found",
+                        "agent_id": "agent-1",  # the carrier who first assessed it
+                        "agent_name": "browser-tester",
+                    }
+                ],
+            ),
+        ],
+        agent_graph=_graph(
+            statuses={"agent-1": "completed", "agent-2": "completed"},
+            names={"agent-1": "browser-tester", "agent-2": "redirect-tester"},
+            metadata={
+                "agent-1": {"skills": ["browser_security"]},
+                "agent-2": {"skills": ["open_redirect"]},
+            },
+        ),
+    )
+
+    sub = {g["risk_area"] for g in doc["gaps"] if g["kind"] == "unrecorded_sub_topic"}
+    assert "client-side path traversal" not in sub  # preserved carrier authorship counts
+
+
+def test_registry_backs_skill_phrasings_and_cwe() -> None:
+    """_SKILL_PHRASINGS is derived from the registry, so they cannot drift."""
+    assert _SKILL_PHRASINGS == {name: v.aliases for name, v in VULN_CLASSES.items()}
+    assert cwe_for_skill("sql_injection") == ("CWE-89",)
+    assert cwe_for_skill("vulnerabilities/ssrf") == ("CWE-918",)
+    assert cwe_for_skill("subdomain_takeover") == ()
+
+
 def test_non_risk_skills_carry_no_coverage_obligation() -> None:
     """Tooling skills describe how an agent works, not what it hunts."""
     doc = _document(agent_graph=_graph(metadata={"agent-1": {"skills": ["idor", "caido"]}}))