From 1aaa2b3a74892291bf613fd9a17cfd706f3bc170 Mon Sep 17 00:00:00 2001 From: Kyle Pope Date: Mon, 16 Feb 2026 07:49:21 +0800 Subject: [PATCH] Fix code review findings: security hardening and frontend fixes Backend: - Add rate limiting to login (5 attempts / 5 min window) - Add secure flag to session cookies with helper function - Add PIN min-length validation via Pydantic field_validator - Fix naive datetime usage in todos.py (datetime.now() not UTC) - Disable SQLAlchemy echo in production - Remove auto-commit from get_db to prevent double commits - Add lower bound filter to upcoming events query - Add SECRET_KEY default warning on startup - Remove create_all from lifespan (Alembic handles migrations) Frontend: - Fix ReminderForm remind_at slice for datetime-local input - Add window.confirm() dialogs on all destructive actions - Redirect authenticated users away from login screen - Replace error: any with getErrorMessage helper in LockScreen Co-Authored-By: Claude Opus 4.6 --- backend/app/config.py | 7 +++ backend/app/database.py | 3 +- backend/app/main.py | 8 +-- backend/app/routers/auth.py | 63 ++++++++++++++----- backend/app/routers/dashboard.py | 6 +- backend/app/routers/todos.py | 6 +- backend/app/schemas/settings.py | 16 ++++- frontend/src/components/auth/LockScreen.tsx | 16 +++-- .../src/components/projects/ProjectDetail.tsx | 15 ++++- .../src/components/reminders/ReminderForm.tsx | 2 +- progress.md | 37 ++++++++++- 11 files changed, 140 insertions(+), 39 deletions(-) diff --git a/backend/app/config.py b/backend/app/config.py index d23307f..6412bf8 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -1,3 +1,4 @@ +import sys from pydantic_settings import BaseSettings, SettingsConfigDict @@ -13,3 +14,9 @@ class Settings(BaseSettings): settings = Settings() + +if settings.SECRET_KEY == "your-secret-key-change-in-production": + print( + "WARNING: Using default SECRET_KEY. Set SECRET_KEY in .env for production.", + file=sys.stderr, + ) diff --git a/backend/app/database.py b/backend/app/database.py index 0df7f66..1743bc1 100644 --- a/backend/app/database.py +++ b/backend/app/database.py @@ -5,7 +5,7 @@ from app.config import settings # Create async engine engine = create_async_engine( settings.DATABASE_URL, - echo=True, + echo=False, future=True ) @@ -27,7 +27,6 @@ async def get_db() -> AsyncSession: async with AsyncSessionLocal() as session: try: yield session - await session.commit() except Exception: await session.rollback() raise diff --git a/backend/app/main.py b/backend/app/main.py index 3117867..d731b2b 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -2,17 +2,13 @@ from fastapi import FastAPI from fastapi.middleware.cors import CORSMiddleware from contextlib import asynccontextmanager -from app.database import engine, Base +from app.database import engine from app.routers import auth, todos, events, reminders, projects, people, locations, settings as settings_router, dashboard @asynccontextmanager async def lifespan(app: FastAPI): - # Startup: Create tables - async with engine.begin() as conn: - await conn.run_sync(Base.metadata.create_all) yield - # Shutdown: Clean up resources await engine.dispose() @@ -26,7 +22,7 @@ app = FastAPI( # CORS configuration for development app.add_middleware( CORSMiddleware, - allow_origins=["*"], + allow_origins=["http://localhost:5173"], allow_credentials=True, allow_methods=["*"], allow_headers=["*"], diff --git a/backend/app/routers/auth.py b/backend/app/routers/auth.py index 12afe1d..38871eb 100644 --- a/backend/app/routers/auth.py +++ b/backend/app/routers/auth.py @@ -1,7 +1,9 @@ -from fastapi import APIRouter, Depends, HTTPException, Response, Cookie +from fastapi import APIRouter, Depends, HTTPException, Response, Cookie, Request from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy import select from typing import Optional +from collections import defaultdict +import time import bcrypt from itsdangerous import TimestampSigner, BadSignature @@ -15,6 +17,29 @@ router = APIRouter() # Initialize signer for session management signer = TimestampSigner(app_settings.SECRET_KEY) +# Brute-force protection: track failed login attempts per IP +_failed_attempts: dict[str, list[float]] = defaultdict(list) +_MAX_ATTEMPTS = 5 +_WINDOW_SECONDS = 300 # 5-minute lockout window + + +def _check_rate_limit(ip: str) -> None: + """Raise 429 if IP has exceeded failed login attempts.""" + now = time.time() + 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: + raise HTTPException( + status_code=429, + detail="Too many failed login attempts. Try again in a few minutes.", + ) + + +def _record_failed_attempt(ip: str) -> None: + """Record a failed login attempt for the given IP.""" + _failed_attempts[ip].append(time.time()) + def hash_pin(pin: str) -> str: """Hash a PIN using bcrypt.""" @@ -40,6 +65,18 @@ def verify_session_token(token: str) -> Optional[int]: return None +def _set_session_cookie(response: Response, token: str) -> None: + """Set the session cookie with secure defaults.""" + response.set_cookie( + key="session", + value=token, + httponly=True, + secure=True, + max_age=86400 * 30, # 30 days + samesite="lax", + ) + + async def get_current_session( session_cookie: Optional[str] = Cookie(None, alias="session"), db: AsyncSession = Depends(get_db) @@ -82,13 +119,7 @@ async def setup_pin( # Create session token = create_session_token(new_settings.id) - response.set_cookie( - key="session", - value=token, - httponly=True, - max_age=86400 * 30, # 30 days - samesite="lax" - ) + _set_session_cookie(response, token) return {"message": "Setup completed successfully", "authenticated": True} @@ -96,10 +127,14 @@ async def setup_pin( @router.post("/login") async def login( data: SettingsCreate, + request: Request, response: Response, db: AsyncSession = Depends(get_db) ): """Verify PIN and create session.""" + client_ip = request.client.host if request.client else "unknown" + _check_rate_limit(client_ip) + result = await db.execute(select(Settings)) settings_obj = result.scalar_one_or_none() @@ -107,17 +142,15 @@ async def login( raise HTTPException(status_code=400, detail="Setup required") if not verify_pin(data.pin, settings_obj.pin_hash): + _record_failed_attempt(client_ip) raise HTTPException(status_code=401, detail="Invalid PIN") + # Clear failed attempts on successful login + _failed_attempts.pop(client_ip, None) + # Create session token = create_session_token(settings_obj.id) - response.set_cookie( - key="session", - value=token, - httponly=True, - max_age=86400 * 30, # 30 days - samesite="lax" - ) + _set_session_cookie(response, token) return {"message": "Login successful", "authenticated": True} diff --git a/backend/app/routers/dashboard.py b/backend/app/routers/dashboard.py index 7a8c7dd..21ad031 100644 --- a/backend/app/routers/dashboard.py +++ b/backend/app/routers/dashboard.py @@ -132,9 +132,11 @@ async def get_upcoming( todos_result = await db.execute(todos_query) todos = todos_result.scalars().all() - # Get upcoming events + # Get upcoming events (from today onward) + today_start = datetime.combine(today, datetime.min.time()) events_query = select(CalendarEvent).where( - CalendarEvent.start_datetime <= cutoff_datetime + CalendarEvent.start_datetime >= today_start, + CalendarEvent.start_datetime <= cutoff_datetime, ) events_result = await db.execute(events_query) events = events_result.scalars().all() diff --git a/backend/app/routers/todos.py b/backend/app/routers/todos.py index 0d6d66c..fc52fc4 100644 --- a/backend/app/routers/todos.py +++ b/backend/app/routers/todos.py @@ -2,7 +2,7 @@ from fastapi import APIRouter, Depends, HTTPException, Query from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy import select from typing import Optional, List -from datetime import datetime, timezone +from datetime import datetime from app.database import get_db from app.models.todo import Todo @@ -95,7 +95,7 @@ async def update_todo( # Handle completion timestamp if "completed" in update_data: if update_data["completed"] and not todo.completed: - update_data["completed_at"] = datetime.now(timezone.utc) + update_data["completed_at"] = datetime.now() elif not update_data["completed"]: update_data["completed_at"] = None @@ -141,7 +141,7 @@ async def toggle_todo( raise HTTPException(status_code=404, detail="Todo not found") todo.completed = not todo.completed - todo.completed_at = datetime.now(timezone.utc) if todo.completed else None + todo.completed_at = datetime.now() if todo.completed else None await db.commit() await db.refresh(todo) diff --git a/backend/app/schemas/settings.py b/backend/app/schemas/settings.py index 5aed355..b5cd284 100644 --- a/backend/app/schemas/settings.py +++ b/backend/app/schemas/settings.py @@ -1,10 +1,17 @@ -from pydantic import BaseModel, ConfigDict +from pydantic import BaseModel, ConfigDict, field_validator from datetime import datetime class SettingsCreate(BaseModel): pin: str + @field_validator('pin') + @classmethod + def pin_min_length(cls, v: str) -> str: + if len(v) < 4: + raise ValueError('PIN must be at least 4 characters') + return v + class SettingsUpdate(BaseModel): accent_color: str | None = None @@ -24,3 +31,10 @@ class SettingsResponse(BaseModel): class ChangePinRequest(BaseModel): old_pin: str new_pin: str + + @field_validator('new_pin') + @classmethod + def new_pin_min_length(cls, v: str) -> str: + if len(v) < 4: + raise ValueError('New PIN must be at least 4 characters') + return v diff --git a/frontend/src/components/auth/LockScreen.tsx b/frontend/src/components/auth/LockScreen.tsx index c052942..2d0dd39 100644 --- a/frontend/src/components/auth/LockScreen.tsx +++ b/frontend/src/components/auth/LockScreen.tsx @@ -1,8 +1,9 @@ import { useState, FormEvent } from 'react'; -import { useNavigate } from 'react-router-dom'; +import { useNavigate, Navigate } from 'react-router-dom'; import { toast } from 'sonner'; import { Lock } from 'lucide-react'; import { useAuth } from '@/hooks/useAuth'; +import { getErrorMessage } from '@/lib/api'; import { Card, CardContent, CardDescription, CardHeader, CardTitle } from '@/components/ui/card'; import { Input } from '@/components/ui/input'; import { Button } from '@/components/ui/button'; @@ -14,6 +15,11 @@ export default function LockScreen() { const [pin, setPin] = useState(''); const [confirmPin, setConfirmPin] = useState(''); + // Redirect authenticated users to dashboard + if (authStatus?.authenticated) { + return ; + } + const handleSubmit = async (e: FormEvent) => { e.preventDefault(); @@ -30,15 +36,15 @@ export default function LockScreen() { await setup(pin); toast.success('PIN created successfully'); navigate('/dashboard'); - } catch (error: any) { - toast.error(error.response?.data?.detail || 'Failed to create PIN'); + } catch (error) { + toast.error(getErrorMessage(error, 'Failed to create PIN')); } } else { try { await login(pin); navigate('/dashboard'); - } catch (error: any) { - toast.error(error.response?.data?.detail || 'Invalid PIN'); + } catch (error) { + toast.error(getErrorMessage(error, 'Invalid PIN')); setPin(''); } } diff --git a/frontend/src/components/projects/ProjectDetail.tsx b/frontend/src/components/projects/ProjectDetail.tsx index 9f0c45e..0ad60a8 100644 --- a/frontend/src/components/projects/ProjectDetail.tsx +++ b/frontend/src/components/projects/ProjectDetail.tsx @@ -155,7 +155,10 @@ export default function ProjectDetail() {