From 5ba367af2ff23aacc0519d6d7cd6b45f91af9c88 Mon Sep 17 00:00:00 2001 From: Ken Date: Tue, 26 May 2026 18:52:12 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20code=20review=20fixes=20=E2=80=94=20auth?= =?UTF-8?q?=20gap,=20path=20traversal,=20dead=20code,=20health-check=20tim?= =?UTF-8?q?eout?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - backend/app.py: add Depends(_require_auth) to /api/copilotkit endpoint (critical security fix — endpoint was fully unprotected; now requires valid session cookie) - backend/app.py: move 'from ag_ui.core import RunStartedEvent' from inline function body to top-level imports (style consistency) - backend/auth.py: remove dead AUTH_PASS_HASH module-level constant (verify_password already reads from os.environ at call time; the cached binding was unused dead code) - backend/artifacts.py: add path traversal guards to list_artifacts, get_artifact, and save_artifact — resolve paths and verify they stay within ARTIFACTS_DIR - entrypoint.sh: health-check loop now sets READY flag and exits 1 if amplifierd fails to start within 30s (previously fell through silently, causing 502s) - tests/test_app.py: add TestCopilotKitAuth — verifies unauthenticated requests to /api/copilotkit return 401; includes note explaining why streaming is not tested here - tests/test_structure.sh: update to reflect that Dockerfile and entrypoint.sh now exist (created in Tasks 7+8); convert absent-checks to presence-checks Co-authored-by: Amplifier --- backend/app.py | 10 ++++++---- backend/artifacts.py | 25 +++++++++++++++++++------ backend/auth.py | 5 +++-- entrypoint.sh | 6 ++++++ tests/test_app.py | 19 +++++++++++++++++++ tests/test_structure.sh | 19 +++++++++++++++++-- 6 files changed, 70 insertions(+), 14 deletions(-) diff --git a/backend/app.py b/backend/app.py index e363b56..344dad9 100644 --- a/backend/app.py +++ b/backend/app.py @@ -12,6 +12,7 @@ from pathlib import Path from typing import Any, AsyncGenerator import httpx +from ag_ui.core import RunStartedEvent from fastapi import Cookie, Depends, FastAPI, HTTPException, Request, Response from fastapi.middleware.cors import CORSMiddleware from fastapi.responses import HTMLResponse, JSONResponse, StreamingResponse @@ -234,14 +235,15 @@ async def get_transcript( @app.post("/api/copilotkit") -async def copilotkit(request: Request) -> StreamingResponse: +async def copilotkit( + request: Request, + email: str = Depends(_require_auth), +) -> StreamingResponse: """AG-UI streaming endpoint. Accepts CopilotKit protocol payload, executes via amplifierd, and streams - AG-UI SSE events back to the client. + AG-UI SSE events back to the client. Requires a valid session cookie. """ - from ag_ui.core import RunStartedEvent - body = await request.json() thread_id: str = body.get("threadId", str(uuid.uuid4())) run_id: str = body.get("runId", str(uuid.uuid4())) diff --git a/backend/artifacts.py b/backend/artifacts.py index 0bb8e96..846b402 100644 --- a/backend/artifacts.py +++ b/backend/artifacts.py @@ -9,9 +9,12 @@ def list_artifacts(session_id: str) -> list[dict]: """Return sorted list of artifact dicts for a session. Each dict contains: name, session_id, size. - Returns [] if the session directory does not exist. + Returns [] if the session directory does not exist or if session_id + would resolve outside ARTIFACTS_DIR (path traversal guard). """ - session_dir = ARTIFACTS_DIR / session_id + session_dir = (ARTIFACTS_DIR / session_id).resolve() + if not session_dir.is_relative_to(ARTIFACTS_DIR.resolve()): + return [] if not session_dir.exists(): return [] return sorted( @@ -29,9 +32,15 @@ def list_artifacts(session_id: str) -> list[dict]: def get_artifact(session_id: str, name: str) -> str | None: - """Return file content (utf-8) or None if the artifact does not exist.""" - artifact_path = ARTIFACTS_DIR / session_id / name - if not artifact_path.exists(): + """Return file content (utf-8) or None if artifact does not exist. + + Returns None (silently) if the resolved path would escape ARTIFACTS_DIR + (path traversal guard). + """ + artifact_path = (ARTIFACTS_DIR / session_id / name).resolve() + if not artifact_path.is_relative_to(ARTIFACTS_DIR.resolve()): + return None + if not artifact_path.is_file(): return None return artifact_path.read_text(encoding="utf-8") @@ -40,9 +49,13 @@ def save_artifact(session_id: str, name: str, content: str) -> Path: """Write content to the artifact file, creating parent dirs as needed. Returns the Path of the written file. + Raises ValueError if the resolved path would escape ARTIFACTS_DIR + (path traversal guard). """ session_dir = ARTIFACTS_DIR / session_id + artifact_path = (session_dir / name).resolve() + if not artifact_path.is_relative_to(ARTIFACTS_DIR.resolve()): + raise ValueError(f"Path traversal detected in artifact name: {name!r}") session_dir.mkdir(parents=True, exist_ok=True) - artifact_path = session_dir / name artifact_path.write_text(content, encoding="utf-8") return artifact_path diff --git a/backend/auth.py b/backend/auth.py index 38b6291..9727397 100644 --- a/backend/auth.py +++ b/backend/auth.py @@ -5,9 +5,10 @@ import secrets import bcrypt -# Module-level defaults (read from env at import time for AUTH_USER/AUTH_PASS_HASH) +# AUTH_USER is read at import time (used by app.py to compare against the submitted email). +# AUTH_PASS_HASH is intentionally NOT cached here — verify_password() reads it fresh from +# os.environ at call time so runtime env changes (e.g. secret rotation) are respected. AUTH_USER: str = os.environ.get("AUTH_USER", "admin@localhost") -AUTH_PASS_HASH: str = os.environ.get("AUTH_PASS_HASH", "") # In-memory session store: token -> email _sessions: dict[str, str] = {} diff --git a/entrypoint.sh b/entrypoint.sh index fe56a1f..2baba65 100755 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -26,13 +26,19 @@ amplifierd --port 8410 --bundle /app/bundle/bundle.md & AMPLIFIERD_PID=$! echo "Waiting for amplifierd..." +READY=0 for i in $(seq 1 30); do if curl -s http://localhost:8410/health > /dev/null 2>&1; then echo "[OK] amplifierd on :8410" + READY=1 break fi sleep 1 done +if [ "$READY" -eq 0 ]; then + echo "[ERROR] amplifierd failed to start within 30s — aborting" >&2 + exit 1 +fi # (5) Start FastAPI backend cd /app/backend diff --git a/tests/test_app.py b/tests/test_app.py index 1eb5d28..30625aa 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -161,3 +161,22 @@ class TestMCPAppsEndpoint: """GET /api/apps/{name} for missing app file → 404.""" response = auth_client.get("/api/apps/nonexistent-app-xyz") assert response.status_code == 404 + + +# --------------------------------------------------------------------------- +# TestCopilotKitAuth +# --------------------------------------------------------------------------- + +# NOTE: /api/copilotkit is not fully tested here because it requires a live +# amplifierd stream. See integration tests (requires container runtime). +# Auth enforcement is tested below. + + +class TestCopilotKitAuth: + def test_copilotkit_unauthenticated_returns_401(self, client): + """POST /api/copilotkit without session cookie → 401.""" + response = client.post( + "/api/copilotkit", + json={"threadId": "test-thread", "messages": []}, + ) + assert response.status_code == 401 diff --git a/tests/test_structure.sh b/tests/test_structure.sh index 87b65a7..95a463c 100755 --- a/tests/test_structure.sh +++ b/tests/test_structure.sh @@ -44,11 +44,26 @@ check_absent "sites.yaml" check_absent "app.py" check_absent "static" check_absent "results" -check_absent "Dockerfile" -check_absent "entrypoint.sh" check_absent "pyproject.toml" check_absent "uv.lock" +echo "" +echo "=== Checking new root-level files exist ===" +if [ -f "$REPO/Dockerfile" ]; then + echo "PASS: 'Dockerfile' exists (Task 7)" + ((PASS++)) +else + echo "FAIL: 'Dockerfile' should exist (Task 7) but is missing" + ((FAIL++)) +fi +if [ -f "$REPO/entrypoint.sh" ]; then + echo "PASS: 'entrypoint.sh' exists (Task 8)" + ((PASS++)) +else + echo "FAIL: 'entrypoint.sh' should exist (Task 8) but is missing" + ((FAIL++)) +fi + echo "" echo "=== Checking new directories exist ===" check_dir "backend"