From 7727ad76169f0b9b0f1927b50c6f113fbf43fbbd Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Wed, 26 Aug 2026 06:01:06 +0200 Subject: [PATCH] Say what `exists:folders,id` was already being read as Folder uses SoftDeletes. The `exists` rule runs against the table, so a folder in the trash passes it -- while every resolution that follows goes through Folder::query(), which honours the soft delete and finds nothing. Ten rules across five controllers rely on that check, and each one reads it as "this folder exists". Two of them then wrote the id anyway. Api\FilesController::store() resolves the folder, hands the null to Folder::uploadableBy(), is told yes -- correctly, that is the rule for a root upload -- and passes $validated['folder_id'] to the write. FilesController::store() is the same shape once #1694 gives it the guard. FilesController::update() and its API twin write it straight through with nothing in between. The result is a live file inside a deleted folder, which is a state nothing else in the application produces: FolderService::delete() deletes every file in the subtree along with it. The row is reachable by id, in search and over the API, and missing from the listing its uploader would look in. Rules::folderId() makes the check mean what its readers assume, once, where the reasoning can be written down -- the same argument slug() makes for itself one method above. Every site takes it, so the file cannot end up with two spellings of the same rule and no way to tell which is the safe one. What changes, path by path: - POST /files, POST /api/v1/files, PATCH /files/{file} and PATCH /api/v1/files/{file} refuse a folder in the trash instead of writing its id. This is the fix. - POST /uploads used to accept it and quietly file the upload at the root -- its guard and its write already agreed, on null. It now says so instead, which is what the other upload paths do. - files/{file}/move, files/bulk-edit, folders, folders/{folder}/move and the portal's my-folders already refused, through StaffLibraryScope::folders() or Folder::scopeVisibleToClient(), both of which drop trashed rows. They still refuse; the answer is now 422 naming folder_id rather than a bare 404. Those two guards are asking a different question -- "is this folder yours" -- and they keep asking it. No live folder id behaves differently anywhere, and the root (a null folder_id) is untouched. The published API document is unchanged: `exists` renders the same either way. Regenerated with php artisan scramble:export and byte-identical. --- .../Http/Controllers/Api/FilesController.php | 4 +- .../Controllers/ChunkedUploadsController.php | 3 +- .../Http/Controllers/FilesController.php | 8 +- .../Http/Controllers/FoldersController.php | 4 +- .../Http/Controllers/MyFoldersController.php | 3 +- app/Support/Rules.php | 34 +++++ .../Feature/Files/DeletedFolderTargetTest.php | 141 ++++++++++++++++++ 7 files changed, 187 insertions(+), 10 deletions(-) create mode 100644 tests/Feature/Files/DeletedFolderTargetTest.php diff --git a/app/Modules/Files/Http/Controllers/Api/FilesController.php b/app/Modules/Files/Http/Controllers/Api/FilesController.php index a8951f0c..7362aaad 100644 --- a/app/Modules/Files/Http/Controllers/Api/FilesController.php +++ b/app/Modules/Files/Http/Controllers/Api/FilesController.php @@ -174,7 +174,7 @@ class FilesController extends Controller 'file' => ['required', 'file'], 'name' => ['nullable', 'string', 'max:255'], 'description' => ['nullable', 'string', 'max:2000'], - 'folder_id' => ['nullable', 'integer', 'exists:folders,id'], + 'folder_id' => Rules::folderId(), ]); /** @var UploadedFile $upload */ @@ -275,7 +275,7 @@ class FilesController extends Controller $validated = $request->validate([ 'name' => ['sometimes', 'string', 'max:255'], 'description' => ['sometimes', 'nullable', 'string', 'max:2000'], - 'folder_id' => ['sometimes', 'nullable', 'integer', 'exists:folders,id'], + 'folder_id' => ['sometimes', ...Rules::folderId()], 'public' => ['sometimes', 'boolean'], 'commentable' => ['sometimes', 'boolean'], 'slug' => Rules::slug('files', $file->id), diff --git a/app/Modules/Files/Http/Controllers/ChunkedUploadsController.php b/app/Modules/Files/Http/Controllers/ChunkedUploadsController.php index 1f376bf3..cabcb0a9 100644 --- a/app/Modules/Files/Http/Controllers/ChunkedUploadsController.php +++ b/app/Modules/Files/Http/Controllers/ChunkedUploadsController.php @@ -24,6 +24,7 @@ use App\Modules\Identity\UserType; use App\Modules\Notifications\Notifier; use App\Modules\Platform\Settings\Setting; use App\Modules\Platform\Settings\Settings; +use App\Support\Rules; use Illuminate\Auth\Access\AuthorizationException; use Illuminate\Http\JsonResponse; use Illuminate\Http\RedirectResponse; @@ -71,7 +72,7 @@ class ChunkedUploadsController extends Controller 'size' => ['required', 'integer', 'min:1'], 'type' => ['nullable', 'string', 'max:255'], 'description' => ['nullable', 'string', 'max:2000'], - 'folder_id' => ['nullable', 'integer', 'exists:folders,id'], + 'folder_id' => Rules::folderId(), 'previous_file_id' => ['nullable', 'integer'], ]); diff --git a/app/Modules/Files/Http/Controllers/FilesController.php b/app/Modules/Files/Http/Controllers/FilesController.php index 9d3e0736..b74c2cfd 100644 --- a/app/Modules/Files/Http/Controllers/FilesController.php +++ b/app/Modules/Files/Http/Controllers/FilesController.php @@ -76,7 +76,7 @@ class FilesController extends Controller 'file' => ['required', 'file', 'max:102400'], 'name' => ['nullable', 'string', 'max:255'], 'description' => ['nullable', 'string', 'max:2000'], - 'folder_id' => ['nullable', 'integer', 'exists:folders,id'], + 'folder_id' => Rules::folderId(), ]); /** @var UploadedFile $upload */ @@ -242,7 +242,7 @@ class FilesController extends Controller $validated = $request->validate([ 'name' => ['required', 'string', 'max:255'], 'description' => ['nullable', 'string', 'max:2000'], - 'folder_id' => ['nullable', 'integer', 'exists:folders,id'], + 'folder_id' => Rules::folderId(), 'public' => ['sometimes', 'boolean'], 'commentable' => ['sometimes', 'boolean'], // The slug only matters (and is only shown) once a file is @@ -327,7 +327,7 @@ class FilesController extends Controller Gate::authorize('update', $file); $validated = $request->validate([ - 'folder_id' => ['nullable', 'integer', 'exists:folders,id'], + 'folder_id' => Rules::folderId(), ]); $folderId = $validated['folder_id'] ?? null; @@ -363,7 +363,7 @@ class FilesController extends Controller 'file_ids.*' => ['integer', 'distinct'], 'folder_action' => ['required', Rule::in(['no_change', 'move'])], - 'folder_id' => ['nullable', 'integer', 'exists:folders,id'], + 'folder_id' => Rules::folderId(), 'description_action' => ['required', Rule::in(['no_change', 'set'])], 'description' => ['nullable', 'string', 'max:2000'], diff --git a/app/Modules/Files/Http/Controllers/FoldersController.php b/app/Modules/Files/Http/Controllers/FoldersController.php index de25005e..424a15c7 100644 --- a/app/Modules/Files/Http/Controllers/FoldersController.php +++ b/app/Modules/Files/Http/Controllers/FoldersController.php @@ -305,7 +305,7 @@ class FoldersController extends Controller $validated = $request->validate([ 'name' => ['required', 'string', 'max:255'], - 'parent_id' => ['nullable', 'integer', 'exists:folders,id'], + 'parent_id' => Rules::folderId(), 'public' => ['sometimes', 'boolean'], 'slug' => Rules::slug('folders'), 'allow_client_uploads' => ['sometimes', 'boolean'], @@ -389,7 +389,7 @@ class FoldersController extends Controller Gate::authorize('update', $folder); $validated = $request->validate([ - 'parent_id' => ['nullable', 'integer', 'exists:folders,id'], + 'parent_id' => Rules::folderId(), ]); $newParent = $this->resolveParent($request->user(), $validated['parent_id'] ?? null); diff --git a/app/Modules/Files/Http/Controllers/MyFoldersController.php b/app/Modules/Files/Http/Controllers/MyFoldersController.php index 793e2d76..7b490f8a 100644 --- a/app/Modules/Files/Http/Controllers/MyFoldersController.php +++ b/app/Modules/Files/Http/Controllers/MyFoldersController.php @@ -10,6 +10,7 @@ use App\Modules\Audit\ActivityLogger; use App\Modules\Files\Folders\FolderService; use App\Modules\Files\Models\File; use App\Modules\Files\Models\Folder; +use App\Support\Rules; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Illuminate\Support\Facades\Gate; @@ -43,7 +44,7 @@ class MyFoldersController extends Controller $validated = $request->validate([ 'name' => ['required', 'string', 'max:255'], - 'parent_id' => ['nullable', 'integer', 'exists:folders,id'], + 'parent_id' => Rules::folderId(), ]); $parent = null; diff --git a/app/Support/Rules.php b/app/Support/Rules.php index 37a7380d..67fb5de4 100644 --- a/app/Support/Rules.php +++ b/app/Support/Rules.php @@ -50,6 +50,40 @@ class Rules ]; } + /** + * The rule for an id naming a library folder. + * + * Shared because the plain `exists:folders,id` it replaces is not + * true: Folder uses SoftDeletes, and the presence check runs against + * the table, so a folder in the trash passes it. Every caller then + * reads the rule as "this folder exists" and behaves accordingly — + * and the ones that resolve the id afterwards resolve it through + * Folder::query(), which does honour the soft delete, so the guard + * sees no folder at all while the value that reaches the write is + * still the id. + * + * FilesController::store() and Api\FilesController::store() ended up + * filing an upload into a deleted folder that way: the guard read + * null and allowed it as a root upload, and the row was written with + * the id. Deleting a folder deletes every file in its subtree, so + * that is a live file inside a folder whose deletion already removed + * everything in it — reachable by id, in search and over the API, + * and absent from the listing its uploader would look in. + * + * Making the rule mean what its readers already assume fixes those + * and leaves the paths that resolve through StaffLibraryScope alone; + * they refuse a trashed id today by a longer route. + * + * Presence is the caller's business, as with slug() above: spread it + * behind `sometimes` where a PATCH may omit the field. + * + * @return array + */ + public static function folderId(): array + { + return ['nullable', 'integer', Rule::exists('folders', 'id')->whereNull('deleted_at')]; + } + /** * The rule for an IANA timezone identifier. * diff --git a/tests/Feature/Files/DeletedFolderTargetTest.php b/tests/Feature/Files/DeletedFolderTargetTest.php new file mode 100644 index 00000000..63cbad13 --- /dev/null +++ b/tests/Feature/Files/DeletedFolderTargetTest.php @@ -0,0 +1,141 @@ +admin = User::factory()->create(); + + $folder = app(FolderService::class)->create('Doomed', null); + $this->trashedId = $folder->id; + app(FolderService::class)->delete($folder); + + expect(Folder::query()->whereKey($this->trashedId)->exists())->toBeFalse() + ->and(Folder::withTrashed()->whereKey($this->trashedId)->exists())->toBeTrue(); +}); + +test('a web upload cannot be filed into a deleted folder', function () { + $this->actingAs($this->admin)->post('/files', [ + 'file' => UploadedFile::fake()->create('b.txt', 1, 'text/plain'), + 'folder_id' => $this->trashedId, + ])->assertSessionHasErrors('folder_id'); + + expect(File::query()->where('original_name', 'b.txt')->exists())->toBeFalse(); +}); + +test('an API upload cannot be filed into a deleted folder', function () { + $token = $this->admin->createToken('t', ['upload'])->plainTextToken; + + $this->withToken($token)->postJson('/api/v1/files', [ + 'file' => UploadedFile::fake()->create('c.txt', 1, 'text/plain'), + 'folder_id' => $this->trashedId, + ])->assertJsonValidationErrors('folder_id'); + + expect(File::query()->where('original_name', 'c.txt')->exists())->toBeFalse(); +}); + +test('a file already in the library cannot be moved into a deleted folder', function () { + $file = uploadNamedFile($this->admin, 'movable'); + + $this->actingAs($this->admin) + ->patch("/files/{$file->id}", ['name' => 'movable', 'folder_id' => $this->trashedId]) + ->assertSessionHasErrors('folder_id'); + + expect($file->fresh()->folder_id)->toBeNull(); +}); + +test('the API cannot move one there either', function () { + $file = uploadNamedFile($this->admin, 'api-movable'); + $token = $this->admin->createToken('t', ['edit_files'])->plainTextToken; + + $this->withToken($token) + ->patchJson("/api/v1/files/{$file->id}", ['folder_id' => $this->trashedId]) + ->assertJsonValidationErrors('folder_id'); + + expect($file->fresh()->folder_id)->toBeNull(); +}); + +test('a chunked upload says so up front instead of quietly using the root', function () { + $this->actingAs($this->admin)->postJson('/uploads', [ + 'filename' => 'a.txt', + 'size' => 5, + 'folder_id' => $this->trashedId, + ])->assertJsonValidationErrors('folder_id'); + + expect(UploadSession::query()->count())->toBe(0); +}); + +test('the paths that resolve through the library scope keep refusing too', function () { + $file = uploadNamedFile($this->admin, 'movable'); + + $this->actingAs($this->admin) + ->patch("/files/{$file->id}/move", ['folder_id' => $this->trashedId]) + ->assertSessionHasErrors('folder_id'); + + $this->actingAs($this->admin)->patch('/files/bulk-edit', [ + 'file_ids' => [$file->id], + 'folder_action' => 'move', + 'folder_id' => $this->trashedId, + 'description_action' => 'no_change', + 'expiration_action' => 'no_change', + ])->assertSessionHasErrors('folder_id'); + + expect($file->fresh()->folder_id)->toBeNull(); +}); + +test('a folder cannot be created inside, or moved into, a deleted one', function () { + $this->actingAs($this->admin) + ->post('/folders', ['name' => 'Orphan', 'parent_id' => $this->trashedId]) + ->assertSessionHasErrors('parent_id'); + + $folder = app(FolderService::class)->create('Live', null); + + $this->actingAs($this->admin) + ->patch("/folders/{$folder->id}/move", ['parent_id' => $this->trashedId]) + ->assertSessionHasErrors('parent_id'); + + expect(Folder::query()->where('name', 'Orphan')->exists())->toBeFalse() + ->and($folder->fresh()->parent_id)->toBeNull(); +}); + +test('a live folder is still a perfectly good upload target', function () { + $live = app(FolderService::class)->create('Live', null); + + $this->actingAs($this->admin)->post('/files', [ + 'file' => UploadedFile::fake()->create('ok.txt', 1, 'text/plain'), + 'folder_id' => $live->id, + ])->assertSessionHasNoErrors(); + + expect(File::query()->where('original_name', 'ok.txt')->value('folder_id'))->toBe($live->id); + + $this->actingAs($this->admin)->postJson('/uploads', [ + 'filename' => 'chunk.txt', + 'size' => 5, + 'folder_id' => $live->id, + ])->assertOk(); + + expect(UploadSession::query()->value('folder_id'))->toBe($live->id); +}); + +test('the root is still the root', function () { + $this->actingAs($this->admin)->post('/files', [ + 'file' => UploadedFile::fake()->create('root.txt', 1, 'text/plain'), + 'folder_id' => null, + ])->assertSessionHasNoErrors(); + + expect(File::query()->where('original_name', 'root.txt')->value('folder_id'))->toBeNull(); +});