From 5b7a55a05d55a142f9376474cbaea828d112759d Mon Sep 17 00:00:00 2001 From: Ethanfel Date: Thu, 16 Apr 2026 14:10:27 +0200 Subject: [PATCH] fix: second-pass review bugs in server and core - ExportRunner: stop batch on first error (was continuing, overwriting error status with done) - Export route: validate input_path against MEDIA_DIRS - Export route: validate encoder, portrait_ratio, folder_suffix, name - Export route: fix format check for WebP sequence - Export route: add _ separator in folder_suffix (match GUI) - Export route: use realpath consistently in delete endpoint - Export route: drop runner ref on completion (prevent memory leak) - ProcessedDB: use cursor-level row_factory (thread-safe) - WebSocket: catch all exceptions in connect, cleanup in finally - Dockerfile: use uvicorn[standard] for websockets support Co-Authored-By: Claude Opus 4.6 --- Dockerfile | 2 +- core/db.py | 6 ++--- core/export.py | 1 + server/routes/export.py | 53 +++++++++++++++++++++++++++++++---------- server/ws.py | 4 +++- 5 files changed, 49 insertions(+), 17 deletions(-) diff --git a/Dockerfile b/Dockerfile index cf9c513..2d9fce7 100644 --- a/Dockerfile +++ b/Dockerfile @@ -3,6 +3,6 @@ RUN apt-get update && apt-get install -y ffmpeg && rm -rf /var/lib/apt/lists/* WORKDIR /app COPY core/ core/ COPY server/ server/ -RUN pip install --no-cache-dir fastapi uvicorn +RUN pip install --no-cache-dir fastapi uvicorn[standard] EXPOSE 8000 CMD ["uvicorn", "server.app:app", "--host", "0.0.0.0", "--port", "8000"] diff --git a/core/db.py b/core/db.py index 4862fa7..f741934 100644 --- a/core/db.py +++ b/core/db.py @@ -121,14 +121,14 @@ class ProcessedDB: """Return config dict for an output_path, or None.""" if not self._enabled: return None - self._con.row_factory = sqlite3.Row - row = self._con.execute( + cur = self._con.cursor() + cur.row_factory = sqlite3.Row + row = cur.execute( "SELECT label, category, short_side, portrait_ratio, crop_center, format," " clip_count, spread" " FROM processed WHERE output_path = ?", (output_path,), ).fetchone() - self._con.row_factory = None return dict(row) if row else None def delete_by_output_path(self, output_path: str) -> None: diff --git a/core/export.py b/core/export.py index 2873014..4f04070 100644 --- a/core/export.py +++ b/core/export.py @@ -113,6 +113,7 @@ class ExportRunner: except Exception as e: if "cancelled" not in str(e) and self._on_error: self._on_error(str(e)) + return except Exception as e: if self._on_error: self._on_error(str(e)) diff --git a/server/routes/export.py b/server/routes/export.py index 05b5240..6f47714 100644 --- a/server/routes/export.py +++ b/server/routes/export.py @@ -8,14 +8,16 @@ from pydantic import BaseModel from core.export import ExportRunner from core.paths import build_export_path, build_sequence_dir -from core.ffmpeg import apply_keyframes_to_jobs +from core.ffmpeg import _RATIOS, apply_keyframes_to_jobs from .. import ws as ws_module -from ..config import EXPORT_DIR +from ..config import EXPORT_DIR, MEDIA_DIRS router = APIRouter() _jobs: dict[str, dict] = {} +_VALID_ENCODERS = {"libx264", "h264_nvenc", "h264_vaapi", "h264_qsv", "h264_amf", "h264_videotoolbox"} + class ExportRequest(BaseModel): input_path: str @@ -49,16 +51,41 @@ def _next_counter(folder: str, basename: str) -> int: return highest + 1 +def _validate_input_path(path: str) -> str: + """Verify input_path falls under a configured MEDIA_DIR.""" + real = os.path.realpath(path) + for root in MEDIA_DIRS: + root_real = os.path.realpath(root) + if real == root_real or real.startswith(root_real + os.sep): + return real + raise HTTPException(status_code=403, detail="input_path outside media directories") + + @router.post("/export") def start_export(req: ExportRequest): from ..app import db + # Validate inputs + input_path = _validate_input_path(req.input_path) + + if req.encoder not in _VALID_ENCODERS: + raise HTTPException(status_code=400, detail=f"invalid encoder: {req.encoder}") + + if req.portrait_ratio is not None and req.portrait_ratio not in _RATIOS: + raise HTTPException(status_code=400, detail=f"invalid portrait_ratio: {req.portrait_ratio}") + + if req.folder_suffix and ("/" in req.folder_suffix or "\\" in req.folder_suffix or ".." in req.folder_suffix): + raise HTTPException(status_code=400, detail="folder_suffix must not contain path separators") + + if "/" in req.name or "\\" in req.name or ".." in req.name: + raise HTTPException(status_code=400, detail="name must not contain path separators") + job_id = str(uuid.uuid4())[:8] folder = EXPORT_DIR if req.folder_suffix: - folder = folder + req.folder_suffix + folder = folder.rstrip(os.sep) + "_" + req.folder_suffix - image_sequence = req.format == "WebP" + image_sequence = req.format in ("WebP", "WebP sequence") counter = _next_counter(folder, req.name) # Build job list: (start, output_path, portrait_ratio, crop_center) @@ -87,7 +114,7 @@ def start_export(req: ExportRequest): completed.append(path) # Record in DB so markers show up db.add( - filename=os.path.basename(req.input_path), + filename=os.path.basename(input_path), start_time=req.cursor, output_path=path, label=req.label, @@ -104,15 +131,17 @@ def start_export(req: ExportRequest): def on_all_done(): _jobs[job_id]["status"] = "done" + _jobs[job_id].pop("runner", None) ws_module.broadcast({"type": "all_done", "job_id": job_id}) def on_error(msg: str): _jobs[job_id]["status"] = "error" _jobs[job_id]["error"] = msg + _jobs[job_id].pop("runner", None) ws_module.broadcast({"type": "error", "job_id": job_id, "msg": msg}) runner = ExportRunner( - input_path=req.input_path, + input_path=input_path, jobs=jobs, short_side=req.short_side, image_sequence=image_sequence, @@ -154,9 +183,9 @@ def delete_export(output_path: str): real = os.path.realpath(output_path) if not real.startswith(os.path.realpath(EXPORT_DIR) + os.sep): raise HTTPException(status_code=403, detail="path outside export directory") - db.delete_by_output_path(output_path) - if os.path.isfile(output_path): - os.unlink(output_path) - elif os.path.isdir(output_path): - shutil.rmtree(output_path) - return {"deleted": output_path} + db.delete_by_output_path(real) + if os.path.isfile(real): + os.unlink(real) + elif os.path.isdir(real): + shutil.rmtree(real) + return {"deleted": real} diff --git a/server/ws.py b/server/ws.py index 9ff2742..1abc1d4 100644 --- a/server/ws.py +++ b/server/ws.py @@ -18,7 +18,9 @@ async def connect(ws: WebSocket): try: while True: await ws.receive_text() # keep alive - except WebSocketDisconnect: + except (WebSocketDisconnect, Exception): + pass + finally: with _lock: if ws in _connections: _connections.remove(ws)