Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
13 changes: 13 additions & 0 deletions app/Filament/GuestPanel/Widgets/GuestScheduledSeriesWidget.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
84 changes: 84 additions & 0 deletions tests/Feature/GuestDvrRecordingResourceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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();
});
69 changes: 69 additions & 0 deletions tests/Feature/GuestDvrRuleResourceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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([]);
});
85 changes: 85 additions & 0 deletions tests/Feature/GuestScheduledSeriesWidgetTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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([]);
});
Loading