Files
ignacionelson 5fb17388cd 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.
2026-09-08 18:40:33 -03:00

157 lines
5.8 KiB
PHP

<?php
declare(strict_types=1);
namespace App\Modules\Groups\Http\Controllers\Api;
use App\Http\Controllers\Controller;
use App\Modules\Api\Support\PollingQuery;
use App\Modules\Audit\Action;
use App\Modules\Audit\ActivityLogger;
use App\Modules\Groups\Http\Resources\Api\GroupResource;
use App\Modules\Files\Access\StaffLibraryScope;
use App\Modules\Groups\Models\Group;
use App\Support\Rules;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Relations\BelongsToMany;
use Illuminate\Http\JsonResponse;
use Illuminate\Http\Request;
use Illuminate\Http\Resources\Json\AnonymousResourceCollection;
use Illuminate\Validation\Rule;
/**
* Groups — collections of clients used as a sharing target.
*
* Distinct from a Category (a label on a file) and a Folder (hierarchy);
* the three are easy to conflate from outside, which is why the guide
* names them separately.
*/
class GroupsController extends Controller
{
public function __construct(
private readonly PollingQuery $polling,
private readonly ActivityLogger $activity,
private readonly StaffLibraryScope $scope,
) {}
public function index(Request $request): AnonymousResourceCollection
{
$filters = $request->validate($this->polling->rules() + [
'search' => ['nullable', 'string', 'max:255'],
'visibility' => ['nullable', Rule::in(['public', 'private'])],
]);
$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'];
$query->where(fn (Builder $inner) => $inner
->where('name', 'like', "%{$search}%")
->orWhere('description', 'like', "%{$search}%"));
}
if (($filters['visibility'] ?? null) !== null) {
$query->where('public', $filters['visibility'] === 'public');
}
return GroupResource::collection($this->polling->paginate($request, $query, 'groups'));
}
public function show(Request $request, Group $group): GroupResource
{
$viewer = $request->user();
assert($viewer !== null);
// The web edit screen's boundary, on its API twin: this is the read
// half of the group that update() and destroy() below already refuse
// to touch, and it hands back the membership with addresses.
abort_unless($this->scope->allowsGroupChange($viewer, $group), 404);
return new GroupResource($group->loadCount('members')->load([
'members' => fn (BelongsToMany $members) => $members
->whereIn('users.id', $this->scope->clients($viewer)->select('id')),
]));
}
public function store(Request $request): JsonResponse
{
$validated = $request->validate([
'name' => ['required', 'string', 'max:255'],
'slug' => Rules::slug('groups'),
'description' => ['nullable', 'string', 'max:2000'],
'public' => ['required', 'boolean'],
]);
$validated['slug'] = ($validated['slug'] ?? '') ?: Group::uniqueSlugFrom($validated['name']);
$group = Group::query()->create($validated);
$this->activity->log(Action::GroupCreated, subject: $group);
if ($group->public) {
$this->activity->log(Action::GroupMadePublic, subject: $group, context: ['slug' => $group->slug]);
}
return (new GroupResource($group->loadCount('members')))->response()->setStatusCode(201);
}
public function update(Request $request, Group $group): GroupResource
{
$viewer = $request->user();
assert($viewer !== null);
// Mirrors the web controller: a group reaching past this token
// owner's library is not theirs to change, and deleting one
// revokes its members' access to everything assigned to it.
abort_unless($this->scope->allowsGroupChange($viewer, $group), 404);
$validated = $request->validate([
'name' => ['sometimes', 'string', 'max:255'],
'slug' => Rules::slug('groups', $group->id),
'description' => ['sometimes', 'nullable', 'string', 'max:2000'],
'public' => ['sometimes', 'boolean'],
]);
// A slug must never change silently just because the name did.
$validated['slug'] = ($validated['slug'] ?? '') ?: ($group->slug ?: Group::uniqueSlugFrom($validated['name'] ?? $group->name, $group->id));
$wasPublic = $group->public;
$group->update($validated);
$this->activity->log(Action::GroupUpdated, subject: $group);
if (! $wasPublic && $group->public) {
$this->activity->log(Action::GroupMadePublic, subject: $group, context: ['slug' => $group->slug]);
} elseif ($wasPublic && ! $group->public) {
$this->activity->log(Action::GroupMadePrivate, subject: $group);
}
return new GroupResource($group->refresh()->loadCount('members'));
}
public function destroy(Request $request, Group $group): JsonResponse
{
$viewer = $request->user();
assert($viewer !== null);
// Mirrors the web controller: a group reaching past this token
// owner's library is not theirs to change, and deleting one
// revokes its members' access to everything assigned to it.
abort_unless($this->scope->allowsGroupChange($viewer, $group), 404);
$name = $group->name;
$group->delete();
$this->activity->log(Action::GroupDeleted, context: ['name' => $name]);
return response()->json(status: 204);
}
}