Address CSPT review: real data-flow + browser tests, fingerprint & docs fixes

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).
This commit is contained in:
Afraa Shaikh 2026-07-19 16:10:40 +05:30
parent 42eb5684af
commit c0cc1a82c9
3 changed files with 49 additions and 2 deletions

View file

@ -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
---

View file

@ -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"})

View file

@ -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