Skip to content

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

Open
DevamShah wants to merge 1 commit into
msoedov:mainfrom
DevamShah:fix-cors-credentials-and-icon-traversal
Open

fix: wildcard CORS + credentials spec violation, path-traversal guard on /icons proxy#320
DevamShah wants to merge 1 commit into
msoedov:mainfrom
DevamShah:fix-cors-credentials-and-icon-traversal

Conversation

@DevamShah

Copy link
Copy Markdown

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

… on /icons proxy

CORS: drop allow_credentials=True. With allow_origins=['*'] Starlette reflects
the request Origin and emits Access-Control-Allow-Credentials: true on any
credentialed request — a reflect-any-origin hole. The app authenticates with
Bearer tokens in the Authorization header, so credentialed CORS was never needed.

/icons/{icon_name}: validate against a strict allowlist (re.fullmatch
[A-Za-z0-9._-]+\.png) and assert the resolved path stays inside ICONS_DIR before
any filesystem write or outbound fetch. Closes the path-traversal / arbitrary-write
(CWE-22) and controlled-URL (CWE-73) surface on the unsanitized icon_name.

Adds CORS middleware tests and icon-name validation tests (traversal, encoded
slash, null byte, trailing newline, wrong extension).

Closes msoedov#298

Signed-off-by: Devam Shah <devamshah91@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Best-practice: wildcard CORS w/ credentials + path-traversal defense-in-depth on /icons proxy

1 participant