From ee4d44ed396097d6010c5490ab4fd7cfae694624 Mon Sep 17 00:00:00 2001 From: jekkos Date: Fri, 13 Mar 2026 11:13:21 +0000 Subject: [PATCH] Fix IDOR vulnerability in password change (GHSA-mcc2-8rp2-q6ch) (#4427) * Fix IDOR vulnerability in password change (GHSA-mcc2-8rp2-q6ch) The previous authorization check using can_modify_employee() was too permissive - it allowed non-admin users to change other non-admin users' passwords. For password changes, users should only be able to change their own password. Only admins should be able to change any user's password. This fix replaces the can_modify_employee() check with a stricter authorization that only allows: - Users to change their own password - Admins to change any user's password Affected endpoints: - GET /home/changePassword/{employee_id} - POST /home/save/{employee_id} Added tests to verify non-admin users cannot access or change other non-admin users' passwords. * Address PR review feedback - Replace header/exit redirect with proper 403 response in getChangePassword - Refactor createNonAdminEmployee helper to accept overrides array - Simplify tests by reusing the helper - Update tests to expect 403 response instead of redirect --------- Co-authored-by: Ollama --- app/Controllers/Home.php | 11 +++-- tests/Controllers/HomeTest.php | 74 +++++++++++++++++++++++++++++----- 2 files changed, 70 insertions(+), 15 deletions(-) diff --git a/app/Controllers/Home.php b/app/Controllers/Home.php index fb5177925..97d270ffd 100644 --- a/app/Controllers/Home.php +++ b/app/Controllers/Home.php @@ -36,19 +36,18 @@ class Home extends Secure_Controller /** * Load "change employee password" form * - * @return string + * @return ResponseInterface|string * @noinspection PhpUnused */ - public function getChangePassword(int $employeeId = NEW_ENTRY): string + public function getChangePassword(int $employeeId = NEW_ENTRY) { $loggedInEmployee = $this->employee->get_logged_in_employee_info(); $currentPersonId = $loggedInEmployee->person_id; $employeeId = $employeeId === NEW_ENTRY ? $currentPersonId : $employeeId; - if (!$this->employee->can_modify_employee($employeeId, $currentPersonId)) { - header('Location: ' . base_url('no_access/home/home')); - exit(); + if (!$this->employee->isAdmin($currentPersonId) && $employeeId !== $currentPersonId) { + return $this->response->setStatusCode(403)->setBody(lang('Employees.unauthorized_modify')); } $person_info = $this->employee->get_info($employeeId); @@ -71,7 +70,7 @@ class Home extends Secure_Controller $employeeId = $employeeId === NEW_ENTRY ? $currentUser->person_id : $employeeId; - if (!$this->employee->can_modify_employee($employeeId, $currentUser->person_id)) { + if (!$this->employee->isAdmin($currentUser->person_id) && $employeeId !== $currentUser->person_id) { return $this->response->setStatusCode(403)->setJSON([ 'success' => false, 'message' => lang('Employees.unauthorized_modify') diff --git a/tests/Controllers/HomeTest.php b/tests/Controllers/HomeTest.php index dddb1c716..5d5f70653 100644 --- a/tests/Controllers/HomeTest.php +++ b/tests/Controllers/HomeTest.php @@ -232,20 +232,21 @@ class HomeTest extends CIUnitTestCase /** * Create a non-admin employee for testing * + * @param array $overrides Optional overrides for username, email, password, etc. * @return int The person_id of the created employee */ - protected function createNonAdminEmployee(): int + protected function createNonAdminEmployee(array $overrides = []): int { $personData = [ - 'first_name' => 'NonAdmin', - 'last_name' => 'User', - 'email' => 'nonadmin@test.com', - 'phone_number' => '555-1234' + 'first_name' => $overrides['first_name'] ?? 'NonAdmin', + 'last_name' => $overrides['last_name'] ?? 'User', + 'email' => $overrides['email'] ?? 'nonadmin@test.com', + 'phone_number' => $overrides['phone_number'] ?? '555-1234' ]; $employeeData = [ - 'username' => 'nonadmin', - 'password' => password_hash('password123', PASSWORD_DEFAULT), + 'username' => $overrides['username'] ?? 'nonadmin', + 'password' => password_hash($overrides['password'] ?? 'password123', PASSWORD_DEFAULT), 'hash_version' => 2, 'language_code' => 'en', 'language' => 'english' @@ -291,8 +292,7 @@ class HomeTest extends CIUnitTestCase $response = $this->get('/home/changePassword/1'); - $response->assertRedirect(); - $this->assertStringContainsString('no_access', $response->getRedirectUrl()); + $response->assertStatus(403); } /** @@ -423,4 +423,60 @@ class HomeTest extends CIUnitTestCase $response->assertStatus(200); $response->assertSee('nonadmin'); } + + /** + * Test non-admin cannot view another non-admin's password form + * IDOR vulnerability fix: GHSA-mcc2-8rp2-q6ch + * + * @return void + */ + public function testNonAdminCannotViewOtherNonAdminPasswordForm(): void + { + $nonAdminId1 = $this->createNonAdminEmployee(); + $this->loginAs($nonAdminId1); + + $otherUserId = $this->createNonAdminEmployee([ + 'username' => 'otheruser', + 'email' => 'other@test.com', + 'password' => 'password456' + ]); + + $response = $this->get('/home/changePassword/' . $otherUserId); + + $response->assertStatus(403); + } + + /** + * Test non-admin cannot change another non-admin's password + * IDOR vulnerability fix: GHSA-mcc2-8rp2-q6ch + * + * @return void + */ + public function testNonAdminCannotChangeOtherNonAdminPassword(): void + { + $nonAdminId1 = $this->createNonAdminEmployee(); + $this->loginAs($nonAdminId1); + + $victimId = $this->createNonAdminEmployee([ + 'username' => 'victimuser', + 'email' => 'victim@test.com', + 'password' => 'victimpass123' + ]); + + $response = $this->post('/home/save/' . $victimId, [ + 'username' => 'victimuser', + 'current_password' => 'victimpass123', + 'password' => 'hacked123456' + ]); + + $response->assertStatus(403); + $result = json_decode($response->getJSON(), true); + $this->assertFalse($result['success']); + + // Verify victim's password was NOT changed + $employeeModel = model(Employee::class); + $victim = $employeeModel->get_info($victimId); + $this->assertTrue(password_verify('victimpass123', $victim->password), + 'Non-admin should not be able to change another non-admin password'); + } } \ No newline at end of file