Fix encryption key persistence for Docker environments (Issue #4554) - #4556
Open
jekkos wants to merge 2 commits into
Open
Fix encryption key persistence for Docker environments (Issue #4554)#4556jekkos wants to merge 2 commits into
jekkos wants to merge 2 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two-tier encryption key persistence (.env primary, writable JSON fallback); Config loads fallback when key missing; adds guarded encryptValue()/decryptValue() helpers; updates controllers, libraries, and migration to use these helpers; save endpoints return JSON failure when encryption fails. ChangesEncryption key initialization and credential helpers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Config/Encryption.php`:
- Around line 147-152: loadKeyFromWritable() currently returns any non-empty
$data['key'], causing malformed fallback keys to be accepted; change it to fully
validate the key before returning by verifying the key field is a string,
decodes from base64 (or the expected encoding) and matches the required byte
length/format for the encrypter used, and if validation fails return null so the
constructor/encrypter initialization will ignore the bad fallback; update
loadKeyFromWritable(), and any call sites in the constructor that rely on its
result, to only install/assign a key when validation passes.
In `@app/Controllers/Config.php`:
- Around line 516-530: The code currently falls back to saving secrets (SMTP
password, msg_pwd, mailchimp_api_key, mailchimp_list_id) in plaintext when
$this->encrypter is null or check_encryption() is false; update the branches in
the SMTP block and the similar blocks handling msg_pwd and Mailchimp
(references: $passwordInput/$password, $msg_pwd, $mailchimp_api_key,
$mailchimp_list_id, $this->encrypter, check_encryption()) to refuse to store
secrets unencrypted — log an error and return a JSON failure response (same
pattern used in the encrypt() catch) instead of assigning the plaintext value so
no secret is silently downgraded to plaintext. Ensure the response messages use
lang('Config.encryption_failed') or an appropriate message and keep existing
try/catch encryption logic intact.
In `@app/Helpers/security_helper.php`:
- Around line 33-39: The current flow only writes the restart-safe copy via
write_encryption_key_to_writable($key, $old_key) when
write_encryption_key_to_env(...) fails; change this so the writable copy is
always created/mirrored regardless of whether the .env write succeeded.
Specifically, after calling write_encryption_key_to_env($key, $old_key) (and
handling its success/failure as before), always invoke
write_encryption_key_to_writable($key, $old_key) to ensure
WRITEPATH/config/encryption.key is created or updated; keep existing error
handling/logging around both write_encryption_key_to_env and
write_encryption_key_to_writable so failures are reported but the mirror is
still attempted.
In `@app/Libraries/Email_lib.php`:
- Around line 31-40: The code currently bypasses decryption when
check_encryption() is false and may forward ciphertext as SMTPPass; update the
Email_lib constructor logic so that when $smtp_pass is non-empty and
check_encryption() returns false you treat this as a decryption failure: log an
error (including context) and clear $smtp_pass (set to empty) instead of passing
the ciphertext through; modify the block around check_encryption(),
Services::encrypter(), and the EncryptionException handling to add an else
branch that logs and zeroes $smtp_pass so SMTP auth cannot proceed with
ciphertext.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd385989-2772-47c0-9e73-72b969e5f984
📒 Files selected for processing (5)
app/Config/Encryption.phpapp/Controllers/Config.phpapp/Helpers/security_helper.phpapp/Language/en/Config.phpapp/Libraries/Email_lib.php
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Helpers/security_helper.php (1)
79-81:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLogic error: condition is inverted due to operator precedence.
The
!operator binds tighter than!==, so this evaluates as(!@file_put_contents(...)) !== false. Whenfile_put_contentssucceeds and returns bytes written (e.g., 27),!27isfalse, andfalse !== falseisfalse—so the log is skipped on success. When it fails and returnsfalse,!falseistrue, andtrue !== falseistrue—so the log triggers on failure. However, there's also a stray!that inverts the entire intended behavior.Proposed fix
- if (!`@file_put_contents`($config_path, "# OSPOS Configuration\n\n") !== false) { + if (`@file_put_contents`($config_path, "# OSPOS Configuration\n\n") === false) { log_message('debug', 'Could not create .env file'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Helpers/security_helper.php` around lines 79 - 81, The condition around the file creation is inverted due to the stray '!' and operator precedence; update the check on `@file_put_contents` so it explicitly tests for failure (=== false) rather than negating the call—i.e., remove the leading '!' and change the if to test if `@file_put_contents`($config_path, "...") === false, then call log_message('debug', 'Could not create .env file'); reference the symbols file_put_contents, $config_path, and log_message in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/Helpers/security_helper.php`:
- Around line 79-81: The condition around the file creation is inverted due to
the stray '!' and operator precedence; update the check on `@file_put_contents` so
it explicitly tests for failure (=== false) rather than negating the call—i.e.,
remove the leading '!' and change the if to test if
`@file_put_contents`($config_path, "...") === false, then call
log_message('debug', 'Could not create .env file'); reference the symbols
file_put_contents, $config_path, and log_message in your change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6335108-824d-405a-bc58-36882d1e294e
📒 Files selected for processing (6)
app/Controllers/Config.phpapp/Controllers/Customers.phpapp/Helpers/security_helper.phpapp/Libraries/Email_lib.phpapp/Libraries/Mailchimp_lib.phpapp/Libraries/Sms_lib.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/Helpers/security_helper.php (1)
33-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways mirror the key into writable storage.
A successful
.envwrite still skipsWRITEPATH/config/encryption.key, so containerized installs can lose the key on restart even though this request succeeded.Suggested change
- $persisted = write_encryption_key_to_env($key, $old_key); - - // Attempt 2: WRITEPATH/config/encryption.key (Docker/container fallback) - if (!$persisted) { - $persisted = write_encryption_key_to_writable($key, $old_key); - } + $envPersisted = write_encryption_key_to_env($key, $old_key); + $writablePersisted = write_encryption_key_to_writable($key, $old_key); + $persisted = $envPersisted || $writablePersisted;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Helpers/security_helper.php` around lines 33 - 39, Currently the code stops after a successful write_encryption_key_to_env($key, $old_key) and skips write_encryption_key_to_writable($key, $old_key), which means containerized installs may not persist the key; change the flow so that after write_encryption_key_to_env returns true you still call write_encryption_key_to_writable($key, $old_key) (and handle/log its result) so the key is mirrored into WRITEPATH/config/encryption.key regardless of .env success; reference the functions write_encryption_key_to_env and write_encryption_key_to_writable and ensure any errors from the writable write are captured or logged but do not prevent the request from succeeding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Config/Encryption.php`:
- Around line 147-152: Guard access to $data['key'] before using it: update the
validation in the Encryption.php logic that currently checks
is_string($data['key']) and strlen($data['key']) to first verify
isset($data['key']) (or array_key_exists) so you don't read an undefined array
key; in other words, change the condition around $data (used in the method
handling json_decode($content)) to require isset($data['key']) &&
is_string($data['key']) && strlen($data['key']) >= 64 so the code safely
validates the key.
In `@app/Helpers/security_helper.php`:
- Around line 305-313: encrypt_value() wrongly treats the string "0" as empty
because it uses empty($value); change the guard to only treat null or
empty-string as missing (e.g. replace if (empty($value)) with if ($value ===
null || $value === '') ), so valid values like "0" are processed/returned
normally; keep the later check_encryption() behavior and the existing return
semantics for $require unchanged.
---
Duplicate comments:
In `@app/Helpers/security_helper.php`:
- Around line 33-39: Currently the code stops after a successful
write_encryption_key_to_env($key, $old_key) and skips
write_encryption_key_to_writable($key, $old_key), which means containerized
installs may not persist the key; change the flow so that after
write_encryption_key_to_env returns true you still call
write_encryption_key_to_writable($key, $old_key) (and handle/log its result) so
the key is mirrored into WRITEPATH/config/encryption.key regardless of .env
success; reference the functions write_encryption_key_to_env and
write_encryption_key_to_writable and ensure any errors from the writable write
are captured or logged but do not prevent the request from succeeding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: deec40e9-5707-4f77-a5a5-8ed2724768f0
📒 Files selected for processing (2)
app/Config/Encryption.phpapp/Helpers/security_helper.php
objecttothis
requested changes
May 22, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/Helpers/security_helper.php (3)
254-272: 💤 Low valueInconsistent empty check pattern with
encryptValue().
encryptValue()at line 288 uses$value === null || $value === ''(per past review feedback aboutempty("0")), butdecryptValue()still usesempty($encryptedValue). While encrypted values are base64 and won't be"0", using the same pattern improves consistency.♻️ Proposed fix for consistency
function decryptValue(?string $encryptedValue, string $default = ''): string { - if (empty($encryptedValue)) { + if ($encryptedValue === null || $encryptedValue === '') { return $default; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Helpers/security_helper.php` around lines 254 - 272, decryptValue uses empty($encryptedValue) which is inconsistent with encryptValue's null/empty-string check and can mis-handle values like "0"; update decryptValue to check for $encryptedValue === null || $encryptedValue === '' instead of empty($encryptedValue), keeping the rest of the logic (checkEncryption(), Services::encrypter(), EncryptionException handling and log_message calls) unchanged so behavior remains consistent with encryptValue.
69-71: ⚡ Quick winConfusing negation logic—simplify the condition.
The
!@file_put_contents(...) !== falseconstruct works by accident but is difficult to reason about. The intent is to log when the write fails.♻️ Proposed fix
- if (!`@file_put_contents`($configPath, "# OSPOS Configuration\n\n") !== false) { + if (`@file_put_contents`($configPath, "# OSPOS Configuration\n\n") === false) { log_message('debug', 'Could not create .env file'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Helpers/security_helper.php` around lines 69 - 71, The condition around the attempted write to $configPath is using confusing negation: replace the current check that uses !`@file_put_contents`(...) !== false with a direct comparison against false so failures are explicit; call `@file_put_contents`($configPath, ...) and if that expression === false then invoke log_message('debug', 'Could not create .env file'). This simplifies the logic and correctly detects write failures for the file_put_contents call in security_helper.php.
234-241: 💤 Low valueInconsistent path separator.
Uses
/directly while other functions in this file useDIRECTORY_SEPARATOR.♻️ Proposed fix
function removeBackup(): void { - $backupPath = WRITEPATH . '/backup/.env.bak'; + $backupPath = WRITEPATH . 'backup' . DIRECTORY_SEPARATOR . '.env.bak'; if (file_exists($backupPath)) { unlink($backupPath); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Helpers/security_helper.php` around lines 234 - 241, The removeBackup function builds $backupPath using a hardcoded '/' which is inconsistent; change it to build the path using DIRECTORY_SEPARATOR (e.g. compose WRITEPATH, 'backup' and '.env.bak' with DIRECTORY_SEPARATOR) so $backupPath uses the platform-safe separator, keeping the existing file_exists and unlink logic intact and updating the $backupPath variable in removeBackup().app/Controllers/Customers.php (1)
13-13: 💤 Low valueRemove unused
Config\Servicesimport.The
Servicesclass is no longer used after refactoring decryption to use thedecryptValue()helper function.Suggested fix
use CodeIgniter\HTTP\DownloadResponse; use CodeIgniter\HTTP\ResponseInterface; use Config\OSPOS; -use Config\Services; use stdClass;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Controllers/Customers.php` at line 13, Remove the unused import of Config\Services from the Customers controller: delete the "use Config\Services;" statement since decryption now uses the decryptValue() helper; ensure any references to Services in this file are also removed or replaced (look for the Customers class and any methods previously calling Services) so there are no leftover unused imports or undefined references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Database/Migrations/20220127000000_convert_to_ci4.php`:
- Around line 36-40: check the boolean return value of checkEncryption()
wherever it's called in the migration flow (the calls just before removeBackup()
and the later call around the encryption step) and fail fast if it returns
false: do not proceed with removeBackup() or any further migration/encryption
steps — instead log the error and abort the process by throwing an exception or
returning a failure/exit status so migration stops on persistence failure.
In `@app/Helpers/security_helper.php`:
- Around line 214-227: In abortEncryptionConversion, guard against
file_get_contents($backupPath) returning false before calling file_put_contents
to avoid writing an empty .env: check the result into a variable, log and return
on failure, and only proceed to chmod and file_put_contents when the read
succeeded; also normalize $backupPath construction to use DIRECTORY_SEPARATOR
instead of a hard-coded '/' to match other functions and avoid cross-platform
issues (refer to symbols abortEncryptionConversion, $backupPath, $configPath,
file_get_contents, file_put_contents, chmod, log_message).
---
Nitpick comments:
In `@app/Controllers/Customers.php`:
- Line 13: Remove the unused import of Config\Services from the Customers
controller: delete the "use Config\Services;" statement since decryption now
uses the decryptValue() helper; ensure any references to Services in this file
are also removed or replaced (look for the Customers class and any methods
previously calling Services) so there are no leftover unused imports or
undefined references.
In `@app/Helpers/security_helper.php`:
- Around line 254-272: decryptValue uses empty($encryptedValue) which is
inconsistent with encryptValue's null/empty-string check and can mis-handle
values like "0"; update decryptValue to check for $encryptedValue === null ||
$encryptedValue === '' instead of empty($encryptedValue), keeping the rest of
the logic (checkEncryption(), Services::encrypter(), EncryptionException
handling and log_message calls) unchanged so behavior remains consistent with
encryptValue.
- Around line 69-71: The condition around the attempted write to $configPath is
using confusing negation: replace the current check that uses
!`@file_put_contents`(...) !== false with a direct comparison against false so
failures are explicit; call `@file_put_contents`($configPath, ...) and if that
expression === false then invoke log_message('debug', 'Could not create .env
file'). This simplifies the logic and correctly detects write failures for the
file_put_contents call in security_helper.php.
- Around line 234-241: The removeBackup function builds $backupPath using a
hardcoded '/' which is inconsistent; change it to build the path using
DIRECTORY_SEPARATOR (e.g. compose WRITEPATH, 'backup' and '.env.bak' with
DIRECTORY_SEPARATOR) so $backupPath uses the platform-safe separator, keeping
the existing file_exists and unlink logic intact and updating the $backupPath
variable in removeBackup().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2792a9e3-ea13-4499-a77b-3947d4267859
📒 Files selected for processing (7)
app/Controllers/Config.phpapp/Controllers/Customers.phpapp/Database/Migrations/20220127000000_convert_to_ci4.phpapp/Helpers/security_helper.phpapp/Libraries/Email_lib.phpapp/Libraries/Mailchimp_lib.phpapp/Libraries/Sms_lib.php
objecttothis
previously approved these changes
May 28, 2026
|
❌ Staging deployment failed Check the workflow logs for details. |
7 tasks
Member
|
@jekkos I approved the PR. I would look at the coderabbit comments before merging though. |
jekkos
force-pushed
the
fix/4554-encryption-docker-issue
branch
2 times, most recently
from
June 4, 2026 08:11
9c49de3 to
0b1a995
Compare
objecttothis
requested changes
Jun 4, 2026
| $config_file = substr_replace($config_file, $old_line, $insertion_point, 0); | ||
| } | ||
| } | ||
| if (strpos($configFile, 'encryption.key') !== false) { |
Member
There was a problem hiding this comment.
This needs to be updated to match the changes in #4571
if (preg_match('/^\s*encryption\.key\s*=/m', $config_file)) {
$config_file = preg_replace("/^(\s*encryption\.key\s*=\s*).*/m", "\$1'$key'", $config_file, 1);
| @chmod($config_path, 0640); | ||
| if (!empty($oldKey)) { | ||
| $oldLine = "# encryption.key = '$oldKey' REMOVE IF UNNEEDED\r\n"; | ||
| $insertionPoint = stripos($configFile, 'encryption.key'); |
Member
There was a problem hiding this comment.
This also needs to be updated to match the changes in #4571
if (preg_match('/^encryption\.key\s*=/m', $config_file, $matches, PREG_OFFSET_CAPTURE)) {
$config_file = substr_replace($config_file, $old_line, $matches[0][1], 0);
jekkos
force-pushed
the
fix/4554-encryption-docker-issue
branch
3 times, most recently
from
June 9, 2026 21:39
0a4eac5 to
48e3f94
Compare
jekkos
force-pushed
the
fix/4554-encryption-docker-issue
branch
from
June 18, 2026 18:59
48e3f94 to
46bfe2a
Compare
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
jekkos
force-pushed
the
fix/4554-encryption-docker-issue
branch
from
June 29, 2026 18:48
e17417a to
befa231
Compare
3 tasks
objecttothis
requested changes
Aug 6, 2026
| 'enable_new_look' => '', | ||
| 'enable_right_bar' => '', | ||
| 'enable_right_bar_tooltip' => '', | ||
| 'encryption_failed' => 'Failed to encrypt data. Please check encryption configuration.', |
Member
There was a problem hiding this comment.
@jekkos do we need to propagate this key to the other language files with an empty '' or will weblate pick it up and propagate for us?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4554 - Encryption key not being persisted in Docker environments, causing "forNeedsStarterKey" exceptions and inability to decrypt stored passwords after container restarts.
Problem
When running OSPOS in Docker:
/app/.envwhich is read-only or ephemeral.envto image defaultcheck_encryption()always returnedtrueeven when key persistence failedSolution
1. Fix
check_encryption()return value (security_helper.php)falsewhen key persistence fails@error suppression to properly detect write failureswritable/as persistent storage2. Add fallback key loading (Encryption.php)
3. Handle encryption unavailability (Config.php, Email_lib.php)
EncrypterInterfaceproperty to nullable4. Add language string for encryption failure
Files Changed
app/Helpers/security_helper.php- Key persistence with Docker fallbackapp/Config/Encryption.php- Fallback key loading from WRITEPATHapp/Controllers/Config.php- Graceful encryption failure handlingapp/Libraries/Email_lib.php- Check encryption before using encrypterapp/Language/en/Config.php- Error message localizationTesting
Docker Volumes
The fix uses the existing
logsDocker volume which mounts to/app/writable/:The encryption key is stored at
/app/writable/config/encryption.keywhich is in the same persistent volume.Summary by CodeRabbit
New Features
Bug Fixes