From 4ed25f48e1bdc9b8cd38fdda7c046bf55ec25d0b Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Fri, 24 Apr 2026 10:23:12 -0700 Subject: [PATCH 1/4] fix(auth): add authorization checks to profile edit methods Profile edit methods accepted an unvalidated id parameter from the request body, allowing any authenticated user to edit any other user's profile, password, photo, or role. This adds ownership-or-admin gates to postProfileInfoEdit, postProfilePasswordEdit, and postProfilePictureEdit, and an admin-only gate to postAdminEdit. The role update in postAdminEdit is also changed from mass assignment to direct assignment in preparation for removing role from $fillable. --- app/Http/Controllers/UserController.php | 34 +++++++++++-------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index fe174a0c8f..425f47537e 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -149,10 +149,9 @@ public function postProfileInfoEdit(Request $request, App\Helpers\Geocoder $geoc return redirect()->back()->withErrors($validator)->withInput(); } - if ($request->input('id') !== null) { - $id = $request->input('id'); - } else { - $id = Auth::id(); + $id = $request->input('id', Auth::id()); + if ($id != Auth::id() && !Auth::user()->hasRole('Administrator')) { + abort(403); } User::find($id)->update([ @@ -188,10 +187,9 @@ public function postProfileInfoEdit(Request $request, App\Helpers\Geocoder $geoc public function postProfilePasswordEdit(Request $request): RedirectResponse { - if ($request->input('id') !== null) { - $id = $request->input('id'); - } else { - $id = Auth::id(); + $id = $request->input('id', Auth::id()); + if ($id != Auth::id() && !Auth::user()->hasRole('Administrator')) { + abort(403); } $user = User::find($id); @@ -346,10 +344,9 @@ public function postProfileTagsEdit(Request $request): RedirectResponse public function postProfilePictureEdit(Request $request): RedirectResponse { - if ($request->input('id') !== null) { - $id = $request->input('id'); - } else { - $id = Auth::id(); + $id = $request->input('id', Auth::id()); + if ($id != Auth::id() && !Auth::user()->hasRole('Administrator')) { + abort(403); } if (isset($_FILES) && ! empty($_FILES)) { @@ -364,20 +361,19 @@ public function postProfilePictureEdit(Request $request): RedirectResponse public function postAdminEdit(Request $request): RedirectResponse { - if ($request->input('id') !== null) { - $user_id = $request->input('id'); - } else { - $user_id = Auth::id(); + if (!Auth::user()->hasRole('Administrator')) { + abort(403); } + $user_id = $request->input('id', Auth::id()); + $user = User::find($user_id); $oldRole = $user->role; // Set role for User - $user->update([ - 'role' => $request->input('user_role'), - ]); + $user->role = $request->input('user_role'); + $user->save(); // If we are demoting from NetworkCoordinator, remove them from the list of coordinators for // any networks they are currently coordinating. From ecef4ffd985dc775bb987112c08fa61c1aaf37a4 Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Fri, 24 Apr 2026 10:23:33 -0700 Subject: [PATCH 2/4] fix(auth): switch user edit to field allowlist The edit() method passed $request->post() directly to User::update() after unsetting only a few keys. This denylist approach allowed injection of sensitive fields like role and api_token. Switching to $request->only() ensures only explicitly listed profile fields can be mass-assigned through this endpoint. --- app/Http/Controllers/UserController.php | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 425f47537e..0504b44508 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -819,12 +819,15 @@ public function edit($id, Request $request) $Groups = new Group; $Groups = $Groups->findAll(); - $data = $request->post(); - if (! Fixometer::hasRole($User->find($id), 'Administrator')) { - $sent_groups = $data['groups']; + $sent_groups = $request->input('groups'); } + $data = $request->only([ + 'name', 'email', 'location', 'age', 'gender', 'country_code', + 'biography', 'language', 'newsletter', 'invites', + ]); + $error = false; // check for email in use $editingUser = $User->find($id); @@ -832,21 +835,14 @@ public function edit($id, Request $request) $error['email'] = 'The email you entered is already in use in our database. Please use another one.'; } - if (! empty($data['new-password'])) { - if ($data['new-password'] !== $data['password-confirm']) { + 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($data['new-password']); + $data['password'] = Hash::make($request->input('new-password')); } } - unset($data['new-password']); - unset($data['password-confirm']); - - unset($data['groups']); - unset($data['profile']); - unset($data['id']); - if (! is_array($error)) { $u = $User->find($id)->update($data); From e0c5e09112e7ee1344a0eb39be467457df03c0a4 Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Fri, 24 Apr 2026 10:24:07 -0700 Subject: [PATCH 3/4] fix(model): remove role and api_token from User $fillable These sensitive fields were mass-assignable, enabling privilege escalation if any controller passed unfiltered request data to User::update() or User::create(). All call sites that previously set role via mass assignment now use direct property assignment. --- app/Console/Commands/UserCreate.php | 4 +++- app/Http/Controllers/Auth/RegisterController.php | 5 ++++- app/Http/Controllers/UserController.php | 4 +++- app/Models/User.php | 5 ++++- 4 files changed, 14 insertions(+), 4 deletions(-) diff --git a/app/Console/Commands/UserCreate.php b/app/Console/Commands/UserCreate.php index fd08f1d5ee..2ebd83f65a 100644 --- a/app/Console/Commands/UserCreate.php +++ b/app/Console/Commands/UserCreate.php @@ -101,7 +101,6 @@ public function handle(DiscourseService $discourseService): void 'name' => $name, 'email' => $email, 'password' => Hash::make($password), - 'role' => $role, 'recovery' => substr(bin2hex(openssl_random_pseudo_bytes(32)), 0, 24), 'recovery_expires' => strftime('%Y-%m-%d %X', time() + (24 * 60 * 60)), 'calendar_hash' => Str::random(15), @@ -118,6 +117,9 @@ public function handle(DiscourseService $discourseService): void if ($user) { + $user->role = $role; + $user->save(); + $this->info("User created {$user->name} (#{$user->id})"); $this->info("Role: {$this->getRoleName($user->role)}"); diff --git a/app/Http/Controllers/Auth/RegisterController.php b/app/Http/Controllers/Auth/RegisterController.php index 1920b75796..ec8f1a46dc 100644 --- a/app/Http/Controllers/Auth/RegisterController.php +++ b/app/Http/Controllers/Auth/RegisterController.php @@ -5,6 +5,7 @@ use Illuminate\Routing\Controllers\HasMiddleware; use Illuminate\Routing\Controllers\Middleware; use App\Http\Controllers\Controller; +use App\Models\Role; use App\Models\User; use Illuminate\Foundation\Auth\RegistersUsers; use Illuminate\Support\Facades\Hash; @@ -62,11 +63,13 @@ protected function create(array $data): User 'name' => $data['name'], 'email' => $data['email'], 'password' => Hash::make($data['password']), - 'role' => 4, 'recovery' => substr(bin2hex(openssl_random_pseudo_bytes(32)), 0, 24), 'recovery_expires' => strftime('%Y-%m-%d %X', time() + (24 * 60 * 60)), ]); + $user->role = Role::RESTARTER; + $user->save(); + Session::createSession($user->id); return $user; diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 0504b44508..7885306162 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -1007,7 +1007,6 @@ public function postRegister(Request $request, $hash = null): RedirectResponse 'name' => $request->input('name'), 'email' => $request->input('email'), 'password' => Hash::make($request->input('password')), - 'role' => $role, 'recovery' => substr(bin2hex(openssl_random_pseudo_bytes(32)), 0, 24), 'recovery_expires' => strftime('%Y-%m-%d %X', time() + (24 * 60 * 60)), 'country_code' => $request->input('country'), @@ -1017,6 +1016,9 @@ public function postRegister(Request $request, $hash = null): RedirectResponse 'calendar_hash' => Str::random(15), 'username' => '', ]); + + $user->role = $role; + $user->save(); } $user->generateAndSetUsername(); diff --git a/app/Models/User.php b/app/Models/User.php index 9cd00516e5..2d34a32d2c 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -40,8 +40,11 @@ class User extends Authenticatable implements Auditable, HasLocalePreference * * @var array */ + // Note: `role` and `api_token` are intentionally excluded from $fillable. + // They must be set via direct assignment ($user->role = X; $user->save()) + // to prevent privilege escalation via mass assignment (security: C2/M1). protected $fillable = [ - 'name', 'email', 'password', 'role', 'recovery', 'recovery_expires', 'language', 'repair_network', 'location', 'age', 'gender', 'country_code', 'newsletter', 'invites', 'biography', 'consent_future_data', 'consent_past_data', 'consent_gdpr', 'number_of_logins', 'latitude', 'longitude', 'last_login_at', 'api_token', 'access_group_tag_id', 'calendar_hash', 'repairdir_role', 'mediawiki', 'username', 'external_user_id', 'external_username', + 'name', 'email', 'password', 'recovery', 'recovery_expires', 'language', 'repair_network', 'location', 'age', 'gender', 'country_code', 'newsletter', 'invites', 'biography', 'consent_future_data', 'consent_past_data', 'consent_gdpr', 'number_of_logins', 'latitude', 'longitude', 'last_login_at', 'access_group_tag_id', 'calendar_hash', 'repairdir_role', 'mediawiki', 'username', 'external_user_id', 'external_username', ]; /** From f41ee6a7e40d0fb4872fef4bfde6ef0d5f1f93f2 Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Fri, 24 Apr 2026 10:27:54 -0700 Subject: [PATCH 4/4] test(auth): add privilege escalation security tests Eight tests covering the C1 (IDOR), C2 (mass assignment), and M1 (sensitive fillable fields) vulnerabilities. Includes both negative cases verifying non-admin users are blocked and positive cases confirming administrators retain access. --- .../Feature/Users/PrivilegeEscalationTest.php | 179 ++++++++++++++++++ 1 file changed, 179 insertions(+) create mode 100644 tests/Feature/Users/PrivilegeEscalationTest.php diff --git a/tests/Feature/Users/PrivilegeEscalationTest.php b/tests/Feature/Users/PrivilegeEscalationTest.php new file mode 100644 index 0000000000..9a7ed4774d --- /dev/null +++ b/tests/Feature/Users/PrivilegeEscalationTest.php @@ -0,0 +1,179 @@ +restarter()->create(); + $victim = User::factory()->restarter()->create(); + + $this->actingAs($attacker); + + $response = $this->post('/profile/edit-info', [ + 'id' => $victim->id, + 'email' => $victim->email, + 'name' => 'Hacked Name', + 'country' => $victim->country_code, + 'townCity' => $victim->location, + 'age' => $victim->age, + 'gender' => $victim->gender, + 'biography' => 'hacked', + ]); + + $response->assertStatus(403); + $this->assertNotEquals('Hacked Name', $victim->fresh()->name); + } + + /** + * C1: Restarter cannot change another user's password. + */ + public function test_restarter_cannot_change_another_users_password(): void + { + $attacker = User::factory()->restarter()->create(); + $victim = User::factory()->restarter()->create(); + + $this->actingAs($attacker); + + $response = $this->post('/profile/edit-password', [ + 'id' => $victim->id, + 'current-password' => 'secret', + 'new-password' => 'hacked123', + 'new-password-repeat' => 'hacked123', + ]); + + $response->assertStatus(403); + } + + /** + * C1: Restarter cannot change another user's photo. + */ + public function test_restarter_cannot_change_another_users_photo(): void + { + $attacker = User::factory()->restarter()->create(); + $victim = User::factory()->restarter()->create(); + + $this->actingAs($attacker); + + $response = $this->post('/profile/edit-photo', [ + 'id' => $victim->id, + ]); + + $response->assertStatus(403); + } + + /** + * C1: Restarter cannot access admin edit settings. + */ + public function test_restarter_cannot_access_admin_edit_settings(): void + { + $attacker = User::factory()->restarter()->create(); + + $this->actingAs($attacker); + + $response = $this->post('/profile/edit-admin-settings', [ + 'id' => $attacker->id, + 'user_role' => Role::ROOT, + ]); + + $response->assertStatus(403); + $this->assertEquals(Role::RESTARTER, $attacker->fresh()->role); + } + + /** + * C1 positive: Admin CAN change role via admin edit settings. + */ + public function test_admin_can_change_role_via_admin_edit(): void + { + $admin = User::factory()->administrator()->create(); + $user = User::factory()->restarter()->create(); + + $this->actingAs($admin); + + $response = $this->post('/profile/edit-admin-settings', [ + 'id' => $user->id, + 'user_role' => Role::HOST, + 'assigned_groups' => [], + 'preferences' => [], + 'permissions' => [], + ]); + + $response->assertSessionHas('message'); + $this->assertEquals(Role::HOST, $user->fresh()->role); + } + + /** + * C1 positive: Admin CAN edit another user's info. + */ + public function test_admin_can_edit_another_users_info(): void + { + $admin = User::factory()->administrator()->create(); + $user = User::factory()->restarter()->create(); + + $this->actingAs($admin); + + $response = $this->post('/profile/edit-info', [ + 'id' => $user->id, + 'email' => $user->email, + 'name' => 'Updated By Admin', + 'country' => $user->country_code, + 'townCity' => $user->location, + 'age' => $user->age, + 'gender' => $user->gender, + 'biography' => $user->biography, + ]); + + $response->assertRedirect(); + $this->assertEquals('Updated By Admin', $user->fresh()->name); + } + + /** + * C2/M1: User cannot escalate role via /user/edit endpoint. + */ + public function test_user_cannot_escalate_role_via_edit_endpoint(): void + { + $admin = User::factory()->administrator()->create(); + $user = User::factory()->restarter()->create(); + + $this->actingAs($admin); + + $response = $this->post("/user/edit/{$user->id}", [ + 'name' => $user->name, + 'email' => $user->email, + 'role' => Role::ROOT, + ]); + + $this->assertNotEquals(Role::ROOT, $user->fresh()->role); + } + + /** + * C2/M1: User cannot overwrite api_token via /user/edit endpoint. + */ + public function test_user_cannot_overwrite_api_token_via_edit_endpoint(): void + { + $admin = User::factory()->administrator()->create(); + $user = User::factory()->restarter()->create(); + $originalToken = $user->api_token; + + $this->actingAs($admin); + + $response = $this->post("/user/edit/{$user->id}", [ + 'name' => $user->name, + 'email' => $user->email, + 'api_token' => 'stolen_token_value', + ]); + + $this->assertEquals($originalToken, $user->fresh()->api_token); + } +}