From 5de1f457030ace14a8dff7ce4a5715c2fe466311 Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 03:50:19 +0200 Subject: [PATCH] Close two ways a mail.manage session could take over outgoing mail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I3: nothing stopped an operator unchecking Aktiv on the mailbox that mail.purpose.system points at. MailboxResolver::active() filters an inactive mailbox at BOTH the direct lookup and the system fallback, so that one checkbox could take mail.purpose.system to null and every purpose without its own mapping down with it — the existing system_required guard on savePurposes() reads as if this were covered, but it only blocks the empty-mapping path, not this one. EditMailbox::save() now refuses active=false against the mailbox 'system' currently points at, the same way savePurposes() already refuses to leave 'system' empty. I4: Admin\Mail::saveServer() writes mail.host — the platform's outbound relay for every purpose mailbox at once — and CloudReady::toMail() puts a customer's Nextcloud admin password in cleartext in the message body. So a signed-in mail.manage session was already a credential-interception primitive, the same threat Admin\Secrets' second gate exists for. Both saveServer() and EditMailbox::save() (only when a new password is actually being typed) now also require passwordRecentlyConfirmed(), following the ConfirmsPassword pattern Secrets.php and Settings.php's two-factor actions already use — including their per-action UI shape (an inline confirm form scoped to the gated action), not a whole-page lock, so address, display name, username, no-reply, active and the purpose mapping stay reachable without confirming, per the brief's capability split. Both gated and ungated paths are covered by tests, and both guards were mutated away and confirmed to fail before being restored. --- app/Livewire/Admin/Mail.php | 14 ++ app/Livewire/EditMailbox.php | 50 +++++-- lang/de/mail_settings.php | 4 + lang/en/mail_settings.php | 4 + resources/views/livewire/admin/mail.blade.php | 24 ++- .../views/livewire/edit-mailbox.blade.php | 21 +++ tests/Feature/Mail/MailSettingsPageTest.php | 139 +++++++++++++++++- 7 files changed, 239 insertions(+), 17 deletions(-) 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]);