mirror of
https://github.com/opensourcepos/opensourcepos.git
synced 2026-09-22 10:45:03 -04:00
* 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>
204 lines
7.0 KiB
PHP
204 lines
7.0 KiB
PHP
<?php
|
|
|
|
namespace Tests\Libraries;
|
|
|
|
use App\Libraries\CI3SecretConverter;
|
|
use App\Models\Appconfig;
|
|
use CodeIgniter\Encryption\EncrypterInterface;
|
|
use CodeIgniter\Test\CIUnitTestCase;
|
|
use Config\Encryption as EncryptionConfig;
|
|
use Config\Services;
|
|
|
|
/**
|
|
* Tests for CI3SecretConverter.
|
|
*
|
|
* The converter performs no DB I/O of its own except saveAll(); the Appconfig
|
|
* model is injectable so we can fake get_value()/batch_save() with an anonymous
|
|
* subclass. decryptAll() is the only path that touches the real cipher, so we
|
|
* verify it by encrypting a known plaintext with the *CI3 cipher* and asserting
|
|
* the converter decrypts it back.
|
|
*/
|
|
final class CI3SecretConverterTest extends CIUnitTestCase
|
|
{
|
|
private string $oldKey;
|
|
private array $plain;
|
|
|
|
protected function setUp(): void
|
|
{
|
|
parent::setUp();
|
|
|
|
// Make sure the app Encryption config has a valid key so CI4 default
|
|
// cipher (encryptAll/verifyAll) works in this test process.
|
|
$env = config('Encryption');
|
|
if (empty($env->key) || strlen((string) $env->key) < 64) {
|
|
$env->key = bin2hex(random_bytes(32));
|
|
}
|
|
|
|
// The CI3-era key this test uses to seed fake legacy ciphertext.
|
|
$this->oldKey = bin2hex(random_bytes(16));
|
|
|
|
$this->plain = [
|
|
'clcdesq_api_key' => 'capi-1234567890abcdef',
|
|
'clcdesq_api_url' => 'https://ci3.example.org/api',
|
|
'mailchimp_api_key' => 'mc-abc-123',
|
|
'mailchimp_list_id' => 'list-5a6b7c',
|
|
'smtp_pass' => 's3cr3t-smtp',
|
|
];
|
|
}
|
|
|
|
/**
|
|
* Build the CI3 ciphertext the same way the converter decrypts it, so we
|
|
* have a known round-trip fixture without depending on a DB row.
|
|
*/
|
|
private function ci3Encrypt(string $plaintext): string
|
|
{
|
|
$cfg = new EncryptionConfig();
|
|
$cfg->driver = 'OpenSSL';
|
|
$cfg->digest = 'SHA512';
|
|
$cfg->key = $this->oldKey;
|
|
$cfg->cipher = 'AES-128-CBC';
|
|
$cfg->rawData = false;
|
|
$cfg->encryptKeyInfo = 'encryption';
|
|
$cfg->authKeyInfo = 'authentication';
|
|
$cfg->previousKeys = [];
|
|
|
|
return Services::encrypter($cfg)->encrypt($plaintext);
|
|
}
|
|
|
|
/**
|
|
* @return array<string,string>
|
|
*/
|
|
private function ci3Ciphertexts(): array
|
|
{
|
|
return array_map(
|
|
fn ($v) => $v === '' ? '' : $this->ci3Encrypt($v),
|
|
$this->plain
|
|
);
|
|
}
|
|
|
|
private function fake(array $values, bool $saveSucceeds = true): Appconfig
|
|
{
|
|
return new class($values, $saveSucceeds) extends Appconfig {
|
|
public function __construct(
|
|
private array $vals,
|
|
private bool $ok
|
|
) {
|
|
parent::__construct();
|
|
}
|
|
|
|
public function get_value(string $key, string $default = ''): string
|
|
{
|
|
return $this->vals[$key] ?? $default;
|
|
}
|
|
|
|
public function batch_save(array $data): bool
|
|
{
|
|
return $this->ok;
|
|
}
|
|
};
|
|
}
|
|
|
|
public function testDecryptAllRoundTripWithCi3Cipher(): void
|
|
{
|
|
$conv = new CI3SecretConverter($this->fake($this->ci3Ciphertexts()));
|
|
$plain = $conv->decryptAll($this->oldKey);
|
|
|
|
foreach ($this->plain as $k => $v) {
|
|
$this->assertSame($v, $plain[$k], "decryptAll mismatch for {$k}");
|
|
}
|
|
}
|
|
|
|
public function testDecryptAllEmptyRowsStayEmpty(): void
|
|
{
|
|
$ct = $this->ci3Ciphertexts();
|
|
$ct['mailchimp_api_key'] = '';
|
|
$ct['mailchimp_list_id'] = '';
|
|
|
|
$plain = (new CI3SecretConverter($this->fake($ct)))->decryptAll($this->oldKey);
|
|
|
|
$this->assertSame('capi-1234567890abcdef', $plain['clcdesq_api_key']);
|
|
$this->assertSame('', $plain['mailchimp_api_key']);
|
|
$this->assertSame('', $plain['mailchimp_list_id']);
|
|
}
|
|
|
|
public function testEncryptVerifyRoundTripWithCi4Cipher(): void
|
|
{
|
|
$conv = new CI3SecretConverter($this->fake([]));
|
|
|
|
$enc = $conv->encryptAll($this->plain);
|
|
foreach ($this->plain as $k => $v) {
|
|
$this->assertNotSame($v, $enc[$k], "encryptAll must change {$k}");
|
|
}
|
|
|
|
$this->assertSame($this->plain, $conv->verifyAll($enc));
|
|
}
|
|
|
|
public function testEncryptAllKeepsEmptyValuesEmpty(): void
|
|
{
|
|
$conv = new CI3SecretConverter($this->fake([]));
|
|
$in = $this->plain;
|
|
$in['smtp_pass'] = '';
|
|
|
|
$enc = $conv->encryptAll($in);
|
|
$this->assertSame('', $enc['smtp_pass']);
|
|
$this->assertNotSame('', $enc['clcdesq_api_key']);
|
|
}
|
|
|
|
public function testCI3BranchSavesCiphertextNotPlaintext(): void
|
|
{
|
|
// Regression guard for the env:provision CI3 branch (mirrored by the
|
|
// ConvertToCI4 migration): after decryptAll() with the legacy CI3 key,
|
|
// the command must persist encryptAll()'s CI4 *ciphertext* -- never the
|
|
// decrypted plaintext. A payload equal to $plain would mean secrets were
|
|
// written to ospos_app_config in the clear.
|
|
$conv = new CI3SecretConverter($this->fake($this->ci3Ciphertexts()));
|
|
$plain = $conv->decryptAll($this->oldKey);
|
|
$payload = $conv->encryptAll($plain); // <- exactly what run() passes to saveAll()
|
|
|
|
// saveAll() must receive the ciphertext form, i.e. a value that differs
|
|
// from every plaintext secret yet round-trips to it under the CI4 cipher.
|
|
foreach ($this->plain as $col => $secret) {
|
|
$this->assertNotSame($secret, $payload[$col], "saveAll() payload for {$col} must be ciphertext, not the plain secret");
|
|
}
|
|
$this->assertSame($plain, $conv->verifyAll($payload), 'persisted ciphertext must verify back to the original plaintext');
|
|
}
|
|
|
|
public function testHasLegacyDataTrueWhenAnyPresent(): void
|
|
{
|
|
$conv = new CI3SecretConverter($this->fake($this->ci3Ciphertexts()));
|
|
$this->assertTrue($conv->hasLegacyData($this->oldKey));
|
|
}
|
|
|
|
public function testHasLegacyDataFalseWhenAllEmpty(): void
|
|
{
|
|
$conv = new CI3SecretConverter($this->fake([]));
|
|
$this->assertFalse($conv->hasLegacyData($this->oldKey));
|
|
}
|
|
|
|
public function testSaveAllPersistsAndReturnsTrue(): void
|
|
{
|
|
$conv = new CI3SecretConverter($this->fake([], true));
|
|
$this->assertTrue($conv->saveAll(['x' => 'y']));
|
|
}
|
|
|
|
public function testSaveAllThrowsWhenModelFails(): void
|
|
{
|
|
$conv = new CI3SecretConverter($this->fake([], false));
|
|
$this->expectException(\RuntimeException::class);
|
|
$this->expectExceptionMessage('Failed to save converted encryption data');
|
|
$conv->saveAll(['x' => 'y']);
|
|
}
|
|
|
|
public function testLegacyKeysConstantExposesExpectedSet(): void
|
|
{
|
|
$expected = [
|
|
'clcdesq_api_key',
|
|
'clcdesq_api_url',
|
|
'mailchimp_api_key',
|
|
'mailchimp_list_id',
|
|
'smtp_pass',
|
|
];
|
|
$this->assertSame($expected, CI3SecretConverter::LEGACY_KEYS);
|
|
}
|
|
}
|