From 6a0a4e889beee102d7f9c0af5741cfbf6c6b549f Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 9 Oct 2026 08:29:39 +0200 Subject: [PATCH] fix(github): show why the GitHub App install callback fails instead of a bare 403 The install callback swallowed every verification error (clock skew, network, missing key, GitHub API errors) and returned an opaque 403. It also compared the state's team with the session team, so switching teams during the GitHub install returned 403. - Return the verification reason, log it, and redirect to the GitHub App page with an error toast (JSON requests still get 403 with the reason) - Check membership of the state's team, and switch the session to it - Show flashed errors on the GitHub App page Fixes #12122 Co-Authored-By: Claude Opus 5.5 --- app/Http/Controllers/Webhook/Github.php | 62 ++++++++++++++----- .../livewire/source/github/change.blade.php | 4 ++ .../Security/GithubAppSetupCallbackTest.php | 51 +++++++++++++++ 3 files changed, 100 insertions(+), 17 deletions(-) diff --git a/app/Http/Controllers/Webhook/Github.php b/app/Http/Controllers/Webhook/Github.php index 009b11f299..ba270cf688 100644 --- a/app/Http/Controllers/Webhook/Github.php +++ b/app/Http/Controllers/Webhook/Github.php @@ -26,6 +26,7 @@ use Illuminate\Http\Request; use Illuminate\Support\Collection; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Http; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Str; class Github extends Controller @@ -747,11 +748,21 @@ class Github extends Controller action: 'install', ); - abort_unless( - $this->githubInstallationBelongsToApp($github_app, $installation_id), - 403, - 'GitHub App installation could not be verified.' - ); + $verification_error = $this->githubInstallationVerificationError($github_app, $installation_id); + if ($verification_error) { + Log::warning('GitHub App installation could not be verified.', [ + 'github_app_id' => $github_app->id, + 'installation_id' => $installation_id, + 'reason' => $verification_error, + ]); + + $message = "GitHub App installation could not be verified: {$verification_error}"; + abort_if($request->expectsJson(), 403, $message); + + return redirect() + ->route('source.github.show', ['github_app_uuid' => $github_app->uuid]) + ->with('error', $message); + } $github_app->installation_id = $installation_id; $github_app->save(); @@ -778,11 +789,13 @@ class Github extends Controller * The installation id arrives as an untrusted query parameter on an * unauthenticated-reachable GET callback, so it must be confirmed against * the GitHub API using the App's own credentials before it is persisted. + * + * @return string|null The reason the verification failed, or null when it passed. */ - private function githubInstallationBelongsToApp(GithubApp $github_app, string $installation_id): bool + private function githubInstallationVerificationError(GithubApp $github_app, string $installation_id): ?string { - if (blank($github_app->app_id) || blank($github_app->privateKey?->private_key)) { - return false; + if (blank($github_app->app_id) || blank(githubAppPrivateKey($github_app)?->private_key)) { + return 'The GitHub App ID or private key is missing.'; } try { @@ -794,12 +807,19 @@ class Github extends Controller ->timeout(10) ->connectTimeout(5) ->get("{$github_app->api_url}/app/installations/{$installation_id}"); - - return $response->successful() - && (string) data_get($response->json(), 'app_id') === (string) $github_app->app_id; - } catch (\Throwable) { - return false; + } catch (\Throwable $e) { + return strip_tags(str_replace('
', ' ', $e->getMessage())); } + + if (! $response->successful()) { + return "GitHub API returned HTTP {$response->status()}: ".data_get($response->json(), 'message', 'no error message found'); + } + + if ((string) data_get($response->json(), 'app_id') !== (string) $github_app->app_id) { + return 'The installation belongs to a different GitHub App.'; + } + + return null; } private function consumeGithubAppSetupState(Request $request, string $state, string $action): GithubApp @@ -813,12 +833,20 @@ class Github extends Controller $this->rejectInvalidGithubAppSetupState($request); } - $team_id = $request->user()?->currentTeam()?->id; - abort_unless(! is_null($team_id) && (int) data_get($payload, 'team_id') === $team_id, 403); + // The state is bound to the team that started the setup. The user may have switched + // teams while on GitHub, so check membership of that team, not the session team. + $team_id = (int) data_get($payload, 'team_id'); + abort_unless((bool) $request->user()?->teams->contains('id', $team_id), 403); - return GithubApp::whereKey(data_get($payload, 'github_app_id')) - ->where('team_id', data_get($payload, 'team_id')) + $github_app = GithubApp::whereKey(data_get($payload, 'github_app_id')) + ->where('team_id', $team_id) ->firstOrFail(); + + if ($request->user()->currentTeam()?->id !== $team_id) { + refreshSession($github_app->team); + } + + return $github_app; } private function rejectInvalidGithubAppSetupState(Request $request): never diff --git a/resources/views/livewire/source/github/change.blade.php b/resources/views/livewire/source/github/change.blade.php index dcebcfac14..252a2d4406 100644 --- a/resources/views/livewire/source/github/change.blade.php +++ b/resources/views/livewire/source/github/change.blade.php @@ -3,6 +3,10 @@ {{ $github_app->name ?: 'GitHub App' }} | Sources | Coolify + @if (session('error')) + + @endif + @if (data_get($github_app, 'app_id')) @php $githubAppRouteParameters = ['github_app_uuid' => $github_app->uuid]; diff --git a/tests/Feature/Security/GithubAppSetupCallbackTest.php b/tests/Feature/Security/GithubAppSetupCallbackTest.php index 69671f2c24..93ba0e533f 100644 --- a/tests/Feature/Security/GithubAppSetupCallbackTest.php +++ b/tests/Feature/Security/GithubAppSetupCallbackTest.php @@ -400,3 +400,54 @@ it('allows reinstalling an already configured github app installation id', funct $this->githubApp->refresh(); expect($this->githubApp->installation_id)->toBe(222222); }); + +it('redirects to the github app with the reason when github does not confirm the installation', function () { + authenticateGithubSetupCallbackTest($this); + configureGithubAppCredentials($this->githubApp); + fakeGithubInstallationVerificationFailure(); + cacheGithubAppSetupState('valid-install-state', 'install', $this->githubApp); + + $this->get('/webhooks/source/github/install?state=valid-install-state&setup_action=install&installation_id=999999') + ->assertRedirect(route('source.github.show', ['github_app_uuid' => $this->githubApp->uuid])) + ->assertSessionHas('error', fn (string $error) => str_contains($error, 'HTTP 404: Not Found')); + + $this->githubApp->refresh(); + expect($this->githubApp->installation_id)->toBeNull(); +}); + +it('redirects to the github app with the reason when the server clock is out of sync with github', function () { + authenticateGithubSetupCallbackTest($this); + configureGithubAppCredentials($this->githubApp); + Http::preventStrayRequests(); + Http::fake([ + 'https://api.github.com/zen' => Http::response('Keep it logically awesome.', 200, [ + 'Date' => now()->addMinutes(5)->toRfc7231String(), + ]), + ]); + cacheGithubAppSetupState('valid-install-state', 'install', $this->githubApp); + + $this->get('/webhooks/source/github/install?state=valid-install-state&setup_action=install&installation_id=123456') + ->assertRedirect(route('source.github.show', ['github_app_uuid' => $this->githubApp->uuid])) + ->assertSessionHas('error', fn (string $error) => str_contains($error, 'System time is out of sync with GitHub API time')); + + $this->githubApp->refresh(); + expect($this->githubApp->installation_id)->toBeNull(); +}); + +it('completes the github app installation after the user switched to another of their teams', function () { + authenticateGithubSetupCallbackTest($this); + configureGithubAppCredentials($this->githubApp); + fakeGithubInstallationVerification($this->githubApp->app_id); + cacheGithubAppSetupState('valid-install-state', 'install', $this->githubApp); + + $otherTeam = Team::factory()->create(); + $otherTeam->members()->attach($this->user->id, ['role' => 'owner']); + session(['currentTeam' => $otherTeam]); + + $this->get('/webhooks/source/github/install?state=valid-install-state&setup_action=install&installation_id=123456') + ->assertRedirect(route('source.github.show', ['github_app_uuid' => $this->githubApp->uuid])); + + $this->githubApp->refresh(); + expect($this->githubApp->installation_id)->toBe(123456) + ->and(data_get(session('currentTeam'), 'id'))->toBe($this->team->id); +});