The check_encryption() function now properly handles Docker/container
environments where ROOTPATH/.env may be read-only or ephemeral.
Changes:
- Returns false when key persistence fails instead of always returning true
- Removes error suppression (@) to properly detect write failures
- Adds fallback to WRITEPATH/config/encryption.key for container volumes
- Splits logic into separate functions for clarity and testability
Fixes encryption key being lost on container restarts, which caused
stored passwords to become undecryptable.
GitHub-Issue: #4554
Add fallback key loading from WRITEPATH in Encryption config
When encryption key is not available from .env or environment variables,
the config now attempts to load from WRITEPATH/config/encryption.key.
This supports Docker environments where:
- .env file is read-only or ephemeral
- Key was persisted to the writable volume via check_encryption()
GitHub-Issue: #4554
Handle encryption unavailability gracefully in controllers
Changed EncrypterInterface property to nullable and added proper error
handling for cases where encryption key is not available.
Changes:
- Config controller: nullable encrypter property, try/catch around encryption
- Email_lib: check encryption before using encrypter
- Return meaningful error messages when encryption fails
- Log warnings when passwords saved without encryption
Users will now see clear error messages instead of unhandled exceptions
when encryption key cannot be initialized.
GitHub-Issue: #4554
Add encryption_failed error message to language file
Added localization string for encryption failure error messages.
GitHub-Issue: #4554
Add decrypt_value() and encrypt_value() helper functions
Extracts the recurring decryption/encryption pattern into reusable helper
functions with consistent error handling:
- decrypt_value(): Safely decrypts encrypted values with try/catch
- encrypt_value(): Safely encrypts values with error handling
Both functions handle:
- Empty/null values gracefully
- Missing encryption key (logs warning)
- Encryption/decryption failures (logs error, returns default)
This pattern appears in 8+ locations across the codebase.
GitHub-Issue: #4554
Refactor all encryption/decryption to use helper functions
Replaces direct encrypter calls with decrypt_value() and encrypt_value()
helpers throughout the codebase for consistent error handling:
- Config controller: SMTP, SMS, Mailchimp credential encryption
- Email_lib: SMTP password decryption
- Sms_lib: SMS password decryption
- Mailchimp_lib: API key decryption
- Customers controller: Mailchimp list ID decryption
Removes nullable EncrypterInterface property from Config controller as
encryption is now handled via helper functions.
GitHub-Issue: #4554
Address CodeRabbit feedback: validate key length, clarify encryption failure handling
- loadKeyFromWritable() now validates key length >= 64 before accepting
- encrypt_value() renamed param, defaults to failing encryption required
- Clearer error message when credentials not saved
GitHub-Issue: #4554
fix: address CodeRabbit review comments for encryption key persistence
- Always mirror encryption key to both .env and WRITEPATH (Docker safety)
- Guard array key access with isset() before reading in Encryption.php
- Fix encrypt_value() to not treat string '0' as empty
- Improve error logging for failed encryption attempts
refactor: PSR-compliant naming and address objecttothis review comments
- Rename functions to camelCase: checkEncryption, writeEncryptionKeyToEnv, writeEncryptionKeyToWritable, loadEncryptionKeyFromWritable, abortEncryptionConversion, removeBackup, decryptValue, encryptValue
- Update all callers in Config.php, Customers.php, Migrations, Email_lib.php, Sms_lib.php, Mailchimp_lib.php
- Add EncryptionException import in security_helper.php (removed FQN)
- Use camelCase variables: $smtpPass, $emailConfig, $batchSaveData in affected files
- Remove unnecessary inline comments (code is self-documenting)
- Keep necessary docstrings for public API documentation
Address remaining CodeRabbit review comments
- Fix decryptValue() to use explicit null/empty check instead of empty()
(handles string "0" correctly)
- Guard checkEncryption() result in migration before proceeding
- Check read success before writing backup restoration
- Consistent DIRECTORY_SEPARATOR usage in paths
GitHub-Issue: #4554
- Fix typo 'mayl' to 'may' in Calendar.php for lo, ka, ml, nb locales
- Improve Spanish translation in Items.php for csv_import_invalid_location
- Add trailing newlines to Calendar.php files (ka, ml, nb, lo) per PSR-12
Co-authored-by: Ollama <ollama@steganos.dev>
Add validation for the mailpath POST parameter to prevent command injection
attacks. The path is validated to only allow alphanumeric characters,
underscores, dashes, forward slashes, and dots.
- Required mailpath when protocol is "sendmail"
- Validates format for all non-empty mailpath values
- Blocks common injection vectors: ; | & ` $() spaces newlines
- Added mailpath_invalid translation to all 43 language files
- Simplified validation logic to avoid redundant conditions
Files changed:
- app/Controllers/Config.php: Add regex validation with protocol check
- app/Language/*/Config.php: Add mailpath_invalid error message (43 languages)
- tests/Controllers/ConfigTest.php: Unit tests for validation
* Fix business logic vulnerability allowing negative sale totals (GHSA-wv3j-pp8r-7q43)
Add server-side validation in postEditItem() to reject negative prices,
quantities, and discounts, as well as percentage discounts exceeding 100%
and fixed discounts exceeding the item total. Also block sale completion
with negative totals in non-return mode to prevent fraud/theft.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Fix: exempt return mode from negative quantity validation
Return mode legitimately stores items with negative quantities.
The quantity validation now skips the non-negative check in return mode,
consistent with the existing return mode exemption in postComplete().
Also use abs() for fixed discount comparison to handle return quantities.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Refactor: use $rules + validate() pattern per review feedback
Address review comments from jekkos on PR #4450:
1. Use CI4 $rules variable with custom non_negative_decimal validation
rule instead of manual if-checks for price/discount validation.
2. Add validation error strings to all 44 non-English language files
(English fallback values used until translations are contributed).
3. Use validate() method with $messages array for localized error
display, maintaining the existing controller pattern.
Additional improvements:
- Add non_negative_decimal rule to OSPOSRules.php (leverages
parse_decimals() for locale-aware decimal parsing)
- Preserve manual checks for business logic (return mode quantity
exemption, discount bounds via bccomp)
- Fix PHP 8.1+ compatibility: avoid passing method return to reset()
- Explicit empty discount handling for bc-math safety
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Fix: rename to nonNegativeDecimal (PSR), clear non-English translation strings
- Rename validation rule method non_negative_decimal → nonNegativeDecimal in
OSPOSRules.php and all $rules/$messages references in Sales.php (PSR naming
per @objecttothis review)
- Replace English fallback text with "" in 43 non-English language files so
CI4 falls back to the base language string; weblate will handle translations
(per @jekkos and @objecttothis agreement)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Paul <morimori-dev@github.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: objecttothis <17935339+objecttothis@users.noreply.github.com>
- Show 'Database Migration Required' heading instead of welcome message when migrations pending
- Add warning alert explaining credentials needed to authorize migration
- Change button text from 'Go' to 'Migrate' during migration flow
- Add language strings for clear migration messaging
This makes it clear to the user that they are performing a migration
authorization step (not a regular login), and will need to re-authenticate
after migrations complete.
