Fix: Encryption ignores APP_KEY from env.toml and silently uses a public test key

phpunit.xml.dist

@@ -10,6 +10,7 @@
         <ini name="intl.default_locale" value="C.UTF-8" />
         <ini name="memory_limit" value="2048M" />
         <env name="DB_CONNECTION" value="testing" />
+        <env name="APP_KEY" value="base64:ImGTGoQ6ZhM7yBlMvp41ejp0nt8juImy1aIbf6shQCI=" />
         <!--
         <env name="REDIS_CLIENT" value="phpredis" />
         <env name="REDIS_HOST" value="127.0.0.1" />

src/Phaseolies/Support/Encryption.php

@@ -39,15 +39,6 @@ class Encryption
      */
     private const TAG_LENGTH = 16;
 
-    /**
-     * Fallback key used only when no APP_KEY is configured in the
-     * environment (this package's own test suite never loads a
-     * .env file).
-     *
-     * @var string
-     */
-    private const TESTING_KEY = 'base64:ImGTGoQ6ZhM7yBlMvp41ejp0nt8juImy1aIbf6shQCI=';
-
     /**
      * Encrypt data using authenticated AES-256-GCM encryption.
      *
@@ -145,6 +136,11 @@ private function decryptAead(string $payload): string|false
      * to derive before the APP_KEY decoding bug was fixed, so data
      * encrypted before that fix remains readable.
      *
+     * CBC carries no authentication, so the wrong key still "succeeds"
+     * whenever its garbage output happens to end in valid padding
+     * (roughly 1 in 256). When both keys decrypt, the result that is
+     * valid UTF-8 is preferred, since wrong-key output is random bytes.
+     *
      * @param string $encryptedData
      * @return string|false
      */
@@ -158,15 +154,23 @@ private function decryptLegacy(string $encryptedData): string|false
 
         [$ciphertext, $iv] = explode('::', $data, 2);
 
+        $candidates = [];
+
         foreach ([$this->getAppKey(), $this->getLegacyBuggyAppKey()] as $key) {
             $decrypted = openssl_decrypt($ciphertext, self::LEGACY_CIPHER, $key, 0, $iv);
 
             if ($decrypted !== false) {
-                return $decrypted;
+                $candidates[] = $decrypted;
             }
         }
 
-        return false;
+        foreach ($candidates as $candidate) {
+            if (preg_match('//u', $candidate) === 1) {
+                return $candidate;
+            }
+        }
+
+        return $candidates[0] ?? false;
     }
 
     /**
@@ -198,17 +202,26 @@ private function getLegacyBuggyAppKey(): string
     }
 
     /**
-     * Get the raw, undecoded APP_KEY value from the environment,
-     * falling back to a fixed testing key only when no real key is
-     * configured at all.
+     * Get the raw, undecoded APP_KEY value from the environment.
+     *
+     * The key is looked up the same way env() does, so a key loaded
+     * from env.toml (which only populates $_ENV and $_SERVER) is found
+     * as well as one set as a real process environment variable.
      *
      * @return string
+     * @throws RuntimeException
      */
     private function getRawAppKey(): string
     {
-        $key = getenv('APP_KEY');
+        $key = $_ENV['APP_KEY'] ?? $_SERVER['APP_KEY'] ?? getenv('APP_KEY');
+
+        if (!is_string($key) || trim($key) === '') {
+            throw new RuntimeException(
+                'No application encryption key has been specified. Set APP_KEY in env.toml or run "php pool key:generate".'
+            );
+        }
 
-        return ($key !== false && $key !== '') ? $key : self::TESTING_KEY;
+        return $key;
     }
 
     /**

tests/EncryptionTest.php

@@ -112,6 +112,26 @@ public function test_decrypts_legacy_ciphertext_encrypted_with_pre_fix_buggy_key
         $this->assertSame('pre-fix secret', $this->encryption->decrypt($legacyBlob));
     }
 
+    public function test_legacy_ciphertext_is_not_decrypted_to_garbage_by_the_other_key()
+    {
+        // Without a MAC, the wrong CBC key yields valid padding about 1 time
+        // in 256, so enough random IVs are guaranteed to hit that case for
+        // both the correct key and the pre-fix buggy key.
+        $keys = [
+            $this->encryption->getAppKey(),
+            base64_decode(getenv('APP_KEY') ?: 'base64:ImGTGoQ6ZhM7yBlMvp41ejp0nt8juImy1aIbf6shQCI='),
+        ];
+
+        foreach ($keys as $key) {
+            for ($i = 0; $i < 3000; $i++) {
+                $iv = openssl_random_pseudo_bytes(16);
+                $legacyBlob = base64_encode(openssl_encrypt('legacy secret', 'AES-256-CBC', $key, 0, $iv) . '::' . $iv);
+
+                $this->assertSame('legacy secret', $this->encryption->decrypt($legacyBlob));
+            }
+        }
+    }
+
     public function test_get_app_key_strips_base64_prefix_correctly()
     {
         $rawEnv = 'base64:ImGTGoQ6ZhM7yBlMvp41ejp0nt8juImy1aIbf6shQCI=';
@@ -124,4 +144,95 @@ public function test_get_app_key_strips_base64_prefix_correctly()
         $this->assertSame(32, strlen($key));
         $this->assertNotSame($buggyKey, $key);
     }
+
+    public function test_uses_app_key_loaded_from_env_toml()
+    {
+        // env.toml only populates $_ENV and $_SERVER, never putenv()
+        $tomlKey = 'base64:' . base64_encode(random_bytes(32));
+
+        $this->withAppKey($tomlKey, function () use ($tomlKey) {
+            $this->assertSame(base64_decode(substr($tomlKey, 7)), $this->encryption->getAppKey());
+
+            $encrypted = $this->encryption->encrypt('secret');
+
+            $this->assertSame('secret', $this->encryption->decrypt($encrypted));
+        }, putenv: false);
+    }
+
+    public function test_ciphertext_cannot_be_decrypted_with_another_key()
+    {
+        $encrypted = null;
+
+        $this->withAppKey('base64:' . base64_encode(random_bytes(32)), function () use (&$encrypted) {
+            $encrypted = $this->encryption->encrypt('secret');
+        });
+
+        $this->withAppKey('base64:' . base64_encode(random_bytes(32)), function () use ($encrypted) {
+            $this->assertFalse($this->encryption->decrypt($encrypted));
+        });
+    }
+
+    public function test_throws_when_app_key_is_missing()
+    {
+        $this->withAppKey(null, function () {
+            $this->expectException(RuntimeException::class);
+            $this->expectExceptionMessage('No application encryption key has been specified');
+
+            $this->encryption->encrypt('secret');
+        });
+    }
+
+    public function test_throws_when_app_key_is_empty()
+    {
+        $this->withAppKey('', function () {
+            $this->expectException(RuntimeException::class);
+            $this->expectExceptionMessage('No application encryption key has been specified');
+
+            $this->encryption->decrypt('v2:anything');
+        });
+    }
+
+    /**
+     * Run the callback with APP_KEY set to the given value (null unsets it),
+     * restoring the original environment afterwards.
+     */
+    private function withAppKey(?string $key, callable $callback, bool $putenv = true): void
+    {
+        $original = [
+            'env' => $_ENV['APP_KEY'] ?? null,
+            'server' => $_SERVER['APP_KEY'] ?? null,
+            'getenv' => getenv('APP_KEY'),
+        ];
+
+        unset($_ENV['APP_KEY'], $_SERVER['APP_KEY']);
+        putenv('APP_KEY');
+
+        if ($key !== null) {
+            $_ENV['APP_KEY'] = $key;
+            $_SERVER['APP_KEY'] = $key;
+
+            if ($putenv) {
+                putenv("APP_KEY={$key}");
+            }
+        }
+
+        try {
+            $callback();
+        } finally {
+            unset($_ENV['APP_KEY'], $_SERVER['APP_KEY']);
+            putenv('APP_KEY');
+
+            if ($original['env'] !== null) {
+                $_ENV['APP_KEY'] = $original['env'];
+            }
+
+            if ($original['server'] !== null) {
+                $_SERVER['APP_KEY'] = $original['server'];
+            }
+
+            if ($original['getenv'] !== false) {
+                putenv("APP_KEY={$original['getenv']}");
+            }
+        }
+    }
 }