* fix(security): handle special characters in `.env` key values and improve insertion logic
- Escape backslashes and dollar signs in `applyEnvKeyReplacement` to prevent unintended value corruption.
- Ensure new keys are inserted after `encryption.key` for better organization and manageability.
- Add explicit cast to int to prevent wrong concatenation operator warning.
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* fix(security): handle null return in `applyEnvKeyReplacement` and ensure proper `.env` updates
- Update `applyEnvKeyReplacement` to return `null` on failure, improving error handling.
- Adjust calls to `atomicWriteFile` with updated content to prevent unintended behavior.
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* fix(security): improve error logging and exception messages in file locking
- Add detailed logging for file open and locking errors in `security_helper`.
- Remove unused `helper` and `checkThrottleEncryption` calls from `Events` for cleanup.
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* fix(security): improve atomic file write and handle encryption key placement
- Throw `RandomException` for better error reporting in `atomicWriteFile`.
- Simplify Windows-specific `rename()` fallback logic.
- Fix `encryption.key` assignment order to ensure consistency in `.env` updates.
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* fix(security): improve `.env` file handling and add unit tests for helper functions
- Suppress warnings in `file_get_contents` to prevent unnecessary error logs.
- Update `applyEnvKeyReplacement` to use `preg_replace_callback` for better safety.
- Add comprehensive unit tests for `security_helper` functions to ensure `.env` updates and key management work as expected.
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* fix(security): enhance `.env` update logic and add robust exception handling
- Add `RandomException` to improve error reporting in encryption key management.
- Introduce environment file locking for safer `.env` updates.
- Ensure `applyEnvKeyReplacement` properly handles and inserts old key comments.
- Replace direct file writes with `atomicWriteFile` for consistency.
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* fix(security): refactor `.env` file initialization and encryption key handling
- Introduce `initializeEnvFile` for reusable `.env` setup logic.
- Add `backupEnvFile` and `writeNewEncryptionKey` for robust key management with backups.
- Simplify and clean up redundant `.env` handling code paths.
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* fix(security): clarify `checkEncryption` docblock return value description
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* fix(security): escape backslashes and dollar signs in `applyEnvKeyReplacement`
- Ensure `applyEnvKeyReplacement` properly escapes special characters when inserting or appending `.env` keys.
- Add new unit tests to validate correct handling of backslashes and dollar signs.
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* fix(i18n): add localized error messages and improve error reporting in `security_helper`
- Add missing translations for error messages across multiple language files.
- Update `security_helper` to use localized exception messages with placeholders.
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* Redesign encryption/throttle key provisioning as read-only runtime
- checkEncryption()/checkThrottleEncryption() are now read-only guards that
throw when no valid key is provisioned, instead of writing .env at
request time.
- Add rotateEncryptionKey() and provisionThrottleKey() for explicit,
idempotent provisioning.
- Add php spark env:provision (app/Commands/EnvProvision.php) so Docker can
provision keys once at container startup before any request.
- Add app/Libraries/CI3SecretConverter.php shared CI3->CI4 secret converter
(AES-128-CBC decrypt + CI4 re-encrypt/verify/save) used by both the
interactive migration and the docker startup path.
- Refactor convertToCI4 migration to use the shared converter.
- Persist .env in a named volume and run spark env:provision on boot; stop
baking .env into the shipped image.
- Add guard/rotation/throttle + converter tests; clean up orphaned
msg_pwd_required language keys across all locales.
* fix: save CI4 ciphertext in env:provision and bind-mount a .env file
Addresses CodeRabbit review on PR #4656:
- env:provision CI3 branch was persisting *plaintext* secrets (saveAll($plain))
instead of the CI4 ciphertext, unlike the ConvertToCI4 migration. Now
encrypts with encryptAll(), verifies the round trip, and saves the ciphertext.
- The ospos_env named volume mounted at /app/.env made .env a directory, so
atomicWriteFile's rename() failed and spark env:provision could not start apache.
Switch to a bind mount of a host file (./.env) which persists and stays a file.
- Add a regression test asserting the command persists ciphertext (not plaintext).
* chore: trim redundant docblocks in EnvProvision and provision throttle.key in CI
Follow up on @objecttothis review comments:
- app/Commands/EnvProvision.php: remove the boilerplate docblocks the
property names already convey (group/name/usage/description, run()),
the two inline step comments, the anyNonEmpty() param docblock, and the
legacySecretsPresent() docblock. Keeps the class-level docblock since it
is the only place that states the read-only runtime design + the
never-persist-plaintext invariant.
- .github/workflows/phpunit.yml: provision a per-run throttle.key the same
way the encryption key is already provisioned. The PR makes
checkThrottleEncryption() a read-only guard that throws when
env('throttle.key') is unset; CI only started exporting ENCRYPTION_KEY,
so every test that goes through the Throttle filter (7 ThrottleTest
cases + 4 LoginTest cases) failed with
"No throttle key is provisioned. Run `php spark env:provision`".
Writing `throttle.key=<KEY>` into .env matches what
`php spark env:provision` does on a real container start.
* fix(ci): write throttle.key into .env instead of exporting an OS env var
The previous attempt exported throttle.key via GITHUB_ENV, but CodeIgniter's
env() helper resolves in the order $_ENV[$key] ?? $_SERVER[$key] ?? getenv($key),
and DotEnv populates $_ENV['throttle.key'] from the .env file first. Because the
.env (copied from .env.example) ships with the empty placeholder throttle.key='',
that $_ENV entry exists as '' and short-circuits the ?? chain before getenv()
is reached — so the OS env var was never consulted and every Throttle/Login test
still threw 'No throttle key is provisioned'.
Write the per-run key into the .env file itself (sed-replacing the empty
placeholder), which is exactly what `php spark env:provision` does in
production and is the single source env() actually reads from.
Verify the replacement happened (grep -Eq '^throttle\.key=.') so a future change
to the placeholder format fails the run loudly instead of silently breaking
the 11 throttle-dependent tests.
* fix(security): restore CI3->CI4 auto-provisioning gated by .env writability
checkEncryption()/checkThrottleEncryption() again provision the keys
inline when .env is writable (empty key -> generate; short key -> decrypt,
rotate, re-encrypt, verify, persist legacy CI3 secrets). When .env is not
writable they assume the key was provisioned externally (e.g. docker
env:provision) and throw. Update helper tests to match and correct the
EnvProvision docblock that claimed the runtime was strictly read-only.
* test(security): make short-key conversion branch injectable and test it
checkEncryption() now accepts an optional CI3SecretConverter so the
CI3->CI4 conversion branch can be exercised in unit tests without a
database. Adds testCheckEncryptionConvertsCi3ShortKeyWhenEnvWritable
which seeds CI3-era ciphertexts via a fake Appconfig model and asserts
the key is rotated and the payload verifies back to the original
plaintext.
* fix(security): abort on backup/read/saveAll failure to avoid data loss
Three related data-integrity fixes:
- backupEnvFile() now returns true/false based on whether the backup
actually exists and is readable. rotateEncryptionKey() aborts before
destroying the key when the backup could not be written to disk.
- rotateEncryptionKey() and provisionThrottleKey() throw
RuntimeException(Error.unable_to_read_env_file) when the .env read
fails, instead of silently replacing the whole file with an empty
string. This prevents a permission error from wiping all keys.
- checkEncryption() and EnvProvision::run() now both roll back to the
backup with abortEncryptionConversion() when the post-rotation
saveAll() throws, matching the migration path (which already did this).
A failing fake Appconfig is used to exercise this in the new
testCheckEncryptionRollsBackWhenSaveAllFails test.
* fix(ci): skip comment job in deploy-pr.yml when prepare was not run
The comment job had if: always(), so it ran even when the prepare job
was skipped (e.g. review was not approved). With PR_NUMBER empty the gh
api call posted to issues//comments, received a 404, and the entire run
showed up as failure. Guard the job with
needs.prepare.result == 'success' so it only runs when PR_NUMBER is valid.
* address coderabbit open items: placeholder guards, message neutrality, ar-EG alignment
- backupEnvFile(): fail when mkdir() or either chmod() fails, so the
pre-rotation backup is actually persisted before the key is replaced
- email/message config views: only show the 'already set' placeholder when
the secret is actually present (prevented false positives on fresh installs)
- Error.unable_to_create_env_file / .unable_to_read_env_file (en + en-GB):
use key-neutral wording since both keys are provisioned with the same keys
- ar-EG/Error.php: align all => arrows on the longest key
Item 7 (filesystem test isolation) is a larger refactor — the tests are
serial on CI and tearDown() restores state per test. Left for follow-up.
* test(security): isolate helper FS tests via Config\SecurityEnv
Introduce Config\SecurityEnv holding envPath/backupPath/lockPath so the
security helper reads its target paths from shared configuration instead of
hardcoded ROOTPATH/WRITEPATH literals. security_helperTest.php now redirects
all three to a unique per-run sandbox under sys_get_temp_dir() and tears it
down in tearDown(), so the suite no longer reads/writes the repository's real
.env and is safe to run in parallel.
No helper signature changes; production callers unaffected.
Addresses CodeRabbit item 7 (issue #4700).
Co-Authored-By: opencode <bot@opencode.ai>
* fix(security): run key-conversion as one locked transaction
Address CodeRabbit Major findings from the 4th re-review of the env
helper and its callers:
1. Hold .env.lock for the entire CI3 -> CI4 conversion transaction
(backup -> rotate -> re-encrypt -> verify -> persist -> cleanup) so a
concurrent worker cannot interleave a key write between the rotation
and the ciphertext save. Split rotateEncryptionKey into a lock-free
core (rotateEncryptionKeyUnlock) plus the existing lock wrapper and a
new rotateEncryptionKeyTransaction that owns the lock across the full
unit and performs both the in-lock rollback (abortEncryptionConversion)
and the in-lock backup removal on success.
2. Treat the legacy value '0' as non-empty data so key rotation still
persists the re-encrypted ciphertext when '0' is the only stored
secret (array_filter would have dropped it and skipped saveAll).
3. Wrap the post-rotation re-encrypt/verify/saveAll sequence in a
catch (Throwable) across all three call-sites so CI4
EncryptionException, ReflectionException from batch_save, a failed
round-trip verify, and any other failure all roll the .env key back
to the pre-rotation state.
4. In Docker Compose, use long-syntax bind with create_host_path: false
and document in INSTALL.md that the host .env must be a regular file
(a missing one is no longer auto-created as a directory, and the
mount now rejects a missing source on Compose implementations that
support the flag).
Files touched: app/Helpers/security_helper.php, app/Commands/EnvProvision.php,
app/Database/Migrations/20220127000000_convertToCI4.php, docker-compose.yml,
INSTALL.md. All 4 existing helper tests still pass via CI.
* fix(security): make abortEncryptionConversion fail loudly on restore failure
The rollback path restored the .env backup with a suppressed
file_put_contents() and an unchecked file_get_contents(). If the restore
failed after the key had already been rotated, .env was left holding the new
CI4 key while the DB still held CI3-era ciphertext, so the data became
undecryptable after the next restart.
Now the backup read is checked for false and the restore goes through the
existing atomicWriteFile() helper; either failure throws so the error is
surfaced instead of silently corrupting the config. Adds a regression test
that forces an unreadable backup and asserts the throw plus that .env is
left untouched.
* fix(security): guard abortEncryptionConversion backup read before touching it
Validate the backup is a regular readable file (is_file/is_readable) before
reading it, so a missing/malformed backup fails loudly instead of emitting a
file_get_contents() warning. The unreadable-backup regression test now
exercises this guard rather than relying on a promoted warning.
---------
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
Co-authored-by: jekkos <jeroen.peelaerts@gmail.com>
Co-authored-by: jekkos <jekkos@users.noreply.github.com>
Co-authored-by: opencode <bot@opencode.ai>
fix(validation): allow Windows sendmail paths, tighten shell metachar exclusions
Broaden PLAIN_FILESYSTEM_PATH_STRICT to accept real-world sendmail formats
while blocking command injection characters not needed in valid paths.
- OSPOSRules.php: allow space, colon, backslash for Windows paths
(e.g. C:\wamp64\...) and trailing args (-t -i); still excludes
ampersand, backtick, subshell, redirect, and cmd.exe metacharacters
- OSPOSRulesTest.php: add cases for Windows paths, trailing args, and
injection payloads
- Remove 7 ConfigTest assertions that expected metacharacter rejection;
add acceptance test for sendmail path with trailing args
i18n(lang): expand mailpath_invalid message across all locales
- Fill previously empty mailpath_invalid keys across all locales
- Update existing translations (de-CH, de-DE, es-ES, es-MX, fr, nl-BE,
nl-NL) to reflect newly allowed characters; nl locales corrected from
English loanwords to proper Dutch terms
- Add missing key to ckb/Config.php
docs: remove security advisory IDs from public-facing files
- AGENTS.md: extend no-advisory-ID rule to documentation and URLs
- INSTALL.md: drop GHSA reference and advisory link from Host Header
Injection guidance; rationale and fix instructions remain intact
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* Validate gift-card payment amounts (GHSA-9847)
Close the negative gift-card amount minting vector: when a forged
payment_type like 'Gift Card:<number>' reaches the catch-all validation
branch, a negative amount_tendered previously passed decimal_locale and was
then routed into Giftcard::decrementGiftcardValue, where value - (-N)
increased the balance (store credit minted at will).
- Add nonNegativeDecimal rule + 'Sales.negative_amount_tendered' message to
the catch-all amount_tendered rules in Sales::postAddPayment(); add the
language key to all 46 locale files (populated in en, empty elsewhere).
- Guard Giftcard::decrementGiftcardValue() against non-positive amounts so
the sink itself can no longer add balance from an inverted subtraction.
- Regression tests: controller-level rejection of negative amount_tendered
and model-level rejection of negative/zero decrements.
* Address PR review: align locale keys, drop advisory refs, add decimal_locale message
- Align negative_amount_tendered '=> with all other keys (46 locale files)
- Remove docblock + inline comment above decrementGiftcardValue()
- Remove GHSA ID and attack-detail description from test; scrub redundant comment
- Add decimal_locale message override + focused malformed-amount test
* Fix formatting and spacing in SalesControllerTest
* fix(lang): remove duplicate negative amount tendered key
Consolidate 'negative_amount_invalid' and 'negative_amount_tendered'
translation keys in Sales.php across all locale files. Both keys held
identical messages, causing redundant translation maintenance.
- Drop 'negative_amount_invalid' key, keep 'negative_amount_tendered'
- Move existing translated text into 'negative_amount_tendered' where
it was previously empty
- Applied across all app/Language/*/Sales.php locale files
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* fix(sales): allow negative amount_tendered in return mode
Return transactions legitimately produce negative amount_due and
prefilled amount_tendered values, but validation rules previously
enforced nonNegativeDecimal unconditionally, blocking valid returns.
- Detect return mode via sale_lib->get_mode() in Sales::process
- Build amount_tendered rule conditionally: skip nonNegativeDecimal
check when in return mode, keep it for sale/giftcard flows
- Apply the conditional rule to both giftcard and standard payment
branches
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* test: update expected error message in negative payment test
Sales controller now returns generic numeric-validation message
instead of specific negative-amount message for negative tendered
amounts. Update test assertion to match new lang key.
- tests/Controllers/SalesControllerTest.php: assert
Sales.must_enter_numeric instead of
Sales.negative_amount_tendered
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* test: remove regression tests for GHSA-9847 negative amount fix
Drop testDecrementGiftcardValueRejectsNegativeAmount and
testDecrementGiftcardValueRejectsZeroAmount from GiftcardTest.
- Remove coverage for decrementGiftcardValue() rejecting
non-positive amounts (negative/zero) in tests/Models/GiftcardTest.php
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
---------
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
Co-authored-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
- Validate paymentType is a non-empty string before processing
- Reject negative or zero amounts for all payment types
- Enforce full payment coverage before completing a sale
- Bypass coverage check for invoice and quote mode sales
- Require a valid gift card number before decrementing value;
rollback and return insufficient balance error on missing input
- Add "amount_due_not_covered" and "negative_amount_invalid"
translations across 40+ locales
- Add test coverage for gift card validation, negative amounts,
and quote/invoice zero-payment completion
- Rename snake_case locals to camelCase in postComplete (no behavior change)
bugfix(items, validation): reject unsafe tax names and fix payments temp table collision
- Add unicode_alpha_numeric_punct rule (OSPOSRules) to allow accented/CJK
chars in text fields while blocking HTML-unsafe chars (<, >) as
defense-in-depth against injection
- Items controller: extract validateItemFields/validateBulkUpdateFields,
validate tax_names on save and bulk update using new rule; add shared
validateFields helper in Secure_Controller to DRY up validation +
JSON error response
- Escape tax_group output in sales/quote.php and receipt_email.php views
to harden output encoding at render time
- Rename sales_payments_temp -> sales_report_payments_temp (Summary_report)
and -> sales_search_payments_temp (Sale model) to avoid name collision
between concurrently-created temp tables
- AGENTS.md: document alignment rule for => columns when inserting new
language keys
Tests:
- Add ItemsControllerTest covering postSave/bulkupdate tax_names validation
- Reject <, > in tax_names on /items/save and /items/bulkupdate
- Verify unicode and apostrophe-containing tax names are accepted
- Cover CSV import helpers: header generation (basic, multiple locations,
attributes), stock-location/attribute header builders, get_csv_file
parsing (plain, BOM-prefixed, multi-row)
- Validate required-header detection for import templates
- Remove outdated tax name test from SalesControllerTest
- Simplify Database class references in SalesControllerTest
i18n:
- Add tax_name_invalid translation to Items.php for 20+ locales, inserted
alphabetically after tax_category in each file
- Normalize quote style in ar-EG/Items.php to single quotes
- Add ka/Items.php Georgian locale scaffold
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
fix(employees): harden permissions UI and access control
- Prevent admins from removing their own minimum module grants (employees, home, office)
- Add session_status check before session regeneration
- Disable submit button and return no_access view for AJAX requests
- Replace fade class with active-only for Bootstrap compatibility
- Update permission toggle selectors to .module-toggle
- Add error_cannot_remove_own_minimum_grant translations for Armenian, Bulgarian, Georgian, Swedish, and Ukrainian
---------
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
fix(sales): gate per-record endpoints behind reports_sales grant (REDACTED)
Cashiers holding only the base `sales` grant could reach per-sale endpoints
(getRow, getEdit, postSave, getReceipt, getInvoice, getSendPdf, getSendReceipt)
that require `reports_sales`. getManage() enforced this at the list level, but
individual endpoints did not re-check. Regression tests added.
Auth:
- Introduce `IsLoggedIn` filter to centralize login checks across controllers
- Replace custom `AccessDeniedRedirectException` with built-in `RedirectException`
Employees:
- Add `DISALLOW_PASSWORD_CHANGE` and `DISALLOW_GRANT_CHANGE` env vars to restrict
credential and permission changes in locked-down environments
- Extract `hasGrantsChanged()` to streamline `postSave`
Refactor:
- Rename snake_case variables to camelCase in Sales, Items, and Employees
controllers for PSR-12 compliance
- Use explicit `db_connect()` for transaction clarity in Items controller
Fixes:
- SMTP config entries fall back to defaults via null coalescing
- Migration uses `DROP FOREIGN KEY` instead of `DROP CONSTRAINT`
- Password hash upgrade only sets session on successful `hash_version` update
- Correct lang key for unknown error in Module model
Language:
- Translate `error_grant_change_disallowed` / `error_password_change_disallowed`
across all 44 supported locales with => alignment matching en reference
- Fix "cannot be deleted" messages and misc typos across ~15 language files
Tests:
- Bootstrap seeder only once in ItemsCsvImportTest; close connection after
- Restore `DISALLOW_GRANT_CHANGE` in teardown to prevent side effects
- Use `uniqid()` for test user data to avoid collisions
Signed-off-by: 17935339+objecttothis@users.noreply.github.com
* Implement atomic updates for gift card and reward point decrements, enhance error handling for insufficient balances, and add regression tests for concurrency safety.
* Add translations for insufficient gift card balance and reward points error messages across all supported languages.
* Reorder `clear_suspended_sale_detail` call to ensure transactional consistency.
* Reorder `clear_all` call to align with success and error handling logic.
* Ensure soft-deleted gift cards are excluded in balance updates.
* Refactor change_quantity logic with atomic upserts, improve error handling for insufficient stock, and update related tests and constants.
* Added check for NEW_ENTRY
* Added unit tests to test changes.
* Fix class name casing in ItemQuantityTest for consistency.
* Fix Bulgarian translations for insufficient balance error messages in Sales module.
* Fix Greek translations for insufficient balance error messages in Sales module.
* Fix Armenian translations for insufficient balance error messages in Sales module.
* Fix Tamil translations for insufficient balance error messages in Sales module.
* Implement race condition testing for database methods with concurrent process support.
* Fix class name casing in ItemTest for consistency.
* Improve concurrent process handling in race condition tests; add readiness and synchronization barriers.
* Improve handling of process I/O streams and timeout management in race condition tests.
* Add test for decrementing gift card value when marked as deleted
* Add `finally` block to ensure proper cleanup in async database race condition tests
* Improve error handling and timeout management in async database race condition tests.
* Refactor test utilities to use shared `EmployeeFixtureTrait` and `ItemFixtureTrait`.
* Track process exit codes explicitly in race condition tests for improved error detection and debugging.
* Improve error handling in `ConcurrentDbRaceTrait` by adding exceptions for `mysqli_poll` and `mysqli_reap_async_query`.
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
---------
Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* fix(auth): throttle login attempts to prevent brute-force attacks
Add Throttle filter and wire into login route to mitigate
credential-stuffing/brute-force risk (redacted).
- Register App\\Filters\\Throttle in Filters config
- Add \"too_many_attempts\" language string for throttled responses
- Add tests for Throttle filter and Login controller throttling
* Correct bug causing error to not display.
* i18n(login): add too_many_attempts translation for login throttling
Add localized \"too many attempts\" message across all language files
to support login throttling feature. Message informs users to wait
before retrying after exceeding attempt limit.
* fix(auth): rate limit login attempts to prevent brute-force attacks
Add Throttle filter that rate limits login/migrate POST requests,
keyed by IP and submitted username, using CodeIgniter's cache-based
Throttler (redacted).
- Wire filter into login/migrate routes
- Show localized error message when rate limit exceeded
- Broaden writable/cache ignore pattern to cover throttler cache file
* Update app/Language/ar-EG/Login.php
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* style(i18n): use single quotes for translation array keys and values
Convert double-quoted array keys and string values to single quotes
across all Login.php language files for style consistency.
* test(auth): update LoginTest to use throttler service directly
Replace clearThrottleState's Services::resetSingle('throttler') call
with Services::throttler() to reset state via the service instance.
Add usedKeys property to track throttle keys used across test cases.
* test(auth): skip login test assertion when migration required
LoginTest now check migration-required response state before
asserting HTTP 200. Prevent false failures when app force
pending-migration redirect during test run.
* Added missing return statements
* fix(items): validate item_number and skip receiving quantity default for temp items
- Validate item_number against alpha_numeric_punct rule, return JSON error on failure
- Add item_number_invalid language string
* i18n: reorder Items language keys and add item_number_invalid string
Add item_number_invalid translation across all locale files and
resort surrounding keys alphabetically to match key ordering
convention.
* PSR-12 refactoring.
- Change local variable to camelCase.
- Use single quote in language files.
* i18n: translate item_number_invalid string in ta, th, tl, zh-Hans
Item_number_invalid key had English placeholder text in Tamil,
Thai, Tagalog, Chinese Simplified language files. Translate to
match each locale.
* fix(items): use FormatRules for item_number validation
* refactor(items): use camelCase for variable names
* refactor(items): use camelCase for variable names in Items controller
* Add `reference_code` to sale payment queries and group by statements
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* Refactor `Sales` controller to improve payment handling readability and replace snake_case with camelCase variables
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* Add missing translations for `Sales` language file and include new keys like `must_enter_rrn` and `reference_code`
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* Refactor `Sales` payment handling to use camelCase and extend `addPayment` with `referenceCode` support
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* Refactor `Sales` models, controllers, and libraries to adopt camelCase naming conventions and improve readability
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* Add translations and updates for `must_enter_reference_code` and `reference_code` across language files and update `Sales` controller to replace `must_enter_rrn` with the new key
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* feat(sales): add reference code input and payment type helper
- Add `get_reference_code_payment_types()` to locale_helper as single
source of truth for card-requiring payment types
- Add reference code row to register view, shown/hidden via JS based
on selected payment type
- Fix payment type dropdown width to 100% for consistent layout
- Add min-width to payment buttons and right-padding to button group
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* feat(config): add payment reference code length configuration
- Add payment_reference_code_min and payment_reference_code_max fields
to Config controller save logic
- Add translation keys for reference code length limits across all
language files (min/max label + section header)
- Align array key formatting in Config controller for readability
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* style(lang): normalize string delimiters to single quotes across all language files
Convert double-quoted array keys and values to single quotes in all
app/Language/*/Config.php and app/Language/*/Sales.php variants.
No translation content changed — formatting only.
Also add Localization section to AGENTS.md documenting language file
conventions for new keys and fallback behavior.
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* feat(lang): add payment reference code length translations
Add localized strings for payment_reference_code_length_limits,
payment_reference_code_length_max_label, and
payment_reference_code_length_min_label across all supported locales.
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* test(config,sales): add payment reference code min/max validation tests
- Add baseLocalePayload() helper in ConfigTest for postSaveLocale tests
- Add testSaveLocale_AcceptsValidReferenceCodeMinMax and related boundary tests
- Add Sale_libPaymentTest for payment reference code validation in Sale_lib
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* fix(sales): add type-specific validation for amount_tendered
Gift card payments use amount_tendered as giftcard number (integer);
cash/other payments require decimal_locale format. Apply correct
validation rule per payment type instead of generic required.
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* fix(lang): replace self-closing </br> with <br> in all locales
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* fix(sales): use configurable precision in discount comparison
Replace hardcoded precision 2 with totals_decimals() when comparing
discount against item total via bccomp/bcmul, so discount validation
respects the configured decimal precision setting.
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* fix(lang): correct Azerbaijani translations in Config.php
Replace placeholder/mismatched strings with accurate translations:
- email_mailpath, email_smtp_pass, invoice_email_message
- number_locale_invalid/required, receipt_template
- reward_configuration, right, tax_decimals, theme
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* feat(sales): add reference code support to payment edit flow
- Add reference_code field to new payment row in sale edit form
- Persist reference_code on insert in Sale model
- Validate reference_code_new in Sales controller using configurable min/max length rules
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* feat(lang): add Georgian (ka) language stubs for Config and Sales
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* Match the fallback maximum reference_code length to the maximum of the field in the db
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* Bug fixes
- Add validation of UI settings to prevent overridden values from being passed.
- Correct maximum value in JS for payment_reference_code maximum length to 40.
- Fix bug causing copy_entire_sale() to incorrectly copy the reference code and cash_adjustment
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
---------
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
Co-authored-by: Travis Garrison <travis@chiraqbookstore.com>
Add validation for the mailpath POST parameter to prevent command injection
attacks. The path is validated to only allow alphanumeric characters,
underscores, dashes, forward slashes, and dots.
- Required mailpath when protocol is "sendmail"
- Validates format for all non-empty mailpath values
- Blocks common injection vectors: ; | & ` $() spaces newlines
- Added mailpath_invalid translation to all 43 language files
- Simplified validation logic to avoid redundant conditions
Files changed:
- app/Controllers/Config.php: Add regex validation with protocol check
- app/Language/*/Config.php: Add mailpath_invalid error message (43 languages)
- tests/Controllers/ConfigTest.php: Unit tests for validation
* Fix business logic vulnerability allowing negative sale totals (GHSA-wv3j-pp8r-7q43)
Add server-side validation in postEditItem() to reject negative prices,
quantities, and discounts, as well as percentage discounts exceeding 100%
and fixed discounts exceeding the item total. Also block sale completion
with negative totals in non-return mode to prevent fraud/theft.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Fix: exempt return mode from negative quantity validation
Return mode legitimately stores items with negative quantities.
The quantity validation now skips the non-negative check in return mode,
consistent with the existing return mode exemption in postComplete().
Also use abs() for fixed discount comparison to handle return quantities.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Refactor: use $rules + validate() pattern per review feedback
Address review comments from jekkos on PR #4450:
1. Use CI4 $rules variable with custom non_negative_decimal validation
rule instead of manual if-checks for price/discount validation.
2. Add validation error strings to all 44 non-English language files
(English fallback values used until translations are contributed).
3. Use validate() method with $messages array for localized error
display, maintaining the existing controller pattern.
Additional improvements:
- Add non_negative_decimal rule to OSPOSRules.php (leverages
parse_decimals() for locale-aware decimal parsing)
- Preserve manual checks for business logic (return mode quantity
exemption, discount bounds via bccomp)
- Fix PHP 8.1+ compatibility: avoid passing method return to reset()
- Explicit empty discount handling for bc-math safety
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Fix: rename to nonNegativeDecimal (PSR), clear non-English translation strings
- Rename validation rule method non_negative_decimal → nonNegativeDecimal in
OSPOSRules.php and all $rules/$messages references in Sales.php (PSR naming
per @objecttothis review)
- Replace English fallback text with "" in 43 non-English language files so
CI4 falls back to the base language string; weblate will handle translations
(per @jekkos and @objecttothis agreement)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Paul <morimori-dev@github.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
* 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>
- 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.
* 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
