diff --git a/app/Livewire/Admin/Mail.php b/app/Livewire/Admin/Mail.php index a7190d9..34b2742 100644 --- a/app/Livewire/Admin/Mail.php +++ b/app/Livewire/Admin/Mail.php @@ -2,6 +2,7 @@ namespace App\Livewire\Admin; +use App\Livewire\Concerns\ConfirmsPassword; use App\Models\Mailbox; use App\Services\Mail\MailboxTester; use App\Services\Mail\MailPurpose; @@ -22,6 +23,8 @@ use Livewire\Component; #[Layout('layouts.admin')] class Mail extends Component { + use ConfirmsPassword; + public string $host = ''; public int|string $port = 587; @@ -64,6 +67,16 @@ class Mail extends Component { $this->authorize('mail.manage'); + // $this->host is the platform's outbound relay for every purpose + // mailbox at once — pointing it at an attacker's server intercepts + // everything CluPilot sends. The capability decides who may open this + // page; a recent password decides whether THIS session may repoint + // it, the same second gate Admin\Secrets uses and for the same + // reason: the realistic threat is an unlocked machine, not a + // stranger. savePurposes() and test() stay on the capability alone — + // this is the "changing an address" split's one exception. + abort_unless($this->passwordRecentlyConfirmed(), 403); + // Rules on the ACTION, not on the property: a #[Validate] attribute on // a Livewire property applies class-wide, and savePurposes() below // would drag these along. @@ -120,6 +133,7 @@ class Mail extends Component return view('livewire.admin.mail', [ 'mailboxes' => Mailbox::query()->orderBy('key')->get(), 'purposeList' => MailPurpose::ALL, + 'passwordConfirmed' => $this->passwordRecentlyConfirmed(), ]); } } diff --git a/app/Livewire/EditMailbox.php b/app/Livewire/EditMailbox.php index c226c0a..4d29fdf 100644 --- a/app/Livewire/EditMailbox.php +++ b/app/Livewire/EditMailbox.php @@ -2,8 +2,11 @@ namespace App\Livewire; +use App\Livewire\Concerns\ConfirmsPassword; use App\Models\Mailbox; +use App\Services\Mail\MailPurpose; use App\Services\Secrets\SecretCipher; +use App\Support\Settings; use LivewireUI\Modal\ModalComponent; /** @@ -18,6 +21,8 @@ use LivewireUI\Modal\ModalComponent; */ class EditMailbox extends ModalComponent { + use ConfirmsPassword; + public string $uuid = ''; public string $address = ''; @@ -58,19 +63,42 @@ class EditMailbox extends ModalComponent 'password' => ['nullable', 'string', 'max:255'], ]); - // A password can only be STORED where SECRETS_KEY exists to encrypt it - // under — the same condition the page's own banner already names. - // Checked here rather than left for Mailbox::setPasswordAttribute() to - // throw through: an uncaught RuntimeException would render Laravel's - // debug page, whose request-payload inspector can show the very - // password just typed. The page must say so, not crash on it. - if ($this->password !== '' && ! app(SecretCipher::class)->isUsable()) { - $this->addError('password', __('mail_settings.no_key')); + $box = Mailbox::query()->where('uuid', $this->uuid)->firstOrFail(); + + // Deactivating the mailbox "system" currently points at is not just + // this purpose losing its sender: MailboxResolver::active() filters an + // inactive mailbox at BOTH the direct lookup AND the system fallback, + // so every OTHER purpose that has no mailbox of its own falls through + // to nothing too. savePurposes() already refuses to leave system + // empty; this closes the other way to reach the same broken state. + if (! $this->active && $box->key === (string) Settings::get(MailPurpose::settingKey(MailPurpose::SYSTEM))) { + $this->addError('active', __('mail_settings.cannot_deactivate_system')); return; } - $box = Mailbox::query()->where('uuid', $this->uuid)->firstOrFail(); + // Setting a new SMTP password is one of the two actions the + // mail.manage split still leaves able to intercept mail outright (the + // other is Admin\Mail::saveServer()) — gated the same second way + // Admin\Secrets is: a capability decides who may open this modal at + // all, a recent password decides whether THIS session may point the + // outgoing account at new credentials. + if ($this->password !== '') { + abort_unless($this->passwordRecentlyConfirmed(), 403); + + // A password can only be STORED where SECRETS_KEY exists to + // encrypt it under — the same condition the page's own banner + // already names. Checked here rather than left for + // Mailbox::setPasswordAttribute() to throw through: an uncaught + // RuntimeException would render Laravel's debug page, whose + // request-payload inspector can show the very password just + // typed. The page must say so, not crash on it. + if (! app(SecretCipher::class)->isUsable()) { + $this->addError('password', __('mail_settings.no_key')); + + return; + } + } $box->address = $this->address; $box->display_name = $this->displayName ?: null; @@ -95,6 +123,8 @@ class EditMailbox extends ModalComponent public function render() { - return view('livewire.edit-mailbox'); + return view('livewire.edit-mailbox', [ + 'passwordConfirmed' => $this->passwordRecentlyConfirmed(), + ]); } } diff --git a/lang/de/mail_settings.php b/lang/de/mail_settings.php index dad22f2..b16ed3d 100644 --- a/lang/de/mail_settings.php +++ b/lang/de/mail_settings.php @@ -29,6 +29,10 @@ return [ 'saved' => 'Postfach gespeichert.', 'never_verified' => 'Noch nicht bestätigt', 'last_verified' => 'Zuletzt bestätigt', + 'cannot_deactivate_system' => '„System" verweist derzeit auf dieses Postfach — es kann nicht deaktiviert werden, sonst bliebe kein Rückfall mehr übrig.', + + 'confirm_first' => 'Bitte zuerst Ihr Passwort bestätigen — eine angemeldete Sitzung allein genügt hier nicht.', + 'confirm_button' => 'Bestätigen', 'purposes_title' => 'Wer verschickt was', 'purposes_hint' => 'Ein Zweck ohne Postfach verschickt über „System".', diff --git a/lang/en/mail_settings.php b/lang/en/mail_settings.php index dadeaf2..8f5eb64 100644 --- a/lang/en/mail_settings.php +++ b/lang/en/mail_settings.php @@ -29,6 +29,10 @@ return [ 'saved' => 'Mailbox saved.', 'never_verified' => 'Not yet verified', 'last_verified' => 'Last verified', + 'cannot_deactivate_system' => '"System" currently falls back to this mailbox — it cannot be deactivated, or nothing would be left to fall back to.', + + 'confirm_first' => 'Confirm your password first — a signed-in session alone is not enough here.', + 'confirm_button' => 'Confirm', 'purposes_title' => 'Who sends what', 'purposes_hint' => 'A purpose with no mailbox sends through "System".', diff --git a/resources/views/livewire/admin/mail.blade.php b/resources/views/livewire/admin/mail.blade.php index e5a5bc9..f67ad4e 100644 --- a/resources/views/livewire/admin/mail.blade.php +++ b/resources/views/livewire/admin/mail.blade.php @@ -28,9 +28,27 @@ - - {{ __('mail_settings.save') }} - + @if (! $passwordConfirmed) + {{-- The second gate. $host is the platform's outbound relay for + every purpose at once — being signed in is not enough to + repoint it, the same reasoning as the credentials page. --}} +
+

{{ __('mail_settings.confirm_first') }}

+
+
+ +
+ + {{ __('mail_settings.confirm_button') }} + +
+
+ @else + + {{ __('mail_settings.save') }} + + @endif {{-- 2. The mailboxes. NO input field inside a — R20. --}} diff --git a/resources/views/livewire/edit-mailbox.blade.php b/resources/views/livewire/edit-mailbox.blade.php index 40b6a90..884f70e 100644 --- a/resources/views/livewire/edit-mailbox.blade.php +++ b/resources/views/livewire/edit-mailbox.blade.php @@ -12,11 +12,32 @@ + @if (! $passwordConfirmed) + {{-- The second gate, only reached if a new password is actually + typed above: address, display name, username and active save + without it. Shown here rather than blocking the whole modal + behind it, the same as saveServer() leaves savePurposes() + alone on the console page. --}} +
+

{{ __('mail_settings.confirm_first') }}

+
+
+ +
+ + {{ __('mail_settings.confirm_button') }} + +
+
+ @endif +

{{ __('mail_settings.no_reply_hint') }}

+ @error('active')

{{ $message }}

@enderror
diff --git a/tests/Feature/Mail/MailSettingsPageTest.php b/tests/Feature/Mail/MailSettingsPageTest.php index 715bc2a..0d8ecc7 100644 --- a/tests/Feature/Mail/MailSettingsPageTest.php +++ b/tests/Feature/Mail/MailSettingsPageTest.php @@ -106,12 +106,20 @@ it('fails the password save with a form error, not an exception, when SECRETS_KE // password-writing tests above run under this file's valid-key beforeEach, // so none of them would have caught it. $box = Mailbox::factory()->create(['key' => 'support']); + $owner = User::factory()->operator('Owner')->create(); + + // Confirmed BEFORE the key is cleared: this test is about the SECRETS_KEY + // guard specifically, not about the password-confirmation gate above it — + // conflating the two would leave it unclear which one actually produced + // the error. + $page = Livewire::actingAs($owner) + ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('confirmablePassword', 'password') + ->call('confirmPassword'); config()->set('admin_access.secrets_key', ''); - Livewire::actingAs(User::factory()->operator('Owner')->create()) - ->test(EditMailbox::class, ['uuid' => $box->uuid]) - ->set('password', 'neu-und-sicher') + $page->set('password', 'neu-und-sicher') ->call('save') ->assertHasErrors('password'); }); @@ -129,6 +137,45 @@ it('refuses to leave the system purpose empty, because it is the fallback', func expect(Settings::get(MailPurpose::settingKey(MailPurpose::SYSTEM)))->toBe('no-reply'); }); +it('refuses to deactivate the mailbox that "system" currently points at', function () { + // savePurposes() above refuses to leave system EMPTY. Nothing stopped an + // operator reaching the identical broken state — every purpose without + // its own mailbox unable to send — by unchecking Aktiv on the mailbox + // system already points at instead: MailboxResolver::active() filters an + // inactive mailbox at both the direct lookup and the system fallback, so + // resolution() returns null everywhere at once. + $box = Mailbox::factory()->create(['key' => 'no-reply', 'active' => true]); + Settings::set(MailPurpose::settingKey(MailPurpose::SYSTEM), 'no-reply'); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('active', false) + ->call('save') + ->assertHasErrors('active'); + + expect($box->fresh()->active)->toBeTrue(); +}); + +it('still allows deactivating a mailbox that is not behind "system"', function () { + // The control case for the guard above: the fix must refuse THIS ONE + // transition, not deactivation in general — "saves display name, + // username, no-reply and active together" already proves the field + // itself still works, but not against a system mapping that exists and + // simply points elsewhere. + $systemBox = Mailbox::factory()->create(['key' => 'no-reply']); + $otherBox = Mailbox::factory()->create(['key' => 'support', 'active' => true]); + Settings::set(MailPurpose::settingKey(MailPurpose::SYSTEM), 'no-reply'); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(EditMailbox::class, ['uuid' => $otherBox->uuid]) + ->set('active', false) + ->call('save') + ->assertHasNoErrors(); + + expect($otherBox->fresh()->active)->toBeFalse() + ->and($systemBox->fresh()->active)->toBeTrue(); +}); + // --- Coverage beyond the brief's list: behaviours the page and modal claim // but the tests above never exercise. Each was verified to fail for the // right reason before the implementation existed, and again by mutation @@ -155,6 +202,8 @@ it('shows the currently configured mailbox for each purpose', function () { it('saves the mail server settings', function () { Livewire::actingAs(User::factory()->operator('Owner')->create()) ->test(MailPage::class) + ->set('confirmablePassword', 'password') + ->call('confirmPassword') ->set('host', 'smtp.example.com') ->set('port', 2525) ->set('encryption', 'ssl') @@ -169,11 +218,53 @@ it('saves the mail server settings', function () { it('rejects an empty mail server host', function () { Livewire::actingAs(User::factory()->operator('Owner')->create()) ->test(MailPage::class) + ->set('confirmablePassword', 'password') + ->call('confirmPassword') ->set('host', '') ->call('saveServer') ->assertHasErrors('host'); }); +it('refuses to change the mail server without a recently confirmed password, capability notwithstanding', function () { + // saveServer() writes mail.host — the platform's outbound relay for every + // purpose mailbox at once. An Owner has mail.manage for the whole test; + // nothing here proves the SECOND gate exists unless a properly capable + // session still gets blocked for want of a recent confirmation. + $before = Settings::get('mail.host'); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('host', 'evil-relay.example.com') + ->set('port', 2525) + ->set('encryption', 'ssl') + ->call('saveServer') + ->assertForbidden(); + + // Not ->toBeNull(): RefreshDatabase's baseline migrate already seeds + // mail.host from the real environment (see the seed migration), so a + // blank slate is not what "the write was blocked" looks like here — an + // unchanged value, still not the attacker's relay, is. + expect(Settings::get('mail.host'))->toBe($before) + ->and(Settings::get('mail.host'))->not->toBe('evil-relay.example.com'); +}); + +it('does not require a confirmed password to save the purpose mapping', function () { + // The mirror of the test above: saveServer() is gated, savePurposes() is + // deliberately not — this is the "changing an address" split the brief + // describes, proven from the ungated side rather than left implicit in + // the tests that merely never happen to call confirmPassword(). + Mailbox::factory()->create(['key' => 'support']); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('purposes.system', 'support') + ->set('purposes.support', 'support') + ->call('savePurposes') + ->assertHasNoErrors(); + + expect(Settings::get(MailPurpose::settingKey(MailPurpose::SUPPORT)))->toBe('support'); +}); + it('saves the purpose-to-mailbox mapping', function () { Mailbox::factory()->create(['key' => 'support']); Mailbox::factory()->create(['key' => 'no-reply']); @@ -239,11 +330,14 @@ it('updates the password when a new one is entered', function () { // The mirror of "keeps the old password when the field is left empty": // that test alone would stay green even if save() stopped writing the // password entirely, since "never changes" also satisfies "kept the old - // one". This proves the other half — a new value actually lands. + // one". This proves the other half — a new value actually lands, once the + // second gate (below) is satisfied. $box = Mailbox::factory()->create(['password' => 'alt']); Livewire::actingAs(User::factory()->operator('Owner')->create()) ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('confirmablePassword', 'password') + ->call('confirmPassword') ->set('password', 'neu-und-sicher') ->call('save'); @@ -255,12 +349,49 @@ it('clears last_verified_at when the password changes, since it no longer proves Livewire::actingAs(User::factory()->operator('Owner')->create()) ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('confirmablePassword', 'password') + ->call('confirmPassword') ->set('password', 'neu-und-sicher') ->call('save'); expect($box->fresh()->last_verified_at)->toBeNull(); }); +it('refuses to set a new mailbox password without a recently confirmed password', function () { + // The credential-interception primitive the brief names: a signed-in + // mail.manage session alone must not be enough to repoint where a + // mailbox's outgoing SMTP password comes from. + $box = Mailbox::factory()->create(['password' => 'alt']); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('password', 'neu-und-sicher') + ->call('save') + ->assertForbidden(); + + expect($box->fresh()->password)->toBe('alt'); +}); + +it('does not require a confirmed password to save address, display name, username or active when the password field is left empty', function () { + // The other half of the split the brief draws: setting a NEW password is + // gated, but "changing an address" — everything else this modal does — + // is not. "saves display name, username, no-reply and active together" + // already exercises this path without ever calling confirmPassword(); + // this test says so explicitly and checks the gate was never even + // reachable (no 403), rather than leaving the claim implicit. + $box = Mailbox::factory()->create(['address' => 'alt@clupilot.com']); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('address', 'neu@clupilot.com') + ->set('displayName', 'Neu-Team') + ->call('save') + ->assertHasNoErrors() + ->assertOk(); + + expect($box->fresh()->address)->toBe('neu@clupilot.com'); +}); + it('saves display name, username, no-reply and active together', function () { $box = Mailbox::factory()->create(['no_reply' => false, 'active' => true]);