fix: stop caching raw Eloquent models to prevent incomplete-object 500s

Caching an Eloquent model in a Cache::remember closure could deserialize
into a __PHP_Incomplete_Class on read, throwing 'attempt to access a
property on an incomplete object' and returning a 500. This surfaced on
guest profile pages (ProfileController::buildProfile reading
$user->user->settings) and affected several other latent call sites.

Changes:
- ProfileController: cache a plain settings array instead of the
  UserSetting model; fall back to defaults when the settings row is missing
- StoryService::getById: fetch a live model instead of caching it
- InstanceService::getByDomain, CustomEmoji::scan: cache arrays
- Site/MobileController: cache Page data as an array via a shared
  ManagesCachedPages trait; update blade views to array access
- Add public-route smoke/regression tests covering the cache-read path
pull/6914/head
Your Name 4 weeks ago
parent 3be6dbf547
commit f0e951dcce

@ -0,0 +1,32 @@
<?php
namespace App\Http\Controllers\Concerns;
use App\Models\Page;
trait ManagesCachedPages
{
/**
* Fetch an active CMS Page as a plain array suitable for caching.
*
* Returning an array (rather than the Eloquent model) avoids caching a
* model instance, which can deserialize into a __PHP_Incomplete_Class and
* throw "attempt to access a property on an incomplete object".
*
* @return array{title: ?string, content: ?string, created_at: ?string}|null
*/
protected function cachedPage(string $slug): ?array
{
$page = Page::whereSlug($slug)->whereActive(true)->first();
if (! $page) {
return null;
}
return [
'title' => $page->title,
'content' => $page->content,
'created_at' => optional($page->created_at)->format('M d, Y'),
];
}
}

@ -2,19 +2,19 @@
namespace App\Http\Controllers;
use App\Models\Page;
use App\Http\Controllers\Concerns\ManagesCachedPages;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\View;
class MobileController extends Controller
{
use ManagesCachedPages;
public function terms(Request $request)
{
$page = Cache::remember('site:terms', now()->addDays(120), function () {
$slug = '/site/terms';
return Page::whereSlug($slug)->whereActive(true)->first();
return $this->cachedPage('/site/terms');
});
return View::make('mobile.terms')->with(compact('page'))->render();
@ -23,9 +23,7 @@ class MobileController extends Controller
public function privacy(Request $request)
{
$page = Cache::remember('site:privacy', now()->addDays(120), function () {
$slug = '/site/privacy';
return Page::whereSlug($slug)->whereActive(true)->first();
return $this->cachedPage('/site/privacy');
});
return View::make('mobile.privacy')->with(compact('page'))->render();

@ -76,7 +76,19 @@ class ProfileController extends Controller
$key = 'profile:settings:'.$user->id;
$ttl = now()->addHours(6);
$settings = Cache::remember($key, $ttl, function () use ($user) {
return $user->user->settings;
$s = optional($user->user)->settings;
return [
'crawlable' => $s->crawlable ?? true,
'following' => [
'count' => $s->show_profile_following_count ?? true,
'list' => $s->show_profile_following ?? false,
],
'followers' => [
'count' => $s->show_profile_follower_count ?? true,
'list' => $s->show_profile_followers ?? false,
],
];
});
if ($user->is_private == true) {
@ -89,17 +101,6 @@ class ProfileController extends Controller
$is_following = false;
$profile = $user;
$settings = [
'crawlable' => $settings->crawlable,
'following' => [
'count' => $settings->show_profile_following_count,
'list' => $settings->show_profile_following,
],
'followers' => [
'count' => $settings->show_profile_follower_count,
'list' => $settings->show_profile_followers,
],
];
if ($carousel) {
return view('profile.show_carousel', compact('profile', 'settings'));
@ -110,7 +111,19 @@ class ProfileController extends Controller
$key = 'profile:settings:'.$user->id;
$ttl = now()->addHours(6);
$settings = Cache::remember($key, $ttl, function () use ($user) {
return $user->user->settings;
$s = optional($user->user)->settings;
return [
'crawlable' => $s->crawlable ?? true,
'following' => [
'count' => $s->show_profile_following_count ?? true,
'list' => $s->show_profile_following ?? false,
],
'followers' => [
'count' => $s->show_profile_follower_count ?? true,
'list' => $s->show_profile_followers ?? false,
],
];
});
if ($user->is_private == true) {
@ -132,17 +145,6 @@ class ProfileController extends Controller
$is_admin = is_null($user->domain) ? $user->user->is_admin : false;
$profile = $user;
$settings = [
'crawlable' => $settings->crawlable,
'following' => [
'count' => $settings->show_profile_following_count,
'list' => $settings->show_profile_following,
],
'followers' => [
'count' => $settings->show_profile_follower_count,
'list' => $settings->show_profile_followers,
],
];
if ($carousel) {
return view('profile.show_carousel', compact('profile', 'settings'));
}

@ -2,6 +2,7 @@
namespace App\Http\Controllers;
use App\Http\Controllers\Concerns\ManagesCachedPages;
use App\Models\Page;
use App\Models\Profile;
use App\Models\User;
@ -18,6 +19,8 @@ use Illuminate\Support\Str;
class SiteController extends Controller
{
use ManagesCachedPages;
public function home(Request $request)
{
if ($request->user() !== null) {
@ -86,9 +89,7 @@ class SiteController extends Controller
public function privacy(Request $request)
{
$page = Cache::remember('site:privacy', now()->addDays(120), function () {
$slug = '/site/privacy';
return Page::whereSlug($slug)->whereActive(true)->first();
return $this->cachedPage('/site/privacy');
});
return View::make('site.privacy')->with(compact('page'))->render();
@ -97,9 +98,7 @@ class SiteController extends Controller
public function terms(Request $request)
{
$page = Cache::remember('site:terms', now()->addDays(120), function () {
$slug = '/site/terms';
return Page::whereSlug($slug)->whereActive(true)->first();
return $this->cachedPage('/site/terms');
});
return View::make('site.terms')->with(compact('page'))->render();
@ -171,9 +170,7 @@ class SiteController extends Controller
public function legalNotice(Request $request)
{
$page = Cache::remember('site:legal-notice', now()->addDays(120), function () {
$slug = '/site/legal-notice';
return Page::whereSlug($slug)->whereActive(true)->first();
return $this->cachedPage('/site/legal-notice');
});
abort_if(! $page, 404);

@ -27,20 +27,32 @@ class CustomEmoji extends Model
->matchAll(self::SCAN_RE)
->map(function ($match) use ($activitypub) {
$tag = Cache::remember(self::CACHE_KEY.$match, 14400, function () use ($match) {
return self::orderBy('id')->whereDisabled(false)->whereShortcode(':'.$match.':')->first();
$emoji = self::orderBy('id')->whereDisabled(false)->whereShortcode(':'.$match.':')->first();
if (! $emoji) {
return null;
}
return [
'id' => $emoji->id,
'shortcode' => $emoji->shortcode,
'media_path' => $emoji->media_path,
'updated_at' => optional($emoji->updated_at)->toAtomString(),
'disabled' => $emoji->disabled,
];
});
if ($tag) {
$url = url('/storage/'.$tag->media_path);
$url = url('/storage/'.$tag['media_path']);
if ($activitypub == true) {
$mediaType = Str::endsWith($url, '.png') ? 'image/png' : 'image/jpg';
return [
'id' => url('emojis/'.$tag->id),
'id' => url('emojis/'.$tag['id']),
'type' => 'Emoji',
'name' => $tag->shortcode,
'updated' => $tag->updated_at->toAtomString(),
'name' => $tag['shortcode'],
'updated' => $tag['updated_at'],
'icon' => [
'type' => 'Image',
'mediaType' => $mediaType,
@ -52,7 +64,7 @@ class CustomEmoji extends Model
'shortcode' => $match,
'url' => $url,
'static_url' => $url,
'visible_in_picker' => $tag->disabled == false,
'visible_in_picker' => $tag['disabled'] == false,
];
}
}

@ -34,7 +34,9 @@ class InstanceService
public static function getByDomain($domain)
{
return Cache::remember(self::CACHE_KEY_BY_DOMAIN.$domain, 3600, function () use ($domain) {
return Instance::whereDomain($domain)->first();
$instance = Instance::whereDomain($domain)->first();
return $instance ? $instance->toArray() : null;
});
}

@ -35,9 +35,7 @@ class StoryService
public static function getById($id)
{
return Cache::remember(self::STORY_KEY.'by-id:id-'.$id, 3600, function () use ($id) {
return Story::find($id);
});
return Story::find($id);
}
public static function delById($id)

@ -8,8 +8,8 @@
<div class="card shadow-none">
<div class="card-body p-md-5 text-justify mx-md-3">
@if($page && $page->content)
{!! $page->content !!}
@if($page && $page['content'])
{!! $page['content'] !!}
@else
<div class="terms">
<h5 class="font-weight-bold" id="1">1. What information do we collect?</h5>

@ -7,8 +7,8 @@
<p class="text-muted small">Last Updated: Sept 28, 2022</p>
<div class="card shadow-none">
<div class="card-body text-justify">
@if($page && $page->content)
{!! $page->content !!}
@if($page && $page['content'])
{!! $page['content'] !!}
@else
<div class="terms">
<h5 class="font-weight-bold">1. Terms</h5>

@ -3,11 +3,11 @@
@section('content')
<div class="container mt-5">
<div class="col-12">
<p class="font-weight-bold text-lighter text-uppercase">{{ $page->title ?? 'Legal Notice' }}</p>
<p class="font-weight-bold text-lighter text-uppercase">{{ $page['title'] ?? 'Legal Notice' }}</p>
<div class="card border shadow-none">
<div class="card-body p-md-5 text-justify mx-md-3" style="white-space: pre-line">
@if($page && $page->content)
{!! $page->content !!}
@if($page && $page['content'])
{!! $page['content'] !!}
@endif
</div>
</div>
@ -15,5 +15,5 @@
</div>
@endsection
@push('meta')
<meta property="og:description" content="{{ $page->title ?? 'Legal Notice' }}">
<meta property="og:description" content="{{ $page['title'] ?? 'Legal Notice' }}">
@endpush

@ -6,8 +6,8 @@
<p class="font-weight-bold text-lighter text-uppercase">Privacy Policy</p>
<div class="card border shadow-none">
<div class="card-body p-md-5 text-justify mx-md-3">
@if($page && $page->content)
{!! $page->content !!}
@if($page && $page['content'])
{!! $page['content'] !!}
@else
<div class="terms">
<h5 class="font-weight-bold" id="1">1. What information do we collect?</h5>

@ -6,8 +6,8 @@
<p class="font-weight-bold text-lighter text-uppercase">Terms of Use</p>
<div class="card border shadow-none">
<div class="card-body p-md-5 text-justify mx-md-3">
@if($page && $page->content)
{!! $page->content !!}
@if($page && $page['content'])
{!! $page['content'] !!}
@else
<div class="terms">
<h5 class="font-weight-bold">1. Terms</h5>

@ -1,5 +1,9 @@
<?php
use App\Models\Page;
use App\Models\Status;
use App\Models\User;
use App\Models\UserSetting;
use Illuminate\Foundation\Testing\RefreshDatabase;
/*
@ -71,3 +75,219 @@ describe('routes that require database', function () {
->assertJsonStructure(['client_id', 'client_secret']);
});
});
/*
|--------------------------------------------------------------------------
| Cached-model regression tests
|--------------------------------------------------------------------------
|
| These guard against a class of bug where an Eloquent model was cached
| directly (e.g. Cache::remember(..., fn () => $model)). On a cache read the
| model could deserialize into a __PHP_Incomplete_Class, throwing
| "attempt to access a property on an incomplete object" and returning a 500.
|
| The failure only surfaced on the SECOND request (the cache-read path), so
| every test here hits the route twice.
|
*/
describe('guest profile page (regression: cached UserSetting model)', function () {
uses(RefreshDatabase::class);
it('renders a public profile for an unauthenticated user', function () {
$user = User::factory()->create();
$user->refresh();
$this->get('/'.$user->username)
->assertStatus(200);
});
it('renders a public profile on a repeated request (cache-read path)', function () {
$user = User::factory()->create();
$user->refresh();
// First request populates the profile:settings cache.
$this->get('/'.$user->username)
->assertStatus(200);
// Second request reads from cache — this is where a cached Eloquent
// model previously deserialized as an incomplete object and 500'd.
$this->get('/'.$user->username)
->assertStatus(200);
});
it('renders a public profile when the user has no settings row', function () {
$user = User::factory()->create();
$user->refresh();
// Simulate a missing user_settings row; the controller must fall back
// to defaults instead of throwing on a null relation.
UserSetting::where('user_id', $user->id)->delete();
$this->get('/'.$user->username)->assertStatus(200);
$this->get('/'.$user->username)->assertStatus(200);
});
});
describe('static site pages (regression: cached Page model)', function () {
uses(RefreshDatabase::class);
it('loads terms of use twice', function () {
$this->get('/site/terms')->assertStatus(200);
$this->get('/site/terms')->assertStatus(200);
});
it('loads privacy policy twice', function () {
$this->get('/site/privacy')->assertStatus(200);
$this->get('/site/privacy')->assertStatus(200);
});
it('renders db-backed terms content twice', function () {
Page::create([
'slug' => '/site/terms',
'title' => 'Terms',
'content' => '<p>Custom terms content</p>',
'active' => true,
]);
$this->get('/site/terms')
->assertStatus(200)
->assertSee('Custom terms content', false);
$this->get('/site/terms')
->assertStatus(200)
->assertSee('Custom terms content', false);
});
it('loads legal notice twice when a page exists', function () {
Page::create([
'slug' => '/site/legal-notice',
'title' => 'Legal Notice',
'content' => '<p>Legal notice body</p>',
'active' => true,
]);
$this->get('/site/legal-notice')
->assertStatus(200)
->assertSee('Legal notice body', false);
$this->get('/site/legal-notice')
->assertStatus(200)
->assertSee('Legal notice body', false);
});
it('loads mobile terms and privacy twice', function () {
$this->get('/e/terms')->assertStatus(200);
$this->get('/e/terms')->assertStatus(200);
$this->get('/e/privacy')->assertStatus(200);
$this->get('/e/privacy')->assertStatus(200);
});
});
describe('static informational pages load for guests', function () {
test('help index loads', function () {
$this->get('/site/help')->assertStatus(200);
});
test('fediverse info page loads', function () {
$this->get('/site/fediverse')->assertStatus(200);
});
test('open source page loads', function () {
$this->get('/site/open-source')->assertStatus(200);
});
test('developer api page loads', function () {
$this->get('/site/developer-api')->assertStatus(200);
});
test('getting started kb page loads', function () {
$this->get('/site/kb/getting-started')->assertStatus(200);
});
test('what is the fediverse kb page loads', function () {
$this->get('/site/kb/what-is-the-fediverse')->assertStatus(200);
});
});
describe('community guidelines page (regression: cached page render)', function () {
uses(RefreshDatabase::class);
it('loads the fallback twice', function () {
// No Page row exists; the route caches the rendered view either way.
$this->get('/site/kb/community-guidelines')->assertStatus(200);
$this->get('/site/kb/community-guidelines')->assertStatus(200);
});
it('renders db-backed content twice', function () {
Page::create([
'slug' => '/site/kb/community-guidelines',
'title' => 'Community Guidelines',
'content' => '<p>Be excellent to each other</p>',
'active' => true,
]);
$this->get('/site/kb/community-guidelines')
->assertStatus(200)
->assertSee('Be excellent to each other', false);
$this->get('/site/kb/community-guidelines')
->assertStatus(200)
->assertSee('Be excellent to each other', false);
});
});
describe('public status page (regression: cached status/media services)', function () {
uses(RefreshDatabase::class);
it('renders a public post for a guest on repeated requests', function () {
$user = User::factory()->create();
$user->refresh();
$status = Status::factory()->create([
'profile_id' => $user->profile_id,
'type' => 'photo',
'scope' => 'public',
'visibility' => 'public',
'uri' => null,
]);
$url = "/p/{$user->username}/{$status->id}";
// Two requests: the second exercises the cache-read path in
// StatusService/MediaService.
$this->get($url)->assertStatus(200);
$this->get($url)->assertStatus(200);
});
});
describe('profile activitypub object (regression: cached AP object)', function () {
uses(RefreshDatabase::class);
beforeEach(function () {
// config_cache() falls back to config() when the DB-backed config
// cache is disabled, so setting these makes the test deterministic.
config([
'instance.enable_cc' => false,
'federation.activitypub.enabled' => true,
]);
});
it('returns activitypub json for a guest on repeated requests', function () {
$user = User::factory()->create();
$user->refresh();
$this->getJson('/users/'.$user->username, [
'Accept' => 'application/activity+json',
])
->assertStatus(200)
->assertHeader('Content-Type', 'application/activity+json');
// Second request reads the cached AP object.
$this->getJson('/users/'.$user->username, [
'Accept' => 'application/activity+json',
])
->assertStatus(200)
->assertHeader('Content-Type', 'application/activity+json');
});
});

Loading…
Cancel
Save