diff --git a/backend/app/routes/_common.py b/backend/app/routes/_common.py index 19205df3..3526248f 100644 --- a/backend/app/routes/_common.py +++ b/backend/app/routes/_common.py @@ -93,13 +93,30 @@ async def capped_stream_to( neither of them a second copy of the loop. The caller owns `out` and owns cleaning it up: a body refused half-way has already written what - arrived before the limit was passed.""" + arrived before the limit was passed. + + **The refusal DRAINS the rest of the body before it is raised, and that is not politeness.** + Answering mid-upload and closing the connection is what the client sees as a network failure, + not as a refusal: the browser's XHR fires `onerror` before it can read the response, so a file + over the limit reported itself as "the server couldn't be reached" instead of naming the limit — + measured in a real browser, not reasoned about. Reading the remainder to nowhere lets the client + finish sending, read the 422 and say the true thing. + + The bytes are discarded, never written, and the whole request is already bounded by + `bodylimit`'s per-route ceiling — so the worst case is bandwidth an authenticated caller was + sending anyway, against the old behaviour which wrote exactly those bytes to disk.""" size = 0 + over = False async for chunk in request.stream(): size += len(chunk) + if over: + continue # draining — see the docstring; counted, not stored if size > limit: - raise refusal + over = True + continue out.write(chunk) + if over: + raise refusal return size diff --git a/backend/tests/test_upload.py b/backend/tests/test_upload.py index 4e983b5a..33f6ad83 100644 --- a/backend/tests/test_upload.py +++ b/backend/tests/test_upload.py @@ -12,11 +12,15 @@ parsing (image → file, cover-art skip, dims, duration) is covered directly by below, which stubs one level down (`run_ffprobe_json`) and exercises the real `probe_media`. """ +import asyncio +import io from pathlib import Path import pytest +from fastapi import HTTPException from app.config import settings +from app.routes._common import capped_stream_to from app.downloads import mediakind, quota, storage from app.downloads import probe as probemod from app.downloads import upload as uploadmod @@ -211,6 +215,43 @@ def test_an_authenticated_upload_still_has_its_body_read(as_user, make_user, _ro assert sum(pulled) == 4096 +def test_capped_stream_to_drains_the_rest_before_refusing(): + """Measured in a real browser, then pinned HERE rather than at the route. + + The cap now fires while the client is still sending, and answering + closing mid-upload is what + a browser reports as a NETWORK failure: the dialog said "the server couldn't be reached" + instead of naming the limit, because XHR never got to read the 422. `capped_stream_to` reads + the remainder to nowhere before raising, so the client finishes sending and reads the real + answer. + + It is asserted against the helper and not through TestClient on purpose — httpx's ASGI + transport pumps the request body whether or not the app reads it, so a route-level version of + this test passes just as happily with the drain deleted. It was written that way first and + caught by its own mutation check. + """ + pulled = [] + + async def stream(): + for i in range(8): + pulled.append(i) + yield b"x" * 1024 + + class _Req: + def stream(self): + return stream() + + out = io.BytesIO() + refusal = HTTPException(status_code=422, detail={"code": "upload_too_large", "limit": 2048}) + # `asyncio.run` rather than an async test: the suite has no async plugin, and adding one to + # pin four lines of helper would mean a new dev dependency and a lock regeneration. + with pytest.raises(HTTPException) as exc: + asyncio.run(capped_stream_to(_Req(), out, 2048, refusal)) + assert exc.value is refusal + assert len(pulled) == 8, "the body was abandoned mid-stream, not drained" + # Drained means DISCARDED: nothing past the limit is written. + assert len(out.getvalue()) <= 2048 + + # --- route ----------------------------------------------------------------------------------