fix: address PR #222 review comments (security, race condition, defensive coding)

- Block unauthorized vertex provider requests in default routing path (privilege escalation)
- Add IDOR protection to get_result endpoint with ownership check
- Remove os.environ mutation in vertex branch to prevent race conditions
- Pass resolved_key to decide_provider to fix whitespace BYOK mismatch
- Add defensive exception handling in _get_user_doc
- Extract _to_admin_response helper, add ObjectId validation and self-demotion guard
- Fix llm_policy docstring indentation
- Use pytest.raises for idiomatic exception tests
- Fix admin page: separate accessDenied state, cleanup setTimeout on unmount
- Document allowlist env vars in .env.example

Author: ComBba

backend/.env.example

@@ -23,6 +23,12 @@ VERTEX_API_KEY=your_vertex_express_api_key_here
 GOOGLE_CLOUD_PROJECT=your_gcp_project_id
 GOOGLE_CLOUD_LOCATION=asia-northeast3
 
+# Vertex AI role-based routing allowlists (comma-separated)
+# VERTEX_PREMIUM_USER_IDS=user_id_1,user_id_2
+# VERTEX_ADMIN_USER_IDS=admin_id_1,admin_id_2
+# VERTEX_PREMIUM_EMAILS=premium@example.com
+# VERTEX_ADMIN_EMAILS=admin@example.com
+
 # GitHub Personal Access Token
 # Required permissions: repo (read access)
 # Generate at: https://github.com/settings/tokens

backend/app/api/routes/admin.py

@@ -8,6 +8,8 @@
 import logging
 from typing import Any
 
+from bson import ObjectId
+from bson.errors import InvalidId
 from fastapi import APIRouter, Depends, HTTPException
 from pydantic import BaseModel, Field
 
@@ -50,27 +52,27 @@ class AdminUserResponse(BaseModel):
     created_at: str | None = None
 
 
+def _to_admin_response(u: dict) -> AdminUserResponse:
+    return AdminUserResponse(
+        id=str(u["_id"]),
+        username=u.get("username", ""),
+        email=u.get("email"),
+        github_id=str(u.get("github_id", "")),
+        avatar_url=u.get("avatar_url"),
+        role=u.get("role", "user"),
+        plan=u.get("plan", "free"),
+        created_at=str(u.get("created_at", "")),
+    )
+
+
 @router.get("/users", response_model=list[AdminUserResponse])
 async def list_users(
     _admin: User = Depends(require_admin),
 ) -> list[AdminUserResponse]:
     """List all users with their roles and plans."""
     user_repo = UserRepository()
     users = await user_repo.list(limit=500)
-
-    return [
-        AdminUserResponse(
-            id=str(u["_id"]),
-            username=u.get("username", ""),
-            email=u.get("email"),
-            github_id=str(u.get("github_id", "")),
-            avatar_url=u.get("avatar_url"),
-            role=u.get("role", "user"),
-            plan=u.get("plan", "free"),
-            created_at=str(u.get("created_at", "")),
-        )
-        for u in users
-    ]
+    return [_to_admin_response(u) for u in users]
 
 
 @router.patch("/users/{user_id}", response_model=AdminUserResponse)
@@ -80,6 +82,11 @@ async def update_user_role(
     _admin: User = Depends(require_admin),
 ) -> AdminUserResponse:
     """Update a user's role and/or plan."""
+    try:
+        ObjectId(user_id)
+    except (InvalidId, Exception):
+        raise HTTPException(status_code=400, detail="Invalid user ID format")
+
     update_data: dict[str, Any] = {}
 
     if update.role is not None:
@@ -88,6 +95,11 @@ async def update_user_role(
                 status_code=400,
                 detail=f"Invalid role: {update.role}. Must be one of {VALID_ROLES}",
             )
+        if update.role != "admin" and user_id == _admin.id:
+            raise HTTPException(
+                status_code=400,
+                detail="Cannot demote yourself. Ask another admin.",
+            )
         update_data["role"] = update.role
 
     if update.plan is not None:
@@ -107,15 +119,8 @@ async def update_user_role(
     if not updated:
         raise HTTPException(status_code=404, detail="User not found")
 
-    logger.info(f"[Admin] User {user_id} updated: {update_data} by admin {_admin.id}")
-
-    return AdminUserResponse(
-        id=str(updated["_id"]),
-        username=updated.get("username", ""),
-        email=updated.get("email"),
-        github_id=str(updated.get("github_id", "")),
-        avatar_url=updated.get("avatar_url"),
-        role=updated.get("role", "user"),
-        plan=updated.get("plan", "free"),
-        created_at=str(updated.get("created_at", "")),
+    logger.info(
+        "[Admin] User %s updated: %s by admin %s", user_id, update_data, _admin.id
     )
+
+    return _to_admin_response(updated)

backend/app/api/routes/evaluate.py

@@ -302,6 +302,11 @@ async def get_result(
         raise CorkedError("Authentication required to view this evaluation")
 
     try:
+        if not is_public_demo:
+            progress = await get_evaluation_progress(evaluation_id)
+            if progress.get("user_id") != user.id:
+                raise CorkedError("Access denied: evaluation belongs to another user")
+
         result = await get_evaluation_result(evaluation_id)
     except EmptyCellarError:
         raise EmptyCellarError(f"Evaluation not found: {evaluation_id}") from None

backend/app/providers/llm.py

@@ -1,4 +1,3 @@
-import os
 from dataclasses import dataclass
 from typing import Optional
 
@@ -124,7 +123,6 @@ def build_llm(
         resolved_model = model or PROVIDER_DEFAULTS["vertex"]
         if not settings.VERTEX_API_KEY:
             raise ValueError("VERTEX_API_KEY is required for Vertex AI Express")
-        os.environ["GOOGLE_API_KEY"] = settings.VERTEX_API_KEY
         vertex_kwargs: dict = {
             "model": resolved_model,
             "temperature": resolved_temperature,

backend/app/providers/llm_policy.py

@@ -125,7 +125,7 @@ async def invoke_with_policy(
     Args:
         llm: The LangChain LLM instance
         messages: Messages to send to the LLM
-    provider: Provider name (gemini, vertex)
+        provider: Provider name (gemini, vertex)
         config: Retry configuration (defaults to RetryConfig())
         langchain_config: Optional config to pass to llm.ainvoke()
         on_retry: Optional callback(attempt, delay, message) for logging

backend/app/services/evaluation_service.py

@@ -66,8 +66,14 @@ async def _prepare_repo_context(
 async def _get_user_doc(user_id: str | None) -> dict | None:
     if not user_id:
         return None
-    user_repo = UserRepository()
-    return await user_repo.get_by_id(user_id)
+    try:
+        user_repo = UserRepository()
+        return await user_repo.get_by_id(user_id)
+    except Exception:
+        logger.warning(
+            "Failed to fetch user doc for provider routing", extra={"user_id": user_id}
+        )
+        return None
 
 
 def _create_initial_state(
@@ -227,7 +233,9 @@ async def run_evaluation_pipeline(
         repo_url, api_key, github_token
     )
     user_doc = await _get_user_doc(user_id)
-    provider_decision = decide_provider(user_doc, provider, api_key)
+    provider_decision = decide_provider(
+        user_doc, provider, resolved_key if api_key else None
+    )
     state = _create_initial_state(
         repo_url, repo_context, criteria, user_id=user_id or ""
     )
@@ -493,7 +501,9 @@ async def run_evaluation_pipeline_with_events(
             repo_url, api_key, github_token
         )
         user_doc = await _get_user_doc(user_id)
-        provider_decision = decide_provider(user_doc, provider, api_key)
+        provider_decision = decide_provider(
+            user_doc, provider, resolved_key if api_key else None
+        )
         state = _create_initial_state(
             repo_url,
             repo_context,

backend/app/services/provider_routing.py

@@ -59,4 +59,10 @@ def decide_provider(
     if _is_premium(user_doc):
         return ProviderDecision(provider="vertex", reason="premium")
 
+    # Block unauthorized vertex requests — only admin/premium may use vertex
+    if requested_provider and requested_provider.lower() == "vertex":
+        return ProviderDecision(
+            provider="gemini", reason="unauthorized_vertex_fallback"
+        )
+
     return ProviderDecision(provider=requested_provider or "gemini", reason="default")

backend/tests/test_llm_provider.py

@@ -1,5 +1,6 @@
 from unittest.mock import patch
 
+import pytest
 from langchain_google_genai import ChatGoogleGenerativeAI
 
 from app.providers.llm import (
@@ -57,18 +58,12 @@ def test_build_llm_vertex_routing(self, mock_settings, mock_vertex):
     def test_build_llm_vertex_missing_key_raises(self):
         with patch("app.providers.llm.settings") as mock_settings:
             mock_settings.VERTEX_API_KEY = ""
-            try:
+            with pytest.raises(ValueError, match="VERTEX_API_KEY"):
                 build_llm("vertex", None, None, 0.1, 128)
-                assert False, "Should have raised ValueError"
-            except ValueError as e:
-                assert "VERTEX_API_KEY" in str(e)
 
     def test_build_llm_unsupported_provider_raises(self):
-        try:
+        with pytest.raises(ValueError, match="Unsupported provider"):
             build_llm("openai", "test-key", None, 0.1, 128)
-            assert False, "Should have raised ValueError"
-        except ValueError as e:
-            assert "Unsupported provider" in str(e)
 
     def test_build_llm_default_is_gemini(self):
         llm = build_llm(None, "test-key", None, 0.1, 128)

frontend/src/app/admin/page.tsx

@@ -1,6 +1,6 @@
 'use client';
 
-import { useEffect, useState, useCallback } from 'react';
+import { useEffect, useState, useCallback, useRef } from 'react';
 import { useAuth } from '@/contexts/AuthContext';
 import { useRouter } from 'next/navigation';
 import { Shield, Users, Crown, Loader2, AlertCircle, Check } from 'lucide-react';
@@ -42,6 +42,8 @@ export default function AdminPage() {
   const [error, setError] = useState<string | null>(null);
   const [saving, setSaving] = useState<string | null>(null);
   const [saved, setSaved] = useState<string | null>(null);
+  const [accessDenied, setAccessDenied] = useState(false);
+  const savedTimerRef = useRef<ReturnType<typeof setTimeout>>(null);
 
   const fetchUsers = useCallback(async () => {
     if (!token) return;
@@ -52,7 +54,7 @@ export default function AdminPage() {
         headers: { Authorization: `Bearer ${token}` },
       });
       if (res.status === 403) {
-        setError('관리자 권한이 없습니다.');
+        setAccessDenied(true);
         return;
       }
       if (!res.ok) throw new Error(`HTTP ${res.status}`);
@@ -72,6 +74,12 @@ export default function AdminPage() {
     if (isAuthenticated) fetchUsers();
   }, [isAuthenticated, authLoading, fetchUsers, router]);
 
+  useEffect(() => {
+    return () => {
+      if (savedTimerRef.current) clearTimeout(savedTimerRef.current);
+    };
+  }, []);
+
   const updateUser = async (userId: string, field: 'role' | 'plan', value: string) => {
     if (!token) return;
     setSaving(userId);
@@ -91,7 +99,8 @@ export default function AdminPage() {
       const updated: AdminUser = await res.json();
       setUsers((prev) => prev.map((u) => (u.id === userId ? updated : u)));
       setSaved(userId);
-      setTimeout(() => setSaved(null), 1500);
+      if (savedTimerRef.current) clearTimeout(savedTimerRef.current);
+      savedTimerRef.current = setTimeout(() => setSaved(null), 1500);
     } catch (err) {
       setError(err instanceof Error ? err.message : 'Update failed');
     } finally {
@@ -107,7 +116,7 @@ export default function AdminPage() {
     );
   }
 
-  if (error === '관리자 권한이 없습니다.') {
+  if (accessDenied) {
     return (
       <div className="flex flex-col items-center justify-center min-h-[60vh] gap-4">
         <Shield className="w-16 h-16 text-red-400" />
@@ -129,7 +138,7 @@ export default function AdminPage() {
         </div>
       </div>
 
-      {error && error !== '관리자 권한이 없습니다.' && (
+      {error && (
         <div className="mb-6 flex items-center gap-2 rounded-lg border border-red-200 bg-red-50 p-4 text-sm text-red-800">
           <AlertCircle size={16} />
           {error}