fix: wildcard CORS + credentials spec violation, path-traversal guard on /icons proxy#320
Open
DevamShah wants to merge 1 commit into
Open
Conversation
… 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)
allow_origins=["*"]combined withallow_credentials=Trueis 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_credentialsis unnecessary here.2. Path traversal / arbitrary-file-write surface (routes/static.py:100)
icon_nameis 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.pyallow_credentials=True. Wildcard origins remain; credentialed cross-origin access was not required and the setting was non-functional.agentic_security/routes/static.pyICON_NAME_RE = re.compile(r"^[A-Za-z0-9._-]+\.png$").serve_icon()now validatesicon_nameagainstICON_NAME_REbefore any I/O; returns HTTP 400 on mismatch.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)CORSMiddlewareis registered.allow_credentials=True.Access-Control-Allow-Credentials: trueis absent whenACAO: *.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 viaTestClientconfirming 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
The regex allowlist follows the principle of positive validation (allowlist over denylist), which is the OWASP-recommended approach for filename inputs. The
is_relative_tocheck is a belt-and-suspenders layer that survives any future URL-decoding changes upstream.Testing / validation
Result: 36 passed (new CORS + icon-validation tests plus the existing
test_static.pysuite), no regressions.icon_nameis matched withre.fullmatchagainst an anchorless allowlist so a trailing newline (icon.png\n) is also rejected.Regex positive/negative coverage verified independently:
openai.pngclaude-3.png../etc/passwd../../secret.pngfoo%2Fbar.pngicon.PNGicon.png.sh.png(empty stem)