Make isUsable() apply the same 32-byte rule encrypter() does

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.
feat/mailboxes
nexxo 2026-07-28 05:09:09 +02:00
parent f0c3bd9c2e
commit 74ac406fc0
5 changed files with 140 additions and 13 deletions

View File

@ -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');

View File

@ -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');
});

View File

@ -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'),
);
});

View File

@ -1,5 +1,6 @@
<?php
use App\Livewire\Admin\Secrets;
use App\Livewire\Admin\Secrets as SecretsPage;
use App\Services\Secrets\SecretVault;
use Livewire\Livewire;
@ -12,11 +13,26 @@ use Livewire\Livewire;
* stranger but an unlocked machine, and a session is exactly what that gives
* away.
*/
beforeEach(function () {
config()->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();
});

View File

@ -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'));