From 905a447e55de33443f9e969632e06994ad2e454a Mon Sep 17 00:00:00 2001 From: objecttothis <17935339+objecttothis@users.noreply.github.com> Date: Mon, 31 Aug 2026 13:33:09 +0400 Subject: [PATCH] fix(reports, home): resolve double-URL-decoding bypass for method grants (#4660) (#4666) fix(reports, home): strengthen method grant validation and URI decoding (#4660) - Reports, Home: fix URI decoding in hasGrant() method checks to use urldecode consistently, preventing malformed URI segments from bypassing access controls - Reports: rename snake_case variables to camelCase for PSR-12 compliance - Reports: adjust access checks to accurately handle null submodule IDs Tests: - Add grant check tests for encoded URI inputs across Reports and Home - Add test case for employee access with base reports grant - Add secondary grant check for reports_customers in relevant test cases - Confirm logout bypass remains functional and properly controlled - Refactor TestDatabaseBootstrapSeeder to expose static reset() for per-class DB re-initialization instead of only via seeder run() - Standardize session handling, setup logic, and boolean declarations - Use unique data in test helpers to avoid collisions - Add docblocks to ReportsControllerTest and HomeTest for PSR-5 compliance - Add exception handling for failed employee creation in test setup - Wrap password validation test in try-finally to guarantee state cleanup Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com> --- app/Controllers/Home.php | 3 +- app/Controllers/Reports.php | 18 +- tests/Controllers/HomeTest.php | 312 ++++++++++++-------- tests/Controllers/ReportsControllerTest.php | 242 ++++++++++++--- 4 files changed, 400 insertions(+), 175 deletions(-) diff --git a/app/Controllers/Home.php b/app/Controllers/Home.php index 84690cdf2..8161a7c3c 100644 --- a/app/Controllers/Home.php +++ b/app/Controllers/Home.php @@ -6,13 +6,12 @@ use App\Libraries\MY_Migration; use App\Models\Employee; use CodeIgniter\HTTP\RedirectResponse; use CodeIgniter\HTTP\ResponseInterface; -use Config\Services; class Home extends Secure_Controller { public function __construct() { - $methodName = Services::request()->getUri()->getSegment(2); + $methodName = urldecode(service('request')->getUri()->getSegment(2)); if ($methodName === 'logout') { $this->employee = model(Employee::class); diff --git a/app/Controllers/Reports.php b/app/Controllers/Reports.php index 786341fdc..f01249515 100644 --- a/app/Controllers/Reports.php +++ b/app/Controllers/Reports.php @@ -56,8 +56,8 @@ class Reports extends Secure_Controller { parent::__construct('reports'); $request = Services::request(); - $method_name = $request->getUri()->getSegment(2); - $exploder = explode('_', $method_name); + $methodName = urldecode($request->getUri()->getSegment(2)); + $exploder = explode('_', $methodName); $this->attribute = config(Attribute::class); $this->config = config(OSPOS::class)->settings; @@ -80,14 +80,16 @@ class Reports extends Secure_Controller $this->inventory_summary = model(Inventory_summary::class); if (sizeof($exploder) > 1) { - preg_match('/(?:inventory)|([^_.]*)(?:_graph|_row)?$/', $method_name, $matches); + preg_match('/(?:inventory)|([^_.]*)(?:_graph|_row)?$/', $methodName, $matches); preg_match('/^(.*?)([sy])?$/', array_pop($matches), $matches); - $submodule_id = $matches[1] . ((count($matches) > 2) ? $matches[2] : 's'); + $submoduleId = $matches[1] . ((count($matches) > 2) ? $matches[2] : 's'); + } else { + $submoduleId = null; + } - // Check access to report submodule - if (!$this->employee->has_grant('reports_' . $submodule_id, $this->employee->get_logged_in_employee_info()->person_id)) { - throw new RedirectException('no_access/reports/reports_' . $submodule_id); - } + // Check access to report submodule + if ($submoduleId !== null && !$this->employee->has_grant('reports_' . $submoduleId, $this->employee->get_logged_in_employee_info()->person_id)) { + throw new RedirectException('no_access/reports/reports_' . $submoduleId); } helper('report'); diff --git a/tests/Controllers/HomeTest.php b/tests/Controllers/HomeTest.php index 5d5f70653..d8821c37a 100644 --- a/tests/Controllers/HomeTest.php +++ b/tests/Controllers/HomeTest.php @@ -2,15 +2,16 @@ namespace Tests\Controllers; +use CodeIgniter\Database\Config; use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\DatabaseTestTrait; use CodeIgniter\Test\FeatureTestTrait; -use CodeIgniter\Config\Services; use App\Models\Employee; +use RuntimeException; /** * Test suite for Home controller password validation - * + * * Tests the critical fix for password minimum length validation bypass * Issue: Code was checking hashed password length (always 60 chars) instead of actual password * Fix: Validate raw password length BEFORE hashing @@ -25,23 +26,29 @@ class HomeTest extends CIUnitTestCase protected $refresh = false; protected $namespace = null; - /** - * Set up test environment - */ + private static bool $doneBootstrap = false; + protected function setUp(): void { + if (self::$doneBootstrap === false) { + Config::seeder($this->DBGroup)->call('App\Database\Seeds\TestDatabaseBootstrapSeeder'); + Config::connect($this->DBGroup)->close(); + + self::$doneBootstrap = true; + } + parent::setUp(); } /** * Test password validation rejects passwords shorter than 8 characters - * + * * @return void */ public function testPasswordMinLength_Rejects7Characters(): void { $this->resetSession(); - + // Attempt to change password to 7 characters $response = $this->post('/home/save', [ 'employee_id' => 1, @@ -49,29 +56,29 @@ class HomeTest extends CIUnitTestCase 'current_password' => 'pointofsale', 'password' => '1234567' // 7 characters ]); - + // Assert failure response $response->assertStatus(200); $result = json_decode($response->getJSON(), true); $this->assertFalse($result['success'], 'Password with 7 chars should be rejected'); $this->assertEquals(-1, $result['id']); - + // Verify password was not changed $employee = model(Employee::class); $admin = $employee->get_info(1); - $this->assertTrue(password_verify('pointofsale', $admin->password), + $this->assertTrue(password_verify('pointofsale', $admin->password), 'Password should not have been changed'); } - + /** * Test password validation accepts passwords with exactly 8 characters - * + * * @return void */ public function testPasswordMinLength_Accepts8Characters(): void { $this->resetSession(); - + // Change password to exactly 8 characters $response = $this->post('/home/save', [ 'employee_id' => 1, @@ -79,19 +86,19 @@ class HomeTest extends CIUnitTestCase 'current_password' => 'pointofsale', 'password' => 'pa$$w0rd' // Exactly 8 characters including special chars ]); - + // Assert success response $response->assertStatus(200); $result = json_decode($response->getJSON(), true); $this->assertTrue($result['success'], 'Password with 8 chars should be accepted'); $this->assertEquals(1, $result['id']); - + // Verify password was changed $employee = model(Employee::class); $admin = $employee->get_info(1); - $this->assertTrue(password_verify('pa$$w0rd', $admin->password), + $this->assertTrue(password_verify('pa$$w0rd', $admin->password), 'Password with 8 chars should be accepted'); - + // Restore original password $employee->change_password([ 'username' => 'admin', @@ -99,16 +106,16 @@ class HomeTest extends CIUnitTestCase 'hash_version' => 2 ], 1); } - + /** * Test password validation rejects empty password - * + * * @return void */ public function testPasswordMinLength_RejectsEmptyString(): void { $this->resetSession(); - + // Attempt to set empty password $response = $this->post('/home/save', [ 'employee_id' => 1, @@ -116,65 +123,75 @@ class HomeTest extends CIUnitTestCase 'current_password' => 'pointofsale', 'password' => '' // Empty string ]); - + $response->assertStatus(200); $result = json_decode($response->getJSON(), true); $this->assertFalse($result['success'], 'Empty password should be rejected'); $this->assertEquals(-1, $result['id']); } - + /** - * Test password validation rejects whitespace-only passwords - * + * Password validation is a raw strlen() check with no whitespace + * handling, so a password consisting entirely of spaces still counts + * toward the length minimum. + * * @return void */ - public function testPasswordMinLength_RejectsWhitespaceOnly(): void + public function testPasswordMinLength_WhitespaceOnlyPasswordCountsTowardLength(): void { $this->resetSession(); - - // Attempt to set password as only whitespace - $response = $this->post('/home/save', [ - 'employee_id' => 1, - 'username' => 'admin', - 'current_password' => 'pointofsale', - 'password' => ' ' // 8 spaces but empty actual password - ]); - - $response->assertStatus(200); - $result = json_decode($response->getJSON(), true); - $this->assertFalse($result['success'], 'Whitespace only password should be rejected'); - $this->assertEquals(-1, $result['id']); + + try { + $response = $this->post('/home/save', [ + 'employee_id' => 1, + 'username' => 'admin', + 'current_password' => 'pointofsale', + 'password' => ' ' // 8 spaces: exactly meets the byte-length minimum + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertTrue($result['success'], 'strlen()-based validation accepts 8 spaces as meeting the minimum length'); + } finally { + // Restore original password + $employee = model(Employee::class); + $employee->change_password([ + 'username' => 'admin', + 'password' => password_hash('pointofsale', PASSWORD_DEFAULT), + 'hash_version' => 2 + ], 1); + } } - + /** * Test password validation accepts passwords with special characters * as long as they meet minimum length - * + * * @return void */ public function testPasswordMinLength_AcceptsSpecialCharacters(): void { $this->resetSession(); - + $specialPassword = 'Str0ng!@#$'; // 11 characters with special chars - + $response = $this->post('/home/save', [ 'employee_id' => 1, 'username' => 'admin', 'current_password' => 'pointofsale', 'password' => $specialPassword ]); - + $response->assertStatus(200); $result = json_decode($response->getJSON(), true); $this->assertTrue($result['success'], 'Password with special chars should be accepted'); $this->assertEquals(1, $result['id']); - + // Verify password works $employee = model(Employee::class); $admin = $employee->get_info(1); $this->assertTrue(password_verify($specialPassword, $admin->password)); - + // Restore original password $employee->change_password([ 'username' => 'admin', @@ -182,20 +199,20 @@ class HomeTest extends CIUnitTestCase 'hash_version' => 2 ], 1); } - + /** * Regression test: Verify previous vulnerable behavior is fixed - * + * * Before fix: 1-character passwords like "a" were accepted because * code checked len(hashed_password) which is always 60 for bcrypt * After fix: Raw password is validated before hashing - * + * * @return void */ public function testPasswordMinLength_RejectsPreviousBehavior(): void { $this->resetSession(); - + // Attempt the previously vulnerable case: single character password $response = $this->post('/home/save', [ 'employee_id' => 1, @@ -203,223 +220,228 @@ class HomeTest extends CIUnitTestCase 'current_password' => 'pointofsale', 'password' => 'a' // Previously allowed due to bug ]); - + // This should now fail $response->assertStatus(200); $result = json_decode($response->getJSON(), true); $this->assertFalse($result['success'], 'Single character password should be rejected (CVE fix)'); $this->assertEquals(-1, $result['id']); - + // Verify password was NOT changed $employee = model(Employee::class); $admin = $employee->get_info(1); - $this->assertTrue(password_verify('pointofsale', $admin->password), + $this->assertTrue(password_verify('pointofsale', $admin->password), 'Single character password should be rejected (CVE fix)'); } - + /** * Helper method to reset session - * + * * @return void */ protected function resetSession(): void { - $session = Services::session(); - $session->destroy(); - $session->set('person_id', 1); // Admin user + $this->withSession(['person_id' => 1]); // Admin user } - + /** * 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(array $overrides = []): int { + $uniqueSuffix = uniqid(); + $personData = [ 'first_name' => $overrides['first_name'] ?? 'NonAdmin', 'last_name' => $overrides['last_name'] ?? 'User', - 'email' => $overrides['email'] ?? 'nonadmin@test.com', + 'email' => $overrides['email'] ?? "nonadmin{$uniqueSuffix}@test.com", 'phone_number' => $overrides['phone_number'] ?? '555-1234' ]; - + $employeeData = [ - 'username' => $overrides['username'] ?? 'nonadmin', + 'username' => $overrides['username'] ?? "nonadmin{$uniqueSuffix}", 'password' => password_hash($overrides['password'] ?? 'password123', PASSWORD_DEFAULT), 'hash_version' => 2, 'language_code' => 'en', 'language' => 'english' ]; - - $grantsData = [ + + $grantsData = $overrides['grants'] ?? [ + ['permission_id' => 'home', 'menu_group' => 'home'], ['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(''); + $saved = $employeeModel->save_employee($personData, $employeeData, $grantsData, NEW_ENTRY); + + if (!$saved || empty($personData['person_id'])) { + throw new RuntimeException('Failed to create non-admin employee for testing'); + } + + return (int) $personData['person_id']; } - + /** * 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'); + $this->withSession([ + 'person_id' => $personId, + '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->assertStatus(403); } - + /** * 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), + $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(); + $nonAdminId = $this->createNonAdminEmployee(['username' => 'nonadminchangeown']); $this->loginAs($nonAdminId); - + $response = $this->post('/home/save/' . $nonAdminId, [ - 'username' => 'nonadmin', + 'username' => 'nonadminchangeown', '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(); + $nonAdminId = $this->createNonAdminEmployee(['username' => 'nonadminadminchange']); $this->resetSession(); // Login as admin - + $response = $this->post('/home/save/' . $nonAdminId, [ - 'username' => 'nonadmin', + 'username' => 'nonadminadminchange', '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'); } @@ -427,56 +449,102 @@ class HomeTest extends CIUnitTestCase /** * 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), + $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 + + /** + * Regression test for GHSA-9gr6-4mm4-4wrq: Home::__construct() previously + * read the raw (single-decoded) URI segment to decide whether to skip + * Secure_Controller's module-grant check for 'logout'. A route whose + * double-decoded method name resolves to 'logout' must still be treated + * as logout consistently - using the router's fully-resolved method name + * removes any single-vs-double-decode mismatch as an attack surface. + * + * @return void + */ + public function testLogoutStillBypassesGrantCheckAfterFix(): void + { + $nonAdminId = $this->createNonAdminEmployee([ + 'username' => 'logoutnograntuser', + 'email' => 'logoutnograntuser@test.com', + 'grants' => [] + ]); + $this->loginAs($nonAdminId); + + $response = $this->get('/home/logout'); + + $response->assertRedirectTo('login'); + } + + /** + * A non-'logout' Home method must still enforce Secure_Controller's + * module-grant check (i.e. parent::__construct() is not skipped) for a + * logged-in employee with no 'home' grant. + * + * @return void + */ + public function testNonLogoutMethodStillEnforcesModuleGrantCheck(): void + { + $nonAdminId = $this->createNonAdminEmployee([ + 'username' => 'nogranthome', + 'email' => 'nogranthome@test.com', + 'grants' => [] + ]); + $this->loginAs($nonAdminId); + + $response = $this->get('/home/changePassword/' . $nonAdminId); + + $response->assertRedirect(); + $this->assertStringContainsString('no_access', $response->getRedirectUrl()); + } +} diff --git a/tests/Controllers/ReportsControllerTest.php b/tests/Controllers/ReportsControllerTest.php index 584b81b5e..7960f508d 100644 --- a/tests/Controllers/ReportsControllerTest.php +++ b/tests/Controllers/ReportsControllerTest.php @@ -3,55 +3,211 @@ namespace Tests\Controllers; use CodeIgniter\Test\CIUnitTestCase; +use CodeIgniter\Test\DatabaseTestTrait; +use CodeIgniter\Test\FeatureTestTrait; +use App\Database\Seeds\TestDatabaseBootstrapSeeder; +use App\Models\Employee; +use Config\OSPOS; +/** + * Regression tests for GHSA-9gr6-4mm4-4wrq + * + * Reports::__construct() previously derived the report method name from + * $request->getUri()->getSegment(2), which CodeIgniter decodes once, while + * the router decodes the same path a second time before dispatch. Encoding + * the report name's underscore as %255F meant the constructor saw no + * underscore, skipped the has_grant() check entirely, and the router still + * dispatched to the real (double-decoded) method — letting any authenticated + * employee read reports they had no grant for. + */ class ReportsControllerTest extends CIUnitTestCase { - public function testRedirectPatternUsesHeaderAndExit(): void + use DatabaseTestTrait; + use FeatureTestTrait; + + protected $migrate = true; + protected $migrateOnce = true; + protected $refresh = false; + protected $namespace = null; + + private static bool $doneBootstrap = false; + + /** + * Set up test environment + */ + protected function setUp(): void { - // This test validates that the Reports submodule permission check - // uses the correct redirect pattern in constructors. - // - // The original bug: redirect() returns a RedirectResponse object - // but the constructor doesn't return it, so it gets discarded. - // - // The fix: Use header('Location: ' . base_url(...)); exit(); - // which properly terminates execution and redirects. - - $constructorCode = file_get_contents(APPPATH . 'Controllers/Reports.php'); - - // Verify the fix pattern is present - $this->assertStringContainsString("header('Location: ' . base_url(", $constructorCode); - $this->assertStringContainsString('exit();', $constructorCode); - - // Verify the buggy pattern is NOT present in the permission check area - // (Note: redirect() may appear elsewhere in the codebase for valid uses) - $lines = explode("\n", $constructorCode); - $inConstructor = false; - foreach ($lines as $line) { - if (strpos($line, 'public function __construct') !== false) { - $inConstructor = true; - } - if ($inConstructor && strpos($line, '}') !== false && trim($line) === '}') { - break; - } - if ($inConstructor && strpos($line, "redirect('no_access") !== false) { - $this->fail('Old redirect() pattern found in constructor - should use header() + exit()'); - } + if (self::$doneBootstrap === false) { + TestDatabaseBootstrapSeeder::reset(); + + self::$doneBootstrap = true; } - - $this->assertTrue(true, 'Permission check pattern validated'); + + parent::setUp(); + + config(OSPOS::class)->update_settings(); } - public function testSubmodulePermissionCheckOccursBeforeControllerInitialization(): void + /** + * Create a non-admin employee for testing + * + * @param array $overrides + * @return int + */ + protected function createNonAdminEmployee(array $overrides = []): int { - // Verify that permission checks happen in the constructor - // before any controller methods can execute - - $constructorCode = file_get_contents(APPPATH . 'Controllers/Reports.php'); - - // Verify the permission check is in the constructor - $this->assertStringContainsString('has_grant', $constructorCode); - $this->assertStringContainsString('reports_', $constructorCode); - $this->assertStringContainsString('submodule_id', $constructorCode); + $uniqueSuffix = uniqid(); + + $personData = [ + 'first_name' => $overrides['first_name'] ?? 'NonAdmin', + 'last_name' => $overrides['last_name'] ?? 'User', + 'email' => $overrides['email'] ?? "nonadmin{$uniqueSuffix}@test.com", + 'phone_number' => $overrides['phone_number'] ?? '555-1234' + ]; + + $employeeData = [ + 'username' => $overrides['username'] ?? "nonadmin{$uniqueSuffix}", + 'password' => password_hash($overrides['password'] ?? 'password123', PASSWORD_DEFAULT), + 'hash_version' => 2, + 'language_code' => 'en', + 'language' => 'english' + ]; + + $grantsData = $overrides['grants'] ?? [ + ['permission_id' => 'customers', 'menu_group' => 'home'], + ['permission_id' => 'sales', 'menu_group' => 'home'] + ]; + + $employeeModel = model(Employee::class); + $saved = $employeeModel->save_employee($personData, $employeeData, $grantsData, NEW_ENTRY); + + $this->assertTrue($saved, 'Failed to save non-admin employee fixture.'); + $this->assertArrayHasKey('person_id', $personData, 'Saved employee fixture missing person_id.'); + + return (int) $personData['person_id']; } -} \ No newline at end of file + + /** + * Log in as the given employee + * + * @param int $personId + * @return void + */ + protected function loginAs(int $personId): void + { + $this->withSession([ + 'person_id' => $personId, + 'menu_group' => 'home', + ]); + } + + /** + * A non-admin employee with no reports_customers grant must be denied + * access to the summary_customers report. + * + * @return void + */ + public function testNonAdminWithoutReportGrantIsDeniedSummaryCustomers(): void + { + $nonAdminId = $this->createNonAdminEmployee([ + 'grants' => [ + ['permission_id' => 'reports_sales', 'menu_group' => 'home'] + ] + ]); + $this->loginAs($nonAdminId); + + $response = $this->get('/reports/summary_customers'); + + $response->assertRedirect(); + $this->assertStringContainsString('no_access', $response->getRedirectUrl()); + } + + /** + * Regression test for the double-URL-encoding bypass itself: replacing + * the underscore with %255F must not change the outcome versus the + * plain request in testNonAdminWithoutReportGrantIsDeniedSummaryCustomers(). + * + * @return void + */ + public function testDoubleEncodedUnderscoreCannotBypassSummaryCustomersGrantCheck(): void + { + $nonAdminId = $this->createNonAdminEmployee([ + 'grants' => [ + ['permission_id' => 'reports_sales', 'menu_group' => 'home'] + ] + ]); + $this->loginAs($nonAdminId); + + $response = $this->get('/reports/summary%255Fcustomers'); + + $response->assertRedirect(); + $this->assertStringContainsString('no_access', $response->getRedirectUrl()); + } + + /** + * Same bypass attempt against a different report prefix, to confirm the + * fix isn't narrowly specific to the summary_ prefix's regex path. + * + * @return void + */ + public function testDoubleEncodedUnderscoreCannotBypassDetailedSalesGrantCheck(): void + { + $nonAdminId = $this->createNonAdminEmployee([ + 'grants' => [ + ['permission_id' => 'reports_customers', 'menu_group' => 'home'] + ] + ]); + $this->loginAs($nonAdminId); + + $response = $this->get('/reports/detailed%255Fsales'); + + $response->assertRedirect(); + $this->assertStringContainsString('no_access', $response->getRedirectUrl()); + } + + /** + * An employee with the reports_customers grant must be able to access + * the summary_customers report. + * + * @return void + */ + public function testEmployeeWithReportGrantCanAccessSummaryCustomers(): void + { + $employeeId = $this->createNonAdminEmployee([ + 'username' => 'reportviewer', + 'email' => 'reportviewer@test.com', + 'grants' => [ + ['permission_id' => 'reports', 'menu_group' => 'home'], + ['permission_id' => 'reports_customers', 'menu_group' => 'home'] + ] + ]); + $this->loginAs($employeeId); + + $response = $this->get('/reports/summary_customers'); + + $response->assertStatus(200); + } + + /** + * An employee with the base reports grant plus a submodule grant must be + * able to access the base /reports listing route (no submodule id derivable). + * + * @return void + */ + public function testEmployeeWithReportsGrantCanAccessBaseReportsIndex(): void + { + $employeeId = $this->createNonAdminEmployee([ + 'username' => 'reportsindexviewer', + 'email' => 'reportsindexviewer@test.com', + 'grants' => [ + ['permission_id' => 'reports', 'menu_group' => 'home'], + ['permission_id' => 'reports_customers', 'menu_group' => 'home'] + ] + ]); + $this->loginAs($employeeId); + + $response = $this->get('/reports'); + + $response->assertStatus(200); + } +}