From ca6a1b35af3d51a084f2bd4d145547d3650942bb Mon Sep 17 00:00:00 2001 From: jekkos Date: Fri, 6 Mar 2026 17:08:36 +0100 Subject: [PATCH] Add row-level authorization to password change endpoints (#4401) * fix(security): add row-level authorization to password change endpoints - Prevents non-admin users from viewing other users' password forms - Prevents non-admin users from changing other users' passwords - Uses can_modify_employee() check consistent with Employees controller fix - Addresses BOLA vulnerability in Home controller (GHSA-q58g-gg7v-f9rf) * test(security): add BOLA authorization tests for Home controller - Test non-admin cannot view/change admin password - Test user can view/change own password - Test admin can view/change any password - Test default employee_id uses current user - Add JUnit test result upload to CI workflow * refactor: apply PSR-12 naming and add DEFAULT_EMPLOYEE_ID constant - Add DEFAULT_EMPLOYEE_ID constant to Constants.php - Rename variables to follow PSR-12 camelCase convention - Use ternary for default employee ID assignment * refactor: use NEW_ENTRY constant instead of adding DEFAULT_EMPLOYEE_ID Reuse existing NEW_ENTRY constant for default employee ID parameter. Avoids adding redundant constants to Constants.php with same value (-1). --------- Co-authored-by: jekkos --- .github/workflows/phpunit.yml | 10 +- app/Controllers/Home.php | 49 ++++++--- tests/Controllers/HomeTest.php | 195 +++++++++++++++++++++++++++++++++ 3 files changed, 239 insertions(+), 15 deletions(-) diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index 3845ffcae..fe7b3ff48 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -111,7 +111,15 @@ jobs: env: CI_ENVIRONMENT: testing MYSQL_HOST_NAME: 127.0.0.1 - run: composer test + run: composer test -- --log-junit test-results/junit.xml + + - name: Upload test results + uses: actions/upload-artifact@v4 + if: always() + with: + name: test-results-php-${{ matrix.php-version }} + path: test-results/ + retention-days: 30 - name: Stop MariaDB if: always() diff --git a/app/Controllers/Home.php b/app/Controllers/Home.php index 88a5d0b20..fb5177925 100644 --- a/app/Controllers/Home.php +++ b/app/Controllers/Home.php @@ -39,9 +39,19 @@ class Home extends Secure_Controller * @return string * @noinspection PhpUnused */ - public function getChangePassword(int $employee_id = -1): string // TODO: Replace -1 with a constant + public function getChangePassword(int $employeeId = NEW_ENTRY): string { - $person_info = $this->employee->get_info($employee_id); + $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(); + } + + $person_info = $this->employee->get_info($employeeId); foreach (get_object_vars($person_info) as $property => $value) { $person_info->$property = $value; } @@ -55,9 +65,20 @@ class Home extends Secure_Controller * * @return ResponseInterface */ - public function postSave(int $employee_id = -1): ResponseInterface // TODO: Replace -1 with a constant + public function postSave(int $employeeId = NEW_ENTRY): ResponseInterface { - if (!empty($this->request->getPost('current_password')) && $employee_id != -1) { + $currentUser = $this->employee->get_logged_in_employee_info(); + + $employeeId = $employeeId === NEW_ENTRY ? $currentUser->person_id : $employeeId; + + if (!$this->employee->can_modify_employee($employeeId, $currentUser->person_id)) { + return $this->response->setStatusCode(403)->setJSON([ + 'success' => false, + 'message' => lang('Employees.unauthorized_modify') + ]); + } + + if (!empty($this->request->getPost('current_password')) && $employeeId != NEW_ENTRY) { if ($this->employee->check_password($this->request->getPost('username', FILTER_SANITIZE_FULL_SPECIAL_CHARS), $this->request->getPost('current_password'))) { // Validate password length BEFORE hashing $new_password = $this->request->getPost('password'); @@ -66,7 +87,7 @@ class Home extends Secure_Controller return $this->response->setJSON([ 'success' => false, 'message' => lang('Employees.password_minlength'), - 'id' => -1 + 'id' => NEW_ENTRY ]); } @@ -76,32 +97,32 @@ class Home extends Secure_Controller 'hash_version' => 2 ]; - if ($this->employee->change_password($employee_data, $employee_id)) { + if ($this->employee->change_password($employee_data, $employeeId)) { return $this->response->setJSON([ 'success' => true, 'message' => lang('Employees.successful_change_password'), - 'id' => $employee_id + 'id' => $employeeId ]); - } else { // Failure // TODO: Replace -1 with constant + } else { return $this->response->setJSON([ 'success' => false, 'message' => lang('Employees.unsuccessful_change_password'), - 'id' => -1 + 'id' => NEW_ENTRY ]); } - } else { // TODO: Replace -1 with constant + } else { return $this->response->setJSON([ 'success' => false, 'message' => lang('Employees.current_password_invalid'), - 'id' => -1 + 'id' => NEW_ENTRY ]); } - } else { // TODO: Replace -1 with constant + } else { return $this->response->setJSON([ 'success' => false, 'message' => lang('Employees.current_password_invalid'), - 'id' => -1 + 'id' => NEW_ENTRY ]); } } -} +} \ No newline at end of file diff --git a/tests/Controllers/HomeTest.php b/tests/Controllers/HomeTest.php index 0381779d5..dddb1c716 100644 --- a/tests/Controllers/HomeTest.php +++ b/tests/Controllers/HomeTest.php @@ -228,4 +228,199 @@ class HomeTest extends CIUnitTestCase $session->destroy(); $session->set('person_id', 1); // Admin user } + + /** + * Create a non-admin employee for testing + * + * @return int The person_id of the created employee + */ + protected function createNonAdminEmployee(): int + { + $personData = [ + 'first_name' => 'NonAdmin', + 'last_name' => 'User', + 'email' => 'nonadmin@test.com', + 'phone_number' => '555-1234' + ]; + + $employeeData = [ + 'username' => 'nonadmin', + 'password' => password_hash('password123', PASSWORD_DEFAULT), + 'hash_version' => 2, + 'language_code' => 'en', + 'language' => 'english' + ]; + + $grantsData = [ + ['permission_id' => 'customers', 'menu_group' => 'home'], + ['permission_id' => 'sales', 'menu_group' => 'home'] + ]; + + $employeeModel = model(Employee::class); + $employeeModel->save_employee($personData, $employeeData, $grantsData, NEW_ENTRY); + + return $employeeModel->get_found_rows(''); + } + + /** + * Login as a specific user + * + * @param int $personId + * @return void + */ + protected function loginAs(int $personId): void + { + $session = Services::session(); + $session->destroy(); + $session->set('person_id', $personId); + $session->set('menu_group', 'home'); + } + + // ========== BOLA Authorization Tests ========== + + /** + * Test non-admin cannot view admin password change form + * BOLA vulnerability fix: GHSA-q58g-gg7v-f9rf + * + * @return void + */ + public function testNonAdminCannotViewAdminPasswordForm(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->loginAs($nonAdminId); + + $response = $this->get('/home/changePassword/1'); + + $response->assertRedirect(); + $this->assertStringContainsString('no_access', $response->getRedirectUrl()); + } + + /** + * Test non-admin cannot change admin password + * BOLA vulnerability fix: GHSA-q58g-gg7v-f9rf + * + * @return void + */ + public function testNonAdminCannotChangeAdminPassword(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->loginAs($nonAdminId); + + $response = $this->post('/home/save/1', [ + 'username' => 'admin', + 'current_password' => 'pointofsale', + 'password' => 'hacked123' + ]); + + $response->assertStatus(403); + $result = json_decode($response->getJSON(), true); + $this->assertFalse($result['success']); + + // Verify admin password was NOT changed + $employee = model(Employee::class); + $admin = $employee->get_info(1); + $this->assertTrue(password_verify('pointofsale', $admin->password), + 'Admin password should not have been changed by non-admin'); + } + + /** + * Test user can view their own password change form + * + * @return void + */ + public function testUserCanViewOwnPasswordForm(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->loginAs($nonAdminId); + + $response = $this->get('/home/changePassword/' . $nonAdminId); + + $response->assertStatus(200); + $response->assertSee('nonadmin'); // Username should be visible + } + + /** + * Test user can change their own password + * + * @return void + */ + public function testUserCanChangeOwnPassword(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->loginAs($nonAdminId); + + $response = $this->post('/home/save/' . $nonAdminId, [ + 'username' => 'nonadmin', + 'current_password' => 'password123', + 'password' => 'newpassword123' + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertTrue($result['success']); + + // Verify password was changed + $employee = model(Employee::class); + $user = $employee->get_info($nonAdminId); + $this->assertTrue(password_verify('newpassword123', $user->password)); + } + + /** + * Test admin can view any user's password form + * + * @return void + */ + public function testAdminCanViewAnyPasswordForm(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->resetSession(); // Login as admin + + $response = $this->get('/home/changePassword/' . $nonAdminId); + + $response->assertStatus(200); + $response->assertSee('nonadmin'); + } + + /** + * Test admin can change any user's password + * + * @return void + */ + public function testAdminCanChangeAnyPassword(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->resetSession(); // Login as admin + + $response = $this->post('/home/save/' . $nonAdminId, [ + 'username' => 'nonadmin', + 'current_password' => 'password123', + 'password' => 'adminset123' + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertTrue($result['success']); + + // Verify password was changed + $employee = model(Employee::class); + $user = $employee->get_info($nonAdminId); + $this->assertTrue(password_verify('adminset123', $user->password)); + } + + /** + * Test default employee_id parameter uses current user + * + * @return void + */ + public function testDefaultEmployeeIdUsesCurrentUser(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->loginAs($nonAdminId); + + // Calling without employee_id should use current user + $response = $this->get('/home/changePassword'); + + $response->assertStatus(200); + $response->assertSee('nonadmin'); + } } \ No newline at end of file