From 6f557cf17fdb960d515fcf8c5044a3b4b0d9cbfd Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 20 Jul 2026 23:20:08 +0200 Subject: [PATCH] fix(security): enforce GitLab App authorization parity with GitHub GitlabAppPolicy previously allowed any authenticated user to update, delete, and create GitLab sources. Align it with GithubAppPolicy, require Application create authorization on the private-repo wizard, and reject OAuth callbacks from non-admins so members cannot escalate privileges. --- app/Http/Controllers/Webhook/Gitlab.php | 5 + .../Project/New/GitlabPrivateRepository.php | 5 + app/Policies/GitlabAppPolicy.php | 37 +++- tests/Feature/GitlabAppAuthorizationTest.php | 92 ++++++++++ .../Feature/GitlabOAuthCallbackStateTest.php | 21 +++ tests/Unit/Policies/GitlabAppPolicyTest.php | 172 ++++++++++++++++++ 6 files changed, 326 insertions(+), 6 deletions(-) create mode 100644 tests/Feature/GitlabAppAuthorizationTest.php create mode 100644 tests/Unit/Policies/GitlabAppPolicyTest.php diff --git a/app/Http/Controllers/Webhook/Gitlab.php b/app/Http/Controllers/Webhook/Gitlab.php index 0a7efed99..9371d9d91 100644 --- a/app/Http/Controllers/Webhook/Gitlab.php +++ b/app/Http/Controllers/Webhook/Gitlab.php @@ -40,6 +40,11 @@ class Gitlab extends Controller $gitlabApp = GitlabApp::whereKey(data_get($payload, 'gitlab_app_id'))->firstOrFail(); + // Only users who may administer the source can complete OAuth and store tokens. + if (! $request->user()->can('update', $gitlabApp)) { + return redirect()->route('source.all')->with('error', 'You are not authorized to connect this GitLab App.'); + } + $baseUrl = rtrim($gitlabApp->html_url, '/'); $response = Http::asForm()->post("{$baseUrl}/oauth/token", [ diff --git a/app/Livewire/Project/New/GitlabPrivateRepository.php b/app/Livewire/Project/New/GitlabPrivateRepository.php index 798a0dc50..a0ff15304 100644 --- a/app/Livewire/Project/New/GitlabPrivateRepository.php +++ b/app/Livewire/Project/New/GitlabPrivateRepository.php @@ -7,11 +7,14 @@ use App\Models\GitlabApp; use App\Models\Project; use App\Rules\ValidGitBranch; use App\Support\ValidationPatterns; +use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Illuminate\Support\Facades\Route; use Livewire\Component; class GitlabPrivateRepository extends Component { + use AuthorizesRequests; + public $current_step = 'gitlab_apps'; public $gitlab_apps; @@ -158,6 +161,8 @@ class GitlabPrivateRepository extends Component public function submit() { try { + $this->authorize('create', Application::class); + $validator = validator([ 'selected_repository_path' => $this->selected_repository_path, 'selected_branch_name' => $this->selected_branch_name, diff --git a/app/Policies/GitlabAppPolicy.php b/app/Policies/GitlabAppPolicy.php index 596e7e15a..56861e89b 100644 --- a/app/Policies/GitlabAppPolicy.php +++ b/app/Policies/GitlabAppPolicy.php @@ -7,44 +7,69 @@ use App\Models\User; class GitlabAppPolicy { + /** + * Determine whether the user can view any models. + */ public function viewAny(User $user): bool { return true; } + /** + * Determine whether the user can view the model. + */ public function view(User $user, GitlabApp $gitlabApp): bool { - return true; + if ($gitlabApp->is_system_wide) { + return true; + } + + return $user->teams->contains('id', $gitlabApp->team_id); } + /** + * Determine whether the user can create models. + */ public function create(User $user): bool { - return true; + return $user->isAdmin(); } + /** + * Determine whether the user can update the model. + */ public function update(User $user, GitlabApp $gitlabApp): bool { if ($gitlabApp->is_system_wide) { - return true; + return $user->canAccessSystemResources(); } - return true; + return $user->isAdminOfTeam($gitlabApp->team_id); } + /** + * Determine whether the user can delete the model. + */ public function delete(User $user, GitlabApp $gitlabApp): bool { if ($gitlabApp->is_system_wide) { - return true; + return $user->canAccessSystemResources(); } - return true; + return $user->isAdminOfTeam($gitlabApp->team_id); } + /** + * Determine whether the user can restore the model. + */ public function restore(User $user, GitlabApp $gitlabApp): bool { return false; } + /** + * Determine whether the user can permanently delete the model. + */ public function forceDelete(User $user, GitlabApp $gitlabApp): bool { return false; diff --git a/tests/Feature/GitlabAppAuthorizationTest.php b/tests/Feature/GitlabAppAuthorizationTest.php new file mode 100644 index 000000000..7fc20d07f --- /dev/null +++ b/tests/Feature/GitlabAppAuthorizationTest.php @@ -0,0 +1,92 @@ +team = Team::factory()->create(); + $this->owner = User::factory()->create(); + $this->member = User::factory()->create(); + $this->team->members()->attach($this->owner->id, ['role' => 'owner']); + $this->team->members()->attach($this->member->id, ['role' => 'member']); + + InstanceSettings::forceCreate([ + 'id' => 0, + 'fqdn' => null, + 'public_ipv4' => null, + 'public_ipv6' => null, + ]); + + $this->gitlabApp = GitlabApp::create([ + 'name' => 'Self-hosted GitLab', + 'api_url' => 'https://gitlab.example.com/api/v4', + 'html_url' => 'https://gitlab.example.com', + 'custom_user' => 'git', + 'custom_port' => 22, + 'client_id' => 'client-id', + 'client_secret' => 'client-secret', + 'webhook_token' => 'secret-webhook-token', + 'access_token' => 'access-token', + 'refresh_token' => 'refresh-token', + 'expires_at' => time() + 3600, + 'redirect_uri' => 'https://coolify.example.com/webhooks/source/gitlab/redirect', + 'team_id' => $this->team->id, + 'is_system_wide' => false, + 'is_public' => false, + ]); +}); + +describe('GitLab App authorization', function () { + test('team member cannot update a gitlab app via instantSave', function () { + $this->actingAs($this->member); + session(['currentTeam' => $this->team]); + + Livewire::withQueryParams(['gitlab_app_uuid' => $this->gitlabApp->uuid]) + ->test(Change::class) + ->set('isSystemWide', true) + ->call('instantSave') + ->assertDispatched('error'); + + expect($this->gitlabApp->refresh()->is_system_wide)->toBeFalse(); + }); + + test('team owner can update a gitlab app via instantSave', function () { + $this->actingAs($this->owner); + session(['currentTeam' => $this->team]); + + Livewire::withQueryParams(['gitlab_app_uuid' => $this->gitlabApp->uuid]) + ->test(Change::class) + ->set('isSystemWide', true) + ->call('instantSave') + ->assertDispatched('success'); + + expect($this->gitlabApp->refresh()->is_system_wide)->toBeTrue(); + }); + + test('team member cannot create an application from a private gitlab repository', function () { + $this->actingAs($this->member); + session(['currentTeam' => $this->team]); + + $applicationsBefore = Application::count(); + + // Avoid setting selected_project_id — its updated* hook loads branches and is unrelated to this auth check. + Livewire::test(GitlabPrivateRepository::class, ['type' => 'private-gitlab-app']) + ->set('selected_repository_path', 'group/repo') + ->set('selected_branch_name', 'main') + ->set('selected_gitlab_app_id', $this->gitlabApp->id) + ->set('gitlab_app_id', $this->gitlabApp->id) + ->call('submit') + ->assertDispatched('error'); + + expect(Application::count())->toBe($applicationsBefore); + }); +}); diff --git a/tests/Feature/GitlabOAuthCallbackStateTest.php b/tests/Feature/GitlabOAuthCallbackStateTest.php index 2bd8e2aa8..c9d693dfe 100644 --- a/tests/Feature/GitlabOAuthCallbackStateTest.php +++ b/tests/Feature/GitlabOAuthCallbackStateTest.php @@ -105,4 +105,25 @@ describe('GitLab OAuth callback state validation', function () { $response->assertRedirect(route('login')); }); + + test('rejects a callback from a team member who cannot administer the source', function () { + Http::fake(); + + $member = User::factory()->create(); + $this->team->members()->attach($member->id, ['role' => 'member']); + $this->actingAs($member); + session(['currentTeam' => $this->team]); + + $state = 'member-state'; + Cache::put(GitlabSource::oauthStateCacheKey($state), [ + 'gitlab_app_id' => $this->gitlabApp->id, + 'team_id' => $this->team->id, + ], now()->addMinutes(60)); + + $response = $this->get('/webhooks/source/gitlab/redirect?code=any&state='.$state); + + $response->assertRedirect(route('source.all')); + Http::assertNothingSent(); + expect($this->gitlabApp->refresh()->access_token)->toBeNull(); + }); }); diff --git a/tests/Unit/Policies/GitlabAppPolicyTest.php b/tests/Unit/Policies/GitlabAppPolicyTest.php new file mode 100644 index 000000000..a2cd2a091 --- /dev/null +++ b/tests/Unit/Policies/GitlabAppPolicyTest.php @@ -0,0 +1,172 @@ +makePartial(); + + $policy = new GitlabAppPolicy; + expect($policy->viewAny($user))->toBeTrue(); +}); + +it('allows any user to view system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + + $model = mockGitlabApp(teamId: 1, isSystemWide: true); + + $policy = new GitlabAppPolicy; + expect($policy->view($user, $model))->toBeTrue(); +}); + +it('allows team member to view non-system-wide gitlab app', function () { + $teams = collect([ + (object) ['id' => 1, 'pivot' => (object) ['role' => 'member']], + ]); + + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->view($user, $model))->toBeTrue(); +}); + +it('denies non-team member to view non-system-wide gitlab app', function () { + $teams = collect([ + (object) ['id' => 2, 'pivot' => (object) ['role' => 'member']], + ]); + + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->view($user, $model))->toBeFalse(); +}); + +it('allows admin to create gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('isAdmin')->andReturn(true); + + $policy = new GitlabAppPolicy; + expect($policy->create($user))->toBeTrue(); +}); + +it('denies non-admin to create gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('isAdmin')->andReturn(false); + + $policy = new GitlabAppPolicy; + expect($policy->create($user))->toBeFalse(); +}); + +it('allows user with system access to update system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('canAccessSystemResources')->andReturn(true); + + $model = mockGitlabApp(teamId: 1, isSystemWide: true); + + $policy = new GitlabAppPolicy; + expect($policy->update($user, $model))->toBeTrue(); +}); + +it('denies user without system access to update system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('canAccessSystemResources')->andReturn(false); + + $model = mockGitlabApp(teamId: 1, isSystemWide: true); + + $policy = new GitlabAppPolicy; + expect($policy->update($user, $model))->toBeFalse(); +}); + +it('allows team admin to update non-system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('isAdminOfTeam')->with(1)->andReturn(true); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->update($user, $model))->toBeTrue(); +}); + +it('denies team member to update non-system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('isAdminOfTeam')->with(1)->andReturn(false); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->update($user, $model))->toBeFalse(); +}); + +it('allows user with system access to delete system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('canAccessSystemResources')->andReturn(true); + + $model = mockGitlabApp(teamId: 1, isSystemWide: true); + + $policy = new GitlabAppPolicy; + expect($policy->delete($user, $model))->toBeTrue(); +}); + +it('denies user without system access to delete system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('canAccessSystemResources')->andReturn(false); + + $model = mockGitlabApp(teamId: 1, isSystemWide: true); + + $policy = new GitlabAppPolicy; + expect($policy->delete($user, $model))->toBeFalse(); +}); + +it('allows team admin to delete non-system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('isAdminOfTeam')->with(1)->andReturn(true); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->delete($user, $model))->toBeTrue(); +}); + +it('denies team member to delete non-system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('isAdminOfTeam')->with(1)->andReturn(false); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->delete($user, $model))->toBeFalse(); +}); + +it('denies restore of gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->restore($user, $model))->toBeFalse(); +}); + +it('denies force delete of gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->forceDelete($user, $model))->toBeFalse(); +}); + +function mockGitlabApp(int $teamId, bool $isSystemWide): GitlabApp +{ + $gitlabApp = Mockery::mock(GitlabApp::class)->makePartial(); + $gitlabApp->team_id = $teamId; + $gitlabApp->is_system_wide = $isSystemWide; + + return $gitlabApp; +}