mirror of
https://github.com/opensourcepos/opensourcepos.git
synced 2026-03-08 00:59:04 -05:00
Complete Content-Type application/json fix for all AJAX responses - Add missing return statements to all ->response->setJSON() calls - Fix Items.php method calls from JSON() to setJSON() - Convert echo statements to proper JSON responses - Ensure consistent Content-Type headers across all controllers - Fix 46+ instances across 12 controller files - Change Config.php methods to : ResponseInterface (all return setJSON only): - postSaveRewards(), postSaveBarcode(), postSaveReceipt() - postSaveInvoice(), postRemoveLogo() - Update PHPDoc @return tags - Change Receivings.php _reload() to : string (only returns view) - Change Receivings.php methods to : string (all return _reload()): - getIndex(), postSelectSupplier(), postChangeMode(), postAdd() - postEditItem(), getDeleteItem(), getRemoveSupplier() - postComplete(), postRequisitionComplete(), getReceipt(), postCancelReceiving() - Change postSave() to : ResponseInterface (returns setJSON) - Update all PHPDoc @return tags Fix XSS vulnerabilities in sales templates, login, and config pages This commit addresses 5 XSS vulnerabilities by adding proper escaping to all user-controlled configuration values in HTML contexts. Fixed Files: - app/Views/sales/invoice.php: Escaped company_logo (URL context) and company (HTML) - app/Views/sales/work_order.php: Escaped company_logo (URL context) - app/Views/sales/receipt_email.php: Added file path validation and escaping for logo - app/Views/login.php: Escaped all config values in title, logo src, and alt - app/Views/configs/info_config.php: Escaped company_logo (URL context) Security Impact: - Prevents stored XSS attacks if configuration is compromised - Defense-in-depth principle applied to administrative interfaces - Follows OWASP best practices for output encoding Testing: - Verified no script execution with XSS payloads in config values - Confirmed proper escaping in HTML, URL, and file contexts - All templates render correctly with valid configuration Severity: High (4 files), Medium-High (1 file) CVSS Score: ~6.1 CWE: CWE-79 (Improper Neutralization of Input During Web Page Generation) Fix critical password validation bypass and add unit tests This commit addresses a critical security vulnerability where the password minimum length check was performed on the HASHED password (always 60 characters for bcrypt) instead of the actual password before hashing. Vulnerability Details: - Original code: strlen($employee_data['password']) >= 8 - This compared the hash length (always 60) instead of raw password - Impact: Users could set 1-character passwords like "a" - Severity: Critical (enables brute force attacks on weak passwords) - CVE-like issue: CWE-307 (Improper Restriction of Excessive Authentication Attempts) Fix Applied: - Validate password length BEFORE hashing - Clear error message when password is too short - Added unit tests to verify minimum length enforcement - Regression test to prevent future vulnerability re-introduction Test Coverage: - testPasswordMinLength_Rejects7Characters: Verify 7 chars rejected - testPasswordMinLength_Accepts8Characters: Verify 8 chars accepted - testPasswordMinLength_RejectsEmptyString: Verify empty rejected - testPasswordMinLength_RejectsWhitespaceOnly: Verify whitespace rejected - testPasswordMinLength_AcceptsSpecialCharacters: Verify special chars OK - testPasswordMinLength_RejectsPreviousBehavior: Regression test for bug Files Modified: - app/Controllers/Home.php: Fixed password validation logic - tests/Controllers/HomeTest.php: Added comprehensive unit tests Security Impact: - Enforces 8-character minimum password policy - Prevents extremely weak passwords that facilitate brute-force attacks - Critical for credential security and user account protection Breaking Changes: - Users with passwords < 8 characters will need to reset their password - This is the intended security improvement Severity: Critical CVSS Score: ~7.5 CWE: CWE-305 (Authentication Bypass by Primary Weakness), CWE-307 Add GitHub Actions workflow to run PHPUnit tests Move business logic from views to controllers for better separation of concerns - Move logo URL computation from info_config view to Config::getIndex() - Move image base64 encoding from receipt_email view to Sales controller - Improves separation of concerns by keeping business logic in controllers - Simplifies view templates to only handle presentation Fix XSS vulnerabilities in report views - escape user-controllable summary data and labels Fix base64 encoding URL issue in delete payment - properly URL encode base64 string Fix remaining return type declarations for Sales controller Fixed additional methods that call _reload(): - postAdd() - returns _reload($data) - postAddPayment() - returns _reload($data) - postEditItem() - returns _reload($data) - postSuspend() - returns _reload($data) - postSetPaymentType() - returns _reload() All methods now return ResponseInterface|string to match _reload() signature. This resolves PHP TypeError errors.
231 lines
7.5 KiB
PHP
231 lines
7.5 KiB
PHP
<?php
|
|
|
|
namespace Tests\Controllers;
|
|
|
|
use CodeIgniter\Test\CIUnitTestCase;
|
|
use CodeIgniter\Test\DatabaseTestTrait;
|
|
use CodeIgniter\Test\FeatureTestTrait;
|
|
use CodeIgniter\Config\Services;
|
|
use App\Models\Employee;
|
|
|
|
/**
|
|
* 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
|
|
*/
|
|
class HomeTest extends CIUnitTestCase
|
|
{
|
|
use DatabaseTestTrait;
|
|
use FeatureTestTrait;
|
|
|
|
protected $migrate = true;
|
|
protected $migrateOnce = true;
|
|
protected $refresh = false;
|
|
protected $namespace = null;
|
|
|
|
/**
|
|
* Set up test environment
|
|
*/
|
|
protected function setUp(): void
|
|
{
|
|
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,
|
|
'username' => 'admin',
|
|
'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),
|
|
'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,
|
|
'username' => 'admin',
|
|
'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),
|
|
'Password with 8 chars should be accepted');
|
|
|
|
// Restore original password
|
|
$employee->change_password([
|
|
'username' => 'admin',
|
|
'password' => password_hash('pointofsale', PASSWORD_DEFAULT),
|
|
'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,
|
|
'username' => 'admin',
|
|
'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
|
|
*
|
|
* @return void
|
|
*/
|
|
public function testPasswordMinLength_RejectsWhitespaceOnly(): 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']);
|
|
}
|
|
|
|
/**
|
|
* 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',
|
|
'password' => password_hash('pointofsale', PASSWORD_DEFAULT),
|
|
'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,
|
|
'username' => 'admin',
|
|
'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),
|
|
'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
|
|
}
|
|
} |