Fix Round 2 code review findings: type safety, security, and correctness

Backend:
- Add Literal types for status/priority fields (project_task, todo, project schemas)
- Add AccentColor Literal validation to prevent CSS injection (settings schema)
- Add PIN max-length (72 char bcrypt limit) validation
- Fix event date filtering to use correct range overlap logic
- Add revocation check to auth_status endpoint for consistency
- Config: env-aware SECRET_KEY fail-fast, configurable COOKIE_SECURE

Frontend:
- Add withCredentials to axios for cross-origin cookie support
- Replace .toISOString() with local date formatter in DashboardPage
- Replace `as any` casts with proper indexed type access in forms
- Nginx: add CSP, Referrer-Policy headers; remove deprecated X-XSS-Protection
- Nginx: duplicate security headers in static asset location block

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

backend/app/config.py

@@ -5,6 +5,8 @@ from pydantic_settings import BaseSettings, SettingsConfigDict
 class Settings(BaseSettings):
     DATABASE_URL: str = "postgresql+asyncpg://postgres:postgres@localhost:5432/umbra"
     SECRET_KEY: str = "your-secret-key-change-in-production"
+    ENVIRONMENT: str = "development"
+    COOKIE_SECURE: bool = False
 
     model_config = SettingsConfigDict(
         env_file=".env",
@@ -16,7 +18,15 @@ class Settings(BaseSettings):
 settings = Settings()
 
 if settings.SECRET_KEY == "your-secret-key-change-in-production":
-    print(
+    if settings.ENVIRONMENT != "development":
-        "WARNING: Using default SECRET_KEY. Set SECRET_KEY in .env for production.",
+        print(
-        file=sys.stderr,
+            "FATAL: Default SECRET_KEY detected in non-development environment. "
-    )
+            "Set a unique SECRET_KEY in .env immediately.",
+            file=sys.stderr,
+        )
+        sys.exit(1)
+    else:
+        print(
+            "WARNING: Using default SECRET_KEY. Set SECRET_KEY in .env for production.",
+            file=sys.stderr,
+        )

backend/app/routers/auth.py

@@ -22,6 +22,9 @@ _failed_attempts: dict[str, list[float]] = defaultdict(list)
 _MAX_ATTEMPTS = 5
 _WINDOW_SECONDS = 300  # 5-minute lockout window
 
+# Server-side session revocation (in-memory, sufficient for single-user app)
+_revoked_sessions: set[str] = set()
+
 
 def _check_rate_limit(ip: str) -> None:
     """Raise 429 if IP has exceeded failed login attempts."""
@@ -29,7 +32,10 @@ def _check_rate_limit(ip: str) -> None:
     attempts = _failed_attempts[ip]
     # Prune old entries outside the window
     _failed_attempts[ip] = [t for t in attempts if now - t < _WINDOW_SECONDS]
-    if len(_failed_attempts[ip]) >= _MAX_ATTEMPTS:
+    # Remove the key entirely if no recent attempts remain
+    if not _failed_attempts[ip]:
+        del _failed_attempts[ip]
+    elif len(_failed_attempts[ip]) >= _MAX_ATTEMPTS:
         raise HTTPException(
             status_code=429,
             detail="Too many failed login attempts. Try again in a few minutes.",
@@ -71,7 +77,7 @@ def _set_session_cookie(response: Response, token: str) -> None:
         key="session",
         value=token,
         httponly=True,
-        secure=True,
+        secure=app_settings.COOKIE_SECURE,
         max_age=86400 * 30,  # 30 days
         samesite="lax",
     )
@@ -85,6 +91,10 @@ async def get_current_session(
     if not session_cookie:
         raise HTTPException(status_code=401, detail="Not authenticated")
 
+    # Check if session has been revoked
+    if session_cookie in _revoked_sessions:
+        raise HTTPException(status_code=401, detail="Session has been revoked")
+
     user_id = verify_session_token(session_cookie)
     if user_id is None:
         raise HTTPException(status_code=401, detail="Invalid or expired session")
@@ -105,7 +115,7 @@ async def setup_pin(
     db: AsyncSession = Depends(get_db)
 ):
     """Create initial PIN. Only works if no settings exist."""
-    result = await db.execute(select(Settings))
+    result = await db.execute(select(Settings).with_for_update())
     existing = result.scalar_one_or_none()
 
     if existing:
@@ -156,9 +166,19 @@ async def login(
 
 
 @router.post("/logout")
-async def logout(response: Response):
+async def logout(
-    """Clear session cookie."""
+    response: Response,
-    response.delete_cookie(key="session")
+    session_cookie: Optional[str] = Cookie(None, alias="session")
+):
+    """Clear session cookie and invalidate server-side session."""
+    if session_cookie:
+        _revoked_sessions.add(session_cookie)
+    response.delete_cookie(
+        key="session",
+        httponly=True,
+        secure=app_settings.COOKIE_SECURE,
+        samesite="lax"
+    )
     return {"message": "Logout successful"}
 
 
@@ -175,8 +195,11 @@ async def auth_status(
     authenticated = False
 
     if not setup_required and session_cookie:
-        user_id = verify_session_token(session_cookie)
+        if session_cookie in _revoked_sessions:
-        authenticated = user_id is not None
+            authenticated = False
+        else:
+            user_id = verify_session_token(session_cookie)
+            authenticated = user_id is not None
 
     return {
         "authenticated": authenticated,

backend/app/routers/events.py

@@ -24,10 +24,10 @@ async def get_events(
     query = select(CalendarEvent)
 
     if start:
-        query = query.where(CalendarEvent.start_datetime >= start)
+        query = query.where(CalendarEvent.end_datetime >= start)
 
     if end:
-        query = query.where(CalendarEvent.end_datetime <= end)
+        query = query.where(CalendarEvent.start_datetime <= end)
 
     query = query.order_by(CalendarEvent.start_datetime.asc())
 

backend/app/schemas/project.py

@@ -1,13 +1,15 @@
 from pydantic import BaseModel, ConfigDict
 from datetime import datetime, date
-from typing import Optional, List
+from typing import Optional, List, Literal
 from app.schemas.project_task import ProjectTaskResponse
 
+ProjectStatus = Literal["not_started", "in_progress", "completed"]
+
 
 class ProjectCreate(BaseModel):
     name: str
     description: Optional[str] = None
-    status: str = "not_started"
+    status: ProjectStatus = "not_started"
     color: Optional[str] = None
     due_date: Optional[date] = None
 
@@ -15,7 +17,7 @@ class ProjectCreate(BaseModel):
 class ProjectUpdate(BaseModel):
     name: Optional[str] = None
     description: Optional[str] = None
-    status: Optional[str] = None
+    status: Optional[ProjectStatus] = None
     color: Optional[str] = None
     due_date: Optional[date] = None
 

backend/app/schemas/project_task.py

@@ -1,13 +1,16 @@
 from pydantic import BaseModel, ConfigDict
 from datetime import datetime, date
-from typing import Optional, List
+from typing import Optional, List, Literal
+
+TaskStatus = Literal["pending", "in_progress", "completed"]
+TaskPriority = Literal["low", "medium", "high"]
 
 
 class ProjectTaskCreate(BaseModel):
     title: str
     description: Optional[str] = None
-    status: str = "pending"
+    status: TaskStatus = "pending"
-    priority: str = "medium"
+    priority: TaskPriority = "medium"
     due_date: Optional[date] = None
     person_id: Optional[int] = None
     sort_order: int = 0
@@ -17,8 +20,8 @@ class ProjectTaskCreate(BaseModel):
 class ProjectTaskUpdate(BaseModel):
     title: Optional[str] = None
     description: Optional[str] = None
-    status: Optional[str] = None
+    status: Optional[TaskStatus] = None
-    priority: Optional[str] = None
+    priority: Optional[TaskPriority] = None
     due_date: Optional[date] = None
     person_id: Optional[int] = None
     sort_order: Optional[int] = None

backend/app/schemas/settings.py

@@ -1,5 +1,16 @@
 from pydantic import BaseModel, ConfigDict, field_validator
 from datetime import datetime
+from typing import Literal, Optional
+
+AccentColor = Literal["cyan", "blue", "green", "purple", "red", "orange", "pink", "yellow"]
+
+
+def _validate_pin_length(v: str, label: str = "PIN") -> str:
+    if len(v) < 4:
+        raise ValueError(f'{label} must be at least 4 characters')
+    if len(v) > 72:
+        raise ValueError(f'{label} must be at most 72 characters')
+    return v
 
 
 class SettingsCreate(BaseModel):
@@ -7,14 +18,12 @@ class SettingsCreate(BaseModel):
 
     @field_validator('pin')
     @classmethod
-    def pin_min_length(cls, v: str) -> str:
+    def pin_length(cls, v: str) -> str:
-        if len(v) < 4:
+        return _validate_pin_length(v)
-            raise ValueError('PIN must be at least 4 characters')
-        return v
 
 
 class SettingsUpdate(BaseModel):
-    accent_color: str | None = None
+    accent_color: Optional[AccentColor] = None
     upcoming_days: int | None = None
 
 
@@ -34,7 +43,5 @@ class ChangePinRequest(BaseModel):
 
     @field_validator('new_pin')
     @classmethod
-    def new_pin_min_length(cls, v: str) -> str:
+    def new_pin_length(cls, v: str) -> str:
-        if len(v) < 4:
+        return _validate_pin_length(v, "New PIN")
-            raise ValueError('New PIN must be at least 4 characters')
-        return v

backend/app/schemas/todo.py

@@ -1,12 +1,14 @@
 from pydantic import BaseModel, ConfigDict
 from datetime import datetime, date
-from typing import Optional
+from typing import Optional, Literal
+
+TodoPriority = Literal["low", "medium", "high"]
 
 
 class TodoCreate(BaseModel):
     title: str
     description: Optional[str] = None
-    priority: str = "medium"
+    priority: TodoPriority = "medium"
     due_date: Optional[date] = None
     category: Optional[str] = None
     recurrence_rule: Optional[str] = None
@@ -16,7 +18,7 @@ class TodoCreate(BaseModel):
 class TodoUpdate(BaseModel):
     title: Optional[str] = None
     description: Optional[str] = None
-    priority: Optional[str] = None
+    priority: Optional[TodoPriority] = None
     due_date: Optional[date] = None
     completed: Optional[bool] = None
     category: Optional[str] = None

frontend/nginx.conf

@@ -32,10 +32,15 @@ server {
     location ~* \.(js|css|png|jpg|jpeg|gif|ico|svg|woff|woff2|ttf|eot)$ {
         expires 1y;
         add_header Cache-Control "public, immutable";
+        add_header X-Frame-Options "SAMEORIGIN" always;
+        add_header X-Content-Type-Options "nosniff" always;
+        add_header Referrer-Policy "strict-origin-when-cross-origin" always;
+        add_header Content-Security-Policy "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self';" always;
     }
 
     # Security headers
     add_header X-Frame-Options "SAMEORIGIN" always;
     add_header X-Content-Type-Options "nosniff" always;
-    add_header X-XSS-Protection "1; mode=block" always;
+    add_header Referrer-Policy "strict-origin-when-cross-origin" always;
+    add_header Content-Security-Policy "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self';" always;
 }

frontend/src/components/dashboard/DashboardPage.tsx

@@ -17,7 +17,8 @@ export default function DashboardPage() {
   const { data, isLoading } = useQuery({
     queryKey: ['dashboard'],
     queryFn: async () => {
-      const today = new Date().toISOString().split('T')[0];
+      const now = new Date();
+      const today = `${now.getFullYear()}-${String(now.getMonth() + 1).padStart(2, '0')}-${String(now.getDate()).padStart(2, '0')}`;
       const { data } = await api.get<DashboardData>(`/dashboard?client_date=${today}`);
       return data;
     },

frontend/src/components/projects/ProjectForm.tsx

@@ -93,7 +93,7 @@ export default function ProjectForm({ project, onClose }: ProjectFormProps) {
             <Select
               id="status"
               value={formData.status}
-              onChange={(e) => setFormData({ ...formData, status: e.target.value as any })}
+              onChange={(e) => setFormData({ ...formData, status: e.target.value as Project['status'] })}
             >
               <option value="not_started">Not Started</option>
               <option value="in_progress">In Progress</option>

frontend/src/components/projects/TaskForm.tsx

@@ -109,7 +109,7 @@ export default function TaskForm({ projectId, task, parentTaskId, onClose }: Tas
               <Select
                 id="status"
                 value={formData.status}
-                onChange={(e) => setFormData({ ...formData, status: e.target.value as any })}
+                onChange={(e) => setFormData({ ...formData, status: e.target.value as ProjectTask['status'] })}
               >
                 <option value="pending">Pending</option>
                 <option value="in_progress">In Progress</option>
@@ -122,7 +122,7 @@ export default function TaskForm({ projectId, task, parentTaskId, onClose }: Tas
               <Select
                 id="priority"
                 value={formData.priority}
-                onChange={(e) => setFormData({ ...formData, priority: e.target.value as any })}
+                onChange={(e) => setFormData({ ...formData, priority: e.target.value as ProjectTask['priority'] })}
               >
                 <option value="low">Low</option>
                 <option value="medium">Medium</option>

frontend/src/lib/api.ts

@@ -5,6 +5,7 @@ const api = axios.create({
   headers: {
     'Content-Type': 'application/json',
   },
+  withCredentials: true,
 });
 
 api.interceptors.response.use(

progress.md

@@ -128,7 +128,7 @@ These were found during first Docker build and integration testing:
 
 ---
 
-## Code Review Findings (Senior Review)
+## Code Review Findings — Round 1 (Senior Review)
 
 ### Critical:
 - [x] C1: CORS `allow_origins=["*"]` with `allow_credentials=True` — already restricted to `["http://localhost:5173"]` (`main.py`)
@@ -151,6 +151,39 @@ These were found during first Docker build and integration testing:
 - [x] M7: Authenticated users can still navigate to `/login` — added `Navigate` redirect in `LockScreen.tsx`
 - [x] L1: Error handling in LockScreen used `error: any` — replaced with `getErrorMessage` helper
 
+## Code Review Findings — Round 2 (Senior Review)
+
+### Critical:
+- [x] C1: Default SECRET_KEY only warns, doesn't block production — added env-aware fail-fast (`config.py`)
+- [x] C2: `secure=True` cookie breaks HTTP development — made configurable via `COOKIE_SECURE` setting (`auth.py`, `config.py`)
+- [x] C3: No enum validation on status/priority fields — added `Literal` types (`schemas/project_task.py`, `todo.py`, `project.py`)
+- [x] C4: Race condition in PIN setup (TOCTOU) — added `select().with_for_update()` (`auth.py`)
+
+### High:
+- [x] H1: Rate limiter memory leak — added stale key cleanup, `del` empty entries (`auth.py`)
+- [ ] H2: Dashboard runs 7 sequential DB queries — deferred (asyncpg single-session limitation)
+- [ ] H3: Subtask eager loading fragile at 2 levels — accepted (business logic enforces single nesting)
+- [x] H4: No `withCredentials` on Axios for Vite dev — added to `api.ts`
+- [x] H5: Logout doesn't invalidate session server-side — added in-memory `_revoked_sessions` set (`auth.py`)
+
+### Medium:
+- [ ] M1: TodosPage fetches all then filters client-side — deferred (acceptable for personal app scale)
+- [x] M2: Dashboard uses `.toISOString()` violating CLAUDE.md rules — replaced with local date formatter (`DashboardPage.tsx`)
+- [x] M3: No CSP header in nginx — added CSP + Referrer-Policy, removed deprecated X-XSS-Protection (`nginx.conf`)
+- [x] M4: Event date filtering misses range-spanning events — fixed range overlap logic (`events.py`)
+- [x] M5: `accent_color` accepts arbitrary strings — added `Literal` validation for allowed colors (`schemas/settings.py`)
+- [x] M6: Logout `delete_cookie` doesn't match `set_cookie` attributes — matched all cookie params (`auth.py`)
+- [x] M7: bcrypt silently truncates PIN at 72 bytes — added max 72 char validation (`schemas/settings.py`)
+
+### Low:
+- [x] L1: `as any` type casts in frontend forms — replaced with proper `Type['field']` casts (`TaskForm.tsx`, `ProjectForm.tsx`)
+- [x] L2: Unused imports in `events.py` — false positive, all imports are used
+- [ ] L3: Upcoming endpoint mixes date/datetime string sorting — deferred (works correctly for ISO format)
+- [ ] L4: Backend port 8000 exposed directly, bypassing nginx — deferred (useful for dev)
+- [ ] L5: `parseInt(id!)` without NaN validation — deferred (low risk, route-level protection)
+- [x] L6: `X-XSS-Protection` header is deprecated — removed, replaced with CSP (`nginx.conf`)
+- [x] L7: Missing `Referrer-Policy` header — added `strict-origin-when-cross-origin` (`nginx.conf`)
+
 ---
 
 ## Outstanding Items (Resume Here If Halted)