mirror of
https://github.com/opensourcepos/opensourcepos.git
synced 2026-09-27 21:25:09 -04:00
ca3d982dc28c8781aa359784924643a5836ea312
8
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
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> |
||
|
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> |
||
|
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 |
||
|
BudsieBuds
|
e83c23cf0c |
Improve code style and PSR-12 compliance (#4204)
* 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 |
||
|
objecttothis
|
e90b5b87da |
Replace tabs with spaces (#4196)
Signed-off-by: objecttothis <objecttothis@gmail.com> |
||
|
jekkos
|
b4d117011a | Add CI4 language migration scripts (#3468) | ||
|
jekkos
|
828fd639b2 |
Porting over 4f3226b
|