From 6a53930365fc342433acba7fc5da50a116a06e34 Mon Sep 17 00:00:00 2001 From: MrChengLen Date: Thu, 29 Oct 2026 15:46:08 +0100 Subject: [PATCH] =?UTF-8?q?fix(privacy):=20the=20upload=20goes=20as=20soon?= =?UTF-8?q?=20as=20the=20result=20exists=20=E2=80=94=20not=20when=20the=20?= =?UTF-8?q?response=20ends?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The single-file routes answer with TempDirFileResponse, which removes the request's temp dir when the response ends. Until then the upload sat in that dir next to the result. The web framework also kept its own copy: Starlette spools every file part over 1 MB to a temp file, and FastAPI closes it only after the response. Both outlived the processing they were for. What changes: - /convert, /compress, /pdf/extract and /pdf/compress delete the upload (input_path.unlink) right after the work succeeds, as /pdf/insert already did. - /convert skips that when the result has the upload's name: pdf -> pdf, the one identity pair in the registry, writes its result over the upload. - Every route closes the framework's copy as soon as it has copied it into the request dir: - /convert and /compress, single and batch: await file.close(); - the PDF routes (extract, split, compress and both insert uploads): upload.file.close() in pdf_pages._write_upload_and_check_magic; - redaction (off by default): await file.close() after its read. - The four pdf_pages handlers for an HTTPException now remove the temp dir before the metric await, as convert and compress do (security rule: rmtree first). - responses.py's docstring and the docs (architecture, security-overview) say what stays on disk now. Tests, in tests/test_temp_cleanup.py: - test_only_the_result_is_left_when_the_response_starts, over every file route (_FILE_ROUTES, now with convert-pdf-to-pdf): when the response is built, the request dir holds exactly the result, and every framework copy of an upload is closed. On main it fails for convert, compress and extract. - test_a_large_upload_on_disk_is_closed_before_the_response: a 1.2 MB upload that was rolled to disk is closed by then. Reviews: - security-auditor: READY after one round. It found the framework copy (Medium: without the close, the promise held only for small files) and the wording, and confirmed nothing reads an upload after it goes and that the magic-byte check still runs first. - code-reviewer: approved after one round. Adopted: - the check moved into test_temp_cleanup over every file route; - the result's name read from the response; - a batch test that fails without the batch closes; - the docstring and changelog wording. - Not in this change, noted for later: on Windows a damaged image to insert keeps its file open, so that request dir waits for the sweep (Linux unaffected). Full suite: 3067 passed, 272 skipped locally (Starlette and FastAPI at the lock's versions). The one failure, test_stripe_ca_bundle, depends on the local certifi and fails on unchanged main too. ruff, ruff format, the i18n drift-check and the pre-commit hook are clean. Co-Authored-By: Claude Opus 5.5 --- app/api/routes/ai.py | 1 + app/api/routes/compress.py | 7 ++ app/api/routes/convert.py | 9 ++ app/api/routes/pdf_pages.py | 16 ++- app/core/responses.py | 8 +- .../2026-10-29-upload-deleted-early.md | 10 ++ docs/architecture.md | 10 +- docs/security-overview.md | 10 +- tests/test_temp_cleanup.py | 98 +++++++++++++++++++ 9 files changed, 154 insertions(+), 15 deletions(-) create mode 100644 changelog.d/2026-10-29-upload-deleted-early.md diff --git a/app/api/routes/ai.py b/app/api/routes/ai.py index 559a41f..e4a1e4e 100644 --- a/app/api/routes/ai.py +++ b/app/api/routes/ai.py @@ -135,6 +135,7 @@ async def _read_validated_bytes(file: UploadFile, quota) -> bytes: """Read the upload and apply the shared upload-safety plumbing (size cap, magic-byte deny-list) — same guards as the converters.""" data = await file.read() + await file.close() # the framework's own copy (on disk above 1 MB) goes now if len(data) > quota.max_file_size_bytes: limit_mb = quota.max_file_size_bytes // _MB raise HTTPException( diff --git a/app/api/routes/compress.py b/app/api/routes/compress.py index c2c78f0..733678f 100644 --- a/app/api/routes/compress.py +++ b/app/api/routes/compress.py @@ -163,6 +163,9 @@ async def _do_compress( with input_path.open("wb") as f: shutil.copyfileobj(file.file, f) + # The framework's own copy (on disk above 1 MB) goes now, not after the + # response. + await file.close() input_size_bytes = input_path.stat().st_size # A-1: Magic-byte validation @@ -235,6 +238,9 @@ async def _do_compress( else: output_disk_size = output_path.stat().st_size + # Only the result is needed to answer; the upload goes now. + input_path.unlink(missing_ok=True) + # S1-B: Output-cap check — defensive guard; compression should shrink # files, but a pathological input (already-optimized PNG, very-low-bitrate # video) may not gain anything, and we still want an upper bound. @@ -476,6 +482,7 @@ async def _do_compress_batch( with input_path.open("wb") as f: shutil.copyfileobj(upload.file, f) + await upload.close() # the framework's own copy goes now with open(input_path, "rb") as f: header = f.read(16) diff --git a/app/api/routes/convert.py b/app/api/routes/convert.py index 4aa79f8..501bbce 100644 --- a/app/api/routes/convert.py +++ b/app/api/routes/convert.py @@ -158,6 +158,9 @@ async def _do_convert( with input_path.open("wb") as f: shutil.copyfileobj(file.file, f) + # The framework's own copy (on disk above 1 MB) goes now, not after the + # response. + await file.close() input_size_bytes = input_path.stat().st_size # A-1: Magic-byte validation @@ -231,6 +234,11 @@ async def _do_convert( else: output_disk_size = output_path.stat().st_size + # Only the result is needed to answer; the upload goes now. PDF → PDF + # writes its result over the upload, so there the one file stays. + if input_path != output_path: + input_path.unlink(missing_ok=True) + # S1-B: Output-cap check — reject before loading into memory. Guards # against bandwidth amplification (JPG→PNG ~5-10×, MP3→WAV ~11×). if output_disk_size is None or output_disk_size > quota.output_cap_bytes: @@ -489,6 +497,7 @@ async def _do_convert_batch( with input_path.open("wb") as f: shutil.copyfileobj(upload.file, f) + await upload.close() # the framework's own copy goes now with open(input_path, "rb") as f: header = f.read(16) diff --git a/app/api/routes/pdf_pages.py b/app/api/routes/pdf_pages.py index c8b2c2b..4c75da5 100644 --- a/app/api/routes/pdf_pages.py +++ b/app/api/routes/pdf_pages.py @@ -92,6 +92,8 @@ def _write_upload_and_check_magic( """ with input_path.open("wb") as f: shutil.copyfileobj(upload.file, f) + # The framework's own copy (on disk above 1 MB) goes now, not after the response. + upload.file.close() with open(input_path, "rb") as f: header = f.read(16) if any(header.startswith(sig) for sig in BLOCKED_MAGIC): @@ -187,6 +189,9 @@ async def _do_extract( detail="Page extraction failed. Verify your file is a valid PDF.", ) + # Only the result is needed to answer; the upload goes now. + input_path.unlink(missing_ok=True) + output_disk_size = output_path.stat().st_size _enforce_output_cap(output_disk_size, quota, tier) @@ -215,9 +220,9 @@ async def _do_extract( duration_ms=duration_ms, ) except HTTPException: + shutil.rmtree(tmp_dir, ignore_errors=True) await metric_increment("failures.pdf_extract") record_conversion("pdf_extract", "pdf", "pdf", "failure") - shutil.rmtree(tmp_dir, ignore_errors=True) raise except BaseException: shutil.rmtree(tmp_dir, ignore_errors=True) @@ -335,9 +340,9 @@ async def _do_split( duration_ms=duration_ms, ) except HTTPException: + shutil.rmtree(tmp_dir, ignore_errors=True) await metric_increment("failures.pdf_split") record_conversion("pdf_split", "pdf", "zip", "failure") - shutil.rmtree(tmp_dir, ignore_errors=True) raise except BaseException: shutil.rmtree(tmp_dir, ignore_errors=True) @@ -496,6 +501,9 @@ async def _do_compress( headers={"X-FileMorph-Error-Code": "invalid_pdf"}, ) + # Only the result is needed to answer; the upload goes now. + input_path.unlink(missing_ok=True) + output_disk_size = output_path.stat().st_size # The engine targets ``target_bytes`` (already ≤ cap), but a # text-heavy PDF can legitimately come out larger than the target @@ -536,9 +544,9 @@ async def _do_compress( duration_ms=duration_ms, ) except HTTPException: + shutil.rmtree(tmp_dir, ignore_errors=True) await metric_increment("failures.pdf_compress") record_conversion("pdf_compress", "pdf", "pdf", "failure") - shutil.rmtree(tmp_dir, ignore_errors=True) raise except BaseException: shutil.rmtree(tmp_dir, ignore_errors=True) @@ -796,9 +804,9 @@ async def _do_insert( duration_ms=duration_ms, ) except HTTPException: + shutil.rmtree(tmp_dir, ignore_errors=True) await metric_increment("failures.pdf_insert") record_conversion("pdf_insert", "pdf", "pdf", "failure") - shutil.rmtree(tmp_dir, ignore_errors=True) raise except BaseException: shutil.rmtree(tmp_dir, ignore_errors=True) diff --git a/app/core/responses.py b/app/core/responses.py index 2d3b82b..fb1e9da 100644 --- a/app/core/responses.py +++ b/app/core/responses.py @@ -14,12 +14,14 @@ class TempDirFileResponse(FileResponse): - """Send ``path``, then remove ``tmp_dir`` — the request's temp dir that - holds the result and the upload — however the response ends. + """Send ``path``, then remove ``tmp_dir`` — the request's temp dir, which + by now holds only the result (the routes delete the upload once the + result exists) — however the response ends. A ``background`` task runs only after a complete send: a send that fails (a server reporting the client gone) or is cancelled (a middleware or a - shutdown stopping the response) would leave both on disk until the sweep. + shutdown stopping the response) would leave the result on disk until the + sweep. The removal sits in a ``finally`` instead, shielded so that a cancelled response still finishes it. """ diff --git a/changelog.d/2026-10-29-upload-deleted-early.md b/changelog.d/2026-10-29-upload-deleted-early.md new file mode 100644 index 0000000..a32119f --- /dev/null +++ b/changelog.d/2026-10-29-upload-deleted-early.md @@ -0,0 +1,10 @@ +### Changed — the upload is deleted as soon as the result exists + +Single-file conversions and compressions, page extraction and PDF compression +now delete the uploaded file — the copy in the request's temporary folder and +the web framework's own — as soon as the result is written, instead of +together with the result when the response ends. Inserting pages already +deleted its own copies early; it, splitting and redaction now release the +framework's copy early too, and a PDF → PDF conversion writes its result over +the upload. Batch requests release the framework's copy of each file as soon +as it is copied. diff --git a/docs/architecture.md b/docs/architecture.md index 2f4e282..b3cc987 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -109,10 +109,12 @@ Every conversion or compression request follows the same path: written (PDF compression: 1 GiB), with their temporary files in the request's own temp folder. 7. **Output stream + temp cleanup.** Converted bytes are returned via a - streaming response. The request's UUID-named temp dir — the upload, the - result and LibreOffice's scratch files — is removed when the response - ends, sent in full or cut off (`app/core/responses.py`), or in the - request's `finally` block when the conversion fails. Ghostscript's PDF/A + streaming response. The upload is deleted as soon as the result exists, + together with the web framework's own copy of it (PDF → PDF writes its + result over the upload). The request's UUID-named temp dir + — the result and LibreOffice's scratch files — is removed when the + response ends, sent in full or cut off (`app/core/responses.py`), or in + the request's `finally` block when the conversion fails. Ghostscript's PDF/A run keeps its scratch files in an `fm_gs_*` dir of its own, removed after the run. A sweep on app start and every ten minutes deletes any `fm_*` temp dir older than ten minutes (a worker killed mid-request). diff --git a/docs/security-overview.md b/docs/security-overview.md index 6128d74..df8d2df 100644 --- a/docs/security-overview.md +++ b/docs/security-overview.md @@ -561,10 +561,12 @@ mechanically. to disk under the original filename. - Where a temp path is needed for a multi-step pipeline, the path uses a `fm_`-prefixed directory and a UUID stem. -- Cleanup removes the request's temp dir when the response ends — sent - in full or cut off — and in the request's `finally` block when the - conversion fails. LibreOffice and Ghostscript keep their own temporary - files in that dir, or in an `fm_`-prefixed one of their own. +- The upload is deleted as soon as the result exists, together with the + web framework's own copy of it. Cleanup removes the request's temp + dir, with the result, when the response ends — sent in full or cut + off — and in the request's `finally` block when the conversion fails. + LibreOffice and Ghostscript keep their own temporary files in that + dir, or in an `fm_`-prefixed one of their own. - A startup sweep, plus a periodic background sweep (`TEMP_SWEEP_INTERVAL_MINUTES`, default 10 min), removes any `fm_`-prefixed directory older than `TEMP_SWEEP_MAX_AGE_MINUTES` diff --git a/tests/test_temp_cleanup.py b/tests/test_temp_cleanup.py index bf97e1c..618c6a2 100644 --- a/tests/test_temp_cleanup.py +++ b/tests/test_temp_cleanup.py @@ -15,9 +15,11 @@ import pytest from PIL import Image from pypdf import PdfWriter +from starlette import formparsers from app.api.routes import compress as compress_route from app.api.routes import convert as convert_route +from app.api.routes import pdf_pages as pdf_route from app.core import quotas as quotas_module from app.core.audit import AuditWriteError from app.core.quotas import QUOTAS @@ -150,9 +152,105 @@ async def audit_down(*args, **kwargs): marks=_NEEDS_PIKEPDF, ), pytest.param("/api/v1/pdf/insert", {"file": "pdf", "insert_file": "pdf"}, {}, id="pdf-insert"), + # The one conversion whose result has the upload's name: written over it. + pytest.param( + "/api/v1/convert", {"file": "pdf"}, {"target_format": "pdf"}, id="convert-pdf-to-pdf" + ), ] +@pytest.fixture +def spools(monkeypatch): + """Every copy of an upload part the framework makes (a SpooledTemporaryFile, + on disk above 1 MB).""" + made: list = [] + + class RecordingSpool(formparsers.SpooledTemporaryFile): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + made.append(self) + + monkeypatch.setattr(formparsers, "SpooledTemporaryFile", RecordingSpool) + return made + + +@pytest.fixture +def answer_state(monkeypatch, spools): + """What is on disk when a file route answers: the result's name, the + names in the request dir, and (rolled to disk, closed) for each of the + framework's copies of the uploads.""" + seen: list = [] + + class Recording(TempDirFileResponse): + def __init__(self, path, *args, tmp_dir, **kwargs): + names = sorted(p.name for p in Path(tmp_dir).iterdir()) + copies = [(spool._rolled, spool.closed) for spool in spools] + seen.append((Path(path).name, names, copies)) + super().__init__(path, *args, tmp_dir=tmp_dir, **kwargs) + + for route in (convert_route, compress_route, pdf_route): + monkeypatch.setattr(route, "TempDirFileResponse", Recording) + return seen + + +@pytest.mark.parametrize(("endpoint", "uploads", "data"), _FILE_ROUTES) +def test_only_the_result_is_left_when_the_response_starts( + client, auth_headers, answer_state, endpoint, uploads, data +): + """The upload goes as soon as the result exists, not when the download + ends: the route's copy in the request dir, and the framework's own.""" + r = client.post( + endpoint, + headers=auth_headers, + files={field: _UPLOADS[kind]() for field, kind in uploads.items()}, + data=data, + ) + assert r.status_code == 200, r.text + [(result, names, copies)] = answer_state + assert names == [result] + assert copies and all(closed for _rolled, closed in copies), copies + + +def test_a_large_upload_on_disk_is_closed_before_the_response(client, auth_headers, answer_state): + """Above 1 MB the framework keeps its copy on disk, not in memory.""" + big = _jpg_bytes() + bytes(1_200_000) # past the JPEG's end: ignored by the decoder + r = client.post( + "/api/v1/convert", + headers=auth_headers, + files={"file": ("big.jpg", big, "image/jpeg")}, + data={"target_format": "png"}, + ) + assert r.status_code == 200, r.text + [(_result, _names, copies)] = answer_state + assert copies == [(True, True)] + + +@pytest.mark.parametrize( + ("route", "endpoint", "data"), + [ + (convert_route, "/api/v1/convert/batch", {"target_formats": ["png"]}), + (compress_route, "/api/v1/compress/batch", {"quality": "85"}), + ], + ids=["convert", "compress"], +) +def test_a_batch_closes_each_upload_before_its_zip( + client, auth_headers, monkeypatch, spools, route, endpoint, data +): + closed_at_zip: list[bool] = [] + real = route.build_batch_zip + + def spy(*args, **kwargs): + closed_at_zip.extend(spool.closed for spool in spools) + return real(*args, **kwargs) + + monkeypatch.setattr(route, "build_batch_zip", spy) + # One file: the test client's tier takes one per batch. + files = [("files", ("a.jpg", _jpg_bytes(), "image/jpeg"))] + r = client.post(endpoint, headers=auth_headers, files=files, data=data) + assert r.status_code == 200, r.text + assert closed_at_zip == [True] + + @pytest.mark.parametrize(("endpoint", "uploads", "data"), _FILE_ROUTES) def test_a_download_cut_off_by_the_server_leaves_no_temp_dir( client, auth_headers, endpoint, uploads, data