mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-11 22:38:54 +00:00
Stop scoped staff reaching groups that are not theirs
Reported by @Drescargot as GHSA-r3hg-3fxw-rcmr, in two halves. The groups listing never narrowed at all. Every other action in that controller is guarded with allowsGroupChange(), and index() — web and API alike — built a bare Group::query(), so a client-scoped staff member was shown every group on the installation with its name, description and member count. StaffLibraryScope::groups() is that narrowing, and assignableGroupIds() now reads from it rather than restating the same rule a second time, which is how the two drifted apart to begin with. The second half is the one that mattered. allowsGroupChange() asked only groupReachesNoFurther() — "is anything shared with this group outside my library" — which a group with nothing shared with it yet passes vacuously. So a scoped staff member could rename, delete or publish a group whose every member was somebody else's client. Publishing is the sharp end: whatever is shared with the group afterwards is reachable without signing in. The reporter suggested putting the membership check inside groupReachesNoFurther(). Tried, and it breaks two things. That predicate is shared with allowsGroupMembership(), where a group nobody has joined must stay usable so its creator can add the first member. And "every member must be mine" is the obvious reading of the rule and is wrong: it turns GHSA-whmp-p9hv-r7j7's narrowing — a mixed group's edit screen loads and simply does not name the stranger — back into a 404, undoing that fix. Four tests from it fail that way. So the check sits in allowsGroupChange() alone, and asks whether the group is wholly somebody else's rather than whether it is wholly theirs. A mixed group stays workable and is still covered by the reach check; an empty one stays nameable by whoever just made it; a group with members and none of them theirs is refused.
This commit is contained in:
ignacionelson
@@ -15,6 +15,18 @@ when a version is cut.
|
||||
|
||||
**Fixed**
|
||||
|
||||
- **A staff member limited to some clients can no longer see or change other people's groups.** The
|
||||
groups list showed every group on the installation — name, description and member count — whatever
|
||||
the viewer's roster, and a group that nothing had been shared with yet could be renamed, deleted or
|
||||
made public by somebody with no relationship to any of its members. Making one public is the part
|
||||
that mattered: whatever is shared with the group afterwards becomes reachable without signing in.
|
||||
|
||||
*Who this affected:* only installations using a role with client scoping turned on. If every staff
|
||||
role on your installation sees all clients, nothing changed for you. Groups holding at least one of
|
||||
a scoped viewer's own clients stay visible and editable to them, exactly as before; groups holding
|
||||
none of them are now hidden and refused. The API behaves the same way as the screens do.
|
||||
|
||||
Reported by [@Drescargot](https://github.com/Drescargot).
|
||||
- **A public gallery no longer renders the same thumbnail several times at once.** The first visit
|
||||
to a page of large images started one full-size decode per thumbnail in parallel, which on a
|
||||
memory-limited server could exhaust it — and because a decode that dies writes nothing, the page
|
||||
|
||||
@@ -159,15 +159,37 @@ class StaffLibraryScope
|
||||
return null;
|
||||
}
|
||||
|
||||
$clientIds = $this->assignableClientIds($user) ?? [];
|
||||
return array_values($this->groups($user)->pluck('id')->map(fn ($id): int => (int) $id)->all());
|
||||
}
|
||||
|
||||
if ($clientIds === []) {
|
||||
return [];
|
||||
/**
|
||||
* Every group this staff member may be told about, as a query.
|
||||
*
|
||||
* The listing half of assignableGroupIds(), and the same rule: a
|
||||
* group counts as theirs because one of their clients is in it. The
|
||||
* two were not the same code, and the listing simply had none — so
|
||||
* `/groups` and `/api/v1/groups` showed a scoped staff member every
|
||||
* group on the installation, name, description and member count,
|
||||
* including groups whose every member was somebody else's client
|
||||
* (GHSA-r3hg-3fxw-rcmr).
|
||||
*
|
||||
* Deliberately the *sharing* rule rather than the change rule below.
|
||||
* A scoped staff member may already share a file with a mixed group,
|
||||
* so its existence is not news to them; what they may not do is
|
||||
* rename, publish or delete it.
|
||||
*
|
||||
* @return Builder<Group>
|
||||
*/
|
||||
public function groups(User $user): Builder
|
||||
{
|
||||
$query = Group::query();
|
||||
$clientIds = $this->assignableClientIds($user);
|
||||
|
||||
if ($clientIds === null) {
|
||||
return $query;
|
||||
}
|
||||
|
||||
return array_values(Group::query()
|
||||
->whereHas('members', fn (Builder $members) => $members->whereIn('users.id', $clientIds))
|
||||
->pluck('id')->map(fn ($id): int => (int) $id)->all());
|
||||
return $query->whereHas('members', fn (Builder $members) => $members->whereIn('users.id', $clientIds));
|
||||
}
|
||||
|
||||
public function canAssignClient(User $user, User $client): bool
|
||||
@@ -249,7 +271,53 @@ class StaffLibraryScope
|
||||
*/
|
||||
public function allowsGroupChange(User $user, Group $group): bool
|
||||
{
|
||||
return $this->groupReachesNoFurther($user, $group);
|
||||
return $this->groupIsNotWhollySomebodyElses($user, $group)
|
||||
&& $this->groupReachesNoFurther($user, $group);
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether this group is somebody else's entirely — every member
|
||||
* outside the staff member's roster, and none of theirs in it.
|
||||
*
|
||||
* The half allowsGroupChange() was missing. Reach answers "what would
|
||||
* this group hand somebody", which is the right question for putting a
|
||||
* client *into* it; it says nothing about who is already there. So a
|
||||
* group with nothing shared with it yet passed the reach check
|
||||
* vacuously, and a scoped staff member could rename it, delete it, or
|
||||
* publish it — a group made entirely of clients they had never been
|
||||
* assigned (GHSA-r3hg-3fxw-rcmr).
|
||||
*
|
||||
* **Not "every member is mine", which is the obvious reading and is
|
||||
* wrong.** A mixed group has to stay changeable: GHSA-whmp-p9hv-r7j7
|
||||
* settled that a scoped staff member opens such a group's edit screen
|
||||
* and is shown only their own clients in it, rather than being refused
|
||||
* the screen. Requiring every member to be theirs turns that narrowing
|
||||
* back into a 404 and undoes the earlier fix. What is left over — a
|
||||
* mixed group whose shared content reaches past their library — is
|
||||
* refused by groupReachesNoFurther() beside this, which is the check
|
||||
* that has always covered it.
|
||||
*
|
||||
* **And deliberately not folded into groupReachesNoFurther() either.**
|
||||
* That predicate is shared with allowsGroupMembership(), where a group
|
||||
* nobody has joined must stay usable so its creator can put the first
|
||||
* member in — the case that method's own docblock calls out.
|
||||
*
|
||||
* An empty group is nobody else's, so whoever just made it can still
|
||||
* name it.
|
||||
*/
|
||||
private function groupIsNotWhollySomebodyElses(User $user, Group $group): bool
|
||||
{
|
||||
$clientIds = $this->assignableClientIds($user);
|
||||
|
||||
if ($clientIds === null) {
|
||||
return true;
|
||||
}
|
||||
|
||||
if (! $group->members()->exists()) {
|
||||
return true;
|
||||
}
|
||||
|
||||
return $group->members()->whereIn('users.id', $clientIds)->exists();
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -41,7 +41,13 @@ class GroupsController extends Controller
|
||||
'visibility' => ['nullable', Rule::in(['public', 'private'])],
|
||||
]);
|
||||
|
||||
$query = Group::query()->withCount('members');
|
||||
$viewer = $request->user();
|
||||
assert($viewer !== null);
|
||||
|
||||
// The API twin of the web listing's narrowing, and it has to be
|
||||
// here rather than only there: the same disclosure through a token
|
||||
// is the same disclosure (GHSA-r3hg-3fxw-rcmr).
|
||||
$query = $this->scope->groups($viewer)->withCount('members');
|
||||
|
||||
if (($filters['search'] ?? null) !== null) {
|
||||
$search = $filters['search'];
|
||||
|
||||
@@ -40,7 +40,14 @@ class GroupsController extends Controller
|
||||
'visibility' => $validated['visibility'] ?? null,
|
||||
];
|
||||
|
||||
$groups = Group::query()
|
||||
$viewer = $request->user();
|
||||
assert($viewer !== null);
|
||||
|
||||
// Scoped, not Group::query(): a client-scoped staff member is told
|
||||
// about a group because one of their clients is in it. Without
|
||||
// this the listing showed every group on the installation, to a
|
||||
// viewer who could reach nothing of theirs (GHSA-r3hg-3fxw-rcmr).
|
||||
$groups = $this->scope->groups($viewer)
|
||||
->withCount('members')
|
||||
->when($filters['search'], fn (Builder $query, string $search) => $query->where(fn (Builder $q) => $q
|
||||
->where('name', 'like', "%{$search}%")
|
||||
|
||||
@@ -0,0 +1,203 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
use App\Models\User;
|
||||
use App\Modules\Groups\Models\Group;
|
||||
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;
|
||||
use Inertia\Testing\AssertableInertia;
|
||||
use Laravel\Sanctum\Sanctum;
|
||||
|
||||
/**
|
||||
* A group belongs to the people in it, and a client-scoped staff member
|
||||
* holds only some of them.
|
||||
*
|
||||
* GroupMembershipScopeTest beside this one covers *reach*: what joining a
|
||||
* group would hand somebody. This covers the group object itself — being
|
||||
* told it exists, and renaming, deleting or publishing it. The two are
|
||||
* different questions and were answered by the same predicate, which only
|
||||
* asked the first.
|
||||
*
|
||||
* Reported as GHSA-r3hg-3fxw-rcmr.
|
||||
*/
|
||||
beforeEach(function () {
|
||||
Storage::fake('files');
|
||||
$this->admin = User::factory()->create();
|
||||
|
||||
$role = Role::query()->create(['name' => 'Reps '.Str::random(6), 'client_scoped' => true]);
|
||||
foreach ([Permission::ManageGroups, Permission::EditGroups, Permission::DeleteGroups, Permission::CreateGroups] as $permission) {
|
||||
RolePermission::query()->create(['role_id' => $role->id, 'permission' => $permission->value]);
|
||||
}
|
||||
|
||||
$this->rep = User::factory()->create(['role_id' => $role->id]);
|
||||
$this->mine = User::factory()->client()->create(['name' => 'Mine']);
|
||||
$this->rep->assignedClients()->sync([$this->mine->id]);
|
||||
|
||||
$this->stranger = User::factory()->client()->create(['name' => 'Not Mine']);
|
||||
|
||||
// The group at the centre of the report: somebody else's client is its
|
||||
// only member, and nothing has been shared with it yet — so every
|
||||
// "does this group reach past my library" check answers no, vacuously.
|
||||
$this->theirs = Group::query()->create(['name' => 'Theirs Only', 'slug' => 'theirs-only', 'public' => false]);
|
||||
$this->theirs->members()->syncWithoutDetaching([$this->stranger->id]);
|
||||
|
||||
$this->ours = Group::query()->create(['name' => 'Ours', 'slug' => 'ours', 'public' => false]);
|
||||
$this->ours->members()->syncWithoutDetaching([$this->mine->id]);
|
||||
});
|
||||
|
||||
function listedGroupNames(User $viewer): array
|
||||
{
|
||||
$names = [];
|
||||
|
||||
test()->actingAs($viewer)->get('/groups')->assertOk()->assertInertia(
|
||||
function (AssertableInertia $page) use (&$names) {
|
||||
$names = collect($page->toArray()['props']['groups']['data'] ?? $page->toArray()['props']['groups'])
|
||||
->pluck('name')->all();
|
||||
},
|
||||
);
|
||||
|
||||
return $names;
|
||||
}
|
||||
|
||||
/*
|
||||
|--------------------------------------------------------------------------
|
||||
| Being told it exists
|
||||
|--------------------------------------------------------------------------
|
||||
*/
|
||||
|
||||
test('the listing hides a group made entirely of other people\'s clients', function () {
|
||||
expect(listedGroupNames($this->rep))->not->toContain('Theirs Only');
|
||||
});
|
||||
|
||||
test('the listing still shows a group holding one of their own clients', function () {
|
||||
expect(listedGroupNames($this->rep))->toContain('Ours');
|
||||
});
|
||||
|
||||
test('an unscoped administrator still sees every group', function () {
|
||||
expect(listedGroupNames($this->admin))->toContain('Theirs Only')->toContain('Ours');
|
||||
});
|
||||
|
||||
test('the API listing hides it too', function () {
|
||||
Sanctum::actingAs($this->rep, ['manage_groups']);
|
||||
|
||||
$names = collect($this->getJson('/api/v1/groups')->assertOk()->json('data'))->pluck('name')->all();
|
||||
|
||||
expect($names)->not->toContain('Theirs Only')->toContain('Ours');
|
||||
});
|
||||
|
||||
/*
|
||||
|--------------------------------------------------------------------------
|
||||
| Changing it
|
||||
|--------------------------------------------------------------------------
|
||||
|
|
||||
| The higher half of the report. Renaming, deleting or publishing a group
|
||||
| lands on every member, and none of this group's members are theirs.
|
||||
*/
|
||||
|
||||
test('they cannot open the edit form for it', function () {
|
||||
$this->actingAs($this->rep)->get("/groups/{$this->theirs->id}")->assertNotFound();
|
||||
});
|
||||
|
||||
test('they cannot rename it', function () {
|
||||
$this->actingAs($this->rep)
|
||||
->patch("/groups/{$this->theirs->id}", ['name' => 'Renamed', 'public' => false])
|
||||
->assertNotFound();
|
||||
|
||||
expect($this->theirs->fresh()->name)->toBe('Theirs Only');
|
||||
});
|
||||
|
||||
test('they cannot publish it', function () {
|
||||
// The consequence the report calls the serious one: a public group
|
||||
// becomes an anonymous listing for whatever is shared with it later.
|
||||
$this->actingAs($this->rep)
|
||||
->patch("/groups/{$this->theirs->id}", ['name' => 'Theirs Only', 'public' => true])
|
||||
->assertNotFound();
|
||||
|
||||
expect($this->theirs->fresh()->public)->toBeFalse();
|
||||
});
|
||||
|
||||
test('they cannot delete it out from under its members', function () {
|
||||
$this->actingAs($this->rep)->delete("/groups/{$this->theirs->id}")->assertNotFound();
|
||||
|
||||
expect(Group::query()->whereKey($this->theirs->id)->exists())->toBeTrue();
|
||||
});
|
||||
|
||||
test('the API refuses the same three', function () {
|
||||
Sanctum::actingAs($this->rep, ['edit_groups', 'delete_groups']);
|
||||
|
||||
$this->getJson("/api/v1/groups/{$this->theirs->id}")->assertNotFound();
|
||||
$this->patchJson("/api/v1/groups/{$this->theirs->id}", ['name' => 'Renamed'])->assertNotFound();
|
||||
$this->deleteJson("/api/v1/groups/{$this->theirs->id}")->assertNotFound();
|
||||
|
||||
expect($this->theirs->fresh()->name)->toBe('Theirs Only');
|
||||
});
|
||||
|
||||
/*
|
||||
|--------------------------------------------------------------------------
|
||||
| What must keep working
|
||||
|--------------------------------------------------------------------------
|
||||
|
|
||||
| The pins. The fix suggested in the report puts the membership check
|
||||
| inside groupReachesNoFurther(), which allowsGroupMembership() also
|
||||
| calls — and that would have broken both of these.
|
||||
*/
|
||||
|
||||
test('they can still rename a group of their own client', function () {
|
||||
$this->actingAs($this->rep)
|
||||
->patch("/groups/{$this->ours->id}", ['name' => 'Ours Renamed', 'public' => false])
|
||||
->assertRedirect();
|
||||
|
||||
expect($this->ours->fresh()->name)->toBe('Ours Renamed');
|
||||
});
|
||||
|
||||
test('they can still put the first member into a group they just made', function () {
|
||||
// A group nobody has joined belongs to nobody, and this is the case
|
||||
// StaffLibraryScope's own docblock says must keep working.
|
||||
$fresh = Group::query()->create(['name' => 'Brand New', 'slug' => 'brand-new', 'public' => false]);
|
||||
|
||||
$this->actingAs($this->rep)
|
||||
->post("/groups/{$fresh->id}/members", ['user_id' => $this->mine->id])
|
||||
->assertRedirect();
|
||||
|
||||
expect($fresh->members()->pluck('users.id')->all())->toContain($this->mine->id);
|
||||
});
|
||||
|
||||
test('they can still rename an empty group', function () {
|
||||
$fresh = Group::query()->create(['name' => 'Brand New', 'slug' => 'brand-new', 'public' => false]);
|
||||
|
||||
$this->actingAs($this->rep)
|
||||
->patch("/groups/{$fresh->id}", ['name' => 'Named At Last', 'public' => false])
|
||||
->assertRedirect();
|
||||
|
||||
expect($fresh->fresh()->name)->toBe('Named At Last');
|
||||
});
|
||||
|
||||
test('a mixed group stays changeable, with its stranger unnamed', function () {
|
||||
// The boundary between this report and GHSA-whmp-p9hv-r7j7. "Every
|
||||
// member must be mine" is the obvious reading of the fix and it is
|
||||
// wrong: it turns that advisory's narrowing back into a 404. A group
|
||||
// holding one of their clients is theirs to work with; what protects
|
||||
// the stranger in it is that they are never named, and that anything
|
||||
// shared with the group beyond this viewer's library still refuses
|
||||
// the change.
|
||||
$mixed = Group::query()->create(['name' => 'Mixed', 'slug' => 'mixed', 'public' => false]);
|
||||
$mixed->members()->syncWithoutDetaching([$this->mine->id, $this->stranger->id]);
|
||||
|
||||
$this->actingAs($this->rep)
|
||||
->patch("/groups/{$mixed->id}", ['name' => 'Mixed Renamed', 'public' => false])
|
||||
->assertRedirect();
|
||||
|
||||
expect($mixed->fresh()->name)->toBe('Mixed Renamed');
|
||||
});
|
||||
|
||||
test('an unscoped administrator can still change anything', function () {
|
||||
$this->actingAs($this->admin)
|
||||
->patch("/groups/{$this->theirs->id}", ['name' => 'Admin Renamed', 'public' => false])
|
||||
->assertRedirect();
|
||||
|
||||
expect($this->theirs->fresh()->name)->toBe('Admin Renamed');
|
||||
});
|
||||
Reference in New Issue
Block a user