Users without the 'employees' permission can no longer impersonate other
employees when creating or editing expenses and receivings. The employee
field is now restricted to the current user for new records and shows the
stored employee for existing records.
Changes:
- Expenses controller: Add permission check in getView() and postSave()
- Receivings controller: Add permission check in getEdit() and postSave()
- Form views: Conditionally display dropdown or read-only field
Fixes#3616
Add 'only_debit' filter to Daily Sales and Takings dropdown. Reuses
existing 'Sales.debit' language string for the filter label. Includes
filter default initialization in getSearch() to prevent PHP warnings.
Fixes#4439
* Fix stored XSS vulnerability in Attribute Definitions
GHSA-rvfg-ww4r-rwqf: Stored XSS via Attribute Definition Name
Security Impact:
- Authenticated users with attribute management permission can inject XSS payloads
- Payloads execute when viewing/editing attributes in admin panel
- Can steal session cookies, perform CSRF attacks, or compromise admin operations
Root Cause:
1. Input: Attributes.php postSaveDefinition() accepts definition_name without sanitization
2. Output: Views echo definition_name without proper escaping
Fix Applied:
- Input sanitization: Added FILTER_SANITIZE_FULL_SPECIAL_CHARS to definition_name and definition_unit
- Output escaping: Added esc() wrapper when displaying definition_name in views
- Defense-in-depth: htmlspecialchars on attribute values saved to database
Files Changed:
- app/Controllers/Attributes.php - Sanitize inputs on save
- app/Views/attributes/form.php - Escape output on display
- app/Views/attributes/item.php - Escape output on display
* Remove input sanitization, keep output escaping only
Use escaping on output (esc() in views) as the sole XSS prevention
measure instead of sanitizing on input. This preserves the original
data in the database while still protecting against XSS attacks.
* Add validation for definition_fk foreign key in attribute definitions
Validate definition_group input before saving:
- Must be a positive integer (> 0)
- Must exist in attribute_definitions table
- Must be of type GROUP to ensure data integrity
Also add translation for definition_invalid_group error message
in all 45 language files (English placeholder for translations).
* Refactor definition_fk validation into single conditional statement
* Add esc() to attribute value outputs for XSS protection
- Add esc() to TEXT input value in item.php
- Add esc() to definition_unit in form.php
These fields display user-provided content and need output escaping
to prevent stored XSS attacks.
* Refactor definition_group validation into separate method
Extract validation logic for definition_fk into validateDefinitionGroup()
private method to improve code readability and reduce method complexity.
Returns:
- null if input is empty (no group selected)
- false if validation fails (invalid group)
- integer ID if valid
* Add translations for definition_invalid_group in all languages
- Added proper translations for 28 languages (de, es, fr, it, nl, pl, pt-BR, ru, tr, uk, th, zh-Hans, zh-Hant, ro, sv, vi, id, el, he, fa, hu, da, sw-KE, sw-TZ, ar-LB, ar-EG)
- Set empty string for 14 languages to fallback to English (cs, hr-HR, bg, bs, ckb, hy, km, lo, ml, nb, ta, tl, ur, az)
---------
Co-authored-by: Ollama <ollama@steganos.dev>
* Fix DECIMAL attribute not respecting locale format
Issue: DECIMAL attribute values were displayed as raw database values
instead of being formatted according to the user's locale settings.
Fix:
1. Modified Attribute::get_definitions_by_flags() to optionally return
definition types along with names (new $include_types parameter)
2. Updated expand_attribute_values() in tabular_helper.php to detect
DECIMAL attributes and apply to_decimals() locale formatting
3. Updated callers (Reports, Items table) to pass include_types=true
where attributes are displayed
The DECIMAL values in table views (items, sales reports, receiving reports)
now respect the configured locale number format, matching DATE attributes
which already use locale-based formatting.
* Apply PSR-12 camelCase naming to new variables
Response to PR review comments:
- Rename to
- Rename to
- Rename to
---------
Co-authored-by: Ollama <ollama@steganos.dev>
* Fix SQL injection in suggestions column configuration
The suggestions_first_column, suggestions_second_column, and
suggestions_third_column configuration values were concatenated
directly into SQL SELECT statements without validation, allowing
SQL injection attacks through the item search suggestions.
Changes:
- Add whitelist validation in Config controller to only allow
valid column names (name, item_number, description, cost_price,
unit_price)
- Add defensive validation in Item model's get_search_suggestion_format()
and get_search_suggestion_label() methods
- Default invalid values to 'name' column for safety
- Add unit tests to verify malicious inputs are rejected
This is a critical security fix as attackers with config permissions
could inject arbitrary SQL through these configuration fields.
Vulnerability reported as additional injection point in bug report.
* Refactor: Move allowed suggestions columns to Item model constants
Extract the list of valid suggestion columns into two constants in the Item model for better cohesion:
- ALLOWED_SUGGESTIONS_COLUMNS: valid column names
- ALLOWED_SUGGESTIONS_COLUMNS_WITH_EMPTY: includes empty string for config validation
This consolidates the validation logic in one place and makes it reusable across Config controller and Item model.
* Address PR review comments: improve validation and code quality
Changes:
- Use camelCase naming for validateSuggestionsColumn() method (PSR-12)
- Add field-aware validation with different fallbacks for first vs other columns
- Handle non-string POST input by checking is_string() before validation
- Refactor duplicate validation logic into suggestionColumnIsAllowed() helper
- Use consistent camelCase variable names ($suggestionsFirstColumn)
- Update tests to validate constants and behavior rather than implementation
- Tests now focus on security properties of the allowlist itself
The validation now properly handles:
- First column: defaults to 'name' when invalid
- Second/Third columns: defaults to '' (empty) when invalid
- Non-string inputs: treated as invalid with appropriate fallback
---------
Co-authored-by: Ollama <ollama@steganos.dev>
The redirect() in getManage() returned a RedirectResponse that was never
executed, allowing unauthorized access to reports_sales. Updated method
signature to return ResponseInterface|string and properly return the
redirect response.
Refs: GHSA-94jm-c32g-48r5
Co-authored-by: Ollama <ollama@steganos.dev>
* 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 <ollama@steganos.dev>
This commit adds URL-based filter persistence for table views, allowing
users to navigate away from a filtered view (e.g., clicking into sale
details) and return without losing their filter settings.
The solution uses history.replaceState() to update the URL without
triggering a page reload, providing a seamless user experience while
maintaining shareable/bookmarkable URLs.
Fixes navigation issue where filters are lost when viewing details or
navigating away from table views.
* Move filter restoration to server-side for cleaner architecture
Changes:
- Controllers now restore filters from URL query string on initial page load:
* Sales.php: Reads start_date, end_date, and filters[] from GET
* Items.php: Reads start_date, end_date, filters[], and stock_location
* Expenses.php: Reads start_date, end_date, and filters[]
* Cashups.php: Reads start_date, end_date, and filters[]
- Views now receive restored filter values from controllers:
* Server-side date override via JavaScript variables
* form_multiselect() receives $selected_filters from controller
* Removed setTimeout hack from table_filter_persistence.php
- Simplified table_filter_persistence.php:
* Now only handles URL updates on filter changes
* No longer responsible for restoring state
* Cleaner, single responsibility (client-side URL management)
Benefits:
- Works without JavaScript for initial render
- Cleaner architecture (server controls initial state)
- Client-side JS only handles "live" filter updates
- Filters persist across navigation via URL query string
- Shareable/bookmarkable URLs
How it works:
1. User visits /sales/manage?start_date=2024-01-01&filters[]=only_cash
2. Controller reads GET params and passes to view
3. View renders with correct initial filter values
4. User changes filter → JavaScript updates URL via replaceState()
5. User navigates away and back → Controller restores from URL again
* Refactor filter restoration into helper function and use PSR-12 naming
* Use array_merge with helper to reduce code duplication
---------
Co-authored-by: Ollama <ollama@steganos.dev>
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.
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
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>
- 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.
- Refactored function names for PSR-12 compliance
- Programmatically cascade delete attribute_link rows when a drop-down attribute is deleted but leave attribute_link rows associated with transactions.
- Added `WHERE item_id IS NOT NULL` to migration to prevent failure on MySQL databases during migration
- Retroactive correction of migration to prevent MySQL databases from failing.
- Refactored generic functions to helper
- Reverted attribute_links foreign key to ON DELETE RESTRICT which is required for a unique constraint on this table. Cascading deletes are now handled programmatically.
- Migration Session table to match Code Igniter 4.6
- Add index to attribute_links to prevent query timeout in items view on large databases
- Added overridePrefix() function to the migration_helper. Any time QueryBuilder is adding a prefix to the query when we don't want it to, this query can be used to override the prefix then set it back after you're done.
- Added dropAllForeignKeyConstraints() helper function.
- Added deleteIndex() helper function.
- Added indexExists() helper function.
- Added primaryKeyExists() helper function.
- Added recreateForeignKeyConstraints() helper function.
- Added CRUD section headings to the Attribute model.
- Replaced `==` with `===` to prevent type juggling.
- Removed unused delete_value function.
- Reworked deleteDefinition() and deleteDefinitionList() functions to delete rows from the attribute_links table which are associated.
- Added deleteAttributeLinksByDefinitionId() function
Implement Cascading Delete
- Function to delete attribute links with one or more attribute definitions.
- Call function to implement an effective cascading delete.
- Refactor function naming to meet PSR-12 conventions
Fix Migration
- Add drop of Generated Column to prevent failure of migration on MySQL databases.
Fix Migration
- Removed blank lines
- Refactored function naming for PSR compliance
- Reformatted code for PSR compliance
- Added logic to drop dependent foreign key constraints before deleting an index then recreating them.
Migrate ospos_sessions table
- DROP and CREATE session table to prevent migration problems on populated databases
Fixed Bug in Migration
- In the event that item_id = null (e.g., it's a dropdown) it should not be included in the results.
Fixed bug in Dropdown deletes
- Removed delete_value function in Attributes Controller as it is unused.
- Renamed postDelete_attribute_value function for PSR-12 compliance.
- Renamed delete_value Attribute model function for PSR-12 compliance.
- Refactored out function to getAttributeIdByValue
- Replaced == with === to prevent type juggling
- Reorganized parts of model to make it easier to find CRUD functions.
Refactoring
- PSR-12 Compliance formatting changes
- Refactored several generic functions into the migration_helper.php
- First check if primary key exists before attempting to create it.
- Grouped functions together in migration_helper.php
- phpdoc commenting functions
Optimizing Indices
- There are two queries run while opening the Items view which time out on large databases with weak hardware. These indices cut the query execution in half or better.
Add Unique constraint back into attribute_links
- This migration reverts ospos_attribute_links_ibfk_1 and 2 to ON DELETE RESTRICT. Cascade delete is done programmatically. This is needed to have a unique column on the attribute_links table which prevents duplicate attributes from begin created with the same item_id-attribute_id-definition_id combination
Correct spacing after if for PSR-12
Minor code cleanup.
- Removed Comments separating sections of code in Attribute model
- Removed extra log line to prevent cluttering of the log
* Improve code style and PSR-12 compliance
- refactored code formatting to adhere to PSR-12 guidelines
- standardized coding conventions across the codebase
- added missing framework files and reverted markup changes
- reformatted arrays for enhanced readability
- updated language files for consistent styling and clarity
- minor miscellaneous improvements
* Replace tabs with spaces
Signed-off-by: objecttothis <objecttothis@gmail.com>
* Composer package bumps
- Bump codeigniter4/framework to 4.6.0
- Bump codeIgniter/coding-standard to ^1.8
- Bump codeigniter4/devkit to ^1.3
- Updated framework files required by CI4.6.0
- Removed Deprecated variables
- Added new file in the repo from framework
Signed-off-by: objecttothis <objecttothis@gmail.com>
* Reflect PHP 8.4 support
Updates for PHP 8.4 support introduced with the upgrade to CodeIgniter 4.6.x
* Update INSTALL.md
- Revert PHP 8.4 support for now.
- Removed extra space before comma
---------
Signed-off-by: objecttothis <objecttothis@gmail.com>
Co-authored-by: BudsieBuds <bas_hubers@hotmail.com>
Random fixes in time for the 3.4.0 release.
- corrects typo in the items controller
- small update to login view
- removes deprecated code from header view
- ospos license updated to end 2024
- moved gulp packages to dev dependencies
- updated gulp-zip and npm-check-updates to latest version
- updated readme for consistency
- makes ospos license in config fully readable
- fixes composer libraries license view in config
- gulp now updates composer libraries license and ospos license
- updated other license views in config
