diff --git a/app/Filament/GuestPanel/Resources/DvrRecordings/GuestDvrRecordingResource.php b/app/Filament/GuestPanel/Resources/DvrRecordings/GuestDvrRecordingResource.php index f77dd32cd..01b5a8eb6 100644 --- a/app/Filament/GuestPanel/Resources/DvrRecordings/GuestDvrRecordingResource.php +++ b/app/Filament/GuestPanel/Resources/DvrRecordings/GuestDvrRecordingResource.php @@ -168,6 +168,19 @@ public static function getEloquentQuery(): Builder $currentAuth = static::getCurrentPlaylistAuth(); + // Stale session: the credentials no longer resolve to a live + // PlaylistAuth row (e.g. revoked/disabled mid-session). Without this + // guard, Laravel's query builder turns `where(col, null)` into + // `whereNull(col)` and would happily return the playlist OWNER's + // recordings (the only ones with playlist_auth_id = null), re-opening + // the leak issue #1398 exists to close. isOwnerAuth() must be allowed + // through — the owner has no PlaylistAuth row, so getCurrentPlaylistAuth() + // legitimately returns null for them, and they own the recordings with + // playlist_auth_id = null. + if (! $currentAuth && ! static::isOwnerAuth()) { + return parent::getEloquentQuery()->whereRaw('1 = 0'); + } + return parent::getEloquentQuery() ->with(['channel', 'playlistAuth', 'dvrSetting.playlist', 'dvrSetting.customPlaylist', 'dvrSetting.mergedPlaylist']) ->where('dvr_setting_id', $dvrSetting->id) diff --git a/app/Filament/GuestPanel/Resources/DvrRules/GuestDvrRuleResource.php b/app/Filament/GuestPanel/Resources/DvrRules/GuestDvrRuleResource.php index 590750208..71096f93c 100644 --- a/app/Filament/GuestPanel/Resources/DvrRules/GuestDvrRuleResource.php +++ b/app/Filament/GuestPanel/Resources/DvrRules/GuestDvrRuleResource.php @@ -119,6 +119,19 @@ public static function getEloquentQuery(): Builder $currentAuth = static::getCurrentPlaylistAuth(); + // Stale session: the credentials no longer resolve to a live + // PlaylistAuth row (e.g. revoked/disabled mid-session). Without this + // guard, Laravel's query builder turns `where(col, null)` into + // `whereNull(col)` and would happily return the playlist OWNER's + // rules (the only ones with playlist_auth_id = null), re-opening the + // leak issue #1398 exists to close. isOwnerAuth() must be allowed + // through — the owner has no PlaylistAuth row, so getCurrentPlaylistAuth() + // legitimately returns null for them, and they own the rules with + // playlist_auth_id = null. + if (! $currentAuth && ! static::isOwnerAuth()) { + return parent::getEloquentQuery()->whereRaw('1 = 0'); + } + return parent::getEloquentQuery() ->with(['channel', 'playlistAuth']) ->where('dvr_setting_id', $dvrSetting->id) diff --git a/app/Filament/GuestPanel/Widgets/GuestScheduledSeriesWidget.php b/app/Filament/GuestPanel/Widgets/GuestScheduledSeriesWidget.php index 980c3f932..42670b6d6 100644 --- a/app/Filament/GuestPanel/Widgets/GuestScheduledSeriesWidget.php +++ b/app/Filament/GuestPanel/Widgets/GuestScheduledSeriesWidget.php @@ -28,6 +28,19 @@ public function getSeriesRules(): Collection $currentAuth = static::getCurrentPlaylistAuth(); + // Stale session: the credentials no longer resolve to a live + // PlaylistAuth row (e.g. revoked/disabled mid-session). Without this + // guard, Laravel's query builder turns `where(col, null)` into + // `whereNull(col)` and would happily return the playlist OWNER's + // series rules (the only ones with playlist_auth_id = null), + // re-opening the leak issue #1398 exists to close. isOwnerAuth() must + // be allowed through — the owner has no PlaylistAuth row, so + // getCurrentPlaylistAuth() legitimately returns null for them, and + // they own the rules with playlist_auth_id = null. + if (! $currentAuth && ! static::isOwnerAuth()) { + return new Collection; + } + return DvrRecordingRule::with(['channel']) ->where('dvr_setting_id', $dvrSetting->id) ->where('type', DvrRuleType::Series) diff --git a/tests/Feature/GuestDvrRecordingResourceTest.php b/tests/Feature/GuestDvrRecordingResourceTest.php index db0e2914a..c8daf67f8 100644 --- a/tests/Feature/GuestDvrRecordingResourceTest.php +++ b/tests/Feature/GuestDvrRecordingResourceTest.php @@ -41,6 +41,25 @@ function setOwnerAuthRecordingContext(Playlist $playlist, User $user): void session()->put("{$prefix}guest_auth_password", $playlist->uuid); } +/** + * Set up a "stale guest" session: the session still has credentials in the + * expected keys (so getCurrentAuth() returns non-null), but they don't match + * any PlaylistAuth row (so getCurrentPlaylistAuth() returns null). This is + * the state a guest lands in when their PlaylistAuth is revoked/disabled + * mid-session while stale session credentials still exist — the exact + * scenario issue #1398 follow-up is about. getDvrSetting() must still + * resolve normally via the request attribute so the only path that can leak + * is the playlist_auth_id whereNull() coercion of the original fix. + */ +function setStaleGuestRecordingContext(Playlist $playlist): void +{ + request()->attributes->set('playlist_uuid', $playlist->uuid); + + $prefix = base64_encode($playlist->uuid).'_'; + session()->put("{$prefix}guest_auth_username", 'nonexistent_guest'); + session()->put("{$prefix}guest_auth_password", 'irrelevant_password'); +} + beforeEach(function () { Queue::fake(); config()->set('dvr.dvr_enabled', true); @@ -454,3 +473,68 @@ function makeGuestPlayRecording(object $ctx, DvrRecordingStatus $status, ?int $a expect(GuestDvrRecordingResource::guestCanCancel($recording, null))->toBeFalse(); }); + +// --- Stale-guest null-auth fail-open (issue #1398 follow-up) --- +// +// When a guest's PlaylistAuth is revoked/disabled while they still hold a +// session with credentials in the expected keys, getCurrentAuth() returns +// non-null but getCurrentPlaylistAuth() returns null. The merged fix for +// #1398 (scoping to playlist_auth_id) is correct for live guests, but the +// `?->id` fallback to whereNull() was turning that null into "show every +// recording with playlist_auth_id = null" — i.e. the playlist owner's. The +// fix in getEloquentQuery() must fail closed in this state. isOwnerAuth() +// must be allowed through, otherwise the legitimate playlist-owner login +// (which has no PlaylistAuth row) regresses. + +it('returns no recordings when getCurrentPlaylistAuth() resolves to null and the session is not owner-auth', function () { + $ownerRecording = DvrRecording::factory() + ->for($this->dvrSetting) + ->for($this->user) + ->create(['playlist_auth_id' => null]); + + DvrRecording::factory() + ->for($this->dvrSetting) + ->for($this->user) + ->create(['playlist_auth_id' => $this->guestA->id]); + + setStaleGuestRecordingContext($this->playlist); + + // Sanity check on the precondition — if these stop holding the test no + // longer exercises the bug it's meant to. + expect(GuestDvrRecordingResource::getDvrSetting())->not->toBeNull() + ->and(GuestDvrRecordingResource::getCurrentPlaylistAuth())->toBeNull(); + + $count = GuestDvrRecordingResource::getEloquentQuery()->count(); + + expect($count)->toBe(0); +}); + +it('returns no recordings when getCurrentPlaylistAuth() resolves to null even if the owner has recordings', function () { + // Specifically guard against leaking the owner's recording, which is the + // exact privacy regression #1398 exists to prevent. + DvrRecording::factory() + ->for($this->dvrSetting) + ->for($this->user) + ->create(['playlist_auth_id' => null]); + + setStaleGuestRecordingContext($this->playlist); + + $ids = GuestDvrRecordingResource::getEloquentQuery()->pluck('id')->all(); + + expect($ids)->toBe([]); +}); + +it('navigation badge is null when getCurrentPlaylistAuth() resolves to null', function () { + DvrRecording::factory() + ->for($this->dvrSetting) + ->for($this->user) + ->create(['playlist_auth_id' => null, 'status' => DvrRecordingStatus::Scheduled]); + DvrRecording::factory() + ->for($this->dvrSetting) + ->for($this->user) + ->create(['playlist_auth_id' => $this->guestA->id, 'status' => DvrRecordingStatus::Scheduled]); + + setStaleGuestRecordingContext($this->playlist); + + expect(GuestDvrRecordingResource::getNavigationBadge())->toBeNull(); +}); diff --git a/tests/Feature/GuestDvrRuleResourceTest.php b/tests/Feature/GuestDvrRuleResourceTest.php index fa3ed194f..a9ac40688 100644 --- a/tests/Feature/GuestDvrRuleResourceTest.php +++ b/tests/Feature/GuestDvrRuleResourceTest.php @@ -40,6 +40,25 @@ function setOwnerAuthRuleContext(Playlist $playlist, User $user): void session()->put("{$prefix}guest_auth_password", $playlist->uuid); } +/** + * Set up a "stale guest" session: credentials are present in the expected + * session keys (so getCurrentAuth() returns non-null), but they don't match + * any PlaylistAuth row (so getCurrentPlaylistAuth() returns null). This is + * the state a guest lands in when their PlaylistAuth is revoked/disabled + * mid-session while stale session credentials still exist — the exact + * scenario issue #1398 follow-up is about. getDvrSetting() must still + * resolve normally via the request attribute so the only path that can leak + * is the playlist_auth_id whereNull() coercion of the original fix. + */ +function setStaleGuestRuleContext(Playlist $playlist): void +{ + request()->attributes->set('playlist_uuid', $playlist->uuid); + + $prefix = base64_encode($playlist->uuid).'_'; + session()->put("{$prefix}guest_auth_username", 'nonexistent_guest'); + session()->put("{$prefix}guest_auth_password", 'irrelevant_password'); +} + beforeEach(function () { Queue::fake(); config()->set('dvr.dvr_enabled', true); @@ -364,3 +383,53 @@ function setOwnerAuthRuleContext(Playlist $playlist, User $user): void expect(GuestDvrRuleResource::canEdit($rule))->toBeFalse() ->and(GuestDvrRuleResource::canDelete($rule))->toBeFalse(); }); + +// --- Stale-guest null-auth fail-open (issue #1398 follow-up) --- +// +// When a guest's PlaylistAuth is revoked/disabled while they still hold a +// session with credentials in the expected keys, getCurrentAuth() returns +// non-null but getCurrentPlaylistAuth() returns null. The merged fix for +// #1398 (scoping to playlist_auth_id) is correct for live guests, but the +// `?->id` fallback to whereNull() was turning that null into "show every +// rule with playlist_auth_id = null" — i.e. the playlist owner's. The fix +// in getEloquentQuery() must fail closed in this state. isOwnerAuth() must +// be allowed through, otherwise the legitimate playlist-owner login (which +// has no PlaylistAuth row) regresses. + +it('returns no rules when getCurrentPlaylistAuth() resolves to null and the session is not owner-auth', function () { + $ownerRule = DvrRecordingRule::factory() + ->for($this->dvrSetting) + ->for($this->user) + ->create(['playlist_auth_id' => null]); + + DvrRecordingRule::factory() + ->for($this->dvrSetting) + ->for($this->user) + ->create(['playlist_auth_id' => $this->guestA->id]); + + setStaleGuestRuleContext($this->playlist); + + // Sanity check on the precondition — if these stop holding the test no + // longer exercises the bug it's meant to. + expect(GuestDvrRuleResource::getDvrSetting())->not->toBeNull() + ->and(GuestDvrRuleResource::getCurrentPlaylistAuth())->toBeNull(); + + $count = GuestDvrRuleResource::getEloquentQuery()->count(); + + expect($count)->toBe(0); +}); + +it('returns no rules when getCurrentPlaylistAuth() resolves to null even if the owner has rules', function () { + // Specifically guard against leaking the owner's rules, which is the + // exact privacy regression #1398 exists to prevent. + DvrRecordingRule::factory() + ->for($this->dvrSetting) + ->for($this->user) + ->create(['playlist_auth_id' => null]); + + setStaleGuestRuleContext($this->playlist); + + $ids = GuestDvrRuleResource::getEloquentQuery()->pluck('id')->all(); + + expect($ids)->toBe([]); +}); diff --git a/tests/Feature/GuestScheduledSeriesWidgetTest.php b/tests/Feature/GuestScheduledSeriesWidgetTest.php index 84b1e1071..09e697e86 100644 --- a/tests/Feature/GuestScheduledSeriesWidgetTest.php +++ b/tests/Feature/GuestScheduledSeriesWidgetTest.php @@ -25,6 +25,25 @@ function setGuestScheduledSeriesContext(Playlist $playlist, PlaylistAuth $auth): session()->put("{$prefix}guest_auth_password", $auth->password); } +/** + * Set up a "stale guest" session: credentials are present in the expected + * session keys (so getCurrentAuth() returns non-null), but they don't match + * any PlaylistAuth row (so getCurrentPlaylistAuth() returns null). This is + * the state a guest lands in when their PlaylistAuth is revoked/disabled + * mid-session while stale session credentials still exist — the exact + * scenario issue #1398 follow-up is about. getDvrSetting() must still + * resolve normally via the request attribute so the only path that can leak + * is the playlist_auth_id whereNull() coercion of the original fix. + */ +function setStaleGuestSeriesContext(Playlist $playlist): void +{ + request()->attributes->set('playlist_uuid', $playlist->uuid); + + $prefix = base64_encode($playlist->uuid).'_'; + session()->put("{$prefix}guest_auth_username", 'nonexistent_guest'); + session()->put("{$prefix}guest_auth_password", 'irrelevant_password'); +} + beforeEach(function () { config()->set('dvr.dvr_enabled', true); config()->set('proxy.proxy_integration_enabled', true); @@ -93,3 +112,69 @@ function setGuestScheduledSeriesContext(Playlist $playlist, PlaylistAuth $auth): expect($ids)->toBe([$ownRule->id]); }); + +// --- Stale-guest null-auth fail-open (issue #1398 follow-up) --- +// +// When a guest's PlaylistAuth is revoked/disabled while they still hold a +// session with credentials in the expected keys, getCurrentAuth() returns +// non-null but getCurrentPlaylistAuth() returns null. The merged fix for +// #1398 (scoping to playlist_auth_id) is correct for live guests, but the +// `?->id` fallback to whereNull() was turning that null into "show every +// series rule with playlist_auth_id = null" — i.e. the playlist owner's. +// The fix in getSeriesRules() must fail closed in this state. isOwnerAuth() +// must be allowed through, otherwise the legitimate playlist-owner login +// (which has no PlaylistAuth row) regresses. + +it('returns no series rules when getCurrentPlaylistAuth() resolves to null and the session is not owner-auth', function () { + $ownerRule = DvrRecordingRule::factory() + ->for($this->dvrSetting) + ->for($this->user) + ->create([ + 'type' => DvrRuleType::Series, + 'series_title' => 'Owner Show', + 'enabled' => true, + 'playlist_auth_id' => null, + ]); + + DvrRecordingRule::factory() + ->for($this->dvrSetting) + ->for($this->user) + ->create([ + 'type' => DvrRuleType::Series, + 'series_title' => 'Other Guest Show', + 'enabled' => true, + 'playlist_auth_id' => $this->guestB->id, + ]); + + setStaleGuestSeriesContext($this->playlist); + + // Sanity check on the precondition — if these stop holding the test no + // longer exercises the bug it's meant to. + expect(GuestScheduledSeriesWidget::getDvrSetting())->not->toBeNull() + ->and(GuestScheduledSeriesWidget::getCurrentPlaylistAuth())->toBeNull(); + + $widget = new GuestScheduledSeriesWidget; + + expect($widget->getSeriesRules())->toHaveCount(0); +}); + +it('returns no series rules when getCurrentPlaylistAuth() resolves to null even if the owner has series rules', function () { + // Specifically guard against leaking the owner's series rules, which is + // the exact privacy regression #1398 exists to prevent. + DvrRecordingRule::factory() + ->for($this->dvrSetting) + ->for($this->user) + ->create([ + 'type' => DvrRuleType::Series, + 'series_title' => 'Owner Show', + 'enabled' => true, + 'playlist_auth_id' => null, + ]); + + setStaleGuestSeriesContext($this->playlist); + + $widget = new GuestScheduledSeriesWidget; + $ids = $widget->getSeriesRules()->pluck('id')->all(); + + expect($ids)->toBe([]); +});