Close the two-request version of the deleted-folder target, and say why it failed

Follow-up to #1703, which made `exists:folders,id` mean what its ten
readers already assumed. Two things it named and deliberately left.

**The chunked upload is two requests.** store()'s rule only ever sees the
first: POST /uploads records the resolved folder on the UploadSession and
complete() reads it back from the session rather than from the caller, so
deleting the folder while the bytes are in flight still files the
assembled file into it -- the same orphan state #1703 removes, reached by
a door a validation rule cannot watch. complete() now re-resolves through
Folder::query() and files at the root when the folder has gone.

Root rather than a refusal, because the two moments cost different
things. At store() nothing has been sent, so refusing is free and honest,
which is the call #1703 made. Here the bytes are already uploaded, and
discarding 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.

**The refusal now explains itself.** "The selected folder id is invalid"
says nothing when the answer is that the folder has been deleted -- and
that is the usual way to meet this rule, since a live id picked from a
list is how anybody gets here. It matters most on the chunked path, the
one place #1703 makes a previously-working request fail. A small
ValidationRule object carries the message, which keeps the single
definition Rules::folderId() exists for: a messages() array would have to
be repeated at all ten call sites, and rules meaning different things in
ten places is what went wrong in the first place.

One note for whoever writes the next test here. Upload parts live in
storage_path('app/uploads-tmp/{session_id}'), which is a real shared
directory rather than a faked disk, and each parallel worker's database
restarts session ids at 1 -- so two files writing parts on two workers
collide, and ChunkedUploadsTest's afterEach deletes the whole tree for
everybody. Six test files write parts today. These two cases live in
ChunkedUploadsTest rather than beside the rest of their subject so this
change does not add a seventh racer; the underlying isolation problem
predates it and is worth its own fix.
This commit is contained in:
ignacionelson
2026-08-26 17:41:07 -03:00
parent 8d896191e4
commit d7e639b7af
6 changed files with 155 additions and 2 deletions
+10
View File
@@ -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
@@ -0,0 +1,54 @@
<?php
declare(strict_types=1);
namespace App\Modules\Files\Folders;
use App\Modules\Files\Models\Folder;
use Closure;
use Illuminate\Contracts\Validation\ValidationRule;
/**
* An id naming a folder that is actually there.
*
* A rule object rather than `Rule::exists(...)->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.'));
}
}
@@ -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'],
);
+5 -1
View File
@@ -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<int, mixed>
*/
public static function folderId(): array
{
return ['nullable', 'integer', Rule::exists('folders', 'id')->whereNull('deleted_at')];
return ['nullable', 'integer', new FolderExistsRule];
}
/**
@@ -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);
});
@@ -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.');
});