gsadmin/projectsend
1
0
Fork 0
mirror of https://github.com/projectsend/projectsend.git synced 2026-09-16 16:45:07 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #1703 from denkfabrik-li/fix/deleted-folder-upload-target

Browse Source 
Say what `exists:folders,id` was already being read as
This commit is contained in:
Ignacio Nelson
2026-08-26 17:35:34 -03:00
committed by GitHub
parent 93d5b21c6a 7727ad7616
commit 8d896191e4
7 changed files with 187 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/Modules/Files/Http/Controllers/Api/FilesController.php
+2 -2
View File
@@ -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),
app/Modules/Files/Http/Controllers/ChunkedUploadsController.php
+2 -1
View File
@@ -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'],
]);
app/Modules/Files/Http/Controllers/FilesController.php
+4 -4
View File
@@ -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'],
app/Modules/Files/Http/Controllers/FoldersController.php
+2 -2
View File
@@ -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);
app/Modules/Files/Http/Controllers/MyFoldersController.php
+2 -1
View File
@@ -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;
app/Support/Rules.php
+34
View File
@@ -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<int, mixed>
*/
public static function folderId(): array
{
return ['nullable', 'integer', Rule::exists('folders', 'id')->whereNull('deleted_at')];
}
/**
* The rule for an IANA timezone identifier.
*
tests/Feature/Files/DeletedFolderTargetTest.php
+141
View File
@@ -0,0 +1,141 @@
<?php
declare(strict_types=1);
use App\Models\User;
use App\Modules\Files\Folders\FolderService;
use App\Modules\Files\Models\File;
use App\Modules\Files\Models\Folder;
use App\Modules\Files\Uploads\UploadSession;
use Illuminate\Http\UploadedFile;
use Illuminate\Support\Facades\Storage;
/**
* Folder uses SoftDeletes, so `exists:folders,id` — which runs against
* the table — passes for a folder in the trash, while every resolution
* that follows goes through Folder::query() and finds nothing. The
* upload paths wrote the id anyway.
*/
beforeEach(function () {
Storage::fake('files');
$this->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();
});