mirror of
https://github.com/opensourcepos/opensourcepos.git
synced 2026-10-03 08:05:13 -04:00
348a8352cf439c4e1af5d1bc1eb033fc8d0f8646
60
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
b9ad77ba07 |
fix(security): report unwritable .env.lock, make throttle limits configurable (#4714)
* fix(security): report unwritable .env.lock, make throttle limits configurable
envFileIsWritable() previously checked is_writable(.env) (the file), which passes in Docker even when the mutex file is root-owned by a prior env:provision run. It now checks the real write path: the directory (to create .env.tmp.* + .env.lock) and any existing .env.lock must be writable, so the app throws the clear 'run env:provision' error instead of crashing on 'Unable to open .env.lock'.
The Throttle filter now reads throttle.capacity / throttle.seconds from env (default 5 per 60s); capacity <= 0 disables throttling, so operators serving sequential HTTP clients (e.g. Zabbix) are not caught by the lockout.
* fix(security): address CodeRabbit review findings
- Throttle: validate throttle.capacity as an integer before treating a non-positive value as 'disabled', so a non-numeric value (e.g. 'five') falls back to the default instead of silently bypassing the lockout. Apply the same validation to throttle.seconds.
- envFileIsWritable(): also reject an existing non-writable .env on Windows (where rename() cannot replace a read-only destination); keep the check Windows-only since POSIX rename() replaces a read-only dest when the directory is writable.
- Tests: restore the prior throttle.capacity env state in ThrottleTest (capture/restore instead of delete); add an invalid-capacity fallback case; skip the not-writable fixtures when running as root (where is_writable() is bypassed).
* fix(migration): guard ConvertToCI4 key-write branches with envFileIsWritable()
The migration's 'no key' and 'CI3 key' branches called rotateEncryptionKey()/rotateEncryptionKeyTransaction() directly, bypassing the envFileIsWritable() guard that checkEncryption() uses. On a fresh Docker/Compose install where the web runtime cannot write /app/.env.lock, this produced a raw 'fopen(/app/.env.lock): Permission denied' error instead of the actionable 'run php spark env:provision' message.
A valid CI4 key now short-circuits to checkEncryption() (no write); every write branch is gated on envFileIsWritable() first.
* fix: read provisioned encryption.key so config sees it
env:provision persists the key as 'encryption.key' in .env (matching the
throttle path and .env.example), but Config\Encryption only read the
ENCRYPTION_KEY env var. On a fresh Docker instance the provisioned key was
invisible to config('Encryption')->key, so the app believed no key existed
and tried to write one -- hitting the .env.lock permission wall.
Read encryption.key (via $_SERVER/$_ENV/getenv) first, then fall back to
ENCRYPTION_KEY for Docker '-e' usage. Mirrors checkThrottleEncryption().
* fix: cascade encryption.key lookup past empty-string sources
* refactor: extract Encryption::resolveKey() and test it without global env mutation
* fix(security): decode fallback-selected encryption key like BaseConfig
A key picked in the constructor fallback (notably ENCRYPTION_KEY, which
BaseConfig never inspects) was assigned verbatim, bypassing the
hex2bin:/base64: decode the parent applies to `encryption.key`. Route the
selected key through a parseKey() helper mirroring BaseConfig's
parseEncryptionKey() so prefixed values decrypt consistently, and add a
pure regression test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore: remove advisory ID from comments and tighten verbose comments
Per review: treat GHSA advisory IDs like secrets (drop from code) and
replace the multi-paragraph comments with concise one-liners that keep
the non-obvious "why". No logic changes.
---------
Co-authored-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
jekkos
|
9eaa2f34f3 |
chore: strip advisory IDs from code comments and changelog (#4720)
Per the project's policy of treating security advisory IDs as secret-like, remove the identifiers embedded in source/test comments and CHANGELOG entries. Each keeps its human-readable description (and PR number where present), so traceability is preserved. No logic changes. |
||
|
jekkos
|
dc1accc65a | fix(security): HTML-escape attribute dropdown option labels in items attributes view (#4715) | ||
|
jekkos
|
00b97c3302 |
feat(security): add THROTTLE_KEY env-var fallback for throttle.key (#4707)
checkThrottleEncryption() now consults the THROTTLE_KEY environment variable when throttle.key is empty, mirroring the ENCRYPTION_KEY fallback in Config/Encryption. This lets Docker/Compose deployments supply the throttle HMAC secret without writing a shared value into a read-only .env. An explicit throttle.key always takes precedence. Adds regression tests to the existing security_helperTest suite and documents THROTTLE_KEY in .env.example. |
||
|
jekkos
|
edcb4bb655 |
Fix GHSA-frx7-c5vv-m3mr: recompute cashup total server-side and force owner identity (#4706)
* Fix GHSA-frx7-c5vv-m3mr: recompute cashup total server-side and force owner identity * Add required description field to cashup test POSTs |
||
|
jekkos
|
47aade5024 |
fix(locale): validate language_code against known locales to block path traversal (#4704)
* fix(locale): validate language_code against known locales to block path traversal postSaveLocale() stored language_code from user input with no allow-list validation, and it later flows into Language::setLocale()/load() where the locale segment is require()'d. An authenticated config-grant account could store a relative path (e.g. ../../public/uploads) and, combined with a planted file in public/uploads/, achieve unauthenticated RCE on the next request. Validate the submitted language against array_keys(get_languages()) before storing, and harden languageExists() to reject path separators and dot-dot sequences. Adds regression tests. * test(locale): give locale fixture valid reference-code min/max defaults * fix(locale): reject null bytes in languageExists guard A stored language_code containing a NUL byte passes the existing path-separator and parent-dir checks, then reaches file_exists(). On PHP 8.5+ file_exists() throws a ValueError for NUL-byte paths, which breaks configuration loading. Reject NUL bytes in the guard and add regression tests. |
||
|
|
184918d914 |
fix(security): handle special characters in .env key values and improve insertion logic (#4656)
* 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> |
||
|
objecttothisandcoderabbitai[bot]
|
b610ae28ac |
fix(validation): broaden sendmail path regex, expand i18n, strip advisory IDs
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> |
||
|
jekkos
|
28755dfd50 |
fix(tests): resolve all phpunit failures — clean-DB suite green (#4626) (#4691)
* fix(tests): resolve all phpunit failures (#4626) Bring the phpunit suite from 153 failures to 0 (281 tests passing): - Employee: decouple grants block from save_value success; restructure save_employee new-employee + disallowed-grants early return - Sale: unify sales_payments_temp schema (add sale_cash_refund, reference_code) so both creators produce an identical superset table - Employees controller: provide placeholder password/hash in testing env so new-employee insert succeeds and grant logic is testable - TestDatabaseBootstrapSeeder: reset shared connection table-name cache after bootstrap reset to avoid stale listTables()/tableExists() results - Config: fix postSaveLocale validation rule syntax - Test data: use unique employee usernames to avoid UNIQUE constraint collisions latching strict-mode transStatus=false on the shared conn - Various test-file and language-string corrections * test: consolidate employee fixtures in shared trait Route test employee creation through a single EmployeeFixtureTrait that delegates to Employee::save_employee(), so fixtures exercise the same production code path instead of raw DB inserts. Removes six near-duplicate helpers across EmployeeTest, SalesControllerTest, and EmployeesControllerTest while preserving each test's specific grant set. Closes a piece of the fixture-scattering flagged in #4626. Closes #4626 * test: add global DROP/CREATE grant and commit theme fixtures * fix(ci): remove redundant symlink step, set working encryption key * fix(ci): run phpunit with --no-coverage to avoid no-driver warning * fix: address code review findings - Config: restore strict locale validation (min required|integer|>0) and fix max cross-field check with a new gte_field rule (CI4's greater_than_equal_to[field] does not resolve the field value) - Tests: assert rejection for non-numeric/zero/negative/min>max limits - .env.example: remove shared hard-coded encryption.key (auto-generates); document Docker env-var usage - phpunit.yml: scope CREATE/DROP grant to ospos_test.* and provision a per-run encryption key as an env var * feat: support ENCRYPTION_KEY env var for encryption key Read ENCRYPTION_KEY as a fallback for the encryption key when the config value is empty. This is a supported, reliable path for Docker / container deploys and CI, avoiding reliance on the raw dotted encryption.key env var. * fix: align Summary_report temp tables with Sale temp table schema Summary_report created sales_items_taxes_temp and sales_payments_temp with fewer columns than the canonical create_temp_table() in Sale.php. A later reader expecting those columns hit a schema-mismatch SQL error on the shared temp tables. Add internal_tax/sales_tax (sales_items_taxes_temp) and reference_code (sales_payments_temp) so all creators emit the identical column set. |
||
|
objecttothis
|
9ecabf6f41 |
fix(sales): harden unsuspend with auth, status gating, and null safety
- Require reports_sales grant on postUnsuspend; return 403 on denial - Reject unsuspend of non-SUSPENDED sales; skip silently on invalid state - Move clear_all() after validation so an invalid sale_id no longer wipes the active in-progress cart - Null-guard get_sale_status() on missing row instead of fatal property access; widen return type to ?int - Fix getSaleType null-coalescing — CI4 session default only fires when key is unset, not when value is null - Rename get_sale_type → getSaleType, sale_id → saleId (PSR-12 camelCase) - Extract SaleFixtureTrait with createSale()/createSuspendedSale(); add regression coverage for auth denial, status gating, and cart preservation |
||
|
jekkosandobjecttothis
|
839821e2eb |
bugfix(sales): reject non-negative gift-card amount_tendered (#4674)
* 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> |
||
|
jekkos
|
bdbc6d9cf1 |
fix(sales): gate getSearch behind reports_sales grant
Sales::getSearch() — the AJAX endpoint backing the Sales Takings list — lacked the authorization check present on all sibling endpoints (getRow, getEdit, postSave, getReceipt, getInvoice), allowing a cashier with only the base sales grant to pull the full ledger. - Add reports_sales guard with 403 JSON response on denial - Add regression tests: cashier without grant → 403; employee with grant → search payload returned - Clarify getSearch() coverage in SalesControllerTest comments - Remove duplicate test methods introduced during initial commit |
||
|
objecttothis
|
f5f9052de1 |
fix(sales): harden payment validation and gift card handling
- 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) |
||
|
objecttothis
|
fdc1c38b43 |
feat(validation, tests): add valid_path_strict rule and integrate into mailpath validation (#4684)
- Introduce `valid_path_strict` rule in `OSPOSRules` to enforce stricter path validation, preventing security issues like injection attempts with newline or special characters. - Update mail configuration validation in `Config` controller to use the new rule for the `mailpath` field. - Add unit tests in `OSPOSRulesTest` to cover edge cases for `valid_path_strict`. Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com> |
||
|
objecttothis
|
6db3dde491 |
Bugfix tax names (#4677)
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> |
||
|
objecttothis
|
905a447e55 |
fix(reports, home): resolve double-URL-decoding bypass for method grants (#4660) (#4666)
fix(reports, home): strengthen method grant validation and URI decoding (#4660) - Reports, Home: fix URI decoding in hasGrant() method checks to use urldecode consistently, preventing malformed URI segments from bypassing access controls - Reports: rename snake_case variables to camelCase for PSR-12 compliance - Reports: adjust access checks to accurately handle null submodule IDs Tests: - Add grant check tests for encoded URI inputs across Reports and Home - Add test case for employee access with base reports grant - Add secondary grant check for reports_customers in relevant test cases - Confirm logout bypass remains functional and properly controlled - Refactor TestDatabaseBootstrapSeeder to expose static reset() for per-class DB re-initialization instead of only via seeder run() - Standardize session handling, setup logic, and boolean declarations - Use unique data in test helpers to avoid collisions - Add docblocks to ReportsControllerTest and HomeTest for PSR-5 compliance - Add exception handling for failed employee creation in test setup - Wrap password validation test in try-finally to guarantee state cleanup Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com> |
||
|
objecttothis
|
8cfd1a4b1d |
fix(sales): enforce reports_sales grant on search endpoint (#4673)
- Sales::getSearch now enforces reports_sales grant before running search, returns 403 with lang message when missing - Rename snake_case helpers/methods to camelCase across Sales controller, Sale model, and tabular_helper (get_sale_data_row -> getSaleDataRow, get_payments_summary -> getPaymentsSummary, sales_headers -> salesHeaders, etc.) - Config/OSPOS: reset DB data cache before checking app_config table existence to avoid stale schema cache in tests - TestDatabaseBootstrapSeeder: expose static reset() so tests can rebuild schema once per class instead of only via seeder run() - SalesControllerTest: bootstrap DB once per class, seed once, refresh app settings each setUp, add tests for search endpoint authorization (cashier denied, supervisor allowed), move createTestItem into shared ItemFixtureTrait Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com> |
||
|
objecttothis
|
d63d31700d |
Ensure payload data is escaped to prevent XSS (#4664)
fix(barcode): escape payload fields to prevent XSS; PSR-12 refactor - Apply `esc()` to name, ID, item number, category, and company name in `Barcode_lib` payloads - Remove redundant `urldecode()` in `Item_kitsController` to prevent triple decoding - Rename variables and methods to camelCase across barcode, item_kits, and tests - Add type hint for `$layoutType` parameter in `manageDisplayLayout` - Add/update unit tests covering escaping and HTTP response assertions Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com> |
||
|
objecttothis
|
c701d21886 |
fix(sales): gate per-record endpoints behind reports_sales grant (#4627)
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 |
||
|
objecttothis
|
cc93c31355 |
fix(items): add explicit sentinel value for clearing supplier in bulk edit (#4617)
* fix(items): add explicit sentinel value for clearing supplier in bulk edit This fixes a regression introduced in the fix for [REDACTED] Introduce `Item::CLEAR_SUPPLIER_OPTION = 'NONE'` to distinguish between \"leave supplier_id unchanged\" (empty string) and \"clear supplier_id\" (sentinel). Previously, empty string was ambiguous. - Add `CLEAR_SUPPLIER_OPTION` constant with doc comment explaining intent - Update supplier dropdown to include sentinel as first real option - Shift empty string to mean \"do nothing\" across all bulk edit fields * test(items): add regression tests for mass assignment in bulk edit Cover [REDACTED]: Item::update_multiple() bypasses model $allowedFields via Query Builder, allowing unintended field writes during bulk edit operations. * fix(items): add type validation to bulk-edit field filter filterBulkEditFields now validates field values before accepting them: - Non-scalar values are rejected (array injection guard) - Price/quantity fields are locale-parsed to floats, invalid strings skipped - Boolean fields must be 0 or 1, other values skipped - supplier_id must be numeric; CLEAR_SUPPLIER_OPTION still nulls it Update tests to assert parsed types (float for prices, int for supplier_id) and replace the fill-all-fields fixture with a realistic input that only covers fields a form would actually submit. * test(items): add supplier cleanup and helper methods to bulk update tests - Track created supplier person IDs for teardown cleanup - Delete supplier records in tearDown to prevent test pollution - Extract item/supplier creation into reusable helper methods * style(tests): rename variables to camelCase in ItemBulkUpdateTest * refactor(items): rename snake_case variables to camelCase Convert Item model, Items controller, and bulk update tests to PSR-compliant camelCase naming per project conventions. - Rename update_multiple to updateMultiple in Item model - Rename local variables (item_data, items_to_update, tax_names, etc.) to camelCase across Items controller and Item model - Update ItemBulkUpdateTest to use new updateMultiple method name - Reorder and update AGENTS.md naming conventions * style(tests): convert snake_case variables to camelCase in ItemBulkUpdateTest Rename local variables and property names to camelCase for PSR-12 consistency, matching convention used elsewhere in new test code. |
||
|
objecttothis
|
61bb1a2c2a |
hotfix(auth): hash throttler keys to improve security (#4646)
* fix(auth): hash throttler keys to improve security - Use MD5 hashing for IP and username-based throttler keys to obfuscate sensitive data while maintaining functionality. * test(auth): add IPv6 throttling test and hash used throttler keys - Add a test to ensure throttling works correctly with IPv6 addresses. - Update throttler keys to use MD5 hashes for IPs and usernames for improved security and consistency. * fix(auth): handle non-scalar usernames in throttler keys - Ensure username input is validated as scalar before processing to prevent errors and maintain throttling logic integrity. * fix(auth): enhance throttler key security with HMAC hashing - Replace MD5 with HMAC-SHA256 for generating throttler keys. - Include encryption key from app configuration for added security. * test(filters): update ThrottleTest to use HMAC-SHA256 for throttler keys - Replace MD5 with HMAC-SHA256 for generating throttler keys in tests. - Introduce `check_encryption()` to ensure encryption configuration is available. * fix(events): validate encryption key on app initialization - Throw ConfigException if encryption key is missing or invalid during `pre_system` event. - Remove redundant `check_encryption()` call from Throttle filter and tests. * fix(events): improve encryption key validation in `pre_system` - Add `check_encryption()` helper call for additional security verification. - Update error message to highlight `.env` writability issues if the key is invalid. * test(filters): handle non-scalar usernames in ThrottleTest - Update `makeRequest` to validate usernames as scalar and cast them to strings before processing. - Add a test to ensure array usernames are ignored, and throttling is applied only based on IP. - Improve status code assertions for throttled requests. --------- Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com> |
||
|
objecttothis
|
29a9b1a7e7 |
Bugfix: Resolve Race Condition in Rewards and Gift Card Spending (#4640)
* 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> |
||
|
objecttothis
|
a2b91493c7 |
Feature: CodeIgniter Throttler (#4619)
* 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
|
||
|
Sai Asish Yandobjecttothis
|
0e8fc0963a |
Reject item CSV imports whose header row is missing required columns (#4597)
* Reject item CSV imports whose header row is missing required columns Signed-off-by: Sai Asish Y <say.apm35@gmail.com> * Require all template columns when validating CSV import headers Signed-off-by: Sai Asish Y <say.apm35@gmail.com> * fix: derive required CSV import headers from the template generator --------- Signed-off-by: Sai Asish Y <say.apm35@gmail.com> Co-authored-by: objecttothis <17935339+objecttothis@users.noreply.github.com> |
||
|
objecttothisandTravis Garrison
|
5c9b1b81e6 |
fix(xss): remove redundant escaping that double-encoded item attribute values (#4628)
* fix(xss): remove redundant escaping that double-encoded item attribute values - Remove esc()/html_entity_decode() calls now that output is escaped at render time by the framework, preventing double-encoding of special characters in attribute names, units, and definition values - Fix employee_name form_input value fields to stop pre-escaping before form_input applies its own escaping - Reorder Items.php use statements and add missing BaseConnection import - Change items/manage.php start_date from let to plain assignment for proper reassignment scope Signed-off-by: Travis Garrison <travis@chiraqbookstore.com> * test(sales): add regression tests for permission checks on sales endpoints - Ensure role-based permissions correctly restrict access to sensitive actions like price edits, receipt/invoice views, and report generation. - Add tests for both granted and restricted user scenarios to validate the behavior. Signed-off-by: Travis Garrison <travis@chiraqbookstore.com> * fix(attributes): validate `attribute_value` before processing - Add checks to ensure `attribute_value` is a non-empty string in `postSaveAttributeValue` and `postDeleteDropdownAttributeValue` methods. - Return error response if validation fails to prevent invalid data handling. Signed-off-by: Travis Garrison <travis@chiraqbookstore.com> * fix(attributes): improve error handling and optimize affected items processing - Use `array_column` for extracting item IDs to streamline logic. - Add JSON validation with `JSON_THROW_ON_ERROR` and return proper error response for invalid `definition_values`. Signed-off-by: Travis Garrison <travis@chiraqbookstore.com> * test(sales): enable database refresh for consistent test state Signed-off-by: Travis Garrison <travis@chiraqbookstore.com> * test(sales): assert unauthorized message is displayed on restricted access Signed-off-by: Travis Garrison <travis@chiraqbookstore.com> * refactor(attributes): use camelCase for `attributeValue` in controller methods - Standardize variable naming in `postSaveAttributeValue` and `postDeleteDropdownAttributeValue` methods by switching to camelCase. Signed-off-by: Travis Garrison <travis@chiraqbookstore.com> --------- Signed-off-by: Travis Garrison <travis@chiraqbookstore.com> Co-authored-by: Travis Garrison <travis@chiraqbookstore.com> |
||
|
objecttothisandTravis Garrison
|
a5d70c07bb |
fix(auth): validate gcaptcha before password to prevent bypass (#4618)
* fix(auth): validate gcaptcha before password to prevent bypass
Move gcaptcha check before credential validation so a valid captcha
is required prior to any login attempt. Previously, password auth
ran first, allowing timing-based enumeration without captcha.
Signed-off-by: Travis Garrison <travis@chiraqbookstore.com>
* test(auth): add regression tests for gcaptcha validation order in OSPOSRules
Guards fix from
|
||
|
|
2c69dc0c34 |
fix(config): validate theme param to prevent XSS via invalid theme (#4620)
* fix(config): validate theme param to prevent XSS via invalid theme Add validation rule for theme field before batch save, rejecting requests with unrecognized theme values. Add test coverage for theme validation in postSaveGeneral. Signed-off-by: Travis Garrison <travis@chiraqbookstore.com> * Update app/Config/Validation/OSPOSRules.php Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Signed-off-by: Travis Garrison <travis@chiraqbookstore.com> Co-authored-by: Travis Garrison <travis@chiraqbookstore.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> |
||
|
objecttothisandTravis Garrison
|
9c542efaf6 |
Feature: Payment reference code (#4587)
* 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> |
||
|
objecttothis
|
e6388deed8 |
Fix overly lenient date validation (#4574)
* Fix overly lenient date validation - In the past date validation would roll over dates that didn't exist into the next month. Now they return an error. Signed-off-by: objec <objecttothis@gmail.com> * Refactor naming - Refactor parameter - Refactor function name. Signed-off-by: objec <objecttothis@gmail.com> * Add unit tests for LocaleHelper Signed-off-by: objec <objecttothis@gmail.com> * Remove files from being tracked. Signed-off-by: objec <objecttothis@gmail.com> --------- Signed-off-by: objec <objecttothis@gmail.com> |
||
|
jekkosandOllama
|
84aeeb52fe |
fix(security): Fix DOMPDF RCE and customer email sanitization (#4568)
* fix(security): Fix DOMPDF RCE and customer email sanitization - Disable isPhpEnabled in DOMPDF to prevent RCE via embedded PHP in HTML - Disable isRemoteEnabled to prevent SSRF attacks - Add email validation and sanitization in CSV import (FILTER_SANITIZE_EMAIL, FILTER_VALIDATE_EMAIL) - Reject invalid email formats during customer import * fix(security): Escape email addresses in mailto() to prevent XSS Email columns in bootstrap tables had escaping disabled (line 52) and mailto() function doesn't escape its parameters. This fix escapes email addresses before passing to mailto() in: - get_person_data_row() (employees) - get_customer_data_row() (customers) - get_supplier_data_row() (suppliers) Attack vector: Malicious email via CSV import renders XSS in table view. * test(security): Add tests for customer CSV import email validation Tests cover: - Valid email acceptance - Invalid email rejection with row-specific error - XSS payload sanitization in email field - Mixed valid/invalid email handling - Email with special characters sanitization Verifies fixes for customer email import vulnerability. * fix(security): Allow empty email addresses in customer import - Empty emails are now allowed (customers may not have email addresses) - Validation only applies when email is non-empty - Added test case for empty email acceptance This fixes a regression where FILTER_VALIDATE_EMAIL rejected empty strings, breaking imports for customers without email addresses. --------- Co-authored-by: Ollama <ollama@steganos.dev> |
||
|
objecttothis
|
9509a97164 |
Add fallback for allowedHostnames environment variable (#4565)
* Add fallback for allowedHostnames environment variable - In some cases allowedHostnames is set in env but not loaded at the time of check, yet available in other sources. This adds fallback checks. - Add UnitTest Signed-off-by: objec <objecttothis@gmail.com> * Improve the fallback logic for allowedHostnames environment variable Signed-off-by: objec <objecttothis@gmail.com> --------- Signed-off-by: objec <objecttothis@gmail.com> |
||
|
|
2f5c0130f4 |
feat: add ALLOWED_HOSTNAMES environment variable support for Docker/Compose (#4544)
Allow configuring allowed hostnames via ALLOWED_HOSTNAMES environment variable as an alternative to app.allowedHostnames in .env file. This is more convenient for Docker/Compose deployments where environment variables are set directly in compose files. The ALLOWED_HOSTNAMES variable takes precedence over app.allowedHostnames if both are set, allowing deployment-specific overrides. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Ollama <ollama@steganos.dev> Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> |
||
|
objecttothisandcoderabbitai[bot]
|
577cf55b6a |
[Feature]: Case-sensitive attribute updates and CSV Import attribute deletion capability (#4384)
PSR and Readability Changes - Removed unused import - Corrected PHPdoc to include the correct return type - Refactored out a function to get attribute data from the row in a CSV item import. - refactored snake_case variables and function names to camelCase - Refactored the naming of saveAttributeData() to better reflect the functions purpose. - Improved PHPdocs - Remove whitespace - Remove unneeded comment - Refactored abbreviated variable name for clarity - Removed $csvHeaders as it is unused - Corrected spacing and curly brace location - Refactored Stock Locations validation inside general validation Bugfixes - Fixed bug causing attribute_id and item_id to not be properly assigned when empty() returns true. - Fixed bug causing CSV Item import to not update barcode when changed in the import file. - Fixed saveAttributeValue() logic causing attribute_value to be updated to a value that already exists for a different attribute_id - Fixed bug preventing Category as dropdown functionality from working - Fixed bug preventing barcodes from updating. in Item CSV Imports - Corrected bug in stock_location->save_value() - Corrected incorrect helper file references. - Removed duplicate call to save attribute link - Rollback transaction on failure before returning false - Rollback transaction and return 0 on failure to save attribute link. - Account for '0' being an acceptable TEXT or DECIMAL attributeValue. - Corrected Business logic - Resolved incorrect array key - Account for 0 in column values - Correct check empty attribute check - Previously 0 would have been skipped even though that's a valid value for an attribute. - Removed unused foreach loop index variables - Corrected CodeIgniter Framework version to specific version UnitTest Seeder and tests - Created a seeder to automatically prepare the test database. - Modified the Unit Test setup to properly seed the test database. - Wrote a unit test to test deleting an attribute from an item through the CSV. - Corrected errors in unit tests preventing them from passing. save_value() returns a bool, not the itemId - Fix Unit Tests that were failing - Corrected the logic in itemUpdate test - Replaced precision test with one reflecting testing of actual value. - This test does not test cash rounding rules. That should go into a different test. - Correct expected value in test. - Update app/Database/Seeds/TestDatabaseBootstrapSeeder.php - Added check to testImportDeleteAttributeFromExistingItem - Correct mocking of dropdowns - Remove code depending on removed database.sql - Removed FQN in seeder() call - Added checks in Database seeder - Moved the function to the attribute model where it belongs which allows testability. Case Change Capability (CSV Import and Form) - CSV Import and view Case Changes of `attribute_value` - Store attribute even when just case is different. - Add getAttributeValueByAttributeId() to assist in comparing the value - Corrected Capitalization in File Handling Logic CSV Import Attribute Link Deletion Capability - Validation checks bypass magic word cells. - Delete the attribute link for an item if the CSV contains `_DELETE_` - Added calls to deleteOrphanedValues() - Items CSV Import Attribute Delete - Exclude the itemId in the check to see if the barcode number exists Error Checking and Reporting Improvements - Fail the import if an invalid stock location is found in the CSV - Return false if deleteAttributeLinks fails - Match sanitization of description field to Form submission import - Fold errors into result and return value - Populated $allowedStockLocations before sending it to the validation function - Added logic to not ignore failed saveItemAttributes calls - Add error checking to failed row insert - Reworked &= to && logic so that it short-circuits the function call after if success is already false. - Add transaction to storeCSVAttributeValue function to prevent deleting the attribute links before confirming the new value successfully saved. - Modified generate_message in Db_log.php to be defensive. Attribute Improvements - Move ATTRIBUTE_VALUE_TYPES to the helper - Normalize AttributeId in saveAttributeLink() - normalize itemId in saveAttributeLink() - Account for '0' in column values for allow_alt_description - Remove duplicate saveAttributeValue call - Correct return value of function - Like other save_value() functions, the location_data variable is passed by reference. - Unlike other save_value() functions, the location_data variable is not being updated with the primary key id. - Added updateAttributeValue() function as part of logic fix. - Added attribute_helper.php - Simplified logic to store attribute values --------- Signed-off-by: objec <objecttothis@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> |
||
|
jekkosandOllama
|
e70395bb85 |
Fix: Improve allowedHostnames .env configuration and fail-fast in production (#4482)
* Fix: Improve allowedHostnames .env configuration and fail-fast in production Addresses GitHub issue #4480: .env app.allowedHostnames does not work as intended ## Problem - CodeIgniter 4 cannot override array properties from .env - Setting app.allowedHostnames.0, app.allowedHostnames.1 did NOT populate the array - Application always fell back to 'localhost' silently in production - Host header injection protection was effectively disabled ## Solution 1. Support comma-separated .env values: app.allowedHostnames = 'domain1.com,domain2.com' 2. Fail explicitly in production if not configured (throws RuntimeException) 3. Allow localhost fallback in development/testing with ERROR-level logging 4. Update documentation with clear setup instructions ## Changes - app/Config/App.php: Parse comma-separated .env values, fail in production - .env.example: Update format documentation - INSTALL.md: Add prominent security section - tests/Config/AppTest.php: Comprehensive tests for new behavior Fixes #4480 Related: GHSA-jchf-7hr6-h4f3 --------- Co-authored-by: Ollama <ollama@steganos.dev> |
||
|
Ollama
|
8da4aff262 |
fix(security): prevent command injection in sendmail path configuration
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 |
||
|
Ollama
|
56670271d6 |
fix: remove duplicate phpunit.xml that prevented tests from running
The tests/phpunit.xml was incomplete - it only configured helpers and Libraries testsuites, while phpunit.xml.dist at root contains all tests. PHPUnit was likely using the incomplete config, resulting in empty test results. |
||
|
Ollama
|
f74f286a51 |
feat: migrate CI from Travis to GitHub Actions with enhancements
- Convert Travis CI configuration to GitHub Actions workflows - Add multi-arch Docker builds (amd64/arm64) - Implement initial schema migration for fresh database installs - Add multi-attribute search with AND logic and sort by attribute columns - Address various PR review feedback and formatting fixes |
||
|
Ollama
|
38d672592b |
Add seed data to tests for proper integration testing
- Add setUp() to seed test data: items, sales, sales_items, sales_items_taxes - Add tearDown() to clean up seeded data after tests - Remove skip conditions since we now have guaranteed test data - Add testTaxDataIsGroupedByTaxNameAndPercent to verify grouping - Use narrow date range to isolate seeded data |
||
|
Ollama
|
6f7e06e986 |
Rewrite tests to use database integration testing
Tests now: - Use DatabaseTestTrait for real database integration - Actually call getData() and getSummaryData() methods - Verify row totals (subtotal + tax = total) from real queries - Verify summary data matches sum of rows - Test getDataColumns() returns expected structure - Use assertEqualsWithDelta for float comparisons with tolerance These tests exercise the actual SQL queries and verify the mathematical consistency of the calculations returned. |
||
|
Ollama
|
fda40d9340 |
Fix rounding consistency and update tests per review feedback
- Ensure total = subtotal + tax by deriving total from rounded components - Use assertEqualsWithDelta for float comparisons in tests - Add defensive null coalescing in calculateSummary helper - Add missing 'count' key to test data rows - Add testRoundingAtBoundary test case |
||
|
Ollama
|
b49186ec7c |
Add unit tests for Taxes Summary Report calculations
Tests verify: - Row totals add up (subtotal + tax = total) - Summary totals match sum of row values - Tax-included and tax-not-included modes calculate correctly - Rounding consistency across calculations - Negative values (returns) are handled correctly - Zero tax rows are handled correctly |
||
|
Ollama
|
234f930079 |
Fix strftime directives handling and tighten test assertions
- Remove incorrect %C mapping (was mapping century to full year) - Add special handling for %C (century), %c (datetime), %n (newline), %t (tab), %x (date) - Add %h mapping (same as %b for abbreviated month) - Tighten edge-case test assertions to use assertSame/assertMatchesRegularExpression - Add tests for new directives: %C, %c, %n, %t, %x, %h |
||
|
Ollama
|
3001dc0e17 |
Fix: Pass parameter to generate() and add composite format tests
- Fixed bug where render() was not passing caller-supplied to generate(), causing ad-hoc tokens to be ignored - Added %F (yyyy-MM-dd) and %D (MM/dd/yy) composite date formats to the IntlDateFormatter pattern map - Added test coverage for composite date format directives (%F, %D, %T, %R) |
||
|
Ollama
|
3ba207e8b9 | Use CIUnitTestCase for consistency with other tests | ||
|
Ollama
|
d684c49ebd |
Fix Token_lib::render() for PHP 8.4 compatibility
- Replaced deprecated strftime() with IntlDateFormatter - Added proper handling for edge cases: - Strings with '%' not in date format (e.g., 'Discount: 50%') - Invalid date formats (e.g., '%-%-%', '%Y-%q-%bad') - Very long strings - Added comprehensive unit tests for Token_lib - All date format specifiers now mapped to IntlDateFormatter patterns |
||
|
Ollama
|
7cb1d95da7 |
Fix: Host Header Injection vulnerability (GHSA-jchf-7hr6-h4f3)
Security: Prevent Host Header Injection attacks by validating HTTP_HOST against a whitelist of allowed hostnames before constructing the baseURL. Changes: - Add getValidHost() method to validate HTTP_HOST against allowedHostnames - If allowedHostnames is empty, log warning and fall back to 'localhost' - If host not in whitelist, log warning and use first allowed hostname - Update .env.example with allowedHostnames documentation - Add security configuration section to INSTALL.md - Add unit tests for host validation This addresses the security advisory where the application constructed baseURL from the attacker-controllable HTTP_HOST header, allowing: - Login form phishing via manipulated form actions - Cache poisoning via poisoned asset URLs Fixes GHSA-jchf-7hr6-h4f3 |
||
|
jekkosandOllama
|
ce411707b4 |
Fix SQL injection in suggestions column configuration (#4421)
* 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> |
||
|
jekkosandOllama
|
ee4d44ed39 |
Fix IDOR vulnerability in password change (GHSA-mcc2-8rp2-q6ch) (#4427)
* 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>
|
||
|
jekkos
|
f25a0f5b09 |
Refactor: Move ADMIN_MODULES to constants, rename methods to camelCase
- 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 |
||
|
jekkosandjekkos
|
ca6a1b35af |
Add row-level authorization to password change endpoints (#4401)
* 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> |