From 302e3442b684c98fb8e21a63ffa2a104ace82229 Mon Sep 17 00:00:00 2001 From: Afraa Shaikh Date: Fri, 17 Jul 2026 01:29:28 +0530 Subject: [PATCH] Fix CSPT fingerprint collision and correct fixture normalization paths - SARIF class fingerprint now prefers the structured finding_class over title-keyword matching, so a CSPT finding whose title contains "path traversal" no longer collapses onto the server-side traversal fingerprint - Correct documented normalized paths in positive_direct/positive_encoded fixtures, the evidence runbook, and the skill (leading ../ segments pop both path prefixes, e.g. /admin/keys/detail, not /api/admin/keys) - Add regression tests covering the fingerprint precedence and the corrected fixture paths Addresses PR #789 review (Closes #776) --- docs/cspt-evidence-runbook.md | 11 +- strix/report/sarif.py | 38 +++++-- .../client_side_path_traversal.md | 8 +- tests/fixtures/cspt/positive_direct.html | 7 +- tests/fixtures/cspt/positive_encoded.html | 6 +- tests/test_cspt_skill.py | 105 +++++++++++++++++- 6 files changed, 151 insertions(+), 24 deletions(-) diff --git a/docs/cspt-evidence-runbook.md b/docs/cspt-evidence-runbook.md index 86e86524..be2a2a3a 100644 --- a/docs/cspt-evidence-runbook.md +++ b/docs/cspt-evidence-runbook.md @@ -41,7 +41,7 @@ agent-browser network requests # note the intended path, e.g. /api/ # 3b. TRAVERSAL — drive the same source with a crafted value. agent-browser open "/#/orders/..%2f..%2fadmin%2fkeys" agent-browser wait --load networkidle -agent-browser network requests # note the traversed path, e.g. /api/admin/keys +agent-browser network requests # note the traversed path, e.g. /admin/keys/detail # 4. Stop the capture and keep the HAR as the primary artifact. agent-browser network har stop /workspace/.agent-browser-screenshots/cspt.har @@ -71,7 +71,7 @@ agent-browser open "" A finding is a real CSPT only when **all** of these hold, evidenced from the HAR: 1. **Traversed path on the wire** — the outbound request path is the traversed - target (e.g. `/api/admin/keys`), not the intended one + target (e.g. `/admin/keys/detail`), not the intended one (`/api/orders/12345/detail`). 2. **Ambient credentials** — that request carried the victim's session (`Cookie` / `Authorization` header present; read it from the HAR entry). @@ -103,7 +103,7 @@ Intended (benign control): Traversed (CSPT): GET /api/orders/../../admin/keys/detail - → browser normalizes to: GET /api/admin/keys + → browser normalizes to: GET /admin/keys/detail Cookie: session= → 200 OK (admin key material reachable via a low-priv user's fragment) ``` @@ -119,9 +119,10 @@ python -m http.server 8000 --directory tests/fixtures/cspt agent-browser network har start agent-browser open "http://localhost:8000/positive_direct.html#/orders/../../admin/keys" agent-browser wait --load networkidle -agent-browser network requests # observe the normalized /api/admin/keys request +agent-browser network requests # observe the normalized /admin/keys/detail request ``` -The positive fixtures emit a traversed `/api/...` request; the negative fixtures +The positive fixtures emit a traversed request whose path escapes the intended +`/api/...` prefix (e.g. `/admin/keys/detail`); the negative fixtures (`negative_validated.html`, `negative_safe_construction.html`) either reject the input before the sink or keep the value out of the path entirely. diff --git a/strix/report/sarif.py b/strix/report/sarif.py index 481772bb..e5030f5b 100644 --- a/strix/report/sarif.py +++ b/strix/report/sarif.py @@ -918,20 +918,36 @@ 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. + Composite of (rule_id, vuln-class token). The class token is the + structured ``finding_class`` when it carries signal (i.e. is present + and not the default ``dynamic``), otherwise a keyword extracted from + the 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. + Preferring ``finding_class`` matters when a finding's title happens to + contain a broader curated keyword: a CSPT report titled "Path Traversal + via fragment" carries ``finding_class="client_side_path_traversal"`` + but its title matches the generic ``path traversal`` keyword, which + would otherwise collapse it onto the same fingerprint as a server-side + path-traversal finding (both key on CWE-22). The structured field is + authoritative, so it wins; title-keyword extraction is the fallback + only when ``finding_class`` is absent or ``dynamic``. + + Title is LLM-authored so it's stochastic at the prose level, but the + 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. """ - title = _string_value(report.get("title")) or "" - keyword = _class_keyword(title) if title else "" - if not keyword: + finding_class = _string_value(report.get("finding_class")) + if finding_class and finding_class != "dynamic": + token = finding_class + else: + title = _string_value(report.get("title")) or "" + token = _class_keyword(title) if title else "" + 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() diff --git a/strix/skills/vulnerabilities/client_side_path_traversal.md b/strix/skills/vulnerabilities/client_side_path_traversal.md index 2dffc539..125433f7 100644 --- a/strix/skills/vulnerabilities/client_side_path_traversal.md +++ b/strix/skills/vulnerabilities/client_side_path_traversal.md @@ -88,8 +88,10 @@ axios.get("/api/orders/" + params.id) // id from route param xhr.open("GET", baseApi + "/" + segment) // segment from postMessage ``` -If `userId` = `../../admin/keys#`, the browser normalizes -`/api/users/../../admin/keys` → `/api/admin/keys`, sent with the victim's session. +If `userId` = `../../admin/keys#` (the trailing `#` starts a fragment, dropping +the appended `/avatar`), the browser normalizes +`/api/users/../../admin/keys` → `/admin/keys` (the two `../` segments pop `users` +then `api`), sent with the victim's session. ## Reconnaissance @@ -166,7 +168,7 @@ agent-browser screenshot **Confirmation criteria** — you have a real CSPT when *all* hold: 1. The **outbound request path in the proxy/HAR** is the traversed target - (`/api/admin/keys`), not the intended one (`/api/users//avatar`). + (e.g. `/admin/keys`), not the intended one (`/api/users//avatar`). 2. That request carried the **victim's credentials** (session cookie / `Authorization` header present on the request — read it from the HAR). 3. There is a **benign same-endpoint control**: the same flow with a normal diff --git a/tests/fixtures/cspt/positive_direct.html b/tests/fixtures/cspt/positive_direct.html index 6fdee5c4..962bce3d 100644 --- a/tests/fixtures/cspt/positive_direct.html +++ b/tests/fixtures/cspt/positive_direct.html @@ -4,8 +4,11 @@ Source: URL fragment (location.hash) Sink: fetch() The fragment segment is concatenated straight into the request path with no - validation. A hash of `../../admin/keys` normalizes the outbound request to - /api/admin/keys, sent with the page's ambient session. + validation. With a hash of `#/orders/../../admin/keys`, orderId becomes + `../../admin/keys` and the request URL is built as + `/api/orders/../../admin/keys/detail`. The browser normalizes that outbound + path to /admin/keys/detail (the two `../` segments pop `orders` then `api`), + sent with the page's ambient session. --> Order detail diff --git a/tests/fixtures/cspt/positive_encoded.html b/tests/fixtures/cspt/positive_encoded.html index 5ea76d03..331653bc 100644 --- a/tests/fixtures/cspt/positive_encoded.html +++ b/tests/fixtures/cspt/positive_encoded.html @@ -4,8 +4,10 @@ Source: query parameter (?resource=) Sink: XMLHttpRequest.open() The value is URL-decoded once, then joined into the path. A payload of - %2e%2e%2f%2e%2e%2fadmin%2fconfig decodes to ../../admin/config and the browser - normalizes the outbound path to /api/admin/config. + %2e%2e%2f%2e%2e%2fadmin%2fconfig decodes to ../../admin/config, the request + URL is built as `/api/resources/../../admin/config`, and the browser + normalizes the outbound path to /admin/config (the two `../` segments pop + `resources` then `api`). --> Resource viewer diff --git a/tests/test_cspt_skill.py b/tests/test_cspt_skill.py index f103590e..eef9e217 100644 --- a/tests/test_cspt_skill.py +++ b/tests/test_cspt_skill.py @@ -178,7 +178,7 @@ def _create_kwargs(**overrides: Any) -> dict[str, Any]: "poc_description": "Open the app with a crafted fragment.", "poc_script_code": "location.hash = '#/orders/../../admin/keys'", "remediation_steps": "Validate the segment against an allowlist.", - "evidence": "Outbound request path was /api/admin/keys with the session cookie.", + "evidence": "Outbound request path was /admin/keys/detail with the session cookie.", "assumptions": "Assumes the victim opens a crafted link while authenticated.", "fix_effort": "low", "cvss_breakdown": _cvss(), @@ -298,3 +298,106 @@ def test_sarif_separates_cspt_from_server_side_traversal_class(tmp_path: Path) - hashes = [r["properties"].get("strix_vuln_class_hash") for r in results] assert all(hashes), "expected a class fingerprint on both findings" assert hashes[0] != hashes[1], "CSPT and server-side traversal share a class fingerprint" + + +def test_sarif_class_fingerprint_uses_finding_class_over_colliding_title(tmp_path: Path) -> None: + # Regression: a CSPT finding whose *title* contains the generic "path + # traversal" keyword must still be separated from a genuine server-side + # path-traversal finding. Before the fix the class fingerprint was derived + # from the title keyword alone, so both collapsed onto the same hash even + # though the structured finding_class distinguished them. The structured + # field must win. + write_sarif( + tmp_path, + [ + { + "id": "vuln-0001", + "title": "Path Traversal via fragment", # collides on the generic keyword + "severity": "high", + "cwe": "CWE-22", + "finding_class": "client_side_path_traversal", + "timestamp": "2026-07-16 10:00:00 UTC", + "code_locations": [{"file": "app.js", "start_line": 12}], + }, + { + "id": "vuln-0002", + "title": "Path Traversal in download file parameter", + "severity": "high", + "cwe": "CWE-22", + "finding_class": "dynamic", + "timestamp": "2026-07-16 10:00:00 UTC", + "code_locations": [{"file": "server.py", "start_line": 30}], + }, + ], + ) + results = json.loads((tmp_path / "findings.sarif").read_text(encoding="utf-8"))["runs"][0][ + "results" + ] + hashes = [r["properties"].get("strix_vuln_class_hash") for r in results] + assert all(hashes), "expected a class fingerprint on both findings" + assert hashes[0] != hashes[1], ( + "CSPT finding with a 'path traversal' title collapsed onto the " + "server-side traversal fingerprint; finding_class must take precedence" + ) + + +# --------------------------------------------------------------------------- # +# Fixture path normalization (documented expected paths must be correct) +# --------------------------------------------------------------------------- # + + +def _normalize_url_path(path: str) -> str: + """Resolve ``.`` / ``..`` segments the way a browser normalizes a URL path. + + Leading ``..`` that would escape the root are clamped (dropped), matching + the URL parser rather than a filesystem join. + """ + parts = path.split("/") + out: list[str] = [] + for part in parts: + if part == "..": + if out and out[-1] not in ("", ".."): + out.pop() + elif part != ".": + out.append(part) + return "/".join(out) + + +def test_normalize_helper_matches_documented_examples() -> None: + # Guard the helper itself so the assertions below mean something. + assert _normalize_url_path("/api/orders/../../admin/keys/detail") == "/admin/keys/detail" + assert _normalize_url_path("/api/resources/../../admin/config") == "/admin/config" + assert _normalize_url_path("/api/users/../../admin/keys") == "/admin/keys" + + +def test_positive_direct_fixture_documents_correct_normalized_path() -> None: + text = (FIXTURES / "positive_direct.html").read_text(encoding="utf-8") + # The fixture builds this request URL from orderId="../../admin/keys": + assert _normalize_url_path("/api/orders/../../admin/keys/detail") == "/admin/keys/detail" + # ...and the comment must document that correct result, not the wrong + # /api/admin/keys. + assert "/admin/keys/detail" in text + assert "/api/admin/keys" not in text + + +def test_positive_encoded_fixture_documents_correct_normalized_path() -> None: + text = (FIXTURES / "positive_encoded.html").read_text(encoding="utf-8") + assert _normalize_url_path("/api/resources/../../admin/config") == "/admin/config" + assert "/admin/config" in text + assert "/api/admin/config" not in text + + +def test_no_fixture_or_doc_claims_the_wrong_api_admin_path() -> None: + # Belt-and-suspenders: the incorrect "/api/admin/..." normalized paths must + # not reappear in any fixture, the skill, or the runbook. + repo = Path(__file__).parent.parent + targets = [ + FIXTURES / "positive_direct.html", + FIXTURES / "positive_encoded.html", + repo / "docs" / "cspt-evidence-runbook.md", + repo / "strix" / "skills" / "vulnerabilities" / "client_side_path_traversal.md", + ] + for path in targets: + text = path.read_text(encoding="utf-8") + assert "/api/admin/keys" not in text, f"{path.name} claims wrong path /api/admin/keys" + assert "/api/admin/config" not in text, f"{path.name} claims wrong path /api/admin/config"