Skip to content
Merged
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
13 changes: 10 additions & 3 deletions app/Http/Controllers/ItemController.php
Original file line number Diff line number Diff line change
Expand Up @@ -328,12 +328,18 @@ public static function storelogic(Request $request, $id = null): Item
$config = json_encode($configObject);
}

$current_user = User::currentUser();
$request->merge([
'description' => $config,
'user_id' => $current_user->getId(),
]);

// Only assign ownership when creating; updates must keep the existing owner.
if ($id === null) {
$current_user = User::currentUser();
$request->merge([
'user_id' => $current_user->getId(),
]);
}

if ($request->input('appid') === 'null' || $request->input('appid') === null) {
$request->merge([
'class' => null,
Expand All @@ -348,7 +354,8 @@ public static function storelogic(Request $request, $id = null): Item
$item = Item::create($request->all());
} else {
$item = Item::find($id);
$item->update($request->all());
// Exclude user_id so an update can never reassign ownership
$item->update($request->except(['user_id']));
}

$item->parents()->sync($request->tags);
Expand Down
3 changes: 2 additions & 1 deletion app/Http/Controllers/TagController.php
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ public function update(Request $request, int $id): RedirectResponse
'url' => $slug,
]);

Item::find($id)->update($request->all());
// Exclude user_id so an update can never reassign ownership
Item::find($id)->update($request->except(['user_id']));

$route = route('dash', []);

Expand Down
6 changes: 6 additions & 0 deletions app/Http/Controllers/UserController.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace App\Http\Controllers;

use App\Item;
use App\User;
use Illuminate\Contracts\View\View;
use Illuminate\Http\RedirectResponse;
Expand Down Expand Up @@ -154,6 +155,11 @@ public function update(Request $request, User $user): RedirectResponse
public function destroy(User $user): RedirectResponse
{
if ($user->id !== 1) {
// Hard-delete this user's items (tiles and tags) so they don't become
// orphaned; item_tag pivot rows cascade via the existing FK. Shared
// items (user_id = 0) are left untouched.
Item::withoutGlobalScopes()->where('user_id', $user->id)->forceDelete();

$user->delete();
$route = route('dash', []);

Expand Down
4 changes: 3 additions & 1 deletion app/Item.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ protected static function boot(): void
static::addGlobalScope('user_id', function (Builder $builder) {
$current_user = User::currentUser();
if ($current_user) {
$builder->where('user_id', $current_user->getId())->orWhere('user_id', 0);
$builder->where(function ($query) use ($current_user) {
$query->where('user_id', $current_user->getId())->orWhere('user_id', 0);
});
} else {
$builder->where('user_id', 0);
}
Expand Down
5 changes: 3 additions & 2 deletions app/Providers/AppServiceProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,10 @@ public function setupDatabase(): void
$db_type = config()->get('database.default');

if ($db_type == 'sqlite') {
$db_file = database_path(env('DB_DATABASE', 'app.sqlite'));
$db_file = config()->get('database.connections.sqlite.database');
Log::debug('SQLite Database Path: ' . $db_file);
if (! is_file($db_file)) {
// Do not create a file for the in-memory database identifier.
if ($db_file !== ':memory:' && ! is_file($db_file)) {
touch($db_file);
}
}
Expand Down
6 changes: 5 additions & 1 deletion config/database.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@
'connections' => [
'sqlite' => [
'driver' => 'sqlite',
'database' => database_path(env('DB_DATABASE', 'app.sqlite')), // Make sure to use the correct path
// Use the correct path, but let the special in-memory identifier
// pass through untouched so tests can run against ':memory:'.
'database' => env('DB_DATABASE', 'app.sqlite') === ':memory:'
? ':memory:'
: database_path(env('DB_DATABASE', 'app.sqlite')),
'prefix' => '',
'foreign_key_constraints' => env('DB_FOREIGN_KEYS', true), // Enable foreign key constraints
],
Expand Down
46 changes: 46 additions & 0 deletions database/migrations/2026_07_09_120000_reassign_orphaned_items.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Support\Facades\DB;

return new class extends Migration
{
/**
* Run the migrations.
*
* Reassign items whose user_id references a user that no longer exists to a
* surviving user, so previously orphaned tiles and tags become visible again.
* user_id = 0 means "shared with all users" and is never treated as orphaned.
*
* The DB query builder is used directly (not the Item model) so the global
* scope and soft-delete constraints don't interfere.
*/
public function up(): void
{
// Prefer user id 1 if it still exists, otherwise the lowest existing id.
$target = DB::table('users')->where('id', 1)->value('id')
?? DB::table('users')->min('id');

// No users left: nothing to reassign to.
if ($target === null) {
return;
}

DB::table('items')
->where('user_id', '!=', 0)
->whereNotIn('user_id', function ($query) {
$query->select('id')->from('users');
})
->update(['user_id' => $target]);
}

/**
* Reverse the migrations.
*
* This is a data migration; the original ownership cannot be recovered.
*/
public function down(): void
{
//
}
};
4 changes: 2 additions & 2 deletions phpunit.xml
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
<env name="APP_ENV" value="testing"/>
<env name="BCRYPT_ROUNDS" value="4"/>
<env name="CACHE_STORE" value="array"/>
<!-- <env name="DB_CONNECTION" value="sqlite"/> -->
<!-- <env name="DB_DATABASE" value=":memory:"/> -->
<env name="DB_CONNECTION" value="sqlite"/>
<env name="DB_DATABASE" value=":memory:"/>
<env name="MAIL_MAILER" value="array"/>
<env name="PULSE_ENABLED" value="false"/>
<env name="QUEUE_CONNECTION" value="sync"/>
Expand Down
13 changes: 13 additions & 0 deletions tests/Feature/ItemExportTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use App\Item;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Illuminate\Support\Facades\Date;
use Illuminate\Support\Facades\DB;
use Tests\TestCase;

class ItemExportTest extends TestCase
Expand Down Expand Up @@ -39,6 +40,18 @@ public function test_returns_exactly_the_defined_fields(): void

public function test_exports_assigned_tag_titles_excluding_the_root_tag(): void
{
// Mirror the root/default dashboard row that production seeds (id 0),
// so the item_tag pivot's foreign key to items.id is satisfied on a
// fresh in-memory database.
DB::table('items')->insert([
'id' => 0,
'title' => 'app.dashboard',
'url' => '',
'type' => 1,
'user_id' => 0,
'pinned' => 0,
]);

$item = Item::factory()
->create([
'title' => 'Tagged Item',
Expand Down
132 changes: 132 additions & 0 deletions tests/Feature/ItemOwnershipTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
<?php

namespace Tests\Feature;

use App\Item;
use App\User;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Tests\TestCase;

class ItemOwnershipTest extends TestCase
{
use RefreshDatabase;

/**
* Create a passwordless user (so the "allowed" middleware lets the
* request through) and make it the current session user.
*/
private function actAsCurrentUser(array $attributes = []): User
{
$user = User::factory()->create(array_merge([
'password' => null,
'public_front' => 1,
], $attributes));

$this->withSession(['current_user' => $user]);

return $user;
}

public function test_creating_an_item_assigns_the_creator_as_owner(): void
{
$this->seed();

$creator = $this->actAsCurrentUser();

$response = $this->post('/items', [
'pinned' => 1,
'appid' => 'null',
'website' => null,
'title' => 'Owned Item',
'colour' => '#00f',
'url' => 'http://10.0.1.1',
'tags' => [0],
]);

$response->assertStatus(302);

$item = Item::withoutGlobalScopes()->where('title', 'Owned Item')->first();
$this->assertNotNull($item);
$this->assertSame($creator->id, (int) $item->user_id);
}

public function test_creating_an_item_ignores_a_crafted_user_id(): void
{
$this->seed();

$creator = $this->actAsCurrentUser();
$other = User::factory()->create();

$response = $this->post('/items', [
'pinned' => 1,
'appid' => 'null',
'title' => 'Crafted Owner Item',
'colour' => '#00f',
'url' => 'http://10.0.1.2',
'user_id' => $other->id, // attempt to create on behalf of another user
'tags' => [0],
]);

$response->assertStatus(302);

$item = Item::withoutGlobalScopes()->where('title', 'Crafted Owner Item')->first();
$this->assertNotNull($item);
$this->assertSame($creator->id, (int) $item->user_id);
}

public function test_updating_a_shared_item_does_not_change_its_owner(): void
{
$this->seed();

// Attacker is a different logged-in user.
$attacker = $this->actAsCurrentUser();

// A shared item (user_id = 0) is visible to every user.
$item = Item::factory()->create([
'title' => 'Shared Item',
'user_id' => 0,
]);

$response = $this->patch('/items/'.$item->id, [
'appid' => 'null',
'title' => 'Shared Item Edited',
'url' => 'http://example.test',
'user_id' => $attacker->id, // crafted mass-assignment attempt
'tags' => [0],
]);

$response->assertRedirect(route('dash'));

$fresh = Item::withoutGlobalScopes()->find($item->id);
// Ownership is unchanged despite the crafted user_id field...
$this->assertSame(0, (int) $fresh->user_id);
// ...but the rest of the edit still applied.
$this->assertSame('Shared Item Edited', $fresh->title);
}

public function test_updating_an_owned_item_does_not_change_its_owner(): void
{
$this->seed();

$owner = $this->actAsCurrentUser();

$item = Item::factory()->create([
'title' => 'Owned Item',
'user_id' => $owner->id,
]);

$response = $this->patch('/items/'.$item->id, [
'appid' => 'null',
'title' => 'Owned Item Edited',
'url' => 'http://example.test',
'user_id' => 999, // crafted mass-assignment attempt
'tags' => [0],
]);

$response->assertRedirect(route('dash'));

$fresh = Item::withoutGlobalScopes()->find($item->id);
$this->assertSame($owner->id, (int) $fresh->user_id);
$this->assertSame('Owned Item Edited', $fresh->title);
}
}
52 changes: 52 additions & 0 deletions tests/Feature/OrphanItemRecoveryTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

namespace Tests\Feature;

use App\Item;
use App\User;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Tests\TestCase;

class OrphanItemRecoveryTest extends TestCase
{
use RefreshDatabase;

/**
* RefreshDatabase already runs every migration up-front, so the orphans
* are created afterwards and the data migration is invoked directly.
*/
public function test_reassigns_orphaned_items_but_leaves_shared_and_valid_items(): void
{
$this->seed();

// A surviving, valid owner.
$validOwner = User::factory()->create();

// Orphan: user_id references a user that does not exist.
$orphan = Item::factory()->create([
'title' => 'Orphaned Item',
'user_id' => 999,
]);

// Shared items (user_id = 0) must never be reassigned.
$shared = Item::factory()->create([
'title' => 'Shared Item',
'user_id' => 0,
]);

// A validly-owned item must be left untouched.
$valid = Item::factory()->create([
'title' => 'Valid Item',
'user_id' => $validOwner->id,
]);

$migration = include database_path('migrations/2026_07_09_120000_reassign_orphaned_items.php');
$migration->up();

// Orphan reassigned to the surviving admin (id 1).
$this->assertSame(1, (int) Item::withoutGlobalScopes()->find($orphan->id)->user_id);
// Shared and valid rows unchanged.
$this->assertSame(0, (int) Item::withoutGlobalScopes()->find($shared->id)->user_id);
$this->assertSame($validOwner->id, (int) Item::withoutGlobalScopes()->find($valid->id)->user_id);
}
}
Loading
Loading