fix(uploads): drain an over-cap body before refusing it
Found in the browser UAT, not by the gate. Moving the cap onto the arriving bytes meant it fires while the client is still sending, and answering + closing mid-upload is what XHR reports as a network failure: the upload dialog said "the server couldn't be reached" while the access log showed a correct 422 carrying the limit. A refusal the client cannot read is an outage as far as the user is concerned. `capped_stream_to` now reads the remainder to nowhere before raising, so the client finishes sending and reads the real answer -- re-measured in the browser, which now names the limit. Nothing past the cap is written, and the request is still bounded by bodylimit's ceiling, so the cost is bandwidth an AUTHENTICATED caller was sending anyway. The anonymous path is untouched: auth still resolves before a byte is read. The regression test sits on the helper, not the route, and the reason is worth keeping: the route-level version was written first and PASSED with the drain deleted. httpx's ASGI transport pumps the request body whether or not the app reads it, so "drained" and "abandoned" are indistinguishable through TestClient. Its own mutation check is what exposed it.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
|
||||
@@ -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 ----------------------------------------------------------------------------------
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user