fix: wildcard CORS + credentials spec violation, path-traversal guard on /icons proxy

[DevamShah]

Closes #298

Summary

Remove the allow_credentials=True / allow_origins=["*"] spec-invalid combo and add a strict allowlist + path-containment check on the /icons/{icon_name} endpoint before any filesystem write or outbound fetch.

Problem / motivation

Two interlocking issues were filed in #298:

1. CORS (middleware/cors.py:10)

origins = ["*"]
app.add_middleware(CORSMiddleware, allow_origins=origins, allow_credentials=True, ...)

allow_origins=["*"] combined with allow_credentials=True is explicitly forbidden by the CORS spec (Fetch §4.7, RFC 6454 §7.2). Browsers respond by silently stripping credentials on every cross-origin request, so the configuration neither works as intended nor provides any actual access control. The code reads as "any origin may make authenticated requests" but behaves as "any origin may make unauthenticated requests." Because the app uses Bearer tokens in request headers (not cookies), allow_credentials is unnecessary here.

2. Path traversal / arbitrary-file-write surface (routes/static.py:100)

icon_path = ICONS_DIR / icon_name     # unsanitized path join
url = f"https://registry.npmmirror.com/.../dark/{icon_name}"  # unsanitized URL path
...
icon_path.write_bytes(response.content)   # writes attacker-controlled name

icon_name is taken directly from the URL path parameter and interpolated into both a filesystem path and an outbound URL with no validation. FastAPI's single-segment path parameters normally block literal /, but %2F-encoded slashes are handled inconsistently across Starlette versions and can decode to directory separators. The combined write surface represents CWE-22 (path traversal / arbitrary write) and a constrained SSRF-adjacent risk (controlled path component against a fixed host).

Change

agentic_security/middleware/cors.py

• Removed allow_credentials=True. Wildcard origins remain; credentialed cross-origin access was not required and the setting was non-functional.

agentic_security/routes/static.py

• Added module-level ICON_NAME_RE = re.compile(r"^[A-Za-z0-9._-]+\.png$").
• serve_icon() now validates icon_name against ICON_NAME_RE before any I/O; returns HTTP 400 on mismatch.
• After path join, resolves the path and asserts is_relative_to(ICONS_DIR.resolve()) as a defense-in-depth containment check; returns HTTP 400 if outside the icons directory. This layer defends against any future URL-handling change that could pass through encoded separators.

tests/unit/test_cors_middleware.py (new)

• Asserts CORSMiddleware is registered.
• Asserts wildcard origins are not paired with allow_credentials=True.
• Asserts OPTIONS preflight returns CORS headers.
• Asserts Access-Control-Allow-Credentials: true is absent when ACAO: *.

tests/integration/routes/test_static_icon_validation.py (new)

• TestIconNameRegex: parametrized unit tests for 7 valid and 11 invalid name patterns including path traversal, encoded slashes, wrong extensions, null bytes, and double extensions.
• TestServeIconValidation: integration tests via TestClient confirming 400 for invalid names, pass-through to 404 (not 400) for a valid-but-absent icon, and 200 for a cached icon (mocked FS).

Security rationale

Finding	CWE	Impact
CORS wildcard + credentials	CWE-942 (Overly Permissive CORS)	Broken access control; credentials silently dropped — OWASP A01:2021
Icon path traversal + write	CWE-22 (Path Traversal)	Arbitrary write within the server's FS permissions — OWASP A03:2021
Icon URL path injection	CWE-73 (External Control of File Name or Path)	Controlled path against fixed upstream host

The regex allowlist follows the principle of positive validation (allowlist over denylist), which is the OWASP-recommended approach for filename inputs. The is_relative_to check is a belt-and-suspenders layer that survives any future URL-decoding changes upstream.

Testing / validation

python3 -m pytest tests/unit/test_cors_middleware.py \
                  tests/integration/routes/test_static_icon_validation.py \
                  tests/integration/routes/test_static.py -v

Result: 36 passed (new CORS + icon-validation tests plus the existing test_static.py suite), no regressions. icon_name is matched with re.fullmatch against an anchorless allowlist so a trailing newline (icon.png\n) is also rejected.

Regex positive/negative coverage verified independently:

Input	Expected	Result
openai.png	MATCH	MATCH
claude-3.png	MATCH	MATCH
../etc/passwd	NO MATCH	NO MATCH
../../secret.png	NO MATCH	NO MATCH
foo%2Fbar.png	NO MATCH	NO MATCH
icon.PNG	NO MATCH	NO MATCH
icon.png.sh	NO MATCH	NO MATCH
.png (empty stem)	NO MATCH	NO MATCH

[jackspiece]

I reproduced the 36 focused tests, plus the full suite at 402 passed, 5 skipped, and 41 deselected. The icon allowlist and containment check look sound. One CORS concern remains: dropping allow_credentials fixes the invalid header combination, but allow_origins=["*"] still permits browser requests to unauthenticated state-changing endpoints such as /verify, /scan, and /stop when network policy allows them. The bundled UI derives SELF_URL from window.location.href, so it appears to be same-origin. Is cross-origin access required elsewhere? If not, removing CORS or using an explicit configurable allowlist would close the more important browser-to-local-service surface. Otherwise, I suggest tracking that separately and documenting the expected cross-origin client.

[msoedov]

@DevamShah @jackspiece Thank you for the patch and testing

[DevamShah]

@jackspiece your CORS point was correct, and it turned out to be worse than I assumed when I scoped this PR to the header-combination bug. I went back and checked it against main at 1ef1314.

Confirming your reading first:

• middleware/cors.py:6 — origins = ["*"] still stands, with allow_methods=["*"] and allow_headers=["*"].
• routes/scan.py — /verify (:28), /scan (:67), /stop (:79) and /scan-csv (:85) are unauthenticated and state-changing. require_auth is defined at core/security.py:146 and applied to nothing; grep -rn "require_auth" . --include="*.py" returns only the definition.
• static/base.js:1 — let SELF_URL = window.location.href;, so the bundled UI is same-origin exactly as you said. Nothing shipped needs the wildcard.
• __main__.py:13 — default bind is 127.0.0.1:8718, which is what makes this a browser-to-local-service problem rather than a network one.

The part I had not appreciated: /verify and /scan-csv take an operator-supplied HTTP spec, and parse_http_spec interpolates the locally configured secrets into the spec body at http_spec.py:234 before dispatching to whatever URL the spec names. So this is not only drive-by CSRF, it is a key-egress path. I tested it with a canary in OPENAI_API_KEY and a local sink: cross-origin POST /verify returned 200 under ACAO: * and the sink received the real value. Preflight OPTIONS on /stop, /verify and /scan all answer 200 with ACAO: *.

To your question — no, cross-origin access is not required anywhere I can find. So the answer is the explicit configurable allowlist you suggested, defaulting to empty rather than *.

One thing worth adding to your framing: the allowlist alone does not finish the job. CORS governs whether the browser exposes the response, not whether the request is sent, and two of these endpoints are CORS simple requests that are never preflighted — /stop has no body so fetch() sets no Content-Type, and /scan-csv is multipart/form-data. With the allowlist patch applied and no origins configured, a foreign-origin POST /scan-csv still returned 200 and still delivered the canary to my sink. /verify and /scan are genuinely covered by the allowlist because they 422 on Content-Type: text/plain and so do require a preflight. Closing /stop and /scan-csv needs an Origin check on unsafe methods (absent Origin → allow, so curl and the CLI are unaffected).

I have filed that as #334 with the full write-up, and pushed both parts to DevamShah:harden-cors-origin-allowlist (d888659). pytest tests/unit/test_cors_middleware.py tests/unit/test_origin_guard.py -q → 23 passed; full suite pytest tests -q -p no:randomly → 407 passed with one pre-existing failure and one pre-existing collection error that I confirmed reproduce on unmodified main.

I deliberately did not touch authentication. Adding auth to a single-operator localhost tool is a product decision for @msoedov, and the Origin guard closes the browser surface without it.