- Add csv_import_invalid_location to Items.php for CSV import validation
- Add error_deleting_admin and error_updating_admin to Employees.php for admin protection messages
Strings added with empty values so they fallback to English and show as untranslated in Weblate.
The bulk edit function iterated over all $_POST keys without a whitelist,
allowing authenticated users to inject arbitrary database columns (e.g.,
cost_price, deleted, item_type) into the update query. This bypassed
CodeIgniter 4's $allowedFields protection since Query Builder was used
directly.
Fix: Add ALLOWED_BULK_EDIT_FIELDS constant to Item model defining the
explicit whitelist of fields that can be bulk-updated. Use this constant
in the controller instead of iterating over $_POST directly.
Fields allowed: name, category, supplier_id, cost_price, unit_price,
reorder_level, description, allow_alt_description, is_serialized
Security impact: High (CVSS 8.1) - Could allow price manipulation and
data integrity violations.
The previous SQL injection fix (GHSA-hmjv-wm3j-pfhw) used named parameter
syntax :search: with having(), but CodeIgniter 4's having() method does
not support named parameters. This caused the query to fail.
The fix uses havingLike() which properly:
- Escapes the search value to prevent SQL injection
- Handles the LIKE clause construction internally (wraps value with %)
- Works correctly with HAVING clauses for aggregated columns
This maintains the security fix while actually working on CI4.
When localization uses dot (.) as thousands separator (e.g., it_IT, es_ES, pt_PT),
the payment_amount value was displayed as raw float (e.g., '10.50') but parsed
using parse_decimals() which expects locale-formatted numbers.
In these locales, '.' is thousands separator and ',' is decimal separator.
parse_decimals('10.50') would return false, causing the condition
!= 0 to evaluate incorrectly (false == 0 in PHP),
resulting in the payment being deleted instead of updated.
Fix: Use to_currency_no_money() to format payment_amount and cash_refund
values according to locale before displaying in the form, so parse_decimals()
can correctly parse them on submission.
Modified definition_values() function in app/Views/attributes/item.php to properly handle checkbox attributes.
The issue was that checkbox attributes have two input elements (hidden and checkbox) with the same name pattern. When collecting attribute values during the refresh operation, both inputs were being processed, with the hidden input potentially overwriting the checkbox state.
Changes:
- Skip hidden inputs that have a corresponding checkbox input
- For checkbox inputs, explicitly capture the checked state using prop('checked')
- Convert checked state to '1' or '0' for consistency
This ensures that when adding another attribute to an item, existing checkbox states are preserved correctly.
1. Stock Location XSS (GHSA-7hg5-68rx-xpmg):
- Stock location names were rendered unescaped in employee form
- Malicious stock locations could contain XSS payloads that execute
when viewing employee permissions
- Fixed by adding esc() to permission display in employees/form.php
2. Customer Name XSS (GHSA-hcfr-9hfv-mcwp):
- Bootstrap-table columns had escape disabled for customer_name,
email, phone_number, and note fields
- Malicious customer names could execute XSS in Daily Sales view
- Fixed by removing user-controlled fields from escape exception list
- Only 'edit', 'messages', and 'item_pic' remain in exception list
(these contain safe server-generated HTML)
Both vulnerabilities allow authenticated attackers with basic permissions
to inject JavaScript that executes in admin/other user sessions.
GHSA-q58g-gg7v-f9rf: Stored XSS via Item Description
Security Impact:
- Authenticated users with item management permission can inject XSS payloads
- Payloads execute in POS register view (sales and receivings)
- Can steal session cookies, perform CSRF attacks, or compromise POS operations
Root Cause:
1. Input: Items.php:614 accepts description without sanitization
2. Output: register.php:255 and receiving.php:220 echo description without escaping
Fix Applied:
- Input sanitization: Added FILTER_SANITIZE_FULL_SPECIAL_CHARS to description POST
- Output escaping: Added esc() wrapper when echoing item descriptions
- Defense-in-depth approach: sanitize on input, escape on output
Files Changed:
- app/Controllers/Items.php - Sanitize description on save
- app/Views/sales/register.php - Escape description on display
- app/Views/receivings/receiving.php - Escape description on display
Testing:
- XSS payloads like '<script>alert(1)</script>' are now sanitized on input
- Any existing malicious descriptions are escaped on output
- Does not break legitimate descriptions with special characters
Parameterize LIKE queries in HAVING clause to prevent SQL injection
when search_custom filter is enabled. Also sanitize search parameter
input at controller level for defense-in-depth.
Fixes vulnerability where user input was directly interpolated into
SQL queries without sanitization.
- Move admin modules list from is_admin method to ADMIN_MODULES constant
- Rename is_admin() to isAdmin() following CodeIgniter naming conventions
- Rename can_modify_employee() to canModifyEmployee() following conventions
- Update all callers in Employees controller and tests
This migration will:
- Scan all items for filenames containing spaces
- Rename both original and thumbnail files on the filesystem
- Update database records with sanitized filenames
- Only process files that actually exist on the filesystem
When uploading item images with filenames containing spaces, the thumbnails fail to load due to Apache mod_rewrite rejecting URLs with spaces.
Changes:
- Modified upload_image() method to sanitize filenames by replacing spaces and special characters with underscores
- Uses regex to keep only alphanumeric, underscores, hyphens, and periods
- Preserves original filename in 'orig_name' field for reference
- Fixes issue where thumbnail URLs would fail with 'AH10411: Rewritten query string contains control characters or spaces'
Example: 'banana marsmellow.jpg' becomes 'banana_marsmellow.jpg'
Fixes: #4372
* 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 <jeroen@steganos.dev>
* Fix second-order SQL injection in currency_symbol config
The currency_symbol value was concatenated directly into SQL queries
without proper escaping, allowing SQL injection attacks via the
Summary Discounts report.
Changes:
- Use $this->db->escape() in Summary_discounts::getData() to properly
escape the currency symbol value before concatenation
- Add htmlspecialchars() validation in Config::postSaveLocale() to
sanitize the input at storage time
- Add unit tests to verify escaping of malicious inputs
Fixes SQL injection vulnerability described in bug report where
attackers with config permissions could inject arbitrary SQL through
the currency_symbol field.
* Update test to use CIUnitTestCase for consistency
Per code review feedback, updated test to extend CIUnitTestCase
instead of PHPUnit TestCase to maintain consistency with other
tests in the codebase.
---------
Co-authored-by: Ollama <ollama@steganos.dev>
- Add whitelist validation for invoice_type to prevent path traversal and LFI
- Validate invoice_type against allowed values in Sale_lib
- Sanitize invoice_type input in Config controller before saving
- Default to 'invoice' template for invalid types
Security: Prevents arbitrary file inclusion via user-controlled invoice_type config
- Add validateCSVStockLocations() method to check CSV columns against allowed locations
- Log error when invalid stock location columns are detected
- Tests for valid, invalid, and mixed stock location columns
- Tests for location name case sensitivity
- Tests for CSV parsing and detecting location columns
- Add error message language string for invalid locations
Co-authored-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
The round_number() method signature declares $amount as string, but the
HALF_FIVE case and other rounding operations pass string values to round()
and other arithmetic operations which expect numeric types. This causes
type errors when strict type checking is enabled.
Fix by casting $amount to float before arithmetic operations in both
migration files:
- 20170502221506_sales_tax_data.php (line 268)
- 20200202000000_taxamount.php (line 244)
Also cast sale_tax_amount to float in round_sales_taxes() method before
passing to round() operations (lines 381 in sales_tax_data.php and 358 in
taxamount.php).
Fixes#4324
- Non-admin employees can no longer view/modify admin accounts
- Non-admin employees can no longer delete admin accounts
- Non-admin employees can only grant permissions they themselves have
- Added is_admin() and can_modify_employee() methods to Employee model
- Prevents privilege escalation via permission grants
Add tests for BOLA fix and permission delegation
- EmployeeTest: Unit tests for is_admin() and can_modify_employee() methods
- EmployeesControllerTest: Test cases for authorization checks (integration tests require DB)
- ReportsControllerTest: Test validating the constructor redirect fix pattern
Fix return type error in Employees controller
Use $this->response->setJSON() instead of echo json_encode() + return
to properly satisfy the ResponseInterface return type.
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.
The redirect() in constructor returned a RedirectResponse that was never executed, allowing unauthorized access to report submodules. Replaced with header() + exit() to enforce permission checks.
* Add show/hide cost price & profit feature
* .env should be ignored.
* js code formatted. .vscode folder ignore for vscode user settings.json
* style is replaced with bootstrap class, formatted and .env.example
* toggle button on table to like in other
* comment corrected.
* class re-factored
* minor refactor
* formatted with 4 space
---------
Co-authored-by: Lotussoft Youngtech <lotussoftyoungtech@gmail.com>
* Add attachment cid when sending emails (#4308)
Also check if an encryption key is set before decrypting the SMTP
password.
* Upgrade to CI 4.6.3 (#4308)
* Fix for changing invoice id in email (#4308)
* `execute_script()` now returns a boolean for error handling.
* Added transaction to `Migration_MissingConfigKeys.up()`.
* Added logging to various migrations.
* Added transaction to `Migration_MissingConfigKeys.up()`.
* Added logging to various migrations.
* Formatting and function call fixes
Fixed a minor formatting issue in the migration helper.
Replaced a few remaining error_log() calls.
Updated executeScriptWithTransaction() to use log_message()
* Function call fix
Replaced the last error_log() calls with log_message().
---------
Co-authored-by: Joe Williams <hey-there-joe@outlook.com>
* `execute_script()` now returns a boolean for error handling.
* Added transaction to `Migration_MissingConfigKeys.up()`.
* Added `executeScriptWithTransaction()` to migration helpers.
* Many changes for testing; also minor formatting fixes.
* Removed test code and pointed the `NullableTaxCategoryId` migration at the right SQL file.
* Fixed header.php
* Code cleanup from code review:
- Added IGNORE to SQL scripts.
- Added try-catch to executeScriptWithTransaction().
- Various comment changes.
* Fixed naming issue
Nullable tax category ID migration now runs the correct script.
* Updated SQL
Replaced INSERT WHERE NOT EXISTS in missing config keys sql script to use a single INSERT IGNORE.
* Updated migration helper
Updated executeScriptWithTransaction to use transRollback
---------
Co-authored-by: Joe Williams <hey-there-joe@outlook.com>
