From b3256d4df14e21c6d8936d972f45cbc47e07cca4 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Sun, 29 Mar 2026 20:56:04 +0200 Subject: [PATCH] fix(security): harden model assignment and sensitive data handling Restrict mass-assignable attributes across user/team/redis models and switch privileged root/team creation paths to forceFill/forceCreate. Encrypt legacy ClickHouse admin passwords via migration and cast the correct ClickHouse password field as encrypted. Tighten API and runtime exposure by removing sensitive team fields from responses and sanitizing Git/compose error messages. Expand security-focused feature coverage for command-injection and mass assignment protections. --- app/Actions/Fortify/CreateNewUser.php | 3 +- app/Http/Controllers/Api/TeamController.php | 8 -- app/Models/Application.php | 4 +- app/Models/ServerSetting.php | 1 + app/Models/StandaloneClickhouse.php | 2 +- app/Models/StandaloneRedis.php | 1 - app/Models/Team.php | 3 +- app/Models/User.php | 11 +- ...pt_existing_clickhouse_admin_passwords.php | 39 ++++++ database/seeders/RootUserSeeder.php | 3 +- tests/Feature/GetLogsCommandInjectionTest.php | 120 +++++++++++++----- .../Feature/MassAssignmentProtectionTest.php | 18 ++- 12 files changed, 154 insertions(+), 59 deletions(-) create mode 100644 database/migrations/2026_03_29_000000_encrypt_existing_clickhouse_admin_passwords.php diff --git a/app/Actions/Fortify/CreateNewUser.php b/app/Actions/Fortify/CreateNewUser.php index 9f97dd0d4..7ea6a871e 100644 --- a/app/Actions/Fortify/CreateNewUser.php +++ b/app/Actions/Fortify/CreateNewUser.php @@ -37,12 +37,13 @@ public function create(array $input): User if (User::count() == 0) { // If this is the first user, make them the root user // Team is already created in the database/seeders/ProductionSeeder.php - $user = User::create([ + $user = (new User)->forceFill([ 'id' => 0, 'name' => $input['name'], 'email' => $input['email'], 'password' => Hash::make($input['password']), ]); + $user->save(); $team = $user->teams()->first(); // Disable registration after first user is created diff --git a/app/Http/Controllers/Api/TeamController.php b/app/Http/Controllers/Api/TeamController.php index fd0282d96..03b36e4e0 100644 --- a/app/Http/Controllers/Api/TeamController.php +++ b/app/Http/Controllers/Api/TeamController.php @@ -14,14 +14,6 @@ private function removeSensitiveData($team) 'custom_server_limit', 'pivot', ]); - if (request()->attributes->get('can_read_sensitive', false) === false) { - $team->makeHidden([ - 'smtp_username', - 'smtp_password', - 'resend_api_key', - 'telegram_token', - ]); - } return serializeApiResponse($team); } diff --git a/app/Models/Application.php b/app/Models/Application.php index a4789ae4a..3312f4c76 100644 --- a/app/Models/Application.php +++ b/app/Models/Application.php @@ -1767,7 +1767,7 @@ public function loadComposeFile($isInit = false, ?string $restoreBaseDirectory = $fileList = collect([".$workdir$composeFile"]); $gitRemoteStatus = $this->getGitRemoteStatus(deployment_uuid: $uuid); if (! $gitRemoteStatus['is_accessible']) { - throw new RuntimeException("Failed to read Git source:\n\n{$gitRemoteStatus['error']}"); + throw new RuntimeException('Failed to read Git source. Please verify repository access and try again.'); } $getGitVersion = instant_remote_process(['git --version'], $this->destination->server, false); $gitVersion = str($getGitVersion)->explode(' ')->last(); @@ -1825,7 +1825,7 @@ public function loadComposeFile($isInit = false, ?string $restoreBaseDirectory = } throw new RuntimeException('Repository does not exist. Please check your repository URL and try again.'); } - throw new RuntimeException($e->getMessage()); + throw new RuntimeException('Failed to read the Docker Compose file from the repository.'); } finally { // Cleanup only - restoration happens in catch block $commands = collect([ diff --git a/app/Models/ServerSetting.php b/app/Models/ServerSetting.php index 504cfa60a..efc7bc8de 100644 --- a/app/Models/ServerSetting.php +++ b/app/Models/ServerSetting.php @@ -56,6 +56,7 @@ class ServerSetting extends Model protected $guarded = []; protected $casts = [ + 'force_disabled' => 'boolean', 'force_docker_cleanup' => 'boolean', 'docker_cleanup_threshold' => 'integer', 'sentinel_token' => 'encrypted', diff --git a/app/Models/StandaloneClickhouse.php b/app/Models/StandaloneClickhouse.php index 74382d87c..c192e5360 100644 --- a/app/Models/StandaloneClickhouse.php +++ b/app/Models/StandaloneClickhouse.php @@ -42,7 +42,7 @@ class StandaloneClickhouse extends BaseModel protected $appends = ['internal_db_url', 'external_db_url', 'database_type', 'server_status']; protected $casts = [ - 'clickhouse_password' => 'encrypted', + 'clickhouse_admin_password' => 'encrypted', 'public_port_timeout' => 'integer', 'restart_count' => 'integer', 'last_restart_at' => 'datetime', diff --git a/app/Models/StandaloneRedis.php b/app/Models/StandaloneRedis.php index 812a0e5cb..2320619cf 100644 --- a/app/Models/StandaloneRedis.php +++ b/app/Models/StandaloneRedis.php @@ -16,7 +16,6 @@ class StandaloneRedis extends BaseModel protected $fillable = [ 'name', 'description', - 'redis_password', 'redis_conf', 'status', 'image', diff --git a/app/Models/Team.php b/app/Models/Team.php index 4b9751706..8eb8fa050 100644 --- a/app/Models/Team.php +++ b/app/Models/Team.php @@ -43,10 +43,9 @@ class Team extends Model implements SendsDiscord, SendsEmail, SendsPushover, Sen protected $fillable = [ 'name', 'description', + 'personal_team', 'show_boarding', 'custom_server_limit', - 'use_instance_email_settings', - 'resend_api_key', ]; protected $casts = [ diff --git a/app/Models/User.php b/app/Models/User.php index 6b6f93239..a62cb8358 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -49,9 +49,6 @@ class User extends Authenticatable implements SendsEmail 'password', 'force_password_reset', 'marketing_emails', - 'pending_email', - 'email_change_code', - 'email_change_code_expires_at', ]; protected $hidden = [ @@ -98,7 +95,7 @@ protected static function boot() $team['id'] = 0; $team['name'] = 'Root Team'; } - $new_team = Team::create($team); + $new_team = Team::forceCreate($team); $user->teams()->attach($new_team, ['role' => 'owner']); }); @@ -201,7 +198,7 @@ public function recreate_personal_team() $team['id'] = 0; $team['name'] = 'Root Team'; } - $new_team = Team::create($team); + $new_team = Team::forceCreate($team); $this->teams()->attach($new_team, ['role' => 'owner']); return $new_team; @@ -412,11 +409,11 @@ public function requestEmailChange(string $newEmail): void $expiryMinutes = config('constants.email_change.verification_code_expiry_minutes', 10); $expiresAt = Carbon::now()->addMinutes($expiryMinutes); - $this->update([ + $this->forceFill([ 'pending_email' => $newEmail, 'email_change_code' => $code, 'email_change_code_expires_at' => $expiresAt, - ]); + ])->save(); // Send verification email to new address $this->notify(new EmailChangeVerification($this, $code, $newEmail, $expiresAt)); diff --git a/database/migrations/2026_03_29_000000_encrypt_existing_clickhouse_admin_passwords.php b/database/migrations/2026_03_29_000000_encrypt_existing_clickhouse_admin_passwords.php new file mode 100644 index 000000000..a4a6988f2 --- /dev/null +++ b/database/migrations/2026_03_29_000000_encrypt_existing_clickhouse_admin_passwords.php @@ -0,0 +1,39 @@ +chunkById(100, function ($clickhouses) { + foreach ($clickhouses as $clickhouse) { + $password = $clickhouse->clickhouse_admin_password; + + if (empty($password)) { + continue; + } + + // Skip if already encrypted (idempotent) + try { + Crypt::decryptString($password); + + continue; + } catch (Exception) { + // Not encrypted yet — encrypt it + } + + DB::table('standalone_clickhouses') + ->where('id', $clickhouse->id) + ->update(['clickhouse_admin_password' => Crypt::encryptString($password)]); + } + }); + } catch (Exception $e) { + echo 'Encrypting ClickHouse admin passwords failed.'; + echo $e->getMessage(); + } + } +} diff --git a/database/seeders/RootUserSeeder.php b/database/seeders/RootUserSeeder.php index e3968a1c9..c4e93af63 100644 --- a/database/seeders/RootUserSeeder.php +++ b/database/seeders/RootUserSeeder.php @@ -45,12 +45,13 @@ public function run(): void } try { - User::create([ + $user = (new User)->forceFill([ 'id' => 0, 'name' => env('ROOT_USERNAME', 'Root User'), 'email' => env('ROOT_USER_EMAIL'), 'password' => Hash::make(env('ROOT_USER_PASSWORD')), ]); + $user->save(); echo "\n SUCCESS Root user created successfully.\n\n"; } catch (\Exception $e) { echo "\n ERROR Failed to create root user: {$e->getMessage()}\n\n"; diff --git a/tests/Feature/GetLogsCommandInjectionTest.php b/tests/Feature/GetLogsCommandInjectionTest.php index 34824b48b..3e5a33b66 100644 --- a/tests/Feature/GetLogsCommandInjectionTest.php +++ b/tests/Feature/GetLogsCommandInjectionTest.php @@ -1,8 +1,40 @@ user = User::factory()->create(); + $this->team = Team::factory()->create(); + $this->user->teams()->attach($this->team, ['role' => 'owner']); + + $this->server = Server::factory()->create(['team_id' => $this->team->id]); + // Server::created auto-creates a StandaloneDocker, reuse it + $this->destination = StandaloneDocker::where('server_id', $this->server->id)->first(); + $this->project = Project::factory()->create(['team_id' => $this->team->id]); + $this->environment = Environment::factory()->create(['project_id' => $this->project->id]); + + $this->application = Application::factory()->create([ + 'environment_id' => $this->environment->id, + 'destination_id' => $this->destination->id, + 'destination_type' => $this->destination->getMorphClass(), + ]); + + $this->actingAs($this->user); + session(['currentTeam' => $this->team]); +}); describe('GetLogs locked properties', function () { test('container property has Locked attribute', function () { @@ -34,47 +66,67 @@ }); }); -describe('GetLogs container name validation in getLogs', function () { - test('getLogs method validates container name with ValidationPatterns', function () { - $method = new ReflectionMethod(GetLogs::class, 'getLogs'); - $startLine = $method->getStartLine(); - $endLine = $method->getEndLine(); - $lines = array_slice(file($method->getFileName()), $startLine - 1, $endLine - $startLine + 1); - $methodBody = implode('', $lines); +describe('GetLogs Livewire action validation', function () { + test('getLogs rejects invalid container name', function () { + // Make server functional by setting settings directly + $this->server->settings->forceFill([ + 'is_reachable' => true, + 'is_usable' => true, + 'force_disabled' => false, + ])->save(); + // Reload server with fresh settings to ensure casted values + $server = Server::with('settings')->find($this->server->id); - expect($methodBody)->toContain('ValidationPatterns::isValidContainerName'); + Livewire::test(GetLogs::class, [ + 'server' => $server, + 'resource' => $this->application, + 'container' => 'container;malicious-command', + ]) + ->call('getLogs') + ->assertSet('outputs', 'Invalid container name.'); }); - test('downloadAllLogs method validates container name with ValidationPatterns', function () { - $method = new ReflectionMethod(GetLogs::class, 'downloadAllLogs'); - $startLine = $method->getStartLine(); - $endLine = $method->getEndLine(); - $lines = array_slice(file($method->getFileName()), $startLine - 1, $endLine - $startLine + 1); - $methodBody = implode('', $lines); + test('getLogs rejects unauthorized server access', function () { + $otherTeam = Team::factory()->create(); + $otherServer = Server::factory()->create(['team_id' => $otherTeam->id]); - expect($methodBody)->toContain('ValidationPatterns::isValidContainerName'); - }); -}); - -describe('GetLogs authorization checks', function () { - test('getLogs method checks server ownership via ownedByCurrentTeam', function () { - $method = new ReflectionMethod(GetLogs::class, 'getLogs'); - $startLine = $method->getStartLine(); - $endLine = $method->getEndLine(); - $lines = array_slice(file($method->getFileName()), $startLine - 1, $endLine - $startLine + 1); - $methodBody = implode('', $lines); - - expect($methodBody)->toContain('Server::ownedByCurrentTeam()'); + Livewire::test(GetLogs::class, [ + 'server' => $otherServer, + 'resource' => $this->application, + 'container' => 'test-container', + ]) + ->call('getLogs') + ->assertSet('outputs', 'Unauthorized.'); }); - test('downloadAllLogs method checks server ownership via ownedByCurrentTeam', function () { - $method = new ReflectionMethod(GetLogs::class, 'downloadAllLogs'); - $startLine = $method->getStartLine(); - $endLine = $method->getEndLine(); - $lines = array_slice(file($method->getFileName()), $startLine - 1, $endLine - $startLine + 1); - $methodBody = implode('', $lines); + test('downloadAllLogs returns empty for invalid container name', function () { + $this->server->settings->forceFill([ + 'is_reachable' => true, + 'is_usable' => true, + 'force_disabled' => false, + ])->save(); + $server = Server::with('settings')->find($this->server->id); - expect($methodBody)->toContain('Server::ownedByCurrentTeam()'); + Livewire::test(GetLogs::class, [ + 'server' => $server, + 'resource' => $this->application, + 'container' => 'container$(whoami)', + ]) + ->call('downloadAllLogs') + ->assertReturned(''); + }); + + test('downloadAllLogs returns empty for unauthorized server', function () { + $otherTeam = Team::factory()->create(); + $otherServer = Server::factory()->create(['team_id' => $otherTeam->id]); + + Livewire::test(GetLogs::class, [ + 'server' => $otherServer, + 'resource' => $this->application, + 'container' => 'test-container', + ]) + ->call('downloadAllLogs') + ->assertReturned(''); }); }); diff --git a/tests/Feature/MassAssignmentProtectionTest.php b/tests/Feature/MassAssignmentProtectionTest.php index f6518648f..18de67ce7 100644 --- a/tests/Feature/MassAssignmentProtectionTest.php +++ b/tests/Feature/MassAssignmentProtectionTest.php @@ -96,6 +96,9 @@ expect($user->isFillable('remember_token'))->toBeFalse('remember_token should not be fillable'); expect($user->isFillable('two_factor_secret'))->toBeFalse('two_factor_secret should not be fillable'); expect($user->isFillable('two_factor_recovery_codes'))->toBeFalse('two_factor_recovery_codes should not be fillable'); + expect($user->isFillable('pending_email'))->toBeFalse('pending_email should not be fillable'); + expect($user->isFillable('email_change_code'))->toBeFalse('email_change_code should not be fillable'); + expect($user->isFillable('email_change_code_expires_at'))->toBeFalse('email_change_code_expires_at should not be fillable'); }); test('User model allows mass assignment of profile fields', function () { @@ -110,7 +113,18 @@ $team = new Team; expect($team->isFillable('id'))->toBeFalse(); - expect($team->isFillable('personal_team'))->toBeFalse('personal_team should not be fillable'); + expect($team->isFillable('use_instance_email_settings'))->toBeFalse('use_instance_email_settings should not be fillable (migrated to EmailNotificationSettings)'); + expect($team->isFillable('resend_api_key'))->toBeFalse('resend_api_key should not be fillable (migrated to EmailNotificationSettings)'); + }); + + test('Team model allows mass assignment of expected fields', function () { + $team = new Team; + + expect($team->isFillable('name'))->toBeTrue(); + expect($team->isFillable('description'))->toBeTrue(); + expect($team->isFillable('personal_team'))->toBeTrue(); + expect($team->isFillable('show_boarding'))->toBeTrue(); + expect($team->isFillable('custom_server_limit'))->toBeTrue(); }); test('standalone database models block mass assignment of relationship IDs', function () { @@ -145,7 +159,7 @@ expect($model->isFillable('limits_memory'))->toBeTrue(); $model = new StandaloneRedis; - expect($model->isFillable('redis_password'))->toBeTrue(); + expect($model->isFillable('redis_conf'))->toBeTrue(); $model = new StandaloneMysql; expect($model->isFillable('mysql_root_password'))->toBeTrue();