diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ddf4ec0..e354bb3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -255,6 +255,16 @@ a version is cut. (found, diagnosed and fixed by [@denkfabrik-li](https://github.com/denkfabrik-li) in [#1704](https://github.com/projectsend/projectsend/pull/1704)) +- **A file can no longer be filed into a folder that has been deleted.** Deleting a folder deletes + everything inside it, so a file that lands in one afterwards sits somewhere that was already + emptied — reachable by link and in search, but missing from the folder listing its uploader would + look in. Uploading or moving a file into a deleted folder now says so instead, and picks up the + case where a folder is deleted while a large upload is still transferring: the finished file lands + at the top level rather than being thrown away, since the transfer had already happened. The + message says the folder no longer exists rather than that the value was invalid. + (found, diagnosed and fixed by [@denkfabrik-li](https://github.com/denkfabrik-li) in + [#1703](https://github.com/projectsend/projectsend/pull/1703)) + ## 2.1.0 — 18 August 2026 Updating, mostly. ProjectSend now tells you when there is a new version, ends an update somewhere diff --git a/app/Modules/Files/Folders/FolderExistsRule.php b/app/Modules/Files/Folders/FolderExistsRule.php new file mode 100644 index 00000000..68b3dc32 --- /dev/null +++ b/app/Modules/Files/Folders/FolderExistsRule.php @@ -0,0 +1,54 @@ +whereNull('deleted_at')` + * for one reason: the message. The generic form says "The selected folder + * id is invalid", which tells somebody nothing when the real answer is + * that the folder they picked has since been deleted — and that is the + * usual way to meet this rule, since a live id they chose from a list is + * how they got here. It matters most on the chunked upload path, which is + * the one place a request that used to succeed now fails. + * + * Carrying the message on the rule keeps the single definition + * Rules::folderId() exists for: a `messages()` array would have to be + * repeated at every call site, which is how the plain `exists:folders,id` + * it replaced came to mean two different things in ten places. + */ +class FolderExistsRule implements ValidationRule +{ + /** + * @param Closure(string, string|null=): \Illuminate\Translation\PotentiallyTranslatedString $fail + */ + public function validate(string $attribute, mixed $value, Closure $fail): void + { + if ($value === null || $value === '') { + return; + } + + if (! is_numeric($value)) { + // Reached only when a caller drops `integer`; the message + // still has to make sense to whoever sees it. + $fail(__('That folder could not be found.')); + + return; + } + + // Folder::query() honours the soft delete, which is the whole + // point — the table-level `exists` rule this replaces does not. + if (Folder::query()->whereKey((int) $value)->exists()) { + return; + } + + $fail(__('That folder no longer exists. Pick another one and try again.')); + } +} diff --git a/app/Modules/Files/Http/Controllers/ChunkedUploadsController.php b/app/Modules/Files/Http/Controllers/ChunkedUploadsController.php index cabcb0a9..a72d81bc 100644 --- a/app/Modules/Files/Http/Controllers/ChunkedUploadsController.php +++ b/app/Modules/Files/Http/Controllers/ChunkedUploadsController.php @@ -273,6 +273,23 @@ class ChunkedUploadsController extends Controller // the previewer's browser. Detect the real mime type from the assembled bytes. $mimeType = Storage::disk($assembled['disk'])->mimeType($assembled['path']) ?: 'application/octet-stream'; + // Re-resolved rather than taken from the session. A chunked + // upload is two requests, and store()'s rule only ever sees the + // first: delete the folder while the bytes are in flight and the + // recorded id names a folder whose own deletion already removed + // every file in it. Filing into it would recreate exactly the + // state Rules::folderId() exists to prevent. + // + // The root, rather than a refusal, because the two moments cost + // different things. At store() nothing has been sent, so refusing + // is free and honest. Here the bytes are already uploaded, and + // throwing away somebody's finished transfer over a folder that + // vanished underneath them is the harsher of the two surprises — + // the file lands somewhere they can see it and move it. + $folderId = $session->folder_id !== null && Folder::query()->whereKey($session->folder_id)->exists() + ? $session->folder_id + : null; + $file = $this->storeFile->create( uploader: $user, originalName: $session->original_name, @@ -281,7 +298,7 @@ class ChunkedUploadsController extends Controller size: $assembled['size'], checksum: $assembled['checksum'], description: $session->description, - folderId: $session->folder_id, + folderId: $folderId, disk: $assembled['disk'], ); diff --git a/app/Support/Rules.php b/app/Support/Rules.php index 67fb5de4..9506639c 100644 --- a/app/Support/Rules.php +++ b/app/Support/Rules.php @@ -6,6 +6,7 @@ namespace App\Support; use App\Modules\Platform\Captcha\Captcha; use App\Modules\Platform\Captcha\CaptchaForm; +use App\Modules\Files\Folders\FolderExistsRule; use App\Modules\Platform\Captcha\CaptchaRule; use App\Modules\Platform\Localization\TimezoneRegistry; use Illuminate\Validation\Rule; @@ -77,11 +78,14 @@ class Rules * Presence is the caller's business, as with slug() above: spread it * behind `sometimes` where a PATCH may omit the field. * + * A rule object rather than a conditional `exists`, so the refusal can + * say why — see FolderExistsRule. + * * @return array */ public static function folderId(): array { - return ['nullable', 'integer', Rule::exists('folders', 'id')->whereNull('deleted_at')]; + return ['nullable', 'integer', new FolderExistsRule]; } /** diff --git a/tests/Feature/Files/ChunkedUploadsTest.php b/tests/Feature/Files/ChunkedUploadsTest.php index 1959159d..7e6d18b4 100644 --- a/tests/Feature/Files/ChunkedUploadsTest.php +++ b/tests/Feature/Files/ChunkedUploadsTest.php @@ -281,3 +281,54 @@ test('a storage backend that refuses the write fails the upload instead of recor // The point of the whole test: no row for bytes that were never stored. expect(File::query()->count())->toBe($before); }); + +// A chunked upload is two requests, and store()'s rule only ever sees the +// first one. Delete the folder while the bytes are in flight and the +// session still names it -- the version of this that nobody can ask for +// in a single request. +test('a folder deleted mid-upload does not swallow the finished file', function () { + $folder = app(\App\Modules\Files\Folders\FolderService::class)->create('Doomed', null); + + $this->actingAs($this->admin); + + $session = $this->postJson('/uploads', [ + 'filename' => 'report.pdf', + 'size' => 11, + 'type' => 'application/pdf', + 'folder_id' => $folder->id, + ])->assertOk()->json('uploadId'); + + putPart($session, 1, 'hello world')->assertOk(); + + // Somebody empties the folder while the transfer is running. Deleting + // a folder deletes every file in its subtree, so anything filed into + // it afterwards sits inside a folder that was already emptied. + app(\App\Modules\Files\Folders\FolderService::class)->delete($folder); + + $fileId = $this->postJson("/uploads/{$session}/complete")->assertOk()->json('file_id'); + + // The bytes are kept -- they are already uploaded, and discarding + // somebody's finished transfer over a folder that vanished under them + // is the harsher surprise. They land at the root, where the uploader + // can see them and move them. + expect(File::query()->whereKey($fileId)->value('folder_id'))->toBeNull(); +}); + +test('a folder that survives the upload still receives the file', function () { + $folder = app(\App\Modules\Files\Folders\FolderService::class)->create('Fine', null); + + $this->actingAs($this->admin); + + $session = $this->postJson('/uploads', [ + 'filename' => 'report.pdf', + 'size' => 11, + 'type' => 'application/pdf', + 'folder_id' => $folder->id, + ])->assertOk()->json('uploadId'); + + putPart($session, 1, 'hello world')->assertOk(); + + $fileId = $this->postJson("/uploads/{$session}/complete")->assertOk()->json('file_id'); + + expect(File::query()->whereKey($fileId)->value('folder_id'))->toBe($folder->id); +}); diff --git a/tests/Feature/Files/DeletedFolderTargetTest.php b/tests/Feature/Files/DeletedFolderTargetTest.php index 63cbad13..54ca91c0 100644 --- a/tests/Feature/Files/DeletedFolderTargetTest.php +++ b/tests/Feature/Files/DeletedFolderTargetTest.php @@ -139,3 +139,20 @@ test('the root is still the root', function () { expect(File::query()->where('original_name', 'root.txt')->value('folder_id'))->toBeNull(); }); + +// The refusal has to say why: "The selected folder id is invalid" tells +// somebody nothing when the answer is that the folder has been deleted. +test('the refusal explains itself', function () { + $folder = app(FolderService::class)->create('Gone', null); + app(FolderService::class)->delete($folder); + + $this->actingAs($this->admin) + ->postJson('/uploads', [ + 'filename' => 'report.pdf', + 'size' => 11, + 'type' => 'application/pdf', + 'folder_id' => $folder->id, + ]) + ->assertStatus(422) + ->assertJsonPath('errors.folder_id.0', 'That folder no longer exists. Pick another one and try again.'); +});