Repository navigation
refactor(http): harden multipart parsing and upload lifecycle - #6309
Merged
Merged
Conversation
The large-upload preprocessor rebuilds multipart bodies and adds Swoole-Upload-File after saving a file part. A client-provided copy was previously written into that rebuilt body and later treated as generated upload metadata. Reject the reserved header while parsing client input and return a bad request. Log the specific cause through the existing invalid-request error code so operators can suppress repeated client errors. Rejection after an earlier file part follows the parser's existing abort cleanup and can leave that earlier temporary path behind. That needs per-request path ownership and remains separate from this focused fix. The callback warning provides the useful reason; the existing generic parser notice may remain empty rather than adding a new multipart parser error code here. (cherry picked from commit 8cf1af1)
Force the large-upload preprocessor and send the reserved header both in the first file part and after a completed file part. Both requests must return a bad request without reaching the application handler or registering the supplied paths as uploaded files. Suppress partial-write warnings because an early rejection may close the connection before the client finishes sending. Use an isolated upload directory and remove existing abort-path files after the assertions without making that separate cleanup behavior part of this test. (cherry picked from commit de11e2d)
Keep generated upload paths owned by the reactor until request dispatch succeeds. Remove them when parsing, size validation, or dispatch fails. Return a service-unavailable response when temporary-file setup fails, and reject malformed multipart boundaries before the parser can be dereferenced. (cherry picked from commit 1224813)
Cover malformed boundaries, reserved-header orderings, temporary-file setup failure, rebuilt-body limits, worker survival, and cleanup of every generated upload path. Use the logged rebuilt content length to prove the size-limit case passed through the multipart preprocessor. (cherry picked from commit 1850907)
BASE mode may run the request handler inside dispatch_request() and destroy the reactor-side Request before the call returns. Moving its temporary-path list afterward therefore accessed freed memory. Move the path list before dispatch, remove the generated files when dispatch fails, and cover successful preprocessed upload dispatch and cleanup in BASE mode. (cherry picked from commit 74e4302)
Do not create an upload temporary file or internal marker for filename="" while preprocessing a request containing another large file. Leave the empty part for the PHP multipart consumer so it retains UPLOAD_ERR_NO_FILE. Also drop untested fdopen failure handling that was added earlier to this pull request without a reproducible path. (cherry picked from commit e68ceb1)
Force the large-upload preprocessing path with one real file and verify that a second empty file input keeps its normal error code and empty temporary path. (cherry picked from commit 4fe52f6)
Remove per-PR package.xml entries for the added PHPTs. Swoole updates the packaged test list in release commits rather than in individual bug fixes. (cherry picked from commit e349638)
Carry the preprocessed-request state through server dispatch and only interpret internal upload markers on that path. Reject and unlink unmatched generated paths, and release temporary parser state on malformed metadata exits. (cherry picked from commit 962c8b2)
Cover ordinary and coroutine request paths, valid preprocessed uploads, malformed generated markers, temporary-file cleanup, and worker survival. (cherry picked from commit 1341a90)
Key wrapper uploads by their multipart field name and reject generated markers without matching file metadata. Remove unmatched and duplicate generated paths without stopping multipart parsing, so later upload files remain visible for cleanup. (cherry picked from commit 05705ea)
Assert direct and preprocessed wrapper uploads use the expected field key. Cover nameless and duplicate generated paths, including continued parsing and temporary-file cleanup in the C++ and PHP paths. (cherry picked from commit 9e37f2d)
Buffer fragmented multipart header names and values until each header is complete, propagate parser callback errors, and reject incomplete bodies once their declared length has been consumed. Reset derived state for repeated Content-Type fields, close temporary upload handles before cleanup, and allow large-upload preprocessing on Windows. (cherry picked from commit 2f58ba2)
Cover fragmented header fields and values, parser callback failures, repeated Content-Type replacement, incomplete bodies, valid split closing boundaries, and oversized upload preprocessing. (cherry picked from commit ca0e3f1)
Exercise split untrusted upload markers in manual parsing and preprocessing. Verify fragmented uploads and truncated-body cleanup in BASE and PROCESS modes, and adapt the rebuilt-body limit assertion to earlier validation.
Move temporary upload path tracking from Request to FormData to avoid increasing ordinary HTTP request size. Retain multipart state through dispatch and preserve cleanup on failure.
Collect preprocessed upload paths independently of PHP metadata parsing and clean up rejected or interrupted uploads. Reject duplicate dispositions, check buffered file errors, and share bounded multipart header handling. Keep multipart state lazy, reject incomplete HTTP/2 multipart streams, and add regression coverage for cleanup, dispatch failures, header limits, and file descriptor leaks.
Use the platform temporary directory instead of hardcoding /tmp in Request::create(). Cover default and explicit upload directories and unfinished upload cleanup on Windows.
Replace short sleeps and random worker lifetimes with queue handshakes. Keep workers alive during status assertions and bound the detached thread exit wait.
This was referenced Oct 16, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6309 +/- ##
==========================================
+ Coverage 83.72% 83.81% +0.08%
==========================================
Files 114 114
Lines 18771 18939 +168
Branches 3347 3381 +34
==========================================
+ Hits 15716 15873 +157
- Misses 3055 3066 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ChiragAgg5k
added a commit
to ChiragAgg5k/appwrite
that referenced
this pull request
Oct 23, 2026
It only covers Swoole builds without the swoole/swoole-src#6309 fix.
ChiragAgg5k
added a commit
to appwrite/appwrite
that referenced
this pull request
Oct 24, 2026
…pter Swoole's multipart parser honors an internal `Swoole-Upload-File` part header and pins part metadata to a client-chosen `tmp_name` with `error: 0` and the real size of the target file (swoole/swoole-src#6309). Because `is_uploaded_file()` trusts Swoole metadata, a remote unauthenticated request can make upload-handling code read an arbitrary server-local file (e.g. /etc/passwd) as if it were a standard multipart upload. No released Swoole version contains the upstream fix yet (`upload_preprocessed` lands with 6.3/master). The Swoole adapter is the single choke point: genuine uploads are always written to `<upload_tmp_dir>/swoole.upfile.XXXXXX`, so getFiles() now rejects any `tmp_name` outside those directories/that naming scheme, neutralizing spoofed entries into UPLOAD_ERR_CANT_WRITE with no size, while real uploads (flat or bracketed list form) pass through untouched. Parser-level regression tests cover the spoofed marker through SwooleRequest::parse(), genuine uploads, mixed lists, wrong-dir and nonexistent paths.
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.
Summary
Integrate and refine the multipart upload changes from #6161, #6162, and #6154 for
master.This is a substantial refactor intended for
master; it should not be backported wholesale to6.1or6.2.Validation
An earlier broader local HTTP/2 run also exposed a
session_shutdown.phptdebug assertion, reproduced with the pre-fix extension; this was independent of these changes.Merge strategy
Squash merge into
master.