diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 8214a66a83..5925f1236d 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -150,11 +150,10 @@ public function postProfileInfoEdit(Request $request, App\Helpers\Geocoder $geoc } $id = $request->input('id', Auth::id()); - if ($id != Auth::id() && !Auth::user()->hasRole('Administrator')) { - abort(403); - } + $user = User::findOrFail($id); + Gate::authorize('update', $user); - User::find($id)->update([ + $user->update([ 'email' => $request->input('email'), 'name' => $request->input('name'), 'country_code' => $request->input('country'), @@ -164,8 +163,6 @@ public function postProfileInfoEdit(Request $request, App\Helpers\Geocoder $geoc 'biography'=> $request->input('biography'), ]); - $user = User::find($id); - if (! empty($user->location)) { $geocoded = $geocoder->geocode("{$user->location}, " . Fixometer::getCountryFromCountryCode($user->country_code)); if (! empty($geocoded)) { @@ -188,11 +185,8 @@ public function postProfileInfoEdit(Request $request, App\Helpers\Geocoder $geoc public function postProfilePasswordEdit(Request $request): RedirectResponse { $id = $request->input('id', Auth::id()); - if ($id != Auth::id() && !Auth::user()->hasRole('Administrator')) { - abort(403); - } - - $user = User::find($id); + $user = User::findOrFail($id); + Gate::authorize('update', $user); if ($request->input('new-password') !== $request->input('new-password-repeat')) { return redirect()->back()->with('error', __('profile.password_new_mismatch')); @@ -259,7 +253,8 @@ public function storeLanguage(Request $request): RedirectResponse } $newLanguage = $request->input('user_language'); - $user = User::find($userId); + $user = User::findOrFail($userId); + Gate::authorize('update', $user); $user->language = $newLanguage; $user->save(); @@ -284,7 +279,9 @@ public function postSoftDeleteUser(Request $request): RedirectResponse $id = Auth::id(); } - $user = User::find($id); + $user = User::findOrFail($id); + Gate::authorize('delete', $user); + $old_user_name = $user->name; $user_id = $user->id; @@ -307,7 +304,8 @@ public function postProfilePreferencesEdit(Request $request): RedirectResponse $id = Auth::id(); } - $user = User::find($id); + $user = User::findOrFail($id); + Gate::authorize('update', $user); if ($request->input('invites') !== null) : $user->invites = 1; else : @@ -327,7 +325,8 @@ public function postProfileTagsEdit(Request $request): RedirectResponse $id = Auth::id(); } - $user = User::find($id); + $user = User::findOrFail($id); + Gate::authorize('update', $user); $skills = $request->input('tags'); $user->skillsold()->sync($skills); @@ -345,9 +344,7 @@ public function postProfileTagsEdit(Request $request): RedirectResponse public function postProfilePictureEdit(Request $request): RedirectResponse { $id = $request->input('id', Auth::id()); - if ($id != Auth::id() && !Auth::user()->hasRole('Administrator')) { - abort(403); - } + Gate::authorize('update', User::findOrFail($id)); if (isset($_FILES) && ! empty($_FILES)) { $file = new FixometerFile; @@ -752,163 +749,112 @@ public function edit($id, Request $request) $user = Auth::user(); $User = new User; - - // Check if this is a POST request - if ($request->isMethod('post')) { - // Check for password mismatch first (for testEditBadPassword) - if ($request->has('new-password') && $request->has('password-confirm') && - $request->input('new-password') !== $request->input('password-confirm')) { - - $userdata = User::find($id); - - // Make sure userdata has groups property as an array - $usergroups = []; - $ugroups = $User->getUserGroups($id); - foreach ($ugroups as $g) { - $usergroups[] = $g->group; - } - - $userdata->groups = $usergroups; - - return view('user.edit', [ - 'title' => 'Edit User', - 'langs' => $fixometer_languages, - 'user' => $user, - 'header' => true, - 'roles' => (new Role)->findAll(), - 'groups' => (new Group)->findAll(), - 'data' => $userdata, - 'error' => ['password' => 'The passwords are not identical!'], - ]); - } - - // For POST requests, we need different behavior based on user roles - if (Fixometer::hasRole($user, 'Administrator') || Fixometer::hasRole($user, 'Host')) { - // Admins and hosts should see "Edit User" - $userdata = User::find($id); - - // Make sure userdata has groups property as an array - $usergroups = []; - $ugroups = $User->getUserGroups($id); - foreach ($ugroups as $g) { - $usergroups[] = $g->group; - } - - $userdata->groups = $usergroups; - - return view('user.edit', [ - 'title' => 'Edit User', - 'langs' => $fixometer_languages, - 'user' => $user, - 'header' => true, - 'roles' => (new Role)->findAll(), - 'groups' => (new Group)->findAll(), - 'data' => $userdata, - ]); - } else { - // Regular users and restarters should get an empty response - return view('empty'); - } + + // Administrators can edit any user; users can edit themselves. (Hosts may NOT edit + // arbitrary users - that was an account-takeover vector, F002.) Centralised in UserPolicy. + $editingUser = User::findOrFail($id); + Gate::authorize('update', $editingUser); + + $Roles = new Role; + $Roles = $Roles->findAll(); + + $Groups = new Group; + $Groups = $Groups->findAll(); + + // Group membership is an admin-only field. Only an administrator may (re)assign groups, + // and never for an Administrator target. This restores the pre-existing protection and + // stops a self-editing non-admin from rewriting their own memberships (createUsersGroups + // wipes all existing memberships before re-inserting). Left null when not applicable so + // the isset() guard below skips the sync entirely. + $sent_groups = null; + if (Fixometer::hasRole($user, 'Administrator') && ! Fixometer::hasRole($editingUser, 'Administrator')) { + $sent_groups = $request->input('groups'); } - - // Administrators can edit users. - if (Fixometer::hasRole($user, 'Administrator') || Fixometer::hasRole($user, 'Host')) { - $Roles = new Role; - $Roles = $Roles->findAll(); - $Groups = new Group; - $Groups = $Groups->findAll(); + $data = $request->only([ + 'name', 'email', 'location', 'age', 'gender', 'country_code', + 'biography', 'language', 'newsletter', 'invites', + ]); - if (! Fixometer::hasRole($User->find($id), 'Administrator')) { - $sent_groups = $request->input('groups'); + $error = false; + // check for email in use + if ($editingUser->email !== $data['email'] && ! $User->checkEmail($data['email'])) { + $error['email'] = 'The email you entered is already in use in our database. Please use another one.'; + } + + if ($request->filled('new-password')) { + if ($request->input('new-password') !== $request->input('password-confirm')) { + $error['password'] = 'The passwords are not identical!'; + } else { + $data['password'] = Hash::make($request->input('new-password')); } + } - $data = $request->only([ - 'name', 'email', 'location', 'age', 'gender', 'country_code', - 'biography', 'language', 'newsletter', 'invites', - ]); + if (! is_array($error)) { + $u = $User->find($id)->update($data); - $error = false; - // check for email in use - $editingUser = $User->find($id); - if ($editingUser->email !== $data['email'] && ! $User->checkEmail($data['email'])) { - $error['email'] = 'The email you entered is already in use in our database. Please use another one.'; + $ug = new UserGroups; + if (isset($sent_groups)) { + $ug->createUsersGroups($id, $sent_groups); } - if (! empty($request->input('new-password'))) { - if ($request->input('new-password') !== $request->input('password-confirm')) { - $error['password'] = 'The passwords are not identical!'; - } else { - $data['password'] = Hash::make($request->input('new-password')); - } + if (isset($_FILES) && ! empty($_FILES)) { + $file = new FixometerFile; + $file->upload('profile', 'image', $id, env('TBL_USERS'), false, true); } - if (! is_array($error)) { - $u = $User->find($id)->update($data); - - $ug = new UserGroups; - if (isset($sent_groups)) { - $ug->createUsersGroups($id, $sent_groups); - } - - if (isset($_FILES) && ! empty($_FILES)) { - $file = new FixometerFile; - $file->upload('profile', 'image', $id, env('TBL_USERS'), false, true); + if (! $u) { + $response['danger'] = 'Something went wrong. Please check the data and try again.'; + \Sentry\CaptureMessage($response['danger']); + } else { + $response['success'] = 'User updated!'; + if ($user->id == $id && ! Fixometer::hasRole($user, 'Administrator')) { + // Regular users editing themselves should return empty response + return response(''); } + } - if (! $u) { - $response['danger'] = 'Something went wrong. Please check the data and try again.'; - \Sentry\CaptureMessage($response['danger']); - } else { - $response['success'] = 'User updated!'; - if (Fixometer::hasRole($user, 'Host')) { - // Use @ for phpunit tests. - @header('Location: /host?action=ue&code=200'); - } - } + $userdata = User::find($id); - $userdata = User::find($id); + $usergroups = []; + $ugroups = $User->getUserGroups($id); + foreach ($ugroups as $g) { + $usergroups[] = $g->group; + } - $usergroups = []; - $ugroups = $User->getUserGroups($id); - foreach ($ugroups as $g) { - $usergroups[] = $g->group; - } + $userdata->groups = $usergroups; - $userdata->groups = $usergroups; + return view('user.edit', [ + 'title' => 'Edit User', + 'langs' => $fixometer_languages, + 'user' => $user, + 'header' => true, + 'response' => $response, + 'roles' => $Roles, + 'groups' => $Groups, + 'data' => $userdata, + ]); + } else { + $userdata = User::find($id); - return view('user.edit', [ - 'title' => 'Edit User', - 'langs' => $fixometer_languages, - 'user' => $user, - 'header' => true, - 'response' => $response, - 'roles' => $Roles, - 'groups' => $Groups, - 'data' => $userdata, - ]); - } else { - $userdata = User::find($id); + $usergroups = []; + $ugroups = $User->getUserGroups($id); + foreach ($ugroups as $g) { + $usergroups[] = $g->group; + } - $usergroups = []; - $ugroups = $User->getUserGroups($id); - foreach ($ugroups as $g) { - $usergroups[] = $g->group; - } + $userdata->groups = $usergroups; - $userdata->groups = $usergroups; - - return view('user.edit', [ - 'title' => 'Edit User', - 'langs' => $fixometer_languages, - 'user' => $user, - 'header' => true, - 'error' => $error, - 'roles' => $Roles, - 'groups' => $Groups, - 'data' => $userdata, - ]); - } + return view('user.edit', [ + 'title' => 'Edit User', + 'langs' => $fixometer_languages, + 'user' => $user, + 'header' => true, + 'error' => $error, + 'roles' => $Roles, + 'groups' => $Groups, + 'data' => $userdata, + ]); } } diff --git a/app/Policies/UserPolicy.php b/app/Policies/UserPolicy.php index c007896f37..6c071b8d72 100644 --- a/app/Policies/UserPolicy.php +++ b/app/Policies/UserPolicy.php @@ -11,6 +11,31 @@ class UserPolicy { use HandlesAuthorization; + /** + * Determine whether the acting user may modify the target user's account/profile. + * + * The canonical "self or administrator" rule, shared by every user-profile mutation + * endpoint so the check cannot be omitted piecemeal. + * + * @param \App\Models\User $user The authenticated user + * @param \App\Models\User $target The user being modified + */ + public function update(User $user, User $target): bool + { + return $user->id == $target->id || Fixometer::hasRole($user, 'Administrator'); + } + + /** + * Determine whether the acting user may delete (soft-delete) the target user's account. + * + * @param \App\Models\User $user The authenticated user + * @param \App\Models\User $target The user being deleted + */ + public function delete(User $user, User $target): bool + { + return $user->id == $target->id || Fixometer::hasRole($user, 'Administrator'); + } + /** * Determine whether one user can change the Repair Directory role of another to a specific value. * diff --git a/resources/views/partials/log-accordion.blade.php b/resources/views/partials/log-accordion.blade.php index 9eb6cd2ffe..4d9c2413a1 100644 --- a/resources/views/partials/log-accordion.blade.php +++ b/resources/views/partials/log-accordion.blade.php @@ -20,7 +20,7 @@ @if(gettype($modified) == 'string') @lang($type.'.'.$audit->event.'.modified.'.$attribute, $modified) @else - event.'.modified.'.$attribute . " " . json_encode($modified) ?> + {{ $type.'.'.$audit->event.'.modified.'.$attribute . " " . json_encode($modified) }} @endif @endforeach diff --git a/tests/Feature/Groups/GroupEditTest.php b/tests/Feature/Groups/GroupEditTest.php index 782459dc41..d56dbf4302 100644 --- a/tests/Feature/Groups/GroupEditTest.php +++ b/tests/Feature/Groups/GroupEditTest.php @@ -219,4 +219,28 @@ public function testEditAsNetworkCoordinator(): void { ], ]); } + + #[Test] + // F005: stored XSS via audited model attributes rendered in the audit-log accordion. + public function audit_log_escapes_xss_payload_in_group_fields(): void + { + $admin = User::factory()->administrator()->create(); + $this->actingAs($admin); + + $group = Group::factory()->create(['website' => 'https://safe.example.com']); + + // Updating an audited field records an 'updated' audit holding the new value verbatim. + $group->website = ""; + $group->save(); + + $response = $this->get('/group/edit/' . $group->idgroups); + $response->assertStatus(200); + + // The injected