Repository navigation
fix(privacy): the upload goes as soon as the result exists — not when the response ends - #300
Merged
Merged
Conversation
… the response ends
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The upload goes as soon as the result exists, not when the response ends. 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./convert,/compress,/pdf/extractand/pdf/compressdelete the upload right after the work succeeds, as/pdf/insertalready did./convertskips that for pdf → pdf, the one identity pair, whose result is written over the upload./convertand/compress, single and batch:await file.close();upload.file.close()in_write_upload_and_check_magic;await file.close()after its read.pdf_pageshandlers for anHTTPExceptionnowrmtreefirst, per the security rule.responses.py's docstring,architecture.mdandsecurity-overview.mdsay what stays on disk.Tests
In
tests/test_temp_cleanup.py:_FILE_ROUTES, now withconvert-pdf-to-pdf): when the response is built, the request dir holds exactly the result, and every framework copy is closed. Onmainthis fails for convert, compress and extract.Checks
test_stripe_ca_bundle, depends on the local certifi and fails on unchangedmaintoo.🤖 Generated with Claude Code