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