diff --git a/CHANGELOG.md b/CHANGELOG.md index aef9932b9..890a137b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,7 +34,7 @@ All notable changes to this project will be documented in this file. ## [Unreleased] -## [3.4.2] - 2026-09-24 +## [3.4.2] - 2026-09-30 - Fix writable folder permission check (#4270) (#4273) by @jekkos - Extended payment delete fix (#4274) by @jekkos - Upgrade github workflow (#3708) (#4280) by @jekkos @@ -262,6 +262,14 @@ All notable changes to this project will be documented in this file. - fix(i18n): swap print_delay_autoreturn number/required messages in 5 locales (#4699) by @Rayan Abdul Cader - chore(release): unified git-cliff release workflow (changelog + tag + optional bump) (#4711) by @jekkos - fix(release): push changelog/bump to master via admin PAT (GITHUB_TOKEN blocked by branch protection) by @jekkos +- docs: add 3.4.2 changelog by @github-actions[bot] +- chore: bump version to 3.4.3 by @github-actions[bot] +- fix(security): HTML-escape attribute dropdown option labels in items attributes view (#4715) by @jekkos +- fix(release): keep package-lock.json version in sync on bump (#4718) by @jekkos +- fix(security): strip all HTML tags from $.notify alert messages (#4716) by @jekkos +- chore: strip advisory IDs from code comments and changelog (#4720) by @jekkos +- fix(security): report unwritable .env.lock, make throttle limits configurable (#4714) by @jekkos +- chore: reset 3.4.2 (undo premature 3.4.3 bump + stale changelog) for re-cut by @jekkos ## [3.4.1] - 2025-06-05 - Feature: PSR-12 Compliant Indentation by @objecttothis in ([#4196](https://github.com/opensourcepos/opensourcepos/pull/4196)) diff --git a/app/Config/App.php b/app/Config/App.php index 69fabf65d..de204b036 100644 --- a/app/Config/App.php +++ b/app/Config/App.php @@ -307,7 +307,7 @@ class App extends BaseConfig /** * Validates and returns a trusted hostname. * - * Security: Prevents Host Header Injection attacks (GHSA-jchf-7hr6-h4f3) + * Security: Prevents Host Header Injection attacks * by validating the HTTP_HOST against a whitelist of allowed hostnames. * * In production: Fails fast if allowedHostnames is not configured. diff --git a/app/Config/Encryption.php b/app/Config/Encryption.php index 9147e1031..310a5a174 100644 --- a/app/Config/Encryption.php +++ b/app/Config/Encryption.php @@ -112,8 +112,47 @@ class Encryption extends BaseConfig parent::__construct(); if ($this->key === '') { - $envKey = getenv('ENCRYPTION_KEY'); - $this->key = $envKey === false ? '' : $envKey; + // Fallback sources (notably the ENCRYPTION_KEY Docker var, which the + // parent never reads) were not decode-parsed, so run them through + // the same parser to keep hex2bin:/base64: keys consistent. + $this->key = self::parseKey(self::resolveKey( + (string) ($_SERVER['encryption.key'] ?? ''), + (string) ($_ENV['encryption.key'] ?? ''), + (string) getenv('encryption.key'), + (string) getenv('ENCRYPTION_KEY'), + )); } } + + /** + * Decode a key's `hex2bin:`/`base64:` prefix, mirroring + * BaseConfig::parseEncryptionKey(); kept static so it is unit-testable. + */ + public static function parseKey(string $key): string + { + if (str_starts_with($key, 'hex2bin:')) { + return (string) hex2bin(substr($key, 8)); + } + + if (str_starts_with($key, 'base64:')) { + return (string) base64_decode(substr($key, 7), true); + } + + return $key; + } + + /** + * Return the first non-empty source (highest precedence first). Cascading + * past empty strings (vs `??`) avoids a blank value shadowing the real key. + */ + public static function resolveKey(string ...$sources): string + { + foreach ($sources as $source) { + if ($source !== '') { + return $source; + } + } + + return ''; + } } diff --git a/app/Database/Migrations/20220127000000_convertToCI4.php b/app/Database/Migrations/20220127000000_convertToCI4.php index 1f005cfe1..f860c7aaa 100644 --- a/app/Database/Migrations/20220127000000_convertToCI4.php +++ b/app/Database/Migrations/20220127000000_convertToCI4.php @@ -7,6 +7,7 @@ use CodeIgniter\Database\Exceptions\DatabaseException; use CodeIgniter\Database\Forge; use CodeIgniter\Database\Migration; use CodeIgniter\HTTP\Exceptions\RedirectException; +use RuntimeException; class ConvertToCI4 extends Migration { @@ -32,18 +33,31 @@ class ConvertToCI4 extends Migration $existingKey = (string) config('Encryption')->key; + // A valid CI4 key requires no write — just confirm it is usable. + if ($existingKey !== '' && strlen($existingKey) >= 64) { + checkEncryption(); + + return; + } + + // Every branch below writes to .env. If the runtime user cannot write + // it (e.g. Docker/Compose with a read-only .env mount), fail with an + // actionable message instead of a raw fopen() error deep in the writer. + if (!envFileIsWritable()) { + log_message('critical', 'Encryption key not provisioned and .env is not writable. Run `php spark env:provision` to generate one.'); + + throw new RuntimeException(lang('Error.encryption_key_not_provisioned')); + } + if ($existingKey !== '' && strlen($existingKey) < 64) { // Old CI3-era key: decrypt, rotate, re-encrypt, persist — all under // a single .env lock (see convertCI3EncryptedData). $this->convertCI3EncryptedData($existingKey); - } elseif ($existingKey === '') { + } else { // No key at all: provision a fresh one (single atomic write), then // drop the incidental pre-write backup left behind by the rotation. rotateEncryptionKey(null); removeBackup(); - } else { - // Key already present and a valid CI4 key: confirm it is usable. - checkEncryption(); } } diff --git a/app/Filters/Throttle.php b/app/Filters/Throttle.php index 75ca63a79..ca4a558ab 100644 --- a/app/Filters/Throttle.php +++ b/app/Filters/Throttle.php @@ -8,22 +8,37 @@ use CodeIgniter\HTTP\ResponseInterface; use Config\Services; /** - * Rate limits login/migrate POST attempts, keyed by IP and by submitted - * username, to mitigate brute-force and credential-stuffing attacks - * (GHSA-hm9c-xchj-xgcp). Backed by CodeIgniter's cache-based Throttler, - * so limits are per-server (not shared across nodes on file cache). + * Rate limits login/migrate POST attempts by IP and submitted username to + * mitigate brute-force/credential-stuffing. Backed by CodeIgniter's + * cache-based Throttler, so limits are per-server (not shared on file cache). + * + * Tunable via `throttle.capacity` (default 5; 0 disables) and + * `throttle.seconds` (window, default 60) in .env. */ class Throttle implements FilterInterface { - private const CAPACITY = 5; - private const SECONDS = 60; - public function before(RequestInterface $request, $arguments = null) { if ($request->getMethod() !== 'POST') { return null; } + // Non-positive integer = explicit disable; missing/non-numeric falls + // back to the default so a typo (e.g. "five") cannot bypass lockout. + $capacity = filter_var(env('throttle.capacity'), FILTER_VALIDATE_INT); + if ($capacity === false) { + $capacity = 5; + } + if ($capacity <= 0) { + return null; + } + + $seconds = filter_var(env('throttle.seconds'), FILTER_VALIDATE_INT); + if ($seconds === false) { + $seconds = 60; + } + $seconds = max(1, $seconds); + helper('security'); $throttler = Services::throttler(); @@ -34,8 +49,8 @@ class Throttle implements FilterInterface $username = is_scalar($rawUsername) ? strtolower((string) $rawUsername) : ''; $usernameKey = $username !== '' ? 'login-user-' . hash_hmac('sha256', $username, $secret) : null; - $ipOk = $throttler->check($ipKey, self::CAPACITY, self::SECONDS); - $usernameOk = $usernameKey === null || $throttler->check($usernameKey, self::CAPACITY, self::SECONDS); + $ipOk = $throttler->check($ipKey, $capacity, $seconds); + $usernameOk = $usernameKey === null || $throttler->check($usernameKey, $capacity, $seconds); if (!$ipOk || !$usernameOk) { log_message('warning', 'Login throttled for IP {ip} (username: {username})', [ diff --git a/app/Helpers/security_helper.php b/app/Helpers/security_helper.php index 32635d2c8..3c384ce75 100644 --- a/app/Helpers/security_helper.php +++ b/app/Helpers/security_helper.php @@ -236,17 +236,38 @@ function writeNewEncryptionKey(string $configFile, string $key, string $oldKey): /** * Returns true when the current process can write to (or create) .env. * - * A missing .env file is considered writable when the directory is writable. + * The write path (temp file + lock, then rename) needs write permission on + * the DIRECTORY, not on .env itself — a bind-mounted .env can be writable + * while the dir (or a root-owned .env.lock) is not, so .env's own mode is not + * a reliable signal. * * @return bool */ function envFileIsWritable(): bool { $configPath = config('SecurityEnv')->envPath; + $lockPath = config('SecurityEnv')->lockPath; + $dir = dirname($configPath); - return file_exists($configPath) - ? is_writable($configPath) - : is_writable(dirname($configPath)); + if (!is_writable($dir)) { + return false; + } + + // Windows-only: rename() can't replace a read-only destination, so an + // existing .env must itself be writable (POSIX rename() can, if the dir is). + if (PHP_OS_FAMILY === 'Windows' + && file_exists($configPath) + && !is_writable($configPath) + ) { + return false; + } + + // An existing mutex file (e.g. left by a prior root env:provision) must be writable. + if (file_exists($lockPath) && !is_writable($lockPath)) { + return false; + } + + return true; } /** diff --git a/app/Models/Item.php b/app/Models/Item.php index 1268839a3..f8cd99175 100644 --- a/app/Models/Item.php +++ b/app/Models/Item.php @@ -529,7 +529,7 @@ class Item extends Model */ public function updateMultiple(array $itemData, string $itemIds): bool { - // Query Builder bypasses $allowedFields, so the whitelist is enforced here (GHSA-49mq-h2g4-grr9) + // Query Builder bypasses $allowedFields, so the whitelist is enforced here $itemData = array_intersect_key($itemData, array_flip(self::ALLOWED_BULK_EDIT_FIELDS)); if (empty($itemData)) { diff --git a/tests/Config/EncryptionTest.php b/tests/Config/EncryptionTest.php new file mode 100644 index 000000000..071e04519 --- /dev/null +++ b/tests/Config/EncryptionTest.php @@ -0,0 +1,54 @@ +assertSame( + 'server-key', + Encryption::resolveKey('server-key', 'env-key', 'getenv-key', 'docker-key') + ); + } + + public function testEmptyStringDoesNotShadowLaterSource(): void + { + // A `??`-based lookup would stop at a higher-precedence empty string; + // the cascade must skip empties and reach the real key. + $this->assertSame( + 'getenv-key', + Encryption::resolveKey('', '', 'getenv-key', 'docker-key') + ); + + $this->assertSame( + 'docker-key', + Encryption::resolveKey('', '', '', 'docker-key') + ); + } + + public function testAllEmptySourcesReturnEmptyString(): void + { + $this->assertSame('', Encryption::resolveKey('', '', '', '')); + } + + public function testPrefixedFallbackKeyIsDecoded(): void + { + // Fallback-selected keys (notably ENCRYPTION_KEY, which BaseConfig + // never reads) must be decoded like BaseConfig does, so a + // hex2bin:/base64:-prefixed value is not stored verbatim. + $this->assertSame("\xab\xcd", Encryption::parseKey('hex2bin:abcd')); + $this->assertSame("\x68\x65\x6c\x6c\x6f", Encryption::parseKey('base64:aGVsbG8=')); + + // No prefix / empty value passes through unchanged. + $this->assertSame('plain-key', Encryption::parseKey('plain-key')); + $this->assertSame('', Encryption::parseKey('')); + } +} diff --git a/tests/Controllers/HomeTest.php b/tests/Controllers/HomeTest.php index d8821c37a..d10c8c650 100644 --- a/tests/Controllers/HomeTest.php +++ b/tests/Controllers/HomeTest.php @@ -303,7 +303,7 @@ class HomeTest extends CIUnitTestCase /** * Test non-admin cannot view admin password change form - * BOLA vulnerability fix: GHSA-q58g-gg7v-f9rf + * BOLA vulnerability fix. * * @return void */ @@ -319,7 +319,7 @@ class HomeTest extends CIUnitTestCase /** * Test non-admin cannot change admin password - * BOLA vulnerability fix: GHSA-q58g-gg7v-f9rf + * BOLA vulnerability fix. * * @return void */ @@ -448,7 +448,7 @@ class HomeTest extends CIUnitTestCase /** * Test non-admin cannot view another non-admin's password form - * IDOR vulnerability fix: GHSA-mcc2-8rp2-q6ch + * IDOR vulnerability fix. * * @return void */ @@ -470,7 +470,7 @@ class HomeTest extends CIUnitTestCase /** * Test non-admin cannot change another non-admin's password - * IDOR vulnerability fix: GHSA-mcc2-8rp2-q6ch + * IDOR vulnerability fix. * * @return void */ @@ -503,7 +503,7 @@ class HomeTest extends CIUnitTestCase } /** - * Regression test for GHSA-9gr6-4mm4-4wrq: Home::__construct() previously + * Regression test: Home::__construct() previously * read the raw (single-decoded) URI segment to decide whether to skip * Secure_Controller's module-grant check for 'logout'. A route whose * double-decoded method name resolves to 'logout' must still be treated diff --git a/tests/Controllers/ItemKitsControllerTest.php b/tests/Controllers/ItemKitsControllerTest.php index 566516c40..cfdfca983 100644 --- a/tests/Controllers/ItemKitsControllerTest.php +++ b/tests/Controllers/ItemKitsControllerTest.php @@ -111,7 +111,7 @@ class ItemKitsControllerTest extends CIUnitTestCase $itemKitId = $this->createItemKit(); $this->loginAsAdmin(); - // URL-encoded three times (GHSA-3vpv-jqr3-7256 PoC). + // URL-encoded three times. // The framework's router decodes this twice before routing; the controller used to apply // a third urldecode(), turning the remaining %3C.../%3E into a live tag. // With that urldecode() removed, the value must stay percent-encoded text and never diff --git a/tests/Controllers/ItemsControllerTest.php b/tests/Controllers/ItemsControllerTest.php index 895f87868..1f73c73e5 100644 --- a/tests/Controllers/ItemsControllerTest.php +++ b/tests/Controllers/ItemsControllerTest.php @@ -103,7 +103,7 @@ class ItemsControllerTest extends CIUnitTestCase } /** - * Regression test for GHSA-92cx-fc8x-7wmm: `tax_names[]` containing `<`/`>` + * Regression test: `tax_names[]` containing `<`/`>` * (the stored-XSS vector) must be rejected by postSave. */ public function testPostSaveRejectsMaliciousTaxName(): void @@ -190,7 +190,7 @@ class ItemsControllerTest extends CIUnitTestCase } /** - * Regression test for GHSA-cm7j-957q-8pgg: an attribute definition whose + * Regression test: an attribute definition whose * `definition_name` contains HTML must be entity-escaped when rendered in the * items attributes dropdown, not emitted as a live (executable) tag. */ diff --git a/tests/Controllers/LoginTest.php b/tests/Controllers/LoginTest.php index 8378f98e5..8d75c3134 100644 --- a/tests/Controllers/LoginTest.php +++ b/tests/Controllers/LoginTest.php @@ -9,7 +9,7 @@ use CodeIgniter\Test\FeatureTestTrait; /** * Test suite for the Login controller, including the CI Throttler - * mitigation for brute-force/credential-stuffing (GHSA-hm9c-xchj-xgcp). + * mitigation for brute-force/credential-stuffing. */ class LoginTest extends CIUnitTestCase { diff --git a/tests/Controllers/ReportsControllerTest.php b/tests/Controllers/ReportsControllerTest.php index 7960f508d..c2f59979c 100644 --- a/tests/Controllers/ReportsControllerTest.php +++ b/tests/Controllers/ReportsControllerTest.php @@ -10,7 +10,7 @@ use App\Models\Employee; use Config\OSPOS; /** - * Regression tests for GHSA-9gr6-4mm4-4wrq + * Regression tests for the reports permission bypass * * Reports::__construct() previously derived the report method name from * $request->getUri()->getSegment(2), which CodeIgniter decodes once, while diff --git a/tests/Controllers/SalesControllerTest.php b/tests/Controllers/SalesControllerTest.php index d0b5dec6e..08e643c1a 100644 --- a/tests/Controllers/SalesControllerTest.php +++ b/tests/Controllers/SalesControllerTest.php @@ -14,7 +14,7 @@ use Tests\Support\EmployeeFixtureTrait; use Tests\Support\SaleFixtureTrait; /** - * Regression tests for GHSA-3xf6-8fmq-44wg. + * Regression tests for the Sales per-endpoint access-control bypass. * * A cashier holding only the base "sales" grant (no "reports_sales") must * not be able to reach the per-sale endpoints that getManage() gates diff --git a/tests/Filters/ThrottleTest.php b/tests/Filters/ThrottleTest.php index 997804430..0edf51926 100644 --- a/tests/Filters/ThrottleTest.php +++ b/tests/Filters/ThrottleTest.php @@ -161,4 +161,102 @@ class ThrottleTest extends CIUnitTestCase $this->assertNotNull($result); $this->assertSame(429, $result->getStatusCode()); } + + public function testCustomCapacityIsHonored(): void + { + $ip = '203.0.113.7'; + $prev = $this->captureEnv('throttle.capacity'); + + $this->putEnv('throttle.capacity', '2'); + + try { + $this->assertNull($this->filter->before($this->makeRequest('POST', $ip, 'c1'))); + $this->assertNull($this->filter->before($this->makeRequest('POST', $ip, 'c2'))); + + $result = $this->filter->before($this->makeRequest('POST', $ip, 'c3')); + + $this->assertNotNull($result); + $this->assertSame(429, $result->getStatusCode()); + } finally { + $this->restoreEnv('throttle.capacity', $prev); + } + } + + public function testZeroCapacityDisablesThrottling(): void + { + $ip = '203.0.113.8'; + $prev = $this->captureEnv('throttle.capacity'); + + $this->putEnv('throttle.capacity', '0'); + + try { + for ($i = 0; $i < 10; $i++) { + $result = $this->filter->before($this->makeRequest('POST', $ip, "z{$i}")); + $this->assertNull($result, "Attempt {$i} should not be throttled when disabled"); + } + } finally { + $this->restoreEnv('throttle.capacity', $prev); + } + } + + public function testInvalidCapacityFallsBackToDefault(): void + { + // A non-numeric value must fall back to the default (5), not disable. + $ip = '203.0.113.9'; + $prev = $this->captureEnv('throttle.capacity'); + + $this->putEnv('throttle.capacity', 'five'); + + try { + for ($i = 0; $i < 5; $i++) { + $this->assertNull($this->filter->before($this->makeRequest('POST', $ip, "v{$i}"))); + } + + // 6th attempt exceeds the default capacity of 5. + $result = $this->filter->before($this->makeRequest('POST', $ip, 'v6')); + $this->assertNotNull($result); + $this->assertSame(429, $result->getStatusCode()); + } finally { + $this->restoreEnv('throttle.capacity', $prev); + } + } + + private function captureEnv(string $key): array + { + return [ + 'putenv' => getenv($key), + 'hasENV' => array_key_exists($key, $_ENV), + 'ENV' => $_ENV[$key] ?? null, + 'hasSRV' => array_key_exists($key, $_SERVER), + 'SERVER' => $_SERVER[$key] ?? null, + ]; + } + + private function putEnv(string $key, string $value): void + { + putenv("{$key}={$value}"); + $_ENV[$key] = $value; + $_SERVER[$key] = $value; + } + + private function restoreEnv(string $key, array $prev): void + { + if ($prev['putenv'] === false) { + putenv($key); + } else { + putenv("{$key}={$prev['putenv']}"); + } + + if ($prev['hasENV']) { + $_ENV[$key] = $prev['ENV']; + } else { + unset($_ENV[$key]); + } + + if ($prev['hasSRV']) { + $_SERVER[$key] = $prev['SERVER']; + } else { + unset($_SERVER[$key]); + } + } } diff --git a/tests/Models/CustomerRewardPointsTest.php b/tests/Models/CustomerRewardPointsTest.php index 7671a1d8d..7e89a29d4 100644 --- a/tests/Models/CustomerRewardPointsTest.php +++ b/tests/Models/CustomerRewardPointsTest.php @@ -9,7 +9,7 @@ use Config\Database; use Tests\Support\ConcurrentDbRaceTrait; /** - * Regression tests for GHSA-995p-52qw-5hh2: adjustRewardPoints() must apply + * Regression tests: adjustRewardPoints() must apply * its balance check and its write in a single atomic UPDATE, so that two * concurrent reward-point spends against the same customer can never both * read the same stale balance and double-spend it. diff --git a/tests/Models/GiftcardTest.php b/tests/Models/GiftcardTest.php index e57b88cd9..d097cd673 100644 --- a/tests/Models/GiftcardTest.php +++ b/tests/Models/GiftcardTest.php @@ -9,7 +9,7 @@ use Config\Database; use Tests\Support\ConcurrentDbRaceTrait; /** - * Regression tests for GHSA-995p-52qw-5hh2: decrementGiftcardValue() must + * Regression tests: decrementGiftcardValue() must * apply its balance check and its write in a single atomic UPDATE, so that * two concurrent decrements against the same gift card can never both read * the same stale balance and double-spend it. diff --git a/tests/Models/ItemBulkUpdateTest.php b/tests/Models/ItemBulkUpdateTest.php index a48e2d547..4e1cdcb5a 100644 --- a/tests/Models/ItemBulkUpdateTest.php +++ b/tests/Models/ItemBulkUpdateTest.php @@ -7,7 +7,7 @@ use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\DatabaseTestTrait; /** - * Regression coverage for GHSA-49mq-h2g4-grr9 (mass assignment in bulk edit). + * Regression coverage for mass assignment in bulk edit. * * Item::update_multiple() writes through the Query Builder, which bypasses the * model's $allowedFields, so these assertions go straight to the items table. diff --git a/tests/Models/ItemQuantityTest.php b/tests/Models/ItemQuantityTest.php index b564fb097..8ca38a267 100644 --- a/tests/Models/ItemQuantityTest.php +++ b/tests/Models/ItemQuantityTest.php @@ -10,7 +10,7 @@ use Tests\Support\ConcurrentDbRaceTrait; use Tests\Support\ItemFixtureTrait; /** - * Regression tests for GHSA-995p-52qw-5hh2: changeQuantity() must apply + * Regression tests: changeQuantity() must apply * its write in a single atomic upsert, so that two concurrent sales of the * same item/location can never both read the same stale quantity and * oversell stock. Unlike the gift card and reward point spends, there is diff --git a/tests/Models/ReceivingTest.php b/tests/Models/ReceivingTest.php index 106d9e3a3..318727b09 100644 --- a/tests/Models/ReceivingTest.php +++ b/tests/Models/ReceivingTest.php @@ -9,7 +9,7 @@ use Tests\Support\EmployeeFixtureTrait; use Tests\Support\ItemFixtureTrait; /** - * Regression tests for GHSA-995p-52qw-5hh2: Receiving::delete_value() must + * Regression tests: Receiving::delete_value() must * correctly reverse the stock quantity change it applied via * Item_quantity::changeQuantity(), using the same atomic upsert as the * sale-checkout and sale-cancel paths. diff --git a/tests/Models/SaleTest.php b/tests/Models/SaleTest.php index 4dffad438..aff3b79c5 100644 --- a/tests/Models/SaleTest.php +++ b/tests/Models/SaleTest.php @@ -10,7 +10,7 @@ use Tests\Support\EmployeeFixtureTrait; use Tests\Support\ItemFixtureTrait; /** - * Regression tests for GHSA-995p-52qw-5hh2: Sale::save_value() must reject + * Regression tests: Sale::save_value() must reject * (and roll back) a payment that would overdraw a gift card or a customer's * reward points, instead of silently applying a stale/negative balance. */ diff --git a/tests/helpers/security_helperTest.php b/tests/helpers/security_helperTest.php index 7d18282e7..42a9d7699 100644 --- a/tests/helpers/security_helperTest.php +++ b/tests/helpers/security_helperTest.php @@ -301,14 +301,31 @@ class security_helperTest extends CIUnitTestCase $this->assertTrue(envFileIsWritable()); } - public function testEnvFileIsWritableReturnsFalseWhenFileIsReadonly(): void + public function testEnvFileIsWritableReturnsFalseWhenLockFileIsReadOnly(): void { + $this->skipIfRoot(); file_put_contents($this->envPath, "# tmp\n"); - chmod($this->envPath, 0444); + file_put_contents($this->lockPath, ""); + chmod($this->lockPath, 0444); - $this->assertFalse(envFileIsWritable()); + try { + $this->assertFalse(envFileIsWritable()); + } finally { + @unlink($this->lockPath); + } + } - chmod($this->envPath, 0644); + public function testEnvFileIsWritableReturnsFalseWhenDirectoryIsNotWritable(): void + { + $this->skipIfRoot(); + file_put_contents($this->envPath, "# tmp\n"); + chmod($this->sandbox, 0500); + + try { + $this->assertFalse(envFileIsWritable()); + } finally { + chmod($this->sandbox, 0700); + } } // -- checkEncryption() -- @@ -340,16 +357,18 @@ class security_helperTest extends CIUnitTestCase public function testCheckEncryptionThrowsWhenKeyEmptyAndEnvNotWritable(): void { + $this->skipIfRoot(); config('Encryption')->key = ''; file_put_contents($this->envPath, "encryption.key=''\n"); - chmod($this->envPath, 0444); + file_put_contents($this->lockPath, ""); + chmod($this->lockPath, 0444); try { $this->expectException(RuntimeException::class); $this->expectExceptionMessage('provisioned'); checkEncryption(); } finally { - chmod($this->envPath, 0644); + @unlink($this->lockPath); } } @@ -392,11 +411,9 @@ class security_helperTest extends CIUnitTestCase public function testCheckEncryptionRollsBackWhenSaveAllFails(): void { - // Regression guard (thread #2): if the post-rotation saveAll() fails, - // the freshly rotated .env key must be restored from the backup so the - // original CI3 ciphertext stays decryptable. A failing fake Appconfig - // (injected via CI3SecretConverter) forces saveAll() to throw without - // a real database. + // If the post-rotation saveAll() fails, the rotated key must be rolled + // back so the original CI3 ciphertext stays decryptable. A failing fake + // Appconfig (injected via CI3SecretConverter) makes saveAll() throw. $oldKey = bin2hex(random_bytes(16)); // < 64 chars -> CI3 era $plaintext = ['smtp_pass' => 'keep-me-safe']; $ciphertext = array_map(fn ($v) => $this->ci3Encrypt($v, $oldKey), $plaintext); @@ -453,17 +470,19 @@ class security_helperTest extends CIUnitTestCase public function testCheckThrottleEncryptionThrowsWhenKeyMissingAndEnvNotWritable(): void { + $this->skipIfRoot(); putenv('throttle.key'); unset($_ENV['throttle.key'], $_SERVER['throttle.key']); file_put_contents($this->envPath, "encryption.key='abc'\n"); - chmod($this->envPath, 0444); + file_put_contents($this->lockPath, ""); + chmod($this->lockPath, 0444); try { $this->expectException(RuntimeException::class); $this->expectExceptionMessage('provisioned'); checkThrottleEncryption(); } finally { - chmod($this->envPath, 0644); + @unlink($this->lockPath); } } @@ -657,4 +676,22 @@ class security_helperTest extends CIUnitTestCase $this->assertFileDoesNotExist($this->backupPath); } + + /** + * When PHPUnit runs as root, is_writable() reports 0444 files as writable + * (and root bypasses directory permission bits), so the "not writable" + * fixtures cannot be faked reliably. Skip those tests under root. + */ + private function skipIfRoot(): void + { + $isRoot = false; + if (function_exists('posix_geteuid')) { + $isRoot = posix_geteuid() === 0; + } elseif (function_exists('get_current_user')) { + $isRoot = in_array(get_current_user(), ['root', '0'], true); + } + if ($isRoot) { + $this->markTestSkipped('is_writable() is bypassed when running as root; cannot fake a non-writable fixture'); + } + } }