address reviews

Author: paulbalandan

system/Commands/Encryption/RotateKey.php

@@ -115,15 +115,15 @@ protected function execute(array $arguments, array $options): int
 
         $keep = $options['keep'];
 
-        if (! is_numeric($keep) || (int) $keep < 0) {
+        if (! is_string($keep) || ! ctype_digit($keep)) {
             CLI::error('The --keep option must be a non-negative integer.');
 
             return EXIT_ERROR;
         }
 
         $length = $options['length'];
 
-        if (! is_numeric($length) || (int) $length < 1) {
+        if (! is_string($length) || ! ctype_digit($length) || (int) $length < 1) {
             CLI::error('The --length option must be a positive integer.');
 
             return EXIT_ERROR;
@@ -136,10 +136,24 @@ protected function execute(array $arguments, array $options): int
         // key is preserved on disk).
         $envFile = ((new Paths())->envDirectory ?? ROOTPATH) . '.env'; // @phpstan-ignore nullCoalesce.property
 
+        if (! is_file($envFile)) {
+            CLI::error(sprintf('Cannot rotate: `.env` file not found at %s.', clean_path($envFile)));
+
+            return EXIT_ERROR;
+        }
+
+        if (! is_writable($envFile)) {
+            CLI::error(sprintf('Cannot rotate: `.env` file at %s is not writable.', clean_path($envFile)));
+
+            return EXIT_ERROR;
+        }
+
         if (! $this->writePreviousKeys($previousKeys, $envFile)) {
+            // @codeCoverageIgnoreStart
             CLI::error(sprintf('Failed to write `encryption.previousKeys` to %s.', clean_path($envFile)));
 
             return EXIT_ERROR;
+            // @codeCoverageIgnoreEnd
         }
 
         // Clear `encryption.previousKeys` from all env sources so the DotEnv
@@ -223,21 +237,13 @@ private function mergePreviousKeys(string $currentKey, array $existing, int $kee
 
     /**
      * Replaces or inserts the `encryption.previousKeys` line in the `.env` file.
-     * `key:generate` is responsible for the file's existence and the
-     * `encryption.key` line; this method only touches `encryption.previousKeys`.
+     * The caller is responsible for ensuring `$envFile` exists and is writable;
+     * `key:generate` handles the `encryption.key` line.
      *
      * @param list<string> $previousKeys
      */
     private function writePreviousKeys(array $previousKeys, string $envFile): bool
     {
-        if (! is_file($envFile)) {
-            return false; // @codeCoverageIgnore
-        }
-
-        if (! is_writable($envFile)) {
-            return false;
-        }
-
         $contents = (string) file_get_contents($envFile);
         $value    = implode(',', $previousKeys);
 
@@ -260,8 +266,9 @@ private function writePreviousKeys(array $previousKeys, string $envFile): bool
         );
 
         if ($injected === $contents) {
-            // Fallback: append to the end. Shouldn't trigger because `key:generate`
-            // writes the `encryption.key` line just before this method runs.
+            // Fallback: append to the end. Reachable only when `encryption.key`
+            // is set via a non-`.env` source (e.g., server config / `putenv()`),
+            // so the regex cannot find it as a line in the file.
             $injected = $contents . "\nencryption.previousKeys = {$value}"; // @codeCoverageIgnore
         }
 

tests/system/Commands/Encryption/RotateKeyTest.php

@@ -20,12 +20,15 @@
 use CodeIgniter\Test\CIUnitTestCase;
 use CodeIgniter\Test\Mock\MockInputOutput;
 use CodeIgniter\Test\StreamFilterTrait;
+use PHPUnit\Framework\Attributes\CoversClass;
 use PHPUnit\Framework\Attributes\Group;
+use PHPUnit\Framework\Attributes\RequiresOperatingSystem;
 use PHPUnit\Framework\Attributes\WithoutErrorHandler;
 
 /**
  * @internal
  */
+#[CoversClass(RotateKey::class)]
 #[Group('Others')]
 final class RotateKeyTest extends CIUnitTestCase
 {
@@ -113,6 +116,7 @@ public function testRotateMovesCurrentKeyToPreviousKeysAndGeneratesNew(): void
 
         $this->assertSame(
             <<<'EOT'
+
                 Encryption key rotated. 1 previous key retained for decryption fallback.
                 Re-encrypt existing data with the new key when ready.
 
@@ -140,6 +144,7 @@ public function testRotatePrependsToExistingPreviousKeysList(): void
 
         $this->assertSame(
             <<<'EOT'
+
                 Encryption key rotated. 3 previous keys retained for decryption fallback.
                 Re-encrypt existing data with the new key when ready.
 
@@ -396,6 +401,23 @@ public function testRotateRejectsNonNumericKeepValue(): void
         $this->assertSame(self::SEED_KEY, env('encryption.key'));
     }
 
+    public function testRotateRejectsFractionalKeepValue(): void
+    {
+        $this->seedEnv(self::SEED_KEY);
+
+        command('key:rotate --force --keep=3.5');
+
+        $this->assertSame(
+            <<<'EOT'
+
+                The --keep option must be a non-negative integer.
+
+                EOT,
+            $this->getUndecoratedBuffer(),
+        );
+        $this->assertSame(self::SEED_KEY, env('encryption.key'));
+    }
+
     public function testRotateRejectsNegativeLengthValue(): void
     {
         $this->seedEnv(self::SEED_KEY);
@@ -457,6 +479,57 @@ public function testRotateRejectsNonNumericLengthValue(): void
         $this->assertSame($envContentsBefore, (string) file_get_contents($this->envPath));
     }
 
+    public function testRotateRejectsFractionalLengthValue(): void
+    {
+        $this->seedEnv(self::SEED_KEY);
+        $envContentsBefore = (string) file_get_contents($this->envPath);
+
+        command('key:rotate --force --length=3.5');
+
+        $this->assertSame(
+            <<<'EOT'
+
+                The --length option must be a positive integer.
+
+                EOT,
+            $this->getUndecoratedBuffer(),
+        );
+        $this->assertSame(self::SEED_KEY, env('encryption.key'));
+        $this->assertSame($envContentsBefore, (string) file_get_contents($this->envPath));
+    }
+
+    public function testRotateErrorsWhenEnvFileIsMissing(): void
+    {
+        // No seedEnv() call: `.env` is absent. Populate the env var directly so
+        // the up-front `encryption.key` existence check passes.
+        putenv('encryption.key=' . self::SEED_KEY);
+        $_ENV['encryption.key'] = self::SEED_KEY;
+
+        command('key:rotate --force');
+
+        $this->assertStringContainsString('Cannot rotate: `.env` file not found at', $this->getUndecoratedBuffer());
+        $this->assertFileDoesNotExist($this->envPath, 'No `.env` file should have been created.');
+    }
+
+    #[RequiresOperatingSystem('Linux|Darwin')]
+    public function testRotateErrorsWhenEnvFileIsNotWritable(): void
+    {
+        $this->seedEnv(self::SEED_KEY);
+        $envContentsBefore = (string) file_get_contents($this->envPath);
+        chmod($this->envPath, 0o444);
+
+        try {
+            command('key:rotate --force');
+
+            $output = $this->getUndecoratedBuffer();
+            $this->assertStringContainsString('Cannot rotate: `.env` file at', $output);
+            $this->assertStringContainsString('is not writable', $output);
+            $this->assertSame($envContentsBefore, (string) file_get_contents($this->envPath));
+        } finally {
+            chmod($this->envPath, 0o644);
+        }
+    }
+
     public function testRotateIgnoresCommentMentioningPreviousKeysWhenInserting(): void
     {
         $envContents = "# encryption.previousKeys is for decryption fallback\nencryption.key = " . self::SEED_KEY . "\n";
@@ -514,29 +587,4 @@ public function testRotateInsertsAfterExportPrefixedEncryptionKey(): void
         );
         $this->assertSame(self::SEED_KEY, env('encryption.previousKeys'));
     }
-
-    public function testRotateErrorsWhenEnvFileIsNotWritable(): void
-    {
-        $this->seedEnv(self::SEED_KEY);
-        chmod($this->envPath, 0o444);
-
-        try {
-            command('key:rotate --force');
-
-            $this->assertSame(
-                sprintf(
-                    <<<'EOT'
-
-                        Failed to write `encryption.previousKeys` to %s.
-
-                        EOT,
-                    clean_path($this->envPath),
-                ),
-                $this->getUndecoratedBuffer(),
-            );
-            $this->assertSame(self::SEED_KEY, env('encryption.key'));
-        } finally {
-            chmod($this->envPath, 0o644);
-        }
-    }
 }

user_guide_src/source/libraries/encryption.rst

@@ -252,6 +252,10 @@ Useful options:
 All three options are validated up-front, so an invalid value cannot leave the **.env** file
 half-rotated.
 
+.. warning:: ``key:rotate`` is not safe under concurrent execution. The command edits the
+    **.env** file without taking a file lock, so two operators (or two automation runs)
+    rotating at the same time can lose rotated-out keys.
+
 Padding
 =======