From 858b1906ec34e76950262e18135c0ecc5d22eb15 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 3 Jun 2026 09:33:46 +0200 Subject: [PATCH] Improve GitHub App setup flow --- app/Http/Controllers/Webhook/Github.php | 55 ++++++-- app/Livewire/Source/Github/Change.php | 2 + bootstrap/helpers/github.php | 22 ++-- .../livewire/source/github/change.blade.php | 7 +- tests/Feature/GithubSourceChangeTest.php | 99 +++++++++++++++ .../Security/GithubAppSetupCallbackTest.php | 119 +++++++++++++++++- 6 files changed, 276 insertions(+), 28 deletions(-) diff --git a/app/Http/Controllers/Webhook/Github.php b/app/Http/Controllers/Webhook/Github.php index 8e3ffcd7c..40c5cbdf0 100644 --- a/app/Http/Controllers/Webhook/Github.php +++ b/app/Http/Controllers/Webhook/Github.php @@ -11,6 +11,8 @@ use App\Models\GithubApp; use App\Models\PrivateKey; use Exception; +use Illuminate\Http\Exceptions\HttpResponseException; +use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Http; @@ -539,19 +541,22 @@ public function redirect(Request $request) public function install(Request $request) { - $source = (string) $request->query('source', ''); - abort_if(blank($source), 404); - - $github_app = GithubApp::ownedByCurrentTeam()->where('uuid', $source)->firstOrFail(); - $setup_action = (string) $request->query('setup_action', ''); - if ($setup_action !== 'install') { - return redirect()->route('source.github.show', ['github_app_uuid' => $github_app->uuid]); - } + abort_unless(in_array($setup_action, ['install', 'update'], true), 422, 'Invalid GitHub App setup action.'); $installation_id = (string) $request->query('installation_id', ''); abort_unless(ctype_digit($installation_id), 422, 'Missing GitHub App installation id.'); + if ($setup_action === 'update') { + return $this->redirectAfterGithubAppInstallationUpdate($installation_id); + } + + $github_app = $this->consumeGithubAppSetupState( + request: $request, + state: (string) $request->query('state', ''), + action: 'install', + ); + abort_unless( $this->githubInstallationBelongsToApp($github_app, $installation_id), 403, @@ -564,6 +569,19 @@ public function install(Request $request) return redirect()->route('source.github.show', ['github_app_uuid' => $github_app->uuid]); } + private function redirectAfterGithubAppInstallationUpdate(string $installation_id): RedirectResponse + { + $github_app = GithubApp::ownedByCurrentTeam() + ->where('installation_id', $installation_id) + ->first(); + + if ($github_app) { + return redirect()->route('source.github.show', ['github_app_uuid' => $github_app->uuid]); + } + + return redirect()->route('source.all'); + } + /** * Verify that the given installation id actually belongs to this GitHub App. * @@ -596,11 +614,14 @@ private function githubInstallationBelongsToApp(GithubApp $github_app, string $i private function consumeGithubAppSetupState(Request $request, string $state, string $action): GithubApp { - abort_if(blank($state), 404); + if (blank($state)) { + $this->rejectInvalidGithubAppSetupState($request); + } $payload = Cache::pull($this->githubAppSetupStateCacheKey($state)); - abort_unless(is_array($payload), 404); - abort_unless(data_get($payload, 'action') === $action, 404); + if (! is_array($payload) || data_get($payload, 'action') !== $action) { + $this->rejectInvalidGithubAppSetupState($request); + } $team_id = $request->user()?->currentTeam()?->id; abort_unless(! is_null($team_id) && (int) data_get($payload, 'team_id') === $team_id, 403); @@ -610,6 +631,18 @@ private function consumeGithubAppSetupState(Request $request, string $state, str ->firstOrFail(); } + private function rejectInvalidGithubAppSetupState(Request $request): never + { + if ($request->expectsJson()) { + abort(404); + } + + throw new HttpResponseException( + redirect() + ->route('source.all') + ); + } + private function githubAppSetupStateCacheKey(string $state): string { return 'github-app-setup-state:'.hash('sha256', $state); diff --git a/app/Livewire/Source/Github/Change.php b/app/Livewire/Source/Github/Change.php index 1470b95db..74ed812af 100644 --- a/app/Livewire/Source/Github/Change.php +++ b/app/Livewire/Source/Github/Change.php @@ -210,6 +210,8 @@ public function checkPermissions() GithubAppPermissionJob::dispatchSync($this->github_app); $this->github_app->refresh()->makeVisible('client_secret')->makeVisible('webhook_secret'); + $this->syncData(false); + $this->dispatch('success', 'Github App permissions updated.'); } catch (\Throwable $e) { // Provide better error message for unsupported key formats diff --git a/bootstrap/helpers/github.php b/bootstrap/helpers/github.php index 4a61960fb..bb819e4aa 100644 --- a/bootstrap/helpers/github.php +++ b/bootstrap/helpers/github.php @@ -4,6 +4,7 @@ use App\Models\GitlabApp; use Carbon\Carbon; use Carbon\CarbonImmutable; +use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Http; use Illuminate\Support\Str; use Lcobucci\JWT\Encoding\ChainedFormatter; @@ -20,7 +21,7 @@ function generateGithubToken(GithubApp $source, string $type) $timeDiff = abs($serverTime->diffInSeconds($githubTime)); if ($timeDiff > 50) { - throw new \Exception( + throw new Exception( 'System time is out of sync with GitHub API time:
'. '- System time: '.$serverTime->format('Y-m-d H:i:s').' UTC
'. '- GitHub time: '.$githubTime->format('Y-m-d H:i:s').' UTC
'. @@ -60,7 +61,7 @@ function generateGithubToken(GithubApp $source, string $type) return $response->json()['token']; })(), - default => throw new \InvalidArgumentException("Unsupported token type: {$type}") + default => throw new InvalidArgumentException("Unsupported token type: {$type}") }; } @@ -77,11 +78,11 @@ function generateGithubJwt(GithubApp $source) function githubApi(GithubApp|GitlabApp|null $source, string $endpoint, string $method = 'get', ?array $data = null, bool $throwError = true) { if (is_null($source)) { - throw new \Exception('Source is required for API calls'); + throw new Exception('Source is required for API calls'); } if ($source->getMorphClass() !== GithubApp::class) { - throw new \InvalidArgumentException("Unsupported source type: {$source->getMorphClass()}"); + throw new InvalidArgumentException("Unsupported source type: {$source->getMorphClass()}"); } if ($source->is_public) { @@ -100,7 +101,7 @@ function githubApi(GithubApp|GitlabApp|null $source, string $endpoint, string $m $errorMessage = data_get($response->json(), 'message', 'no error message found'); $remainingCalls = $response->header('X-RateLimit-Remaining', '0'); - throw new \Exception( + throw new Exception( 'GitHub API call failed:
'. "Error: {$errorMessage}
". 'Rate Limit Status:
'. @@ -116,13 +117,20 @@ function githubApi(GithubApp|GitlabApp|null $source, string $endpoint, string $m ]; } -function getInstallationPath(GithubApp $source) +function getInstallationPath(GithubApp $source): string { $github = GithubApp::where('uuid', $source->uuid)->first(); $name = str(Str::kebab($github->name)); $installation_path = $github->html_url === 'https://github.com' ? 'apps' : 'github-apps'; + $state = Str::random(64); - return "$github->html_url/$installation_path/$name/installations/new"; + Cache::put('github-app-setup-state:'.hash('sha256', $state), [ + 'action' => 'install', + 'github_app_id' => $github->id, + 'team_id' => $github->team_id, + ], now()->addMinutes(60)); + + return "$github->html_url/$installation_path/$name/installations/new?".http_build_query(['state' => $state]); } function getPermissionsPath(GithubApp $source) diff --git a/resources/views/livewire/source/github/change.blade.php b/resources/views/livewire/source/github/change.blade.php index d52e35646..dc9560a1f 100644 --- a/resources/views/livewire/source/github/change.blade.php +++ b/resources/views/livewire/source/github/change.blade.php @@ -351,9 +351,8 @@ class="px-2 py-1 text-xs font-bold uppercase tracking-wide bg-neutral-100 dark:b function createGithubApp(webhook_endpoint, use_custom_webhook_endpoint, custom_webhook_endpoint, preview_deployment_permissions, administration) { const { organization, - html_url, - uuid - } = @js($github_app->only(['organization', 'html_url', 'uuid'])); + html_url + } = @js($github_app->only(['organization', 'html_url'])); const selectedEndpoint = webhook_endpoint ? webhook_endpoint.trim() : ''; const customEndpoint = custom_webhook_endpoint ? custom_webhook_endpoint.trim() : ''; if (use_custom_webhook_endpoint && !customEndpoint) { @@ -401,7 +400,7 @@ function createGithubApp(webhook_endpoint, use_custom_webhook_endpoint, custom_w callback_urls: [`${baseUrl}/login/github/app`], public: false, request_oauth_on_install: false, - setup_url: `${webhookBaseUrl}/source/github/install?source=${uuid}`, + setup_url: `${webhookBaseUrl}/source/github/install`, setup_on_update: true, default_permissions, default_events diff --git a/tests/Feature/GithubSourceChangeTest.php b/tests/Feature/GithubSourceChangeTest.php index 91296dd0f..e6f758682 100644 --- a/tests/Feature/GithubSourceChangeTest.php +++ b/tests/Feature/GithubSourceChangeTest.php @@ -7,6 +7,8 @@ use App\Models\Team; use App\Models\User; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Http; use Livewire\Livewire; uses(RefreshDatabase::class); @@ -84,6 +86,44 @@ function validPrivateKey(): string ->assertSet('privateKeyId', null); }); + test('creates one-time states for manifest conversion and installation callbacks', function () { + $githubApp = GithubApp::create([ + 'name' => 'Test GitHub App', + 'api_url' => 'https://api.github.com', + 'html_url' => 'https://github.com', + 'custom_user' => 'git', + 'custom_port' => 22, + 'team_id' => $this->team->id, + 'is_system_wide' => false, + ]); + + $component = Livewire::withQueryParams(['github_app_uuid' => $githubApp->uuid]) + ->test(Change::class) + ->assertSuccessful(); + + $manifestState = $component->get('manifestState'); + $installationUrl = getInstallationPath($githubApp); + parse_str(parse_url($installationUrl, PHP_URL_QUERY), $query); + $installState = $query['state'] ?? null; + + expect($manifestState)->not->toBeEmpty() + ->and($installState)->not->toBeEmpty() + ->and($installState)->not->toBe($manifestState) + ->and($installationUrl)->not->toContain($githubApp->uuid) + ->and(Cache::get('github-app-setup-state:'.hash('sha256', $manifestState))) + ->toMatchArray([ + 'action' => 'manifest', + 'github_app_id' => $githubApp->id, + 'team_id' => $githubApp->team_id, + ]) + ->and(Cache::get('github-app-setup-state:'.hash('sha256', $installState))) + ->toMatchArray([ + 'action' => 'install', + 'github_app_id' => $githubApp->id, + 'team_id' => $githubApp->team_id, + ]); + }); + test('defaults webhook endpoint to app url when it is the first available endpoint', function () { config(['app.url' => 'http://localhost:8000']); @@ -305,4 +345,63 @@ function validPrivateKey(): string return str_contains($message, 'Private Key not found'); }); }); + + test('checkPermissions syncs refetched permissions into input fields', function () { + $privateKey = PrivateKey::create([ + 'name' => 'Test Key', + 'private_key' => validPrivateKey(), + 'team_id' => $this->team->id, + ]); + + $githubApp = GithubApp::create([ + 'name' => 'Test GitHub App', + 'api_url' => 'https://api.github.com', + 'html_url' => 'https://github.com', + 'custom_user' => 'git', + 'custom_port' => 22, + 'app_id' => 12345, + 'installation_id' => 67890, + 'client_id' => 'test-client-id', + 'client_secret' => 'test-client-secret', + 'webhook_secret' => 'test-webhook-secret', + 'private_key_id' => $privateKey->id, + 'team_id' => $this->team->id, + 'is_system_wide' => false, + 'contents' => null, + 'metadata' => null, + 'pull_requests' => null, + ]); + + Http::preventStrayRequests(); + Http::fake([ + 'https://api.github.com/zen' => Http::response('Keep it logically awesome.', 200, [ + 'date' => now()->toRfc7231String(), + ]), + 'https://api.github.com/app' => Http::response([ + 'permissions' => [ + 'contents' => 'read', + 'metadata' => 'read', + 'pull_requests' => 'write', + ], + ]), + ]); + + Livewire::withQueryParams(['github_app_uuid' => $githubApp->uuid]) + ->test(Change::class) + ->assertSuccessful() + ->assertSet('contents', null) + ->assertSet('metadata', null) + ->assertSet('pullRequests', null) + ->call('checkPermissions') + ->assertDispatched('success') + ->assertSet('contents', 'read') + ->assertSet('metadata', 'read') + ->assertSet('pullRequests', 'write'); + + $githubApp->refresh(); + + expect($githubApp->contents)->toBe('read') + ->and($githubApp->metadata)->toBe('read') + ->and($githubApp->pull_requests)->toBe('write'); + }); }); diff --git a/tests/Feature/Security/GithubAppSetupCallbackTest.php b/tests/Feature/Security/GithubAppSetupCallbackTest.php index 9c6893fd1..f56a77d5f 100644 --- a/tests/Feature/Security/GithubAppSetupCallbackTest.php +++ b/tests/Feature/Security/GithubAppSetupCallbackTest.php @@ -199,8 +199,9 @@ function fakeGithubInstallationVerificationFailure(): void it('requires authentication before processing github app install callbacks', function () { Http::preventStrayRequests(); + cacheGithubAppSetupState('valid-install-state', 'install', $this->githubApp); - $this->get('/webhooks/source/github/install?source='.$this->githubApp->uuid.'&setup_action=install&installation_id=123456') + $this->get('/webhooks/source/github/install?state=valid-install-state&setup_action=install&installation_id=123456') ->assertRedirect(); Http::assertNothingSent(); @@ -209,22 +210,110 @@ function fakeGithubInstallationVerificationFailure(): void expect($this->githubApp->installation_id)->toBeNull(); }); -it('rejects github app install callbacks for an unknown github app', function () { +it('rejects github app install callbacks with an app uuid as state', function () { authenticateGithubSetupCallbackTest($this); Http::preventStrayRequests(); - $this->withHeader('Accept', 'application/json')->get('/webhooks/source/github/install?source=does-not-exist&setup_action=install&installation_id=123456') + $this->withHeader('Accept', 'application/json')->get('/webhooks/source/github/install?state='.$this->githubApp->uuid.'&setup_action=install&installation_id=123456') ->assertNotFound(); Http::assertNothingSent(); }); +it('redirects browser github app install callbacks with missing or expired state to sources', function () { + authenticateGithubSetupCallbackTest($this); + Http::preventStrayRequests(); + + $this->get('/webhooks/source/github/install?setup_action=install&installation_id=123456') + ->assertRedirect(route('source.all')); + + $this->get('/webhooks/source/github/install?state=expired-state&setup_action=install&installation_id=123456') + ->assertRedirect(route('source.all')); + + Http::assertNothingSent(); +}); + +it('rejects github app setup states for the wrong callback action', function () { + authenticateGithubSetupCallbackTest($this); + Http::preventStrayRequests(); + cacheGithubAppSetupState('manifest-state', 'manifest', $this->githubApp); + cacheGithubAppSetupState('install-state', 'install', $this->githubApp); + + $this->withHeader('Accept', 'application/json')->get('/webhooks/source/github/install?state=manifest-state&setup_action=install&installation_id=123456') + ->assertNotFound(); + + $this->withHeader('Accept', 'application/json')->get('/webhooks/source/github/redirect?state=install-state&code=real-code') + ->assertNotFound(); + + Http::assertNothingSent(); +}); + +it('allows github app install callbacks for repository update setup actions', function () { + authenticateGithubSetupCallbackTest($this); + configureGithubAppCredentials($this->githubApp); + $this->githubApp->forceFill(['installation_id' => 111111])->save(); + Http::preventStrayRequests(); + + $this->get('/webhooks/source/github/install?setup_action=update&installation_id=111111') + ->assertRedirect(route('source.github.show', ['github_app_uuid' => $this->githubApp->uuid])); + + Http::assertNothingSent(); + + $this->githubApp->refresh(); + expect($this->githubApp->installation_id)->toBe(111111); +}); + +it('redirects github app repository update callbacks without a matching source to the sources page', function () { + authenticateGithubSetupCallbackTest($this); + Http::preventStrayRequests(); + + $this->get('/webhooks/source/github/install?setup_action=update&installation_id=123456') + ->assertRedirect(route('source.all')); + + Http::assertNothingSent(); +}); + +it('rejects github app install callbacks for unknown setup actions', function () { + authenticateGithubSetupCallbackTest($this); + Http::preventStrayRequests(); + cacheGithubAppSetupState('valid-install-state', 'install', $this->githubApp); + + $this->withHeader('Accept', 'application/json')->get('/webhooks/source/github/install?state=valid-install-state&setup_action=remove&installation_id=123456') + ->assertUnprocessable(); + + Http::assertNothingSent(); +}); + +it('rejects github app setup states from another team', function () { + authenticateGithubSetupCallbackTest($this); + Http::preventStrayRequests(); + + $otherTeam = Team::factory()->create(); + $otherGithubApp = GithubApp::create([ + 'name' => 'Other GitHub App', + 'api_url' => 'https://api.github.com', + 'html_url' => 'https://github.com', + 'custom_user' => 'git', + 'custom_port' => 22, + 'team_id' => $otherTeam->id, + 'is_system_wide' => false, + ]); + + cacheGithubAppSetupState('other-team-state', 'manifest', $otherGithubApp); + + $this->withHeader('Accept', 'application/json')->get('/webhooks/source/github/redirect?state=other-team-state&code=real-code') + ->assertForbidden(); + + Http::assertNothingSent(); +}); + it('rejects an installation id that github does not confirm belongs to the app', function () { authenticateGithubSetupCallbackTest($this); configureGithubAppCredentials($this->githubApp); fakeGithubInstallationVerificationFailure(); + cacheGithubAppSetupState('valid-install-state', 'install', $this->githubApp); - $this->withHeader('Accept', 'application/json')->get('/webhooks/source/github/install?source='.$this->githubApp->uuid.'&setup_action=install&installation_id=999999') + $this->withHeader('Accept', 'application/json')->get('/webhooks/source/github/install?state=valid-install-state&setup_action=install&installation_id=999999') ->assertForbidden(); $this->githubApp->refresh(); @@ -235,21 +324,39 @@ function fakeGithubInstallationVerificationFailure(): void authenticateGithubSetupCallbackTest($this); configureGithubAppCredentials($this->githubApp); fakeGithubInstallationVerification($this->githubApp->app_id); + cacheGithubAppSetupState('valid-install-state', 'install', $this->githubApp); - $this->get('/webhooks/source/github/install?source='.$this->githubApp->uuid.'&setup_action=install&installation_id=123456') + $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); }); +it('rejects replayed github app install states', function () { + authenticateGithubSetupCallbackTest($this); + configureGithubAppCredentials($this->githubApp); + fakeGithubInstallationVerification($this->githubApp->app_id); + cacheGithubAppSetupState('valid-install-state', 'install', $this->githubApp); + + $this->get('/webhooks/source/github/install?state=valid-install-state&setup_action=install&installation_id=123456') + ->assertRedirect(); + + $this->withHeader('Accept', 'application/json')->get('/webhooks/source/github/install?state=valid-install-state&setup_action=install&installation_id=123456') + ->assertNotFound(); + + $this->githubApp->refresh(); + expect($this->githubApp->installation_id)->toBe(123456); +}); + it('allows reinstalling an already configured github app installation id', function () { authenticateGithubSetupCallbackTest($this); configureGithubAppCredentials($this->githubApp); $this->githubApp->forceFill(['installation_id' => 111111])->save(); fakeGithubInstallationVerification($this->githubApp->app_id); + cacheGithubAppSetupState('valid-install-state', 'install', $this->githubApp); - $this->get('/webhooks/source/github/install?source='.$this->githubApp->uuid.'&setup_action=install&installation_id=222222') + $this->get('/webhooks/source/github/install?state=valid-install-state&setup_action=install&installation_id=222222') ->assertRedirect(route('source.github.show', ['github_app_uuid' => $this->githubApp->uuid])); $this->githubApp->refresh();