From d19ec119702e6227b7cff08b3a91cdd602f2c1ba Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Wed, 26 Aug 2026 01:16:39 +0200 Subject: [PATCH] Accept only notification types that can actually notify MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit update() validated preferences.*.type as ['required', 'string'], so any string at all became a row in notification_preferences. Nothing reads it afterwards: emailEnabledFor() looks preferences up by a key the registry knows, so a row under an unknown key is invisible for good. It is not a way into somebody else's settings — user_id comes from the session, never the payload — which is why this is validation rather than authorization. The cost is a table that quietly accumulates rows nobody can see, explain, or remove through the interface. edit() already knew the answer. It filters the registry down to the types that can email at all, by either route, and renders exactly those as toggles. That list is now derived once and used by both halves, so what the screen offers and what it accepts back cannot drift apart. Rejecting a registered-but-unmailable key (client_uploaded is the one in tree) is deliberate rather than incidental: FilesServiceProvider explains that it has no mail companion on purpose, so a preference row for it could never change what anybody receives. --- .../NotificationPreferencesController.php | 48 ++++++++++++++----- .../NotificationPreferencesTest.php | 40 ++++++++++++++++ 2 files changed, 76 insertions(+), 12 deletions(-) diff --git a/app/Modules/Notifications/Http/Controllers/NotificationPreferencesController.php b/app/Modules/Notifications/Http/Controllers/NotificationPreferencesController.php index 90822abc..60dedee4 100644 --- a/app/Modules/Notifications/Http/Controllers/NotificationPreferencesController.php +++ b/app/Modules/Notifications/Http/Controllers/NotificationPreferencesController.php @@ -7,9 +7,11 @@ namespace App\Modules\Notifications\Http\Controllers; use App\Http\Controllers\Controller; use App\Modules\Notifications\NotificationPreference; use App\Modules\Notifications\NotificationPreferences; +use App\Modules\Notifications\NotificationTypeDefinition; use App\Modules\Notifications\NotificationTypeRegistry; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; +use Illuminate\Validation\Rule; use Inertia\Inertia; use Inertia\Response; @@ -30,21 +32,12 @@ class NotificationPreferencesController extends Controller $user = $request->user(); assert($user !== null); - // Only types that can email at all have anything to opt in or out - // of — a pure in-app type has no toggle to show. Either route - // counts: Notifier sending a mail class directly, or the digest - // buffering and sending one. - $emailable = array_values(array_filter( - $this->types->all(), - fn ($type) => $type->mailNotification !== null || $type->digestMail !== null, - )); - return Inertia::render('settings/notifications', [ - 'types' => array_map(fn ($type) => [ + 'types' => array_map(fn (NotificationTypeDefinition $type): array => [ 'key' => $type->key, 'label' => $type->label, 'email_enabled' => $this->preferences->emailEnabledFor($user, $type), - ], $emailable), + ], $this->emailable()), ]); } @@ -55,7 +48,11 @@ class NotificationPreferencesController extends Controller $validated = $request->validate([ 'preferences' => ['required', 'array'], - 'preferences.*.type' => ['required', 'string'], + // Against the registry, not merely "a string": a preference row + // for a type nothing can send is a row that will never be read + // again, and the screen only ever offers back what edit() gave + // it. + 'preferences.*.type' => ['required', 'string', Rule::in($this->emailableKeys())], 'preferences.*.email_enabled' => ['required', 'boolean'], ]); @@ -68,4 +65,31 @@ class NotificationPreferencesController extends Controller return back(); } + + /** + * Only types that can email at all have anything to opt in or out of — + * a pure in-app type has no toggle to show. Either route counts: + * Notifier sending a mail class directly, or the digest buffering and + * sending one. + * + * Shared by both halves on purpose, so what the screen offers and what + * it accepts back cannot drift apart. + * + * @return list + */ + private function emailable(): array + { + return array_values(array_filter( + $this->types->all(), + fn (NotificationTypeDefinition $type): bool => $type->mailNotification !== null || $type->digestMail !== null, + )); + } + + /** + * @return list + */ + private function emailableKeys(): array + { + return array_map(fn (NotificationTypeDefinition $type): string => $type->key, $this->emailable()); + } } diff --git a/tests/Feature/Notifications/NotificationPreferencesTest.php b/tests/Feature/Notifications/NotificationPreferencesTest.php index 747c2de6..caaaf4dd 100644 --- a/tests/Feature/Notifications/NotificationPreferencesTest.php +++ b/tests/Feature/Notifications/NotificationPreferencesTest.php @@ -59,6 +59,46 @@ test('updating preferences via the settings page persists a row per type', funct expect($row->email_enabled)->toBeFalse(); }); +test('a preference for a type nothing has registered is refused', function () { + $client = User::factory()->client()->create(); + + $this->actingAs($client)->put('/settings/notifications', [ + 'preferences' => [ + ['type' => 'not.a.real.type', 'email_enabled' => false], + ], + ])->assertInvalid(['preferences.0.type']); + + expect(NotificationPreference::query()->where('user_id', $client->id)->exists())->toBeFalse(); +}); + +test('a preference for a registered type that cannot email is refused', function () { + $client = User::factory()->client()->create(); + + // A real key, but one the screen never offers a toggle for — a row for + // it could never change what anybody receives. + $this->actingAs($client)->put('/settings/notifications', [ + 'preferences' => [ + ['type' => 'client_uploaded', 'email_enabled' => true], + ], + ])->assertInvalid(['preferences.0.type']); + + expect(NotificationPreference::query()->where('user_id', $client->id)->exists())->toBeFalse(); +}); + +test('one bad key rejects the whole submission, leaving no half-applied state', function () { + $client = User::factory()->client()->create(); + + $this->actingAs($client)->put('/settings/notifications', [ + 'preferences' => [ + ['type' => 'group.membership_approved', 'email_enabled' => false], + ['type' => 'not.a.real.type', 'email_enabled' => false], + ], + ])->assertInvalid(['preferences.1.type']); + + // Validation runs before the loop, so the good row is not written either. + expect(NotificationPreference::query()->where('user_id', $client->id)->exists())->toBeFalse(); +}); + test('the master switch off suppresses email regardless of an explicit per-user opt-in', function () { Notification::fake(); app(Settings::class)->set(Setting::EmailNotificationsEnabled, false);