From 017ab2599fc7fe678e7256399da63c149550ffc8 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:34:56 +0100 Subject: [PATCH] fix(api): bound request input that could exhaust memory or stall the loop Four related holes, all reachable with one unauthenticated request. The trim range had no ceiling. `end` is a float that reaches np.zeros(int(round(duration * sample_rate))) in the click renderer, so ?start=0&end=20000&count_in=1 asked for roughly 7 GB, and another 7 GB in the int16 conversion. A larger value raised MemoryError inside a blanket except, which silently shipped the export with no click rather than failing. _validate_trim_range bounds it by the job's own recorded duration, with a six-hour backstop for a job whose duration was never recorded, and a second of slack because ffprobe's duration can sit a hair under the decoded length. The click render ran synchronously inside async handlers, so all of that allocation and a Python loop over every beat blocked the event loop -- every SSE progress stream and the queue worker stalled behind it. It goes through asyncio.to_thread now. The buffer is float32 rather than float64: the output is 16-bit PCM, so the extra mantissa was never audible and a long export was allocating twice what it needed. The click cache pruned without keep=, so a render larger than the cache budget evicted itself the instant it was written and ffmpeg was handed a missing -i. That is the #482 bug, unfixed on this path. The mixdown write had the same gap against a concurrent render's prune. The body-size guard was scoped to paths ending /sections or /beats, leaving /api/search, /api/playlist, /api/settings and the JSON branch of /api/jobs uncapped -- Starlette buffers the whole body, then json.loads runs it on the event loop. A 200 MB body to /api/search stalled every other request with no valid job or prior state needed. It now applies by method, exempting multipart uploads, which stream to disk under their own 400 MB limit. A chunked request with no Content-Length used to skip the check entirely and fall through to an unbounded request.body(); it gets a 411 now. Six of the eight new tests fail against the old code. Refs #512 --- app/api/stems.py | 46 +++++++++++-- app/main.py | 41 +++++++++-- app/pipeline/click_render.py | 6 +- tests/test_jobs_api.py | 10 +-- tests/test_request_body_limits.py | 110 ++++++++++++++++++++++++++++++ 5 files changed, 195 insertions(+), 18 deletions(-) create mode 100644 tests/test_request_body_limits.py diff --git a/app/api/stems.py b/app/api/stems.py index e4710a15..f9605169 100644 --- a/app/api/stems.py +++ b/app/api/stems.py @@ -289,7 +289,10 @@ def _click_lane( return None if rendered is None: return None - _prune_mixdown_cache(_CLICK_CACHE_DIR) + # keep=path or a render larger than the cache budget evicts itself the + # instant it is written, and ffmpeg is then handed a missing -i (#512). + # Same reason the mixdown path passes keep= (#482). + _prune_mixdown_cache(_CLICK_CACHE_DIR, keep=path) return _ClickLane(path, g, lead_in, True) if not path.is_file(): @@ -308,10 +311,37 @@ def _click_lane( return None if rendered is None: return None - _prune_mixdown_cache(_CLICK_CACHE_DIR) + _prune_mixdown_cache(_CLICK_CACHE_DIR, keep=path) return _ClickLane(path, g, 0.0, False) +# A trim range is only ever meaningful inside the track. Without a ceiling, +# `end` is an unbounded float that reaches +# np.zeros(int(round(duration * sample_rate))) in the click renderer -- so +# ?start=0&end=20000&count_in=1 asks for a 7 GB allocation, and a larger value +# raises MemoryError inside a blanket except and silently drops the click +# (#512). The job's own duration is the honest ceiling; MAX_TRIM_SECONDS is the +# backstop for a job whose duration was never recorded. +MAX_TRIM_SECONDS = 6 * 60 * 60 + + +def _validate_trim_range(job_id: str, start: float | None, end: float | None) -> None: + """Reject a trim range that is not inside the track.""" + if start is None or end is None: + return + job = registry_get(job_id) + duration = getattr(job, "duration_sec", None) if job is not None else None + ceiling = float(duration) if duration else float(MAX_TRIM_SECONDS) + # A little slack over the recorded duration: it comes from ffprobe and can + # sit a hair under the decoded length, and the UI legitimately asks for the + # very end of a track. + if end > ceiling + 1.0: + raise HTTPException( + status_code=422, + detail="end is beyond the end of the track", + ) + + def _read_beat_grid(job_id: str) -> dict | None: """The grid an export should click to: the user's edits when present, the detected grid otherwise. Mirrors GET /api/jobs/{id}/beats.""" @@ -439,7 +469,7 @@ async def _stream_ffmpeg(cmd: list[str], context: str = "", cache_path: Path | N if tmp_path is not None: if finished and proc.returncode == 0: os.replace(tmp_path, cache_path) - _prune_mixdown_cache(cache_path.parent) + _prune_mixdown_cache(cache_path.parent, keep=cache_path) else: tmp_path.unlink(missing_ok=True) @@ -655,6 +685,7 @@ async def get_stem( status_code=422, detail="start and end are both required and start must be less than end", ) + _validate_trim_range(job_id, start, end) cmd = [ ffmpeg_executable(), @@ -701,6 +732,7 @@ async def get_stem_mp3( status_code=422, detail="start and end are both required and start must be less than end", ) + _validate_trim_range(job_id, start, end) # Full-stem requests (no trim) are cached to disk so repeat loads — the # common case for the mobile player — are instant instead of re-encoding. @@ -799,6 +831,7 @@ async def get_mixdown( status_code=422, detail="start and end are both required and start must be less than end", ) + _validate_trim_range(job_id, start, end) # Validates job_id (404), job done (404), and path traversal (404) per # stem -- deliberately before the cache lookup below, so a deleted or @@ -807,7 +840,8 @@ async def get_mixdown( paths = [_validate_stem_path(job_id, name) for name in names] media_type = MIXDOWN_MEDIA_TYPES[ext] - click_lane = _click_lane( + click_lane = await asyncio.to_thread( + _click_lane, job_id, click, click_mult, @@ -958,7 +992,9 @@ async def get_video_mixdown( # Click is one more audio input. It must be appended before the video input # so the audio indices the filter graph references stay contiguous from 0. - click_lane = _click_lane(job_id, click, click_mult, click_accent, click_gain) + click_lane = await asyncio.to_thread( + _click_lane, job_id, click, click_mult, click_accent, click_gain + ) if click_lane is not None: paths = [*paths, click_lane[0]] parsed_gains = [*parsed_gains, click_lane[1]] diff --git a/app/main.py b/app/main.py index 99dc0ff6..9bd29fbe 100644 --- a/app/main.py +++ b/app/main.py @@ -850,17 +850,44 @@ def download_logs_zip() -> StreamingResponse: # The ceiling is far above either editor's reach: 10000 sections with the # longest name each is about 1.6 MB, and 20000 beats about 0.4 MB. Uploads are # unaffected -- they are a different path with their own 400 MB limit. -_EDITOR_BODY_LIMIT = 4 * 1024 * 1024 -_EDITOR_PATH_SUFFIXES = ("/sections", "/beats") +_JSON_BODY_LIMIT = 4 * 1024 * 1024 +# Multipart uploads stream to disk and enforce their own, much larger, limit. + + +def _is_upload(request: Request) -> bool: + ctype = request.headers.get("content-type", "") + return ctype.startswith("multipart/form-data") + + +def _is_chunked(request: Request) -> bool: + return "chunked" in request.headers.get("transfer-encoding", "").lower() @app.middleware("http") -async def limit_editor_body_size(request: Request, call_next): - if request.method in ("PATCH", "POST", "PUT") and request.url.path.endswith( - _EDITOR_PATH_SUFFIXES - ): +async def limit_json_body_size(request: Request, call_next): + """Cap JSON request bodies. + + Scoped by path suffix before, which left every other JSON endpoint + uncapped: /api/search, /api/playlist, /api/settings and the JSON branch of + /api/jobs all await request.json(), and Starlette accumulates the whole + body before json.loads runs it on the event loop. A 200 MB body to + /api/search stalled every other request, including a running job's progress + stream, with no valid job or prior state needed (#481, reopened as #512). + + Uploads are exempt: they are multipart, not JSON, and carry their own + 400 MB limit on a path that streams to disk rather than buffering. + """ + if request.method in ("PATCH", "POST", "PUT") and not _is_upload(request): declared = request.headers.get("content-length") - if declared and declared.isdigit() and int(declared) > _EDITOR_BODY_LIMIT: + if declared is None: + # No Content-Length means chunked, which used to skip the check + # entirely and fall through to an unbounded request.body(). + if _is_chunked(request): + return JSONResponse( + {"detail": "request body must declare its length"}, + status_code=411, + ) + elif declared.isdigit() and int(declared) > _JSON_BODY_LIMIT: return JSONResponse({"detail": "request body too large"}, status_code=413) return await call_next(request) diff --git a/app/pipeline/click_render.py b/app/pipeline/click_render.py index 0db91943..e94709a2 100644 --- a/app/pipeline/click_render.py +++ b/app/pipeline/click_render.py @@ -258,7 +258,11 @@ def _render_events( import numpy as np - buf = np.zeros(total, dtype=np.float64) + # float32, not float64: the buffer is one sample per frame for the whole + # render, so a long export was allocating twice what it needed and then + # again in the int16 conversion below. The output is 16-bit PCM, so the + # extra mantissa was never audible (#512). + buf = np.zeros(total, dtype=np.float32) # Only two distinct voices, so render each once and stamp it in. plain = _voice(CLICK_PEAK, CLICK_FREQ, sample_rate) accented = _voice(ACCENT_PEAK, ACCENT_FREQ, sample_rate) diff --git a/tests/test_jobs_api.py b/tests/test_jobs_api.py index 1561044c..e8447c0a 100644 --- a/tests/test_jobs_api.py +++ b/tests/test_jobs_api.py @@ -386,15 +386,15 @@ def test_oversized_editor_body_is_refused_before_it_is_parsed(client, done_job): check from 32 ms to 5219 ms, and 16 ms once Content-Length was checked in middleware first (#481). """ - from app.main import _EDITOR_BODY_LIMIT + from app.main import _JSON_BODY_LIMIT padded = dict(_section(0), name="V" * 64) - count = (_EDITOR_BODY_LIMIT // len(json.dumps(padded, separators=(",", ":")))) + 500 + count = (_JSON_BODY_LIMIT // len(json.dumps(padded, separators=(",", ":")))) + 500 payload = {"sections": [dict(padded, id=f"sec{i}") for i in range(count)]} # Sent as the exact bytes measured, so the assertion cannot drift from what # actually goes on the wire and quietly stop testing the ceiling. raw = json.dumps(payload, separators=(",", ":")).encode() - assert len(raw) > _EDITOR_BODY_LIMIT + assert len(raw) > _JSON_BODY_LIMIT r = client.patch( f"/api/jobs/{done_job.id}/sections", @@ -410,7 +410,7 @@ def test_the_body_ceiling_clears_the_largest_legitimate_editor_payload(client, d """The ceiling must never be reachable by a real track. 10000 sections at the longest permitted name is about 1.6 MB against a 4 MB ceiling.""" import app.api.jobs as jobs_mod - from app.main import _EDITOR_BODY_LIMIT + from app.main import _JSON_BODY_LIMIT payload = { "sections": [ @@ -418,7 +418,7 @@ def test_the_body_ceiling_clears_the_largest_legitimate_editor_payload(client, d ] } raw = json.dumps(payload, separators=(",", ":")).encode() - assert len(raw) < _EDITOR_BODY_LIMIT + assert len(raw) < _JSON_BODY_LIMIT r = client.patch( f"/api/jobs/{done_job.id}/sections", diff --git a/tests/test_request_body_limits.py b/tests/test_request_body_limits.py new file mode 100644 index 00000000..4eb88092 --- /dev/null +++ b/tests/test_request_body_limits.py @@ -0,0 +1,110 @@ +"""Unbounded request input (#512). + +The body-size guard was scoped to paths ending /sections or /beats, so every +other JSON endpoint accumulated an arbitrarily large body and then ran +json.loads on the event loop. A chunked request skipped the check entirely. + +The trim range had no ceiling either: `end` reaches +np.zeros(int(round(duration * sample_rate))) in the click renderer, so +?start=0&end=20000&count_in=1 asked for a multi-GB allocation. +""" + +from __future__ import annotations + +import pytest +from fastapi.testclient import TestClient + +from app.core.models import Job +from app.core.registry import _jobs +from app.core.registry import register as registry_register + + +@pytest.fixture(autouse=True) +def _clean_jobs(): + _jobs.clear() + yield + _jobs.clear() + + +@pytest.fixture +def client(): + from app.main import app + + with TestClient(app) as c: + yield c + + +@pytest.fixture +def big_body(): + from app.main import _JSON_BODY_LIMIT + + return "x" * (_JSON_BODY_LIMIT + 1024) + + +@pytest.mark.parametrize( + "path", + [ + "/api/search", + "/api/playlist", + "/api/playlist/preview", + "/api/settings", + ], +) +def test_a_huge_json_body_is_refused_before_it_is_parsed(client, path, big_body): + # Previously uncapped: Starlette buffers the whole body, then json.loads + # runs it on the event loop and stalls every other request. + res = client.post( + path, content=f'{{"q": "{big_body}"}}', headers={"content-type": "application/json"} + ) + + assert res.status_code == 413 + + +def test_a_chunked_body_cannot_skip_the_check(client): + # No Content-Length made `declared` None, so the guard fell through to an + # unbounded request.body(). + res = client.post( + "/api/search", + content=iter([b'{"q": "', b"x" * 4096, b'"}']), + headers={"content-type": "application/json", "transfer-encoding": "chunked"}, + ) + + assert res.status_code == 411 + + +def test_a_normal_json_body_still_works(client): + res = client.post("/api/settings", json={"max_duration_sec": 600}) + + assert res.status_code == 200 + + +# ─── trim range ─── + + +def _done_job(job_id="a1b2c3d4e5f6", duration=180.0): + job = Job(id=job_id, status="done", title="Song", duration_sec=duration) + registry_register(job) + return job + + +def test_a_trim_end_beyond_the_track_is_refused(client, tmp_path): + _done_job() + + res = client.get( + "/api/jobs/a1b2c3d4e5f6/mixdown.wav", + params={"stems": "vocals", "gains": "1", "start": 0, "end": 20000, "count_in": 1}, + ) + + assert res.status_code == 422, "an unbounded end reaches a multi-GB np.zeros" + + +def test_a_trim_range_inside_the_track_is_not_refused_by_the_bound(client): + # Must not 422 on the bound; a later 404 for missing stems is fine. + _done_job() + + res = client.get( + "/api/jobs/a1b2c3d4e5f6/mixdown.wav", + params={"stems": "vocals", "gains": "1", "start": 0, "end": 120}, + ) + + assert res.status_code != 422