From 47c27aef2ede6d144c29b87435c65924d34a388d Mon Sep 17 00:00:00 2001 From: Afraa Shaikh Date: Fri, 17 Jul 2026 01:07:56 +0530 Subject: [PATCH 1/4] Add explicit CSPT detection skill, categorization, fixtures, and tests Closes #776 --- docs/cspt-evidence-runbook.md | 127 ++++++++ strix/report/sarif.py | 11 + .../client_side_path_traversal.md | 234 ++++++++++++++ strix/tools/reporting/tool.py | 34 ++ tests/fixtures/cspt/README.md | 35 ++ .../cspt/negative_safe_construction.html | 29 ++ tests/fixtures/cspt/negative_validated.html | 32 ++ tests/fixtures/cspt/positive_direct.html | 26 ++ tests/fixtures/cspt/positive_encoded.html | 29 ++ tests/fixtures/cspt/positive_normalized.html | 29 ++ tests/fixtures/cspt/positive_postmessage.html | 29 ++ tests/fixtures/cspt/positive_storage.html | 33 ++ tests/test_cspt_skill.py | 300 ++++++++++++++++++ 13 files changed, 948 insertions(+) create mode 100644 docs/cspt-evidence-runbook.md create mode 100644 strix/skills/vulnerabilities/client_side_path_traversal.md create mode 100644 tests/fixtures/cspt/README.md create mode 100644 tests/fixtures/cspt/negative_safe_construction.html create mode 100644 tests/fixtures/cspt/negative_validated.html create mode 100644 tests/fixtures/cspt/positive_direct.html create mode 100644 tests/fixtures/cspt/positive_encoded.html create mode 100644 tests/fixtures/cspt/positive_normalized.html create mode 100644 tests/fixtures/cspt/positive_postmessage.html create mode 100644 tests/fixtures/cspt/positive_storage.html create mode 100644 tests/test_cspt_skill.py diff --git a/docs/cspt-evidence-runbook.md b/docs/cspt-evidence-runbook.md new file mode 100644 index 00000000..86e86524 --- /dev/null +++ b/docs/cspt-evidence-runbook.md @@ -0,0 +1,127 @@ +# CSPT evidence runbook + +How to reproduce and capture request-path evidence for a Client-Side Path +Traversal (CSPT) finding, so a confirmed report carries the proof the maintainer +asked for. This is the manual counterpart to the automated tests in +`tests/test_cspt_skill.py` — the tests guard the skill/fixtures/categorization in +CI; this runbook produces the live browser + proxy evidence a real finding needs +(which CI can't run). + +The vulnerability and its confirmation both happen **in the browser**: the win +condition is the victim's browser sending a request to an *unintended* same-origin +path, carrying the victim's session. So the evidence is the **outbound request +path on the wire**, read off the Caido proxy / a HAR capture — not the response +body alone. + +## Prerequisites + +- `agent-browser` CLI (pre-installed in the Strix sandbox), which auto-routes + through the Caido proxy via `http_proxy` / `https_proxy`. **Do not pass + `--proxy`** — see the `agent_browser` skill. +- An authenticated session on the target (so requests carry ambient credentials), + or one of the fixtures under `tests/fixtures/cspt/` served locally for a + self-contained demo. + +## Procedure + +```bash +# 1. Load the page. Authenticate so subsequent requests carry a session. +agent-browser open +agent-browser auth login # or: state load ./auth.json + +# 2. Start capturing traffic BEFORE triggering the sink. +agent-browser network har start + +# 3a. BENIGN CONTROL — drive the source with a normal value first. +# This establishes the intended request path for the side-by-side. +agent-browser open "/#/orders/12345" +agent-browser wait --load networkidle +agent-browser network requests # note the intended path, e.g. /api/orders/12345/detail + +# 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 + +# 4. Stop the capture and keep the HAR as the primary artifact. +agent-browser network har stop /workspace/.agent-browser-screenshots/cspt.har + +# 5. Optional visual proof if the traversed response changes the page. +agent-browser screenshot +``` + +For a `postMessage` source, replace step 3b with an eval that posts the crafted +message into the vulnerable listener: + +```bash +cat <<'EOF' | agent-browser eval --stdin +window.postMessage({ path: "../../admin/keys" }, "*"); +EOF +``` + +For a storage source, seed the value first, then reload: + +```bash +agent-browser eval "localStorage.setItem('activeTenant', '../../admin')" +agent-browser open "" +``` + +## Confirmation criteria + +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 + (`/api/orders/12345/detail`). +2. **Ambient credentials** — that request carried the victim's session + (`Cookie` / `Authorization` header present; read it from the HAR entry). +3. **Benign control** — the same flow with a normal value hit the intended path. + Show both, side by side. +4. **Reach** — the traversed endpoint returned data or performed an action the + source value should not have been able to reach. + +Do not exfiltrate real secrets — a status / length / shape delta against a +low-sensitivity sibling endpoint is enough to prove reach. + +## Turning it into a report + +File with `create_vulnerability_report`: + +- `finding_class="client_side_path_traversal"` (**required** — separates it from + server-side path traversal / LFI / RFI, which share CWE-22). +- `cwe="CWE-22"`, `title` prefixed `Client-Side Path Traversal`. +- `evidence` — the intended vs. traversed request paths from the HAR, with the + credential header shown, plus the benign control, in fenced code blocks. + +### Evidence block template + +``` +Intended (benign control): + GET /api/orders/12345/detail + Cookie: session= + → 200 OK (own order) + +Traversed (CSPT): + GET /api/orders/../../admin/keys/detail + → browser normalizes to: GET /api/admin/keys + Cookie: session= + → 200 OK (admin key material reachable via a low-priv user's fragment) +``` + +## Demoing against the bundled fixtures + +The fixtures in `tests/fixtures/cspt/` are self-contained static pages. Serve the +directory and drive each one to see the positive/negative behavior without a real +target: + +```bash +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 +``` + +The positive fixtures emit a traversed `/api/...` request; 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 6d5db177..481772bb 100644 --- a/strix/report/sarif.py +++ b/strix/report/sarif.py @@ -531,6 +531,16 @@ 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 other 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 on every + # result. + finding_class = _string_value(report.get("finding_class")) + if finding_class and finding_class != "dynamic": + strix["finding_class"] = finding_class + # SARIF is written for external upload (code-scanning / ASPM), so it must # NOT carry the weaponized exploit payload — that stays a local run # artifact (vulnerabilities.json / the finding MD). We surface the PoC @@ -773,6 +783,7 @@ _VULN_CLASS_KEYWORDS = ( "default password", "session fixation", "open redirect", + "client-side path traversal", "path traversal", "directory traversal", "command injection", 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..2dffc539 --- /dev/null +++ b/strix/skills/vulnerabilities/client_side_path_traversal.md @@ -0,0 +1,234 @@ +--- +name: client-side-path-traversal +description: Client-Side Path Traversal (CSPT) testing — attacker-controlled client input steering the victim's own authenticated browser requests to unintended same-origin endpoints +--- + +# Client-Side Path Traversal (CSPT) + +Client-Side Path Traversal happens **in the victim's browser**, not on the +server. Front-end JavaScript takes an attacker-influenceable client-side value +and concatenates it into the path of a request the browser then sends with the +victim's ambient credentials (cookies, `Authorization` headers set by the app, +session). By injecting `../` (or encoded / normalizing variants) into that value, +an attacker redirects the request to a **different same-origin endpoint** than the +developer intended — turning the victim's own browser into the request forger. + +The bug is a **path join in client code**, and the impact is realized entirely by +the browser's URL normalization (`new URL()`, the fetch/XHR URL parser) collapsing +`../` before the request leaves the page. Treat every client-readable input that +flows into a request URL as untrusted; validate the *resulting path*, not the raw +segment. + +CSPT is frequently a **gadget**, not a standalone bug: it lets an attacker point +an authenticated request at an endpoint whose JSON/HTML response is then fed into +a sink. Chained with a reflected/DOM XSS sink, a permissive CORS reader, or a +state-changing endpoint that trusts a weak/absent CSRF defense, a "read-only" +CSPT becomes account takeover or a stored-XSS delivery path. + +## Not CSPT (route these elsewhere) + +Keep this class clean. The following are **server-side** file/path issues covered +by the `path-traversal-lfi-rfi` skill — report them as `dynamic` findings, never +as `client_side_path_traversal`: + +- **Server-side path traversal** — server code joins a request parameter into a + filesystem path and reads/returns a file (`/download?file=../../etc/passwd`). + The traversal is resolved by the *server's* filesystem, not the browser. +- **LFI / RFI** — server includes/executes a file whose name/URL is user-controlled + (`php://filter`, remote include). Server-side execution. +- **Zip Slip / archive extraction** — server writes files outside a target dir. +- **SSRF** — server (not the browser) fetches an attacker-controlled URL. +- **Open redirect** — the browser is navigated (Location / `window.location`) to + an attacker origin. CSPT stays *same-origin* and targets a request **path**, not + a top-level navigation. If the sink is a navigation, it's `open-redirect`. + +The discriminator: **who resolves the `../` and who sends the final request?** +If it's the victim's browser building a same-origin `fetch`/XHR path → CSPT. If +it's the server touching its filesystem or making an outbound request → +server-side traversal / LFI / RFI / SSRF. + +## Sources (attacker-influenceable client input) + +Any of these reaching a request-URL sink is a candidate taint source: + +- **URL query params** — `URLSearchParams`, `location.search`, router query + (`?next=`, `?id=`, `?path=`, `?tab=`, `?redirect=`). +- **URL path segments** — SPA route params parsed from `location.pathname` + (`/orders/:id`, `react-router` / `vue-router` params). +- **Fragment / hash** — `location.hash`, hash-router segments. Never sent to the + server, so server-side WAFs don't see it — a favorite CSPT source. +- **`document.referrer`** — attacker controls it by hosting the referring page. +- **`postMessage` data** — `event.data` from another window/iframe without an + `event.origin` check. +- **Browser storage** — `localStorage`, `sessionStorage`, `IndexedDB`, cookies — + when an earlier, attacker-influenceable flow seeded the value. +- **`window.name`**, **BroadcastChannel**, **WebSocket message payloads** — cross + -context channels that survive navigation. + +## Sinks (client request builders whose path is tainted) + +The tainted value must land in the **path** portion (not just a query value) of: + +- `fetch(url)` / `fetch(new Request(url))` +- `XMLHttpRequest.open(method, url)` +- `axios.get/post/... (url)`, an axios instance with a `baseURL` +- jQuery `$.ajax({url})`, `$.get`, `$.getJSON`, `$.load` +- `navigator.sendBeacon(url)` +- `new EventSource(url)`, `new WebSocket(url)` +- framework data layers that build a request path: Angular `HttpClient`, + `HttpClient.get(\`/api/${seg}\`)`, RTK Query / SWR / react-query key→URL builders, + GraphQL client `uri`. + +The dangerous pattern is **template/`+` concatenation of a source into a path**: + +```js +fetch("/api/users/" + userId + "/avatar") // userId from location.hash +fetch(`/api/${tab}/settings`) // tab from ?tab= +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. + +## Reconnaissance + +### Surface map + +- Grep client bundles for the sinks above joined with a source. In `agent_browser`: + ```bash + agent-browser open + # dump all script URLs, then fetch + grep them + agent-browser eval "Array.from(document.scripts).map(s=>s.src).filter(Boolean)" + ``` + Then pull each bundle and search for `fetch(`, `.open(`, `axios`, backtick URL + templates, and `location.hash` / `URLSearchParams` / `postMessage` / referrer. +- Map every endpoint the SPA calls (record with the proxy, see Validation). Note + which build their path from a client value vs. a constant. +- Identify **high-value sibling endpoints** reachable by traversal from a normal + path (admin, other users' resources, token/key endpoints, state-changing POSTs). + +### Capability probes + +Inject into each candidate source and watch the **outbound request path** (via the +proxy), not the response body alone — the win condition is the browser sending a +path you didn't intend. + +- **Direct**: `../`, `../../`, `..%2f` un-decoded, leading `/` (absolute-path + override: `id=/admin/keys` → `/api//admin/keys` or an absolute join). +- **Encoded**: `%2e%2e%2f`, `%2e%2e/`, `..%2f`, double-encoded `%252e%252e%252f` + when an app decodes before joining. The browser decodes `%2e`/`%2f` in the path + during normalization — test both raw and encoded because framework routers may + decode a layer first. +- **Normalized**: `....//` (folds to `../` after one collapse), `..././`, + `.%2e/`, mixed `..\` on apps that rewrite backslashes, redundant `//` and `/./` + segments that shift the base. +- **Base-relative vs root-relative**: confirm whether the app uses a relative + base (`api/x`) or rooted (`/api/x`) — it changes how many `../` are needed and + whether a leading `/` fully replaces the path. + +## Validation (browser + Caido proxy) + +Detection is dynamic: drive a real headless browser and read the **final outbound +request path** off the proxy. The `agent_browser` CLI auto-routes through the +Caido HTTP/HTTPS proxy (do **not** pass `--proxy`; it's wired via +`http_proxy`/`https_proxy`). See the `agent_browser` skill for the full CLI. + +Runbook: + +```bash +# 1. Load the page and (if needed) authenticate so requests carry a session. +agent-browser open +agent-browser auth login # or state load — see agent_browser skill + +# 2. Start capturing traffic BEFORE triggering the sink. +agent-browser network har start + +# 3. Drive the tainted source. Examples per source type: +# - fragment: navigate with a crafted hash +agent-browser open "/#/orders/..%2f..%2fadmin%2fkeys" +# - query: open with a crafted param +agent-browser open "/dashboard?tab=../../admin/config" +# - postMessage: eval a crafted message into the vulnerable listener +cat <<'EOF' | agent-browser eval --stdin +window.postMessage({ path: "../../admin/keys" }, "*"); +EOF + +# 4. Let the request fire, then read what actually left the browser. +agent-browser wait --load networkidle +agent-browser network requests # inspect fired request URLs +agent-browser network har stop /workspace/.agent-browser-screenshots/cspt.har + +# 5. Capture visual proof if the traversed response changes the page. +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`). +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 + value hits the intended path. Show both paths side by side. +4. The traversed endpoint returned data (or performed an action) the source + value should not have been able to reach. + +Record the request/response pair (method, full path, relevant headers, status) +from the HAR as evidence. Prefer reading a low-sensitivity sibling endpoint to +prove reach; do not exfiltrate real secrets — a status/length/shape delta is +enough proof. + +## Reporting + +File confirmed CSPT with `create_vulnerability_report` and: + +- `finding_class="client_side_path_traversal"` — **required** so the finding is + separated from server-side path traversal / LFI / RFI (which share the CWE but + are a different class). Do not leave it `dynamic`. +- `cwe="CWE-22"` (Improper Limitation of a Pathname to a Restricted Directory). +- `title` prefixed **`Client-Side Path Traversal`** (e.g. + `"Client-Side Path Traversal in order-detail fetch via URL fragment"`), so the + class is obvious in the report/SARIF even to a human skimming titles. +- `evidence` — the two request paths (intended vs. traversed) from the proxy/HAR, + with the credential header present, plus the benign control. Fenced code blocks. +- `poc_description` — steps to reproduce (navigate/craft source → observe path). +- `poc_script_code` — the crafted URL / `postMessage` / storage-seeding snippet. +- `impact` — name the concrete sibling endpoint reached and what it exposes or + changes; if chained (XSS/CSRF/CORS), state the chain. +- CVSS — usually `AV:N`, `UI:R` (victim opens a link), `PR:N`; scope/impact + depends on the reached endpoint. + +## False positives + +Not CSPT — do not report: + +- The source is **validated to the resulting path**: an allowlist of endpoints, + a strict `^[a-z0-9-]+$` segment check, or `encodeURIComponent` on a value used + as a **single segment** *and* the app rejects/encodes `/` and `.` so traversal + can't form. (`encodeURIComponent` alone is only safe when the value is one + segment and `%2f`/`%2e` aren't decoded downstream — verify the outbound path.) +- **Safe path construction**: the endpoint path is a constant and the client + value only ever becomes a **query parameter or request body field**, never part + of the path. A tainted query value is not CSPT (it may be another bug). +- The browser **does not normalize** the value into a different path (confirm on + the wire — no traversal actually occurred in the outbound request). +- The traversed request is **not** sent with ambient credentials (no session on + that request), so there's no privilege to abuse — note it, don't over-rate it. +- Server rejects the traversed path (404/403) with no differential — no reach, no + finding. + +## Pro tips + +1. Always judge on the **outbound request path in the proxy**, not the response — + a 404 on the traversed path still proves the browser was steered, but a + *successful* differential response is what makes it impactful. +2. The fragment (`location.hash`) is the strongest source: invisible to server + WAFs/logs and fully attacker-controlled via a link. +3. Test both raw (`../`) and encoded (`%2e%2e%2f`) — SPA routers often decode one + layer, so the effective payload differs by framework. +4. `....//` and other self-collapsing sequences bypass naive `replace("../","")` + filters — a single non-recursive strip leaves a working `../`. +5. CSPT shines when chained. After proving reach, look for a reader sink (CORS, + response rendered into DOM) or a state-changing target to escalate read-only + traversal into real impact. diff --git a/strix/tools/reporting/tool.py b/strix/tools/reporting/tool.py index b6f284ca..dcaef337 100644 --- a/strix/tools/reporting/tool.py +++ b/strix/tools/reporting/tool.py @@ -154,6 +154,15 @@ _REQUIRED_FIELDS = { _VALID_FIX_EFFORT = frozenset({"trivial", "low", "medium", "high"}) +# Dynamically-proven finding sub-classes that a reporting agent may set on +# ``create_vulnerability_report``. ``dynamic`` is the default catch-all. +# ``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 +# ``create_dependency_report`` and is not selectable here. +_VALID_FINDING_CLASS = frozenset({"dynamic", "client_side_path_traversal"}) + async def _do_create( # noqa: PLR0912 *, @@ -175,6 +184,7 @@ async def _do_create( # noqa: PLR0912 cwe: str | None, code_locations: list[dict[str, Any]] | 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]: @@ -201,6 +211,13 @@ async def _do_create( # noqa: PLR0912 f"Invalid fix_effort: {fix_effort!r}. Must be one of: {sorted(_VALID_FIX_EFFORT)}" ) + finding_class = (finding_class or "dynamic").strip().lower() + if finding_class not in _VALID_FINDING_CLASS: + errors.append( + f"Invalid finding_class: {finding_class!r}. Must be one of: " + f"{sorted(_VALID_FINDING_CLASS)}" + ) + if not isinstance(cvss_breakdown, dict) or not cvss_breakdown: errors.append("cvss_breakdown: must be an object with the 8 CVSS metrics") cvss_breakdown = {} @@ -295,6 +312,7 @@ async def _do_create( # noqa: PLR0912 cwe=cwe, code_locations=parsed_locations, fix_pr_body=fix_pr_body, + finding_class=finding_class, agent_id=agent_id if isinstance(agent_id, str) else None, agent_name=agent_name if isinstance(agent_name, str) else None, ) @@ -339,6 +357,7 @@ async def create_vulnerability_report( cwe: str | None = None, code_locations: list[dict[str, Any]] | None = None, fix_pr_body: str | None = None, + finding_class: str | None = None, ) -> str: """File a vulnerability report — one report per fully-verified finding. @@ -543,6 +562,20 @@ 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 finding sub-class for machine-readable + categorization. Defaults to ``"dynamic"``. Set to + ``"client_side_path_traversal"`` for a confirmed CSPT finding + — attacker-controlled client-side input (URL params, path + segments, fragment, ``document.referrer``, ``postMessage`` + data, browser storage) reaching a browser request sink + (``fetch`` / ``XMLHttpRequest`` / ``axios`` / ``$.ajax`` / + ``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 / LFI / RFI (leave those + ``dynamic``). See the ``client-side-path-traversal`` skill. Example (abbreviated — mirror this structure):: @@ -610,6 +643,7 @@ async def create_vulnerability_report( cwe=cwe, code_locations=code_locations, fix_pr_body=fix_pr_body, + finding_class=finding_class, agent_id=agent_id, agent_name=agent_name, ) diff --git a/tests/fixtures/cspt/README.md b/tests/fixtures/cspt/README.md new file mode 100644 index 00000000..4f5d548b --- /dev/null +++ b/tests/fixtures/cspt/README.md @@ -0,0 +1,35 @@ +# CSPT test fixtures + +Self-contained HTML+JS apps exercising Client-Side Path Traversal (CSPT) +detection. Each file is a single static page with no server dependency — a +CSPT specialist can drive them with the `agent_browser` CLI (routed through the +Caido proxy) exactly as it would a real target, and the automated tests in +`tests/test_cspt_skill.py` assert their structural properties so they cannot +silently rot. + +Every app builds a same-origin request URL by joining a **client-side source** +into a request **path** and sending it via a browser request sink (`fetch` / +`XMLHttpRequest` / `axios`-style). The distinction between positive and negative +is whether the resulting path is validated before it reaches the sink. + +## Positive fixtures (a real CSPT — source flows unvalidated into the path) + +| File | Source | Sink | Traversal style | +|------|--------|------|-----------------| +| `positive_direct.html` | URL fragment (`location.hash`) | `fetch` | direct `../` | +| `positive_encoded.html` | query param (`?resource=`) | `XMLHttpRequest` | URL-encoded `%2e%2e%2f` | +| `positive_normalized.html` | route segment (`location.pathname`) | `fetch` | self-collapsing `....//` | +| `positive_postmessage.html` | `postMessage` `event.data` (no origin check) | `fetch` | direct `../` | +| `positive_storage.html` | `localStorage` value | `axios`-style wrapper | direct `../` | + +## Negative fixtures (safe — not CSPT) + +| File | Why it's safe | +|------|---------------| +| `negative_validated.html` | Segment matched against a strict allowlist / `^[a-z0-9-]+$` before use; traversal rejected. | +| `negative_safe_construction.html` | Endpoint path is a constant; the client value only becomes a **query parameter**, never part of the path. | + +## How to drive one manually (evidence runbook) + +See `docs/cspt-evidence-runbook.md` for the full browser + Caido proxy procedure +that produces the request-path evidence a confirmed finding requires. diff --git a/tests/fixtures/cspt/negative_safe_construction.html b/tests/fixtures/cspt/negative_safe_construction.html new file mode 100644 index 00000000..b5e52a32 --- /dev/null +++ b/tests/fixtures/cspt/negative_safe_construction.html @@ -0,0 +1,29 @@ + + + +Search (safe) + +

Search

+
loading…
+ + + diff --git a/tests/fixtures/cspt/negative_validated.html b/tests/fixtures/cspt/negative_validated.html new file mode 100644 index 00000000..486727e2 --- /dev/null +++ b/tests/fixtures/cspt/negative_validated.html @@ -0,0 +1,32 @@ + + + +Order detail (validated) + +

Order detail

+
loading…
+ + + diff --git a/tests/fixtures/cspt/positive_direct.html b/tests/fixtures/cspt/positive_direct.html new file mode 100644 index 00000000..6fdee5c4 --- /dev/null +++ b/tests/fixtures/cspt/positive_direct.html @@ -0,0 +1,26 @@ + + + +Order detail + +

Order detail

+
loading…
+ + + diff --git a/tests/fixtures/cspt/positive_encoded.html b/tests/fixtures/cspt/positive_encoded.html new file mode 100644 index 00000000..5ea76d03 --- /dev/null +++ b/tests/fixtures/cspt/positive_encoded.html @@ -0,0 +1,29 @@ + + + +Resource viewer + +

Resource viewer

+
loading…
+ + + diff --git a/tests/fixtures/cspt/positive_normalized.html b/tests/fixtures/cspt/positive_normalized.html new file mode 100644 index 00000000..c0a3e20d --- /dev/null +++ b/tests/fixtures/cspt/positive_normalized.html @@ -0,0 +1,29 @@ + + + +Profile + +

Profile

+
loading…
+ + + diff --git a/tests/fixtures/cspt/positive_postmessage.html b/tests/fixtures/cspt/positive_postmessage.html new file mode 100644 index 00000000..275b7069 --- /dev/null +++ b/tests/fixtures/cspt/positive_postmessage.html @@ -0,0 +1,29 @@ + + + +Widget host + +

Widget host

+
waiting for message…
+ + + diff --git a/tests/fixtures/cspt/positive_storage.html b/tests/fixtures/cspt/positive_storage.html new file mode 100644 index 00000000..a5d60730 --- /dev/null +++ b/tests/fixtures/cspt/positive_storage.html @@ -0,0 +1,33 @@ + + + +Tenant dashboard + +

Tenant dashboard

+
loading…
+ + + diff --git a/tests/test_cspt_skill.py b/tests/test_cspt_skill.py new file mode 100644 index 00000000..f103590e --- /dev/null +++ b/tests/test_cspt_skill.py @@ -0,0 +1,300 @@ +"""Regression tests for Client-Side Path Traversal (CSPT) detection support. + +Covers the three in-repo pillars of issue #776 that can run in CI without a live +browser/proxy: + +1. The ``client-side-path-traversal`` skill exists, loads, and documents the + source/sink taxonomy plus the "Not CSPT" boundary the agent relies on. +2. The HTML fixtures encode a real source->sink flow (positives) or a validation + guard / safe construction (negatives), asserted structurally so they can't + silently rot. +3. A CSPT finding is categorized separately from server-side path traversal / + LFI / RFI: the ``finding_class`` param round-trips through the reporting tool + and report state, and surfaces distinctly in SARIF (both as a property and as + a distinct vulnerability-class fingerprint) even though both share CWE-22. +""" + +from __future__ import annotations + +import asyncio +import json +from pathlib import Path +from typing import Any + +import pytest + +from strix.report.sarif import write_sarif +from strix.report.state import ReportState, set_global_report_state +from strix.skills import get_all_skill_names, get_available_skills, load_skills +from strix.tools.reporting.tool import _do_create + + +FIXTURES = Path(__file__).parent / "fixtures" / "cspt" + +POSITIVE_FIXTURES = [ + "positive_direct.html", + "positive_encoded.html", + "positive_normalized.html", + "positive_postmessage.html", + "positive_storage.html", +] +NEGATIVE_FIXTURES = [ + "negative_validated.html", + "negative_safe_construction.html", +] + +# Browser request sinks whose *path* being tainted constitutes CSPT. +_SINK_MARKERS = ("fetch(", "XMLHttpRequest", ".open(", "axios", "sendBeacon", "EventSource") +# Client-side taint sources. +_SOURCE_MARKERS = ( + "location.hash", + "location.search", + "location.pathname", + "URLSearchParams", + "postMessage", + "localStorage", + "sessionStorage", + "document.referrer", +) + + +# --------------------------------------------------------------------------- # +# Skill +# --------------------------------------------------------------------------- # + + +def test_cspt_skill_is_discoverable() -> None: + assert "client_side_path_traversal" in get_all_skill_names() + assert "client_side_path_traversal" in get_available_skills().get("vulnerabilities", []) + + +def test_cspt_skill_loads_with_frontmatter_stripped() -> None: + loaded = load_skills(["client_side_path_traversal"]) + body = loaded.get("client_side_path_traversal") + assert body, "CSPT skill failed to load" + # Frontmatter must be stripped by the loader. + assert not body.lstrip().startswith("---") + assert body.lstrip().startswith("# Client-Side Path Traversal") + + +def test_cspt_skill_documents_sources_sinks_and_boundary() -> None: + body = load_skills(["client_side_path_traversal"])["client_side_path_traversal"].lower() + + # It must teach the sources named in the issue. + for source in ("location.hash", "postmessage", "referrer", "localstorage"): + assert source in body, f"CSPT skill missing source: {source}" + # ...and the request sinks. + for sink in ("fetch", "xmlhttprequest", "axios"): + assert sink in body, f"CSPT skill missing sink: {sink}" + # ...and the explicit boundary vs. server-side classes so findings don't get + # miscategorized. + assert "not cspt" in body + for other in ("lfi", "rfi", "server-side"): + assert other in body, f"CSPT skill missing boundary reference: {other}" + + +def test_cspt_skill_prescribes_finding_class_and_cwe() -> None: + body = load_skills(["client_side_path_traversal"])["client_side_path_traversal"] + # The skill must tell the agent to tag the finding for separation. + assert "client_side_path_traversal" in body + assert "CWE-22" in body + + +# --------------------------------------------------------------------------- # +# Fixtures +# --------------------------------------------------------------------------- # + + +@pytest.mark.parametrize("name", POSITIVE_FIXTURES + NEGATIVE_FIXTURES) +def test_fixture_exists_and_is_html(name: str) -> None: + path = FIXTURES / name + assert path.is_file(), f"missing fixture {name}" + text = path.read_text(encoding="utf-8") + assert text.lstrip().lower().startswith("") + + +@pytest.mark.parametrize("name", POSITIVE_FIXTURES) +def test_positive_fixture_has_source_flowing_into_a_request_sink(name: str) -> None: + text = (FIXTURES / name).read_text(encoding="utf-8") + assert any(s in text for s in _SOURCE_MARKERS), f"{name}: no client-side source" + assert any(s in text for s in _SINK_MARKERS), f"{name}: no request sink" + # A positive must join a value into the request *path* (concatenation or a + # template literal), not merely attach a query parameter. + joins_into_path = '" + ' in text or "+ '" in text or "${" in text or "this.baseURL + " in text + assert joins_into_path, f"{name}: no value concatenated into the request path" + + +@pytest.mark.parametrize("name", NEGATIVE_FIXTURES) +def test_negative_fixture_is_guarded_or_safe(name: str) -> None: + text = (FIXTURES / name).read_text(encoding="utf-8") + # Still a real client source + sink (so it's a meaningful negative, not a + # blank page), but neutralized. + assert any(s in text for s in _SOURCE_MARKERS), f"{name}: no client-side source" + assert any(s in text for s in _SINK_MARKERS), f"{name}: no request sink" + + if name == "negative_validated.html": + # A strict allowlist guard on the segment before it reaches the sink. + assert "^[a-z0-9-]" in text + assert ".test(" in text + elif name == "negative_safe_construction.html": + # Value used as a query parameter against a constant path, never joined + # into the path. + assert "searchParams.set" in text + assert '"/api/orders/" +' not in text + + +def test_positive_and_negative_fixtures_are_distinct() -> None: + # Guard against a copy-paste that would make a "negative" actually vulnerable. + negatives = {(FIXTURES / n).read_text(encoding="utf-8") for n in NEGATIVE_FIXTURES} + positives = {(FIXTURES / n).read_text(encoding="utf-8") for n in POSITIVE_FIXTURES} + assert negatives.isdisjoint(positives) + + +# --------------------------------------------------------------------------- # +# Categorization separation +# --------------------------------------------------------------------------- # + + +def _cvss() -> dict[str, str]: + return { + "attack_vector": "N", + "attack_complexity": "L", + "privileges_required": "N", + "user_interaction": "R", + "scope": "U", + "confidentiality": "H", + "integrity": "N", + "availability": "N", + } + + +def _create_kwargs(**overrides: Any) -> dict[str, Any]: + base: dict[str, Any] = { + "title": "Client-Side Path Traversal in order-detail fetch", + "description": "Fragment segment reaches a fetch path unvalidated.", + "impact": "Authenticated request steered to a sibling admin endpoint.", + "target": "https://app.example.com", + "technical_analysis": "location.hash is concatenated into the fetch path.", + "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.", + "assumptions": "Assumes the victim opens a crafted link while authenticated.", + "fix_effort": "low", + "cvss_breakdown": _cvss(), + "cwe": "CWE-22", + "endpoint": None, + "method": None, + "cve": None, + "code_locations": None, + } + base.update(overrides) + return base + + +def test_do_create_accepts_cspt_finding_class() -> None: + state = ReportState(run_name="cspt-test") + set_global_report_state(state) + try: + result = asyncio.run( + _do_create(finding_class="client_side_path_traversal", **_create_kwargs()) + ) + finally: + set_global_report_state(None) # type: ignore[arg-type] + + assert result["success"] is True, result + assert len(state.vulnerability_reports) == 1 + assert state.vulnerability_reports[0]["finding_class"] == "client_side_path_traversal" + + +def test_do_create_defaults_finding_class_to_dynamic() -> None: + state = ReportState(run_name="cspt-default") + set_global_report_state(state) + try: + result = asyncio.run(_do_create(**_create_kwargs())) + finally: + set_global_report_state(None) # type: ignore[arg-type] + + assert result["success"] is True, result + assert state.vulnerability_reports[0]["finding_class"] == "dynamic" + + +def test_do_create_rejects_unknown_finding_class() -> None: + result = asyncio.run(_do_create(finding_class="not_a_real_class", **_create_kwargs())) + assert result["success"] is False + assert any("finding_class" in e for e in result["errors"]) + + +def test_sarif_surfaces_cspt_finding_class(tmp_path: Path) -> None: + write_sarif( + tmp_path, + [ + { + "id": "vuln-0001", + "title": "Client-Side Path Traversal in order-detail fetch", + "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}], + } + ], + ) + doc = json.loads((tmp_path / "findings.sarif").read_text(encoding="utf-8")) + strix = doc["runs"][0]["results"][0]["properties"]["strix"] + assert strix["finding_class"] == "client_side_path_traversal" + + +def test_sarif_omits_default_finding_class(tmp_path: Path) -> None: + write_sarif( + tmp_path, + [ + { + "id": "vuln-0001", + "title": "Path Traversal in download handler", + "severity": "high", + "cwe": "CWE-22", + "finding_class": "dynamic", + "timestamp": "2026-07-16 10:00:00 UTC", + "code_locations": [{"file": "app.py", "start_line": 4}], + } + ], + ) + doc = json.loads((tmp_path / "findings.sarif").read_text(encoding="utf-8")) + strix = doc["runs"][0]["results"][0]["properties"]["strix"] + assert "finding_class" not in strix + + +def test_sarif_separates_cspt_from_server_side_traversal_class(tmp_path: Path) -> None: + # Both share CWE-22 (same ruleId), but the class fingerprint must differ so + # cross-run dismissal tooling doesn't collapse a CSPT alert into a + # server-side path traversal alert. + write_sarif( + tmp_path, + [ + { + "id": "vuln-0001", + "title": "Client-Side Path Traversal in order-detail fetch", + "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 and server-side traversal share a class fingerprint" From 302e3442b684c98fb8e21a63ffa2a104ace82229 Mon Sep 17 00:00:00 2001 From: Afraa Shaikh Date: Fri, 17 Jul 2026 01:29:28 +0530 Subject: [PATCH 2/4] 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" From 42eb5684affd6f6b34a9bcca66644d7c16aef0b0 Mon Sep 17 00:00:00 2001 From: Afraa Shaikh Date: Sun, 19 Jul 2026 15:17:18 +0530 Subject: [PATCH 3/4] 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, and use it 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`, and 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 (`client_side_path_traversal`, `path_traversal_lfi_rfi`). - Fixtures: drive `positive_normalized` from the fragment so it serves under a plain static server. - Tests: add `test_cspt_browser_capture.py` (headless Chromium + local request-capture server asserting the exact outbound path per positive and no traversal per negative) and `test_cspt_dataflow.py` (deterministic taint tracker that fails when 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. --- docs/advanced/skills.mdx | 1 + docs/cspt-evidence-runbook.md | 58 +++- pyproject.toml | 10 + strix/report/dedupe.py | 6 + strix/report/sarif.py | 69 +++-- .../client_side_path_traversal.md | 105 +++++-- strix/tools/reporting/tool.py | 21 +- tests/fixtures/cspt/positive_normalized.html | 12 +- tests/test_cspt_browser_capture.py | 239 +++++++++++++++ tests/test_cspt_dataflow.py | 282 ++++++++++++++++++ tests/test_cspt_skill.py | 131 +++++++- 11 files changed, 856 insertions(+), 78 deletions(-) create mode 100644 tests/test_cspt_browser_capture.py create mode 100644 tests/test_cspt_dataflow.py diff --git a/docs/advanced/skills.mdx b/docs/advanced/skills.mdx index 38aacd00..505cdfdc 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` | Client-side path traversal (CSPT), browser request steering | | `open_redirect` | Redirect bypasses, URL parsing tricks | | `mass_assignment` | Attribute injection, hidden parameter pollution | | `insecure_file_uploads` | Upload bypasses, extension tricks | diff --git a/docs/cspt-evidence-runbook.md b/docs/cspt-evidence-runbook.md index be2a2a3a..7c6a15e9 100644 --- a/docs/cspt-evidence-runbook.md +++ b/docs/cspt-evidence-runbook.md @@ -39,7 +39,11 @@ agent-browser wait --load networkidle agent-browser network requests # note the intended path, e.g. /api/orders/12345/detail # 3b. TRAVERSAL — drive the same source with a crafted value. -agent-browser open "/#/orders/..%2f..%2fadmin%2fkeys" +# Use RAW ../ with literal slashes: the browser's URL parser only collapses +# dot-segments delimited by real "/". A percent-encoded ..%2f..%2f stays a +# single literal segment and does NOT traverse unless the app decodes it +# before joining (see the skill's encoding notes). +agent-browser open "/#/orders/../../admin/keys" agent-browser wait --load networkidle agent-browser network requests # note the traversed path, e.g. /admin/keys/detail @@ -68,20 +72,32 @@ agent-browser open "" ## Confirmation criteria -A finding is a real CSPT only when **all** of these hold, evidenced from the HAR: +Confirm at **two independent levels** — don't require impact to accept the +primitive. + +**Level 1 — Confirmed CSPT primitive** (the bug exists), from the HAR: 1. **Traversed path on the wire** — the outbound request path is the traversed 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). -3. **Benign control** — the same flow with a normal value hit the intended path. + (`/api/orders/12345/detail`), because of attacker-controlled client input. +2. **Benign control** — the same flow with a normal value hit the intended path. Show both, side by side. -4. **Reach** — the traversed endpoint returned data or performed an action the - source value should not have been able to reach. -Do not exfiltrate real secrets — a status / length / shape delta against a -low-sensitivity sibling endpoint is enough to prove reach. +This alone establishes path traversal. A traversed request that is +unauthenticated or returns 404 is still a confirmed primitive. + +**Level 2 — Confirmed exploit impact** (severity), one or more of: + +- **Ambient credentials** — the traversed request carried the victim's session + (`Cookie` / `Authorization` header present; read it from the HAR entry). +- **Reach** — the traversed endpoint returned data or performed an action the + source value should not have been able to reach. +- **Chain** — the response feeds an XSS/CORS sink, or the reached endpoint is + state-changing with weak/absent CSRF defense. + +Credentials and reach set severity; they don't decide whether traversal +occurred. Do not exfiltrate real secrets — a status / length / shape delta +against a low-sensitivity sibling endpoint is enough to prove reach. ## Turning it into a report @@ -110,9 +126,11 @@ Traversed (CSPT): ## Demoing against the bundled fixtures -The fixtures in `tests/fixtures/cspt/` are self-contained static pages. Serve the -directory and drive each one to see the positive/negative behavior without a real -target: +The fixtures in `tests/fixtures/cspt/` are self-contained static pages. Every +positive drives its source from the **fragment, query string, `postMessage`, or +`localStorage`** — never from a server-side route — so they all serve correctly +under a plain static server (no SPA fallback needed). Serve the directory and +drive one to see the behavior without a real target: ```bash python -m http.server 8000 --directory tests/fixtures/cspt @@ -122,6 +140,20 @@ agent-browser wait --load networkidle agent-browser network requests # observe the normalized /admin/keys/detail request ``` +The normalization-bypass fixture is driven the same way, via the fragment: + +```bash +agent-browser open "http://localhost:8000/positive_normalized.html#/view/....//....//admin/settings" +``` + +For an automated, reproducible version of this — used as the CI regression that +proves the source-to-sink flow steers the browser's outbound path — see +`tests/test_cspt_browser_capture.py`, which serves each fixture from a local +request-capture server, drives it with headless Chromium (Playwright), and +asserts the exact traversed path the server observed (and that negatives emit +none). `tests/test_cspt_dataflow.py` adds a deterministic taint-flow check that +fails if a fixture's source and sink are present but not connected. + 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 diff --git a/pyproject.toml b/pyproject.toml index 384773f4..019c3fc8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -63,6 +63,12 @@ dev = [ "pyinstaller>=6.17.0; python_version >= '3.12' and python_version < '3.15'", "pytest>=8.3", "pytest-asyncio>=0.24", + # Drives a real headless Chromium for the CSPT browser/outbound-request + # assertions in tests/test_cspt_browser_capture.py. CI must also run + # `playwright install chromium` once; the tests importorskip when the + # runtime or browser build is unavailable so a browserless environment + # still runs the rest of the suite. + "playwright>=1.40", ] [tool.pytest.ini_options] @@ -245,6 +251,10 @@ ignore = [ "strix/interface/tui/app.py" = ["BLE001", "PLC0415", "PLR0912", "PLR0915", "SIM105"] "strix/interface/main.py" = ["BLE001", "PLC0415", "PLR0912", "PLR0915"] "strix/interface/tui/renderers/agent_message_renderer.py" = ["PLC0415"] +# Browser integration harness: broad except around Playwright/Chromium startup +# is an intentional skip-guard so a browserless CI environment degrades to a +# clean skip rather than an error. +"tests/test_cspt_browser_capture.py" = ["BLE001"] [tool.ruff.lint.isort] force-single-line = false diff --git a/strix/report/dedupe.py b/strix/report/dedupe.py index c2970d95..82410d4e 100644 --- a/strix/report/dedupe.py +++ b/strix/report/dedupe.py @@ -45,6 +45,9 @@ CRITICAL DEDUPLICATION RULES: - Different root causes (e.g., stored XSS vs reflected XSS in same field) - Different severity levels due to different impact - One is authenticated, other is unauthenticated + - Different finding_class even under the same CWE (e.g., client_side_path_traversal + vs a server-side/dynamic path-traversal finding both mapped to CWE-22): these are + distinct classes fixed by different code, so they are NOT duplicates 3. ARE DUPLICATES even if: - Titles are worded differently @@ -67,6 +70,8 @@ COMPARISON GUIDELINES: FIELDS TO ANALYZE: - title, description: General vulnerability info - target, endpoint, method: Exact location of vulnerability +- finding_class: Machine-readable vulnerability class; a different finding_class means a + different class of bug even under a shared CWE — not a duplicate - technical_analysis: Root cause details - poc_description: How it's exploited - impact: What damage it can cause @@ -109,6 +114,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 e5030f5b..85b7a38d 100644 --- a/strix/report/sarif.py +++ b/strix/report/sarif.py @@ -890,12 +890,19 @@ 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: two + # CWE-22 findings (a CSPT with finding_class + # "client_side_path_traversal" and a server-side traversal with + # finding_class "dynamic") can share a generic "path traversal" + # title keyword, so keying on the title alone collapses them onto + # the same 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() @@ -918,39 +925,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 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. - - 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. + 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). """ - 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 "" + token = _class_token(report) if not token: return None 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. + + Prefers the structured ``finding_class`` when it carries signal (i.e. + present and not the default ``dynamic``); otherwise falls back to a + keyword extracted from the (LLM-authored) title. Returns 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". Keeping them aligned 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/vulnerabilities/client_side_path_traversal.md b/strix/skills/vulnerabilities/client_side_path_traversal.md index 125433f7..e0dc354d 100644 --- a/strix/skills/vulnerabilities/client_side_path_traversal.md +++ b/strix/skills/vulnerabilities/client_side_path_traversal.md @@ -1,5 +1,5 @@ --- -name: client-side-path-traversal +name: client_side_path_traversal description: Client-Side Path Traversal (CSPT) testing — attacker-controlled client input steering the victim's own authenticated browser requests to unintended same-origin endpoints --- @@ -28,7 +28,7 @@ CSPT becomes account takeover or a stored-XSS delivery path. ## Not CSPT (route these elsewhere) Keep this class clean. The following are **server-side** file/path issues covered -by the `path-traversal-lfi-rfi` skill — report them as `dynamic` findings, never +by the `path_traversal_lfi_rfi` skill — report them as `dynamic` findings, never as `client_side_path_traversal`: - **Server-side path traversal** — server code joins a request parameter into a @@ -116,19 +116,41 @@ Inject into each candidate source and watch the **outbound request path** (via t proxy), not the response body alone — the win condition is the browser sending a path you didn't intend. -- **Direct**: `../`, `../../`, `..%2f` un-decoded, leading `/` (absolute-path +Know exactly what the **browser's** URL parser normalizes vs. what stays literal — +this is where most CSPT reports go wrong: + +- **Browser-normalized dot-segments** (WHATWG URL Standard): the parser collapses a + path segment that equals `..`, `%2e.`, `.%2e`, or `%2e%2e` **only when it is + delimited by literal `/`**. So `../`, `%2e%2e/`, `.%2e/`, `%2e%2e%2e%2e//`-style + self-collapsing sequences all traverse once real slashes separate them. +- **Encoded separators stay encoded**: the browser does **not** turn `%2f` into a + path delimiter during normalization. `..%2f..%2fadmin` is a single literal + segment `..%2f..%2fadmin` — it does **not** become `../../admin`. Payloads like + `..%2f` or `%2e%2e%2f` only traverse if the **application** decodes them before + concatenating into the URL (see below). Do not assume `%2f` traversal without + confirming decode-then-join in the app code or on the wire. +- **Application/framework decoding before URL construction**: if app code runs + `decodeURIComponent`, reads `URLSearchParams.get()` (which decodes), or a router + decodes a route param, an encoded payload becomes raw `../` *before* it reaches + the sink — then the literal-slash rule above applies. Double-encoding + (`%252e%252e%252f`) matters only when the app decodes twice. +- **Direct** (no app decode needed): `../`, `../../`, leading `/` (absolute-path override: `id=/admin/keys` → `/api//admin/keys` or an absolute join). -- **Encoded**: `%2e%2e%2f`, `%2e%2e/`, `..%2f`, double-encoded `%252e%252e%252f` - when an app decodes before joining. The browser decodes `%2e`/`%2f` in the path - during normalization — test both raw and encoded because framework routers may - decode a layer first. -- **Normalized**: `....//` (folds to `../` after one collapse), `..././`, - `.%2e/`, mixed `..\` on apps that rewrite backslashes, redundant `//` and `/./` - segments that shift the base. + +Two more families to try, both using **literal** `/` separators so the browser +parser acts on them: + +- **Self-collapsing / normalization**: `....//` (folds to `../` after a naive + single strip), `..././`, `.%2e/`, mixed `..\` on apps that rewrite backslashes, + redundant `//` and `/./` segments that shift the base. - **Base-relative vs root-relative**: confirm whether the app uses a relative base (`api/x`) or rooted (`/api/x`) — it changes how many `../` are needed and whether a leading `/` fully replaces the path. +Test both raw and encoded because framework routers may decode a layer first — but +attribute any `%2f`-based traversal to an **app-layer decode**, never to the browser +parser. + ## Validation (browser + Caido proxy) Detection is dynamic: drive a real headless browser and read the **final outbound @@ -147,8 +169,9 @@ agent-browser auth login # or state load — see agent_browse agent-browser network har start # 3. Drive the tainted source. Examples per source type: -# - fragment: navigate with a crafted hash -agent-browser open "/#/orders/..%2f..%2fadmin%2fkeys" +# - fragment: navigate with a crafted hash (RAW ../ — literal slashes; the +# browser only collapses dot-segments split by real "/") +agent-browser open "/#/orders/../../admin/keys" # - query: open with a crafted param agent-browser open "/dashboard?tab=../../admin/config" # - postMessage: eval a crafted message into the vulnerable listener @@ -165,16 +188,38 @@ agent-browser network har stop /workspace/.agent-browser-screenshots/cspt.har agent-browser screenshot ``` -**Confirmation criteria** — you have a real CSPT when *all* hold: +Confirmation has **two independent levels**. Keep them separate — conflating +them causes false negatives (a valid primitive dismissed because impact wasn't +demonstrated *yet*). -1. The **outbound request path in the proxy/HAR** is the traversed target - (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 +**Level 1 — Confirmed CSPT primitive** (the bug exists). Both must hold: + +1. The **outbound request path in the proxy/HAR** differs from the intended one + because of attacker-controlled client input — the traversed target + (e.g. `/admin/keys`) appears on the wire, not the intended + `/api/users//avatar`. +2. There is a **benign same-endpoint control**: the same flow with a normal value hits the intended path. Show both paths side by side. -4. The traversed endpoint returned data (or performed an action) the source - value should not have been able to reach. + +That's enough to establish path traversal occurred. Credentials and a successful +response are **not** required for the primitive to be real — a traversed request +that is unauthenticated or returns 404 still proves attacker input steered the +browser's request path, and may become exploitable when chained. + +**Level 2 — Confirmed exploit impact** (what it currently demonstrates). One or +more of: + +- The traversed request carried the **victim's credentials** (session cookie / + `Authorization` header on the request — read it from the HAR), and +- The traversed endpoint **returned data or performed an action** the source + value should not have reached, or +- The primitive **participates in a demonstrated chain**: response rendered into + a sink (XSS), a permissive CORS reader, a state-changing endpoint with weak/no + CSRF defense, or application-added authorization on the reached path. + +Credentials, reach, and chaining drive **reportability and severity** — they do +not decide *whether* traversal happened. Report a confirmed primitive; let the +impact level set the CVSS and how loudly you rate it. Record the request/response pair (method, full path, relevant headers, status) from the HAR as evidence. Prefer reading a low-sensitivity sibling endpoint to @@ -203,7 +248,7 @@ File confirmed CSPT with `create_vulnerability_report` and: ## False positives -Not CSPT — do not report: +**Not CSPT at all** — the primitive doesn't exist, do not report: - The source is **validated to the resulting path**: an allowlist of endpoints, a strict `^[a-z0-9-]+$` segment check, or `encodeURIComponent` on a value used @@ -214,11 +259,19 @@ Not CSPT — do not report: value only ever becomes a **query parameter or request body field**, never part of the path. A tainted query value is not CSPT (it may be another bug). - The browser **does not normalize** the value into a different path (confirm on - the wire — no traversal actually occurred in the outbound request). -- The traversed request is **not** sent with ambient credentials (no session on - that request), so there's no privilege to abuse — note it, don't over-rate it. -- Server rejects the traversed path (404/403) with no differential — no reach, no - finding. + the wire — no traversal actually occurred in the outbound request). This is the + real negative: if the outbound path is unchanged, there is no primitive. + +**Primitive exists but low/unproven impact** — this is *still CSPT*; report it, +but rate it honestly and say what's missing (do NOT silently drop it): + +- The traversed request carries **no ambient credentials** (no session on that + request). The path was still steered — report the primitive at reduced severity + and note the missing privilege, or look for a chain that supplies impact. +- Server **rejects** the traversed path (404/403) with no differential. Reach + isn't demonstrated *yet*, but the primitive is confirmed. Report it as a + primitive and note that impact depends on a reachable sibling endpoint or a + chain (response rendering, CORS reader, state-changing target). ## Pro tips diff --git a/strix/tools/reporting/tool.py b/strix/tools/reporting/tool.py index dcaef337..11f93bdb 100644 --- a/strix/tools/reporting/tool.py +++ b/strix/tools/reporting/tool.py @@ -164,7 +164,7 @@ _VALID_FIX_EFFORT = frozenset({"trivial", "low", "medium", "high"}) _VALID_FINDING_CLASS = frozenset({"dynamic", "client_side_path_traversal"}) -async def _do_create( # noqa: PLR0912 +async def _do_create( # noqa: PLR0912, PLR0915 *, title: str, description: str, @@ -218,6 +218,22 @@ async def _do_create( # noqa: PLR0912 f"{sorted(_VALID_FINDING_CLASS)}" ) + # CSPT findings are typically locationless (the bug lives in a JS bundle, + # not a server route), so they anchor to SECURITY.md in SARIF. Without a + # discriminator, multiple distinct CSPT findings collapse onto the same + # synthetic fingerprint. Require the traversed endpoint/method or a + # physical code location so each one stays a separate alert. + if finding_class == "client_side_path_traversal": + has_endpoint = bool(str(endpoint or "").strip()) + has_location = bool(_normalize_code_locations(code_locations)) + if not (has_endpoint or has_location): + errors.append( + "finding_class 'client_side_path_traversal' requires a discriminator: " + "set endpoint (the traversed target path, e.g. '/admin/keys') and method, " + "or a code_location for the vulnerable client sink. Locationless CSPT " + "findings otherwise collapse onto one SARIF fingerprint." + ) + if not isinstance(cvss_breakdown, dict) or not cvss_breakdown: errors.append("cvss_breakdown: must be an object with the 8 CVSS metrics") cvss_breakdown = {} @@ -271,6 +287,7 @@ async def _do_create( # noqa: PLR0912 "poc_script_code": poc_script_code, "endpoint": endpoint, "method": method, + "finding_class": finding_class, } dedupe = await check_duplicate(candidate, existing) if dedupe.get("is_duplicate"): @@ -575,7 +592,7 @@ async def create_vulnerability_report( 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 / LFI / RFI (leave those - ``dynamic``). See the ``client-side-path-traversal`` skill. + ``dynamic``). See the ``client_side_path_traversal`` skill. Example (abbreviated — mirror this structure):: diff --git a/tests/fixtures/cspt/positive_normalized.html b/tests/fixtures/cspt/positive_normalized.html index c0a3e20d..f32a7767 100644 --- a/tests/fixtures/cspt/positive_normalized.html +++ b/tests/fixtures/cspt/positive_normalized.html @@ -1,11 +1,17 @@ Profile @@ -13,8 +19,8 @@

Profile

loading…
", html, re.DOTALL)) + + +def _balanced_args(src: str, open_paren_idx: int) -> str: + """Return the full argument-list text between an opening paren and its + matching close paren (exclusive).""" + depth = 0 + out: list[str] = [] + i = open_paren_idx + while i < len(src): + ch = src[i] + if ch == "(": + depth += 1 + if depth == 1: + i += 1 + continue + elif ch == ")": + depth -= 1 + if depth == 0: + break + out.append(ch) + i += 1 + return "".join(out) + + +def _split_top_level_args(arglist: str) -> list[str]: + """Split an argument-list string on top-level commas (ignoring commas nested + in (), [], {}, or string/template literals).""" + args: list[str] = [] + depth = 0 + quote: str | None = None + buf: list[str] = [] + for ch in arglist: + if quote: + buf.append(ch) + if ch == quote: + quote = None + continue + if ch in "\"'`": + quote = ch + buf.append(ch) + continue + if ch in "([{": + depth += 1 + elif ch in ")]}": + depth -= 1 + if ch == "," and depth == 0: + args.append("".join(buf)) + buf = [] + continue + buf.append(ch) + if buf: + args.append("".join(buf)) + return args + + +def _tainted_vars(script: str) -> set[str]: + """Fixed-point propagation of taint through ``const/let/var X = ``. + + Seeds on source expressions, then repeatedly taints any assignment whose + right-hand side references an already-tainted identifier. ``event.data`` + accesses inside a message handler seed the handler's derived var too. + """ + assign_re = re.compile( + r"\b(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(.+?);", + re.DOTALL, + ) + assignments = [(m.group(1), m.group(2)) for m in assign_re.finditer(script)] + + tainted: set[str] = set() + # Seed: direct source expression on the RHS. + for name, rhs in assignments: + if _SOURCE_EXPR.search(rhs): + tainted.add(name) + + # Propagate to a fixed point: RHS references a tainted identifier. + changed = True + while changed: + changed = False + for name, rhs in assignments: + if name in tainted: + continue + idents = set(_JS_IDENT.findall(rhs)) + if idents & tainted: + tainted.add(name) + changed = True + return tainted + + +def _guarded_idents(script: str) -> set[str]: + """Identifiers gated by an allowlist ``.test(ident)`` guard before a sink.""" + guarded: set[str] = set() + if _ALLOWLIST_REGEX_LITERAL.search(script): + for m in _TEST_GUARD.finditer(script): + guarded.add(m.group(1)) + return guarded + + +def _sink_path_args(script: str) -> list[str]: + """The URL/path argument text of every request sink call in the script. + + The URL is the 1st argument for ``fetch``/``sendBeacon``/``EventSource``/ + ``WebSocket`` and the 2nd for ``xhr.open(method, url)``. + """ + args: list[str] = [] + for m in _SINK_CALL.finditer(script): + open_idx = script.index("(", m.end() - 1) + arglist = _balanced_args(script, open_idx) + parts = _split_top_level_args(arglist) + if not parts: + continue + url_arg = parts[1] if m.group("open") and len(parts) > 1 else parts[0] + args.append(url_arg) + return args + + +def _tainted_flows_into_sink_path(script: str) -> bool: + """True iff a tainted identifier is concatenated into a sink's path arg. + + Requires an actual join into the path (``+`` concatenation or a template + literal) so that a tainted value used *only* as a query parameter (e.g. + ``searchParams.set(k, tainted)`` on a constant-path URL) does not count. + """ + tainted = _tainted_vars(script) + guarded = _guarded_idents(script) + live = tainted - guarded + if not live: + return False + + # An axios-style wrapper joins ``baseURL + path``; the tainted value reaches + # the sink transitively through the wrapper argument. Treat a call to such a + # wrapper method (``.get("/x/" + tainted)``) as a path-join sink too. + wrapper_join = re.compile(r"\.get\s*\(") + + candidate_args = list(_sink_path_args(script)) + for m in wrapper_join.finditer(script): + open_idx = script.find("(", m.end() - 1) + if open_idx != -1: + parts = _split_top_level_args(_balanced_args(script, open_idx)) + if parts: + candidate_args.append(parts[0]) + + for arg in candidate_args: + joins_path = "+" in arg or "${" in arg or "`" in arg + if not joins_path: + continue + if set(_JS_IDENT.findall(arg)) & live: + return True + return False + + +# --------------------------------------------------------------------------- # +# Tests +# --------------------------------------------------------------------------- # + + +@pytest.mark.parametrize("name", POSITIVE_FIXTURES) +def test_positive_fixture_source_flows_into_sink_path(name: str) -> None: + script = _script_body((FIXTURES / name).read_text(encoding="utf-8")) + assert _tainted_flows_into_sink_path(script), ( + f"{name}: no data flow from a client source into a request sink's path — " + "source and sink present but not connected" + ) + + +@pytest.mark.parametrize("name", NEGATIVE_FIXTURES) +def test_negative_fixture_flow_is_broken(name: str) -> None: + script = _script_body((FIXTURES / name).read_text(encoding="utf-8")) + assert not _tainted_flows_into_sink_path(script), ( + f"{name}: a tainted value reaches a request sink's path — negative fixture " + "is not actually neutralized" + ) + + +def test_analyzer_detects_a_disconnected_source_and_sink() -> None: + # Meta-test: the analyzer must NOT flag a page where the source and sink are + # both present but unrelated (the exact gap the structural check misses). + disconnected = """ + const tainted = location.hash.slice(1); + console.log(tainted); // taint goes nowhere + fetch("/api/orders/12345/detail", { credentials: "include" }); + """ + assert not _tainted_flows_into_sink_path(disconnected) + + +def test_analyzer_flags_a_connected_source_and_sink() -> None: + # ...and it MUST flag the connected version, so the negative above is real. + connected = """ + const tainted = location.hash.slice(1); + fetch("/api/orders/" + tainted + "/detail", { credentials: "include" }); + """ + assert _tainted_flows_into_sink_path(connected) diff --git a/tests/test_cspt_skill.py b/tests/test_cspt_skill.py index eef9e217..c31c76c7 100644 --- a/tests/test_cspt_skill.py +++ b/tests/test_cspt_skill.py @@ -3,7 +3,7 @@ Covers the three in-repo pillars of issue #776 that can run in CI without a live browser/proxy: -1. The ``client-side-path-traversal`` skill exists, loads, and documents the +1. The ``client_side_path_traversal`` skill exists, loads, and documents the source/sink taxonomy plus the "Not CSPT" boundary the agent relies on. 2. The HTML fixtures encode a real source->sink flow (positives) or a validation guard / safe construction (negatives), asserted structurally so they can't @@ -23,6 +23,7 @@ from typing import Any import pytest +from strix.report.dedupe import _prepare_report_for_comparison from strix.report.sarif import write_sarif from strix.report.state import ReportState, set_global_report_state from strix.skills import get_all_skill_names, get_available_skills, load_skills @@ -196,9 +197,10 @@ def test_do_create_accepts_cspt_finding_class() -> None: state = ReportState(run_name="cspt-test") set_global_report_state(state) try: - result = asyncio.run( - _do_create(finding_class="client_side_path_traversal", **_create_kwargs()) - ) + # CSPT requires a discriminator (endpoint or code location); supply the + # traversed endpoint. + kwargs = _create_kwargs(endpoint="/admin/keys", method="GET") + result = asyncio.run(_do_create(finding_class="client_side_path_traversal", **kwargs)) finally: set_global_report_state(None) # type: ignore[arg-type] @@ -225,6 +227,41 @@ def test_do_create_rejects_unknown_finding_class() -> None: assert any("finding_class" in e for e in result["errors"]) +def test_do_create_cspt_requires_endpoint_or_location() -> None: + # A CSPT finding with no discriminator (no endpoint, no code_locations) is + # rejected so multiple locationless CSPT findings can't collapse onto one + # SARIF fingerprint. + kwargs = _create_kwargs(endpoint=None, method=None, code_locations=None) + result = asyncio.run(_do_create(finding_class="client_side_path_traversal", **kwargs)) + assert result["success"] is False + assert any("discriminator" in e for e in result["errors"]), result + + +def test_do_create_cspt_accepts_with_endpoint() -> None: + state = ReportState(run_name="cspt-endpoint") + set_global_report_state(state) + try: + kwargs = _create_kwargs(endpoint="/admin/keys", method="GET", code_locations=None) + result = asyncio.run(_do_create(finding_class="client_side_path_traversal", **kwargs)) + finally: + set_global_report_state(None) # type: ignore[arg-type] + assert result["success"] is True, result + + +def test_dedupe_comparison_payload_includes_finding_class() -> None: + # The structured discriminator must reach the dedup judge; without it a CSPT + # and a server-side traversal on the same endpoint look identical. + cleaned = _prepare_report_for_comparison( + { + "id": "vuln-1", + "title": "Path Traversal", + "endpoint": "/admin/keys", + "finding_class": "client_side_path_traversal", + } + ) + assert cleaned.get("finding_class") == "client_side_path_traversal" + + def test_sarif_surfaces_cspt_finding_class(tmp_path: Path) -> None: write_sarif( tmp_path, @@ -341,6 +378,92 @@ def test_sarif_class_fingerprint_uses_finding_class_over_colliding_title(tmp_pat ) +def test_sarif_primary_fingerprint_separates_locationless_cspt(tmp_path: Path) -> None: + # Regression for the *primary* fingerprint (partialFingerprints + # .primaryLocationLineHash), not just the custom class property. Both + # findings are locationless (no code_locations, no endpoint/method) so they + # anchor to SECURITY.md and share the CWE-22 ruleId. Only the structured + # finding_class distinguishes them. Before the fix the synthetic branch + # derived its class token from the title keyword alone, so a CSPT titled + # "Path Traversal via fragment" collapsed onto the server-side traversal's + # primary fingerprint — hiding a real, separate alert from code-scanning + # reconciliation. The primary fingerprints must differ. + write_sarif( + tmp_path, + [ + { + "id": "vuln-0001", + "title": "Path Traversal via fragment", # generic keyword collision + "severity": "high", + "cwe": "CWE-22", + "finding_class": "client_side_path_traversal", + "timestamp": "2026-07-16 10:00:00 UTC", + "code_locations": None, + "endpoint": None, + "method": None, + }, + { + "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": None, + "endpoint": None, + "method": None, + }, + ], + ) + results = json.loads((tmp_path / "findings.sarif").read_text(encoding="utf-8"))["runs"][0][ + "results" + ] + primary = [r.get("partialFingerprints", {}).get("primaryLocationLineHash") for r in results] + assert all(primary), "expected a primary fingerprint on both locationless findings" + assert primary[0] != primary[1], ( + "locationless CSPT and server-side traversal share a primary fingerprint; " + "the synthetic-location class token must prefer finding_class" + ) + + +def test_sarif_primary_fingerprint_separates_cspt_by_endpoint(tmp_path: Path) -> None: + # Two locationless CSPT findings that differ only by endpoint must stay + # distinct, so multiple CSPT findings don't collapse onto one alert. + write_sarif( + tmp_path, + [ + { + "id": "vuln-0003", + "title": "Client-Side Path Traversal", + "severity": "high", + "cwe": "CWE-22", + "finding_class": "client_side_path_traversal", + "timestamp": "2026-07-16 10:00:00 UTC", + "endpoint": "/admin/keys", + "method": "GET", + }, + { + "id": "vuln-0004", + "title": "Client-Side Path Traversal", + "severity": "high", + "cwe": "CWE-22", + "finding_class": "client_side_path_traversal", + "timestamp": "2026-07-16 10:00:00 UTC", + "endpoint": "/admin/config", + "method": "GET", + }, + ], + ) + results = json.loads((tmp_path / "findings.sarif").read_text(encoding="utf-8"))["runs"][0][ + "results" + ] + primary = [r.get("partialFingerprints", {}).get("primaryLocationLineHash") for r in results] + assert all(primary) + assert primary[0] != primary[1], ( + "two CSPT findings on different traversed endpoints collapsed onto one primary fingerprint" + ) + + # --------------------------------------------------------------------------- # # Fixture path normalization (documented expected paths must be correct) # --------------------------------------------------------------------------- # From c0cc1a82c9ecb9488b6a10f35f9f84fb24ee06fe Mon Sep 17 00:00:00 2001 From: Afraa Shaikh Date: Sun, 19 Jul 2026 16:10:40 +0530 Subject: [PATCH 4/4] 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