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(); +});