From a94485b13898d1b703648556f3f2c0bc63efb4ed Mon Sep 17 00:00:00 2001 From: Kyle Pope Date: Fri, 13 Mar 2026 00:19:33 +0800 Subject: [PATCH] Address code review findings across all phases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 fixes: - W-01: Add start_period: 30s to backend healthcheck for migration window - W-03: Narrow .dockerignore *.md to specific files (preserve alembic/README) Phase 2 fixes: - C-01: Wrap Argon2id calls in totp.py (disable, regenerate, backup verify, backup store) — missed in initial AC-2 pass - S-01: Extract async wrappers (ahash_password, averify_password, averify_password_with_upgrade) into services/auth.py, refactor all callers to use them instead of manual run_in_executor boilerplate - W-01: Fix ntfy dedup regression — commit per category instead of per-user to preserve dedup records if a later category fails Phase 4 fixes: - C-01: Fix optimistic drag-and-drop cache key to include date range - C-02: Replace toISOString() with format() to avoid UTC date shift in visible range calculation - W-02: Initialize visibleRange from current month to eliminate unscoped first fetch + immediate refetch Co-Authored-By: Claude Opus 4.6 --- backend/.dockerignore | 3 +- backend/app/jobs/notifications.py | 9 ++-- backend/app/routers/admin.py | 9 ++-- backend/app/routers/auth.py | 32 +++++-------- backend/app/routers/totp.py | 19 +++++--- backend/app/services/auth.py | 24 ++++++++++ docker-compose.yaml | 1 + .../src/components/calendar/CalendarPage.tsx | 45 ++++++++++++------- 8 files changed, 87 insertions(+), 55 deletions(-) diff --git a/backend/.dockerignore b/backend/.dockerignore index 0e880dc..6662571 100644 --- a/backend/.dockerignore +++ b/backend/.dockerignore @@ -32,7 +32,8 @@ pytest.ini htmlcov # Documentation -*.md +README.md +CHANGELOG.md LICENSE # Dev scripts diff --git a/backend/app/jobs/notifications.py b/backend/app/jobs/notifications.py index 62dd369..a09e4d5 100644 --- a/backend/app/jobs/notifications.py +++ b/backend/app/jobs/notifications.py @@ -239,17 +239,20 @@ async def _dispatch_for_user(db: AsyncSession, settings: Settings, now: datetime # Batch-fetch all sent keys once per user instead of one query per entity sent_keys = await _get_sent_keys(db, settings.user_id) + # AW-4: Commit after each category to preserve dedup records if a later + # category fails (prevents re-sending already-sent notifications) if settings.ntfy_reminders_enabled: await _dispatch_reminders(db, settings, now, sent_keys) + await db.commit() if settings.ntfy_events_enabled: await _dispatch_events(db, settings, now, sent_keys) + await db.commit() if settings.ntfy_todos_enabled: await _dispatch_todos(db, settings, now.date(), sent_keys) + await db.commit() if settings.ntfy_projects_enabled: await _dispatch_projects(db, settings, now.date(), sent_keys) - - # AW-4: Single commit per user instead of per-notification - await db.commit() + await db.commit() async def _purge_old_sent_records(db: AsyncSession) -> None: diff --git a/backend/app/routers/admin.py b/backend/app/routers/admin.py index 5026fc0..8e9feef 100644 --- a/backend/app/routers/admin.py +++ b/backend/app/routers/admin.py @@ -10,7 +10,6 @@ Security measures implemented: All routes require the `require_admin` dependency (which chains through get_current_user, so the session cookie is always validated). """ -import asyncio import secrets from datetime import datetime from typing import Optional @@ -52,7 +51,7 @@ from app.schemas.admin import ( UserListResponse, ) from app.services.audit import get_client_ip, log_audit_event -from app.services.auth import hash_password +from app.services.auth import ahash_password # --------------------------------------------------------------------------- # Router — all endpoints inherit require_admin @@ -223,11 +222,10 @@ async def create_user( if email_exists.scalar_one_or_none(): raise HTTPException(status_code=409, detail="Email already in use") - loop = asyncio.get_running_loop() new_user = User( username=data.username, umbral_name=data.username, - password_hash=await loop.run_in_executor(None, hash_password, data.password), + password_hash=await ahash_password(data.password), role=data.role, email=email, first_name=data.first_name, @@ -343,8 +341,7 @@ async def reset_user_password( raise HTTPException(status_code=404, detail="User not found") temp_password = secrets.token_urlsafe(16) - loop = asyncio.get_running_loop() - user.password_hash = await loop.run_in_executor(None, hash_password, temp_password) + user.password_hash = await ahash_password(temp_password) user.must_change_password = True user.last_password_change_at = datetime.now() diff --git a/backend/app/routers/auth.py b/backend/app/routers/auth.py index 1478b45..6a85c6b 100644 --- a/backend/app/routers/auth.py +++ b/backend/app/routers/auth.py @@ -16,7 +16,6 @@ Security layers: 4. bcrypt→Argon2id transparent upgrade on first login 5. Role-based authorization via require_role() dependency factory """ -import asyncio import uuid from datetime import datetime, timedelta from typing import Optional @@ -38,6 +37,9 @@ from app.schemas.auth import ( ProfileUpdate, ProfileResponse, ) from app.services.auth import ( + ahash_password, + averify_password, + averify_password_with_upgrade, hash_password, verify_password, verify_password_with_upgrade, @@ -304,8 +306,7 @@ async def setup( if user_count.scalar_one() > 0: raise HTTPException(status_code=400, detail="Setup already completed") - loop = asyncio.get_running_loop() - password_hash = await loop.run_in_executor(None, hash_password, data.password) + password_hash = await ahash_password(data.password) new_user = User( username=data.username, umbral_name=data.username, @@ -358,18 +359,12 @@ async def login( if not user: # M-02: Run Argon2id against a dummy hash so the response time is # indistinguishable from a wrong-password attempt (prevents username enumeration). - # AC-2: run_in_executor to avoid blocking the event loop (~150ms) - loop = asyncio.get_running_loop() - await loop.run_in_executor(None, verify_password, "x", _DUMMY_HASH) + await averify_password("x", _DUMMY_HASH) raise HTTPException(status_code=401, detail="Invalid username or password") # M-02: Run password verification BEFORE lockout check so Argon2id always # executes — prevents distinguishing "locked" from "wrong password" via timing. - # AC-2: run_in_executor to avoid blocking the event loop - loop = asyncio.get_running_loop() - valid, new_hash = await loop.run_in_executor( - None, verify_password_with_upgrade, data.password, user.password_hash - ) + valid, new_hash = await averify_password_with_upgrade(data.password, user.password_hash) await _check_account_lockout(user) @@ -477,8 +472,7 @@ async def register( if existing_email.scalar_one_or_none(): raise HTTPException(status_code=400, detail="Registration could not be completed. Please check your details and try again.") - loop = asyncio.get_running_loop() - password_hash = await loop.run_in_executor(None, hash_password, data.password) + password_hash = await ahash_password(data.password) # SEC-01: Explicit field assignment — never **data.model_dump() new_user = User( username=data.username, @@ -643,10 +637,7 @@ async def verify_password( """ await _check_account_lockout(current_user) - loop = asyncio.get_running_loop() - valid, new_hash = await loop.run_in_executor( - None, verify_password_with_upgrade, data.password, current_user.password_hash - ) + valid, new_hash = await averify_password_with_upgrade(data.password, current_user.password_hash) if not valid: await _record_failed_login(db, current_user) raise HTTPException(status_code=401, detail="Invalid password") @@ -672,10 +663,7 @@ async def change_password( """Change the current user's password. Requires old password verification.""" await _check_account_lockout(current_user) - loop = asyncio.get_running_loop() - valid, _ = await loop.run_in_executor( - None, verify_password_with_upgrade, data.old_password, current_user.password_hash - ) + valid, _ = await averify_password_with_upgrade(data.old_password, current_user.password_hash) if not valid: await _record_failed_login(db, current_user) raise HTTPException(status_code=401, detail="Invalid current password") @@ -683,7 +671,7 @@ async def change_password( if data.new_password == data.old_password: raise HTTPException(status_code=400, detail="New password must be different from your current password") - current_user.password_hash = await loop.run_in_executor(None, hash_password, data.new_password) + current_user.password_hash = await ahash_password(data.new_password) current_user.last_password_change_at = datetime.now() # Clear forced password change flag if set (SEC-12) diff --git a/backend/app/routers/totp.py b/backend/app/routers/totp.py index 1371bf8..9839b18 100644 --- a/backend/app/routers/totp.py +++ b/backend/app/routers/totp.py @@ -17,6 +17,7 @@ Security: - Failed TOTP attempts increment user.failed_login_count (shared lockout counter) - totp-verify uses mfa_token (not session cookie) — user is not yet authenticated """ +import asyncio import uuid import secrets import logging @@ -37,8 +38,7 @@ from app.models.backup_code import BackupCode from app.routers.auth import get_current_user, _set_session_cookie from app.services.audit import get_client_ip from app.services.auth import ( - verify_password_with_upgrade, - hash_password, + averify_password_with_upgrade, verify_mfa_token, verify_mfa_enforce_token, create_session_token, @@ -117,8 +117,10 @@ class EnforceConfirmRequest(BaseModel): async def _store_backup_codes(db: AsyncSession, user_id: int, plaintext_codes: list[str]) -> None: """Hash and insert backup codes for the given user.""" + # AC-2: Run Argon2id hashing in executor to avoid blocking event loop + loop = asyncio.get_running_loop() for code in plaintext_codes: - code_hash = _ph.hash(code) + code_hash = await loop.run_in_executor(None, _ph.hash, code) db.add(BackupCode(user_id=user_id, code_hash=code_hash)) await db.commit() @@ -145,9 +147,12 @@ async def _verify_backup_code( ) unused_codes = result.scalars().all() + # AC-2: Run Argon2id verification in executor to avoid blocking event loop + loop = asyncio.get_running_loop() for record in unused_codes: try: - if _ph.verify(record.code_hash, submitted_code): + matched = await loop.run_in_executor(None, _ph.verify, record.code_hash, submitted_code) + if matched: record.used_at = datetime.now() await db.commit() return True @@ -355,7 +360,8 @@ async def totp_disable( raise HTTPException(status_code=400, detail="TOTP is not enabled") # Verify password (handles bcrypt→Argon2id upgrade transparently) - valid, new_hash = verify_password_with_upgrade(data.password, current_user.password_hash) + # AC-2: async wrapper to avoid blocking event loop + valid, new_hash = await averify_password_with_upgrade(data.password, current_user.password_hash) if not valid: raise HTTPException(status_code=401, detail="Invalid password") @@ -391,7 +397,8 @@ async def regenerate_backup_codes( if not current_user.totp_enabled: raise HTTPException(status_code=400, detail="TOTP is not enabled") - valid, new_hash = verify_password_with_upgrade(data.password, current_user.password_hash) + # AC-2: async wrapper to avoid blocking event loop + valid, new_hash = await averify_password_with_upgrade(data.password, current_user.password_hash) if not valid: raise HTTPException(status_code=401, detail="Invalid password") diff --git a/backend/app/services/auth.py b/backend/app/services/auth.py index c6ef16a..c12ca34 100644 --- a/backend/app/services/auth.py +++ b/backend/app/services/auth.py @@ -6,6 +6,8 @@ Password strategy: - Legacy bcrypt hashes (migrated from PIN auth): accepted on login, immediately rehashed to Argon2id on first successful use. """ +import asyncio + from argon2 import PasswordHasher from argon2.exceptions import VerifyMismatchError, VerificationError, InvalidHashError from itsdangerous import URLSafeTimedSerializer, BadSignature, SignatureExpired @@ -76,6 +78,28 @@ def verify_password_with_upgrade(password: str, hashed: str) -> tuple[bool, str return valid, new_hash +# --------------------------------------------------------------------------- +# Async wrappers — run CPU-bound Argon2id ops in a thread pool (AC-2/S-01) +# --------------------------------------------------------------------------- + +async def ahash_password(password: str) -> str: + """Async wrapper for hash_password — runs Argon2id in executor.""" + loop = asyncio.get_running_loop() + return await loop.run_in_executor(None, hash_password, password) + + +async def averify_password(password: str, hashed: str) -> bool: + """Async wrapper for verify_password — runs Argon2id in executor.""" + loop = asyncio.get_running_loop() + return await loop.run_in_executor(None, verify_password, password, hashed) + + +async def averify_password_with_upgrade(password: str, hashed: str) -> tuple[bool, str | None]: + """Async wrapper for verify_password_with_upgrade — runs Argon2id in executor.""" + loop = asyncio.get_running_loop() + return await loop.run_in_executor(None, verify_password_with_upgrade, password, hashed) + + # --------------------------------------------------------------------------- # Session tokens # --------------------------------------------------------------------------- diff --git a/docker-compose.yaml b/docker-compose.yaml index 616f9af..2dc1a43 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -33,6 +33,7 @@ services: interval: 10s timeout: 5s retries: 3 + start_period: 30s deploy: resources: limits: diff --git a/frontend/src/components/calendar/CalendarPage.tsx b/frontend/src/components/calendar/CalendarPage.tsx index c1754ba..fe71c6d 100644 --- a/frontend/src/components/calendar/CalendarPage.tsx +++ b/frontend/src/components/calendar/CalendarPage.tsx @@ -1,6 +1,7 @@ import { useState, useRef, useEffect, useMemo, useCallback } from 'react'; import { useMediaQuery, DESKTOP } from '@/hooks/useMediaQuery'; import { useLocation } from 'react-router-dom'; +import { format } from 'date-fns'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { toast } from 'sonner'; import FullCalendar from '@fullcalendar/react'; @@ -206,17 +207,23 @@ export default function CalendarPage() { }, []); // AW-2: Track visible date range for scoped event fetching - const [visibleRange, setVisibleRange] = useState<{ start: string; end: string } | null>(null); + // W-02 fix: Initialize from current month to avoid unscoped first fetch + const [visibleRange, setVisibleRange] = useState<{ start: string; end: string }>(() => { + const now = new Date(); + const y = now.getFullYear(); + const m = now.getMonth(); + // FullCalendar month view typically fetches prev month to next month + const start = format(new Date(y, m - 1, 1), 'yyyy-MM-dd'); + const end = format(new Date(y, m + 2, 0), 'yyyy-MM-dd'); + return { start, end }; + }); const { data: events = [] } = useQuery({ - queryKey: ['calendar-events', visibleRange?.start, visibleRange?.end], + queryKey: ['calendar-events', visibleRange.start, visibleRange.end], queryFn: async () => { - const params: Record = {}; - if (visibleRange) { - params.start = visibleRange.start; - params.end = visibleRange.end; - } - const { data } = await api.get('/events', { params }); + const { data } = await api.get('/events', { + params: { start: visibleRange.start, end: visibleRange.end }, + }); return data; }, // AW-3: Reduce from 5s to 30s — personal organiser doesn't need 12 calls/min @@ -270,12 +277,15 @@ export default function CalendarPage() { allDay: boolean, revert: () => void, ) => { - queryClient.setQueryData(['calendar-events'], (old) => - old?.map((e) => - e.id === id - ? { ...e, start_datetime: start, end_datetime: end, all_day: allDay } - : e, - ), + // C-01 fix: match active query key which includes date range + queryClient.setQueryData( + ['calendar-events', visibleRange.start, visibleRange.end], + (old) => + old?.map((e) => + e.id === id + ? { ...e, start_datetime: start, end_datetime: end, all_day: allDay } + : e, + ), ); eventMutation.mutate({ id, start, end, allDay, revert }); }; @@ -477,10 +487,11 @@ export default function CalendarPage() { setCalendarTitle(arg.view.title); setCurrentView(arg.view.type as CalendarView); // AW-2: Capture visible range for scoped event fetching - const start = arg.start.toISOString().split('T')[0]; - const end = arg.end.toISOString().split('T')[0]; + // C-02 fix: use format() not toISOString() to avoid UTC date shift + const start = format(arg.start, 'yyyy-MM-dd'); + const end = format(arg.end, 'yyyy-MM-dd'); setVisibleRange((prev) => - prev?.start === start && prev?.end === end ? prev : { start, end } + prev.start === start && prev.end === end ? prev : { start, end } ); };