From dd59704fdfddeae646cc4b4e4ac883e14fffbd33 Mon Sep 17 00:00:00 2001 From: Devam Shah Date: Mon, 22 Jun 2026 10:10:43 +0530 Subject: [PATCH] fix: wildcard CORS + credentials spec violation, path-traversal guard on /icons proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #298 Signed-off-by: Devam Shah --- agentic_security/middleware/cors.py | 1 - agentic_security/routes/static.py | 28 ++++- .../routes/test_static_icon_validation.py | 109 ++++++++++++++++++ tests/unit/test_cors_middleware.py | 94 +++++++++++++++ 4 files changed, 229 insertions(+), 3 deletions(-) create mode 100644 tests/integration/routes/test_static_icon_validation.py create mode 100644 tests/unit/test_cors_middleware.py diff --git a/agentic_security/middleware/cors.py b/agentic_security/middleware/cors.py index 5390fe77..fb65051d 100644 --- a/agentic_security/middleware/cors.py +++ b/agentic_security/middleware/cors.py @@ -8,7 +8,6 @@ def setup_cors(app: FastAPI): app.add_middleware( CORSMiddleware, allow_origins=origins, - allow_credentials=True, allow_methods=["*"], # Allows all methods allow_headers=["*"], # Allows all headers ) diff --git a/agentic_security/routes/static.py b/agentic_security/routes/static.py index b5a838e0..9cc63133 100644 --- a/agentic_security/routes/static.py +++ b/agentic_security/routes/static.py @@ -1,3 +1,4 @@ +import re from pathlib import Path import requests @@ -13,6 +14,11 @@ STATIC_DIR = Path(__file__).parent.parent / "static" ICONS_DIR = STATIC_DIR / "icons" +# Strict allowlist for icon filenames: lowercase/uppercase alphanumerics, dots, +# underscores, and hyphens, must end in .png. Rejects path separators, +# percent-encoded characters, and non-PNG extensions before any FS/network I/O. +ICON_NAME_RE = re.compile(r"[A-Za-z0-9._-]+\.png") + # Configure templates with custom delimiters to avoid conflicts templates = Jinja2Templates(directory=str(STATIC_DIR)) templates.env = Environment( @@ -96,8 +102,26 @@ async def favicon() -> FileResponse: @router.get("/icons/{icon_name}") async def serve_icon(icon_name: str) -> FileResponse: - """Serve an icon from the icons directory.""" - icon_path = ICONS_DIR / icon_name + r"""Serve an icon from the icons directory. + + ``icon_name`` is validated against a strict allowlist before any filesystem + or outbound-HTTP access: + + * Must match ``^[A-Za-z0-9._-]+\.png$`` — rejects path separators, + percent-encoded characters, non-PNG extensions, and empty names. + * The resolved path must stay inside ``ICONS_DIR`` — defense-in-depth + against any future URL-handling change that could decode ``%2F``. + + Mitigates CWE-22 (path traversal) on both the local write and the + upstream npmmirror fetch. + """ + if not ICON_NAME_RE.fullmatch(icon_name): + raise HTTPException(status_code=400, detail="Invalid icon name") + + icon_path = (ICONS_DIR / icon_name).resolve() + if not icon_path.is_relative_to(ICONS_DIR.resolve()): + raise HTTPException(status_code=400, detail="Invalid icon name") + if not icon_path.exists(): # Fetch the icon from the external URL and cache it url = f"https://registry.npmmirror.com/@lobehub/icons-static-png/latest/files/dark/{icon_name}" diff --git a/tests/integration/routes/test_static_icon_validation.py b/tests/integration/routes/test_static_icon_validation.py new file mode 100644 index 00000000..06aad62e --- /dev/null +++ b/tests/integration/routes/test_static_icon_validation.py @@ -0,0 +1,109 @@ +"""Tests for the /icons/{icon_name} allowlist and path-containment guard (CWE-22).""" + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from agentic_security.routes.static import ICON_NAME_RE, router + +_app = FastAPI() +_app.include_router(router) +_client = TestClient(_app, raise_server_exceptions=False) + + +# --------------------------------------------------------------------------- +# ICON_NAME_RE unit tests — no HTTP round-trip needed +# --------------------------------------------------------------------------- + + +class TestIconNameRegex: + """ICON_NAME_RE rejects all names that could cause path traversal or SSRF.""" + + @pytest.mark.parametrize( + "name", + [ + "openai.png", + "claude-3.png", + "gpt_4.png", + "Anthropic.png", + "my.icon.png", + "test-icon-123.png", + "A.png", + ], + ) + def test_valid_names_match(self, name: str): + assert ICON_NAME_RE.fullmatch(name), f"Expected {name!r} to be accepted" + + @pytest.mark.parametrize( + "name", + [ + "../etc/passwd", # classic path traversal + "../../secret.png", # multi-level traversal + "foo%2Fbar.png", # percent-encoded slash + "foo/bar.png", # literal slash + "icon.PNG", # uppercase extension (not in the dataset) + "icon.jpg", # wrong extension + "icon.gif", # wrong extension + ".png", # empty stem + "", # empty string + "icon.png.sh", # double extension — shell script suffix + "\x00icon.png", # null byte + "icon.png\n", # trailing newline (defeats $-anchored match) + ], + ) + def test_invalid_names_rejected(self, name: str): + assert not ICON_NAME_RE.fullmatch(name), f"Expected {name!r} to be rejected" + + +# --------------------------------------------------------------------------- +# Integration tests — HTTP-level validation via TestClient +# --------------------------------------------------------------------------- + + +class TestServeIconValidation: + """serve_icon returns 400 for names that fail the allowlist check.""" + + @pytest.mark.parametrize( + "bad_name", + [ + "no-extension", + "icon.jpg", + "icon.PNG", + ".png", + "icon.png.sh", + ], + ) + def test_invalid_name_returns_400(self, bad_name: str): + """Names that fail ICON_NAME_RE get a 400 before any FS/network I/O.""" + response = _client.get(f"/icons/{bad_name}") + assert ( + response.status_code == 400 + ), f"Expected 400 for {bad_name!r}, got {response.status_code}" + assert response.json().get("detail") == "Invalid icon name" + + def test_valid_name_does_not_return_400(self, tmp_path, mocker): + """A well-formed name passes validation; 404 is acceptable when the + upstream fetch also fails, but 400 must not be returned.""" + mocker.patch("agentic_security.routes.static.ICONS_DIR", tmp_path) + + # Stub the external HTTP call so the test is hermetic + mock_resp = mocker.MagicMock() + mock_resp.status_code = 404 + mocker.patch( + "agentic_security.routes.static.requests.get", return_value=mock_resp + ) + + response = _client.get("/icons/openai.png") + assert ( + response.status_code != 400 + ), "A valid icon name should not be rejected by the allowlist check" + + def test_valid_name_served_from_cache(self, tmp_path, mocker): + """When the icon file already exists locally it is served directly.""" + mocker.patch("agentic_security.routes.static.ICONS_DIR", tmp_path) + icon_file = tmp_path / "openai.png" + icon_file.write_bytes(b"\x89PNG\r\n\x1a\n") # minimal PNG magic bytes + + response = _client.get("/icons/openai.png") + assert response.status_code == 200 + assert response.headers.get("content-type", "").startswith("image/png") diff --git a/tests/unit/test_cors_middleware.py b/tests/unit/test_cors_middleware.py new file mode 100644 index 00000000..e8d0696f --- /dev/null +++ b/tests/unit/test_cors_middleware.py @@ -0,0 +1,94 @@ +"""Unit tests for CORS middleware configuration. + +Verifies that the wildcard-origins + allow_credentials=True spec violation +(CORS spec §3.2, Fetch §4.7) has been removed. Browsers silently strip +credentials when the response carries Access-Control-Allow-Origin: * paired +with Access-Control-Allow-Credentials: true, so the old config was both +broken and misleading. +""" + +from fastapi import FastAPI +from fastapi.middleware.cors import CORSMiddleware +from fastapi.testclient import TestClient + +from agentic_security.middleware.cors import setup_cors + + +def _get_cors_options(app: FastAPI) -> dict: + """Extract CORS middleware options from the app's middleware stack.""" + for middleware in app.user_middleware: + if middleware.cls is CORSMiddleware: + return middleware.kwargs + return {} + + +class TestCorsSetup: + """CORS middleware is configured correctly.""" + + def test_cors_middleware_is_registered(self): + """setup_cors adds CORSMiddleware to the app.""" + app = FastAPI() + setup_cors(app) + cls_names = [m.cls.__name__ for m in app.user_middleware] + assert "CORSMiddleware" in cls_names + + def test_wildcard_origins_without_credentials(self): + """allow_origins=['*'] must not be paired with allow_credentials=True. + + The combination is forbidden by the CORS spec and causes browsers to + silently drop credentials on every cross-origin request. + """ + app = FastAPI() + setup_cors(app) + opts = _get_cors_options(app) + allow_origins = opts.get("allow_origins", []) + allow_credentials = opts.get("allow_credentials", False) + + if "*" in allow_origins or allow_origins == ["*"]: + assert not allow_credentials, ( + "allow_origins=['*'] with allow_credentials=True is invalid per " + "the CORS spec — browsers reject it and credentials are silently dropped" + ) + + def test_cors_allows_cross_origin_requests(self): + """Cross-origin preflight requests return a 200 with CORS headers.""" + app = FastAPI() + + @app.get("/probe") + async def probe(): + return {"ok": True} + + setup_cors(app) + client = TestClient(app, raise_server_exceptions=True) + + response = client.options( + "/probe", + headers={ + "Origin": "http://localhost:3000", + "Access-Control-Request-Method": "GET", + }, + ) + assert response.status_code == 200 + assert "access-control-allow-origin" in response.headers + + def test_cors_no_credentials_header_with_wildcard(self): + """With wildcard origins, the response must not include + Access-Control-Allow-Credentials: true.""" + app = FastAPI() + + @app.get("/probe") + async def probe(): + return {"ok": True} + + setup_cors(app) + client = TestClient(app) + response = client.get("/probe", headers={"Origin": "http://evil.example.com"}) + + acao = response.headers.get("access-control-allow-origin", "") + acac = response.headers.get("access-control-allow-credentials", "false") + + if acao == "*": + assert acac.lower() != "true", ( + "Wildcard ACAO + ACAC:true is a spec violation (RFC 6454 §7.2, " + "Fetch §4.7) and silently breaks credentialed cross-origin requests" + )