Address code review findings across all phases

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 <noreply@anthropic.com>
This commit is contained in:
Kyle 2026-03-13 00:19:33 +08:00
parent 2ab7121e42
commit a94485b138
8 changed files with 87 additions and 55 deletions

View File

@ -32,7 +32,8 @@ pytest.ini
htmlcov htmlcov
# Documentation # Documentation
*.md README.md
CHANGELOG.md
LICENSE LICENSE
# Dev scripts # Dev scripts

View File

@ -239,16 +239,19 @@ 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 # Batch-fetch all sent keys once per user instead of one query per entity
sent_keys = await _get_sent_keys(db, settings.user_id) 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: if settings.ntfy_reminders_enabled:
await _dispatch_reminders(db, settings, now, sent_keys) await _dispatch_reminders(db, settings, now, sent_keys)
await db.commit()
if settings.ntfy_events_enabled: if settings.ntfy_events_enabled:
await _dispatch_events(db, settings, now, sent_keys) await _dispatch_events(db, settings, now, sent_keys)
await db.commit()
if settings.ntfy_todos_enabled: if settings.ntfy_todos_enabled:
await _dispatch_todos(db, settings, now.date(), sent_keys) await _dispatch_todos(db, settings, now.date(), sent_keys)
await db.commit()
if settings.ntfy_projects_enabled: if settings.ntfy_projects_enabled:
await _dispatch_projects(db, settings, now.date(), sent_keys) 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()

View File

@ -10,7 +10,6 @@ Security measures implemented:
All routes require the `require_admin` dependency (which chains through All routes require the `require_admin` dependency (which chains through
get_current_user, so the session cookie is always validated). get_current_user, so the session cookie is always validated).
""" """
import asyncio
import secrets import secrets
from datetime import datetime from datetime import datetime
from typing import Optional from typing import Optional
@ -52,7 +51,7 @@ from app.schemas.admin import (
UserListResponse, UserListResponse,
) )
from app.services.audit import get_client_ip, log_audit_event 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 # Router — all endpoints inherit require_admin
@ -223,11 +222,10 @@ async def create_user(
if email_exists.scalar_one_or_none(): if email_exists.scalar_one_or_none():
raise HTTPException(status_code=409, detail="Email already in use") raise HTTPException(status_code=409, detail="Email already in use")
loop = asyncio.get_running_loop()
new_user = User( new_user = User(
username=data.username, username=data.username,
umbral_name=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, role=data.role,
email=email, email=email,
first_name=data.first_name, first_name=data.first_name,
@ -343,8 +341,7 @@ async def reset_user_password(
raise HTTPException(status_code=404, detail="User not found") raise HTTPException(status_code=404, detail="User not found")
temp_password = secrets.token_urlsafe(16) temp_password = secrets.token_urlsafe(16)
loop = asyncio.get_running_loop() user.password_hash = await ahash_password(temp_password)
user.password_hash = await loop.run_in_executor(None, hash_password, temp_password)
user.must_change_password = True user.must_change_password = True
user.last_password_change_at = datetime.now() user.last_password_change_at = datetime.now()

View File

@ -16,7 +16,6 @@ Security layers:
4. bcryptArgon2id transparent upgrade on first login 4. bcryptArgon2id transparent upgrade on first login
5. Role-based authorization via require_role() dependency factory 5. Role-based authorization via require_role() dependency factory
""" """
import asyncio
import uuid import uuid
from datetime import datetime, timedelta from datetime import datetime, timedelta
from typing import Optional from typing import Optional
@ -38,6 +37,9 @@ from app.schemas.auth import (
ProfileUpdate, ProfileResponse, ProfileUpdate, ProfileResponse,
) )
from app.services.auth import ( from app.services.auth import (
ahash_password,
averify_password,
averify_password_with_upgrade,
hash_password, hash_password,
verify_password, verify_password,
verify_password_with_upgrade, verify_password_with_upgrade,
@ -304,8 +306,7 @@ async def setup(
if user_count.scalar_one() > 0: if user_count.scalar_one() > 0:
raise HTTPException(status_code=400, detail="Setup already completed") raise HTTPException(status_code=400, detail="Setup already completed")
loop = asyncio.get_running_loop() password_hash = await ahash_password(data.password)
password_hash = await loop.run_in_executor(None, hash_password, data.password)
new_user = User( new_user = User(
username=data.username, username=data.username,
umbral_name=data.username, umbral_name=data.username,
@ -358,18 +359,12 @@ async def login(
if not user: if not user:
# M-02: Run Argon2id against a dummy hash so the response time is # M-02: Run Argon2id against a dummy hash so the response time is
# indistinguishable from a wrong-password attempt (prevents username enumeration). # indistinguishable from a wrong-password attempt (prevents username enumeration).
# AC-2: run_in_executor to avoid blocking the event loop (~150ms) await averify_password("x", _DUMMY_HASH)
loop = asyncio.get_running_loop()
await loop.run_in_executor(None, verify_password, "x", _DUMMY_HASH)
raise HTTPException(status_code=401, detail="Invalid username or password") raise HTTPException(status_code=401, detail="Invalid username or password")
# M-02: Run password verification BEFORE lockout check so Argon2id always # M-02: Run password verification BEFORE lockout check so Argon2id always
# executes — prevents distinguishing "locked" from "wrong password" via timing. # executes — prevents distinguishing "locked" from "wrong password" via timing.
# AC-2: run_in_executor to avoid blocking the event loop valid, new_hash = await averify_password_with_upgrade(data.password, user.password_hash)
loop = asyncio.get_running_loop()
valid, new_hash = await loop.run_in_executor(
None, verify_password_with_upgrade, data.password, user.password_hash
)
await _check_account_lockout(user) await _check_account_lockout(user)
@ -477,8 +472,7 @@ async def register(
if existing_email.scalar_one_or_none(): 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.") 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 ahash_password(data.password)
password_hash = await loop.run_in_executor(None, hash_password, data.password)
# SEC-01: Explicit field assignment — never **data.model_dump() # SEC-01: Explicit field assignment — never **data.model_dump()
new_user = User( new_user = User(
username=data.username, username=data.username,
@ -643,10 +637,7 @@ async def verify_password(
""" """
await _check_account_lockout(current_user) await _check_account_lockout(current_user)
loop = asyncio.get_running_loop() valid, new_hash = await averify_password_with_upgrade(data.password, current_user.password_hash)
valid, new_hash = await loop.run_in_executor(
None, verify_password_with_upgrade, data.password, current_user.password_hash
)
if not valid: if not valid:
await _record_failed_login(db, current_user) await _record_failed_login(db, current_user)
raise HTTPException(status_code=401, detail="Invalid password") 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.""" """Change the current user's password. Requires old password verification."""
await _check_account_lockout(current_user) await _check_account_lockout(current_user)
loop = asyncio.get_running_loop() valid, _ = await averify_password_with_upgrade(data.old_password, current_user.password_hash)
valid, _ = await loop.run_in_executor(
None, verify_password_with_upgrade, data.old_password, current_user.password_hash
)
if not valid: if not valid:
await _record_failed_login(db, current_user) await _record_failed_login(db, current_user)
raise HTTPException(status_code=401, detail="Invalid current password") raise HTTPException(status_code=401, detail="Invalid current password")
@ -683,7 +671,7 @@ async def change_password(
if data.new_password == data.old_password: if data.new_password == data.old_password:
raise HTTPException(status_code=400, detail="New password must be different from your current 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() current_user.last_password_change_at = datetime.now()
# Clear forced password change flag if set (SEC-12) # Clear forced password change flag if set (SEC-12)

View File

@ -17,6 +17,7 @@ Security:
- Failed TOTP attempts increment user.failed_login_count (shared lockout counter) - Failed TOTP attempts increment user.failed_login_count (shared lockout counter)
- totp-verify uses mfa_token (not session cookie) user is not yet authenticated - totp-verify uses mfa_token (not session cookie) user is not yet authenticated
""" """
import asyncio
import uuid import uuid
import secrets import secrets
import logging 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.routers.auth import get_current_user, _set_session_cookie
from app.services.audit import get_client_ip from app.services.audit import get_client_ip
from app.services.auth import ( from app.services.auth import (
verify_password_with_upgrade, averify_password_with_upgrade,
hash_password,
verify_mfa_token, verify_mfa_token,
verify_mfa_enforce_token, verify_mfa_enforce_token,
create_session_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: async def _store_backup_codes(db: AsyncSession, user_id: int, plaintext_codes: list[str]) -> None:
"""Hash and insert backup codes for the given user.""" """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: 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)) db.add(BackupCode(user_id=user_id, code_hash=code_hash))
await db.commit() await db.commit()
@ -145,9 +147,12 @@ async def _verify_backup_code(
) )
unused_codes = result.scalars().all() 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: for record in unused_codes:
try: 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() record.used_at = datetime.now()
await db.commit() await db.commit()
return True return True
@ -355,7 +360,8 @@ async def totp_disable(
raise HTTPException(status_code=400, detail="TOTP is not enabled") raise HTTPException(status_code=400, detail="TOTP is not enabled")
# Verify password (handles bcrypt→Argon2id upgrade transparently) # 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: if not valid:
raise HTTPException(status_code=401, detail="Invalid password") raise HTTPException(status_code=401, detail="Invalid password")
@ -391,7 +397,8 @@ async def regenerate_backup_codes(
if not current_user.totp_enabled: if not current_user.totp_enabled:
raise HTTPException(status_code=400, detail="TOTP is not 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: if not valid:
raise HTTPException(status_code=401, detail="Invalid password") raise HTTPException(status_code=401, detail="Invalid password")

View File

@ -6,6 +6,8 @@ Password strategy:
- Legacy bcrypt hashes (migrated from PIN auth): accepted on login, immediately - Legacy bcrypt hashes (migrated from PIN auth): accepted on login, immediately
rehashed to Argon2id on first successful use. rehashed to Argon2id on first successful use.
""" """
import asyncio
from argon2 import PasswordHasher from argon2 import PasswordHasher
from argon2.exceptions import VerifyMismatchError, VerificationError, InvalidHashError from argon2.exceptions import VerifyMismatchError, VerificationError, InvalidHashError
from itsdangerous import URLSafeTimedSerializer, BadSignature, SignatureExpired 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 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 # Session tokens
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------

View File

@ -33,6 +33,7 @@ services:
interval: 10s interval: 10s
timeout: 5s timeout: 5s
retries: 3 retries: 3
start_period: 30s
deploy: deploy:
resources: resources:
limits: limits:

View File

@ -1,6 +1,7 @@
import { useState, useRef, useEffect, useMemo, useCallback } from 'react'; import { useState, useRef, useEffect, useMemo, useCallback } from 'react';
import { useMediaQuery, DESKTOP } from '@/hooks/useMediaQuery'; import { useMediaQuery, DESKTOP } from '@/hooks/useMediaQuery';
import { useLocation } from 'react-router-dom'; import { useLocation } from 'react-router-dom';
import { format } from 'date-fns';
import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query';
import { toast } from 'sonner'; import { toast } from 'sonner';
import FullCalendar from '@fullcalendar/react'; import FullCalendar from '@fullcalendar/react';
@ -206,17 +207,23 @@ export default function CalendarPage() {
}, []); }, []);
// AW-2: Track visible date range for scoped event fetching // 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({ const { data: events = [] } = useQuery({
queryKey: ['calendar-events', visibleRange?.start, visibleRange?.end], queryKey: ['calendar-events', visibleRange.start, visibleRange.end],
queryFn: async () => { queryFn: async () => {
const params: Record<string, string> = {}; const { data } = await api.get<CalendarEvent[]>('/events', {
if (visibleRange) { params: { start: visibleRange.start, end: visibleRange.end },
params.start = visibleRange.start; });
params.end = visibleRange.end;
}
const { data } = await api.get<CalendarEvent[]>('/events', { params });
return data; return data;
}, },
// AW-3: Reduce from 5s to 30s — personal organiser doesn't need 12 calls/min // AW-3: Reduce from 5s to 30s — personal organiser doesn't need 12 calls/min
@ -270,7 +277,10 @@ export default function CalendarPage() {
allDay: boolean, allDay: boolean,
revert: () => void, revert: () => void,
) => { ) => {
queryClient.setQueryData<CalendarEvent[]>(['calendar-events'], (old) => // C-01 fix: match active query key which includes date range
queryClient.setQueryData<CalendarEvent[]>(
['calendar-events', visibleRange.start, visibleRange.end],
(old) =>
old?.map((e) => old?.map((e) =>
e.id === id e.id === id
? { ...e, start_datetime: start, end_datetime: end, all_day: allDay } ? { ...e, start_datetime: start, end_datetime: end, all_day: allDay }
@ -477,10 +487,11 @@ export default function CalendarPage() {
setCalendarTitle(arg.view.title); setCalendarTitle(arg.view.title);
setCurrentView(arg.view.type as CalendarView); setCurrentView(arg.view.type as CalendarView);
// AW-2: Capture visible range for scoped event fetching // AW-2: Capture visible range for scoped event fetching
const start = arg.start.toISOString().split('T')[0]; // C-02 fix: use format() not toISOString() to avoid UTC date shift
const end = arg.end.toISOString().split('T')[0]; const start = format(arg.start, 'yyyy-MM-dd');
const end = format(arg.end, 'yyyy-MM-dd');
setVisibleRange((prev) => setVisibleRange((prev) =>
prev?.start === start && prev?.end === end ? prev : { start, end } prev.start === start && prev.end === end ? prev : { start, end }
); );
}; };