Skip to content

fix: backend hardening (event loop, path traversal, stop, CORS) + dead code removal - #36

Open
ilyautov wants to merge 1 commit into
BazedFrog:mainfrom
ilyautov:fix/critical-backend-hardening
Open

fix: backend hardening (event loop, path traversal, stop, CORS) + dead code removal#36
ilyautov wants to merge 1 commit into
BazedFrog:mainfrom
ilyautov:fix/critical-backend-hardening

Conversation

@ilyautov

Copy link
Copy Markdown

Summary

A focused backend-hardening pass: four critical fixes plus removal of dead code, with a new pytest suite (44 tests) covering the changes.

Critical fixes

1. Event loop no longer blocked by subprocess calls

subprocess.run() was called directly inside async handlers — most notably video export (main.py, timeout=600) and reference trimming (timeout=60). During a video export the entire server froze, including generation progress polling. All ffmpeg/ffprobe invocations in request handlers now go through a small run_command() helper built on asyncio.to_thread().

2. Path traversal protection

IDs taken from the URL (gen_id, ref_id) were interpolated into filesystem paths without validation (OUTPUT_DIR / gen_id + shutil.rmtree, UPLOADS_DIR.glob(f"{ref_id}_*")). All path-bound IDs are now validated against a strict whitelist (^[A-Za-z0-9][A-Za-z0-9_-]{0,63}$) via require_valid_id(), and reference_audio_id is validated in the Pydantic schema. Uploaded filenames are sanitized with Path(...).name so a crafted filename like ../../evil.wav cannot escape the uploads dir.

3. Stop now works for processing generations

Previously /api/stop returned 400 for processing generations, and the model server's /cancel flag was only checked after inference finished — so an in-flight generation could never actually be stopped. Now stopping a processing generation terminates the model server process (the only reliable way to abort GPU inference); run_generation preserves the stopped status instead of overwriting it with failed, and partial output is removed so a stopped generation isn't resurrected as completed by restore_library() on restart. The next generation restarts the server and reloads the model automatically, as before.

4. CORS fix

allow_origins=["*"] combined with allow_credentials=True is rejected by browsers. The UI is served same-origin, so credentials are simply disabled.

Dead code removal

  • SSE machinery: /api/events returned 410 ("SSE disabled") and the frontend uses polling, so sse.py, /api/test-sse, and the notify_* callbacks threaded through the entire generation call chain did nothing but wasted work (e.g. recomputing get_all_models() for zero listeners). All removed; git history preserves them if SSE is ever revived.
  • USE_MODEL_SERVER flag: the False branch raised "Subprocess mode not implemented" — removed the flag and the dead branch.
  • Debug prints in /api/models, stray notify_callback plumbing in models.py.
  • .gitignore: ignore runtime state files (output/, uploads/, queue.json, timing_history.json, verified_models.json) for running from the repo root in development.

Tests

New tests/ suite (44 tests, pytest + requirements-dev.txt):

  • test_api.py — every ID-taking endpoint rejects unsafe IDs with 400 before touching the filesystem; encoded-slash payloads verified to never match routes; upload filename sanitization (incl. ../../evil.wav).
  • test_generation.pyis_safe_id, reference_audio_id schema validation, build_lyrics_string / build_description / clean_lyrics_line.

Smoke-tested with a live server: /api/health, /api/models, queue endpoints, index page, traversal attempts (400/404 as appropriate), removed endpoints return 404.

python3 -m pytest tests/ -q
# 44 passed

Notes / follow-ups (out of scope)

  • The frontend defines stopGeneration() in api.js but never wires it to a button — with this PR the backend is ready for a working Stop button.
  • GPU-dependent paths (actual generation, model load) can't be exercised in CI here; the fixes around them are covered by the stop/validation logic tests.

…d code removal

- Stop blocking the event loop: run ffmpeg/ffprobe via asyncio.to_thread
  (video export with timeout=600 previously froze the whole server,
  including generation progress polling)
- Validate path-bound IDs (gen_id, ref_id) against a strict whitelist to
  prevent path traversal; sanitize uploaded filenames via Path(...).name
- Make /api/stop work for processing generations by terminating the model
  server process; run_generation keeps the "stopped" status instead of
  overwriting it with "failed"; partial output is removed so stopped
  generations are not resurrected as completed on restart
- Fix invalid CORS combination (allow_origins=["*"] with
  allow_credentials=True is rejected by browsers and unneeded same-origin)
- Remove dead code: SSE machinery (sse.py, 410 /api/events, /api/test-sse,
  notify callbacks threaded through the whole call chain), the
  unimplemented USE_MODEL_SERVER=False path, debug prints in /api/models
- Add pytest suite (44 tests) for ID validation, upload sanitization,
  and lyrics/description builders; add requirements-dev.txt
- Ignore runtime state files (output/, uploads/, *.json state) in .gitignore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant