diff --git a/app/Http/Controllers/ItemController.php b/app/Http/Controllers/ItemController.php index 462580d79..fe3df13ce 100644 --- a/app/Http/Controllers/ItemController.php +++ b/app/Http/Controllers/ItemController.php @@ -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, @@ -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); diff --git a/app/Http/Controllers/TagController.php b/app/Http/Controllers/TagController.php index 8aa5d40a6..299507c25 100644 --- a/app/Http/Controllers/TagController.php +++ b/app/Http/Controllers/TagController.php @@ -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', []); diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 6ac9249ad..ef9782e35 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -2,6 +2,7 @@ namespace App\Http\Controllers; +use App\Item; use App\User; use Illuminate\Contracts\View\View; use Illuminate\Http\RedirectResponse; @@ -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', []); diff --git a/app/Item.php b/app/Item.php index 719e50e71..c64208895 100644 --- a/app/Item.php +++ b/app/Item.php @@ -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); } diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 51a707915..186f85170 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -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); } } diff --git a/config/database.php b/config/database.php index 028c6bc86..5a21e148a 100644 --- a/config/database.php +++ b/config/database.php @@ -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 ], diff --git a/database/migrations/2026_07_09_120000_reassign_orphaned_items.php b/database/migrations/2026_07_09_120000_reassign_orphaned_items.php new file mode 100644 index 000000000..6c77b6f01 --- /dev/null +++ b/database/migrations/2026_07_09_120000_reassign_orphaned_items.php @@ -0,0 +1,46 @@ +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 + { + // + } +}; diff --git a/phpunit.xml b/phpunit.xml index 909feb8dc..348ed5931 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -12,8 +12,8 @@ - - + + diff --git a/tests/Feature/ItemExportTest.php b/tests/Feature/ItemExportTest.php index 51b9d0c33..58c251793 100644 --- a/tests/Feature/ItemExportTest.php +++ b/tests/Feature/ItemExportTest.php @@ -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 @@ -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', diff --git a/tests/Feature/ItemOwnershipTest.php b/tests/Feature/ItemOwnershipTest.php new file mode 100644 index 000000000..2d7d8191f --- /dev/null +++ b/tests/Feature/ItemOwnershipTest.php @@ -0,0 +1,132 @@ +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); + } +} diff --git a/tests/Feature/OrphanItemRecoveryTest.php b/tests/Feature/OrphanItemRecoveryTest.php new file mode 100644 index 000000000..9a7625de4 --- /dev/null +++ b/tests/Feature/OrphanItemRecoveryTest.php @@ -0,0 +1,52 @@ +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); + } +} diff --git a/tests/Feature/UserDeleteItemsTest.php b/tests/Feature/UserDeleteItemsTest.php new file mode 100644 index 000000000..a7e25158f --- /dev/null +++ b/tests/Feature/UserDeleteItemsTest.php @@ -0,0 +1,79 @@ +seed(); + + // Admin (id 1, passwordless) is the only user allowed to manage users. + $this->withSession(['current_user' => User::find(1)]); + + $victim = User::factory()->create(); + + // The victim's own tile and tag should both be hard-deleted. + $victimTile = Item::factory()->create([ + 'title' => 'Victim Tile', + 'type' => 0, + 'user_id' => $victim->id, + ]); + $victimTag = Item::factory()->create([ + 'title' => 'Victim Tag', + 'type' => 1, + 'user_id' => $victim->id, + ]); + + // A shared item and another user's item must be left untouched. + $sharedItem = Item::factory()->create([ + 'title' => 'Shared Item', + 'type' => 0, + 'user_id' => 0, + ]); + $adminItem = Item::factory()->create([ + 'title' => 'Admin Item', + 'type' => 0, + 'user_id' => 1, + ]); + + $response = $this->delete(route('users.destroy', $victim->id)); + + $response->assertRedirect(route('dash')); + + // Victim and their items are gone entirely (force-deleted, not soft-deleted). + $this->assertDatabaseMissing('users', ['id' => $victim->id]); + $this->assertNull(Item::withoutGlobalScopes()->withTrashed()->find($victimTile->id)); + $this->assertNull(Item::withoutGlobalScopes()->withTrashed()->find($victimTag->id)); + + // Shared and other users' items survive. + $this->assertNotNull(Item::withoutGlobalScopes()->find($sharedItem->id)); + $this->assertNotNull(Item::withoutGlobalScopes()->find($adminItem->id)); + } + + public function test_user_id_one_cannot_be_deleted(): void + { + $this->seed(); + + $this->withSession(['current_user' => User::find(1)]); + + $adminItem = Item::factory()->create([ + 'title' => 'Admin Item', + 'type' => 0, + 'user_id' => 1, + ]); + + $this->delete(route('users.destroy', 1)); + + // The admin and their items remain. + $this->assertDatabaseHas('users', ['id' => 1]); + $this->assertNotNull(Item::withoutGlobalScopes()->find($adminItem->id)); + } +} diff --git a/tests/TestCase.php b/tests/TestCase.php index fe1ffc2ff..98e540cd8 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -6,5 +6,42 @@ abstract class TestCase extends BaseTestCase { - // + protected function setUp(): void + { + parent::setUp(); + + $this->guardAgainstRealDatabase(); + } + + /** + * Refuse to run the test suite against anything other than an + * in-memory SQLite database. + * + * The suite uses RefreshDatabase (migrate:fresh), which would wipe + * whatever database it is pointed at. The only supported test + * configuration for this repo is sqlite + ':memory:'. Any other + * connection (a real sqlite file, mysql, pgsql, ...) is rejected so + * we can never destroy real data. + */ + private function guardAgainstRealDatabase(): void + { + $default = config('database.default'); + $driver = config("database.connections.{$default}.driver"); + $database = config("database.connections.{$default}.database"); + + if ($driver === 'sqlite' && $database === ':memory:') { + return; + } + + throw new \RuntimeException(sprintf( + 'Refusing to run tests: the default database connection (%s) is ' + . 'driver "%s" pointing at "%s". Tests only run against an ' + . 'in-memory SQLite database (driver "sqlite", database ":memory:") ' + . 'to avoid wiping real data via RefreshDatabase. Check phpunit.xml ' + . 'DB_CONNECTION/DB_DATABASE overrides.', + $default, + $driver, + is_scalar($database) ? (string) $database : gettype($database) + )); + } }