From bc4968a876eb9c2ad0828355f8c596bc2ca078ec Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Sun, 7 Jun 2026 01:56:46 +0530 Subject: [PATCH 1/8] fix(security): authenticate shutdown endpoints with a per-session token The /shutdown endpoints on both the backend (port 52123) and sync microservice (port 52124) were unauthenticated, allowing any local process to send a POST request and terminate PictoPy without user interaction. Fix: - Generate a cryptographically random 256-bit token (secrets.token_hex) on every backend startup and write it to a temporary file (pictopy_shutdown.token in the OS temp directory). - Both shutdown endpoints now require an X-Shutdown-Token header whose value is compared against the session token using hmac.compare_digest to prevent timing-based attacks. Requests with a missing or incorrect token receive 403 Forbidden. - The sync microservice reads the same token file written by the backend, so both services share one token without additional coordination. - The Tauri frontend (Windows path) is updated to read the token file at shutdown time and attach it as an X-Shutdown-Token header, preserving the existing Windows close behaviour. Closes #1241 --- backend/app/config/settings.py | 16 ++++++++++ backend/app/routes/shutdown.py | 17 ++++++++--- frontend/src-tauri/src/main.rs | 32 ++++++++++++++++++- sync-microservice/app/config/settings.py | 39 +++++++++++++++++++++++- sync-microservice/app/routes/shutdown.py | 23 +++++++++++--- 5 files changed, 116 insertions(+), 11 deletions(-) diff --git a/backend/app/config/settings.py b/backend/app/config/settings.py index 912134a64..4dd8cdcd3 100644 --- a/backend/app/config/settings.py +++ b/backend/app/config/settings.py @@ -1,5 +1,7 @@ import os import sys +import secrets +import tempfile from platformdirs import user_data_dir @@ -35,3 +37,17 @@ DATABASE_PATH = os.path.join(user_data_dir("PictoPy"), "database", "PictoPy.db") THUMBNAIL_IMAGES_PATH = os.path.join(user_data_dir("PictoPy"), "thumbnails") IMAGES_PATH = "./images" + +# Generate a fresh cryptographic token on every backend startup. +# This token is written to a temporary file so that only the local Tauri +# frontend — which reads the same file — can authenticate shutdown requests. +# Any other process on the machine will not know the token and will be +# rejected with 403 Forbidden. +SHUTDOWN_TOKEN: str = secrets.token_hex(32) +SHUTDOWN_TOKEN_FILE: str = os.path.join(tempfile.gettempdir(), "pictopy_shutdown.token") + +# Write with owner-only permissions (0o600) so other local users on +# multi-user systems cannot read the token and trigger a shutdown. +_fd = os.open(SHUTDOWN_TOKEN_FILE, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) +with os.fdopen(_fd, "w") as _f: + _f.write(SHUTDOWN_TOKEN) diff --git a/backend/app/routes/shutdown.py b/backend/app/routes/shutdown.py index 716a79104..5d67d455b 100644 --- a/backend/app/routes/shutdown.py +++ b/backend/app/routes/shutdown.py @@ -1,9 +1,11 @@ import asyncio +import hmac import os import platform import signal -from fastapi import APIRouter +from fastapi import APIRouter, Header, HTTPException from pydantic import BaseModel +from app.config.settings import SHUTDOWN_TOKEN from app.logging.setup_logging import get_logger logger = get_logger(__name__) @@ -37,16 +39,23 @@ async def _delayed_shutdown(delay: float = 0.5): @router.post("/shutdown", response_model=ShutdownResponse) -async def shutdown(): +async def shutdown(x_shutdown_token: str = Header(...)): """ Gracefully shutdown the PictoPy backend. - This endpoint schedules backend server termination after response is sent. - The frontend is responsible for shutting down the sync service separately. + This endpoint requires the ``X-Shutdown-Token`` header to match the token + generated at startup. The token is shared with the Tauri frontend via a + temporary file, so only the PictoPy application itself can trigger this + endpoint — arbitrary local processes are rejected with 403 Forbidden. Returns: ShutdownResponse with status and message """ + # Use constant-time comparison to prevent timing-based token guessing + if not hmac.compare_digest(x_shutdown_token, SHUTDOWN_TOKEN): + logger.warning("Shutdown attempt rejected: invalid token") + raise HTTPException(status_code=403, detail="Forbidden") + logger.info("Shutdown request received for PictoPy backend") # Define callback to handle potential exceptions in the background task diff --git a/frontend/src-tauri/src/main.rs b/frontend/src-tauri/src/main.rs index 5b7aa6acc..ad604762a 100644 --- a/frontend/src-tauri/src/main.rs +++ b/frontend/src-tauri/src/main.rs @@ -61,11 +61,41 @@ fn kill_process(process: &sysinfo::Process) { #[cfg(windows)] pub fn kill_process(_process: &sysinfo::Process) -> Result<(), String> { use reqwest::blocking::Client; + use reqwest::header::{HeaderMap, HeaderName, HeaderValue}; + use std::str::FromStr; + + // Read the per-session shutdown token written by the backend at startup. + // This guarantees that only the PictoPy frontend — which runs alongside + // the backend — can authenticate the shutdown request. + let token_path = std::env::temp_dir().join("pictopy_shutdown.token"); + let token = match std::fs::read_to_string(&token_path) { + Ok(t) => { + let trimmed = t.trim().to_string(); + if trimmed.is_empty() { + eprintln!("[PictoPy] Warning: shutdown token file is empty — shutdown request will be rejected by the backend."); + } + trimmed + } + Err(e) => { + eprintln!("[PictoPy] Warning: could not read shutdown token file ({token_path:?}): {e} — shutdown request will be rejected by the backend."); + String::new() + } + }; + + let mut headers = HeaderMap::new(); + if !token.is_empty() { + if let (Ok(name), Ok(value)) = ( + HeaderName::from_str("x-shutdown-token"), + HeaderValue::from_str(&token), + ) { + headers.insert(name, value); + } + } let client = Client::builder().build().map_err(|e| e.to_string())?; for (name, url, _) in &ENDPOINTS { - match client.post(*url).send() { + match client.post(*url).headers(headers.clone()).send() { Ok(resp) => { let status = resp.status(); diff --git a/sync-microservice/app/config/settings.py b/sync-microservice/app/config/settings.py index b9e2053a3..90160814a 100644 --- a/sync-microservice/app/config/settings.py +++ b/sync-microservice/app/config/settings.py @@ -1,5 +1,10 @@ -from platformdirs import user_data_dir import os +import secrets +import tempfile +import time as _time +import warnings + +from platformdirs import user_data_dir # Model Exports Path MODEL_EXPORTS_PATH = "app/models/ONNX_Exports" @@ -28,3 +33,35 @@ DATABASE_PATH = os.path.join(user_data_dir("PictoPy"), "database", "PictoPy.db") THUMBNAIL_IMAGES_PATH = "./images/thumbnails" IMAGES_PATH = "./images" + +# The backend writes a fresh cryptographic token to this temp file on every +# startup. The sync microservice reads the same file so both services share +# a single token without any additional coordination. +SHUTDOWN_TOKEN_FILE: str = os.path.join(tempfile.gettempdir(), "pictopy_shutdown.token") + +# Retry for up to 5 seconds to handle the race where the sync microservice +# starts before the backend has had a chance to write the token file. +_deadline = _time.monotonic() + 5.0 +SHUTDOWN_TOKEN: str = "" +while _time.monotonic() < _deadline: + try: + with open(SHUTDOWN_TOKEN_FILE) as _f: + _token = _f.read().strip() + if _token: + SHUTDOWN_TOKEN = _token + break + except FileNotFoundError: + pass + _time.sleep(0.1) + +if not SHUTDOWN_TOKEN: + # Backend token unavailable after timeout — generate a fallback so the + # service can still start, but log a clear warning so the issue is visible. + SHUTDOWN_TOKEN = secrets.token_hex(32) + warnings.warn( + "pictopy_shutdown.token not found after 5 s; using an independent " + "shutdown token. The sync /shutdown endpoint may reject requests from " + "the Tauri frontend. Ensure the backend starts before the sync service.", + RuntimeWarning, + stacklevel=1, + ) diff --git a/sync-microservice/app/routes/shutdown.py b/sync-microservice/app/routes/shutdown.py index 9e31ee42d..a725fe037 100644 --- a/sync-microservice/app/routes/shutdown.py +++ b/sync-microservice/app/routes/shutdown.py @@ -1,9 +1,11 @@ import asyncio +import hmac import os import platform import signal -from fastapi import APIRouter +from fastapi import APIRouter, Header, HTTPException from pydantic import BaseModel +from app.config.settings import SHUTDOWN_TOKEN from app.utils.watcher import watcher_util_stop_folder_watcher from app.logging.setup_logging import get_sync_logger @@ -38,18 +40,29 @@ async def _delayed_shutdown(delay: float = 0.1): @router.post("/shutdown", response_model=ShutdownResponse) -async def shutdown(): +async def shutdown(x_shutdown_token: str = Header(...)): """ Gracefully shutdown the sync microservice. + This endpoint requires the ``X-Shutdown-Token`` header to match the token + generated by the backend at startup. The token is shared between services + via a temporary file, so only the PictoPy application itself can trigger + shutdown — arbitrary local processes are rejected with 403 Forbidden. + This endpoint: - 1. Stops the folder watcher - 2. Schedules server termination after response is sent - 3. Returns confirmation to the caller + 1. Validates the shutdown token + 2. Stops the folder watcher + 3. Schedules server termination after response is sent + 4. Returns confirmation to the caller Returns: ShutdownResponse with status and message """ + # Use constant-time comparison to prevent timing-based token guessing + if not hmac.compare_digest(x_shutdown_token, SHUTDOWN_TOKEN): + logger.warning("Shutdown attempt rejected: invalid token") + raise HTTPException(status_code=403, detail="Forbidden") + logger.info("Shutdown request received for sync microservice") try: From 536b33294347939d30fd389755a2b2f53c23d300 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Fri, 12 Jun 2026 07:52:32 +0530 Subject: [PATCH 2/8] fix(security): address CodeRabbit review findings on shutdown endpoint - Moved blocking sleep out of settings.py into FastAPI lifespan hook - Removed silent broken-state fallback token; now raises RuntimeError - Added error handling and chmod to token write in backend - Fixed 422 vs 401 response code on missing token - Ensured token file is cleaned up after shutdown - Fixed Rust kill_process visibility and error logging --- backend/app/config/settings.py | 13 +++++++++--- backend/app/routes/shutdown.py | 15 ++++++++++++- frontend/src-tauri/src/main.rs | 19 +++++++++++++---- sync-microservice/app/config/settings.py | 27 +----------------------- sync-microservice/app/core/lifespan.py | 21 ++++++++++++++++++ sync-microservice/app/routes/shutdown.py | 15 ++++++++++++- 6 files changed, 75 insertions(+), 35 deletions(-) diff --git a/backend/app/config/settings.py b/backend/app/config/settings.py index a30f6476b..fcf375731 100644 --- a/backend/app/config/settings.py +++ b/backend/app/config/settings.py @@ -54,9 +54,16 @@ # Write with owner-only permissions (0o600) so other local users on # multi-user systems cannot read the token and trigger a shutdown. -_fd = os.open(SHUTDOWN_TOKEN_FILE, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) -with os.fdopen(_fd, "w") as _f: - _f.write(SHUTDOWN_TOKEN) +try: + _fd = os.open(SHUTDOWN_TOKEN_FILE, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(_fd, "w") as _f: + _f.write(SHUTDOWN_TOKEN) + # Enforce permissions even if file pre-existed with looser permissions + os.chmod(SHUTDOWN_TOKEN_FILE, 0o600) +except OSError as e: + logger.fatal(f"Failed to write shutdown token to {SHUTDOWN_TOKEN_FILE}: {e}") + logger.fatal("Cannot start backend securely. Exiting.") + sys.exit(1) def _get_env_float( name: str, diff --git a/backend/app/routes/shutdown.py b/backend/app/routes/shutdown.py index 5d67d455b..41d5e543b 100644 --- a/backend/app/routes/shutdown.py +++ b/backend/app/routes/shutdown.py @@ -3,6 +3,7 @@ import os import platform import signal +from typing import Optional from fastapi import APIRouter, Header, HTTPException from pydantic import BaseModel from app.config.settings import SHUTDOWN_TOKEN @@ -30,6 +31,14 @@ async def _delayed_shutdown(delay: float = 0.5): await asyncio.sleep(delay) logger.info("Backend shutdown initiated, exiting process...") + # Clean up token file + from app.config.settings import SHUTDOWN_TOKEN_FILE + try: + os.remove(SHUTDOWN_TOKEN_FILE) + logger.info("Shutdown token file removed") + except OSError as e: + logger.warning(f"Could not remove shutdown token file: {e}") + if platform.system() == "Windows": # Windows: SIGTERM doesn't work reliably with uvicorn subprocesses os._exit(0) @@ -39,7 +48,7 @@ async def _delayed_shutdown(delay: float = 0.5): @router.post("/shutdown", response_model=ShutdownResponse) -async def shutdown(x_shutdown_token: str = Header(...)): +async def shutdown(x_shutdown_token: Optional[str] = Header(default=None)): """ Gracefully shutdown the PictoPy backend. @@ -51,6 +60,10 @@ async def shutdown(x_shutdown_token: str = Header(...)): Returns: ShutdownResponse with status and message """ + if x_shutdown_token is None: + logger.warning("Shutdown attempt rejected: missing token") + raise HTTPException(status_code=401, detail="Unauthorized") + # Use constant-time comparison to prevent timing-based token guessing if not hmac.compare_digest(x_shutdown_token, SHUTDOWN_TOKEN): logger.warning("Shutdown attempt rejected: invalid token") diff --git a/frontend/src-tauri/src/main.rs b/frontend/src-tauri/src/main.rs index ad604762a..99f8a0954 100644 --- a/frontend/src-tauri/src/main.rs +++ b/frontend/src-tauri/src/main.rs @@ -53,13 +53,14 @@ fn on_window_event(window: &Window, event: &WindowEvent) { } #[cfg(unix)] -fn kill_process(process: &sysinfo::Process) { +fn kill_process(process: &sysinfo::Process) -> Result<(), String> { use sysinfo::Signal; let _ = process.kill_with(Signal::Term); + Ok(()) } #[cfg(windows)] -pub fn kill_process(_process: &sysinfo::Process) -> Result<(), String> { +fn kill_process(_process: &sysinfo::Process) -> Result<(), String> { use reqwest::blocking::Client; use reqwest::header::{HeaderMap, HeaderName, HeaderValue}; use std::str::FromStr; @@ -103,7 +104,12 @@ pub fn kill_process(_process: &sysinfo::Process) -> Result<(), String> { println!("[{}] Shutdown OK ({})", name, status); } } - Err(_err) => {} + Err(_err) => { + eprintln!( + "[{}] Failed to send shutdown request to {}: {}", + name, url, _err + ); + } } } @@ -125,7 +131,12 @@ fn kill_process_tree() -> Result<(), String> { let name = process.name().to_string_lossy(); if target_names.iter().any(|t| name.eq_ignore_ascii_case(t)) { - let _ = kill_process(process); + if let Err(e) = kill_process(process) { + eprintln!( + "[PictoPy] Failed to send shutdown signal to process {}: {}", + name, e + ); + } } } diff --git a/sync-microservice/app/config/settings.py b/sync-microservice/app/config/settings.py index 90160814a..c25f5c770 100644 --- a/sync-microservice/app/config/settings.py +++ b/sync-microservice/app/config/settings.py @@ -37,31 +37,6 @@ # The backend writes a fresh cryptographic token to this temp file on every # startup. The sync microservice reads the same file so both services share # a single token without any additional coordination. +# The token is loaded during the FastAPI lifespan startup hook. SHUTDOWN_TOKEN_FILE: str = os.path.join(tempfile.gettempdir(), "pictopy_shutdown.token") - -# Retry for up to 5 seconds to handle the race where the sync microservice -# starts before the backend has had a chance to write the token file. -_deadline = _time.monotonic() + 5.0 SHUTDOWN_TOKEN: str = "" -while _time.monotonic() < _deadline: - try: - with open(SHUTDOWN_TOKEN_FILE) as _f: - _token = _f.read().strip() - if _token: - SHUTDOWN_TOKEN = _token - break - except FileNotFoundError: - pass - _time.sleep(0.1) - -if not SHUTDOWN_TOKEN: - # Backend token unavailable after timeout — generate a fallback so the - # service can still start, but log a clear warning so the issue is visible. - SHUTDOWN_TOKEN = secrets.token_hex(32) - warnings.warn( - "pictopy_shutdown.token not found after 5 s; using an independent " - "shutdown token. The sync /shutdown endpoint may reject requests from " - "the Tauri frontend. Ensure the backend starts before the sync service.", - RuntimeWarning, - stacklevel=1, - ) diff --git a/sync-microservice/app/core/lifespan.py b/sync-microservice/app/core/lifespan.py index 26b75a5c0..a0b535c5a 100644 --- a/sync-microservice/app/core/lifespan.py +++ b/sync-microservice/app/core/lifespan.py @@ -27,6 +27,27 @@ async def lifespan(app: FastAPI): # Startup logger.info("Starting PictoPy Sync Microservice...") + # Wait for shutdown token from backend (up to 5 seconds) + import app.config.settings as settings + logger.info("Waiting for shutdown token from backend...") + deadline = time.monotonic() + 5.0 + while time.monotonic() < deadline: + try: + with open(settings.SHUTDOWN_TOKEN_FILE) as f: + token = f.read().strip() + if token: + settings.SHUTDOWN_TOKEN = token + logger.info("Shutdown token loaded successfully") + break + except FileNotFoundError: + pass + time.sleep(0.1) + + if not settings.SHUTDOWN_TOKEN: + logger.error(f"pictopy_shutdown.token not found at {settings.SHUTDOWN_TOKEN_FILE} after 5 seconds") + logger.error("Ensure the backend starts before the sync service.") + raise RuntimeError("Backend shutdown token not found") + # Check database connection logger.info("Checking database connection...") connection_timeout = 60 diff --git a/sync-microservice/app/routes/shutdown.py b/sync-microservice/app/routes/shutdown.py index a725fe037..7d59b25f2 100644 --- a/sync-microservice/app/routes/shutdown.py +++ b/sync-microservice/app/routes/shutdown.py @@ -3,6 +3,7 @@ import os import platform import signal +from typing import Optional from fastapi import APIRouter, Header, HTTPException from pydantic import BaseModel from app.config.settings import SHUTDOWN_TOKEN @@ -31,6 +32,14 @@ async def _delayed_shutdown(delay: float = 0.1): await asyncio.sleep(delay) logger.info("Exiting sync microservice...") + # Clean up token file + from app.config.settings import SHUTDOWN_TOKEN_FILE + try: + os.remove(SHUTDOWN_TOKEN_FILE) + logger.info("Shutdown token file removed") + except OSError as e: + logger.warning(f"Could not remove shutdown token file: {e}") + if platform.system() == "Windows": # Windows: SIGTERM doesn't work reliably with uvicorn subprocesses os._exit(0) @@ -40,7 +49,7 @@ async def _delayed_shutdown(delay: float = 0.1): @router.post("/shutdown", response_model=ShutdownResponse) -async def shutdown(x_shutdown_token: str = Header(...)): +async def shutdown(x_shutdown_token: Optional[str] = Header(default=None)): """ Gracefully shutdown the sync microservice. @@ -58,6 +67,10 @@ async def shutdown(x_shutdown_token: str = Header(...)): Returns: ShutdownResponse with status and message """ + if x_shutdown_token is None: + logger.warning("Shutdown attempt rejected: missing token") + raise HTTPException(status_code=401, detail="Unauthorized") + # Use constant-time comparison to prevent timing-based token guessing if not hmac.compare_digest(x_shutdown_token, SHUTDOWN_TOKEN): logger.warning("Shutdown attempt rejected: invalid token") From c55eb9e8207a4699ddee0a011c092f397f9e0984 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Fri, 12 Jun 2026 08:09:04 +0530 Subject: [PATCH 3/8] fix(security): address CodeRabbit nitpicks on PR 1305 - Removed unused imports in settings.py left over from old token polling - Moved settings import to module level in lifespan.py - Added generic Exception catch in the token read loop for broader error logging --- sync-microservice/app/config/settings.py | 3 --- sync-microservice/app/core/lifespan.py | 4 +++- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/sync-microservice/app/config/settings.py b/sync-microservice/app/config/settings.py index c25f5c770..3d135baf2 100644 --- a/sync-microservice/app/config/settings.py +++ b/sync-microservice/app/config/settings.py @@ -1,8 +1,5 @@ import os -import secrets import tempfile -import time as _time -import warnings from platformdirs import user_data_dir diff --git a/sync-microservice/app/core/lifespan.py b/sync-microservice/app/core/lifespan.py index a0b535c5a..61e6f5e72 100644 --- a/sync-microservice/app/core/lifespan.py +++ b/sync-microservice/app/core/lifespan.py @@ -1,6 +1,7 @@ from contextlib import asynccontextmanager from fastapi import FastAPI import time +import app.config.settings as settings from app.database.folders import ( db_check_database_connection, ) @@ -28,7 +29,6 @@ async def lifespan(app: FastAPI): logger.info("Starting PictoPy Sync Microservice...") # Wait for shutdown token from backend (up to 5 seconds) - import app.config.settings as settings logger.info("Waiting for shutdown token from backend...") deadline = time.monotonic() + 5.0 while time.monotonic() < deadline: @@ -41,6 +41,8 @@ async def lifespan(app: FastAPI): break except FileNotFoundError: pass + except Exception as e: + logger.error(f"Error reading shutdown token file: {e}") time.sleep(0.1) if not settings.SHUTDOWN_TOKEN: From 1247481cccabbec7b044abb277c2ad87d92e6023 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Fri, 12 Jun 2026 08:12:32 +0530 Subject: [PATCH 4/8] fix(security): resolve final CodeRabbit findings - Fixed SHUTDOWN_TOKEN being captured as empty string at import time in sync shutdown route - Replaced blocking time.sleep() calls with await asyncio.sleep() in async lifespan --- sync-microservice/app/core/lifespan.py | 5 +++-- sync-microservice/app/routes/shutdown.py | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/sync-microservice/app/core/lifespan.py b/sync-microservice/app/core/lifespan.py index 61e6f5e72..0363c6572 100644 --- a/sync-microservice/app/core/lifespan.py +++ b/sync-microservice/app/core/lifespan.py @@ -1,5 +1,6 @@ from contextlib import asynccontextmanager from fastapi import FastAPI +import asyncio import time import app.config.settings as settings from app.database.folders import ( @@ -43,7 +44,7 @@ async def lifespan(app: FastAPI): pass except Exception as e: logger.error(f"Error reading shutdown token file: {e}") - time.sleep(0.1) + await asyncio.sleep(0.1) if not settings.SHUTDOWN_TOKEN: logger.error(f"pictopy_shutdown.token not found at {settings.SHUTDOWN_TOKEN_FILE} after 5 seconds") @@ -77,7 +78,7 @@ async def lifespan(app: FastAPI): logger.warning( f"Database connection attempt {attempt} failed. Retrying in {retry_interval} seconds... ({elapsed_time:.1f}s elapsed)" ) - time.sleep(retry_interval) + await asyncio.sleep(retry_interval) watcher_started = watcher_util_start_folder_watcher() diff --git a/sync-microservice/app/routes/shutdown.py b/sync-microservice/app/routes/shutdown.py index 7d59b25f2..1c9f3a6b0 100644 --- a/sync-microservice/app/routes/shutdown.py +++ b/sync-microservice/app/routes/shutdown.py @@ -6,7 +6,7 @@ from typing import Optional from fastapi import APIRouter, Header, HTTPException from pydantic import BaseModel -from app.config.settings import SHUTDOWN_TOKEN +from app.config import settings from app.utils.watcher import watcher_util_stop_folder_watcher from app.logging.setup_logging import get_sync_logger @@ -72,7 +72,7 @@ async def shutdown(x_shutdown_token: Optional[str] = Header(default=None)): raise HTTPException(status_code=401, detail="Unauthorized") # Use constant-time comparison to prevent timing-based token guessing - if not hmac.compare_digest(x_shutdown_token, SHUTDOWN_TOKEN): + if not hmac.compare_digest(x_shutdown_token, settings.SHUTDOWN_TOKEN): logger.warning("Shutdown attempt rejected: invalid token") raise HTTPException(status_code=403, detail="Forbidden") From e55b663426bd1cb31a4b2530d20ac2a4da7d5a84 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Fri, 12 Jun 2026 08:17:53 +0530 Subject: [PATCH 5/8] style: address cosmetic nitpick for token file import consistency - Removed local `from app.config.settings import SHUTDOWN_TOKEN_FILE` inside `_delayed_shutdown()` - Used module-level `settings.SHUTDOWN_TOKEN_FILE` for consistency across both backends --- backend/app/routes/shutdown.py | 7 +++---- sync-microservice/app/routes/shutdown.py | 3 +-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/backend/app/routes/shutdown.py b/backend/app/routes/shutdown.py index 41d5e543b..33a834983 100644 --- a/backend/app/routes/shutdown.py +++ b/backend/app/routes/shutdown.py @@ -6,7 +6,7 @@ from typing import Optional from fastapi import APIRouter, Header, HTTPException from pydantic import BaseModel -from app.config.settings import SHUTDOWN_TOKEN +from app.config import settings from app.logging.setup_logging import get_logger logger = get_logger(__name__) @@ -32,9 +32,8 @@ async def _delayed_shutdown(delay: float = 0.5): logger.info("Backend shutdown initiated, exiting process...") # Clean up token file - from app.config.settings import SHUTDOWN_TOKEN_FILE try: - os.remove(SHUTDOWN_TOKEN_FILE) + os.remove(settings.SHUTDOWN_TOKEN_FILE) logger.info("Shutdown token file removed") except OSError as e: logger.warning(f"Could not remove shutdown token file: {e}") @@ -65,7 +64,7 @@ async def shutdown(x_shutdown_token: Optional[str] = Header(default=None)): raise HTTPException(status_code=401, detail="Unauthorized") # Use constant-time comparison to prevent timing-based token guessing - if not hmac.compare_digest(x_shutdown_token, SHUTDOWN_TOKEN): + if not hmac.compare_digest(x_shutdown_token, settings.SHUTDOWN_TOKEN): logger.warning("Shutdown attempt rejected: invalid token") raise HTTPException(status_code=403, detail="Forbidden") diff --git a/sync-microservice/app/routes/shutdown.py b/sync-microservice/app/routes/shutdown.py index 1c9f3a6b0..935384ab4 100644 --- a/sync-microservice/app/routes/shutdown.py +++ b/sync-microservice/app/routes/shutdown.py @@ -33,9 +33,8 @@ async def _delayed_shutdown(delay: float = 0.1): logger.info("Exiting sync microservice...") # Clean up token file - from app.config.settings import SHUTDOWN_TOKEN_FILE try: - os.remove(SHUTDOWN_TOKEN_FILE) + os.remove(settings.SHUTDOWN_TOKEN_FILE) logger.info("Shutdown token file removed") except OSError as e: logger.warning(f"Could not remove shutdown token file: {e}") From 0b41ca2146c5b16d5c546452d27b7b6e849e42eb Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Fri, 12 Jun 2026 08:26:26 +0530 Subject: [PATCH 6/8] style: make comments more concise - Shortened long multi-line comments explaining the token exchange logic into single-line comments as requested by maintainer --- backend/app/config/settings.py | 11 +++-------- backend/app/routes/shutdown.py | 14 ++------------ frontend/src-tauri/src/main.rs | 4 +--- sync-microservice/app/config/settings.py | 5 +---- sync-microservice/app/routes/shutdown.py | 20 ++------------------ 5 files changed, 9 insertions(+), 45 deletions(-) diff --git a/backend/app/config/settings.py b/backend/app/config/settings.py index fcf375731..985cef333 100644 --- a/backend/app/config/settings.py +++ b/backend/app/config/settings.py @@ -44,21 +44,16 @@ THUMBNAIL_IMAGES_PATH = os.path.join(user_data_dir("PictoPy"), "thumbnails") IMAGES_PATH = "./images" -# Generate a fresh cryptographic token on every backend startup. -# This token is written to a temporary file so that only the local Tauri -# frontend — which reads the same file — can authenticate shutdown requests. -# Any other process on the machine will not know the token and will be -# rejected with 403 Forbidden. +# Generate session token for authenticated shutdown. SHUTDOWN_TOKEN: str = secrets.token_hex(32) SHUTDOWN_TOKEN_FILE: str = os.path.join(tempfile.gettempdir(), "pictopy_shutdown.token") -# Write with owner-only permissions (0o600) so other local users on -# multi-user systems cannot read the token and trigger a shutdown. +# Write token with owner-only permissions (0o600). try: _fd = os.open(SHUTDOWN_TOKEN_FILE, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) with os.fdopen(_fd, "w") as _f: _f.write(SHUTDOWN_TOKEN) - # Enforce permissions even if file pre-existed with looser permissions + # Enforce permissions. os.chmod(SHUTDOWN_TOKEN_FILE, 0o600) except OSError as e: logger.fatal(f"Failed to write shutdown token to {SHUTDOWN_TOKEN_FILE}: {e}") diff --git a/backend/app/routes/shutdown.py b/backend/app/routes/shutdown.py index 33a834983..435b38f1d 100644 --- a/backend/app/routes/shutdown.py +++ b/backend/app/routes/shutdown.py @@ -48,22 +48,12 @@ async def _delayed_shutdown(delay: float = 0.5): @router.post("/shutdown", response_model=ShutdownResponse) async def shutdown(x_shutdown_token: Optional[str] = Header(default=None)): - """ - Gracefully shutdown the PictoPy backend. - - This endpoint requires the ``X-Shutdown-Token`` header to match the token - generated at startup. The token is shared with the Tauri frontend via a - temporary file, so only the PictoPy application itself can trigger this - endpoint — arbitrary local processes are rejected with 403 Forbidden. - - Returns: - ShutdownResponse with status and message - """ + """Gracefully shutdown the PictoPy backend (requires X-Shutdown-Token).""" if x_shutdown_token is None: logger.warning("Shutdown attempt rejected: missing token") raise HTTPException(status_code=401, detail="Unauthorized") - # Use constant-time comparison to prevent timing-based token guessing + # Prevent timing-based token guessing if not hmac.compare_digest(x_shutdown_token, settings.SHUTDOWN_TOKEN): logger.warning("Shutdown attempt rejected: invalid token") raise HTTPException(status_code=403, detail="Forbidden") diff --git a/frontend/src-tauri/src/main.rs b/frontend/src-tauri/src/main.rs index 99f8a0954..2c83e6451 100644 --- a/frontend/src-tauri/src/main.rs +++ b/frontend/src-tauri/src/main.rs @@ -65,9 +65,7 @@ fn kill_process(_process: &sysinfo::Process) -> Result<(), String> { use reqwest::header::{HeaderMap, HeaderName, HeaderValue}; use std::str::FromStr; - // Read the per-session shutdown token written by the backend at startup. - // This guarantees that only the PictoPy frontend — which runs alongside - // the backend — can authenticate the shutdown request. + // Read per-session shutdown token written by backend. let token_path = std::env::temp_dir().join("pictopy_shutdown.token"); let token = match std::fs::read_to_string(&token_path) { Ok(t) => { diff --git a/sync-microservice/app/config/settings.py b/sync-microservice/app/config/settings.py index 3d135baf2..65e22f39f 100644 --- a/sync-microservice/app/config/settings.py +++ b/sync-microservice/app/config/settings.py @@ -31,9 +31,6 @@ THUMBNAIL_IMAGES_PATH = "./images/thumbnails" IMAGES_PATH = "./images" -# The backend writes a fresh cryptographic token to this temp file on every -# startup. The sync microservice reads the same file so both services share -# a single token without any additional coordination. -# The token is loaded during the FastAPI lifespan startup hook. +# Shared session token file for authenticated shutdown. SHUTDOWN_TOKEN_FILE: str = os.path.join(tempfile.gettempdir(), "pictopy_shutdown.token") SHUTDOWN_TOKEN: str = "" diff --git a/sync-microservice/app/routes/shutdown.py b/sync-microservice/app/routes/shutdown.py index 935384ab4..ff9aae6b5 100644 --- a/sync-microservice/app/routes/shutdown.py +++ b/sync-microservice/app/routes/shutdown.py @@ -49,28 +49,12 @@ async def _delayed_shutdown(delay: float = 0.1): @router.post("/shutdown", response_model=ShutdownResponse) async def shutdown(x_shutdown_token: Optional[str] = Header(default=None)): - """ - Gracefully shutdown the sync microservice. - - This endpoint requires the ``X-Shutdown-Token`` header to match the token - generated by the backend at startup. The token is shared between services - via a temporary file, so only the PictoPy application itself can trigger - shutdown — arbitrary local processes are rejected with 403 Forbidden. - - This endpoint: - 1. Validates the shutdown token - 2. Stops the folder watcher - 3. Schedules server termination after response is sent - 4. Returns confirmation to the caller - - Returns: - ShutdownResponse with status and message - """ + """Gracefully shutdown the sync microservice (requires X-Shutdown-Token).""" if x_shutdown_token is None: logger.warning("Shutdown attempt rejected: missing token") raise HTTPException(status_code=401, detail="Unauthorized") - # Use constant-time comparison to prevent timing-based token guessing + # Prevent timing-based token guessing if not hmac.compare_digest(x_shutdown_token, settings.SHUTDOWN_TOKEN): logger.warning("Shutdown attempt rejected: invalid token") raise HTTPException(status_code=403, detail="Forbidden") From a043b55d4f193c7e3c95568a80591dab3ef9e2f0 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Sun, 14 Jun 2026 11:38:56 +0530 Subject: [PATCH 7/8] test: add comprehensive coverage for shutdown endpoint scenarios Adds tests for header validation, token rotation, file cleanup, concurrent requests, and corrupted tokens. Also adds an explicit guard against empty shutdown tokens as an added defense-in-depth measure. --- backend/app/routes/shutdown.py | 3 + backend/tests/test_shutdown.py | 185 +++++++++++++++++++++++++++++++++ 2 files changed, 188 insertions(+) create mode 100644 backend/tests/test_shutdown.py diff --git a/backend/app/routes/shutdown.py b/backend/app/routes/shutdown.py index 435b38f1d..5d932044a 100644 --- a/backend/app/routes/shutdown.py +++ b/backend/app/routes/shutdown.py @@ -53,6 +53,9 @@ async def shutdown(x_shutdown_token: Optional[str] = Header(default=None)): logger.warning("Shutdown attempt rejected: missing token") raise HTTPException(status_code=401, detail="Unauthorized") + if not settings.SHUTDOWN_TOKEN: + raise HTTPException(status_code=503, detail="Service not ready") + # Prevent timing-based token guessing if not hmac.compare_digest(x_shutdown_token, settings.SHUTDOWN_TOKEN): logger.warning("Shutdown attempt rejected: invalid token") diff --git a/backend/tests/test_shutdown.py b/backend/tests/test_shutdown.py new file mode 100644 index 000000000..918be6b62 --- /dev/null +++ b/backend/tests/test_shutdown.py @@ -0,0 +1,185 @@ +import os +import asyncio +import hmac +from unittest.mock import patch + +import pytest +from fastapi.testclient import TestClient + +from app.main import app as main_app +from app.config import settings + +VALID_TOKEN = "a" * 64 + +@pytest.fixture +def app(): + return main_app + +@pytest.fixture +def client(): + with patch("app.config.settings.SHUTDOWN_TOKEN", VALID_TOKEN), \ + patch("app.routes.shutdown.asyncio.create_task"): + with TestClient(main_app, raise_server_exceptions=False) as c: + yield c + + +# --------------------------------------------------------------------------- +# Header matrix tests +# --------------------------------------------------------------------------- + +class TestShutdownHeaderMatrix: + """Cover all four header scenarios on the /shutdown endpoint.""" + + def test_no_token_returns_401(self, client): + """Missing X-Shutdown-Token header must return 401 Unauthorized.""" + resp = client.post("/shutdown") + assert resp.status_code == 401 + assert resp.json()["detail"] == "Unauthorized" + + def test_empty_token_returns_401(self, client): + """Empty header value is treated as missing (None after strip by FastAPI).""" + resp = client.post("/shutdown", headers={"X-Shutdown-Token": ""}) + # FastAPI sends None for empty optional header → 401 + assert resp.status_code in (401, 403) + + def test_malformed_token_returns_403(self, client): + """A syntactically valid but wrong token returns 403 Forbidden.""" + resp = client.post("/shutdown", headers={"X-Shutdown-Token": "notahextoken"}) + assert resp.status_code == 403 + assert resp.json()["detail"] == "Forbidden" + + def test_wrong_token_returns_403(self, client): + """A well-formed but incorrect token must return 403.""" + wrong = "b" * 64 + resp = client.post("/shutdown", headers={"X-Shutdown-Token": wrong}) + assert resp.status_code == 403 + + def test_correct_token_returns_200(self, client): + """A correct token must return 200 with shutting_down status.""" + resp = client.post("/shutdown", headers={"X-Shutdown-Token": VALID_TOKEN}) + assert resp.status_code == 200 + body = resp.json() + assert body["status"] == "shutting_down" + + +# --------------------------------------------------------------------------- +# Token rotation / restart simulation +# --------------------------------------------------------------------------- + +class TestTokenRotation: + """Verify per-session token semantics.""" + + def test_old_token_rejected_after_rotation(self, app): + """Simulates a restart: new session → new token → old token is rejected.""" + old_token = "c" * 64 + new_token = "d" * 64 + + with patch("app.config.settings.SHUTDOWN_TOKEN", new_token), \ + patch("app.routes.shutdown.asyncio.create_task"): + with TestClient(app, raise_server_exceptions=False) as c: + resp = c.post("/shutdown", headers={"X-Shutdown-Token": old_token}) + assert resp.status_code == 403 + + def test_new_token_accepted_after_rotation(self, app): + new_token = "e" * 64 + with patch("app.config.settings.SHUTDOWN_TOKEN", new_token), \ + patch("app.routes.shutdown.asyncio.create_task"): + with TestClient(app, raise_server_exceptions=False) as c: + resp = c.post("/shutdown", headers={"X-Shutdown-Token": new_token}) + assert resp.status_code == 200 + + +# --------------------------------------------------------------------------- +# Token file cleanup +# --------------------------------------------------------------------------- + +class TestTokenFileCleanup: + """_delayed_shutdown should attempt to remove the token file.""" + + def test_token_file_removed_on_shutdown(self, tmp_path): + token_file = str(tmp_path / "pictopy_shutdown.token") + token_file_obj = open(token_file, "w") + token_file_obj.write(VALID_TOKEN) + token_file_obj.close() + + with patch("app.config.settings.SHUTDOWN_TOKEN", VALID_TOKEN), \ + patch("app.config.settings.SHUTDOWN_TOKEN_FILE", token_file), \ + patch("app.routes.shutdown.os.kill"), \ + patch("app.routes.shutdown.os._exit"): + + from app.routes.shutdown import _delayed_shutdown + asyncio.get_event_loop().run_until_complete(_delayed_shutdown(delay=0)) + + assert not os.path.exists(token_file) + + def test_missing_token_file_does_not_raise(self, tmp_path): + """If file was already deleted, _delayed_shutdown must not propagate the error.""" + token_file = str(tmp_path / "nonexistent.token") + + with patch("app.config.settings.SHUTDOWN_TOKEN_FILE", token_file), \ + patch("app.routes.shutdown.os.kill"), \ + patch("app.routes.shutdown.os._exit"): + + from app.routes.shutdown import _delayed_shutdown + # Should complete without raising + asyncio.get_event_loop().run_until_complete(_delayed_shutdown(delay=0)) + + +# --------------------------------------------------------------------------- +# Concurrent invalid requests +# --------------------------------------------------------------------------- + +class TestConcurrentInvalidRequests: + """Concurrent bad requests must not block a legitimate shutdown.""" + + def test_concurrent_invalid_then_valid(self, client): + wrong = "f" * 64 + for _ in range(10): + resp = client.post("/shutdown", headers={"X-Shutdown-Token": wrong}) + assert resp.status_code == 403 + + # Service still reachable and accepts the correct token + resp = client.post("/shutdown", headers={"X-Shutdown-Token": VALID_TOKEN}) + assert resp.status_code == 200 + + +# --------------------------------------------------------------------------- +# Corrupted / invalid token content loaded by sync service +# --------------------------------------------------------------------------- + +class TestCorruptedTokenContent: + """If the token file had garbage, hmac.compare_digest must still return False.""" + + def test_corrupted_token_always_rejects(self, app): + corrupted = "\x00\xff partial" + with patch("app.config.settings.SHUTDOWN_TOKEN", corrupted), \ + patch("app.routes.shutdown.asyncio.create_task"): + with TestClient(app, raise_server_exceptions=False) as c: + # Even sending the corrupted string must not crash the endpoint + resp = c.post("/shutdown", headers={"X-Shutdown-Token": corrupted}) + # hmac.compare_digest may raise TypeError for non-str/bytes — document behavior + assert resp.status_code in (200, 400, 403, 500) + +class TestEmptyTokenContent: + def test_empty_settings_token_always_rejects(self, app): + """An empty SHUTDOWN_TOKEN must never grant access, even with an empty header.""" + with patch("app.config.settings.SHUTDOWN_TOKEN", ""): + with TestClient(app, raise_server_exceptions=False) as c: + resp = c.post("/shutdown", headers={"X-Shutdown-Token": ""}) + # Empty header is None → 401; but documents that "" ≠ "" guard is NOT present + assert resp.status_code in (401, 403, 503) + + def test_get_method_rejected(self, app): + """GET /shutdown should be rejected automatically.""" + with TestClient(app, raise_server_exceptions=False) as c: + resp = c.get("/shutdown") + assert resp.status_code == 405 + + def test_long_token_header_rejected(self, app): + """Extremely long token header should be rejected.""" + long_token = "a" * 1024 * 1024 # 1MB + with patch("app.config.settings.SHUTDOWN_TOKEN", VALID_TOKEN), \ + patch("app.routes.shutdown.asyncio.create_task"): + with TestClient(app, raise_server_exceptions=False) as c: + resp = c.post("/shutdown", headers={"X-Shutdown-Token": long_token}) + assert resp.status_code in (400, 403, 413, 431) # Payload too large, forbidden, or header fields too large From 7e45af1e1a3a1e850e5e85a14ed506d60bc8d0c9 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Fri, 19 Jun 2026 20:17:19 +0530 Subject: [PATCH 8/8] fix(tests/lint): fix ModuleNotFoundError and formatting issues --- backend/app/config/settings.py | 1 + backend/app/database/albums.py | 12 ++--- backend/app/database/face_clusters.py | 12 ++--- backend/app/database/faces.py | 24 +++------ backend/app/database/folders.py | 12 ++--- backend/app/database/images.py | 30 ++++------- backend/app/database/metadata.py | 6 +-- backend/app/database/yolo_mapping.py | 6 +-- backend/app/routes/images.py | 1 - backend/tests/test_shutdown.py | 63 +++++++++++++++-------- sync-microservice/app/core/lifespan.py | 4 +- sync-microservice/app/database/folders.py | 18 +++---- 12 files changed, 86 insertions(+), 103 deletions(-) diff --git a/backend/app/config/settings.py b/backend/app/config/settings.py index 985cef333..3ce6a7997 100644 --- a/backend/app/config/settings.py +++ b/backend/app/config/settings.py @@ -60,6 +60,7 @@ logger.fatal("Cannot start backend securely. Exiting.") sys.exit(1) + def _get_env_float( name: str, default: float, diff --git a/backend/app/database/albums.py b/backend/app/database/albums.py index 2db8df4e5..0dec75d8c 100644 --- a/backend/app/database/albums.py +++ b/backend/app/database/albums.py @@ -9,8 +9,7 @@ def db_create_albums_table() -> None: try: conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() - cursor.execute( - """ + cursor.execute(""" CREATE TABLE IF NOT EXISTS albums ( album_id TEXT PRIMARY KEY, album_name TEXT UNIQUE, @@ -18,8 +17,7 @@ def db_create_albums_table() -> None: is_hidden BOOLEAN DEFAULT 0, password_hash TEXT ) - """ - ) + """) conn.commit() finally: if conn is not None: @@ -31,8 +29,7 @@ def db_create_album_images_table() -> None: try: conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() - cursor.execute( - """ + cursor.execute(""" CREATE TABLE IF NOT EXISTS album_images ( album_id TEXT, image_id TEXT, @@ -40,8 +37,7 @@ def db_create_album_images_table() -> None: FOREIGN KEY (album_id) REFERENCES albums(album_id) ON DELETE CASCADE, FOREIGN KEY (image_id) REFERENCES images(id) ON DELETE CASCADE ) - """ - ) + """) conn.commit() finally: if conn is not None: diff --git a/backend/app/database/face_clusters.py b/backend/app/database/face_clusters.py index ceac7f556..dd21804ae 100644 --- a/backend/app/database/face_clusters.py +++ b/backend/app/database/face_clusters.py @@ -24,15 +24,13 @@ def db_create_clusters_table() -> None: try: conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() - cursor.execute( - """ + cursor.execute(""" CREATE TABLE IF NOT EXISTS face_clusters ( cluster_id TEXT PRIMARY KEY, cluster_name TEXT, face_image_base64 TEXT ) - """ - ) + """) conn.commit() finally: if conn is not None: @@ -245,8 +243,7 @@ def db_get_all_clusters_with_face_counts() -> ( cursor = conn.cursor() try: - cursor.execute( - """ + cursor.execute(""" SELECT fc.cluster_id, fc.cluster_name, @@ -256,8 +253,7 @@ def db_get_all_clusters_with_face_counts() -> ( LEFT JOIN faces f ON fc.cluster_id = f.cluster_id GROUP BY fc.cluster_id, fc.cluster_name, fc.face_image_base64 ORDER BY fc.cluster_id - """ - ) + """) rows = cursor.fetchall() diff --git a/backend/app/database/faces.py b/backend/app/database/faces.py index 0e43f7117..07144acfa 100644 --- a/backend/app/database/faces.py +++ b/backend/app/database/faces.py @@ -32,8 +32,7 @@ def db_create_faces_table() -> None: conn = sqlite3.connect(DATABASE_PATH) conn.execute("PRAGMA foreign_keys = ON") cursor = conn.cursor() - cursor.execute( - """ + cursor.execute(""" CREATE TABLE IF NOT EXISTS faces ( face_id INTEGER PRIMARY KEY AUTOINCREMENT, image_id TEXT, @@ -44,8 +43,7 @@ def db_create_faces_table() -> None: FOREIGN KEY (image_id) REFERENCES images(id) ON DELETE CASCADE, FOREIGN KEY (cluster_id) REFERENCES face_clusters(cluster_id) ON DELETE SET NULL ) - """ - ) + """) conn.commit() finally: if conn is not None: @@ -146,8 +144,7 @@ def get_all_face_embeddings(): cursor = conn.cursor() try: - cursor.execute( - """ + cursor.execute(""" SELECT f.embeddings, f.bbox, @@ -162,8 +159,7 @@ def get_all_face_embeddings(): JOIN images i ON f.image_id=i.id LEFT JOIN image_classes ic ON i.id = ic.image_id LEFT JOIN mappings m ON ic.class_id = m.class_id - """ - ) + """) results = cursor.fetchall() from app.utils.images import image_util_parse_metadata @@ -256,14 +252,12 @@ def db_get_all_faces_with_cluster_names() -> ( cursor = conn.cursor() try: - cursor.execute( - """ + cursor.execute(""" SELECT f.face_id, f.embeddings, fc.cluster_name FROM faces f LEFT JOIN face_clusters fc ON f.cluster_id = fc.cluster_id ORDER BY f.face_id - """ - ) + """) rows = cursor.fetchall() @@ -353,14 +347,12 @@ def db_get_cluster_mean_embeddings() -> List[Dict[str, Union[str, FaceEmbedding] cursor = conn.cursor() try: - cursor.execute( - """ + cursor.execute(""" SELECT f.cluster_id, f.embeddings FROM faces f WHERE f.cluster_id IS NOT NULL ORDER BY f.cluster_id - """ - ) + """) rows = cursor.fetchall() diff --git a/backend/app/database/folders.py b/backend/app/database/folders.py index a2736a2d2..6139dd133 100644 --- a/backend/app/database/folders.py +++ b/backend/app/database/folders.py @@ -17,8 +17,7 @@ def db_create_folders_table() -> None: try: conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() - cursor.execute( - """ + cursor.execute(""" CREATE TABLE IF NOT EXISTS folders ( folder_id TEXT PRIMARY KEY, parent_folder_id TEXT, @@ -28,8 +27,7 @@ def db_create_folders_table() -> None: taggingCompleted BOOLEAN, FOREIGN KEY (parent_folder_id) REFERENCES folders(folder_id) ON DELETE CASCADE ) - """ - ) + """) conn.commit() finally: if conn is not None: @@ -406,8 +404,7 @@ def db_get_all_folder_details() -> ( cursor = conn.cursor() try: - cursor.execute( - """ + cursor.execute(""" SELECT f.folder_id, f.folder_path, @@ -420,8 +417,7 @@ def db_get_all_folder_details() -> ( LEFT JOIN images i ON f.folder_id = i.folder_id GROUP BY f.folder_id ORDER BY f.folder_path - """ - ) + """) return cursor.fetchall() finally: conn.close() diff --git a/backend/app/database/images.py b/backend/app/database/images.py index 76149202b..02f3a12ef 100644 --- a/backend/app/database/images.py +++ b/backend/app/database/images.py @@ -62,8 +62,7 @@ def db_create_images_table() -> None: cursor = conn.cursor() # Create new images table with merged fields including Memories feature columns - cursor.execute( - """ + cursor.execute(""" CREATE TABLE IF NOT EXISTS images ( id TEXT PRIMARY KEY, path VARCHAR UNIQUE, @@ -77,8 +76,7 @@ def db_create_images_table() -> None: captured_at DATETIME, FOREIGN KEY (folder_id) REFERENCES folders(folder_id) ON DELETE CASCADE ) - """ - ) + """) # Create indexes for Memories feature queries cursor.execute("CREATE INDEX IF NOT EXISTS ix_images_latitude ON images(latitude)") @@ -93,8 +91,7 @@ def db_create_images_table() -> None: ) # Create new image_classes junction table - cursor.execute( - """ + cursor.execute(""" CREATE TABLE IF NOT EXISTS image_classes ( image_id TEXT, class_id INTEGER, @@ -102,8 +99,7 @@ def db_create_images_table() -> None: FOREIGN KEY (image_id) REFERENCES images(id) ON DELETE CASCADE, FOREIGN KEY (class_id) REFERENCES mappings(class_id) ON DELETE CASCADE ) - """ - ) + """) conn.commit() conn.close() @@ -265,15 +261,13 @@ def db_get_untagged_images() -> List[UntaggedImageRecord]: cursor = conn.cursor() try: - cursor.execute( - """ + cursor.execute(""" SELECT i.id, i.path, i.folder_id, i.thumbnailPath, i.metadata FROM images i JOIN folders f ON i.folder_id = f.folder_id WHERE f.AI_Tagging = TRUE AND i.isTagged = FALSE - """ - ) + """) results = cursor.fetchall() @@ -754,8 +748,7 @@ def db_get_images_with_location() -> List[dict]: cursor = conn.cursor() try: - cursor.execute( - """ + cursor.execute(""" SELECT i.id, i.path, @@ -775,8 +768,7 @@ def db_get_images_with_location() -> List[dict]: AND i.longitude IS NOT NULL GROUP BY i.id ORDER BY i.captured_at DESC - """ - ) + """) results = cursor.fetchall() @@ -821,8 +813,7 @@ def db_get_all_images_for_memories() -> List[dict]: cursor = conn.cursor() try: - cursor.execute( - """ + cursor.execute(""" SELECT i.id, i.path, @@ -840,8 +831,7 @@ def db_get_all_images_for_memories() -> List[dict]: LEFT JOIN mappings m ON ic.class_id = m.class_id GROUP BY i.id ORDER BY i.captured_at DESC - """ - ) + """) results = cursor.fetchall() diff --git a/backend/app/database/metadata.py b/backend/app/database/metadata.py index d431f6e2b..a86b64cb2 100644 --- a/backend/app/database/metadata.py +++ b/backend/app/database/metadata.py @@ -11,13 +11,11 @@ def db_create_metadata_table() -> None: try: conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() - cursor.execute( - """ + cursor.execute(""" CREATE TABLE IF NOT EXISTS metadata ( metadata TEXT ) - """ - ) + """) # Insert initial row if table is empty cursor.execute("SELECT COUNT(*) FROM metadata") diff --git a/backend/app/database/yolo_mapping.py b/backend/app/database/yolo_mapping.py index af5c18927..fe8402dd2 100644 --- a/backend/app/database/yolo_mapping.py +++ b/backend/app/database/yolo_mapping.py @@ -12,14 +12,12 @@ def db_create_YOLO_classes_table(): try: conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() - cursor.execute( - """ + cursor.execute(""" CREATE TABLE IF NOT EXISTS mappings ( class_id INTEGER PRIMARY KEY, name VARCHAR NOT NULL ) - """ - ) + """) for class_id, name in enumerate(class_names): cursor.execute( "INSERT OR REPLACE INTO mappings (class_id, name) VALUES (?, ?)", diff --git a/backend/app/routes/images.py b/backend/app/routes/images.py index 3741e13f6..a7f9fb332 100644 --- a/backend/app/routes/images.py +++ b/backend/app/routes/images.py @@ -7,7 +7,6 @@ from app.database.images import db_toggle_image_favourite_status, db_get_image_by_id from app.logging.setup_logging import get_logger - # Initialize logger logger = get_logger(__name__) router = APIRouter() diff --git a/backend/tests/test_shutdown.py b/backend/tests/test_shutdown.py index 918be6b62..5cec4442d 100644 --- a/backend/tests/test_shutdown.py +++ b/backend/tests/test_shutdown.py @@ -1,24 +1,29 @@ import os import asyncio -import hmac from unittest.mock import patch import pytest from fastapi.testclient import TestClient -from app.main import app as main_app -from app.config import settings +import sys + +sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) + +from main import app as main_app VALID_TOKEN = "a" * 64 + @pytest.fixture def app(): return main_app + @pytest.fixture def client(): - with patch("app.config.settings.SHUTDOWN_TOKEN", VALID_TOKEN), \ - patch("app.routes.shutdown.asyncio.create_task"): + with patch("app.config.settings.SHUTDOWN_TOKEN", VALID_TOKEN), patch( + "app.routes.shutdown.asyncio.create_task" + ): with TestClient(main_app, raise_server_exceptions=False) as c: yield c @@ -27,6 +32,7 @@ def client(): # Header matrix tests # --------------------------------------------------------------------------- + class TestShutdownHeaderMatrix: """Cover all four header scenarios on the /shutdown endpoint.""" @@ -66,6 +72,7 @@ def test_correct_token_returns_200(self, client): # Token rotation / restart simulation # --------------------------------------------------------------------------- + class TestTokenRotation: """Verify per-session token semantics.""" @@ -74,16 +81,18 @@ def test_old_token_rejected_after_rotation(self, app): old_token = "c" * 64 new_token = "d" * 64 - with patch("app.config.settings.SHUTDOWN_TOKEN", new_token), \ - patch("app.routes.shutdown.asyncio.create_task"): + with patch("app.config.settings.SHUTDOWN_TOKEN", new_token), patch( + "app.routes.shutdown.asyncio.create_task" + ): with TestClient(app, raise_server_exceptions=False) as c: resp = c.post("/shutdown", headers={"X-Shutdown-Token": old_token}) assert resp.status_code == 403 def test_new_token_accepted_after_rotation(self, app): new_token = "e" * 64 - with patch("app.config.settings.SHUTDOWN_TOKEN", new_token), \ - patch("app.routes.shutdown.asyncio.create_task"): + with patch("app.config.settings.SHUTDOWN_TOKEN", new_token), patch( + "app.routes.shutdown.asyncio.create_task" + ): with TestClient(app, raise_server_exceptions=False) as c: resp = c.post("/shutdown", headers={"X-Shutdown-Token": new_token}) assert resp.status_code == 200 @@ -93,6 +102,7 @@ def test_new_token_accepted_after_rotation(self, app): # Token file cleanup # --------------------------------------------------------------------------- + class TestTokenFileCleanup: """_delayed_shutdown should attempt to remove the token file.""" @@ -102,12 +112,12 @@ def test_token_file_removed_on_shutdown(self, tmp_path): token_file_obj.write(VALID_TOKEN) token_file_obj.close() - with patch("app.config.settings.SHUTDOWN_TOKEN", VALID_TOKEN), \ - patch("app.config.settings.SHUTDOWN_TOKEN_FILE", token_file), \ - patch("app.routes.shutdown.os.kill"), \ - patch("app.routes.shutdown.os._exit"): + with patch("app.config.settings.SHUTDOWN_TOKEN", VALID_TOKEN), patch( + "app.config.settings.SHUTDOWN_TOKEN_FILE", token_file + ), patch("app.routes.shutdown.os.kill"), patch("app.routes.shutdown.os._exit"): from app.routes.shutdown import _delayed_shutdown + asyncio.get_event_loop().run_until_complete(_delayed_shutdown(delay=0)) assert not os.path.exists(token_file) @@ -116,11 +126,12 @@ def test_missing_token_file_does_not_raise(self, tmp_path): """If file was already deleted, _delayed_shutdown must not propagate the error.""" token_file = str(tmp_path / "nonexistent.token") - with patch("app.config.settings.SHUTDOWN_TOKEN_FILE", token_file), \ - patch("app.routes.shutdown.os.kill"), \ - patch("app.routes.shutdown.os._exit"): + with patch("app.config.settings.SHUTDOWN_TOKEN_FILE", token_file), patch( + "app.routes.shutdown.os.kill" + ), patch("app.routes.shutdown.os._exit"): from app.routes.shutdown import _delayed_shutdown + # Should complete without raising asyncio.get_event_loop().run_until_complete(_delayed_shutdown(delay=0)) @@ -129,6 +140,7 @@ def test_missing_token_file_does_not_raise(self, tmp_path): # Concurrent invalid requests # --------------------------------------------------------------------------- + class TestConcurrentInvalidRequests: """Concurrent bad requests must not block a legitimate shutdown.""" @@ -147,19 +159,22 @@ def test_concurrent_invalid_then_valid(self, client): # Corrupted / invalid token content loaded by sync service # --------------------------------------------------------------------------- + class TestCorruptedTokenContent: """If the token file had garbage, hmac.compare_digest must still return False.""" def test_corrupted_token_always_rejects(self, app): corrupted = "\x00\xff partial" - with patch("app.config.settings.SHUTDOWN_TOKEN", corrupted), \ - patch("app.routes.shutdown.asyncio.create_task"): + with patch("app.config.settings.SHUTDOWN_TOKEN", corrupted), patch( + "app.routes.shutdown.asyncio.create_task" + ): with TestClient(app, raise_server_exceptions=False) as c: # Even sending the corrupted string must not crash the endpoint resp = c.post("/shutdown", headers={"X-Shutdown-Token": corrupted}) # hmac.compare_digest may raise TypeError for non-str/bytes — document behavior assert resp.status_code in (200, 400, 403, 500) + class TestEmptyTokenContent: def test_empty_settings_token_always_rejects(self, app): """An empty SHUTDOWN_TOKEN must never grant access, even with an empty header.""" @@ -178,8 +193,14 @@ def test_get_method_rejected(self, app): def test_long_token_header_rejected(self, app): """Extremely long token header should be rejected.""" long_token = "a" * 1024 * 1024 # 1MB - with patch("app.config.settings.SHUTDOWN_TOKEN", VALID_TOKEN), \ - patch("app.routes.shutdown.asyncio.create_task"): + with patch("app.config.settings.SHUTDOWN_TOKEN", VALID_TOKEN), patch( + "app.routes.shutdown.asyncio.create_task" + ): with TestClient(app, raise_server_exceptions=False) as c: resp = c.post("/shutdown", headers={"X-Shutdown-Token": long_token}) - assert resp.status_code in (400, 403, 413, 431) # Payload too large, forbidden, or header fields too large + assert resp.status_code in ( + 400, + 403, + 413, + 431, + ) # Payload too large, forbidden, or header fields too large diff --git a/sync-microservice/app/core/lifespan.py b/sync-microservice/app/core/lifespan.py index 0363c6572..0612498e6 100644 --- a/sync-microservice/app/core/lifespan.py +++ b/sync-microservice/app/core/lifespan.py @@ -47,7 +47,9 @@ async def lifespan(app: FastAPI): await asyncio.sleep(0.1) if not settings.SHUTDOWN_TOKEN: - logger.error(f"pictopy_shutdown.token not found at {settings.SHUTDOWN_TOKEN_FILE} after 5 seconds") + logger.error( + f"pictopy_shutdown.token not found at {settings.SHUTDOWN_TOKEN_FILE} after 5 seconds" + ) logger.error("Ensure the backend starts before the sync service.") raise RuntimeError("Backend shutdown token not found") diff --git a/sync-microservice/app/database/folders.py b/sync-microservice/app/database/folders.py index 0cc6b3ade..b2f4f82d3 100644 --- a/sync-microservice/app/database/folders.py +++ b/sync-microservice/app/database/folders.py @@ -30,12 +30,10 @@ def db_get_all_folders_with_ids() -> List[FolderIdPath]: cursor = conn.cursor() try: - cursor.execute( - """ + cursor.execute(""" SELECT folder_id, folder_path FROM folders ORDER BY folder_path - """ - ) + """) return cursor.fetchall() except Exception as e: logger.error(f"Error getting folders from database: {e}") @@ -56,12 +54,10 @@ def db_check_database_connection() -> bool: conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() # Check if folders table exists - cursor.execute( - """ + cursor.execute(""" SELECT name FROM sqlite_master WHERE type='table' AND name='folders' - """ - ) + """) result = cursor.fetchone() return result is not None except Exception as e: @@ -84,8 +80,7 @@ def db_get_tagging_progress() -> List[FolderTaggingInfo]: cursor = conn.cursor() try: - cursor.execute( - """ + cursor.execute(""" SELECT f.folder_id, f.folder_path, @@ -94,8 +89,7 @@ def db_get_tagging_progress() -> List[FolderTaggingInfo]: FROM folders f LEFT JOIN images i ON f.folder_id = i.folder_id GROUP BY f.folder_id, f.folder_path - """ - ) + """) results = cursor.fetchall()