From 74ac406fc029319cea319f74b6d5f65e0fe72207 Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 05:09:09 +0200 Subject: [PATCH] Make isUsable() apply the same 32-byte rule encrypter() does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isUsable() checked only "is SECRETS_KEY nonempty", so a set-but-malformed key (wrong length, garbage base64) read back as usable even though encrypter() rejects it two lines below. EditMailbox::save() and MailboxTester::run() both gate on isUsable() specifically to avoid an uncaught RuntimeException reaching the operator; a lying isUsable() meant that guard did not fire in exactly the configuration it exists for. Both methods now read resolveKey() — the base64: prefix, the raw-base64 path, the 32-byte check — so they cannot disagree about a value either one is given. SecretVault::isUsable() and the secrets console page's "no key" banner both delegate down to this and are covered here too, not assumed to inherit the fix correctly. --- app/Services/Secrets/SecretCipher.php | 39 ++++++++++++--- tests/Feature/Admin/SecretCipherTest.php | 19 ++++++++ tests/Feature/Admin/SecretVaultTest.php | 19 ++++++-- tests/Feature/Admin/SecretsPageTest.php | 22 +++++++-- tests/Feature/Mail/MailSettingsPageTest.php | 54 +++++++++++++++++++++ 5 files changed, 140 insertions(+), 13 deletions(-) diff --git a/app/Services/Secrets/SecretCipher.php b/app/Services/Secrets/SecretCipher.php index a2fa50f..8a9a252 100644 --- a/app/Services/Secrets/SecretCipher.php +++ b/app/Services/Secrets/SecretCipher.php @@ -29,18 +29,34 @@ final class SecretCipher return $this->encrypter()->decryptString($cipher); } - /** Is storing credentials possible at all on this installation? */ + /** + * Is storing credentials possible at all on this installation? + * + * Delegates to the exact same resolveKey() that encrypter() below checks + * its input against, rather than a second, shorter rule that only looks + * at emptiness. A malformed-but-nonempty key (wrong length, garbage + * base64) used to pass a bare "!== ''" check while encrypter() rejected + * it — which let EditMailbox::save() and MailboxTester::run() sail past + * the very guard they call this method for, straight into the uncaught + * RuntimeException it exists to prevent. + */ public function isUsable(): bool { - return (string) config('admin_access.secrets_key') !== ''; + return $this->resolveKey() !== null; } - private function encrypter(): Encrypter + /** + * The raw, exactly-32-byte key — or null if SECRETS_KEY is empty or + * malformed. The one place that decides "is this key any good", so + * isUsable() and encrypter() read it from the same rule and cannot + * disagree about a value either one is given. + */ + private function resolveKey(): ?string { $key = (string) config('admin_access.secrets_key'); if ($key === '') { - throw new RuntimeException('SECRETS_KEY is not set — refusing to store or read credentials.'); + return null; } if (str_starts_with($key, 'base64:')) { @@ -55,8 +71,19 @@ final class SecretCipher } } - if (strlen($key) !== 32) { - throw new RuntimeException('SECRETS_KEY must be 32 bytes (or base64 of 32 bytes).'); + return strlen($key) === 32 ? $key : null; + } + + private function encrypter(): Encrypter + { + $key = $this->resolveKey(); + + if ($key === null) { + throw new RuntimeException( + (string) config('admin_access.secrets_key') === '' + ? 'SECRETS_KEY is not set — refusing to store or read credentials.' + : 'SECRETS_KEY must be 32 bytes (or base64 of 32 bytes).' + ); } return new Encrypter($key, 'aes-256-cbc'); diff --git a/tests/Feature/Admin/SecretCipherTest.php b/tests/Feature/Admin/SecretCipherTest.php index 360491a..f64cf68 100644 --- a/tests/Feature/Admin/SecretCipherTest.php +++ b/tests/Feature/Admin/SecretCipherTest.php @@ -33,3 +33,22 @@ it('refuses a key that is not 32 bytes', function () { expect(fn () => app(SecretCipher::class)->encrypt('x')) ->toThrow(RuntimeException::class, '32 bytes'); }); + +it('reports unusable for a malformed key, not just an empty one', function () { + // Codex R15#3, P2: isUsable() used to check only "is the string + // nonempty", so a SET-but-malformed SECRETS_KEY (wrong length, garbage + // base64) read back as usable even though encrypter() — called from the + // very same encrypt()/decrypt() this object exposes — rejects exactly + // this value two lines below. EditMailbox::save() and + // MailboxTester::run() both gate on isUsable() specifically to avoid an + // uncaught RuntimeException reaching the operator; a lying isUsable() + // means that guard does not fire in exactly the configuration it exists + // for. + config()->set('admin_access.secrets_key', 'zu-kurz'); + + expect(app(SecretCipher::class)->isUsable())->toBeFalse(); + + // Same key, same verdict from the method it must never disagree with. + expect(fn () => app(SecretCipher::class)->encrypt('x')) + ->toThrow(RuntimeException::class, '32 bytes'); +}); diff --git a/tests/Feature/Admin/SecretVaultTest.php b/tests/Feature/Admin/SecretVaultTest.php index 52432b4..3b4d061 100644 --- a/tests/Feature/Admin/SecretVaultTest.php +++ b/tests/Feature/Admin/SecretVaultTest.php @@ -2,7 +2,9 @@ use App\Models\User; use App\Services\Secrets\SecretVault; +use App\Services\Stripe\HttpStripeClient; use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Http; /** * Credentials stored in the database instead of the .env file. @@ -10,7 +12,6 @@ use Illuminate\Support\Facades\DB; * The stakes are a live payment key, so what is tested here is mostly what the * vault REFUSES to do. */ - beforeEach(function () { config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); config()->set('services.stripe.secret', 'sk_env_fallback'); @@ -65,6 +66,16 @@ it('refuses to store or read anything without its own key', function () { ->toThrow(RuntimeException::class); }); +it('reports unusable for a malformed key too, not only an empty one', function () { + // Codex R15#3, P2: SecretVault::isUsable() only delegates to + // SecretCipher::isUsable() — this proves the delegation actually carries + // the fixed verdict through, rather than assuming it does because the + // one call is a one-liner. + config()->set('admin_access.secrets_key', 'viel-zu-kurz'); + + expect(app(SecretVault::class)->isUsable())->toBeFalse(); +}); + it('fails loudly when a stored value cannot be decrypted, instead of using the old one', function () { // "The key cannot be read" and "there is no key" call for different // actions, and only one of them is fixed by typing it in again. @@ -77,11 +88,11 @@ it('fails loudly when a stored value cannot be decrypted, instead of using the o it('hands the Stripe client the stored key rather than the environment one', function () { app(SecretVault::class)->put('stripe.secret', 'sk_live_from_console', User::factory()->create()); - Illuminate\Support\Facades\Http::fake(['*' => Illuminate\Support\Facades\Http::response(['id' => 'prod_1'], 200)]); + Http::fake(['*' => Http::response(['id' => 'prod_1'], 200)]); - app(App\Services\Stripe\HttpStripeClient::class)->createProduct('Test'); + app(HttpStripeClient::class)->createProduct('Test'); - Illuminate\Support\Facades\Http::assertSent( + Http::assertSent( fn ($request) => $request->hasHeader('Authorization', 'Bearer sk_live_from_console'), ); }); diff --git a/tests/Feature/Admin/SecretsPageTest.php b/tests/Feature/Admin/SecretsPageTest.php index 85fa0ba..9610ddd 100644 --- a/tests/Feature/Admin/SecretsPageTest.php +++ b/tests/Feature/Admin/SecretsPageTest.php @@ -1,5 +1,6 @@ set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); }); +it('shows the no-key banner for a malformed SECRETS_KEY, not only a missing one', function () { + // Codex R15#3, P2: render() feeds 'usable' => $vault->isUsable() to the + // view, and the banner is the one place an operator actually sees that + // verdict. Before the fix, a malformed (nonempty, wrong-length) key made + // isUsable() answer true, so this banner stayed hidden — the page said + // credentials could be stored here when they could not. Asserted before + // the lock gate specifically because @if (! $usable) sits above @if (! + // $unlocked) in the view; a signed-in-but-locked session must still see + // it. + config()->set('admin_access.secrets_key', 'zu-kurz'); + + Livewire::actingAs(operator('Owner')) + ->test(SecretsPage::class) + ->assertSee(__('secrets.no_key')); +}); + it('is not reachable without the capability', function () { // Every operator has console.view. That must not mean "can read the // payment key". @@ -97,6 +113,6 @@ it('binds the form to a key Livewire can actually write to', function () { // A dot in a Livewire property path means nesting, so a registry key with a // dot in it would be written to entered['stripe']['secret'] and the value // would never reach the save. - expect(App\Livewire\Admin\Secrets::field('stripe.secret'))->toBe('stripe_secret') - ->and(str_contains(App\Livewire\Admin\Secrets::field('stripe.secret'), '.'))->toBeFalse(); + expect(Secrets::field('stripe.secret'))->toBe('stripe_secret') + ->and(str_contains(Secrets::field('stripe.secret'), '.'))->toBeFalse(); }); diff --git a/tests/Feature/Mail/MailSettingsPageTest.php b/tests/Feature/Mail/MailSettingsPageTest.php index 1c059e8..2eb6117 100644 --- a/tests/Feature/Mail/MailSettingsPageTest.php +++ b/tests/Feature/Mail/MailSettingsPageTest.php @@ -97,6 +97,21 @@ it('shows the SECRETS_KEY warning on the page', function () { ->assertSee(__('mail_settings.no_key')); }); +it('shows the same SECRETS_KEY warning for a malformed key, not only a missing one', function () { + // Codex R15#3, P2: Mail::render() sets $this->usable from the very same + // SecretCipher::isUsable() EditMailbox and MailboxTester gate on. Before + // the fix, a malformed (nonempty, wrong-length) key answered true here, + // so the page claimed mailbox passwords could be stored when they could + // not — the two tests below are what happened next. + config()->set('admin_access.secrets_key', 'zu-kurz'); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->assertOk() + ->assertSet('usable', false) + ->assertSee(__('mail_settings.no_key')); +}); + it('fails the password save with a form error, not an exception, when SECRETS_KEY is missing', function () { // The test above only proves the PAGE says so. Nothing proved that typing // a password into the MODAL and saving it fails gracefully — the gap a @@ -124,6 +139,29 @@ it('fails the password save with a form error, not an exception, when SECRETS_KE ->assertHasErrors('password'); }); +it('fails the password save with a form error, not an exception, for a malformed SECRETS_KEY too', function () { + // Codex R15#3, P2: the guard this mirrors (EditMailbox::save(), a few + // lines up) is a pre-check on SecretCipher::isUsable() specifically to + // stop an uncaught RuntimeException from Mailbox::setPasswordAttribute() + // reaching the operator as Laravel's debug page — whose request-payload + // inspector can show the very password just typed. Before the fix, that + // guard did not fire here: isUsable() answered true for a malformed key, + // and $box->password = $this->password went ahead and threw uncaught. + $box = Mailbox::factory()->create(['key' => 'support']); + $owner = User::factory()->operator('Owner')->create(); + + $page = Livewire::actingAs($owner) + ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('confirmablePassword', 'password') + ->call('confirmPassword'); + + config()->set('admin_access.secrets_key', 'zu-kurz'); + + $page->set('password', 'neu-und-sicher') + ->call('save') + ->assertHasErrors('password'); +}); + it('refuses to leave the system purpose empty, because it is the fallback', function () { Mailbox::factory()->create(['key' => 'no-reply']); Settings::set(MailPurpose::settingKey(MailPurpose::SYSTEM), 'no-reply'); @@ -674,6 +712,22 @@ it('does not crash the test button when SECRETS_KEY is missing, and says so inst ->assertSet('testResult.error', __('mail_settings.no_key')); }); +it('does not crash the test button for a malformed SECRETS_KEY either, and says so instead', function () { + // Codex R15#3, P2: MailboxTester::run() guards on isUsable() BEFORE + // touching $box->password (which decrypts on read) precisely to avoid an + // uncaught RuntimeException out of a Livewire action. A malformed key + // used to slip that guard exactly like the missing-key case above, just + // one property access later. + $box = Mailbox::factory()->create(['key' => 'support']); + config()->set('admin_access.secrets_key', 'zu-kurz'); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('testRecipient', 'ziel@example.com') + ->call('test', $box->uuid) + ->assertSet('testResult.error', __('mail_settings.no_key')); +}); + it('offers a per-row test-send button, not only edit', function () { $blade = file_get_contents(resource_path('views/livewire/admin/mail.blade.php'));