fix(uploads): read the body after authenticating, not before it
`POST /api/downloads/upload` declared `file: UploadFile = File(...)`, and FastAPI parses and spools a declared body before the first dependency runs -- `require_human` included. An unauthenticated caller could therefore make the process write the whole per-file allowance (500 MiB by default) to temp, once per request. SEC-20's ceiling bounded that number but could not stop the writing, which is why E27 parked it rather than closing it. The route now takes no body parameter. `spool_upload` names `require_human` first and only then streams `request.stream()` to the staging temp file, so a stranger is refused before a byte moves -- the shape the screenshot route has used since E16. Filename and display name travel as percent-encoded headers (latin-1 on the wire, so a Hungarian filename needs the encoding) instead of multipart parts. `capped_stream_to` joins `capped_body` in routes/_common.py: the same counting rule, written to a file descriptor rather than accumulated, because holding 500 MiB would trade a disk problem for a memory one. Its refusal is the caller's, so the upload route keeps answering 422 `upload_too_large` with the admin knob's own number. With no envelope to allow for, the edge is now exactly that knob, and MULTIPART_ENVELOPE_ALLOWANCE is gone. The invariant stops being prose: test_body_limit.py asks FastAPI itself (`route.dependant.body_params`) and fails the gate if any route with a raised ceiling declares a body. A comment could not have prevented the defect it described; that test would have. Both new tests were seen red for the right reason -- before the fix the anonymous probe got its 401 only after the server had read 4236 bytes of a multipart body, and re-declaring the File parameter turns the invariant test red naming the route and the parameter.
This commit is contained in:
+21
-20
@@ -53,31 +53,32 @@ DEFAULT_MAX_BODY_BYTES = 1024 * 1024
|
||||
#
|
||||
# These are RAISED CEILINGS, NOT EXEMPTIONS, and the difference is the whole point. An exemption
|
||||
# would mean "this route is counted by something else" — which is only true when the route's own
|
||||
# limit applies to the bytes as they ARRIVE. That holds for the screenshot route (no declared body
|
||||
# parameter: `capped_body` streams it, and the owner check resolves first) and it does NOT hold for
|
||||
# the upload route: `file: UploadFile = File(...)` is a declared body, so FastAPI parses and spools
|
||||
# the whole multipart part to a temp file BEFORE the first dependency runs — `require_human`
|
||||
# included. Its in-handler cap therefore measures bytes that have already landed. Left uncounted,
|
||||
# an anonymous caller could stream without bound into the container's tmpfs; counted here, the
|
||||
# process refuses at the same number the route itself enforces, before any of it is stored.
|
||||
# limit applies to the bytes as they ARRIVE. That, in turn, is only true of a route with NO DECLARED
|
||||
# BODY PARAMETER: FastAPI parses and spools a declared body BEFORE the first dependency runs, so a
|
||||
# route that declares one has already stored the bytes by the time its own cap — or its auth — gets
|
||||
# a say.
|
||||
#
|
||||
# Both listed routes now satisfy that. The screenshot route always did (`capped_body` streams it,
|
||||
# and the owner check resolves first). The upload route did NOT until P1: it declared
|
||||
# `file: UploadFile = File(...)`, so an anonymous caller could make the process write the whole
|
||||
# per-file allowance to temp before `require_human` refused them — this middleware bounded that
|
||||
# number but could not stop the writing. It now reads its own body from a dependency declared after
|
||||
# the auth one, so its in-handler cap measures bytes as they arrive, like everything else here.
|
||||
#
|
||||
# That rule is no longer only written here: `test_body_limit.py` parses the routes and fails the
|
||||
# gate if any route in this dict declares a body parameter. A comment could not have stopped the
|
||||
# defect it describes; the test can.
|
||||
#
|
||||
# A route belongs here ONLY IF ITS OWN CAP IS LARGER THAN THE DEFAULT. That is why
|
||||
# `POST /api/diagnostics/events` is absent: its `capped_body` limit is 512 KiB, already below the
|
||||
# default, so listing it could only widen what it accepts.
|
||||
# What a multipart envelope costs on top of the file itself: the boundary lines, the part headers
|
||||
# and the sibling `display_name` field. The edge counts RAW BODY bytes while the upload route counts
|
||||
# the DECODED file, so without this allowance a file of exactly the configured maximum — the number
|
||||
# the admin page advertises, and the first thing anyone testing the limit reaches for — is refused
|
||||
# one layer before the route that would have accepted it. Generous on purpose: its job is to keep
|
||||
# the two layers from disagreeing at the boundary, not to be a second limit.
|
||||
MULTIPART_ENVELOPE_ALLOWANCE = 64 * 1024
|
||||
|
||||
ROUTE_MAX_BODY_BYTES: dict[tuple[str, str], int] = {
|
||||
# Ingesting a local video file. `sysconfig`'s `download_upload_max_bytes` is bounded so it can
|
||||
# never exceed this (see sysconfig.SPECS), which is what stops an admin from raising the route's
|
||||
# own cap past the edge and collecting 413s from a layer the admin page never mentions.
|
||||
("POST", "/api/downloads/upload"): settings.download_upload_max_bytes
|
||||
+ MULTIPART_ENVELOPE_ALLOWANCE,
|
||||
# Ingesting a local file. The body IS the file since P1 — no multipart envelope to allow for —
|
||||
# so this is exactly the number the route enforces. `sysconfig`'s `download_upload_max_bytes` is
|
||||
# bounded so it can never exceed it (see sysconfig.SPECS), which is what stops an admin from
|
||||
# raising the route's own cap past the edge and collecting 413s from a layer the admin page
|
||||
# never mentions.
|
||||
("POST", "/api/downloads/upload"): settings.download_upload_max_bytes,
|
||||
("POST", "/api/debug-sessions/{sid}/screenshots"): debugsession.SCREENSHOT_MAX_BYTES,
|
||||
}
|
||||
|
||||
|
||||
@@ -6,7 +6,7 @@ and "fetch a row this user owns or 404" idioms have one definition, one status c
|
||||
one existence-leak policy.
|
||||
"""
|
||||
|
||||
from typing import Any, TypeVar
|
||||
from typing import IO, Any, TypeVar
|
||||
|
||||
from fastapi import HTTPException, Request
|
||||
from sqlalchemy.orm import Session
|
||||
@@ -79,6 +79,30 @@ async def capped_body(request: Request, limit: int) -> bytes:
|
||||
return bytes(body)
|
||||
|
||||
|
||||
async def capped_stream_to(
|
||||
request: Request, out: IO[bytes], limit: int, refusal: HTTPException
|
||||
) -> int:
|
||||
"""`capped_body`'s sibling for a body too large to hold: the same counting rule, written
|
||||
straight to `out` instead of accumulated, returning the size.
|
||||
|
||||
Two deliberate differences from `capped_body`. It never builds a `bytearray` — the upload route
|
||||
admits 500 MiB by default, so holding the body would trade a disk problem for a memory one. And
|
||||
the refusal is the CALLER'S, passed in rather than assumed: `capped_body`'s flat 413 is right
|
||||
for a screenshot, while the upload route answers 422 `upload_too_large` carrying the admin
|
||||
knob's own number, which is what its dialog shows the user. One counting rule, two answers,
|
||||
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."""
|
||||
size = 0
|
||||
async for chunk in request.stream():
|
||||
size += len(chunk)
|
||||
if size > limit:
|
||||
raise refusal
|
||||
out.write(chunk)
|
||||
return size
|
||||
|
||||
|
||||
def issue_refused(exc: gitea.GiteaError) -> HTTPException:
|
||||
"""The refusal an issue-tracker failure (E16) answers as — the send, the preview and the
|
||||
admin's connection test alike. Literal reasons, one per case: the frontend's contract test reads
|
||||
|
||||
@@ -9,11 +9,13 @@ serves finished files (range-aware, with the user's custom display name), and sh
|
||||
import os
|
||||
import re
|
||||
import tempfile
|
||||
from collections.abc import AsyncIterator
|
||||
from dataclasses import dataclass
|
||||
from datetime import datetime, timedelta, timezone
|
||||
from pathlib import Path
|
||||
from urllib.parse import parse_qs, urlparse
|
||||
from urllib.parse import parse_qs, unquote, urlparse
|
||||
|
||||
from fastapi import APIRouter, Depends, File, Form, HTTPException, Request, UploadFile
|
||||
from fastapi import APIRouter, Depends, HTTPException, Request
|
||||
from fastapi.responses import FileResponse
|
||||
from pydantic import BaseModel
|
||||
from sqlalchemy import func, select
|
||||
@@ -38,7 +40,7 @@ from app.models import (
|
||||
MediaAsset,
|
||||
User,
|
||||
)
|
||||
from app.routes._common import get_or_404, not_found, owned_or_404
|
||||
from app.routes._common import capped_stream_to, get_or_404, not_found, owned_or_404
|
||||
from app.security import hash_password
|
||||
from app.userscope import is_messageable_user, messageable_clauses
|
||||
from app.utils import iso
|
||||
@@ -406,61 +408,113 @@ def enqueue_download(
|
||||
return _serialize_job(db, job)
|
||||
|
||||
|
||||
# Read the upload in 1 MiB chunks so a large file never sits fully in memory, and the size cap is
|
||||
# enforced on the ACTUAL bytes read (a client-set Content-Length can't be trusted to gate it).
|
||||
_UPLOAD_CHUNK = 1024 * 1024
|
||||
# The upload's filename and display name travel as HEADERS, not as multipart parts. That is not a
|
||||
# style choice: a DECLARED body parameter (`file: UploadFile = File(...)`) is read and spooled by
|
||||
# FastAPI *before the first dependency runs*, so `require_human` could not gate it and an anonymous
|
||||
# caller could make the process write the whole per-file allowance to temp, per request (P1; SEC-20
|
||||
# bounded the number but could not stop the writing). Reading the body ourselves, from a dependency
|
||||
# declared AFTER the auth one, is the only shape that closes it — the same one
|
||||
# `POST /api/debug-sessions/{sid}/screenshots` uses, and `test_body_limit.py` now enforces it for
|
||||
# every route with a raised ceiling rather than leaving it to a comment like this one.
|
||||
#
|
||||
# Headers are latin-1 on the wire, so both are percent-encoded by the client and unquoted here; a
|
||||
# Hungarian filename would otherwise be unsendable.
|
||||
UPLOAD_FILENAME_HEADER = "x-upload-filename"
|
||||
UPLOAD_DISPLAY_NAME_HEADER = "x-upload-display-name"
|
||||
|
||||
|
||||
@router.post("/upload")
|
||||
def upload_download(
|
||||
file: UploadFile = File(...),
|
||||
display_name: str | None = Form(None),
|
||||
@dataclass(frozen=True)
|
||||
class SpooledUpload:
|
||||
"""What the body dependency hands the handler: the bytes already on disk, plus the two names
|
||||
that came alongside them."""
|
||||
|
||||
path: Path
|
||||
size: int
|
||||
user_id: int
|
||||
filename: str
|
||||
display_name: str | None
|
||||
|
||||
|
||||
def _header_text(request: Request, name: str) -> str:
|
||||
"""One percent-encoded header, decoded. A header that is not valid UTF-8 once unquoted is the
|
||||
caller's doing, not a server fault — it degrades to empty rather than 500ing, and the route's
|
||||
own fallback names the file."""
|
||||
raw = request.headers.get(name, "")
|
||||
try:
|
||||
return unquote(raw, errors="strict").strip()
|
||||
except (UnicodeDecodeError, ValueError):
|
||||
# silent-ok: a mis-encoded header is the caller's own malformed request, and this one is
|
||||
# cosmetic — it names the file. Refusing the upload over it would fail a 500 MiB transfer on
|
||||
# a label; logging it would hand any client a free log line per request.
|
||||
return ""
|
||||
|
||||
|
||||
async def spool_upload(
|
||||
request: Request,
|
||||
user: User = Depends(require_human),
|
||||
db: Session = Depends(get_db),
|
||||
) -> dict:
|
||||
"""Ingest a local file into the caller's library (E7 S5). Any file type is accepted; media
|
||||
(video/audio) gets the full function set, a generic file is download-only. The file is streamed
|
||||
to a temp path under the download volume (enforcing the per-file cap as it goes), then handed to
|
||||
the non-yt-dlp ingest which probes + places it and returns an already-`done` job.
|
||||
) -> AsyncIterator[SpooledUpload]:
|
||||
"""Stream the request body to a staging temp file, enforcing the per-file cap as it arrives.
|
||||
|
||||
A sync (threadpool) handler on purpose: the ingest does blocking work — an ffprobe of up to 30s
|
||||
plus sync DB writes — so it must NOT run on the event loop. `file.file` is the sync file object
|
||||
Starlette spools the multipart body into."""
|
||||
`user` is taken but unused: it is the whole point. Dependencies resolve in declaration order, so
|
||||
naming `require_human` here makes the auth check a precondition of reading the body at all — a
|
||||
stranger is refused before a byte moves. The `yield` shape owns the temp file's life: ingest
|
||||
MOVES it into place on success (the unlink is then a no-op), and on any other exit — cap hit,
|
||||
empty, quota, probe or move error, a client that vanished mid-upload — it is removed."""
|
||||
cap = sysconfig.get_int(db, "download_upload_max_bytes")
|
||||
staging = Path(settings.download_root) / ".staging" / "uploads"
|
||||
staging.mkdir(parents=True, exist_ok=True)
|
||||
fd, tmp = tempfile.mkstemp(dir=str(staging))
|
||||
tmp_path = Path(tmp)
|
||||
try:
|
||||
size = 0
|
||||
with os.fdopen(fd, "wb") as out:
|
||||
while chunk := file.file.read(_UPLOAD_CHUNK):
|
||||
size += len(chunk)
|
||||
if cap and size > cap:
|
||||
raise HTTPException(
|
||||
status_code=422, detail={"code": "upload_too_large", "limit": cap}
|
||||
)
|
||||
out.write(chunk)
|
||||
if size == 0:
|
||||
raise HTTPException(status_code=400, detail={"code": "upload_empty"})
|
||||
try:
|
||||
job = uploadmod.ingest_upload(
|
||||
db,
|
||||
user.id,
|
||||
filename=file.filename or "upload",
|
||||
src_path=tmp_path,
|
||||
size=size,
|
||||
display_name=(display_name or "").strip() or None,
|
||||
size = await capped_stream_to(
|
||||
request,
|
||||
out,
|
||||
cap,
|
||||
HTTPException(status_code=422, detail={"code": "upload_too_large", "limit": cap}),
|
||||
)
|
||||
except quota.QuotaExceeded as e:
|
||||
raise HTTPException(status_code=422, detail=_quota_detail(e))
|
||||
return _serialize_job(db, job)
|
||||
yield SpooledUpload(
|
||||
path=tmp_path,
|
||||
size=size,
|
||||
user_id=user.id,
|
||||
filename=_header_text(request, UPLOAD_FILENAME_HEADER) or "upload",
|
||||
display_name=_header_text(request, UPLOAD_DISPLAY_NAME_HEADER) or None,
|
||||
)
|
||||
finally:
|
||||
# Ingest MOVES the temp file into place on success (unlink is then a no-op); on any early exit
|
||||
# — cap hit, empty, quota, probe/move error — this removes the abandoned staging file.
|
||||
tmp_path.unlink(missing_ok=True)
|
||||
|
||||
|
||||
@router.post("/upload")
|
||||
def upload_download(
|
||||
upload: SpooledUpload = Depends(spool_upload),
|
||||
db: Session = Depends(get_db),
|
||||
) -> dict:
|
||||
"""Ingest a local file into the caller's library (E7 S5). Any file type is accepted; media
|
||||
(video/audio) gets the full function set, a generic file is download-only. The body IS the file
|
||||
(see the header note above); `spool_upload` has already authenticated the caller and streamed it
|
||||
to a temp path under the download volume, so what is left here is the non-yt-dlp ingest which
|
||||
probes + places it and returns an already-`done` job.
|
||||
|
||||
A sync (threadpool) handler on purpose: the ingest does blocking work — an ffprobe of up to 30s
|
||||
plus sync DB writes — so it must NOT run on the event loop. The streaming half is async because
|
||||
reading the body is; keeping them in separate callables is what lets each run where it belongs."""
|
||||
if upload.size == 0:
|
||||
raise HTTPException(status_code=400, detail={"code": "upload_empty"})
|
||||
try:
|
||||
job = uploadmod.ingest_upload(
|
||||
db,
|
||||
upload.user_id,
|
||||
filename=upload.filename,
|
||||
src_path=upload.path,
|
||||
size=upload.size,
|
||||
display_name=upload.display_name,
|
||||
)
|
||||
except quota.QuotaExceeded as e:
|
||||
raise HTTPException(status_code=422, detail=_quota_detail(e))
|
||||
return _serialize_job(db, job)
|
||||
|
||||
|
||||
class EnqueueEditIn(BaseModel):
|
||||
source_job_id: int = 0
|
||||
edit_spec: dict
|
||||
|
||||
Reference in New Issue
Block a user