From c0cc1a82c9ecb9488b6a10f35f9f84fb24ee06fe Mon Sep 17 00:00:00 2001 From: Afraa Shaikh Date: Sun, 19 Jul 2026 16:10:40 +0530 Subject: [PATCH] Address CSPT review: real data-flow + browser tests, fingerprint & docs fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve the "changes required" review on the CSPT PR: - SARIF: add a shared `_class_token` helper preferring the structured `finding_class` over a title keyword, used in BOTH `_class_fingerprint` and the synthetic branch of `_primary_fingerprint`, so a locationless CSPT finding no longer collapses onto a server-side traversal's primary fingerprint. Require CSPT reports to carry a discriminator (endpoint/method or a code location) so multiple locationless CSPT findings stay distinct. - Dedup: include `finding_class` in the candidate and in `_prepare_report_for_comparison`; teach the judge that a differing finding_class under a shared CWE is not a duplicate. - Skill/runbook: correct the %2f guidance — the browser does not decode `%2f` during normalization; only literal-slash dot-segments traverse, and `%2f`-based traversal is an app-layer decode. Switch fragment examples to raw `../../`. Split confirmation into a CSPT *primitive* (path steered) vs. an *impact* level (credentials/reach/chain) to stop false negatives. Fix non-loadable skill names to the underscore keys (`client_side_path_traversal`, `path_traversal_lfi_rfi`), including the skill frontmatter. - Fixtures: drive `positive_normalized` from the fragment so it serves under a plain static server. - Tests: add `test_cspt_browser_capture.py` — headless Chromium (Playwright) + a local request-capture server asserting the exact outbound path per positive and no traversal per negative, plus WHATWG ground-truth tests proving raw `../../` and `%2e%2e/` collapse while un-decoded `..%2f..%2f` does not. Add `test_cspt_dataflow.py` — a deterministic taint tracker that fails when a fixture's source and sink are present but unconnected. Add primary-fingerprint and dedup regressions. Add playwright to dev deps. - Docs: list the CSPT skill in the public skills catalog. Known follow-up (not blocking): `uv.lock` is left at format revision 2; run `uv lock` with the repo's pinned uv to add the playwright dev dep without the unrelated revision-2->3 rewrite. There is no test CI workflow yet; when one is added it should run `uv run playwright install chromium` so the browser tests execute (they importorskip otherwise). --- .../vulnerabilities/path_traversal_lfi_rfi.md | 2 +- strix/tools/reporting/tool.py | 2 +- tests/test_cspt_browser_capture.py | 47 +++++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/strix/skills/vulnerabilities/path_traversal_lfi_rfi.md b/strix/skills/vulnerabilities/path_traversal_lfi_rfi.md index d15a6645..3f2f7e9c 100644 --- a/strix/skills/vulnerabilities/path_traversal_lfi_rfi.md +++ b/strix/skills/vulnerabilities/path_traversal_lfi_rfi.md @@ -1,5 +1,5 @@ --- -name: path-traversal-lfi-rfi +name: path_traversal_lfi_rfi description: Path traversal and file inclusion testing for local/remote file access and code execution --- diff --git a/strix/tools/reporting/tool.py b/strix/tools/reporting/tool.py index 11f93bdb..3d8115fb 100644 --- a/strix/tools/reporting/tool.py +++ b/strix/tools/reporting/tool.py @@ -159,7 +159,7 @@ _VALID_FIX_EFFORT = frozenset({"trivial", "low", "medium", "high"}) # ``client_side_path_traversal`` (CSPT) is called out explicitly so the # finding is machine-separable from server-side path traversal / LFI / RFI, # which share CWE-22 but are a distinct vulnerability class (see the -# ``client-side-path-traversal`` skill). ``dependency_cve`` is reserved for +# ``client_side_path_traversal`` skill). ``dependency_cve`` is reserved for # ``create_dependency_report`` and is not selectable here. _VALID_FINDING_CLASS = frozenset({"dynamic", "client_side_path_traversal"}) diff --git a/tests/test_cspt_browser_capture.py b/tests/test_cspt_browser_capture.py index 3d09c24d..465534e9 100644 --- a/tests/test_cspt_browser_capture.py +++ b/tests/test_cspt_browser_capture.py @@ -237,3 +237,50 @@ def test_negative_safe_construction_stays_on_constant_path(browser, server) -> N assert server.capture.traversed == [], server.capture assert server.capture.intended, "expected the benign /api/search request to fire" assert all(p.startswith("/api/search") for p in server.capture.intended), server.capture + + +# --------------------------------------------------------------------------- # +# Browser URL-normalization ground truth (WHATWG). These pin the exact claim +# the review corrected: the browser collapses raw ``..`` dot-segments delimited +# by literal ``/``, but does NOT decode ``%2f`` into a separator, so an +# un-decoded ``..%2f..%2f`` stays a single literal segment and does not traverse. +# --------------------------------------------------------------------------- # + + +def _fetch_from_page(browser, server, url_to_fetch: str) -> list[str]: # type: ignore[no-untyped-def] + """Load a served origin, issue a raw ``fetch`` of ``url_to_fetch`` from the + page (no application decoding in between), and return the path(s) the server + observed off the wire.""" + context = browser.new_context() + page = context.new_page() + page.goto(f"{server.base_url}/positive_direct.html#noop", wait_until="networkidle") + server.capture.intended.clear() + server.capture.traversed.clear() + page.evaluate("(u) => fetch(u).catch(() => {})", url_to_fetch) + page.wait_for_timeout(200) + context.close() + return server.capture.traversed + server.capture.intended + + +def test_browser_collapses_raw_dotdot_with_literal_slashes(browser, server) -> None: # type: ignore[no-untyped-def] + # Raw ../ delimited by literal / IS collapsed by the URL parser. + observed = _fetch_from_page(browser, server, "/api/orders/../../admin/keys") + assert observed == ["/admin/keys"], observed + assert not observed[0].startswith("/api/"), observed + + +def test_browser_does_not_decode_percent_2f_as_separator(browser, server) -> None: # type: ignore[no-untyped-def] + # ..%2f..%2f is a single literal segment: NOT decoded to ../../, so the path + # does NOT escape the /api/ prefix. This is the exact misconception the + # review flagged. + observed = _fetch_from_page(browser, server, "/api/orders/..%2f..%2fadmin%2fkeys") + assert len(observed) == 1, observed + assert observed[0].startswith("/api/orders/"), observed + # It did not collapse to /admin/keys. + assert observed[0] != "/admin/keys", observed + + +def test_browser_collapses_encoded_dot_segments_with_literal_slashes(browser, server) -> None: # type: ignore[no-untyped-def] + # %2e%2e are recognized as dot-segments when delimited by literal / . + observed = _fetch_from_page(browser, server, "/api/orders/%2e%2e/%2e%2e/admin/keys") + assert observed == ["/admin/keys"], observed