gsadmin/projectsend
1
0
Fork 0
mirror of https://github.com/projectsend/projectsend.git synced 2026-09-11 22:38:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

Ask the picker's own question of what comes back from it

Browse Source 
Reported by @skeletonsec as GHSA-w29w-pj29-x7ww.

Deleting an account that owns files makes the admin choose who inherits
them. The picker narrows that list for a client-scoped staff member to their
own roster, and says why two methods up: "a client-scoped staff member is
not shown the name of somebody they can reach nothing of, and a picker is no
more a reason to hand one over than a listing is."

The write asked something else entirely — exists, active, and not the
account being deleted. All three are true of every account on the
installation. So a scoped staffer could name an id the picker had
deliberately kept off the list, and a roster client's files and folders
landed with a client on somebody else's roster: readable, editable and
deletable there, because a client owns what they uploaded and
visibleToClient() includes uploaded_by.

The entry doors scope the source account and always did — guardTarget goes
through canAssignClient. It is the destination nobody scoped.

candidates() and validate() now run one predicate, reachableTargets(),
rather than two that happened to agree. Two that agree by inspection is what
this was: the narrowing existed, was correct, and was only ever applied to
the list.

The refusal deliberately reads as "no such account". An out-of-roster id and
an id belonging to nobody now produce the same message, because a refusal
that distinguishes them lets a scoped staffer walk the id space and learn
which accounts exist outside their roster. That is why Rule::exists is gone
rather than kept alongside: one code path, one answer. A test pins the two
messages as identical instead of naming either.

Both the web screen and the API twin come through this one validate(), so
both are fixed by it — and the test file proves each separately rather than
assuming the sharing holds.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx
This commit is contained in:
ignacionelson
2026-09-11 12:19:28 -03:00
parent 50f8b578df
commit 8372f42525
3 changed files with 217 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CHANGELOG.md
+13
View File
@@ -38,6 +38,19 @@ installation from starting.
**Fixed**
- **Deleting a client can no longer hand their files to a client you do not manage.** When you
delete an account that owns files, ProjectSend asks who should inherit them, and the list it
offers a staff member whose access is limited to certain clients shows only those clients. The
list was the only thing enforcing that. A request naming any other active account was accepted, so
one client's files and folders could end up owned by a client on somebody else's list — who could
then read, change and delete them, because people own what they upload. The list and the rule
behind it are now the same thing.
*Who this affected:* installations using staff roles that are limited to certain clients, where
such a role can also delete clients. Administrators whose access is not limited are unaffected and
can still reassign to anybody. Nothing to do on upgrade.
Reported by [@skeletonsec](https://github.com/skeletonsec).
- **Moving a file into a public folder now needs the same permission as uploading one there.** A
file in a public folder is public — that is what the folder means, and it applies to anything
inside it, at any depth. Uploading into one was already refused to staff who are not allowed to
app/Modules/Identity/AccountContentDeletion.php
+48 -6
View File
@@ -10,6 +10,7 @@ use App\Modules\Audit\ActivityLogger;
use App\Modules\Files\Access\StaffLibraryScope;
use App\Modules\Files\DeletedAccountContent;
use App\Modules\Identity\Models\Role;
use Closure;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Http\Request;
use Illuminate\Validation\Rule;
@@ -61,12 +62,8 @@ class AccountContentDeletion
*/
public function candidates(?User $viewer, ?int $excludeId = null): array
{
return User::query()
return $this->reachableTargets($viewer)
->when($excludeId, fn (Builder $query, int $id) => $query->whereKeyNot($id))
->when($viewer, fn (Builder $query, User $for) => $query->where(fn (Builder $reachable) => $reachable
->where('type', UserType::Staff)
->orWhereIn('id', $this->scope->clients($for)->select('users.id'))))
->where('active', true)
->with('role')
->orderBy('name')
->get()
@@ -99,17 +96,62 @@ class AccountContentDeletion
return [];
}
$viewer = $request->user();
return $request->validate([
'content_action' => ['required', Rule::in(['cascade_delete', 'reassign'])],
'reassign_to_id' => [
'required_if:content_action,reassign',
'integer',
Rule::exists('users', 'id')->where('active', true),
Rule::notIn([$target->id]),
// The same question the picker asks, asked again of what
// came back from it. It used to be "exists, and is active",
// which is not the boundary the picker documents two
// methods up: a client-scoped staff member was shown their
// own roster and could name anybody, so deleting a roster
// client could hand that client's files and folders to a
// client on somebody else's roster — who then reads, edits
// and deletes them under the own-upload rules
// (GHSA-w29w-pj29-x7ww).
//
// One predicate for both, rather than a matching pair: a
// picker that promises a boundary the write does not keep
// is exactly what this was.
function (string $attribute, mixed $value, Closure $fail) use ($viewer): void {
if (! $this->reachableTargets($viewer)->whereKey($value)->exists()) {
// Deliberately the message an id that does not
// exist at all would get. "Not yours" and "not
// there" have to read the same, or refusing is how
// a scoped staff member enumerates the accounts
// outside their roster.
$fail('validation.exists')->translate();
}
},
],
]);
}
/**
* Every active account $viewer may hand content to: staff, who are
* narrowed nowhere in the application, plus the clients
* StaffLibraryScope shows them. An unscoped viewer gets everybody,
* because clients() returns everybody for them.
*
* $viewer is null only where the question is about the installation
* rather than about a screen — the erasure default in privacy
* settings, which is stored once for everybody.
*
* @return Builder<User>
*/
private function reachableTargets(?User $viewer): Builder
{
return User::query()
->when($viewer, fn (Builder $query, User $for) => $query->where(fn (Builder $reachable) => $reachable
->where('type', UserType::Staff)
->orWhereIn('id', $this->scope->clients($for)->select('users.id'))))
->where('active', true);
}
/**
* @param array{content_action?: string, reassign_to_id?: int} $validated
*/
tests/Feature/Identity/ReassignmentRosterBoundaryTest.php
+156
View File
@@ -0,0 +1,156 @@
<?php
declare(strict_types=1);
use App\Models\User;
use App\Modules\Files\Models\File;
use App\Modules\Files\Models\Folder;
use App\Modules\Identity\AccountContentDeletion;
use App\Modules\Identity\Models\Role;
use App\Modules\Identity\Models\RolePermission;
use App\Modules\Identity\Permissions\Permission;
use Illuminate\Support\Facades\Storage;
use Illuminate\Support\Str;
/**
* GHSA-w29w-pj29-x7ww. Deleting an account that owns content makes the
* admin choose who inherits it, and the picker narrows that list to the
* viewer's own roster — its comment says so: "a client-scoped staff member
* is not shown the name of somebody they can reach nothing of."
*
* The write asked a different question. `exists, active, not the account
* being deleted` is true of every account on the installation, so a scoped
* staff member could name one the picker had deliberately kept off the
* list, and a roster client's files would land with a client on somebody
* else's roster — readable, editable and deletable there, because a client
* owns what they uploaded.
*
* The picker and the write now run one predicate, not two that agree by
* inspection.
*/
beforeEach(function () {
Storage::fake('files');
$this->admin = User::factory()->create();
$role = Role::query()->create(['name' => 'Reps '.Str::random(6), 'client_scoped' => true]);
foreach ([Permission::DeleteClients, Permission::EditClients, Permission::CreateClients] as $permission) {
RolePermission::query()->create(['role_id' => $role->id, 'permission' => $permission->value]);
}
$this->rep = User::factory()->create(['role_id' => $role->id]);
// On this rep's roster, and owning something worth inheriting.
$this->mine = User::factory()->client()->create(['name' => 'Mine']);
$this->rep->assignedClients()->sync([$this->mine->id]);
$this->file = File::factory()->create(['uploaded_by' => $this->mine->id]);
$this->folder = Folder::query()->create([
'name' => 'Theirs', 'slug' => 'theirs-'.Str::random(6), 'path' => '/', 'created_by' => $this->mine->id,
]);
// On nobody's roster as far as this rep is concerned.
$this->stranger = User::factory()->client()->create(['name' => 'Not Mine']);
});
test('the picker does not offer a client outside the roster', function () {
// The promise the write has to keep. Asserted first so that a change
// loosening the picker cannot quietly make the rest of this file vacuous.
$names = collect(app(AccountContentDeletion::class)->candidates($this->rep))
->pluck('name');
expect($names)->toContain('Mine')
->and($names)->not->toContain('Not Mine');
});
test('a scoped staff member cannot hand content to a client off their roster', function () {
$this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [
'content_action' => 'reassign',
'reassign_to_id' => $this->stranger->id,
])->assertSessionHasErrors('reassign_to_id');
expect($this->file->fresh()->uploaded_by)->toBe($this->mine->id)
->and($this->folder->fresh()->created_by)->toBe($this->mine->id)
->and(User::withTrashed()->find($this->mine->id)->trashed())->toBeFalse();
});
test('the API twin refuses it too', function () {
$token = $this->rep->createToken('t', [Permission::DeleteClients->value])->plainTextToken;
$this->withToken($token)
->deleteJson("/api/v1/clients/{$this->mine->id}", [
'content_action' => 'reassign',
'reassign_to_id' => $this->stranger->id,
])->assertStatus(422);
expect($this->file->fresh()->uploaded_by)->toBe($this->mine->id);
});
test('refusing reads the same as an account that is not there at all', function () {
// Otherwise the refusal is an oracle: a scoped staff member could walk
// the id space and learn which accounts exist outside their roster by
// the difference between the two answers.
$outsider = $this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [
'content_action' => 'reassign',
'reassign_to_id' => $this->stranger->id,
]);
$nobody = $this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [
'content_action' => 'reassign',
'reassign_to_id' => 99999999,
]);
expect($outsider->getSession()->get('errors')->get('reassign_to_id'))
->toBe($nobody->getSession()->get('errors')->get('reassign_to_id'));
});
test('a client on the roster is still a valid target', function () {
$alsoMine = User::factory()->client()->create(['name' => 'Also Mine']);
$this->rep->assignedClients()->sync([$this->mine->id, $alsoMine->id]);
$this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [
'content_action' => 'reassign',
'reassign_to_id' => $alsoMine->id,
])->assertRedirect();
expect($this->file->fresh()->uploaded_by)->toBe($alsoMine->id)
->and($this->folder->fresh()->created_by)->toBe($alsoMine->id);
});
test('a staff account is still a valid target, because staff are narrowed nowhere', function () {
$this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [
'content_action' => 'reassign',
'reassign_to_id' => $this->admin->id,
])->assertRedirect();
expect($this->file->fresh()->uploaded_by)->toBe($this->admin->id);
});
test('an unscoped administrator can still reassign to anybody', function () {
// The tightening is about the roster, and an unscoped staff member has
// no roster — StaffLibraryScope::clients() is every client for them.
$this->actingAs($this->admin)->delete("/clients/{$this->mine->id}", [
'content_action' => 'reassign',
'reassign_to_id' => $this->stranger->id,
])->assertRedirect();
expect($this->file->fresh()->uploaded_by)->toBe($this->stranger->id);
});
test('the advisory controls still behave as they did', function () {
// Both were already enforced, and both must survive the new rule:
// an inactive account, and the account being deleted.
$inactive = User::factory()->client()->create(['active' => false]);
$this->rep->assignedClients()->sync([$this->mine->id, $inactive->id]);
$this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [
'content_action' => 'reassign',
'reassign_to_id' => $inactive->id,
])->assertSessionHasErrors('reassign_to_id');
$this->actingAs($this->rep)->delete("/clients/{$this->mine->id}", [
'content_action' => 'reassign',
'reassign_to_id' => $this->mine->id,
])->assertSessionHasErrors('reassign_to_id');
expect($this->file->fresh()->uploaded_by)->toBe($this->mine->id);
});